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.

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

Detect pacemaker version and conditionally run test_check_cluster_configuration #287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dawei-Pang
Copy link
Contributor

@Dawei-Pang Dawei-Pang commented Sep 10, 2024

Refer to #276, test_check_cluster_configuration can works since pacemaker 2.1.8.
Add pacemaker version detection, so that the latest code can correctly with early version pacemaker: skip testing with the pacemaker version less than 2.1.8

VR: http://openqa.suse.de/tests/15389026
The followed docker image with this change is used in this VR
registry.opensuse.org/home/dawei_pang/branches/devel/openqa/ci/tooling/containers_15_4/hawk_test:latest
Generated in https://build.opensuse.org/package/show/home:dawei_pang:branches:devel:openQA:ci:Tooling/hawk_test

@@ -152,7 +156,10 @@ def main():
browser.test('test_remove_clone', results, clone)
browser.test('test_add_group', results, group)
browser.test('test_remove_group', results, group)
browser.test('test_check_cluster_configuration', results, ssh)
if pacemaker_version < "2.1.8":
Copy link
Contributor

Choose a reason for hiding this comment

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

it is OK for me, but notice that the comparison of version strings is different with string comarison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you for the reminding!
I did some test as following, hope that can match the comparison

>>> "2.1.5+20221208.a3f44794f" < "2.1.8"
True
>>> "1.1.24+20210811.f5abda0ee" < "2.1.8"
True
>>> "2.1.7+20221208.a3f44794f" < "2.1.8"
True
>>> "2.1.70+20221208.a3f44794f" < "2.1.8"
True
>>> "2.1.8+20221208.a3f44794f" < "2.1.8"
False

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the libraries are already using LooseVersion from distutils.version. You can perhaps use that one:

acarvajal@airfox:~/git/hawk/e2e_test [master|✔] > python3
Python 3.6.15 (default, Sep 23 2021, 15:41:43) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from distutils.version import LooseVersion as Version
>>> Version("2.1.5+20221208.a3f44794f") < Version("2.1.8")
True
>>> Version("1.1.24+20210811.f5abda0ee") < Version("2.1.8")
True
>>> Version("2.1.7+20221208.a3f44794f") < Version("2.1.8")
True
>>> Version("2.1.70+20221208.a3f44794f") < Version("2.1.8")
False
>>> Version("2.1.8+20221208.a3f44794f") < Version("2.1.8")
False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the document, I think the string comparison is acceptable for current situation.
https://docs.python.org/3/tutorial/datastructures.html#comparing-sequences-and-other-types

Copy link
Contributor

Choose a reason for hiding this comment

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

rpm -q --queryformat '%{VERSION}' only get the version string, we don't need to consider release string, the current comparison is acceptable in most cases(I checked the history of the version string, only included number and dot letters), but we also need to avoid the case like:
2.1.8 > 2.1.10 is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yes, let me research a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @badboywj . Look above at the tests I did with LooseVersion that it correctly returns False when doing 2.1.70 < 2.1.8 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Alvaro and Jun, I update commit to use the followed method to compare package version, how about you?
zypper -t vcmp 2.1.8 $(rpm -q --queryformat '%{VERSION}' pacemaker)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great. Good finding.

Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

LGTM. My only worry is the version check as pointed out by @badboywj

@Dawei-Pang Dawei-Pang force-pushed the check_pacemaker_version_conditional_run_test_check_cluster_configuration branch from 3482a5d to ef759d4 Compare September 11, 2024 12:22
Copy link
Contributor

@badboywj badboywj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Contributor

@alvarocarvajald alvarocarvajald left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants