implement issue-19 add prefer no schedule taint to avoid double draining of pods

[damoon]

This implements #19

The idea is to set a prefer no schedule taint.
The taint is set when the reboot sentinel is found and removed after the restart.

In case the node can not restart immediately the taint asks the kubernetes scheduler to avoid placing new pods on the tainted node.
But this is only preferred, as a last resort the kubernetes scheduler does place pods onto the node.

[damoon]

The test is failing because the taint code uses the update method, but RBAC does only include patching nodes.

[damoon]

it works now 😃
someone wants to review it?

[evrardjp]

First, allow me to thank you for this PR! Well thought out.

Please see my comments inline for the first part of the review (maybe a comment, and a little code change?) on the preferNoSchedule code location/behaviour.
Finally, I see a problem with the PR right now (maybe I am mistaken, could you clarify?):
This PR changes the state of the taint TaintEffectPreferNoSchedule regardless of not it was tained by kured. This is a problem for me, because one administrator might want to mark a node for NoSchedule for other reasons, and we might remove it without its approval. WDYT?

[damoon]

This PR changes the state of the taint TaintEffectPreferNoSchedule regardless of not it was tained by kured. This is a problem for me, because one administrator might want to mark a node for NoSchedule for other reasons, and we might remove it without its approval. WDYT?

Only the taint "weave.works/kured-node-reboot" (by default) is added or remove by the new behavior. Other taints will be untouched. The code from https://github.com/damoon/kured/blob/19-PreferNoSchedule/cmd/kured/main.go#L326 until https://github.com/damoon/kured/blob/19-PreferNoSchedule/cmd/kured/main.go#L367 ensures only to add/remove this one taint.
TaintEffectPreferNoSchedule is the kind of taint. Alternatives are NoSchedule and NoExecute.

[damoon]

I was not sure who the code was organized.
Should this get its own package in https://github.com/weaveworks/kured/tree/master/pkg ?

[evrardjp]

I have not tested it, but I like it.

[evrardjp]

CI tested it, so I guess that's a good start.

Should we extend smoke test to ensure that we can schedule a basic workload? (And maybe ensuring the behaviour of this PR?)

[damoon]

I tested the parallel reboot of nodes and the taint creation/removal.
(But this happened before moving the code to its own package.)
I would feel more comfortable, if someone else would check it too.

[damoon]

i found an issue:
rebootBlocked checks prometheus for alerts and does not indicate an other node has the lock
i moved adding the taint into the !acquire block.

once the testing is done is push the fix.

[damoon]

fixed and tested. works for me.

[bboreham]

I left some minor comments below.

It would be good to mention this feature in the README.

I did not really follow the test / add patch stuff. Do you have a reference to some documentation explaining it, or another program that does the same? (kubectl taint does not appear to do it).

Also I think there is another corner case: if rebootRequired() goes from true to false the taint will not be removed until the end of the reboot window, or until restart of kured.

[damoon]

I updated the parameter section in README.

For patches I found this https://dwmkerr.com/patching-kubernetes-resources-in-golang/ example and for test i found http://jsonpatch.com/#test.
Test is needed to ensure the list of taints did not change on server side while the offset to edit was processed locally.
I believe kubectl uses update instead of patch and avoids this problem with a optimistic lock.
This implementation chose patch because patch was already part of the RBAC profile while update node is missing.

I changed the behavior in case rebootRequired() switches from true to false to remove the taint.

[bboreham]

Looks good to me.
Thanks for all changes, and for the explanation of why it is done this way.

[evrardjp]

I have added a question/comment to clarify our goals. I would prefer if this is clarified before merging. However, I don't think it's worth blocking this PR from merging if we don't get consensus.

Please note that I didn't test this patch. The code looks okay to me.

[evrardjp]

if the reboot is Blocked by a pod or prometheus, shouldn't we still unmark the node?
For me it means that this node was voluntarily put out of the loop by an administrator, even if the reboot is required during the maintenance window.

In this case, I think that rebootBlocked should also remove any existing NoSchedule taint.
On the other hand, one could argue that a node "out of the loop" shouldn't be touched at all.

In any case, this decision should be recorded somewhere (in a comment, or in the commit message).

[bboreham]

Being blocked by prometheus can be from a "pending" alert: in my case I see those come and go quite often.
So I think it would be bad to oscillate the taint; I would say once you need to reboot you shouldn't be scheduling new pods onto that node. (But it's only "preferred" so may get some if necessary).

Maybe we should make the taint "" by default, so people can choose to try out the feature and report back?

[evrardjp]

I am fine with both parts of your answer.

[evrardjp]

@damoon do you mind updating the taint to "" by default?

[evrardjp]

Alternatively, we can merge this after 16.0, and I can do another PR with updating the default taint.

[damoon]

I changed the flag to be disabled by default.

[damoon]

Thinking about the rebootBlocked() cases.

Situation: A node that waits for a long time to reboot, but is blocked by other rebooting nodes. Then a pod, that blocks reboots, is scheduled onto that node.
A) The taint is remove. As a result even more work gets scheduled here, and needs to be drained again. This should be avoided.
B) The taint stays. As a result the nodes without the taint get a higher average load, even so this node can not reboot once it is its turn to do so. This should also be avoided.
The same should be true for prometheus alert.

My feeling is, to leave a node, that wants to reboot, but is blocked to do so, tainted. It expresses the intend best.
As mentioned, the taint is "only" preferred, and in case B i trust the kubernetes scheduler and resource requests enough for uneven load not be a real issue.

[evrardjp]

I agree.

[bboreham]

lgtm, thanks!

[dholbach]

Nice work and thanks for hanging in there.

Do you think you could add a note #295 about this feature?