Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dev: parse: Consider compatibility for role when complete operation actions with advised values #1083

Merged
merged 4 commits into from
Dec 7, 2022
Merged

Dev: parse: Consider compatibility for role when complete operation actions with advised values #1083

merged 4 commits into from
Dec 7, 2022

Conversation

liangxin1300
Copy link
Collaborator

@liangxin1300 liangxin1300 commented Dec 5, 2022 •
edited
Loading

  • Use utils.handle_role_for_ocf_1_1 to convert roles when cib schema version is lower than pacemaker-3.7
crm(live/15sp4-1)configure# primitive st ocf:pacemaker:Stateful
WARNING: Convert "Promoted" to "Master" since the current schema version is old and not upgraded yet. Please consider "crm configure upgrade force"
WARNING: Convert "Unpromoted" to "Slave" since the current schema version is old and not upgraded yet. Please consider "crm configure upgrade force"
crm(live/15sp4-1)configure# show
node 1: 15sp4-1
node 2: 15sp4-2
primitive st ocf:pacemaker:Stateful \
        op monitor timeout=20s interval=10s role=Master \
        op monitor timeout=20s interval=11s role=Slave \
        op start timeout=20s interval=0s \
        op stop timeout=20s interval=0s \
        op promote timeout=10s interval=0s \
        op demote timeout=10s interval=0s
  • Use utils.compatible_role to make sure always get the advised value if given roles are compatible
crm(live/15sp4-1)configure# primitive st ocf:pacemaker:Stateful op monitor role=Master op monitor role=Slave
crm(live/15sp4-1)configure# show
node 1: 15sp4-1
node 2: 15sp4-2
primitive st ocf:pacemaker:Stateful \
        op monitor role=Master interval=10s timeout=20s \
        op monitor role=Slave interval=11s timeout=20s \
        op start timeout=20s interval=0s \
        op stop timeout=20s interval=0s \
        op promote timeout=10s interval=0s \
        op demote timeout=10s interval=0s

@liangxin1300 liangxin1300 requested a review from gao-yan December 5, 2022 03:06
@liangxin1300
Copy link
Collaborator Author

Hi @gao-yan , please take a look at this PR
Thanks!

@liangxin1300 liangxin1300 mentioned this pull request Dec 5, 2022
Merged
gao-yan
gao-yan reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@gao-yan gao-yan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic idea looks good to me. Apart from a nitpicking in below, I added some comments on the context of this part of the code:

#1038 (review)

crmsh/utils.py Outdated
@@ -3062,6 +3062,14 @@ def is_ocf_1_1_cib_schema_detected():
return is_larger_than_min_version(cib_factory.get_schema(), constants.SCHEMA_MIN_VER_SUPPORT_OCF_1_1)


def compatible_role(role1, role2):
master_or_promoted = ("Master", "Promoted")
slave_or_unpromoted = ("Slave", "Unpromoted")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be better to define constants for these role names somewhere and also reuse them in handle_role_for_ocf_1_1(), for example:

RSC_ROLE_PROMOTED = "Promoted"
RSC_ROLE_UNPROMOTED = "Unpromoted"
RSC_ROLE_PROMOTED_LEGACY = "Master"
RSC_ROLE_UNPROMOTED_LEGACY = "Slave"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

liangxin1300
liangxin1300 commented Dec 6, 2022
View reviewed changes
crmsh/constants.py
@@ -241,8 +241,6 @@
date_spec_names = '''hours monthdays weekdays yearsdays months \
weeks years weekyears moon'''.split()
in_range_attrs = ('start', 'end')
roles_names = ('Stopped', 'Started', 'Master', 'Slave')
actions_names = ('start', 'promote', 'demote', 'stop')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete these 2 lines since they are not used

gao-yan
gao-yan reviewed Dec 6, 2022
View reviewed changes
crmsh/ui_resource.py Outdated
@@ -338,7 +338,7 @@ def do_promote(self, context, rsc):
if not xmlutil.RscState().is_ms_or_promotable_clone(rsc):
logger.error("%s is not a promotable resource", rsc)
return False
role = "Promoted" if config.core.OCF_1_1_SUPPORT else "Master"
role = constants.RSC_ROLE_PROMOTED if config.core.OCF_1_1_SUPPORT else constants.RSC_ROLE_PROMOTED_LEGACY
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this is worth to be functionalized. It's up to you though.

gao-yan
gao-yan approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@gao-yan gao-yan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only other two nitpickings.

crmsh/ui_resource.py Outdated
@@ -338,7 +338,7 @@ def do_promote(self, context, rsc):
if not xmlutil.RscState().is_ms_or_promotable_clone(rsc):
logger.error("%s is not a promotable resource", rsc)
return False
role = "Promoted" if config.core.OCF_1_1_SUPPORT else "Master"
role = utils.handle_role_for_ocf_1_1("Master")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use the constant RSC_ROLE_PROMOTED_LEGACY here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed,
Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, nice work!

crmsh/ui_resource.py Outdated
@@ -367,7 +367,7 @@ def do_demote(self, context, rsc):
if not xmlutil.RscState().is_ms_or_promotable_clone(rsc):
logger.error("%s is not a promotable resource", rsc)
return False
role = "Unpromoted" if config.core.OCF_1_1_SUPPORT else "Slave"
role = utils.handle_role_for_ocf_1_1("Slave")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

@liangxin1300
Dev: parse: Consider compatibility for role when complete operation a…
0a85020 
…ctions with advised values

- Use `utils.handle_role_for_ocf_1_1` to convert roles when cib schema version is lower than pacemaker-3.7
- Use `utils.compatible_role` to make sure always get the advised value if given roles are compatible
@liangxin1300
Dev: unittest: Add unit test for utils.compatible_role
eeb5082
@liangxin1300
Dev: parse: complete advised operation values for other actions besid…
b333609 
…e monitor
@liangxin1300
Dev: parse: Don't set timeout value when is not set by user meanwhile…
2801808 
… no value is advised in the metadata
@liangxin1300 liangxin1300 merged commit 84e6ea1 into ClusterLabs:master Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gao-yan gao-yan gao-yan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@liangxin1300 @gao-yan