Fix: ra: Prevent to add unknown operation (bsc#1236442)

[liangxin1300]

• When adding unknown operation, sanity check should give an error instead of a warning
• And sanity check will return a big number to indicate this is a fatal error, so that the commit process will be aborted

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.74%. Comparing base (e63ac0b) to head (eb8e9fc).

Files with missing lines	Patch %	Lines
crmsh/utils.py	0.00%	1 Missing ⚠️

Additional details and impacted files

Flag	Coverage Δ
integration	53.56% <90.00%> (+0.03%)	⬆️
unit	53.07% <10.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crmsh/constants.py	100.00% <100.00%> (ø)
crmsh/ra.py	53.45% <100.00%> (+0.46%)	⬆️
crmsh/ui_configure.py	45.05% <100.00%> (+0.31%)	⬆️
crmsh/utils.py	66.49% <0.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nicholasyang2022]

I think reset() should not be called here.

1. _verify() is also called from do_verify(). This subcommand should not have a side effect.
2. Even when called from _commit(), it is enough to abort the commit, so that users will have a chance to modify their config instead of losing all the changes.

[liangxin1300]

Changed as raising utils.TerminateSubCommand

• non-interactive case:

alp-1:~ # crm configure primitive stateful-1 ocf:pacemaker:Stateful op monitor_Slave interval=10s op monitor_Master interval=5s
WARNING: (unpack_config)        warning: Blind faith: not fencing unseen nodes
ERROR: Action 'monitor_Slave' not found in Resource Agent meta-data
ERROR: Action 'monitor_Master' not found in Resource Agent meta-data
alp-1:~ # crm configure show stateful-1
ERROR: object stateful-1 does not exist

• interactive case:

alp-1:~ # crm
crm(live/alp-1)# configure
crm(live/alp-1)configure# primitive stateful-1 ocf:pacemaker:Stateful op monitor_Slave interval=10s op monitor_Master interval=5s
crm(live/alp-1)configure# primitive d2 Dummy

# terminate the commit, but the temp changes are still there
crm(live/alp-1)configure# commit
WARNING: (unpack_config)        warning: Blind faith: not fencing unseen nodes
ERROR: Action 'monitor_Slave' not found in Resource Agent meta-data
ERROR: Action 'monitor_Master' not found in Resource Agent meta-data
crm(live/alp-1)configure# show stateful-1
primitive stateful-1 ocf:pacemaker:Stateful \
        op monitor_Slave interval=10s \
        op monitor_Master interval=5s
crm(live/alp-1)configure# show d2
primitive d2 Dummy

# verify will raise error, without side effect
crm(live/alp-1)configure# verify
WARNING: (unpack_config)        warning: Blind faith: not fencing unseen nodes
ERROR: Action 'monitor_Slave' not found in Resource Agent meta-data
ERROR: Action 'monitor_Master' not found in Resource Agent meta-data

[nicholasyang2022]

Raising TerminateSubcommand looks like a hack. Why not just return False in _verify()? Does this unknown operation verification has any difference with other verifications?

[liangxin1300]

Based on the current logic, if the function returns False, the changes can still be forcefully committed or committed after user confirmation by asking, "Do you still want to commit?"

Currently, some invalid configurations are allowed to be committed into CIB, such as missing a required parameter for an RA.

However, allowing unknown operations into CIB can lead to high CPU usage (see bsc#1236442), which is why an exception is raised in these cases.

Perhaps here we should have two different handling policies for fatal and non-fatal cases. Fatal cases must terminate the process, whereas non-fatal cases can be forcefully committed after user confirmation.

[nicholasyang2022]

So, it will be better to use different return values in _verify() to represent fatal and non-fatal errors, and add some comments to note why this is a fatal error.