Add kubelet awareness to taint tolerant match caculator. #26501

resouer · 2016-05-29T09:05:41Z

Add kubelet awareness to taint tolerant match caculator.

This is required by TaintEffectNoScheduleNoAdmit & TaintEffectNoScheduleNoAdmitNoExecute, so that node will know if it should expect the taint&tolerant

This change is

davidopp · 2016-06-01T06:50:40Z

Thanks for sending. I will take a look later this week.

resouer · 2016-06-13T03:03:42Z

ping @davidopp any progress on this?

davidopp · 2016-06-13T03:55:27Z

Sorry, I will take a look as soon as I can.

resouer · 2016-06-22T06:48:24Z

@k8s-bot test this issue: #IGNORE

kevin-wangzefeng · 2016-06-24T08:38:40Z

/cc @kevin-wangzefeng

lukaszo · 2016-07-14T13:50:02Z

pkg/kubelet/kubelet.go

@@ -2483,6 +2489,13 @@ func (kl *Kubelet) canAdmitPod(pods []*api.Pod, pod *api.Pod) (bool, string, str
 			glog.V(2).Infof("Predicate failed on Pod: %v, for reason: %v", format.Pod(pod), message)
 			return fit, reason, message
 		}
+		if re, ok := err.(*predicates.ErrTaintsTolerationsNotMatch); ok {


I'm not sure about this check. Correct me if I'm wrong but for now GeneralPredicates does not check Taints. There is a separate function for it predicates.PodToleratesNodeTaints so this error will not appear here.

You are right, I need to call TaintToleratedByTolerations here separately, will fix this

In the meantime I created PR which adds Taint to GeneralPredicates #29116

resouer · 2016-07-22T09:19:27Z

[Updated] Updated the commit. I am hesitate to add PodToleratesNodeTaints in GeneralPredicates, so choose to call it in kubelet directly cc @davidopp @lukaszo

resouer · 2016-07-30T02:19:13Z

ping @davidopp any bandwidth on this?

davidopp · 2016-07-30T21:13:40Z

Sorry, not yet. Maybe toward the end of the upcoming week.

davidopp · 2016-08-07T06:11:15Z

cc/ @kubernetes/sig-node

davidopp · 2016-08-07T06:17:26Z

cc/ @kevin-wangzefeng

davidopp · 2016-08-07T07:03:51Z

pkg/kubelet/kubelet.go

+			}
+		}
+
+		reason := "UnexpectedPredicateFailureType"


I think you should change this to

reason = "PodToleratesNodeTaints" message = re.Error()

davidopp · 2016-08-07T07:45:38Z

You need some kind of e2e or integration test that verifies that Kubelet does not admit a pod when the node has an untolerated taint of type NoScheduleNoAdmit. The pod needs to be sent directly to the kubelet, and not go through the scheduler (because the scheduler would never send such a pod to such a node). In other words, create a pod with NodeName already set to a node that has a taint of type NoScheduleNoAdmit, and verify that Kubelet does not admit the pod.

k8s-ci-robot · 2016-09-22T14:39:45Z

Jenkins GCE e2e failed for commit 03960a945a1e25e284a64d952988af9c50cf3d88.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

resouer · 2016-09-22T16:06:05Z

@davidopp This PR is ready for final review cc @yujuhong

timothysc

minor comments.

timothysc · 2016-09-26T20:30:09Z

pkg/kubelet/lifecycle/predicate.go

+		if len(reasons) == 0 {
+			message = fmt.Sprint("PodToleratesNodeTaints failed due to unknown reason, which is unexpected.")
+			glog.Warningf("Failed to admit pod %v - %s", format.Pod(pod), message)
+			return PodAdmitResult{


Without me popping through the node code, are the events log'd higher up the call chain?

I don't see event will higher it up, more clues?

timothysc · 2016-09-26T20:31:16Z

pkg/kubelet/lifecycle/predicate.go

+				Message: message,
+			}
+		}
+		r := reasons[0]


If multiple reasons are returned, why only output [0]?

The caller: canAdmitPod returns bool value with one reason, so it's enough to use the first reason.

timothysc · 2016-09-26T20:31:50Z

pkg/kubelet/lifecycle/predicate.go

+			glog.Warningf("Failed to admit pod %v - %s", format.Pod(pod), message)
+		}
+
+		return PodAdmitResult{


same comment re: event.

timothysc · 2016-09-26T21:06:12Z

test/e2e_node/noadmit_taint_test.go

+			By("Waiting for static pod rejected event")
+			eventFound := false
+		EventsLoop:
+			for start := time.Now(); time.Since(start) < 4*time.Minute; time.Sleep(2 * time.Second) {


I'm curious why your list looping on events vs. watching the pod in question or events for that namespace? In general lists should be avoided even in the tests.

There exists a reflector / informer in the framework code iirc.

resouer · 2016-10-01T15:04:28Z

Use event watch instead of list based on @timothysc 's comments

timothysc · 2016-10-03T21:08:33Z

test/e2e_node/noadmit_taint_test.go

+							if e.InvolvedObject.Kind == "Pod" && e.Reason == "PodToleratesNodeTaints" && strings.Contains(e.Message,
+								"Taint Toleration unmatched with SomeUntoleratedTaintIsNoAdmit is: true") {
+								By("PodToleratesNodeTaints event found")
+								eventFound = true


Thanks for the update.

timothysc · 2016-10-03T21:09:27Z

lgtm... @davidopp I have no more comments, did you have anything you wanted to add?

/cc @aveshagarwal

resouer · 2016-10-07T15:02:38Z

I'm sure I have signed foundation cla btw, may be a re-push is needed :/

timothysc · 2016-10-07T15:30:53Z

k, I'm going to timeout on this and lgtm.

k8s-github-robot · 2016-10-07T18:26:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

davidopp · 2016-10-07T18:52:40Z

I'm removing the LGTM for now because I'd really like to review this (or at least have @kevin-wangzefeng review it) before it goes in. I'll try to take a look at it soon.

k8s-github-robot · 2016-10-07T19:05:35Z

Automatic merge from submit-queue

davidopp · 2016-10-07T19:10:11Z

Ugh, look like I was too late.
I'm going to send a rollback PR, because it was not reviewed by anyone from node team (I see just one comment from @yujuhong but not a review of the changes to Kubelet).

timothysc · 2016-10-07T19:29:35Z

@davidopp I ping'd 4+ days ago for additional comments.

davidopp · 2016-10-07T19:35:36Z

Yes, sorry, I'm really really backed up right now. I suspect the node folks are too :(

@timothysc

Automatic merge from submit-queue Revert "Add kubelet awareness to taint tolerant match caculator." Reverts #26501 Original PR was not fully reviewed by @kubernetes/sig-node cc/ @timothysc @resouer

resouer · 2016-10-08T20:33:31Z

@davidopp So I need to open a new PR by using this branch, right?

davidopp · 2016-10-08T20:44:38Z

Yes, send out a new PR and we will review it as soon as we can. Thanks and sorry for the trouble.

googlebot added the cla: yes label May 29, 2016

k8s-github-robot assigned davidopp May 29, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 29, 2016

resouer force-pushed the scheduler branch from 8828dd4 to 2df7c8b Compare May 29, 2016 12:18

resouer mentioned this pull request May 31, 2016

Rename TolerationsAnnotationKey and TaintsAnnotationKey from .alpha. to .beta. #26339

Closed

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

lukaszo reviewed Jul 14, 2016
View reviewed changes

resouer force-pushed the scheduler branch 2 times, most recently from 5a17beb to e1609ac Compare July 22, 2016 09:15

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

davidopp reviewed Aug 7, 2016
View reviewed changes

davidopp added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 7, 2016

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

resouer force-pushed the scheduler branch from 03960a9 to 58c1490 Compare September 22, 2016 15:08

timothysc reviewed Sep 26, 2016

View reviewed changes

timothysc added this to the v1.5 milestone Sep 26, 2016

timothysc self-assigned this Sep 26, 2016

Harry Zhang added 2 commits October 1, 2016 01:07

Setup e2e test for no admit

c2cf5bb

Update generated openapi

0767614

resouer force-pushed the scheduler branch from 58c1490 to 0767614 Compare October 1, 2016 14:59

timothysc reviewed Oct 3, 2016

View reviewed changes

timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2016

davidopp removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2016

k8s-github-robot merged commit 21188ca into kubernetes:master Oct 7, 2016

davidopp mentioned this pull request Oct 7, 2016

Revert "Add kubelet awareness to taint tolerant match caculator." #34350

Merged

resouer mentioned this pull request Oct 8, 2016

Revert "Revert "Add kubelet awareness to taint tolerant match calculator."" #34400

Closed

kevin-wangzefeng mentioned this pull request Oct 18, 2016

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

Closed

12 tasks

Add kubelet awareness to taint tolerant match caculator. #26501

Add kubelet awareness to taint tolerant match caculator. #26501

Conversation

resouer commented May 29, 2016 • edited by k8s-oncall Loading

davidopp commented Jun 1, 2016

resouer commented Jun 13, 2016

davidopp commented Jun 13, 2016

resouer commented Jun 22, 2016

kevin-wangzefeng commented Jun 24, 2016

Choose a reason for hiding this comment

resouer Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer commented Jul 22, 2016 • edited Loading

resouer commented Jul 30, 2016

davidopp commented Jul 30, 2016

davidopp commented Aug 7, 2016

davidopp commented Aug 7, 2016

Choose a reason for hiding this comment

davidopp commented Aug 7, 2016

k8s-ci-robot commented Sep 22, 2016

resouer commented Sep 22, 2016

timothysc left a 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

resouer commented Oct 1, 2016

Choose a reason for hiding this comment

timothysc commented Oct 3, 2016

resouer commented Oct 7, 2016

timothysc commented Oct 7, 2016

k8s-github-robot commented Oct 7, 2016

davidopp commented Oct 7, 2016

k8s-github-robot commented Oct 7, 2016

davidopp commented Oct 7, 2016

timothysc commented Oct 7, 2016

davidopp commented Oct 7, 2016

resouer commented Oct 8, 2016

davidopp commented Oct 8, 2016

resouer commented May 29, 2016 •

edited by k8s-oncall

Loading

resouer Jul 21, 2016 •

edited

Loading

resouer commented Jul 22, 2016 •

edited

Loading