WIP: implement forgiveness phase 1 #34825

kevin-wangzefeng · 2016-10-14T14:25:46Z

Why:
This PR is to implement forgiveness phase 1 and features it depends.
Related issue: #1574
Refreshes Implementation: #30191, also incorporates #32498

What changes made in this PR:

update toleration api types to support forgiveness, added a new field forgivenessSeconds to indicate the duration of time it tolerates a taint.
update taint api types, added a new field to indicate the time the taint is added.
automatically add taint notready:NoExecute with timestamp to a node when it goes not ready, and remove it when it turns ready.
automatically add taint unreachable:NoExecute with timestamp to a node when it becomes unknown, and remove it when it turns back ready.
periodically check taints of a node and pods running on it, evict those not tolerate NoExecute taints.
make taints-tolerations matching respect timestamps, useful in forgiveness cases.
make tolerations respect wildcard key
add a new admission-controller to set default forgiveness toleration for pods (that has no forgiveness tolerations) to tolerate taints notready:NoExecute and unreachable:NoExecute for 5 minutes.

Release note:

implement forgiveness alpha verion 
supports taint based pod eviction at runtime

This change is

aveshagarwal · 2016-10-14T15:07:05Z

@kevin-wangzefeng am I right that this PR obsoletes #32498 and #30191 ?

aveshagarwal · 2016-10-14T15:10:28Z

@kevin-wangzefeng Also if you need help with the last item (add a new admission-controller), let me know.

kevin-wangzefeng · 2016-10-14T15:11:20Z

@kevin-wangzefeng am I right that this PR obsoletes #32498 and #30191 ?

Yes, I think so.

kevin-wangzefeng · 2016-10-14T15:13:35Z

There are still some changes need to be finished, but the main changes are ready for reivew.
/cc @davidopp @kubernetes/sig-scheduling

aveshagarwal · 2016-10-14T15:28:10Z

pkg/api/types.go

+	// of effect NoExecute, otherwise this field is ignored) tolerates the taint. By default,
+	// it is not set, which means tolerate the taint forever (do not evict). Zero and
+	// negative values are not allowed.
+	ForgivenessSeconds *int64 `json:"forgivenessSeconds,omitempty"`


ForgivenessPeriod or ForgivenessDuration?

ForgivenessPeriodSeconds ? I like that the seconds is in the var name, otherwise easy to confuse w/ millisec

The pattern we've used elsewhere is XXXSeconds. I think ForgivenessSeconds is fine.

aveshagarwal · 2016-10-14T15:56:34Z

pkg/api/validation/validation.go

-			// string(api.TaintEffectNoScheduleNoAdmit),
-			// string(api.TaintEffectNoScheduleNoAdmitNoExecute),
+			string(api.TaintEffectNoExecute),
+			// TODO: Uncomment this block when implement TaintEffectNoAdmit.


Isn't it NoScheduleNoAdmit (based on the comment #25320 (comment))?

It is changed in prior offline discussion with @davidopp , I will update to the tracking issue as well.

Ops, I just checked, it not supposed to change (should still be NoScheduleNoAdmit), I will discard it.

aveshagarwal · 2016-10-14T16:23:50Z

pkg/api/helpers.go

 	tolerated := false
 	for i := range tolerations {
-		if TolerationToleratesTaint(&tolerations[i], taint) {
+		if tolerations[i].ToleratesTaint(taint) {
 			tolerated = true


You could just return here: return true.

aveshagarwal · 2016-10-14T16:24:06Z

pkg/api/helpers.go

 	tolerated := false
 	for i := range tolerations {
-		if TolerationToleratesTaint(&tolerations[i], taint) {
+		if tolerations[i].ToleratesTaint(taint) {
 			tolerated = true
 			break
 		}
 	}
 	return tolerated


You could just return here false.

aveshagarwal · 2016-10-14T16:37:45Z

pkg/api/helpers.go

+	if t.ForgivenessSeconds != nil {
+		// taint with no added time indicated can only be tolerated
+		// by toleration with no forgivenessSeconds.
+		if taint.AddedTime.IsZero() {


I would test for taint.Effect!=NoExecute then return false.

Per #28082 (comment) comment

aveshagarwal · 2016-10-14T16:56:21Z

pkg/api/helpers_test.go

+			expectTolerated: true,
+		},
+		{
+			description: "toleration and taint have the same key and effect, and operator is Exists, and taint has some value, expect tolerated",


Could add one more explicit test: "toleration and taint have the same key and effect but different values, and operator is Exists, and expect tolerated"

If toleration.operator is Exists, toleration.value must be empty (ensured in validation), I don't think the test you mentioned is necessary.

aveshagarwal · 2016-10-14T17:03:33Z

pkg/api/helpers_test.go

+			expectTolerated: false,
+		},
+		{
+			description: "toleration with explicit forgiveness can't tolerate taint with no added time, expect not tolerated",


This test is invalid and not really a test for "not tolerated". Because it seems as per validation NoExcute must have addedtime so this kind of test should be part of validation tests IMO.

taint.AddedTime is intended to be not required even when the effect is NoExecute, so that the taint can only be tolerated with forever forgiveness toleration.

aveshagarwal · 2016-10-14T17:08:56Z

pkg/api/validation/validation.go

+				allErrors = append(allErrors, field.Invalid(idxPath.Child("effect"), toleration.Effect, "effect must be NoExecute when `ForgivenessSeconds` is set"))
+			}
+			if toleration.Operator != api.TolerationOpExists {
+				allErrors = append(allErrors, field.Invalid(idxPath.Child("operator"), toleration.Operator, "operator must be Exists when `key` is 'nodeNotReady'"))


"operator must be Exists when key is 'nodeNotReady" does not seem to fit here?

should be "operator must be Exists when forgivenessSeconds is set", thanks

Why can't you use forgiveness with operator Equals ?

I don't think there is any particular reason, just in node notready/unreachable case, taints has no value, what about removing this checking?

I agree you should remove this. Forgiveness should work with both Exists and Equals toleration operators.

aveshagarwal · 2016-10-14T17:12:32Z

pkg/api/validation/validation.go

+			if toleration.Effect != api.TaintEffectNoExecute {
+				allErrors = append(allErrors, field.Invalid(idxPath.Child("effect"), toleration.Effect, "effect must be NoExecute when `ForgivenessSeconds` is set"))
+			}
+			if toleration.Operator != api.TolerationOpExists {


I wonder why this matters for toleration.ForgivenessSeconds?

I agree, this is wrong. Forgiveness should work with both Exists and Equals tolerator operators.

aveshagarwal · 2016-10-14T17:20:15Z

pkg/api/helpers.go

 			tolerated = true
 			break
 		}
 	}
 	return tolerated
 }

+type taintsFilterFunc func(Taint) bool


Although I am yet to look into full PR, I wonder if it could be misused if not guarded properly. For example, a malicious pod ignoring all taints of a privileged node and ends up getting scheduled on that node.

Yes, ideally we would be able to control who can create, update, and delete taints and tolerations. (And we should implement that in the future.) But I don't think we're introducing anything more unsafe than what we already have today. In particular

to modify taint, you need to modify the Node object; but if you can modify the Node object, you can already do a lot of damage today (set node capacity to zero, add NotReady condition, etc.).

to modify toleration, you need to modify the Pod object; if but if you can modify the Pod object, you can already do a lot of damage today (change the image name, set NodeName to an arbitrary value, etc.).

I agree that the "dedicated nodes" use case (the one you are alluding to) definitely can't be safely implemented without some ACL scheme for taints and tolerations.

Other topic than above, but it's github... - you probably can go with *Taint and avoid unnecessary copies.

Other topic than above, but it's github... - you probably can go with *Taint and avoid unnecessary copies.

done

aveshagarwal · 2016-10-14T18:02:52Z

pkg/controller/node/nodecontroller.go

+					Effect:    api.TaintEffectNoExecute,
+					AddedTime: nc.now(),
+				}
+				added, err := tryAddTaintToNode(nc.kubeClient, node.Name, nodeUnreachableTaint)


I think it should be checked if the node has TaintNodeNotReady (if the node's status goes from NotReady to Unreachable) and if the taint exists, it should be removed. I dont think both taints TaintNodeNotReady, TaintNodeUnreachable should exist together.

I don't understand what you mean. What is "Unreachable"?

NodeReady can have three values: True, False and Unknown. True means ready, False and Unknown means that something's wrong. False is generally set by Kubelet to mean that something's wrong (e.g. Docker is down). Unknown means that the Node is Unreachable. Those are different things and I don't understand why we shouldn't have both taints.

@gmarek I meant, at the same time, a node can not have both taints (TaintNodeNotReady, and TaintNodeUnreachable) associated to it. So if the NodeReady changes to Unknown(Unreachable), before assigning TaintNodeUnreachable, its better to check if the node had the taint TaintNodeNotReady (if NodeReady goes from False to Unkown), and if TaintNodeNotReady exists, remove it first, then assign TaintNodeUnreachable.

I agree we should try to mimic the current behavior as closely as possible, to minimize the chance of bugs. Ideally we would have TaintNodeNotReady at the same time when we currently have ConditionFalse, and TaintNodeUnreachable at the same time when we currently have ConditionUnknown. Since we cannot have ConditionFalse and ConditionUnknown at the same time, we would not have both taints a tthe same time.

Fair enough.

aveshagarwal · 2016-10-14T18:08:12Z

pkg/controller/node/nodecontroller.go

+
+// tryRemoveTaintsOffNode tries to remove taints off a node,
+// return true if taints removed, or return false if taints don't exist.
+func tryRemoveTaintsOffNode(kubeClient clientset.Interface, nodeName string, taintsToRemove ...api.Taint) (bool, error) {


There seems lot of common code between tryRemoveTaintsOffNode and tryAddTaintToNode, so could be combined onto one function something like tryModifyNodeTaints with a param like add/remove, or the common code could be put into some other common functions which both tryRemoveTaintsOffNode and tryAddTaintToNode could use.

I would prefer that we delay refactoring for code compactness. Let's get this PR into a state where it actually works and implements all the functionality, and then clean it up.

aveshagarwal · 2016-10-14T18:12:21Z

pkg/controller/node/nodecontroller.go

+					Effect:    api.TaintEffectNoExecute,
+					AddedTime: nc.now(),
+				}
+				added, err := tryAddTaintToNode(nc.kubeClient, node.Name, nodeUnreachableTaint)


Above recordNodeStatusChange(nc.recorder, node, "NodeNotReady") sets node status to NodeNotReady, so shouldn't the applied taint be TaintNodeNotReady?

Double checked here, though the node status change is recorded NodeNotReady, it is actually node unreachable (NodeReady status ConditionUnknown) here, so we should use TaintNodeUnreachable

aveshagarwal · 2016-10-14T18:37:23Z

@kevin-wangzefeng thanks for this PR. could you put all generated code in a separate commit for easier review?

jayunit100 · 2016-10-14T21:14:45Z

pkg/api/helpers.go

-	if (len(toleration.Operator) == 0 || toleration.Operator == TolerationOpEqual) && toleration.Value == taint.Value {
-		return true
+
+	// check forgivenessSeconds time out


maybe just delete this comment :)

kevin-wangzefeng · 2017-01-04T02:06:52Z

@davidopp @gmarek @aveshagarwal, thanks for your feedback, I will do the refreshing work in new PRs and get them done in time. For this PR, I will keep as it is now.

timothysc · 2017-01-04T21:09:00Z

We can't let this slip, I'd like to enqueue a status update here during the next sig call.

gmarek · 2017-01-18T16:23:52Z

@timothysc - I'm offcall now and I'll prioritize this work. I probably won't be able to do serious code reviews because of that (@sjug)

k8s-github-robot · 2017-01-24T02:02:10Z

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

We suggest the following people:
cc @derekwaynecarr @lavalamp @wojtek-t @spxtr
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Automatic merge from submit-queue (batch tested with PRs 39469, 40557) Forgiveness api changes **What this PR does / why we need it**: Splited from #34825 , contains api changes that are needed to implement forgiveness: 1. update toleration api types to support forgiveness, added a new field forgivenessSeconds to indicate the duration of time it tolerates a taint. 2. update taint api types, added a new field to indicate the time the taint is added. **Which issue this PR fixes** : Related issue: #1574 Related PR: #34825 **Special notes for your reviewer**: **Release note**: ```release-note forgiveness alpha version api definition ```

timothysc · 2017-02-01T22:26:06Z

@kevin-wangzefeng Is there are reason to keep this PR open given the other PRs are nearly done?

…hanges Automatic merge from submit-queue (batch tested with PRs 40696, 39914, 40374) Forgiveness library changes **What this PR does / why we need it**: Splited from #34825, contains library changes that are needed to implement forgiveness: 1. ~~make taints-tolerations matching respect timestamps, so that one toleration can just tolerate a taint for only a period of time.~~ As TaintManager is caching taints and observing taint changes, time-based checking is now outside the library (in TaintManager). see #40355. 2. make tolerations respect wildcard key. 3. add/refresh some related functions to wrap taints-tolerations operation. **Which issue this PR fixes**: Related issue: #1574 Related PR: #34825, #39469 ~~Please note that the first 2 commits in this PR come from #39469 .~~ **Special notes for your reviewer**: ~~Since currently we have `pkg/api/helpers.go` and `pkg/api/v1/helpers.go`, there are some duplicated periods of code laying in these two files.~~ ~~Ideally we should move taints-tolerations related functions into a separate package (pkg/util/taints), and make it a unified set of implementations. But I'd just suggest to do it in a follow-up PR after Forgiveness ones done, in case of feature Forgiveness getting blocked to long.~~ **Release note**: ```release-note make tolerations respect wildcard key ```

kevin-wangzefeng · 2017-02-07T14:44:48Z

No, we can close it now.

…ission-controller Automatic merge from submit-queue (batch tested with PRs 41043, 39058, 41021, 41603, 41414) add defaultTolerationSeconds admission controller **What this PR does / why we need it**: Splited from #34825, add a new admission-controller that 1. adds toleration (with tolerationSeconds = 300) for taint `notReady:NoExecute` to every pod that does not already have a toleration for that taint, and 2. adds toleration (with tolerationSeconds = 300) for taint `unreachable:NoExecute` to every pod that does not already have a toleration for that taint. **Which issue this PR fixes**: Related issue: #1574 Related PR: #34825 **Special notes for your reviewer**: **Release note**: ```release-note add defaultTolerationSeconds admission controller ```

…ionseconds-admission-controller Automatic merge from submit-queue (batch tested with PRs 40932, 41896, 41815, 41309, 41628) enable DefaultTolerationSeconds admission controller by default **What this PR does / why we need it**: Continuation of PR #41414, enable DefaultTolerationSeconds admission controller by default. **Which issue this PR fixes**: fixes: #41860 related Issue: #1574, #25320 related PRs: #34825, #41133, #41414 **Special notes for your reviewer**: **Release note**: ```release-note enable DefaultTolerationSeconds admission controller by default ```

googlebot added the cla: yes label Oct 14, 2016

k8s-github-robot assigned erictune Oct 14, 2016

kevin-wangzefeng assigned davidopp and unassigned erictune Oct 14, 2016

aveshagarwal reviewed Oct 14, 2016

View reviewed changes

kevin-wangzefeng force-pushed the feature_forgiveness branch from bb15419 to 04fa9c8 Compare October 14, 2016 15:59

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

aveshagarwal reviewed Oct 14, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2016

jayunit100 reviewed Oct 14, 2016

View reviewed changes

kevin-wangzefeng mentioned this pull request Jan 5, 2017

Forgiveness api changes #39469

Merged

kevin-wangzefeng mentioned this pull request Jan 14, 2017

Forgiveness library changes #39914

Merged

k8s-github-robot assigned derekwaynecarr, lavalamp and spxtr and unassigned gmarek, davidopp, derekwaynecarr and lavalamp Jan 30, 2017

apelisse assigned gmarek, davidopp, derekwaynecarr and lavalamp Jan 31, 2017

k8s-github-robot assigned deads2k and timothysc Jan 31, 2017

spxtr removed their assignment Jan 31, 2017

lavalamp removed their assignment Feb 3, 2017

kevin-wangzefeng closed this Feb 7, 2017

kevin-wangzefeng mentioned this pull request Feb 14, 2017

add defaultTolerationSeconds admission controller #41414

Merged

kevin-wangzefeng mentioned this pull request Feb 22, 2017

enable DefaultTolerationSeconds admission controller by default #41815

Merged

WIP: implement forgiveness phase 1 #34825

WIP: implement forgiveness phase 1 #34825

Conversation

kevin-wangzefeng commented Oct 14, 2016 • edited Loading

aveshagarwal commented Oct 14, 2016

aveshagarwal commented Oct 14, 2016

kevin-wangzefeng commented Oct 14, 2016 • edited Loading

kevin-wangzefeng commented Oct 14, 2016

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

kevin-wangzefeng Oct 14, 2016 • 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

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

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

aveshagarwal commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-wangzefeng commented Jan 4, 2017

timothysc commented Jan 4, 2017

gmarek commented Jan 18, 2017

k8s-github-robot commented Jan 24, 2017

timothysc commented Feb 1, 2017

kevin-wangzefeng commented Feb 7, 2017

kevin-wangzefeng commented Oct 14, 2016 •

edited

Loading

kevin-wangzefeng commented Oct 14, 2016 •

edited

Loading

kevin-wangzefeng Oct 14, 2016 •

edited

Loading