-
Notifications
You must be signed in to change notification settings - Fork 37
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
[v2.7.1] Enable support for custom node roles for node groups #73
[v2.7.1] Enable support for custom node roles for node groups #73
Conversation
1bbc32d
to
5ae7af1
Compare
5ae7af1
to
80635b4
Compare
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.
Are you using a test catalog in your setup that has the new crd?
80635b4
to
7597ac0
Compare
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.
Couple nits
60cf084
to
35ca225
Compare
This has been retested on rancher I had an issue with The changes to the fix include
Ready for re-review |
35ca225
to
3da954f
Compare
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
generatedNodeRole := config.Status.GeneratedNodeRole | ||
|
||
if aws.StringValue(group.NodeRole) == "" { | ||
if config.Status.GeneratedNodeRole == "" { |
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.
config.Status.GeneratedRole
is not set until after the first reconciliation, so every node group will end up creating a stack. Is that by design?
e87b089
to
ae06bfd
Compare
|
||
// if a generated node role has not been set on the Status yet and it | ||
// was just generated, set it | ||
if config.Status.GeneratedNodeRole == "" && generatedNodeRole != "" { |
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, the only potential concerns I might have are that if an error returns later on, and we have to re-reconcile this object, it could potentially create more node groups than necessary (i.e. if another error happens between now and the time we end up updating the config object). I consider that outside of the scope of this PR because it looks like the entirety of this operator (and likely all the others) is written in a way that is contradictory to the design of wrangler's controller framework. We should revisit this at some point to prevent unnecessary thrashing by refactoring this. For example, once config.Status.GeneratedNodeRole
is set, we should update the config object, and if it fails delete the node group and reenqueue the config object, and if it succeeds, return early since the next reconciliation will shore up the rest of the status updates. Once again, out of the scope of this PR though.
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
Issue: rancher/rancher#29071
Problem
Enable custom node roles for node groups.
Solution
Changes
Testing
In the UI issue, it is specified
I simulated rancher UI input by setting the empty string and a valid user-defined string on the existing node group
NodeRole
field in the code and verify the expected flow happens in the operator.NodeRole
cannot be nil. Changes have been tested and verified with the PR rancher/rancher#38939.