Only Mark Node Schedulable If Agent Marked Unschedulable #176

hankjacobs · 2018-02-23T23:50:23Z

This PR changes the agent to mark a node schedulable only if the agent was responsible for marking it unschedulable.

Prior to this change, the agent would always mark a node as schedulable when starting up. This could result in a node that had been marked unschedulable by an external process (eg. by an admin using kubectl) being marked schedulable by it's update agent if the agent restarted for some reason.

coreosbot · 2018-02-23T23:51:01Z

Can one of the admins verify this patch?

coreosbot · 2018-02-23T23:51:05Z

Can one of the admins verify this patch?

gojihotsauce · 2018-02-23T23:52:01Z

Can one of the admins verify this patch?

gojihotsauce · 2018-02-23T23:54:02Z

Can one of the admins verify this patch?

gojihotsauce · 2018-02-23T23:54:04Z

Can one of the admins verify this patch?

coreosbot · 2018-02-23T23:56:05Z

Can one of the admins verify this patch?

coreosbot · 2018-03-16T17:26:04Z

Can one of the admins verify this patch?

coreosbot · 2018-03-16T17:26:17Z

Can one of the admins verify this patch?

coreosbot · 2018-03-16T17:26:23Z

Can one of the admins verify this patch?

coreosbot · 2018-03-18T03:01:02Z

Can one of the admins verify this patch?

coreosbot · 2018-03-18T03:01:09Z

Can one of the admins verify this patch?

coreosbot · 2018-03-18T03:01:14Z

Can one of the admins verify this patch?

euank · 2018-03-19T18:16:57Z

Instance of jenkinsci/ghprb-plugin#292.

Honestly, we should probably move CoreOS projects related to k8s over to prow now that they've finished moving upstream off jenkins.

euank

I think other than the ordering of some calls this looks good. Thank you for the contribution! Apologies that it took us so long to get to it

euank · 2018-03-19T18:22:00Z

pkg/agent/agent.go

-		return err
+	if !alreadyUnschedulable {
+		glog.Info("Marking node as unschedulable")
+		if err := k8sutil.Unschedulable(k.nc, k.node, true); err != nil {


I believe there's a slight race here.

If the node is marked unscheduleable, but the annotation call fails (e.g. due to apiserver issues or this code failing), the failure condition is that the node will be unschedulable and never be made schedulable again.

I think that the k8sutil.SetNodeAnnotations call several lines above should include this annotation if necessary.

I think that setting the annotation before marking it unschedulable leaves it in a state where it will ultimately do the right thing once it succeeds.

Ah, right. Good catch. Thanks!

Looks like this was done at line 165.

I believe this got updated after this comment and github just didn't mark it as resolved

euank · 2018-03-19T18:23:12Z

pkg/agent/agent.go

-	glog.Info("Marking node as schedulable")
-	if err := k8sutil.Unschedulable(k.nc, k.node, false); err != nil {
-		return err
+	if makeSchedulable {


I think this should be moved to before the SetNodeAnnotationLabels for MadeUnschedulable: False. As is, I believe if it crashes or has issues with this call, it will not restore state correctly afterwards.

I see your point but I'm concerned with moving it above if err := k.waitForNotOkToReboot(); err != nil {. From the comment above that line, it seems like there might be trouble if we preemptively mark the node as schedulable before waiting for ok-to-reboot to clear. How about if I set the MadeUnschedulable annotation to false after the if makeSchedulable block (around line 129)?

Ah, inded, you're right. Moving the annotation down seems like it should fix the concern I have correctly.

euank · 2018-03-19T18:25:11Z

ok to test

sdemos · 2018-03-27T23:41:51Z

In addition to @euank's comments, if you don't mind squashing your cleanup and merge commits together, that would be awesome. Probably just the first three commits you have should cover it pretty well.

Just ping us whenever that is done and we will give it a final review.

coreosbot · 2018-03-27T23:42:01Z

Can one of the admins verify this patch?

hankjacobs · 2018-03-27T23:48:36Z

Will do! A bit swamped but have this on my to-do list.

coreosbot · 2018-04-06T18:45:04Z

Can one of the admins verify this patch?

sdemos · 2018-04-09T22:44:09Z

@hankjacobs any updates on this? just a friendly ping, no rush.

hankjacobs · 2018-04-09T23:20:07Z

still on my to-do list. sorry about that! I swear I'll get around to it in the next two weeks.

hankjacobs · 2018-04-17T17:47:51Z

@euank @sdemos Good to go. Thanks for your patience!

sdemos · 2018-05-07T23:37:22Z

Thanks for pushing the fix! I'll take a look at this again sometime soon.

My mistake with the issue reference, I had the link in my copy buffer and accidentally put it in a comment on an unrelated issue.

ashcrow · 2018-06-12T20:06:24Z

pkg/k8sutil/metadata.go

+			return fmt.Errorf("failed to get node %q: %v", node, getErr)
+		}
+
+		apiNode = n


nit: Seems like apiNode could be set where n is set. Not worth blocking over though.

Hmm, OTOH doing it this way makes it easy to verify that apiNode can only ever be set if we're done retrying.

ashcrow · 2018-06-12T20:08:35Z

pkg/agent/agent.go

-		return err
+	if !alreadyUnschedulable {
+		glog.Info("Marking node as unschedulable")
+		if err := k8sutil.Unschedulable(k.nc, k.node, true); err != nil {


Looks like this was done at line 165.

jlebon

Looks sane to me overall! Just some nits below.

Also, might be worth rebasing this PR to avoid the merge commit in this series.

jlebon · 2018-06-12T20:49:54Z

pkg/k8sutil/metadata.go

+			return fmt.Errorf("failed to get node %q: %v", node, getErr)
+		}
+
+		apiNode = n


Hmm, OTOH doing it this way makes it easy to verify that apiNode can only ever be set if we're done retrying.

jlebon · 2018-06-12T20:54:33Z

pkg/agent/agent.go

+	}
+
+	// Only make a node schedulable if a reboot was in progress. This prevents a node from being made schedulable
+	// if it was made unschedulable by something other thin the agent


Minor/optional: s/thin/than/

jlebon · 2018-06-12T21:01:52Z

pkg/agent/agent.go

+	}
+
+	glog.Infof("Setting annotations %#v", anno)
+	if err := k8sutil.SetNodeAnnotations(k.nc, k.node, anno); err != nil {


We only need to do this if we're the ones who marked the node unschedulable, right? I.e. we can move this hunk to under the if makeSchedulable { above.

It seems like it can be moved. I agree with this.

sdemos

@hankjacobs sorry that it took so long for us to come back to this. It seems like with just a couple of minor changes it will be good to merge in. If you could do the rebasing that @jlebon mentioned above too, that would be awesome.

I would like to get this in before I release a new version of CLUO. However, if you aren't going to be able to get to it soon, that's totally fine, just let me know and I'll release the new version without this change, and then release another version later when we do get this change merged. Thanks!

sdemos · 2018-06-12T22:09:33Z

pkg/agent/agent.go

+
+	// Only make a node schedulable if a reboot was in progress. This prevents a node from being made schedulable
+	// if it was made unschedulable by something other thin the agent
+	makeSchedulable := node.Annotations[constants.AnnotationAgentMadeUnschedulable] == constants.True


When the agent runs for the first time, this annotation doesn't exist, so this value is set as false, and the log message that says "node was marked unschedulable by external source" is printed out, which could be confusing. We should probably use the map multi-return value to check for the key existence and use that in the if statement below.

sdemos · 2018-06-12T22:11:09Z

pkg/agent/agent.go

+	}
+
+	glog.Infof("Setting annotations %#v", anno)
+	if err := k8sutil.SetNodeAnnotations(k.nc, k.node, anno); err != nil {


It seems like it can be moved. I agree with this.

…a generic retry function that retries on any error.

…lable. This is accomplished using a new annotation

…lable annotation

…annotation if agent made node schedulable.

hankjacobs · 2018-06-13T00:56:10Z

@sdemos All set with the requested changes. I haven't had a chance to test these changes since I had torn down my original testing infrastructure a while back. I may have some time to set it up tomorrow but no guarantees. Would you be able to test? If not, no biggie.

sdemos · 2018-06-13T20:16:55Z

Changes look good to me. I can test it out. Thanks for the quick fixes!

sdemos

Tested out the functionality on a test cluster, everything works as expected. Thanks for your patience!

euank suggested changes Mar 19, 2018

View reviewed changes

sdemos mentioned this pull request Mar 28, 2018

CLUO updates node to be schedulable if pod is deleted #138

Closed

hankjacobs force-pushed the master branch 2 times, most recently from 1814fe4 to 1b765c7 Compare April 17, 2018 16:37

ashcrow approved these changes Jun 12, 2018

View reviewed changes

jlebon approved these changes Jun 12, 2018

View reviewed changes

sdemos reviewed Jun 12, 2018

View reviewed changes

pkg/k8sutil: Add utility function for getting a node with retry. Add …

bf66334

…a generic retry function that retries on any error.

hankjacobs added 4 commits June 12, 2018 17:41

pkg/agent: Make a node schedulable only if the agent made it unschedu…

2f96731

…lable. This is accomplished using a new annotation

doc/lables-and-annotations: Document new annotation

8d9ae52

pkg/agent: reorder annotation and label logic for agent-made-unschedu…

620da75

…lable annotation

pkg/agent: Omit log statement if annotation does not exist. Only set …

f1c65d4

…annotation if agent made node schedulable.

hankjacobs force-pushed the master branch from 1b765c7 to f1c65d4 Compare June 13, 2018 00:53

sdemos approved these changes Jun 13, 2018

View reviewed changes

sdemos merged commit 7996c46 into coreos:master Jun 13, 2018

sdemos mentioned this pull request Jun 13, 2018

release v0.7.0 #181

Merged

Only Mark Node Schedulable If Agent Marked Unschedulable #176

Only Mark Node Schedulable If Agent Marked Unschedulable #176

Conversation

hankjacobs commented Feb 23, 2018

coreosbot commented Feb 23, 2018

coreosbot commented Feb 23, 2018

gojihotsauce commented Feb 23, 2018

gojihotsauce commented Feb 23, 2018

gojihotsauce commented Feb 23, 2018

coreosbot commented Feb 23, 2018

coreosbot commented Mar 16, 2018

coreosbot commented Mar 16, 2018

coreosbot commented Mar 16, 2018

coreosbot commented Mar 18, 2018

coreosbot commented Mar 18, 2018

coreosbot commented Mar 18, 2018

euank commented Mar 19, 2018

euank left a comment • edited Loading

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

hankjacobs Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Mar 19, 2018

sdemos commented Mar 27, 2018

coreosbot commented Mar 27, 2018

hankjacobs commented Mar 27, 2018

coreosbot commented Apr 6, 2018

sdemos commented Apr 9, 2018

hankjacobs commented Apr 9, 2018

hankjacobs commented Apr 17, 2018

sdemos commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon 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

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

hankjacobs commented Jun 13, 2018 • edited Loading

sdemos commented Jun 13, 2018

sdemos left a comment

Choose a reason for hiding this comment

euank left a comment •

edited

Loading

hankjacobs Mar 19, 2018 •

edited

Loading

hankjacobs commented Jun 13, 2018 •

edited

Loading