-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Make it possible to use OnDelete update strategy on addon daemonset #10167
Conversation
channels/pkg/channels/addon.go
Outdated
klog.Infof("addon %v wants to roll %v nodes", a.Name, a.Spec.NeedUpdate) | ||
selector := "" | ||
switch a.Spec.NeedUpdate { | ||
case "masters": |
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.
Should probably use "control-plane" for newly added fields.
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.
Hrm, I wonder if we should introduce a mix of old and new terminology now or instead wait for a new apiVersion to change all the terminology used in the API at once.
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.
I'm willing to let this proceed with the legacy terminology.
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.
This isn't really exposed to users, so I am all for changing the names.
channels/pkg/channels/addon.go
Outdated
switch a.Spec.NeedUpdate { | ||
case "masters": | ||
selector = "node-role.kubrnetes.io/master=" | ||
case "nodes": |
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.
Consider "worker"
9266e90
to
ca0391d
Compare
/retest |
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.
Just a few nits.
channels/pkg/api/channel.go
Outdated
// NeedUpdate determines if we should mark nodes as needing an update. | ||
// Legal values are control-plane, workers, and all | ||
// Empty value means no update needed | ||
NeedUpdate string `json:"needsRollingUpdate,omitempty"` |
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.
Please make the struct field name and json name consistent. I think NeedsRollingUpdate
would be a better struct name.
Make it possible for addons to set needs-update annotation Use onDelete update strategy for cilium and set needs-update annotation Rename node roles
ae9f3d5
to
5ed629e
Compare
5ed629e
to
f4e3dd3
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #7971
Or at least makes it possible to fix it for the addons we need.
Works by adding another element to the addon spec. If it is set, will make channels patch an annotation on the relevant nodes that rolling update uses to determine if an instance needs updating.