Retry scheduling pods after errors more consistently in scheduler #50106

julia-stripe · 2017-08-03T20:04:05Z

What this PR does / why we need it:

This fixes 2 places in the scheduler where pods can get stuck in Pending forever. In both these places, errors happen and sched.config.Error is not called afterwards. This is a problem because sched.config.Error is responsible for requeuing pods to retry scheduling when there are issues (see here), so if we don't call sched.config.Error then the pod will never get scheduled (unless the scheduler is restarted).

One of these (where it returns when ForgetPod fails instead of continuing and reporting an error) is a regression from this refactor, and with the old behavior the error was reported correctly. As far as I can tell changing the error handling in that refactor wasn't intentional.

When AssumePod fails there's never been an error reported but I think adding this will help the scheduler recover when something goes wrong instead of letting pods possibly never get scheduled.

This will help prevent issues like #49314 in the future.

Release note:

Fix incorrect retry logic in scheduler

k8s-ci-robot · 2017-08-03T20:04:13Z

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.

k82cn · 2017-08-03T23:26:07Z

plugin/pkg/scheduler/scheduler.go

@@ -222,7 +227,7 @@ func (sched *Scheduler) bind(assumed *v1.Pod, b *v1.Binding) error {
 	if err != nil {
 		glog.V(1).Infof("Failed to bind pod: %v/%v", assumed.Namespace, assumed.Name)
 		if err := sched.config.SchedulerCache.ForgetPod(assumed); err != nil {
-			return fmt.Errorf("scheduler cache ForgetPod failed: %v", err)
+			glog.Errorf("scheduler cache ForgetPod failed: %v", err)


I think we need to FinisBinding before ForgetPod; if ForgetPod failed, the assumed pod maybe still in cache.

Agree. There were two problems here. One that we were returning here, the second is what @k82cn pointed at.

wojtek-t · 2017-08-04T06:31:36Z

plugin/pkg/scheduler/scheduler.go

-		// This should be fixed properly though.
+
+		// This is most probably result of a BUG in retrying logic.
+		// We report an error here so that pod scheduling can be retried.


This relies on the fact that Error will check if pod was bounded in the meantime and if so will not add it back to the unscheduled pods set (otherwise it would lead to an infinite loop).

This is true now, but can you please add a comment about it.

wojtek-t · 2017-08-04T06:33:29Z

plugin/pkg/scheduler/scheduler.go

@@ -222,7 +227,7 @@ func (sched *Scheduler) bind(assumed *v1.Pod, b *v1.Binding) error {
 	if err != nil {
 		glog.V(1).Infof("Failed to bind pod: %v/%v", assumed.Namespace, assumed.Name)
 		if err := sched.config.SchedulerCache.ForgetPod(assumed); err != nil {
-			return fmt.Errorf("scheduler cache ForgetPod failed: %v", err)
+			glog.Errorf("scheduler cache ForgetPod failed: %v", err)


Agree. There were two problems here. One that we were returning here, the second is what @k82cn pointed at.

julia-stripe · 2017-08-04T13:42:57Z

Thanks for the review! Added a comment & moved FinishBinding back up after bind. (as a side note, these are my first contributions to Kubernetes and I'm extremely impressed by how quickly and carefully people review code)

PTAL @wojtek-t

cblecker · 2017-08-04T14:32:44Z

/ok-to-test

wojtek-t · 2017-08-07T06:17:56Z

/lgtm
/approve no-issue

k8s-github-robot · 2017-08-07T06:18:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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/pkg/scheduler/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

k8s-github-robot · 2017-08-07T07:50:05Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-07T08:35:17Z

Automatic merge from submit-queue

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

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:20Z

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-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 3, 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 3, 2017

k8s-github-robot assigned lavalamp and davidopp Aug 3, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Aug 3, 2017

julia-stripe mentioned this pull request Aug 3, 2017

Fix pods stuck in Pending state forever #49661

Closed

wojtek-t assigned wojtek-t and unassigned lavalamp Aug 3, 2017

k82cn reviewed Aug 3, 2017

View reviewed changes

k82cn mentioned this pull request Aug 3, 2017

Can't delete pod #50077

Closed

wojtek-t reviewed Aug 4, 2017

View reviewed changes

julia-stripe force-pushed the improve-scheduler-error-handling branch from 2540b33 to e5c9fb3 Compare August 4, 2017 13:28

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 4, 2017

julia-stripe force-pushed the improve-scheduler-error-handling branch 2 times, most recently from e19f4be to 00c1e74 Compare August 4, 2017 13:34

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 4, 2017

Handle errors more consistently in scheduler

2d9c6df

julia-stripe force-pushed the improve-scheduler-error-handling branch from 00c1e74 to 2d9c6df Compare August 4, 2017 21:05

wojtek-t 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 Aug 7, 2017

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

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

wojtek-t added this to the v1.7 milestone Aug 7, 2017

wojtek-t added the cherrypick-candidate label Aug 7, 2017

k8s-github-robot merged commit bc7ccfe into kubernetes:master Aug 7, 2017

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

gaorong mentioned this pull request Sep 11, 2018

fix kube-scheduler typo error #68444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry scheduling pods after errors more consistently in scheduler #50106

Retry scheduling pods after errors more consistently in scheduler #50106

julia-stripe commented Aug 3, 2017 •

edited by wojtek-t

Loading

k8s-ci-robot commented Aug 3, 2017

k82cn Aug 3, 2017

wojtek-t Aug 4, 2017

wojtek-t Aug 4, 2017

wojtek-t Aug 4, 2017

julia-stripe commented Aug 4, 2017

cblecker commented Aug 4, 2017

wojtek-t commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

wojtek-t commented Aug 7, 2017

k8s-cherrypick-bot commented Aug 7, 2017

Retry scheduling pods after errors more consistently in scheduler #50106

Retry scheduling pods after errors more consistently in scheduler #50106

Conversation

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

k8s-ci-robot commented Aug 3, 2017

k82cn Aug 3, 2017

Choose a reason for hiding this comment

wojtek-t Aug 4, 2017

Choose a reason for hiding this comment

wojtek-t Aug 4, 2017

Choose a reason for hiding this comment

wojtek-t Aug 4, 2017

Choose a reason for hiding this comment

julia-stripe commented Aug 4, 2017

cblecker commented Aug 4, 2017

wojtek-t commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

wojtek-t commented Aug 7, 2017

k8s-cherrypick-bot commented Aug 7, 2017

julia-stripe commented Aug 3, 2017 •

edited by wojtek-t

Loading