Improve crosync configure #80

liangxin1300 · 2021-03-14T14:15:14Z

To address these issues

Parse multi interface/node as list in the dict
When there is no space between words in corosync.conf, like "totem{" or "ring0_addr:10.10.10.121",
corosync still works, but our parse results are wrong

liangxin1300 · 2021-04-02T03:05:53Z

salt/states/crmshmod.py

@@ -298,11 +299,16 @@ def cluster_configured(
        return ret


-def _convert2dict(file_content_lines):
+DUMMY_SEC_NAME_TEMPL = "__{}_list"
+KNOWN_SEC_NAMES_WITH_LIST = ("totem.interface", "nodelist.node")


Predefined the known section names possible with multi items

liangxin1300 · 2021-04-06T03:23:30Z

salt/states/crmshmod.py

-            output += '{}}}\n'.format(indentation)
-        else:
-            output += '{}{}: {}\n'.format(indentation, key, value)
+        output += _unpack_dict(key, value, indentation)


Refactor this part to pass the codeclimate checking

Nice touch!

liangxin1300 · 2021-04-06T04:54:51Z

salt/states/crmshmod.py

        if key in main_dict.keys() and not isinstance(value, dict):
            if str(main_dict[key]) != str(value):
                applied_changes[current_path] = value
            main_dict[key] = value
        elif key in main_dict.keys():
-            modified_dict, new_changes = _mergedicts(main_dict[key], value, applied_changes, current_path)
-            main_dict[key] = modified_dict
-            applied_changes.update(new_changes)


From observing results, seems no need to update main_dict and applied_changes here.

liangxin1300 · 2021-04-06T07:54:56Z

@arbulu89 Please review this, thank you!

arbulu89

@liangxin1300
The change looks good (even though it is quite complex hehe, but I understand that this complexity is needed).
I have some some superficial comments, so feel free to change or not.
Really nice job!

arbulu89 · 2021-04-06T13:05:04Z

salt/states/crmshmod.py

-            corodict[line_items[0][:-1]] = line_items[-1]
+            initial_path = re.sub("\.{}".format(sec_name), "", initial_path) if "." in initial_path else ""
+        elif ':' in stripped_line:
+            key, *values = stripped_line.split(':')


I don't really like this. I guess you do this packing to filter the IP like values. Is there some better way to find those options? Like some regular expression. It looks pretty strange otherwise (it took me some time to understand what was going on here)

Maybe we could add a comment here to explain the purpose
Regular expression might more prone to errors

arbulu89 · 2021-04-06T13:07:31Z

salt/states/crmshmod.py

+            initial_path += ".{}".format(sec_name) if initial_path else sec_name
+            sub_dict, new_index = _convert2dict(file_content_lines[i+1:], initial_path)
+            if initial_path in KNOWN_SEC_NAMES_WITH_LIST:
+                dummpy_sec_name = DUMMY_SEC_NAME_TEMPL.format(sec_name)


I don't really like the DUMMY term. This value is actually a modified value, right?
Maybe better modified_sec_name ?

arbulu89 · 2021-04-06T13:09:13Z

salt/states/crmshmod.py

-            output += '{}}}\n'.format(indentation)
-        else:
-            output += '{}{}: {}\n'.format(indentation, key, value)
+        output += _unpack_dict(key, value, indentation)


Nice touch!

liangxin1300 · 2021-04-07T01:47:32Z

@liangxin1300
The change looks good (even though it is quite complex hehe, but I understand that this complexity is needed).
I have some some superficial comments, so feel free to change or not.
Really nice job!

@arbulu89 Thanks for review!
BTW, we could discuss how to reuse this part of codes between crmsh and this project.
I mean, current configuration parser in crmsh/corosync.py cannot support future corosync3 well, and also has the issue to parse multi node in nodelist, and to support corosync3, we need quite a lot of refactors on bootstrap, a well design and well tested new parser will be a good infrastructure for those job

arbulu89 · 2021-04-07T06:29:30Z

@liangxin1300
The change looks good (even though it is quite complex hehe, but I understand that this complexity is needed).
I have some some superficial comments, so feel free to change or not.
Really nice job!

@arbulu89 Thanks for review!
BTW, we could discuss how to reuse this part of codes between crmsh and this project.
I mean, current configuration parser in crmsh/corosync.py cannot support future corosync3 well, and also has the issue to parse multi node in nodelist, and to support corosync3, we need quite a lot of refactors on bootstrap, a well design and well tested new parser will be a good infrastructure for those job

@liangxin1300 And this code supports corosync3?
Making this code available on crmsh looks difficult. The best option as some of the next steps would be to translate this to crmsh and import it here. The issue is that we discarded to import crmsh modules here to avoid issues with py2/py3 versioning.
We might need to have a 2nd look. If the code goes in the exact same way to all the crmsh versions we should have any issue, but we should test with both py2 and py3.

We can do the next:

We merge this PR
We copy/paste/adapt this code in crmsh to do the same thing
We do new changes in this project. Import crmsh and use the same commands here
Final thing: Could you add a new changelog entry in this project (osc vc)? To have the changes tracked. You don't have to upgrade the version. Just add a basic text in a new entry in the changes file

liangxin1300 · 2021-04-07T06:50:48Z

@liangxin1300
The change looks good (even though it is quite complex hehe, but I understand that this complexity is needed).
I have some some superficial comments, so feel free to change or not.
Really nice job!

@arbulu89 Thanks for review!
BTW, we could discuss how to reuse this part of codes between crmsh and this project.
I mean, current configuration parser in crmsh/corosync.py cannot support future corosync3 well, and also has the issue to parse multi node in nodelist, and to support corosync3, we need quite a lot of refactors on bootstrap, a well design and well tested new parser will be a good infrastructure for those job

@liangxin1300 And this code supports corosync3?

Not yet.
But support corosync3 is on the way, crmsh will create new branch to maintain SLE15+ codes, while using master branch to support corosync3.
I want to refactor the corosync parser to support parse corosync3 well, then backport this big changes into SLE15+ branch

Making this code available on crmsh looks difficult. The best option as some of the next steps would be to translate this to crmsh and import it here. The issue is that we discarded to import crmsh modules here to avoid issues with py2/py3 versioning.

But in the future, might be after 30 Jun 2022, when 12sp3 end of life, we ha stack could not consider py2 anymore, right?

We might need to have a 2nd look. If the code goes in the exact same way to all the crmsh versions we should have any issue, but we should test with both py2 and py3.

We can do the next:

We merge this PR

We copy/paste/adapt this code in crmsh to do the same thing

We do new changes in this project. Import crmsh and use the same commands here

Agree with you.
Thanks @arbulu89 !

Final thing: Could you add a new changelog entry in this project (osc vc)? To have the changes tracked. You don't have to upgrade the version. Just add a basic text in a new entry in the changes file

Like this 9ce19d9?

arbulu89 · 2021-04-07T06:53:13Z

Like this 9ce19d9?

Yes! Thanks

arbulu89 · 2021-04-07T06:55:28Z

@liangxin1300 By the way, I don't know why the UT execution has been skipped... Most probably these changes will break the tests. Let me check this

arbulu89 · 2021-04-07T07:18:43Z

@liangxin1300 I have re-enabled the test in the CI in: #81
Let's wait until this PR is merged first to see what happens with the UT

Re-enable the UT first in other PR

liangxin1300 · 2021-04-07T07:23:27Z

@liangxin1300 I have re-enabled the test in the CI in: #81
Let's wait until this PR is merged first to see what happens with the UT

ok, I will complete UT code if needed

* Parse multi interface/node as list in the dict * When there is no space between words in corosync.conf, like "totem{" or "ring0_addr:10.10.10.121", corosync still works, but our parse results are wrong

liangxin1300 · 2021-04-08T07:08:59Z

@arbulu89 The UT codes have passed:)

arbulu89

@liangxin1300 Great work! I think we are ready to merge

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from 4193538 to 3c4aa36 Compare March 14, 2021 15:15

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from 4e6f0c7 to 3c4aa36 Compare March 22, 2021 06:49

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from aadb812 to 9febaff Compare April 2, 2021 03:03

liangxin1300 commented Apr 2, 2021

View reviewed changes

liangxin1300 force-pushed the 20210312_corosync_config_dict branch 3 times, most recently from 87ddc98 to 3dba9a8 Compare April 2, 2021 05:55

liangxin1300 commented Apr 6, 2021

View reviewed changes

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from 3dba9a8 to 9ad7dc1 Compare April 6, 2021 04:52

liangxin1300 commented Apr 6, 2021

View reviewed changes

liangxin1300 changed the title ~~[WIP] Improve crosync configure~~ Improve crosync configure Apr 6, 2021

arbulu89 requested review from arbulu89 and Simranpal April 6, 2021 12:37

arbulu89 previously approved these changes Apr 6, 2021

View reviewed changes

liangxin1300 force-pushed the 20210312_corosync_config_dict branch 2 times, most recently from b43c61d to 89648b3 Compare April 7, 2021 01:37

arbulu89 mentioned this pull request Apr 7, 2021

Enable the test job in the ci for PRs #81

Merged

arbulu89 self-requested a review April 7, 2021 07:18

liangxin1300 closed this Apr 7, 2021

liangxin1300 reopened this Apr 7, 2021

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from 9ce19d9 to 022f4a1 Compare April 8, 2021 02:41

liangxin1300 force-pushed the 20210312_corosync_config_dict branch 4 times, most recently from 7e5cc40 to 3956bdc Compare April 8, 2021 06:19

liangxin1300 added 3 commits April 8, 2021 15:06

Dev: crmshmod: Improve parse logic for corner cases

fd3382c

* Parse multi interface/node as list in the dict * When there is no space between words in corosync.conf, like "totem{" or "ring0_addr:10.10.10.121", corosync still works, but our parse results are wrong

salt-shaptools.changes: update the changelog

7acbfbf

unittest: update unit test codes for changes in crmshmod.py

fe7fe55

liangxin1300 force-pushed the 20210312_corosync_config_dict branch from 29b95cd to fe7fe55 Compare April 8, 2021 07:07

arbulu89 approved these changes Apr 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve crosync configure #80

Improve crosync configure #80

liangxin1300 commented Mar 14, 2021 •

edited

Loading

liangxin1300 Apr 2, 2021 •

edited

Loading

liangxin1300 Apr 6, 2021

arbulu89 Apr 6, 2021

liangxin1300 Apr 6, 2021

liangxin1300 commented Apr 6, 2021

arbulu89 left a comment

arbulu89 Apr 6, 2021

liangxin1300 Apr 7, 2021

arbulu89 Apr 6, 2021

liangxin1300 Apr 7, 2021

arbulu89 Apr 6, 2021

liangxin1300 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

liangxin1300 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

liangxin1300 commented Apr 7, 2021

liangxin1300 commented Apr 8, 2021

arbulu89 left a comment

Improve crosync configure #80

Are you sure you want to change the base?

Improve crosync configure #80

Conversation

liangxin1300 commented Mar 14, 2021 • edited Loading

To address these issues

liangxin1300 Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangxin1300 commented Apr 6, 2021

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangxin1300 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

liangxin1300 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

arbulu89 commented Apr 7, 2021

liangxin1300 commented Apr 7, 2021

liangxin1300 commented Apr 8, 2021

arbulu89 left a comment

Choose a reason for hiding this comment

liangxin1300 commented Mar 14, 2021 •

edited

Loading

liangxin1300 Apr 2, 2021 •

edited

Loading