Should move all unscheduable pods when we received move request to active queue #73309

cofyc · 2019-01-25T10:48:29Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Fix weakness of current receivedMoveRequest

add incremental scheduling cycle
instead of set a flag on move reqeust, we cache current scheduling
cycle in moveRequestCycle
when unschedulable pods are added back, compare its cycle with
moveRequestCycle to decide whether it should be added into active queue
or not

See #73216 (comment) and #73078.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

scheduler: use incremental scheduling cycle in PriorityQueue to put all in-flight unschedulable pods back to active queue if we received move request

cofyc · 2019-01-25T10:48:50Z

/sig scheduling

resouer · 2019-01-25T19:13:47Z

pkg/scheduler/factory/factory.go

I can see your point but adding an interface named "Cycle()" which will expose implementation detail of queue to outside doesn't look good.

May need to think twice.

resouer · 2019-01-25T19:55:34Z

/assign

bsalamat

I have a few minor comments. I agree with @resouer's point that exposing scheduling cycle from the scheduling queue is not elegant. A different implementation could be to add the cycle to the scheduler and increment it at the beginning of every scheduling cycle. The scheduler could pass the cycle as an argument to AddUnschedulableIfNotPresent and MoveAllToActiveQueue. However, this approach is not great either and has its own problems. It may in fact be worse in that the scheduler would keep track of scheduling cycles while the only user of the cycle is the scheduling queue.
So, I am fine with the implementation done in this PR.

bsalamat · 2019-01-25T22:24:51Z

pkg/scheduler/internal/queue/scheduling_queue.go

Please rename this to SchedulingCycle. One could think that this is the queue cycle.

bsalamat · 2019-01-25T22:26:06Z

pkg/scheduler/internal/queue/scheduling_queue.go

Rename this to moveRequestCycle.

bsalamat · 2019-01-25T22:27:26Z

pkg/scheduler/internal/queue/scheduling_queue.go

schedulingCycle

bsalamat · 2019-01-25T22:28:05Z

pkg/scheduler/internal/queue/scheduling_queue.go

s/cycle/podSchedulingCycle/

wgliang · 2019-01-26T00:43:21Z

pkg/scheduler/internal/queue/scheduling_queue.go

I will recommend using p.lock.RLock() and p.lock.RUnlock() here. :)

cofyc · 2019-01-26T03:40:17Z

/test pull-kubernetes-e2e-gce-100-performance

Huang-Wei · 2019-01-29T04:24:10Z

/retest

resouer · 2019-01-29T18:50:25Z

pkg/scheduler/internal/queue/scheduling_queue.go

We need to make this comment general and straightforward for upcoming developers to implement their own scheduling queue. For example, at interface level, there should be no concept of activeQ or unschedulableQ.

How about:

// AddUnschedulableIfNotPresent adds an unschedulable pod back to scheduling queue. // The podSchedulingCycle represents the current scheduling cycle number which can be // returned by calling SchedulingCycle().

You are right. We should not assume how interface is implemented.

resouer · 2019-01-29T19:06:43Z

pkg/scheduler/internal/queue/scheduling_queue.go

ditto, let's make the comment straightforward and give tips for developers. My stupid example is like:

// SchedulingCycle returns the current number of scheduling cycle which is cached by scheduling queue. // Normally, incrementing this number whenever a pod is popped (e.g. called Pop()) is enough.

Updated. I think this is good enough. PTAL.

resouer · 2019-01-29T19:13:26Z

Just left two comments to make sure the newly added interfaces are understandable for other contributors or ppl who try to implement their own scheduling queue. PTAL

- add incremental scheduling cycle - instead of set a flag on move reqeust, we cache current scheduling cycle in moveRequestCycle - when unschedulable pods are added back, compare its cycle with moveRequestCycle to decide whether it should be added into active queue or not

Huang-Wei · 2019-01-30T06:01:14Z

/retest

bsalamat

/lgtm
/approve

Thanks, @cofyc!

k8s-ci-robot · 2019-01-30T18:40:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, cofyc

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [bsalamat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2019-01-30T21:02:42Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-01-31T00:32:43Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

cofyc · 2019-01-31T01:56:01Z

/skip

k8s-ci-robot · 2019-01-31T05:35:06Z

@cofyc: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`ba47bef`	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

cofyc · 2019-01-31T07:08:37Z

talked with @Huang-Wei, I cherry picked this PR into 1.12 (#73567) and 1.13 (#73568)

tedyu · 2019-01-31T11:19:54Z

pkg/scheduler/internal/queue/scheduling_queue.go

-// queue and clears p.receivedMoveRequest.
-func (p *PriorityQueue) AddUnschedulableIfNotPresent(pod *v1.Pod) error {
+// p.moveRequestCycle > podSchedulingCycle or to backoff queue if p.moveRequestCycle
+// <= podSchedulingCycle but pod is subject to backoff. In other cases, it adds pod to


Looking at the code on line 414, it seems the condition should be (without =) :

podSchedulingCycle > p.moveRequestCycle

Greater than or equal is correct. In fact, it is very important to have =. Equal means that a move request is received in the same scheduling cycle as the pod was being scheduled. So, it is likely that the scheduler has not seen the changes that led to the move request yet and didn't use those changes when making a scheduling decision. As a result, the pod must be retried if it is unschedulable.

…upstream-release-1.12 Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue

…upstream-release-1.13 Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 25, 2019

k8s-ci-robot requested review from resouer and wgliang January 25, 2019 10:49

cofyc mentioned this pull request Jan 25, 2019

scheduler: makes pod less racing so as to be put back into activeQ properly #73078

Merged

cofyc changed the title ~~WIP: Should move all pods when we received mod request to active queue~~ WIP: Should move all unscheduable pods when we received mod request to active queue Jan 25, 2019

cofyc force-pushed the fix73216-receivedMoveRequest branch from 6dcc5de to c6cbd4c Compare January 25, 2019 10:55

resouer suggested changes Jan 25, 2019

View reviewed changes

k8s-ci-robot assigned resouer Jan 25, 2019

bsalamat reviewed Jan 25, 2019

View reviewed changes

wgliang reviewed Jan 26, 2019

View reviewed changes

cofyc force-pushed the fix73216-receivedMoveRequest branch 5 times, most recently from 3dd2441 to 5328984 Compare January 26, 2019 02:52

cofyc changed the title ~~WIP: Should move all unscheduable pods when we received mod request to active queue~~ Should move all unscheduable pods when we received mod request to active queue Jan 26, 2019

resouer reviewed Jan 29, 2019

View reviewed changes

cofyc force-pushed the fix73216-receivedMoveRequest branch from 0e0d4cb to ba47bef Compare January 30, 2019 02:14

bsalamat reviewed Jan 30, 2019

View reviewed changes

k8s-ci-robot assigned bsalamat Jan 30, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019

cofyc mentioned this pull request Jan 31, 2019

Local PV stress e2e failing #73419

Closed

bsalamat mentioned this pull request Jan 31, 2019

Eclass task 2: Add concept of "equiv class" in scheduling queue #72677

Closed

cofyc changed the title ~~Should move all unscheduable pods when we received mod request to active queue~~ Should move all unscheduable pods when we received move request to active queue Jan 31, 2019

k8s-ci-robot merged commit 9388a27 into kubernetes:master Jan 31, 2019

This was referenced Jan 31, 2019

Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue #73567

Merged

Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue #73568

Merged

tedyu reviewed Jan 31, 2019

View reviewed changes

k8s-ci-robot added a commit that referenced this pull request Feb 1, 2019

Merge pull request #73567 from cofyc/automated-cherry-pick-of-#73309-…

8ddfd4b

…upstream-release-1.12 Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue

Huang-Wei mentioned this pull request Feb 1, 2019

Scheduler leaves pod stuck in pending after worker node reload #72925

Closed

AlexDiamanti mentioned this pull request Feb 19, 2019

Issue with k8s.io/docs/tasks/administer-cluster/static-pod/ kubernetes/website#12467

Closed

2 tasks

bsalamat mentioned this pull request Feb 21, 2019

Pods sharing a PVC on a single node cluster fail to schedule #73216

Closed

k8s-ci-robot added a commit that referenced this pull request Feb 21, 2019

Merge pull request #73568 from cofyc/automated-cherry-pick-of-#73309-…

c8e0153

…upstream-release-1.13 Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue

cofyc deleted the fix73216-receivedMoveRequest branch May 4, 2019 07:20

Should move all unscheduable pods when we received move request to active queue #73309

Should move all unscheduable pods when we received move request to active queue #73309

Uh oh!

Conversation

cofyc commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cofyc commented Jan 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

resouer commented Jan 25, 2019

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgliang Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cofyc commented Jan 26, 2019

Uh oh!

Huang-Wei commented Jan 29, 2019

Uh oh!

resouer Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

resouer commented Jan 29, 2019

Uh oh!

Huang-Wei commented Jan 30, 2019

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 30, 2019

Uh oh!

fejta-bot commented Jan 30, 2019

Uh oh!

fejta-bot commented Jan 31, 2019

Uh oh!

cofyc commented Jan 31, 2019

Uh oh!

k8s-ci-robot commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cofyc commented Jan 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsalamat Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

cofyc commented Jan 25, 2019 •

edited

Loading

wgliang Jan 26, 2019 •

edited

Loading

resouer Jan 29, 2019 •

edited

Loading

k8s-ci-robot commented Jan 31, 2019 •

edited

Loading

bsalamat Feb 1, 2019 •

edited

Loading