agent: set a label when after successful os-update #168
Conversation
|
cc @lucab |
dghubble
left a comment
dghubble
left a comment
There was a problem hiding this comment.
So to be clear, you're unsatisfied with just waiting for the before-reboot label on the node the coordinator chooses for reboot. Instead, you'd like status or reboot-needed information, but in a label so you can have a DaemonSet selecting it. The benefit is some speedup since torcx takes a while, but in this scheme might already be done.
|
|
||
| // Label set to "true" on nodes where the update-agent has installed a new | ||
| // version, and a reboot is needed. Equivalent to the "reboot-needed" | ||
| // annotation, except that other agents sometimes set the annotation |
There was a problem hiding this comment.
huh? "Equivalent except other agents sometimes set the annotation"?
There was a problem hiding this comment.
I was referencing the behavior of the kvo, but I doubt this is worth mentioning.
| if s.CurrentOperation == updateengine.UpdateStatusUpdatedNeedReboot { | ||
| glog.Info("Indicating a reboot is needed") | ||
| anno[constants.AnnotationRebootNeeded] = constants.True | ||
| labels[constants.LabelUpdateStaged] = constants.True |
There was a problem hiding this comment.
Assuming we do need this label, why not have it be "reboot-needed"? Its effectively acting the same as the existing reboot-needed annotation and gets set true/false in the same locations.
We have a bunch of labels and annotations documented in https://github.com/coreos/container-linux-update-operator/blob/master/doc/labels-and-annotations.md#update-agent so I'm not keen to introduce a new one. Its complicated enough to explain them. At least with reboot-needed, folks are already familiar and we can say both a label and an annotation are set in lockstep.
There was a problem hiding this comment.
The only reason I didn't do that is because the reboot-needed annotation is sometimes set by other operators. But I think you're right.
| }) | ||
| } | ||
|
|
||
| // SetNodeAnnotationsLabels sets all keys in a and l to their respective values in |
There was a problem hiding this comment.
likewise -> respectively.
| // SetNodeAnnotationsLabels sets all keys in a and l to their respective values in | ||
| // node's annotations and labels, likewise | ||
| func SetNodeAnnotationsLabels(nc v1core.NodeInterface, node string, a, l map[string]string) error { | ||
| return UpdateNodeRetry(nc, node, func(n *v1api.Node) { |
There was a problem hiding this comment.
Or just call the SetNodeLabels and SetNodeAnnotation methods that are already there.
There was a problem hiding this comment.
Is there any value in doing it in a single transaction?
|
Ah, missed #167 which has the background context. |
|
So can we have the label use the prefix + The fact Tectonic sets CLUO's annotations is an anti-pattern I've tried to squash. CLUO doesn't make guarantees when external apps are setting its own clearly named attributes. So its not a concern for your PR. |
squeed
force-pushed
the
update-staged-label
branch
from
f3f2a63 to
da02653
Compare
|
Thanks for the feedback; updated. PTAL. |
sdemos
left a comment
sdemos
left a comment
There was a problem hiding this comment.
other than wanting slightly more fleshed out docs, this pr looks fine to me.
| | id | coreos | update-agent | Reflects the ID in `/etc/os-release` | | ||
| | version | 1497.7.0 | update-agent | Reflects the VERSION in `/etc/os-release` | | ||
| | group | stable | update-agent | Reflects the GROUP in `/usr/share/coreos/update.conf` or `/etc/coreos/update.conf` | | ||
| | reboot-needed | true | update-agent | Set to true to request a coordinated reboot | |
There was a problem hiding this comment.
this should explicitly say that this label is required to be in lockstep with the associated annotation.
There was a problem hiding this comment.
While we're at it, maybe we should say "Updates to true to request a coordinated reboot". I can see someone misreading "Set" as an instruction to manipulate it themselves.
|
I have no other issues. Just the point by @sdemos. We can do ship this feature out if needed or wait a bit after merge. |
squeed
force-pushed
the
update-staged-label
branch
from
da02653 to
09d5d61
Compare
This sets the label `container-linux-update.v1.coreos.com/reboot-needed: "true"` on nodes where the OS has been upgraded but not yet rebooted. This is so daemonsets like tectonic-torcx can run ASAP. Fixes: coreos#167
squeed
force-pushed
the
update-staged-label
branch
from
09d5d61 to
b9ba316
Compare
|
Thanks for the feedback, updated. It would probably be good to release this soon. |
3 participants
agent: set a label when after successful os-update
This sets the label
container-linux-update.v1.coreos.com/reboot-needed: "true"on nodes where the OS has been upgraded but not yet rebooted. This is so daemonsets like tectonic-torcx can run ASAP.Fixes: #167