-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add rule support for location constraints #43
Add rule support for location constraints #43
Conversation
Hello Michael (@mib1185), Thank you for using pcs-modules-2 modules and for finding time to prepare this PR! From initial look at code this is looks promising and I can see that you have implemented checks for content of the rule to match what pacemaker 2.1 would expect there - nice job! Before having a better look at code itself I want to clarify few things:
As for the testing, documentation and examples I can see that Pacemaker Exaplained 2.1 chapter 10 Rules looks like a nice place to get some inspiration from on what to test. Looking at
As for documentation it is sufficient to update Once there is documentation with examples and it is clear which systems this targets I will give it a try and provide you with feedback on code level. Note that I usually have time for checking code only during weekends so there might be no update from me on weekdays. Once again Thank you for preparing the code and I will be looking to hear from you. Feel free to ask if you have any questions and feel free to take the time needed. -- |
Hello @OndrejHome many thanks for fast reply and of course also developing this ansible module and the corresponding ansible role 👍
yeah ... it take me some time to fetch (hopefully) all possible variants of rules 🙈
For developing the module code itself i've used an ubuntu 22.04 (WSL2).
They are the same as above.
TBH i wasn't aware of the
I'll add documentation and examples soon, but maybe earliest week after next.
Take your time and every feedback is highly appreciated 🙂 Regards, |
Hello @OndrejHome i've added the doc strings and some examples. |
Hello Michael (@mib1185 ), Thank you for the explanations and addition of examples! I like to see the As for the I have done few tests so far and plan to find time to finish them by the end of this week to not let you wait for too long. If it will work functionally as expected on my systems I plan to merge this PR, give repository a new tag and let you know what it is. -- |
Hi Ondrej (@OndrejHome), many thanks for this positiv feedback and reply 🙂
yeah, I just took the example from the official pcs docs about Moving Resources Due to Connectivity Changes 🙈
... but I also fully agree with you 👍
I'm looking forward for your test results 🤞 regards, |
Hello Michael (@mib1185), So far I have following results from my testing on Alma-8.10/Alma-9.4/Fedora-40/Debian-12.5:
Looking at what the Alma systems might be upset about I have tried changing code in class definition to following to make it work:
Every time I run above task I can see this:
While expectation is that it should just be 'ok' once it was created, so there seems to be issue with comparing the data and determining when "rule is same or not". I have tried following to see if I get any better results:
Issue is also present in case the following task is used:
NOTE: some old systems (looking at pacemaker-1.1 don't have all possibilities for date-spec and future pacemaker-3.x is planning to drop support for 'moon' date-spec. So ideally we should compare only the values that are in CIB and that user specified in task as parameter to module.
Hope the above is somewhat readable. If there are any questions, feel free to just ask. I would like to have addressed at minimum the issue-A and issue-B before merge. I will help with testing of solution for issue-C as that is for fairly old, but still in-production systems that I know of. -- |
Hi Ondrej, many thanks for your test efforts.
I'm on a business trip next week, so I'll care about these points earliest the week after. Regards, |
c0837e2 should solve issue A and C |
Hi Ondrej, issue A and C should be solved no (just removed all annotations). About issue B - could you please provide me the CIB xml files of your test clusters, where you have set these constraint rules? (feel free to contact me via discord).
This is already ensured, since the possible attributes are initialized as Regards, |
2024-06-24-issue-b-debian-12.5-files.tgz Hi Michael, Thank you for the changes. For the issue-b I'm attaching the playbook I have used for testing and CIB ( -- |
Hi Ondrej, there was an issue in the regex for finding the date spec parts ( regards, |
Hi Michael, I'm not sure if I'm testing it wrong, but even with new match I still run into same issue - the task is always 'changed'. I have briefly tested it on Debian-12.5, Fedora 40 and even tried to create one new Debian-12.5 cluster but issue-B was still present there. Looking at -- |
Hi Ondrej, thanks for your tests - I'll check it further the upcoming days 👍 Regards, |
Hi Ondrej 👋 sorry for long delay, but I come back with good news - I found the root cause of issue B. Regards, |
Hi Michael, Thank you fro great news. I think it is one of those things that I also tend to overlook easily - underscore vs dash - and if that was really the case and only problem that would be great. I have a bit busy week, but will try to give it a test at end of this week or starting of next. I will keep you updated how my testing went then. -- |
Hi Ondrej, |
Hi Michael, So far some quick results from Alma-8.10, Alma-9.4, CentOS 7.9, Debian-12.5, Fedora-40 = behaviour is now consistent on all those, which is good. 👍 On Centos 7.9 (python 2.7) I had to remove the python3 syntax to allow it to work, diff below. Could you please update that part? (this looks like the past issue-C)
The only task that I now see as always 'changed' is (in my opinion) quite special case of in_range with duration specified in weeks. Example playbook task below. I can live with this one not being perfect, but please let me know if you want to have a better look at this or if it can be left in current state.
I will have again some time around end of this week to do final checks and unless there is anything else big I will do the merging and will move to the part with ha-cluster-pacemaker role to incorporate the related changes. Thank you for your continued efforts! -- |
Hi Ondrej, i always forget about python 2.7 🙈 will remove the annotations soon 👍 About the very special Thanks for your continued and detailed testing efforts. I'm looking forward to bring this on the road 🙂 Regards, |
Hi Michael (@mib1185 ) Thank you for your patience and even more for your contribution here! Nice job! 👍 -- |
Hi Ondrej, |
Hi @OndrejHome
this is my approach to add rule support for location constraints. In fact there is no generic possibility to fetch a matching constraint rule (like we do for
prefers
constraints by checking thenode-name
) I thought we need to add an identifier for rule-based constraints.it is tested with
Todo