TaintController #40355

gmarek · 2017-01-24T11:15:08Z

This PR adds a manager to NodeController that is responsible for removing Pods from Nodes tainted with NoExecute Taints. This feature is beta (as the rest of taints) and enabled by default. It's gated by controller-manager enable-taint-manager flag.

k8s-reviewable · 2017-01-24T11:15:14Z

This change is

gmarek · 2017-01-24T15:52:15Z

cc @davidopp @wojtek-t @kevin-wangzefeng @timothysc

timothysc · 2017-01-24T16:15:41Z

pkg/controller/node/nodecontroller.go

@@ -141,6  141,8 @@ type NodeController struct {
 	daemonSetStore listers.StoreToDaemonSetLister
 	// allocate/recycle CIDRs for node if allocateNodeCIDRs == true
 	cidrAllocator CIDRAllocator
+	// manages taints
+	taintManager *TaintController


Please add a detailed description (from our emails) of why the TaintController is special cased to the NodeController, as this is different then all other controllers.

/cc @aveshagarwal

It's a very, very early untested prototype.

So don't expect comments, or anything;)

It's further along now so I think comment is appropriate.

Yup - I'm applying all comments today. @timothysc - do you have a piece of email I can paste here (I wasn't taking part in those discussions)

kevin-wangzefeng · 2017-01-25T15:06:41Z

pkg/controller/node/taint_controller.go

+				continue
+			}
+			podNamespacedName := types.NamespacedName{Namespace: pods[i].Namespace, Name: pods[i].Name}
+			if len(tolerations) == 0 {


Assume there is a node nodeA tainted with taint {foo=bar:NoSchedule}, and pod podA which has no tolerations is running on nodeA, according to the check here, it results in podA evicted.
Actually what we want is just evict pods those don't tolerate NoExecute taints of nodes, or am I missing something?

We do filter everything except NoExecute taints in XUpdated functions, so no other taints appear in the rest of the code (I hope).

I see, that's fine.

gmarek · 2017-01-25T16:16:00Z

I added unit tests. Only e2e test, flag gate in NC and logging tweaks are needed to finish this PR. I hope to finish it tomorrow. @kevin-wangzefeng @davidopp @wojtek-t @timothysc

Oh, and we need way more pkg/controller approvers: @bprashanth @mikedanese @derekwaynecarr @deads2k - probably split by controller.

gmarek · 2017-01-26T15:06:29Z

I'm getting errors:

I0126 15:04:54.761] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:51: undefined: CertificateSigningRequest
I0126 15:04:54.762] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:62: undefined: CertificateSigningRequest
I0126 15:04:54.762] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:66: undefined: CertificateSigningRequest
I0126 15:04:54.763] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:77: undefined: CertificateSigningRequest
I0126 15:04:54.764] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:81: undefined: CertificateSigningRequestCondition
I0126 15:04:54.764] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:89: undefined: CertificateSigningRequestCondition
I0126 15:04:54.765] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:93: undefined: CertificateSigningRequestCondition
I0126 15:04:54.766] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:101: undefined: CertificateSigningRequestCondition
I0126 15:04:54.766] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:105: undefined: CertificateSigningRequestList
I0126 15:04:54.767] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:111: undefined: CertificateSigningRequestList
I0126 15:04:54.768] k8s.io/kubernetes/pkg/apis/certificates/v1alpha1/zz_generated.conversion.go:111: too many errors

@mikedanese @liggitt @cjcullen - any ideas?

bprashanth · 2017-01-26T15:15:05Z

Re #40355 (comment), there's an OWNERS file per dir, the top lever owners are just for those directories that don't have one, I think. Feel free to add people to it.

liggitt · 2017-01-26T15:27:56Z

the certificates api dropped v1alpha1 types in #39772

timothysc · 2017-02-08T23:45:30Z

@wojtek-t @davidopp I only have a minor nit on testing now. I'd like to get this one out of the way and move on now that I'm unblocked.

timothysc · 2017-02-09T15:32:41Z

please squash, and we need to get the tests green again.

k8s-github-robot · 2017-02-10T01:14:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: gmarek, mikedanese

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-controller-manager/OWNERS~~ [mikedanese]
hack/OWNERS
pkg/api/v1/OWNERS
~~pkg/apis/componentconfig/OWNERS~~ [mikedanese]
~~pkg/controller/node/OWNERS~~ [gmarek,mikedanese]
~~test/OWNERS~~ [mikedanese]

We suggest the following people:
cc @deads2k @vishh @smarterclayton
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

gmarek · 2017-02-10T01:15:15Z

@timothysc @wojtek-t squashed commits.

timothysc · 2017-02-10T02:39:10Z

Reviewed 5 of 17 files at r7, 1 of 2 files at r10, 6 of 8 files at r12, 2 of 2 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 38 unresolved discussions.

Comments from Reviewable

k8s-github-robot · 2017-02-10T12:50:42Z

Automatic merge from submit-queue (batch tested with PRs 39418, 41175, 40355, 41114, 32325)

davidopp · 2017-02-12T01:00:04Z

cmd/kube-controller-manager/app/options/options.go

@@ -104,6  104,7 @@ func NewCMServer() *CMServer {
 			ClusterSigningCertFile:   "/etc/kubernetes/ca/ca.pem",
 			ClusterSigningKeyFile:    "/etc/kubernetes/ca/ca.key",
 			ReconcilerSyncLoopPeriod: metav1.Duration{Duration: 5 * time.Second},
+			EnableTaintManager:       true,


Right, this should be true.

@gmarek What are you referring to as the first of the three PRs -- is it #41068 ?

davidopp · 2017-02-12T01:18:11Z

cmd/kube-controller-manager/app/options/options.go

@@ -196,6  197,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.Float32Var(&s.UnhealthyZoneThreshold, "unhealthy-zone-threshold", 0.55, "Fraction of Nodes in a zone which needs to be not Ready (minimum 3) for zone to be treated as unhealthy. ")
 	fs.BoolVar(&s.DisableAttachDetachReconcilerSync, "disable-attach-detach-reconcile-sync", false, "Disable volume attach detach reconciler sync. Disabling this may cause volumes to be mismatched with pods. Use wisely.")
 	fs.DurationVar(&s.ReconcilerSyncLoopPeriod.Duration, "attach-detach-reconcile-sync-period", s.ReconcilerSyncLoopPeriod.Duration, "The reconciler sync wait time between volume attach detach. This duration must be larger than one second, and increasing this value from the default may allow for volumes to be mismatched with pods.")
+	fs.BoolVar(&s.EnableTaintManager, "enable-taint-manager", s.EnableTaintManager, "WARNING: Beta feature. If set to true enables NoExecute Taints and will evict all not-tolerating Pod running on Nodes tainted with this kind of Taints.")


Is it really necessary to make this be a flag? The code would be simpler without it, and beta is enabled by default... I think if this feature is broken we would do a patch release, not just disable it.

davidopp · 2017-02-12T01:31:03Z

pkg/controller/node/nodecontroller.go

@@ -312,6  356,9 @@ func NewNodeController(
 						utilruntime.HandleError(fmt.Errorf("Error allocating CIDR: %v", err))
 					}
 				}
+				if nc.taintManager != nil {


I don't think we need to make taintManager conditional. If it is broken we will do a patch release. Making it unconditional will simplify the code.

davidopp · 2017-02-12T01:46:05Z

test/utils/runners.go

+}
+
+func RunPodAndGetNodeName(c clientset.Interface, pod *v1.Pod, timeout time.Duration) (string, error) {
+	retries := 5


this is already defined as a top-level const, so delete this?

davidopp · 2017-02-12T01:51:46Z

pkg/controller/node/taint_controller.go

+	}
+}
+
+func getNonExecuteTaints(taints []v1.Taint) []v1.Taint {


why Non? getNoExecuteTaints would be better

davidopp · 2017-02-12T05:26:11Z

pkg/controller/node/timed_workers.go

+	}
+}
+
+// TimedWorkerQueue keeps a set of TimedWorkers that still wait for execution.


s/still wait/are still waiting/

davidopp · 2017-02-12T05:26:44Z

pkg/controller/node/timed_workers.go

+// TimedWorkerQueue keeps a set of TimedWorkers that still wait for execution.
+type TimedWorkerQueue struct {
+	sync.Mutex
+	workers  map[string]*TimedWorker


add a comment what is the map key

davidopp · 2017-02-12T05:41:50Z

pkg/controller/node/timed_workers.go

+		q.Lock()
+		defer q.Unlock()
+		if err == nil {
+			q.workers[key] = nil


Why don't you delete(q.workers, key) in this case?

To avoid thrashing - it's enough to delete Pod once, so I keep the key as a way to dedup calls.

davidopp · 2017-02-12T05:54:42Z

test/e2e/taints_test.go

+	}
+}
+
+// Creates and startes a controller (informer) that watches updates on a pod in given namespace with given name. It puts a new


s/startes/starts/

davidopp · 2017-02-12T06:17:32Z

test/e2e/taints_test.go

@@ -0,0  1,335 @@
+/*


I know it's a lot of work but can you add a test with three pods on the same node

one tolerates T without tolerationSeconds

one tolerates T for some tolerationSeconds==t

one does not tolerate T

Then add taint T to the node and verify that the first one is never evicted, second is evicted after t, and third is evicted immediately?

A couple of other ideas for tests

same pod configuration as above, but add two taints T and S, and verify all three pods are evicted immediately

a pod that tolerates two taints, with different tolerationSeconds, then add both taints and verify that the pod is evicted after the min of the tolerationSeconds

davidopp · 2017-02-12T06:58:16Z

pkg/controller/node/taint_controller.go

+	return func(args *WorkArgs) error {
+		ns := args.NamespacedName.Namespace
+		name := args.NamespacedName.Name
+		glog.V(0).Infof("NoExecuteTaintManager is deleting Pod: %v", args.NamespacedName.String())


Also add an event here?

davidopp · 2017-02-12T07:01:43Z

pkg/controller/node/timed_workers.go

+}
+
+// AddWork adds a work to the WorkerQueue which will be executed not earlier than `fireAt`.
+func (q *TimedWorkerQueue) AddWork(args *WorkArgs, createdAt time.Time, fireAt time.Time) {


Maybe add V(4) logging at entry to this function?

davidopp · 2017-02-12T07:01:59Z

pkg/controller/node/timed_workers.go

+}
+
+// CancelWork removes scheduled function execution from the queue.
+func (q *TimedWorkerQueue) CancelWork(key string) {


Maybe add V(4) logging at entry to this function?

davidopp · 2017-02-12T19:23:36Z

One other question -- I'm a bit rusty on how the Informer stuff works. Is it completely edge-based? That is, when node controller starts up, taint controller doesn't do anything until a pod or node is created/updated/deleted? Or is it level-based at startup, i.e. it somehow triggers the callbacks for every object in the system on startup? I think the behavior we want is that when NC starts up, it scans all the nodes and enqueues the relevant pods for eviction based on the node's taints and the pods' tolerations.

liggitt · 2017-02-12T19:31:16Z

On startup, it gets an Add event for every existing object

gmarek · 2017-02-13T08:49:27Z

@davidopp - I'll create a PR applying all your comments today. As for your question I think that what @liggitt wrote should suffice.

davidopp · 2017-02-13T09:18:31Z

Thanks. And yeah, what @liggitt wrote answered my question.

gmarek added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jan 24, 2017

gmarek self-assigned this Jan 24, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 24, 2017

gmarek force-pushed the nc-taint-handling branch 2 times, most recently from 2eb71de to 5b33298 Compare January 24, 2017 15:51

timothysc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jan 24, 2017

timothysc reviewed Jan 24, 2017

View reviewed changes

gmarek force-pushed the nc-taint-handling branch from 5b33298 to 796cf76 Compare January 24, 2017 16:23

timothysc added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jan 24, 2017

kevin-wangzefeng reviewed Jan 25, 2017

View reviewed changes

gmarek force-pushed the nc-taint-handling branch from 796cf76 to dff53b8 Compare January 25, 2017 16:07

gmarek force-pushed the nc-taint-handling branch from dff53b8 to 4969754 Compare January 25, 2017 16:22

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2017

gmarek force-pushed the nc-taint-handling branch 5 times, most recently from 08d4aff to 92f79c9 Compare January 26, 2017 15:02

timothysc added this to the v1.6 milestone Feb 8, 2017

timothysc approved these changes Feb 8, 2017

View reviewed changes

gmarek force-pushed the nc-taint-handling branch from 457189c to 523f470 Compare February 9, 2017 10:32

gmarek mentioned this pull request Feb 9, 2017

Promote taint helpers from e2e/framework to pkg/controller/controller_utils #41014

Merged

davidopp mentioned this pull request Feb 9, 2017

Requirements for taints, tolerations, dedicated nodes to graduate to Beta and then v1 #25320

Closed

12 tasks

gmarek added 2 commits February 10, 2017 02:13

Taint controller - first commit

2f0e436

generated

004552f

gmarek force-pushed the nc-taint-handling branch from 523f470 to 004552f Compare February 10, 2017 01:14

timothysc added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 10, 2017

k8s-github-robot merged commit a7a74b5 into kubernetes:master Feb 10, 2017

davidopp reviewed Feb 12, 2017

View reviewed changes

davidopp mentioned this pull request Feb 12, 2017

Change taints/tolerations to api fields #38957

Merged

davidopp mentioned this pull request Feb 15, 2017

Apply davidopps comments to TaintController PR #41311

Merged

gmarek deleted the nc-taint-handling branch February 21, 2017 14:17

davidopp mentioned this pull request Feb 28, 2017

Taint Based Eviction kubernetes/enhancements#166

Closed

endocrimes mentioned this pull request May 31, 2022

NoExecuteTaintManager won't modify tolerationSeconds when Pod enqueued #109226

Closed

TaintController #40355

TaintController #40355

Conversation

gmarek commented Jan 24, 2017 • edited Loading

k8s-reviewable commented Jan 24, 2017

gmarek commented Jan 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Jan 25, 2017

gmarek commented Jan 26, 2017 • edited Loading

bprashanth commented Jan 26, 2017

liggitt commented Jan 26, 2017

timothysc commented Feb 8, 2017

timothysc commented Feb 9, 2017

k8s-github-robot commented Feb 10, 2017

gmarek commented Feb 10, 2017 • edited Loading

timothysc commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp Feb 12, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp commented Feb 12, 2017

liggitt commented Feb 12, 2017

gmarek commented Feb 13, 2017

davidopp commented Feb 13, 2017

gmarek commented Jan 24, 2017 •

edited

Loading

gmarek commented Jan 26, 2017 •

edited

Loading

gmarek commented Feb 10, 2017 •

edited

Loading

davidopp Feb 12, 2017 •

edited

Loading