-
Notifications
You must be signed in to change notification settings - Fork 133
Add support for openstack server groups #151
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 support for openstack server groups #151
Conversation
|
Anything wrong with this PR? I tested it with a custom Rancher server image and it seems to work fine. |
|
This PR's changes look good to me too. |
|
Hello ! |
|
This feature would be very helpful for HA. |
|
Hiya, is there something holding this back ? We only have Openstack as a provider and this is of real concern. Thanks very much |
|
Hi, I rebased and merged this PR locally and it still works fine. What is the problem with this? -- Jan |
|
Seems that SUSE does not support OpenStack anymore: https://www.suse.com/support/kb/doc/?id=000020491 . Anyway, it is always possible to fork the OpenStack node driver in a community project and add all the things it is actually missing. |
|
@jiaqiluo I see you were able to request a review 2 years back. This PR is actually still very relevant for rancher users that use Openstack. Taking into account the note from framctr I understand Suse does not actively support Openstack issues but I still see it gets some refactor work now and then. Do you have any insight whether this PR can be moved forward and if so what is needed? |
|
@pvbouwel, I'll bring this matter to the team for further follow-up. |
jiaqiluo
left a comment
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 seems that although both fields are exposed, only ServerGroupName is actually used to fetch the ID from the server and then override ServerGroupId. Since this PR has been inactive for years, the existing logic works, and OpenStack is not officially supported in Rancher, I’d consider this acceptable. Any further improvements can be handled by the community.
That being said, the PR LGTM.
(We need one more approval from the team for merging the PR)
HarrisonWAffel
left a comment
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.
LGTM, seems reasonable
|
As @framctr noted per https://www.suse.com/support/kb/doc/?id=000020491 OpenStack node driver is not officially supported; however per internal discussions we can accept community contributions provided sufficient testing is done by the community - including confirming no regressions are introduced by the change - and understanding that SUSE's QA has not validated these changes. The team has already reviewed this PR from "general sanity perspective" but please be aware we're not going to test it against OpenStack environment. @lu1as / @jweiher / @pvbouwel , could one (or more) of you cover the testing part, specifically provisioning RKE2 clusters with OpenStack in Rancher with AND without newly introduced fields? Now, the testing w/o merging this PR may be a little involved due to the following:
There are multiple options to address above but probably the least involved would be:
Please let me know if one of you could lend a hand in testing this and submitting back test results on this PR? Please also note that RKE1 is EOL and RKE1 provisioning is removed in Rancher v2.12.0+ so this will need to be tested on RKE2. Thank you in advance! |
|
@snasovich - PR is rebased, I also updated the commit message to match the new standard. I haven't used Rancher with RKE for quite a while, but I'll try to update my old Rancher image pipeline and do some testing. |
|
I really appreciate the actions on this. I'll also need to setup an image pipeline and a dedicated environment for testing this but have some urgent tasks currently. I hope to finish my own testing by end of first week of October. |
|
I successfully tested the change with Rancher v2.12.1 and RKE v1.33.4+rke2r1 on OpenStack 2024.1 (caracal). The test cases were:
I also verified that the created OpenStack instances were actually added to the server group. The new fields @snasovich - Let me know, if you have more test cases in mind which I should validate. |
|
@lu1as , sounds good, thank you for testing. Also, once we have this PR merged, |
|
@snasovich - I followed your second approach and replaced the rancher-machine binary in the Rancher image, as this also was the solution I used in the past to build my custom Rancher image. I can help testing the new Rancher image, of course. |
Hello,
this PR adds support for specifying a server group in OpenStack node templates, so that it's possible to ensure that for example master nodes were scheduled on different OpenStack compute nodes.
The server group has to be created and configured in OpenStack beforehand. It would be cool if Rancher could manage the server group itself, but I guess that logic is too much for a node driver, so I kept it simple here.
Related issues: