Task 1: Tainted node by condition. #49257

k82cn · 2017-07-20T02:52:13Z

What this PR does / why we need it:
Tainted node by condition for MemoryPressure, OutOfDisk and so on.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): part of #42001

Release note:

Tainted nodes by conditions as following:
  * 'node.kubernetes.io/network-unavailable=:NoSchedule' if NetworkUnavailable is true
  * 'node.kubernetes.io/disk-pressure=:NoSchedule' if DiskPressure is true
  * 'node.kubernetes.io/memory-pressure=:NoSchedule' if MemoryPressure is true
  * 'node.kubernetes.io/out-of-disk=:NoSchedule' if OutOfDisk is true

k82cn · 2017-07-20T02:52:48Z

/assign

k82cn · 2017-07-20T02:53:14Z

/unassign @wojtek-t @errordeveloper

k82cn · 2017-07-20T13:36:53Z

/cc @gmarek

This PR show the major idea on tainting Node NoSchedule by condition; is this OK for you?

Test MemoryPressure in local cluster with instrument code; I'll continue to add more unit test if it align with the code in your mind :).

k82cn · 2017-07-21T08:43:45Z

cc/ @resouer

k82cn · 2017-07-23T03:47:07Z

/retest

k82cn · 2017-07-24T01:07:08Z

/retest

…

---- Da (Klaus), Ma (马达) | PMP® | Software Architect, IBM Cloud private Development IBM Spectrum Computing, IBM System +86-10-8245 4084 | klaus1982.cn@gmail.com | http://k82.me

On Sun, Jul 23, 2017 at 10:22 AM, k8s-ci-robot ***@***.***> wrote: @k82cn <https://github.com/k82cn>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-etcd3 02a6f18 <02a6f18> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/49257/pull-kubernetes-e2e-gce-etcd3/42331/> /test pull-kubernetes-e2e-gce-etcd3 Full PR test history <https://k8s-gubernator.appspot.com/pr/49257>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/k82cn>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49257 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARRLLT-NuKAJ9dDT3zvevhYKszR4iennks5sQq5wgaJpZM4Odikw> .

gmarek · 2017-07-24T15:14:53Z

pkg/controller/controller_utils.go

+			if !ok {
+				return nil
+			}
+


remove empty line

gmarek · 2017-07-24T15:16:25Z

pkg/controller/controller_utils.go

@@ -885,7 +885,11 @@ func (o ReplicaSetsBySizeNewer) Less(i, j int) bool {
 	return *(o[i].Spec.Replicas) > *(o[j].Spec.Replicas)
 }

-func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taint *v1.Taint) error {
+func AddOrUpdateTaintOnNode(c clientset.Interface, nodeName string, taints ...*v1.Taint) error {


Can you put those changes into separate PR? They have a lot of sense on their own, and we don't want to revert those in case we find a bug somewhere else.

sure, created PR #49524 .

gmarek · 2017-07-24T15:19:52Z

pkg/controller/node/controller_utils.go

 	if err != nil {
 		utilruntime.HandleError(
 			fmt.Errorf(
 				"unable to taint %v unresponsive Node %q: %v",
-				taintToAdd.Key,
+				taintsToAdd,


this will print pointer addresses. Make sure that it prints something understandable.

gmarek · 2017-07-24T15:24:19Z

pkg/controller/node/nodecontroller.go

+	zoneNoExecuteTainer map[string]*RateLimitedTimedQueue
+
+	noScheduleTainerLock sync.Mutex
+	zoneNoScheduleTainer map[string]*RateLimitedTimedQueue


I'll need to read it carefully (I'll try to do this in the evening, but I may fail, so I'm leaving this note here), but I think this will be wrong. We want to have evictor (i.e. rate-limited tainting) only for Unreachable/NotReady Taints. The rest of the taints should not need rate limiting as they're NoSchedule ones. The only thing that we should protect against is TaintFlapping (i.e. adding and removing Taints in quick succession).

As I said - it's possible that this is how it's done, but I didn't have time to read it carefully enough this time.

Yup, I stand by what I wrote. We don't need rate limiting for those taints.

k82cn · 2017-08-12T02:34:44Z

/unassign
/assign @wojtek-t

k82cn · 2017-08-12T02:36:03Z

@davidopp , @wojtek-t , do you have any comments for this :).

davidopp · 2017-08-13T19:00:21Z

I will take a look this week. May not be able to do it for a few days.

k82cn · 2017-08-23T07:49:43Z

ping :).

wojtek-t · 2017-08-23T09:03:54Z

/approve

I will let @davidopp LGTM (it looks good to me).

luxas · 2017-08-24T12:24:19Z

Just noting that networkUnavailable should be network-unavailable before moving to beta...
Is that something we can do at this stage?
(I know this goes for the general TaintBasedEvictions feature, which still is alpha)

cc @thockin

resouer

@luxas This PR does not rely on well known labels, here is a separate fix #51266 , I am asking @k82cn to review.

k82cn · 2017-08-24T12:59:22Z

@luxas , as we slack, there's not hard code in this feature; #51266 is enough to cover, and no impact to this PR.

resouer · 2017-08-24T13:01:08Z

Cool, but I think you may still need to update the PR description.

yastij · 2017-08-24T13:04:05Z

We'll also need to align the design doc

luxas · 2017-08-24T14:01:13Z

@k82cn @resouer Yeah, cool, just stumbled upon that in the release note above; we should replace it there as well. Thanks!

k82cn · 2017-08-25T09:41:17Z

the description was updated :).

@davidopp , would you help to review this PR?

gmarek · 2017-08-28T10:17:38Z

Ping @davidopp

gmarek · 2017-08-30T12:42:46Z

I spoke with David offline. Applying label.

gmarek · 2017-08-30T12:42:51Z

/lgtm

k8s-github-robot · 2017-08-30T12:42:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmarek, k82cn, wojtek-t

Associated issue: 42001

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

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

~~cmd/kube-controller-manager/OWNERS~~ [wojtek-t]
~~pkg/OWNERS~~ [wojtek-t]

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-08-30T13:56:38Z

/retest

k82cn · 2017-08-30T15:46:05Z

/retest

k8s-github-robot · 2017-09-01T06:13:19Z

Automatic merge from submit-queue (batch tested with PRs 51574, 51534, 49257, 44680, 48836)

k8s-github-robot assigned wojtek-t and errordeveloper Jul 20, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 20, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 20, 2017

k8s-ci-robot assigned k82cn Jul 20, 2017

k8s-ci-robot unassigned errordeveloper and wojtek-t Jul 20, 2017

k82cn force-pushed the k8s_42001 branch 4 times, most recently from fa19889 to 9d71f3f Compare July 20, 2017 13:17

k8s-ci-robot requested a review from gmarek July 20, 2017 13:36

resouer self-requested a review July 22, 2017 15:54

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

k82cn force-pushed the k8s_42001 branch from 9d71f3f to 02a6f18 Compare July 23, 2017 02:04

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

k82cn force-pushed the k8s_42001 branch from 02a6f18 to b3e5cbc Compare July 24, 2017 06:55

k82cn changed the title ~~[WIP] Task 1: Tainted node by condition.~~ Task 1: Tainted node by condition. Jul 24, 2017

k82cn force-pushed the k8s_42001 branch from b3e5cbc to 0ef66c5 Compare July 24, 2017 11:08

gmarek reviewed Jul 24, 2017

View reviewed changes

pkg/controller/controller_utils.go Outdated

if !ok {

return nil

}

Copy link

Contributor

gmarek Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

gmarek reviewed Jul 24, 2017

View reviewed changes

k8s-ci-robot assigned wojtek-t and unassigned k82cn Aug 12, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2017

resouer reviewed Aug 24, 2017

View reviewed changes

luxas added this to the v1.8 milestone Aug 24, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

gmarek closed this Aug 30, 2017

gmarek reopened this Aug 30, 2017

k8s-github-robot merged commit b832992 into kubernetes:master Sep 1, 2017

k82cn deleted the k8s_42001 branch September 1, 2017 06:33

davidopp mentioned this pull request Sep 22, 2017

delete pods API call latencies shot up on large cluster tests #51899

Closed

KeyOfSpectator mentioned this pull request Feb 6, 2023

support set node taints from custom node condition kubernetes/node-problem-detector#736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task 1: Tainted node by condition. #49257

Task 1: Tainted node by condition. #49257

k82cn commented Jul 20, 2017 •

edited

Loading

k82cn commented Jul 20, 2017

k82cn commented Jul 20, 2017

k82cn commented Jul 20, 2017

k82cn commented Jul 21, 2017

k82cn commented Jul 23, 2017

k82cn commented Jul 24, 2017 via email

gmarek Jul 24, 2017

gmarek Jul 24, 2017

k82cn Jul 25, 2017

gmarek Jul 24, 2017

k82cn Jul 30, 2017

gmarek Jul 24, 2017

gmarek Jul 25, 2017

k82cn commented Aug 12, 2017

k82cn commented Aug 12, 2017

davidopp commented Aug 13, 2017

k82cn commented Aug 23, 2017

wojtek-t commented Aug 23, 2017

luxas commented Aug 24, 2017

resouer left a comment •

edited

Loading

k82cn commented Aug 24, 2017

resouer commented Aug 24, 2017

yastij commented Aug 24, 2017

luxas commented Aug 24, 2017

k82cn commented Aug 25, 2017

gmarek commented Aug 28, 2017

gmarek commented Aug 30, 2017

gmarek commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

gmarek commented Aug 30, 2017

k82cn commented Aug 30, 2017

k8s-github-robot commented Sep 1, 2017

Task 1: Tainted node by condition. #49257

Task 1: Tainted node by condition. #49257

Conversation

k82cn commented Jul 20, 2017 • edited Loading

k82cn commented Jul 20, 2017

k82cn commented Jul 20, 2017

k82cn commented Jul 20, 2017

k82cn commented Jul 21, 2017

k82cn commented Jul 23, 2017

k82cn commented Jul 24, 2017 via email

gmarek Jul 24, 2017

Choose a reason for hiding this comment

gmarek Jul 24, 2017

Choose a reason for hiding this comment

k82cn Jul 25, 2017

Choose a reason for hiding this comment

gmarek Jul 24, 2017

Choose a reason for hiding this comment

k82cn Jul 30, 2017

Choose a reason for hiding this comment

gmarek Jul 24, 2017

Choose a reason for hiding this comment

gmarek Jul 25, 2017

Choose a reason for hiding this comment

k82cn commented Aug 12, 2017

k82cn commented Aug 12, 2017

davidopp commented Aug 13, 2017

k82cn commented Aug 23, 2017

wojtek-t commented Aug 23, 2017

luxas commented Aug 24, 2017

resouer left a comment • edited Loading

Choose a reason for hiding this comment

k82cn commented Aug 24, 2017

resouer commented Aug 24, 2017

yastij commented Aug 24, 2017

luxas commented Aug 24, 2017

k82cn commented Aug 25, 2017

gmarek commented Aug 28, 2017

gmarek commented Aug 30, 2017

gmarek commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

gmarek commented Aug 30, 2017

k82cn commented Aug 30, 2017

k8s-github-robot commented Sep 1, 2017

k82cn commented Jul 20, 2017 •

edited

Loading

resouer left a comment •

edited

Loading