Added unschedulable taint #61161

k82cn · 2018-03-14T07:18:10Z

Signed-off-by: Da K. Ma klaus1982.cn@gmail.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #59194; fixes #61050

Release note:

When `TaintNodesByCondition` enabled, added `node.kubernetes.io/unschedulable:NoSchedule`
 taint to the node if `spec.Unschedulable` is true.

When `ScheduleDaemonSetPods` enabled, `node.kubernetes.io/unschedulable:NoSchedule` 
toleration is added automatically to DaemonSet Pods; so the `unschedulable` field of 
a node is not respected by the DaemonSet controller.

k82cn · 2018-03-14T10:28:08Z

/cc @janetkuo @bsalamat

liggitt · 2018-03-14T17:26:38Z

pkg/controller/nodelifecycle/node_lifecycle_controller.go

@@ -438,7 +438,20 @@ func (nc *Controller) doNoScheduleTaintingPass(node *v1.Node) error {
 			}
 		}
 	}
+	if node.Spec.Unschedulable {
+		// If unscheduable, append related taint.


what removes this taint when the node becomes schedulable again?

definitely needs a test demonstrating this lifecycle works properly

Good point. This might break kubectl cordon/uncordon too.

@janetkuo +1 as we need to ensure how the taint is removed when kubectl uncordon

The removal is handled here: https://github.com/kubernetes/kubernetes/pull/61161/files#diff-43035107687eb30550696751ac1066e4R458 . we'll get target taints according to the conditions, and the exist taints on the node; and then get taintsToAdd and taintsToDel by TaintSetDiff. SwapNodeControllerTaint will update the taints accordingly.

re a test: definitely :)

added integration test for unschedulable; tested removal manually.

bsalamat

When Unschedulable field is cleared, we need to remove the "NoSchedule" taint.

bsalamat · 2018-03-14T17:40:32Z

pkg/controller/daemon/util/daemonset_util.go

+
+// AppendUnscheduableTaintIfNotExist appends unscheduable toleration to `.spec` if not exist; otherwise,
+// no changes to `.spec.tolerations`.
+func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {


Please rename to AppendNoScheduleTolerationIfNotExist.

bsalamat · 2018-03-14T17:58:35Z

pkg/controller/daemon/util/daemonset_util.go

+func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {
+	unscheduableTaintExist := false
+	for _, t := range tolerations {
+		if t.Key == algorithm.TaintNodeUnscheduable {


Just the key being equal to the taint key does not necessarily mean that the toleration is the right one that we need. It could have an operator or a different effect. We should check if the key is the same, operator is "Exists" and effect is "NoSchedule".

@bsalamat - we should create a function under util/toleration that checks if a toleration exists in a slice of tolerations, IIRC we’re doing the same for taints.

bsalamat · 2018-03-14T18:00:00Z

pkg/controller/daemon/util/daemonset_util.go

+
+	if !unscheduableTaintExist {
+		tolerations = append(tolerations, v1.Toleration{
+			Key:    algorithm.TaintNodeUnscheduable,


I think we need to set operator to "Exists" as well.

bsalamat · 2018-03-14T18:05:54Z

pkg/scheduler/algorithm/well_known_labels.go

+	// TaintNodeUnscheduable will be added when node becomes unscheduable
+	// and feature-gate for TaintNodesByCondition flag is enabled,
+	// and removed when node becomes scheduable.
+	TaintNodeUnscheduable = "node.kubernetes.io/unscheduable"


s/TaintNodeUnscheduable/TaintNodeUnschedulable

s/unscheduable/unschedulable

good catch!

krzyzacy · 2018-03-14T21:20:51Z

/milestone v1.10

jdumars · 2018-03-14T21:26:55Z

/sig node

dims · 2018-03-14T21:26:57Z

pkg/controller/daemon/util/daemonset_util.go

+// AppendUnscheduableTaintIfNotExist appends unscheduable toleration to `.spec` if not exist; otherwise,
+// no changes to `.spec.tolerations`.
+func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {
+	unscheduableTaintExist := false


typo please rename tounschedulableTaintExist

dims · 2018-03-14T21:30:27Z

@k82cn is this related to #60763 ?

jdumars · 2018-03-14T21:34:34Z

@dims see comment from @janetkuo in #61050

janetkuo · 2018-03-14T21:47:31Z

is this related to #60763?

@dims it's not. #60763 is not caused by DaemonSet, see #60763 (comment)

liggitt · 2018-03-14T22:54:14Z

@krzyzacy is this release blocking? it looks like this is just changing function behind alpha gates, and is still WIP. Should it move out of the milestone?

edit: I see it is fixing alpha function for the alpha feature CI job

dims · 2018-03-16T02:09:44Z

@mikedanese ping for pkg/controller/daemon/ approval
@gmarek @bowei ping for pkg/controller/nodelifecycle approval

k82cn · 2018-03-16T02:35:47Z

/assign @gmarek

k8s-github-robot · 2018-03-16T02:36:38Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@bsalamat @gmarek @janetkuo @k82cn

Pull Request Labels

sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

bsalamat · 2018-03-16T04:11:18Z

/lgtm

gmarek · 2018-03-16T13:16:30Z

LGTM for NodeController.

/approve

jberkus · 2018-03-16T16:04:50Z

@mikedanese @janetkuo can one of you approve this PR please?

dims · 2018-03-16T17:11:01Z

@kow3ns does this look good to you too?

janetkuo · 2018-03-16T17:23:44Z

/approve

k8s-ci-robot · 2018-03-16T17:23:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, gmarek, janetkuo, k82cn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/daemon/OWNERS~~ [janetkuo]
~~pkg/controller/nodelifecycle/OWNERS~~ [gmarek,janetkuo]
~~pkg/scheduler/OWNERS~~ [bsalamat,k82cn]
~~test/integration/scheduler/OWNERS~~ [bsalamat,gmarek,janetkuo,k82cn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-03-16T17:24:25Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-03-16T18:22:05Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from ingvagabund and wojtek-t March 14, 2018 07:18

k8s-ci-robot requested review from bsalamat and janetkuo March 14, 2018 10:28

k82cn force-pushed the k8s_59194_4 branch from a96b704 to 9ac3f7d Compare March 14, 2018 12:46

liggitt reviewed Mar 14, 2018

View reviewed changes

bsalamat reviewed Mar 14, 2018

View reviewed changes

janetkuo mentioned this pull request Mar 14, 2018

[test failed] gci-gce-alpha-features #61050

Closed

k8s-ci-robot added this to the v1.10 milestone Mar 14, 2018

k8s-github-robot added the milestone/incomplete-labels label Mar 14, 2018

janetkuo added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/bug Categorizes issue or PR as related to a bug. labels Mar 14, 2018

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Mar 14, 2018

dims reviewed Mar 14, 2018

View reviewed changes

janetkuo added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed sig/node Categorizes an issue or PR as relevant to SIG Node. labels Mar 14, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels Mar 14, 2018

bsalamat mentioned this pull request Mar 14, 2018

Doc of ScheduleDaemonSetPods (alpha in 1.10) kubernetes/website#7678

Closed

k82cn mentioned this pull request Mar 16, 2018

Duplicated toleration output in kubectl describe #61250

Closed

k82cn force-pushed the k8s_59194_4 branch from bae1f72 to b23db30 Compare March 16, 2018 01:35

k8s-ci-robot assigned gmarek Mar 16, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2018

janetkuo approved these changes Mar 16, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2018

k8s-github-robot merged commit ca02c11 into kubernetes:master Mar 16, 2018

bsalamat mentioned this pull request Mar 16, 2018

Move Priority and Preemption to Beta #57471

Closed

13 tasks

jberkus mentioned this pull request Mar 16, 2018

Schedule DaemonSet Pods by default scheduler #59194

Closed

7 tasks

k82cn deleted the k8s_59194_4 branch March 16, 2018 22:09

k82cn mentioned this pull request Apr 3, 2018

Make NodeSpec.unschedulable a Taint #29178

Closed

yujuhong mentioned this pull request May 15, 2018

Race condition when applying the unschedulable taint #63897

Closed

wackxu mentioned this pull request Jul 26, 2018

remove unused predicate CheckNodeUnschedulablePred #66641

Closed

wking mentioned this pull request Aug 3, 2018

Make "kubectl drain" use taint instead of Unschedulable #44944

Closed

janetkuo mentioned this pull request Aug 21, 2018

Pending pod scheduled onto master node when ScheduleDaemonSetPods is enabled #66348

Closed

Huang-Wei mentioned this pull request Aug 24, 2018

apply unschedulable:NoSchedule taint to Daemonset pods #67850

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unschedulable taint #61161

Added unschedulable taint #61161

k82cn commented Mar 14, 2018 •

edited

Loading

k82cn commented Mar 14, 2018

liggitt Mar 14, 2018

liggitt Mar 14, 2018

janetkuo Mar 14, 2018

yastij Mar 14, 2018

k82cn Mar 15, 2018

k82cn Mar 15, 2018

bsalamat left a comment

bsalamat Mar 14, 2018 •

edited

Loading

bsalamat Mar 14, 2018

yastij Mar 14, 2018

bsalamat Mar 14, 2018

bsalamat Mar 14, 2018

janetkuo Mar 14, 2018

krzyzacy commented Mar 14, 2018

jdumars commented Mar 14, 2018

dims Mar 14, 2018

dims commented Mar 14, 2018

jdumars commented Mar 14, 2018

janetkuo commented Mar 14, 2018

liggitt commented Mar 14, 2018 •

edited

Loading

dims commented Mar 16, 2018

k82cn commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

bsalamat commented Mar 16, 2018

gmarek commented Mar 16, 2018

jberkus commented Mar 16, 2018

dims commented Mar 16, 2018

janetkuo commented Mar 16, 2018

k8s-ci-robot commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

Added unschedulable taint #61161

Added unschedulable taint #61161

Conversation

k82cn commented Mar 14, 2018 • edited Loading

k82cn commented Mar 14, 2018

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

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat Mar 14, 2018 • 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

krzyzacy commented Mar 14, 2018

jdumars commented Mar 14, 2018

Choose a reason for hiding this comment

dims commented Mar 14, 2018

jdumars commented Mar 14, 2018

janetkuo commented Mar 14, 2018

liggitt commented Mar 14, 2018 • edited Loading

dims commented Mar 16, 2018

k82cn commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

bsalamat commented Mar 16, 2018

gmarek commented Mar 16, 2018

jberkus commented Mar 16, 2018

dims commented Mar 16, 2018

janetkuo commented Mar 16, 2018

k8s-ci-robot commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

k8s-github-robot commented Mar 16, 2018

k82cn commented Mar 14, 2018 •

edited

Loading

bsalamat Mar 14, 2018 •

edited

Loading

liggitt commented Mar 14, 2018 •

edited

Loading