Add NotReady taint to new nodes during admission #73097

bsalamat · 2019-01-19T00:34:41Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes a race condition between node controller applying taints for new nodes and the scheduler using new nodes for scheduling pods by adding not ready taint to new nodes at admission time.

Which issue(s) this PR fixes:

Fixes #72129

Special notes for your reviewer:
My understanding is that we do not need to add the taint in node updates, mainly because node condition updates may arrive anytime and can race with other K8s operations. For example, pods may be scheduled on nodes that may become unavailable shortly afterwards in normal condition and K8s handles those scenarios well. The bigger problem that we are trying to fix is to avoid placing pods on newly added nodes.

UPDATE: Given that tainting a node at update reduces wasted work and provides better experience with respect to scheduling pods on more reliable nodes, I added the logic to taint nodes at updates as well.

Does this PR introduce a user-facing change?:

A new `TaintNodesByCondition` admission plugin taints newly created Node objects as "not ready", to fix a race condition that could cause pods to be scheduled on new nodes before their taints were updated to accurately reflect their reported conditions. This admission plugin is enabled by default if the `TaintNodesByCondition` feature is enabled.

/sig scheduling
/sig node
/priority critical-urgent

bsalamat · 2019-01-19T00:35:10Z

cc/ @ravisantoshgudimetla @k82cn

ravisantoshgudimetla · 2019-01-19T02:03:45Z

For example, pods may be scheduled on nodes that may become unavailable shortly afterwards in normal condition and K8s handles those scenarios well.

Yeah, that is right, eventually the pods may get evicted from those nodes which become unavailable over time, also the pods get scheduled onto those kind of nodes - more on that scenario can be found here: #72548 (comment)

ravisantoshgudimetla · 2019-01-19T02:08:50Z

plugin/pkg/admission/noderestriction/admission.go

nit: s/the taint/the NotReadytaint.

the comment could be further expanded to something like the new node may receive taint from nodecontroller with some delay..

ravisantoshgudimetla · 2019-01-19T02:11:42Z

plugin/pkg/admission/noderestriction/admission_test.go

nit: Probably a test to ensure we don't add this taint, if the taint already exists.

liggitt · 2019-01-19T03:07:22Z

I think we'd want this in its own admission plugin that we could enable by default here:

kubernetes/pkg/kubeapiserver/options/plugins.go

Line 132 in b6fb157

defaultOnPlugins := sets.NewString(

liggitt · 2019-01-19T03:55:08Z

plugin/pkg/admission/noderestriction/admission.go

Isn't there an existing key for the notready taint the node controller manages?

There is one in scheduler API, but I didn't want to create a dependency to the API only for this label.

resouer · 2019-01-21T18:57:37Z

@answer1991 Please take a look, this patch is critical and align with your original proposal.

resouer · 2019-01-21T19:25:32Z

plugin/pkg/admission/noderestriction/admission.go

And we may also need to point out who is expected to un-taint the node in the future (NodeLifecycleController etc).

bsalamat · 2019-01-22T22:59:24Z

Thanks @liggitt, @ravisantoshgudimetla, @resouer for your comments and reviews. I addressed all the comments. PTAL.

resouer · 2019-01-23T00:54:04Z

Seems still some golints errors to fix. :-)

bsalamat · 2019-01-23T02:53:52Z

The bigger problem is the fact that a NotReady taint is removed in a node update causing TestNodeAuthorizer to fail. I added an experimental commit to let a change of NotReady taint to be accepted, but I don't feel good about this change.
@liggitt what would be the recommended way of addressing this issue?

liggitt · 2019-01-24T22:40:53Z

@liggitt While I agree with you, it is not the case and I don't think we should change that here. We need to cherrypick this to older releases. I don't want to make a risky change to our tests in older releases.

then the safest alternative would seem to be to disable this admission plugin specifically for the node-e2e test, if it wants to run in a partial environment

yujuhong · 2019-01-24T22:59:50Z

I'd expect the node-e2e to run in a relatively realistic environment w.r.t. the controllers that interact with nodes and pods on nodes. That seems useful for flushing out unexpected interactions.

Node e2e was introduced mainly to test kubelet and things below that in different environments (e.g., various os images). The apiserver was added simply to make interacting with kubelet easier. The node e2e suite helps us identify node failures easily (and also allows us to change the kubelet configuration easily to test different features on the node).

The interaction between kubelet and controllers are tested in cluster e2e. It may be useful to have a simpler environment to test control plane on the master with nodes, but that wasn't what node e2e was created for.

then the safest alternative would seem to be to disable this admission plugin specifically for the node-e2e test, if it wants to run in a partial environment

+1 to disable this.

bsalamat · 2019-01-24T23:03:08Z

Thanks, @yujuhong!

I just pushed a commit to disable the new admission plugin in node e2e tests.

resouer

/lgtm

let's see how CI says then :-)

…le controller does not run in the tests

ravisantoshgudimetla · 2019-01-25T00:08:57Z

/lgtm

resouer · 2019-01-25T00:17:19Z

/lgtm
/hold cancel

Thanks ALL working on this issue!

fejta-bot · 2019-01-25T05:36:44Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

tedyu · 2019-01-25T18:48:44Z

plugin/pkg/admission/nodetaint/admission.go

+}
+
+func addNotReadyTaint(node *api.Node) {
+	notReadyTaint := api.Taint{


It seems this can be declared as singleton outside the func

k8s-ci-robot · 2019-01-26T00:44:29Z

@bsalamat: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e	9b780d3537bf488fe895f65ec0a2239949b322a2	link	`/test pull-kubernetes-node-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…97-upstream-release-1.12 Automated cherry pick of #73097 upstream release 1.12

…97-upstream-release-1.13 Automated cherry pick of #73097 upstream release 1.13

bsalamat requested a review from liggitt January 19, 2019 00:34

bsalamat assigned liggitt Jan 19, 2019

k8s-ci-robot requested review from mikedanese and tallclair January 19, 2019 00:35

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jan 19, 2019

ravisantoshgudimetla reviewed Jan 19, 2019

View reviewed changes

liggitt reviewed Jan 19, 2019

View reviewed changes

resouer reviewed Jan 21, 2019

View reviewed changes

bsalamat force-pushed the fix_taint_nodes branch from 3137cec to 02c0e87 Compare January 22, 2019 22:56

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jan 22, 2019

bsalamat force-pushed the fix_taint_nodes branch 3 times, most recently from 77107aa to f2b0959 Compare January 22, 2019 23:31

bsalamat force-pushed the fix_taint_nodes branch from f2b0959 to b91be7d Compare January 23, 2019 02:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2019

resouer approved these changes Jan 24, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2019

Disable TaintNodesByCondition in node e2e tests, because node lifecyc…

37be9f5

…le controller does not run in the tests

bsalamat force-pushed the fix_taint_nodes branch from 9b780d3 to 37be9f5 Compare January 24, 2019 23:42

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2019

This was referenced Jan 25, 2019

Automated cherry pick of #73097 upstream release 1.12 #73298

Merged

Automated cherry pick of #73097 upstream release 1.13 #73296

Merged

k8s-ci-robot merged commit d654b49 into kubernetes:master Jan 25, 2019

This was referenced Jan 25, 2019

Update Taints field comment in the machine spec kubernetes-sigs/cluster-api#651

Merged

Support syncing a set of labels from MachineDeployment/MachineSet/Machine to Nodes kubernetes-sigs/cluster-api#493

Closed

tedyu reviewed Jan 25, 2019

View reviewed changes

ravisantoshgudimetla mentioned this pull request Jan 28, 2019

UPSTREAM: 73097: Add NotReady taint to new nodes during admission openshift/origin#21883

Closed

k8s-ci-robot added a commit that referenced this pull request Jan 29, 2019

Merge pull request #73298 from bsalamat/automated-cherry-pick-of-#730…

fae2376

…97-upstream-release-1.12 Automated cherry pick of #73097 upstream release 1.12

k8s-ci-robot added a commit that referenced this pull request Jan 30, 2019

Merge pull request #73296 from bsalamat/automated-cherry-pick-of-#730…

210452a

…97-upstream-release-1.13 Automated cherry pick of #73097 upstream release 1.13

micafer mentioned this pull request Feb 4, 2019

Pod scheduled in NetworkNotReady node stucks ContainerCreating #73590

Closed

This was referenced Mar 7, 2019

TaintNodesByCondition admission add specified taints to new nodes during admission #75108

Closed

add config file for nodetaint admission #75112

Closed

draveness mentioned this pull request Sep 16, 2019

feat: update taint nodes by condition to GA #82703

Merged

Huang-Wei mentioned this pull request Nov 1, 2019

Enable TaintNodesByCondition admission by default in scripts #84665

Closed

miteshskj mentioned this pull request Nov 6, 2019

Fix #17250 - Add details for TaintNodesByCondition. kubernetes/website#17313

Merged

Add NotReady taint to new nodes during admission #73097

Add NotReady taint to new nodes during admission #73097

Uh oh!

Conversation

bsalamat commented Jan 19, 2019 • edited by liggitt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsalamat commented Jan 19, 2019

Uh oh!

ravisantoshgudimetla commented Jan 19, 2019

Uh oh!

ravisantoshgudimetla Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jan 19, 2019

Uh oh!

liggitt Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

bsalamat Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

resouer commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

resouer Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

bsalamat commented Jan 22, 2019

Uh oh!

resouer commented Jan 23, 2019

Uh oh!

bsalamat commented Jan 23, 2019

Uh oh!

liggitt commented Jan 24, 2019

Uh oh!

yujuhong commented Jan 24, 2019

Uh oh!

bsalamat commented Jan 24, 2019

Uh oh!

resouer left a comment

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Jan 25, 2019

Uh oh!

resouer commented Jan 25, 2019

Uh oh!

fejta-bot commented Jan 25, 2019

Uh oh!

tedyu Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bsalamat commented Jan 19, 2019 •

edited by liggitt

Loading

ravisantoshgudimetla Jan 19, 2019 •

edited

Loading

resouer commented Jan 21, 2019 •

edited

Loading