agent: set a label when after successful os-update #168

squeed · 2018-01-18T16:29:17Z

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

squeed · 2018-01-18T16:29:31Z

cc @lucab

dghubble

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.

dghubble · 2018-01-18T18:03:29Z

pkg/constants/constants.go

@@ -72,6 +72,11 @@ const (
 	// auto-label-container-linux=true
 	LabelUpdateAgentEnabled = Prefix + "agent"

+	// 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


huh? "Equivalent except other agents sometimes set the annotation"?

I was referencing the behavior of the kvo, but I doubt this is worth mentioning.

dghubble · 2018-01-18T18:08:38Z

pkg/agent/agent.go

 	// indicate we need a reboot
 	if s.CurrentOperation == updateengine.UpdateStatusUpdatedNeedReboot {
 		glog.Info("Indicating a reboot is needed")
 		anno[constants.AnnotationRebootNeeded] = constants.True
+		labels[constants.LabelUpdateStaged] = constants.True


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.

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.

dghubble · 2018-01-18T18:10:47Z

pkg/k8sutil/metadata.go

@@ -80,6 +80,20 @@ func SetNodeAnnotations(nc v1core.NodeInterface, node string, m map[string]strin
 	})
 }

+// SetNodeAnnotationsLabels sets all keys in a and l to their respective values in


likewise -> respectively.

dghubble · 2018-01-18T18:11:32Z

pkg/k8sutil/metadata.go

+// 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) {


Or just call the SetNodeLabels and SetNodeAnnotation methods that are already there.

Is there any value in doing it in a single transaction?

dghubble · 2018-01-18T18:21:54Z

Ah, missed #167 which has the background context.

dghubble · 2018-01-22T22:25:49Z

So can we have the label use the prefix + reboot-needed name? We'll document that CLUO sets the reboot-needed annotation and label in lock-step. Label is easier to write deployments / daemonsets against. Annotation is easier to write programs against.

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 · 2018-01-23T19:15:41Z

Thanks for the feedback; updated. PTAL.

sdemos

other than wanting slightly more fleshed out docs, this pr looks fine to me.

sdemos · 2018-01-23T21:10:30Z

doc/labels-and-annotations.md

@@ -30,6 +30,7 @@ A few labels may be set directly by admins to customize behavior. These are call
 | 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 |


this should explicitly say that this label is required to be in lockstep with the associated annotation.

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.

dghubble · 2018-01-24T08:11:25Z

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.

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 · 2018-01-24T17:38:41Z

Thanks for the feedback, updated. It would probably be good to release this soon.

pkg/k8sutil: utility function to set labels and annotations in one txn

dedf1a2

squeed requested a review from dghubble January 18, 2018 16:29

dghubble reviewed Jan 18, 2018

View reviewed changes

squeed force-pushed the update-staged-label branch from f3f2a63 to da02653 Compare January 23, 2018 19:15

sdemos reviewed Jan 23, 2018

View reviewed changes

squeed force-pushed the update-staged-label branch from da02653 to 09d5d61 Compare January 24, 2018 17:37

agent: set a label when after successful os-update

b9ba316

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 January 24, 2018 17:37

dghubble approved these changes Jan 25, 2018

View reviewed changes

dghubble merged commit 420190e into coreos:master Jan 25, 2018

dghubble mentioned this pull request Jan 25, 2018

Release v0.6.0 #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent: set a label when after successful os-update #168

agent: set a label when after successful os-update #168

squeed commented Jan 18, 2018 •

edited

Loading

squeed commented Jan 18, 2018

dghubble left a comment

dghubble Jan 18, 2018

squeed Jan 19, 2018

dghubble Jan 18, 2018

squeed Jan 19, 2018

dghubble Jan 18, 2018

dghubble Jan 18, 2018

squeed Jan 19, 2018

dghubble commented Jan 18, 2018

dghubble commented Jan 22, 2018 •

edited

Loading

squeed commented Jan 23, 2018

sdemos left a comment

sdemos Jan 23, 2018

dghubble Jan 23, 2018

dghubble commented Jan 24, 2018

squeed commented Jan 24, 2018

agent: set a label when after successful os-update #168

agent: set a label when after successful os-update #168

Conversation

squeed commented Jan 18, 2018 • edited Loading

squeed commented Jan 18, 2018

dghubble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble commented Jan 18, 2018

dghubble commented Jan 22, 2018 • edited Loading

squeed commented Jan 23, 2018

sdemos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble commented Jan 24, 2018

squeed commented Jan 24, 2018

squeed commented Jan 18, 2018 •

edited

Loading

dghubble commented Jan 22, 2018 •

edited

Loading