New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 31, 2019

Conversation

@cofyc
Copy link
Contributor

cofyc commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

cofyc commented Jan 25, 2019

/sig scheduling

@k8s-ci-robot k8s-ci-robot requested review from resouer and wgliang Jan 25, 2019

@cofyc 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 cofyc force-pushed the cofyc:fix73216-receivedMoveRequest branch from 6dcc5de to c6cbd4c Jan 25, 2019

@@ -1085,6 +1085,7 @@ func MakeDefaultErrorFunc(client clientset.Interface, backoff *util.PodBackoff,
}

backoff.Gc()
cycle := podQueue.Cycle()

This comment has been minimized.

@resouer

resouer Jan 25, 2019

Member

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

This comment has been minimized.

Copy link
Member

resouer commented Jan 25, 2019

/assign

@bsalamat
Copy link
Contributor

bsalamat left a comment

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.

@@ -59,7 +59,8 @@ const unschedulableQTimeInterval = 60 * time.Second
type SchedulingQueue interface {
Add(pod *v1.Pod) error
AddIfNotPresent(pod *v1.Pod) error
AddUnschedulableIfNotPresent(pod *v1.Pod) error
AddUnschedulableIfNotPresent(pod *v1.Pod, cycle int64) error
Cycle() int64

This comment has been minimized.

@bsalamat

bsalamat Jan 25, 2019

Contributor

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

// we received a move request. Unscheduable pods in and before this
// scheduling cycle should be put back to activeQueue if they are in flight
// when we received move request.
receivedMoveRequest int64

This comment has been minimized.

@bsalamat

bsalamat Jan 25, 2019

Contributor

Rename this to moveRequestCycle.

receivedMoveRequest bool
// cycle represents sequence number of scheduling cycle and is incremented
// when a pod is popped
cycle int64

This comment has been minimized.

@bsalamat

bsalamat Jan 25, 2019

Contributor

schedulingCycle

// p.receivedMoveRequest > cycle or to backoff queue if p.receivedMoveRequest
// <= cycle but pod is subject to backoff. In other cases, it adds pod to
// active queue.
func (p *PriorityQueue) AddUnschedulableIfNotPresent(pod *v1.Pod, cycle int64) error {

This comment has been minimized.

@bsalamat

bsalamat Jan 25, 2019

Contributor

s/cycle/podSchedulingCycle/

@@ -372,12 +381,18 @@ func (p *PriorityQueue) backoffPod(pod *v1.Pod) {
}
}

func (p *PriorityQueue) Cycle() int64 {
p.lock.Lock()

This comment has been minimized.

@wgliang

wgliang Jan 26, 2019

Member

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

This comment has been minimized.

@cofyc

cofyc Jan 26, 2019

Author Contributor

Thanks!

@cofyc cofyc force-pushed the cofyc:fix73216-receivedMoveRequest branch 5 times, most recently from 3dd2441 to 5328984 Jan 26, 2019

@cofyc 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

@cofyc

This comment has been minimized.

Copy link
Contributor Author

cofyc commented Jan 26, 2019

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

@cofyc cofyc force-pushed the cofyc:fix73216-receivedMoveRequest branch from 5a624f5 to 0e0d4cb Jan 29, 2019

@cofyc

This comment has been minimized.

Copy link
Contributor Author

cofyc commented Jan 29, 2019

Test scenario is described too. PTAL.

@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Jan 29, 2019

/retest

@@ -59,7 +59,13 @@ const unschedulableQTimeInterval = 60 * time.Second
type SchedulingQueue interface {
Add(pod *v1.Pod) error
AddIfNotPresent(pod *v1.Pod) error
AddUnschedulableIfNotPresent(pod *v1.Pod) error
// AddUnschedulableIfNotPresent adds an unschedulable pod back to the

This comment has been minimized.

@resouer

resouer Jan 29, 2019

Member

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().

This comment has been minimized.

@cofyc

cofyc Jan 30, 2019

Author Contributor

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

// whether we should move it to active queue instead.
AddUnschedulableIfNotPresent(pod *v1.Pod, podSchedulingCycle int64) error
// SchedulingCycle returns current scheduling cycle which should be passed
// with current pod to AddUnschedulableIfNotPresent.

This comment has been minimized.

@resouer

resouer Jan 29, 2019

Member

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.

This comment has been minimized.

@cofyc

cofyc Jan 30, 2019

Author Contributor

Updated. I think this is good enough. PTAL.

@resouer

This comment has been minimized.

Copy link
Member

resouer commented Jan 29, 2019

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

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

@cofyc cofyc force-pushed the cofyc:fix73216-receivedMoveRequest branch from 0e0d4cb to ba47bef Jan 30, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member

Huang-Wei commented Jan 30, 2019

/retest

@bsalamat
Copy link
Contributor

bsalamat left a comment

/lgtm
/approve

Thanks, @cofyc!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 30, 2019

[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

Needs approval from an approver in each of these files:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 30, 2019

/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.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 31, 2019

/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

This comment has been minimized.

Copy link
Contributor Author

cofyc commented Jan 31, 2019

/skip

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 31, 2019

@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.

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 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 k8s-ci-robot merged commit 9388a27 into kubernetes:master Jan 31, 2019

13 of 17 checks passed

pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-integration Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation cofyc authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@cofyc

This comment has been minimized.

Copy link
Contributor Author

cofyc commented Jan 31, 2019

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

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

This comment has been minimized.

@tedyu

tedyu Jan 31, 2019

Contributor

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

podSchedulingCycle > p.moveRequestCycle

This comment has been minimized.

@bsalamat

bsalamat Feb 1, 2019

Contributor

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.

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-…
…upstream-release-1.12

Automated cherry pick of #73309: Should move all unscheduable pods when we received move request to active queue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment