-
Notifications
You must be signed in to change notification settings - Fork 117
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
Limit the Random Hash in the MachineSet Name #500
Limit the Random Hash in the MachineSet Name #500
Conversation
/invite @danielfoehrKn I don't think this change hinders the g/g worker actuator code by any chance. However, just wanted you to take a look at this change. @AxiomSamarth - Also did you confirm though both scenario based testing and code logic that this wouldn't trigger rolling updates of existing machine sets backed by deployments? |
@prashanth26 I tested this with the fresh deployment. I will test to see if this triggers any rolling updates of existing machine sets backed by deployment. |
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.
@prashanth26 @hardikdr Perhaps a stupid question. Why do we not simply use GenerateName
and let the kube-apiserver
figure out the name generation?
The generic worker actuator deploys the Machine Deployment - how the MCM generates the MachineSets etc. is unrelated I think. |
@danielfoehrKn - Okay thanks for that. I thought the same but wanted to just confirm from you. |
@amshuman-kr Hi Amshu, seems like even with the |
Back then this code was copied over from the replicaSet controller code. And this was the code there back then i think. Even now they use kind of similar code ther. |
Thanks for the clarification. Is there any hurdle to start using |
That's actually a good suggestion, and we should probably move to As this is only a ramp-up task for Samarth, and he is still getting familiar with the code-base, I'd refrain from having him test e2e with this pov. Ref : #502 |
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.
We should probably cross-check once, if this change has any side-effect on the rolling-update, hopefully not.
/lgtm otherwise.
Thanks for the PR :)
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
What this PR does / why we need it:
Currently, MachineSet names are with variable length of the random hash. The MCM is also not limiting the random hash in the machineset names. But if we limit the length of the random hash, it will help to reduce the occurrences of the CSI issue till it is fixed upstream
Which issue(s) this PR fixes:
Fixes #461
Special notes for your reviewer:
Release note: