Fix incorrect call to 'bind' in scheduler #50028

julia-stripe · 2017-08-02T17:36:59Z

I previously submitted #49661 -- I'm not sure if that PR is too big or what, but this is an attempt at a smaller PR that makes progress on the same issue and is easier to review.

What this PR does / why we need it:

In this refactor (ecb962e6585#diff-67f2b61521299ca8d8687b0933bbfb19R223) the scheduler code was refactored into separate bind and assume functions. When that happened, bind was called with pod as an argument. The argument to bind should be the assumed pod, not the original pod. Evidence that assumedPod is the correct argument bind and not pod:

kubernetes/plugin/pkg/scheduler/scheduler.go

Lines 229 to 234 in 80f26fa

    
           if err := sched.config.SchedulerCache.FinishBinding(&assumed); err != nil { 
        
           	glog.Errorf("scheduler cache FinishBinding failed: %v", err) 
        
           } 
        
           if err != nil { 
        
           	glog.V(1).Infof("Failed to bind pod: %v/%v", pod.Namespace, pod.Name) 
        
           	if err := sched.config.SchedulerCache.ForgetPod(&assumed); err != nil {

. (and it says assumed in the function signature for bind, even though it's not called with the assumed pod as an argument).

This is an issue (and causes #49314, where pods that fail to bind to a node get stuck indefinitely) in the following scenario:

The pod fails to bind to the node
bind calls ForgetPod with the pod argument
since ForgetPod is expecting the assumed pod as an argument (because that's what's in the scheduler cache), it fails with an error like scheduler cache ForgetPod failed: pod test-677550-rc-edit-namespace/nginx-jvn09 state was assumed on a different node
The pod gets lost forever because of some incomplete error handling (which I haven't addressed here in the interest of making a simpler PR)

In this PR I've fixed the call to bind and modified the tests to make sure that ForgetPod gets called with the correct argument (the assumed pod) when binding fails.

Which issue this PR fixes: fixes #49314

Special notes for your reviewer:

Release note:

Fix bug in scheduler that caused initially unschedulable pods to stuck in Pending state forever.

k8s-ci-robot · 2017-08-02T17:37:06Z

Hi @julia-stripe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cblecker · 2017-08-02T19:39:39Z

/ok-to-test

julia-stripe · 2017-08-02T22:01:35Z

/retest

dchen1107 · 2017-08-03T01:42:33Z

From the original issue description, it is a serial regression for 1.7 release, and we should patch it. I marked this pr for cherrypick-candidate.

@julia-stripe Looks like there are two prs (this one and #49661) addressing the same issue. I didn't see much difference from both prs. Could you please close one?

dchen1107 · 2017-08-03T01:46:07Z

The pr looks good to me based on the problem described, and I prefer someone from @kubernetes/sig-scheduling-pr-reviews to take a look too. @bsalamat

k82cn · 2017-08-03T01:51:34Z

/assign

bsalamat · 2017-08-03T01:59:17Z

plugin/pkg/scheduler/scheduler.go

 	if err != nil {
 		return
 	}

 	// bind the pod to its host asynchronously (we can do this b/c of the assumption step above).
 	go func() {
-		err := sched.bind(pod, &v1.Binding{
-			ObjectMeta: metav1.ObjectMeta{Namespace: pod.Namespace, Name: pod.Name, UID: pod.UID},
+		err := sched.bind(&assumedPod, &v1.Binding{


Isn't this the same as before.

we used pod directly before the fix; and it was assumedPod before the refactor.

It's not the same - assume() method above is modifying the given pod.

I meant this piece of code is logically the same in this PR.

Is sched.assume() going to modify assumedPod?

@bsalamat I don't understand this question. Yes it is modifying. Yes it is what we are trying to achieve.

Thanks, @wojtek-t! I don't know why I missed it. I guess github UI still confuses me.

wanghaoran1988 · 2017-08-03T03:36:27Z

plugin/pkg/scheduler/scheduler.go

@@ -264,15 +263,16 @@ func (sched *Scheduler) scheduleOne() {

 	// Tell the cache to assume that a pod now is running on a given node, even though it hasn't been bound yet.
 	// This allows us to keep scheduling without waiting on binding to occur.
-	err = sched.assume(pod, suggestedHost)
+	assumedPod := *pod


Why you make a copy here? seems you moved the copy from inside the assume func here.

we should not use obj from cache directly.

Basically we need to make a copy somewhere and @k82cn suggested it would be more clear if we made the copy outside of the assume function. (which I agree with)

wanghaoran1988 · 2017-08-03T03:39:38Z

/cc @aveshagarwal

davidopp · 2017-08-03T05:34:05Z

@wojtek-t

wojtek-t · 2017-08-03T07:25:16Z

Thanks a lot for this PR. I added some minor nits but this overall looks good to me.

k82cn · 2017-08-03T07:28:16Z

@wojtek-t , can you also help to review #49661 ? I think FinishBinding & ForgetPod is also important to 'cherrypick'.

julia-stripe · 2017-08-03T20:21:02Z

/retest

wojtek-t · 2017-08-03T20:50:10Z

@julia-stripe - please squash commits and I will approve.

julia-stripe · 2017-08-03T20:55:38Z

done!

julia-stripe · 2017-08-03T21:40:28Z

/retest

wojtek-t · 2017-08-04T06:17:47Z

/lgtm

/retest

k8s-github-robot · 2017-08-04T06:18:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, julia-stripe, wojtek-t

Associated issue: 49314

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

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

~~plugin/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

fejta-bot · 2017-08-04T13:11:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-04T16:11:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-04T17:33:09Z

Automatic merge from submit-queue

bsalamat · 2017-08-04T18:24:56Z

Thanks a lot, @julia-stripe for debugging this issue and the fix!

davidopp · 2017-08-04T18:52:06Z

+1

davidopp · 2017-08-04T18:54:03Z

BTW I assume we should cherrypick this into 1.7, right?

davidopp · 2017-08-04T18:55:46Z

Ah, @dchen1107 had already marked this as cherypick-candidate.

wojtek-t · 2017-08-07T06:21:43Z

Yes - this will be cherrypicked to 1.7. I will take care of it.

wojtek-t · 2017-08-07T09:06:53Z

Cherrypick in #50240

#50106-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #50028 #50106 upstream release 1.7 Cherry pick of #50028 and #50106 on release-1.7. #50028: Fix incorrect call to 'bind' in scheduler #50106: Retry scheduling pods after errors more consistently in scheduler

k8s-cherrypick-bot · 2017-08-07T21:05:11Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot assigned dchen1107 and thockin Aug 2, 2017

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 2, 2017

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

k8s-github-robot added the release-note-label-needed label Aug 2, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2017

julia-stripe mentioned this pull request Aug 2, 2017

Fix pods stuck in Pending state forever #49661

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2017

dchen1107 mentioned this pull request Aug 3, 2017

Pod that failed to bind, stuck in Pending state forever #49314

Closed

dchen1107 added cherrypick-candidate release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 3, 2017

dchen1107 added this to the v1.7 milestone Aug 3, 2017

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Aug 3, 2017

k8s-ci-robot assigned k82cn Aug 3, 2017

bsalamat reviewed Aug 3, 2017

View reviewed changes

wanghaoran1988 reviewed Aug 3, 2017

View reviewed changes

k8s-ci-robot requested a review from aveshagarwal August 3, 2017 03:39

davidopp assigned davidopp and unassigned thockin Aug 3, 2017

k8s-ci-robot assigned bsalamat Aug 3, 2017

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

Fix incorrect call to 'bind' in scheduler

d584bf4

julia-stripe force-pushed the fix-incorrect-scheduler-bind-call branch from b65bb0a to d584bf4 Compare August 3, 2017 20:55

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2017

k8s-ci-robot assigned wojtek-t Aug 4, 2017

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

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

k8s-github-robot merged commit 898b1b3 into kubernetes:master Aug 4, 2017

julia-stripe deleted the fix-incorrect-scheduler-bind-call branch August 4, 2017 18:52

wojtek-t mentioned this pull request Aug 7, 2017

Automated cherry pick of #50028 #50106 upstream release 1.7 #50240

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 7, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Aug 7, 2017

	if err := sched.config.SchedulerCache.FinishBinding(&assumed); err != nil {
	glog.Errorf("scheduler cache FinishBinding failed: %v", err)
	}
	if err != nil {
	glog.V(1).Infof("Failed to bind pod: %v/%v", pod.Namespace, pod.Name)
	if err := sched.config.SchedulerCache.ForgetPod(&assumed); err != nil {

Fix incorrect call to 'bind' in scheduler #50028

Fix incorrect call to 'bind' in scheduler #50028

Conversation

julia-stripe commented Aug 2, 2017 • edited by wojtek-t Loading

k8s-ci-robot commented Aug 2, 2017

cblecker commented Aug 2, 2017

julia-stripe commented Aug 2, 2017

dchen1107 commented Aug 3, 2017

dchen1107 commented Aug 3, 2017

k82cn commented Aug 3, 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

bsalamat Aug 3, 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

wanghaoran1988 commented Aug 3, 2017

davidopp commented Aug 3, 2017

wojtek-t commented Aug 3, 2017

k82cn commented Aug 3, 2017

julia-stripe commented Aug 3, 2017

wojtek-t commented Aug 3, 2017

julia-stripe commented Aug 3, 2017

julia-stripe commented Aug 3, 2017

wojtek-t commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

fejta-bot commented Aug 4, 2017

fejta-bot commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

bsalamat commented Aug 4, 2017

davidopp commented Aug 4, 2017

davidopp commented Aug 4, 2017

davidopp commented Aug 4, 2017

wojtek-t commented Aug 7, 2017

wojtek-t commented Aug 7, 2017

k8s-cherrypick-bot commented Aug 7, 2017

julia-stripe commented Aug 2, 2017 •

edited by wojtek-t

Loading

bsalamat Aug 3, 2017 •

edited

Loading