-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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
[crmsh-4.6] Dev: pre-migration: implement pre-migration checks for SLES 16 (jsc#PED-11808) #1629
base: crmsh-4.6
Are you sure you want to change the base?
[crmsh-4.6] Dev: pre-migration: implement pre-migration checks for SLES 16 (jsc#PED-11808) #1629
Conversation
70f74a6
to
1a80eee
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
d9038c6
to
a2518dd
Compare
a740b3f
to
2bcad35
Compare
stonith:fence_vmware-rest | ||
stonith:fence_wti | ||
stonith:fence_xenapi | ||
stonith:fence_zvm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How has the above data been generated?
Can we automatically do that based on the locally installed resource-agents and fence-agents-* rpm?
Or, can we generate the data at the time crmsh using it (_load_supported_resource_agents)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How has the above data been generated?
Extracted the file list from resource-agents/fence-agents.
Can we automatically do that based on the locally installed resource-agents and fence-agents-* rpm?
No, because we need to know what is supported by SLES 16 instead of SLES 15 SP6.
2932b25
to
8cf8ef3
Compare
22ea8a0
to
a338bc6
Compare
Suggestion:1. Add completer for
|
Suggestion: 6. How about there are Failed Resource Actions?For some reason, some RA failed. For now, 7. crm report collect pre-migration infoI think we also need to collect the output of 8. Inaccurate usage?
Should be
The same for #1422 9. Fence agents checking
Better to add what's the alternative?
|
crmsh/migration.py
Outdated
handler.log_info("Checking used corosync features...") | ||
transport = 'udpu' if corosync.is_unicast() else 'udp' | ||
handler.handle_tip(f'Corosync transport "{transport}" will be deprecated in corosync 3.', [ | ||
'After migrating to SLES 16, run "crm health sles16 --fix" to migrate it to transport "knet".', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be crm cluster health sles16 --fix
crmsh/migration.py
Outdated
]) | ||
if corosync.get_value("totem.rrp_mode") in {'active', 'passive'}: | ||
handler.handle_tip(f'Corosync RRP will be deprecated in corosync 3.', [ | ||
'After migrating to SLES 16, run "crm health sles16 --fix" to migrate it to knet multilink.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be crm cluster health sles16 --fix
81e76eb
to
2dff378
Compare
@@ -0,0 +1,25 @@ | |||
"""utilities for parsing CIB xml""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to move these codes into xmlutil.py instead of creating a new py file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. xmlutil.py is too long, and its name does not reflect its content. Instead of general xml processing utilities, it is a mixed mess with shell utils, CIB specific routines, and even CLI handlers.
crmsh/cibquery.py
Outdated
) | ||
|
||
|
||
def has_primitive_filesystem_ocfs2(cib: lxml.etree.Element) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we might use this to detect gfs2, could you please change 'ocfs2' as a parameter?
crmsh/migration.py
Outdated
|
||
def check_unsupported_resource_agents(handler: CheckResultHandler): | ||
handler.log_info("Checking used resource agents...") | ||
crm_mon = xmlutil.CrmMonXmlParser() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving this file under crmsh/crmsh/data
, in case there are other txt files in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to do that. I think this will need to add a python module crmsh.data
, making our build scripts more complex.
sys.stdout.write(' Good to migrate to SLES 16.\n\n') | ||
|
||
|
||
def check(args: typing.Sequence[str]) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to add a docstring to:
- explain what the
--json
and--local
option are used for - introduce this function and return code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite self-explained. Adding docstrings like "migration.check: run migration checks", or "--json output the result in json format" does not make sense. And the return code is also the common main() style return code.
This usage is correct
But not this
|
2dff378
to
d1563aa
Compare
d1563aa
to
d9c0449
Compare
Usages changed to
|
handler.handle_tip(f'Corosync transport "{transport}" will be deprecated in corosync 3.', [ | ||
'After migrating to SLES 16, run "crm cluster health sles16 --fix" to migrate it to transport "knet".', | ||
]) | ||
if corosync.get_value("totem.rrp_mode") in {'active', 'passive'}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although with low possibility, rrp_mode
can be set to none
for the one-ring
should include none
value here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none
means SRP, which is supported and does not needs any actions.
* `sles16`: check whether the cluster is good to migrate to SLES 16. | ||
|
||
The optional `--fix` argument attempts to automatically resolve any detected | ||
issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add note that for sles16
, --fix only available at SLES 16
…PED-11808) should use recursive queries
…ilesystem_with_fstype
d9c0449
to
94926c5
Compare
crmsh/migration.py
Outdated
if remote_ret > ret: | ||
ret = remote_ret | ||
if not parsed_args.json: | ||
print('------ summary ------') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better change this line as
****** Summary ******
To mark this is not the node name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, great work!
Thanks!
94926c5
to
252befd
Compare
This pull request implements pre-migration checks for SLES 16. These checks are expected to run before migrating to SLES 16.
These checks ensure all the cluster nodes runs the latest version of corosync 2.x and pacemaker 2.1.x, and report used resource agents and fence agents that will be removed in SLES 16. It also provides advice for further actions need to take after migration.
Use Cases
crm cluster health sles16
crm cluster health sles16 --fix
When some of the nodes are offline