always drain before reboot #294

jackfrancis · 2021-01-11T21:43:13Z

This PR changes the pre-reboot drain functionality so that it always runs, regardless of the value of the Unschedulable node property. Ostensibly this "skip if unschedulable" foo was added due to the fact that the value of Unschedulable will be set to false as a side-effect of a kured reboot operation, and if we're in a "retry" loop here, we shouldn't have to "drain again".

However, because kubectl drain is idempotent, we shouldn't have to worry about any of that: we can run it over and over again. And because this drain func actually does a cordon + drain (and it only performs the drain if a cordon is successful), we can be sure that we aren't going to be thrashing this node w/ respect to scheduled pods.

And in fact, the current implementation of "only drain if node is marked Unschedulable" presents an edge case: if the node has been marked Unschedulable out-of-band, but workloads remain Running on this node, we will reboot the node's underlying VM/machine while it is actively running pods.

Fixes #18

PS 👋 thanks for kured! :)

Michael-Sinz · 2021-01-11T21:49:06Z

This is the same logic our rebooter does (we don't use kured)

A cordon out of band does not mean a drain but if it did drain, the drain will be trivial/fast. However, if it was not drained yet, it is critical to drain before rebooting.

/lgtm

evrardjp

I like this.

bboreham · 2021-01-12T12:33:58Z

Hi, the code looks fine to me, but could I ask you to update the commit description to say what it does and the thinking behind it?

"idempotent" is valid as a justification, but not as a description. A word like "unconditional" is more descriptive of what is happening, or just spell it out - "drain node before reboot even if unschedulable".

You could also put the text of the PR description into the commit message.

Thanks

Michael-Sinz · 2021-01-12T13:07:38Z

cmd/kured/main.go

@@ -344,7 +344,6 @@ func rebootAsRequired(nodeID string, window *timewindow.TimeWindow, TTL time.Dur
 			if err != nil {
 				log.Fatal(err)
 			}
-			nodeMeta.Unschedulable = node.Spec.Unschedulable


This is not an unnecessary assignment. The reason for this is to detect if the node was already cordoned such that when pod restarts it will remain cordoned. (Don't uncordon the node just because of the reboot).

Now, if that is no longer intended, you would want to also change the code above such that it always uncordons the node and does not look at this metadata to make that choice (since, well, that metadata is clearly never set).

Do we really want to touch the nodes if they have been cordonned? I would say no.

This is indeed a good point.

This is a different question.

There are two things:

If the node is already cordoned and we are going to reboot it, we should leave it cordoned after reboot

If the node is already cordoned, do we just ignore it

Before this PR, the behavior was the same as (1) above except is had the problem of not draining the node before rebooting. The point of this change was to not skip the drain if we are going to reboot, which is the simple removal of the conditional. We still need to collect the cordoned state of the node such that it remains so after reboot.

However, if we want to make a new policy (I claim a breaking change) that cordoned nodes are not rebooted, then the logic needs to be very different and the uncordon above should not be gated based on the metadata in the lock.

I claim that the core behavior of the code was correct and acting like (1) and that the main change was to also drain any node that was going to reboot.

Michael is right we don't want to change this value, I've reverted it. It's actually passed by reference to the acquire() func, so we need to make sure that the nodeMeta instance has the correct Unschedulable property value.

@Michael-Sinz That was indeed my understanding, sorry if I haven't been verbose. And yes, I was still asking that new question on top. But let's ignore it for now.

@jackfrancis ok, thanks!

jackfrancis · 2021-01-12T19:46:23Z

Hi, the code looks fine to me, but could I ask you to update the commit description to say what it does and the thinking behind it?

"idempotent" is valid as a justification, but not as a description. A word like "unconditional" is more descriptive of what is happening, or just spell it out - "drain node before reboot even if unschedulable".

You could also put the text of the PR description into the commit message.

Thanks

Thanks for the comments, will do!

jackfrancis · 2021-01-12T19:59:10Z

@bboreham I've updated the commit message (and the PR title as well), thanks again for the comments!

jackfrancis · 2021-01-19T17:23:06Z

@evrardjp I'm not super familiar with these tests, but it doesn't appear that the CI failure for helm + 1.18 E2E is related to this changeset. May I kindly ping a maintainer to re-run the test to queue this up for merge?

Thanks again! Lemme know if there's anything else I can do on this PR.

evrardjp · 2021-01-20T09:11:52Z

Yes it might happen that this test is failing, depending on where the github action runs. As you can see, it's overall good. IMO, there is no need to retrigger the test, it's good enough for me. Sadly I am not aware of a way to retrigger the failing test (except for the "re-trigger" button, which appears only for those having write access to the repo IIRC).

For me, this PR is ready.

jackfrancis · 2021-02-09T18:13:27Z

@dholbach Any thoughts on a final review and merge of this change?

jackfrancis · 2021-02-10T17:14:35Z

@bboreham Is there anything more we want to do to consider this for a merge? Should we retry the helm chart + 1.18 test job?

jackfrancis · 2021-03-02T17:24:17Z

@dholbach do we think that @evrardjp's comments from Jan 20 are sufficient to lgtm and merge this? If you're able to retry the failed 1.18 test (doesn't seem to be testing anything related to this change -- no helm chart changes are included here), that would probably be a good idea.

Thanks!

squaremo · 2021-03-08T17:19:01Z

I'm trying to find someone to rerun the test case -- I would prefer to see it green before it's merged. (Apparently I can merge it but I can't rerun a test 🤷)

dholbach · 2021-03-08T17:27:29Z

To short-circuit this, running tests in my personal GH: https://github.com/dholbach/kured/actions/runs/633086975

dholbach · 2021-03-08T17:52:48Z

To short-circuit this, running tests in my personal GH: https://github.com/dholbach/kured/actions/runs/633086975

Looking good! 🚀

This changes the pre-reboot drain functionality so that it always runs, regardless of the value of the Unschedulable node property. Because kubectl drain is idempotent, we shouldn't have to worry about whether the node has already been set to Unschedulable (perhaps due to a prior, unsuccessful loop of the kured reboot cycle): we can run it over and over again. And because this drain func actually does a cordon + drain (and it only performs the drain if a cordon is successful), we can be sure that we aren't going to be thrashing this node w/ respect to scheduled pods. This also fixes an edge case: if the node has been marked Unschedulable out-of-band, but workloads remain Running on this node, kured will no longer reboot the node's underlying VM/machine while it is actively running pods.

dholbach

Approved, based on Michael's review.

evrardjp approved these changes Jan 12, 2021

View reviewed changes

evrardjp added this to the 1.7.0 milestone Jan 12, 2021

evrardjp added scheduling-improvements enhancement and removed enhancement labels Jan 12, 2021

Michael-Sinz reviewed Jan 12, 2021

View reviewed changes

jackfrancis force-pushed the always-drain branch from 2c66190 to 1f76c6f Compare January 12, 2021 19:56

jackfrancis changed the title ~~idempotent drain~~ always drain before reboot Jan 12, 2021

evrardjp approved these changes Jan 14, 2021

View reviewed changes

jackfrancis mentioned this pull request Jan 22, 2021

Document kured integration jackfrancis/kamino#46

Open

jackfrancis force-pushed the always-drain branch from 1f76c6f to 93c8242 Compare March 9, 2021 01:20

dholbach approved these changes Mar 9, 2021

View reviewed changes

dholbach merged commit 32e01a8 into kubereboot:master Mar 9, 2021

jackfrancis mentioned this pull request May 14, 2021

Investigate kured https://github.com/weaveworks/kured jackfrancis/kamino#35

Closed

ckotzbauer mentioned this pull request May 19, 2021

1.7.0 release notes #295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always drain before reboot #294

always drain before reboot #294

jackfrancis commented Jan 11, 2021 •

edited

Loading

Michael-Sinz commented Jan 11, 2021

evrardjp left a comment

bboreham commented Jan 12, 2021

Michael-Sinz Jan 12, 2021

evrardjp Jan 12, 2021

Michael-Sinz Jan 12, 2021

jackfrancis Jan 12, 2021

evrardjp Jan 13, 2021 •

edited

Loading

jackfrancis commented Jan 12, 2021

jackfrancis commented Jan 12, 2021

jackfrancis commented Jan 19, 2021

evrardjp commented Jan 20, 2021 •

edited

Loading

jackfrancis commented Feb 9, 2021

jackfrancis commented Feb 10, 2021

jackfrancis commented Mar 2, 2021

squaremo commented Mar 8, 2021

dholbach commented Mar 8, 2021

dholbach commented Mar 8, 2021

dholbach left a comment

always drain before reboot #294

always drain before reboot #294

Conversation

jackfrancis commented Jan 11, 2021 • edited Loading

Michael-Sinz commented Jan 11, 2021

evrardjp left a comment

Choose a reason for hiding this comment

bboreham commented Jan 12, 2021

Michael-Sinz Jan 12, 2021

Choose a reason for hiding this comment

evrardjp Jan 12, 2021

Choose a reason for hiding this comment

Michael-Sinz Jan 12, 2021

Choose a reason for hiding this comment

jackfrancis Jan 12, 2021

Choose a reason for hiding this comment

evrardjp Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

jackfrancis commented Jan 12, 2021

jackfrancis commented Jan 12, 2021

jackfrancis commented Jan 19, 2021

evrardjp commented Jan 20, 2021 • edited Loading

jackfrancis commented Feb 9, 2021

jackfrancis commented Feb 10, 2021

jackfrancis commented Mar 2, 2021

squaremo commented Mar 8, 2021

dholbach commented Mar 8, 2021

dholbach commented Mar 8, 2021

dholbach left a comment

Choose a reason for hiding this comment

jackfrancis commented Jan 11, 2021 •

edited

Loading

evrardjp Jan 13, 2021 •

edited

Loading

evrardjp commented Jan 20, 2021 •

edited

Loading