feat: update scheduling queue with options #81263

draveness · 2019-08-11T11:19:26Z

/kind feature
/sig scheduling
/assign @bsalamat @NickrenREN

What this PR does / why we need it:

Use priorityQueueOptions to refactor the priority queue which could make it easier to do configuration.

Add pod max backoff duration to the scheduler API would cause an API review, we could do this in a follow-up PR if the implementation here makes sense.

Which issue(s) this PR fixes:

ref: #81214

Does this PR introduce a user-facing change?:

Add "podInitialBackoffDurationSeconds" and "podMaxBackoffDurationSeconds" to the scheduler config API

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

draveness · 2019-08-11T13:34:09Z

/retest Regards, Draven

…

On Aug 11, 2019, 8:59 PM +0800, Kubernetes Prow Robot ***@***.***>, wrote: @draveness: The following tests failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-integration 4b19c39 link /test pull-kubernetes-integration pull-kubernetes-e2e-gce 4b19c39 link /test pull-kubernetes-e2e-gce 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

alculquicondor · 2019-08-12T16:50:42Z

/lgtm

Just be more accurate with the PR description: feat: add options to scheduling queue reads better.

Also please add a Release note, as this offers a new configuration option.

draveness · 2019-08-12T17:00:45Z

This is an internal package which does not require a release note. Regards, Draven

…

On Aug 13, 2019, 12:53 AM +0800, Aldo Culquicondor ***@***.***>, wrote: /lgtm Just be more accurate with the PR description: feat: add options to scheduling queue reads better. Also please add a Release note, as this offers a new configuration option. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

NickrenREN

not sure if i miss sth

pkg/scheduler/internal/queue/scheduling_queue.go

draveness · 2019-08-13T07:13:39Z

Add pod max backoff duration to the scheduler API would cause an API review, we could do this in a follow-up PR if the implementation here makes sense.

…

On Aug 13, 2019, 3:11 PM +0800, Yuquan Ren ***@***.***>, wrote: @NickrenREN commented on this pull request. In pkg/scheduler/internal/queue/scheduling_queue.go: > @@ -160,12 +188,16 @@ func activeQComp(podInfo1, podInfo2 interface{}) bool { } // NewPriorityQueue creates a PriorityQueue object. -func NewPriorityQueue(stop <-chan struct{}, fwk framework.Framework) *PriorityQueue { - return NewPriorityQueueWithClock(stop, util.RealClock{}, fwk) -} +func NewPriorityQueue( I mean how can i set the max duration value, any flag for that ? or add it somewhere ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

NickrenREN · 2019-08-13T08:09:30Z

Why do we need a follow-up PR ? why not add that in this PR since they are one fix?

alculquicondor · 2019-08-13T14:08:25Z

As @draveness mentioned, that change requires an API review. Possibly a KEP. It shouldn't take too long though.

draveness · 2019-08-16T00:29:52Z

/assign @Huang-Wei

draveness · 2019-08-21T03:23:34Z

Friendly ping @Huang-Wei for review, thanks!

pkg/scheduler/internal/queue/scheduling_queue.go

NickrenREN · 2019-08-21T05:49:58Z

As @draveness mentioned, that change requires an API review. Possibly a KEP. It shouldn't take too long though.

I still prefer combining them together just like what @everpeace does in his PR. Otherwise, i do not know if this PR is meaningful.

draveness · 2019-08-21T06:14:30Z

Otherwise, i do not know if this PR is meaningful.

Use the option pattern could remove initializers like `NewPriorityQueueWithXXX`.

I still prefer combining them together just like ***@***.*** does in his PR.

I don’t have a preference on this, and I’ll update the API later in this PR. Regards, Draven

…

On Aug 21, 2019, 2:07 PM +0800, Yuquan Ren ***@***.***>, wrote: @NickrenREN commented on this pull request. In pkg/scheduler/internal/queue/scheduling_queue.go: > stop: stop, - podBackoff: NewPodBackoffMap(1*time.Second, 10*time.Second), + podBackoff: NewPodBackoffMap(1*time.Second, options.podMaxBackoffDuration), If we just want to make the backoff policy flexible, maybe the way to calculate backoff duration is also need to be configurable... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

everpeace · 2019-08-21T08:12:44Z

Use the option pattern could remove initializers like NewPriorityQueueWithXXX.

I agree this.

As I mentioned in #81263 (comment), initial backoff duration is a one of key parameters for tuning starvation in the scheduler. So I would be glad if you supported the initial backoff duration in the PR. If you don't mind, I can send a PR to your branch to support this.

draveness · 2019-09-30T01:48:41Z

Kindly ping @liggitt for approval

draveness · 2019-10-04T13:24:21Z

/retest

draveness · 2019-10-04T14:11:27Z

/retest

…

On Oct 4, 2019, 9:54 PM +0800, Kubernetes Prow Robot ***@***.***>, wrote: @draveness: The following test failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-gce-100-performance 0a831cf link /test pull-kubernetes-e2e-gce-100-performance 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

liggitt · 2019-10-08T17:04:07Z

/approve
API changes lgtm

will leave final lgtm to scheduler reviewers after rebase is done

Huang-Wei · 2019-10-08T22:36:24Z

pkg/scheduler/apis/config/types.go

+	// PodInitialBackoffSeconds is the initial backoff for unschedulable pods.
+	// If specified, it must be greater than 0. If this value is null, the default value (1s)
+	// will be used.
+	PodInitialBackoffSeconds *int64 `json:"podInitialBackoffSeconds"`


@liggitt Internal version shouldn't have "json:..." tag, right?

(not sure it's mandatory or optional, but I notice most internal types.go don't have the tag)

ah, true. good catch

thanks, this was removed

draveness · 2019-10-09T04:09:02Z

Hi @ahg-g @Huang-Wei, all the comments have been addressed and the API review has completed, please take another look.

ahg-g · 2019-10-09T11:36:07Z

thanks, please squash.

draveness · 2019-10-09T11:49:03Z

thanks, please squash.

done PTAL

ahg-g · 2019-10-09T11:57:55Z

/approve

k8s-ci-robot · 2019-10-09T11:58:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, draveness, liggitt

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:

~~cmd/kube-scheduler/OWNERS~~ [ahg-g,liggitt]
~~pkg/scheduler/OWNERS~~ [ahg-g,liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]

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

draveness · 2019-10-09T15:44:14Z

/approve

It still needs an LGTM, :)

ahg-g · 2019-10-09T15:57:56Z

/lgtm

…eduling-queue-with-options feat: update scheduling queue with options

k8s-ci-robot assigned bsalamat Aug 11, 2019

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 11, 2019

k8s-ci-robot assigned NickrenREN Aug 11, 2019

k8s-ci-robot requested review from Huang-Wei and k82cn August 11, 2019 11:20

k8s-ci-robot assigned alculquicondor Aug 12, 2019

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

NickrenREN reviewed Aug 13, 2019

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Show resolved Hide resolved

k8s-ci-robot assigned Huang-Wei Aug 16, 2019

everpeace reviewed Aug 21, 2019

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Outdated Show resolved Hide resolved

draveness force-pushed the feature/update-scheduling-queue-with-options branch from 00603d4 to 0a831cf Compare October 4, 2019 09:59

liggitt moved this from Changes requested to API review completed, 1.17 in API Reviews Oct 8, 2019

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

Huang-Wei reviewed Oct 8, 2019

View reviewed changes

draveness force-pushed the feature/update-scheduling-queue-with-options branch from 0a831cf to 4c38d90 Compare October 9, 2019 04:03

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2019

draveness force-pushed the feature/update-scheduling-queue-with-options branch from 4c38d90 to f67b516 Compare October 9, 2019 04:04

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2019

draveness requested review from Huang-Wei and NickrenREN October 9, 2019 06:18

feat: add pod initial/max backoff duration to config API

9646afb

draveness force-pushed the feature/update-scheduling-queue-with-options branch from f67b516 to 9646afb Compare October 9, 2019 11:48

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

k8s-ci-robot merged commit b8b7c37 into kubernetes:master Oct 9, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 9, 2019

draveness deleted the feature/update-scheduling-queue-with-options branch October 10, 2019 02:07

draveness mentioned this pull request Oct 10, 2019

Increase scheduling backoff queue max duration and attach specific error message to unschedulable pods #81214

Closed

ohsewon pushed a commit to ohsewon/kubernetes that referenced this pull request Oct 16, 2019

Merge pull request kubernetes#81263 from draveness/feature/update-sch…

d421a01

…eduling-queue-with-options feat: update scheduling queue with options

yqwang-ms mentioned this pull request Dec 18, 2019

Totally avoid Pod starvation (HOL blocking) or clarify the user expectation on the wiki #86373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update scheduling queue with options #81263

feat: update scheduling queue with options #81263

draveness commented Aug 11, 2019 •

edited

draveness commented Aug 11, 2019 via email

alculquicondor commented Aug 12, 2019

draveness commented Aug 12, 2019 via email

NickrenREN left a comment

draveness commented Aug 13, 2019 via email

NickrenREN commented Aug 13, 2019

alculquicondor commented Aug 13, 2019

draveness commented Aug 16, 2019

draveness commented Aug 21, 2019

NickrenREN commented Aug 21, 2019

draveness commented Aug 21, 2019 via email

everpeace commented Aug 21, 2019 •

edited

draveness commented Sep 30, 2019

draveness commented Oct 4, 2019

draveness commented Oct 4, 2019 via email

liggitt commented Oct 8, 2019

Huang-Wei Oct 8, 2019

liggitt Oct 8, 2019

draveness Oct 9, 2019

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

feat: update scheduling queue with options #81263

feat: update scheduling queue with options #81263

Conversation

draveness commented Aug 11, 2019 • edited

draveness commented Aug 11, 2019 via email

alculquicondor commented Aug 12, 2019

draveness commented Aug 12, 2019 via email

NickrenREN left a comment

Choose a reason for hiding this comment

draveness commented Aug 13, 2019 via email

NickrenREN commented Aug 13, 2019

alculquicondor commented Aug 13, 2019

draveness commented Aug 16, 2019

draveness commented Aug 21, 2019

NickrenREN commented Aug 21, 2019

draveness commented Aug 21, 2019 via email

everpeace commented Aug 21, 2019 • edited

draveness commented Sep 30, 2019

draveness commented Oct 4, 2019

draveness commented Oct 4, 2019 via email

liggitt commented Oct 8, 2019

Huang-Wei Oct 8, 2019

Choose a reason for hiding this comment

liggitt Oct 8, 2019

Choose a reason for hiding this comment

draveness Oct 9, 2019

Choose a reason for hiding this comment

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

draveness commented Oct 9, 2019

ahg-g commented Oct 9, 2019

draveness commented Aug 11, 2019 •

edited

everpeace commented Aug 21, 2019 •

edited