[crmsh-4.6] Fix: Don't add time units to values for existing CIB (bsc#1228817)

[liangxin1300]

Problem

Commit 7b2cfb2 automatically appends 's' as the default time unit for timeout and interval. This causes inconsistency in the existing CIB.

How to reproduce

1. Run crm configure primitive vip IPaddr2 params ip=10.10.10.123 op monitor interval=10
2. Run crm configure show vip, we can see interval=10s

   primitive vip IPaddr2 \
       params ip=10.10.10.123 \
       op monitor interval=10s timeout=20s \
       op start timeout=20s interval=0s \
       op stop timeout=20s interval=0s
   

3. Run crm configure edit xml vip, change interval=10s as interval=10, then run crm configure show vip

   xml <primitive id="vip" class="ocf" provider="heartbeat" type="IPaddr2"> \
       <instance_attributes id="vip-instance_attributes"> \
         <nvpair name="ip" value="10.10.10.123" id="vip-instance_attributes-ip"/> \
       </instance_attributes> \
       <operations> \
         <op name="monitor" interval="10" timeout="20s" id="vip-monitor-10s"/> \
         <op name="start" timeout="20s" interval="0s" id="vip-start-0s"/> \
         <op name="stop" timeout="20s" interval="0s" id="vip-stop-0s"/> \
       </operations> \
   </primitive>
   

   The above means for those old existing CIB which time value didn't have time unit, crmsh still tries to add the time unit on it, which causes CIB inconsistency

Root cause

crmsh will compare parsed cib(auto add time unit 's') with original cib(without time unit 's'), if not equal, it will show the XML directly

Fix

This commit renames the variable to reflect that it handles both adding advised operation values and time units, but only when the input is from a new(see above reproduce step 1) CLI command. Don't add time units to values for existing CIB.

# crm configure show vip
primitive vip IPaddr2 \
        params ip=10.10.10.123 \
        op monitor interval=10 timeout=20s \
        op start timeout=20s interval=0s \
        op stop timeout=20s interval=0s

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.65%. Comparing base (5a3c45a) to head (0c2dde6).
Report is 11 commits behind head on crmsh-4.6.

Additional details and impacted files

Flag	Coverage Δ
integration	53.54% <90.90%> (+0.30%)	⬆️
unit	49.83% <90.90%> (-0.05%)	⬇️

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

Files	Coverage Δ
crmsh/cibconfig.py	64.20% <100.00%> (+0.89%)	⬆️
crmsh/parse.py	85.71% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

[nicholasyang2022]

Please refine your pull request description. From it I cannot get why parsing cli parameters affects the behavior of crm configure show.

[nicholasyang2022]

This implementation is hard to understand. It depends on if isinstance(cli, str) at cibconfig.py:865 to call parse(..., auto_add=False).

> /usr/lib/python3.11/site-packages/crmsh/cibconfig.py(865)parse_cli_to_xml()
-> node = parse.parse(s, comments=comments)
(Pdb) list
860  	    default_promotable_meta = False
861  	    comments = []
862  	    if isinstance(cli, str):
863  	        utils.auto_convert_role = False
864  	        for s in lines2cli(cli):
865  ->	            node = parse.parse(s, comments=comments)
866  	    else:  # should be a pre-tokenized list
867  	        utils.auto_convert_role = True
868  	        if len(cli) >= 3 and cli[0] == "primitive" and cli[2].startswith("@"):
869  	            auto_add = False
870  	            default_promotable_meta = False

When calling parse_cli_to_xml, there are not any explicit parameters to tell the difference between: "Oh, we need to add advised operations and convert 600 to 600s as this is a user input." and "Oh, we should not do these as this is loaded from CIB". What parse_cli_to_xml will do depends on whether the cli input is tokenized. This too implicit and hard to understand and unmaintainable.

[liangxin1300]

This implementation is hard to understand. It depends on if isinstance(cli, str) at cibconfig.py:865 to call parse(..., auto_add=False).

> /usr/lib/python3.11/site-packages/crmsh/cibconfig.py(865)parse_cli_to_xml()
-> node = parse.parse(s, comments=comments)
(Pdb) list
860  	    default_promotable_meta = False
861  	    comments = []
862  	    if isinstance(cli, str):
863  	        utils.auto_convert_role = False
864  	        for s in lines2cli(cli):
865  ->	            node = parse.parse(s, comments=comments)
866  	    else:  # should be a pre-tokenized list
867  	        utils.auto_convert_role = True
868  	        if len(cli) >= 3 and cli[0] == "primitive" and cli[2].startswith("@"):
869  	            auto_add = False
870  	            default_promotable_meta = False

When calling parse_cli_to_xml, there are not any explicit parameters to tell the difference between: "Oh, we need to add advised operations and convert 600 to 600s as this is a user input." and "Oh, we should not do these as this is loaded from CIB". What parse_cli_to_xml will do depends on whether the cli input is tokenized. This too implicit and hard to understand and unmaintainable.

Good point. Thanks!
But I don't have much time to do the refactoring in recent. I opened a new issue for tracking #1517
And append a behave test case to make sure we don't introduce regression for future refactoring

If the customer does have positive feedback, I think we can move on this time