feature(scheduler): simplify QueueingHintFn by introducing new status `Pending` #119517

sanposhiho · 2023-07-22T15:02:08Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

QueueingHintFn returns three values, QueueSkip, QueueAfterBackoff, and QueueImmediately.
https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/types.go#L109-L121
QueueSkip is OK as it's pretty clear, but it's unclear when to return QueueAfterBackoff and when to return QueueImmediately. Then, the current unclear situation results in different opinions from different people about which value we should return like this.

#118551 (comment) is the thread which I wanted to clarify that point.
In the thread, #118551 (comment) is one idea to clarify this.
BackoffQ is a light way of keeping throughput high by preventing pods that are "permanently unschedulable" from blocking the queue. (quote from #117561 (comment)).
So, the backoff delay is a penalty of wasting the scheduling cycle -- the more rejection the Pod has gotten at the scheduling cycle, the more delay that Pod should get in backoffQ as a penalty for wasting scheduling cycles.

And based on that concept, we split the failure into two reasons:

scheduling failure: common scheduling failures like PodAffinity rejects Pod in Filter, NodeResourceFit rejects Pod in Filter, etc. Almost all scheduling failure are in this group.
other rejections: There are some rejections that are must for the scheduling. Like DRA needs to wait for the claim to be provisioned from its design - After the scheduling cycle is finished successfully, DRA needs to reject Pod once anyways in order to wait for schedulingcontext to be updated by the driver based on that scheduling result.
- Note that the scheduling cycle, used to decide the Node in this example, isn't a vain effort, considering DRA can proceed the scheduling further by the scheduling result.

And, we consider they equal when to return QueueAfterBackoff (the former) and when to return QueueImmediately (the latter).

Based on the above thoughts from the discussion, it feels like we shouldn't control how to requeue Pods via the returning values (QueueAfterBackoff/QueueImmediately). Rather, I believe we should control it how the Pod is rejected (with which status the Pod is rejected).
That's more natural than QueueAfterBackoff/QueueImmediately because we define when to return QueueAfterBackoff/QueueImmediately is tightly coupled with the reason the Pod was rejected.

This PR proposes to simplify QueueingHintFn's returning value.
QueueingHint will have only two values, Queue or QueueSkip.

And instead, introduce a new status, SuccessButReject.
SuccessButReject is used when the scheduler succeeds everything in the scheduling cycle but needs to reject Pod once, for example, in order to wait for the external component to do something.
For example, as said earlier, the DRA plugin sometimes needs to wait for the external device driver to provision the resource for the Pod based on the scheduling result.
It's different from when to return Unschedulable/UnschedulableAndUnresolvable, because in this case, the scheduling cycle, used to decide the Node in this example, isn't a vain effort, considering DRA can proceed the scheduling further by the scheduling result.

When the plugin rejectes a pod with this code, the scheduler remembers the pod and the plugin name, and when the appropriate event happens, which the plugin's QueueingHintFn returns Queue, the scheduler retries the scheduling immediately without waiting for the backoff. (Yes! it's a same behaviour as QueueImmediately that we now have)

Which issue(s) this PR fixes:

Part of: #114297

Special notes for your reviewer:

/cc @alculquicondor @kerthcet

There might be another better name instead of SuccessButReject. Please propose if any 😅
EDIT: Per discussion in KEP, we use the word Pending

Does this PR introduce a user-facing change?

The scheduler gets new statuses Pending, which are used to achieve efficient enqueueing in the scheduling queue.
QueueingHintFn interface gets simplified by introducing a new status Pending - the returning value is changed to have only Queue and QueueSkip. The scheduling queue determines whether it should treat Queue as QueueAfterBackoff or QueueImmediately via the reason why the Pod was rejected.

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

- [KEP]: http://kep.k8s.io/4247

k8s-ci-robot · 2023-07-22T15:02:16Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

sanposhiho

/hold

We should involve other approves as this PR proposes something completely new.

pkg/scheduler/framework/runtime/framework.go

pkg/scheduler/internal/queue/scheduling_queue.go

alculquicondor · 2023-07-26T13:01:17Z

cc @pohly

alculquicondor · 2023-10-18T20:29:03Z

@sanposhiho are you still working on this?

sanposhiho · 2023-10-19T01:01:50Z

Oh, sorry.. I didn't notice CI failed and I thought it's ready to get reviews. Let me modify it.

alculquicondor · 2023-10-19T17:32:20Z

pkg/scheduler/framework/interface.go

+ // So, Pods rejected by such reasons don't need to suffer a penalty (backoff).
+ // When the scheduling queue requeues Pods, which was rejected with Pending in the last scheduling,
+ // the Pod goes to activeQ directly ignoring backoff.
+ Pending


I prefer we keep them separate, to avoid fitting too many concepts into one word.

alculquicondor · 2023-10-19T17:34:16Z

pkg/scheduler/framework/interface.go

@@ -151,7 +180,7 @@ type Status struct {
 reasons []string
 err error
 // failedPlugin is an optional field that records the plugin name a Pod failed by.
- // It's set by the framework when code is Error, Unschedulable or UnschedulableAndUnresolvable.
+ // It's set by the framework when code is Unschedulable or UnschedulableAndUnresolvable.


or Pending?

maybe failedPlugin is not the best name, but we can rename it in a separate PR. Maybe plugin is enough.

we can rename it in a separate PR. Maybe plugin is enough.

👍

alculquicondor · 2023-10-19T17:35:30Z

pkg/scheduler/framework/interface_test.go

@@ -131,6 +131,7 @@ func TestStatusCodes(t *testing.T) {
 assertStatusCode(t, UnschedulableAndUnresolvable, 3)
 assertStatusCode(t, Wait, 4)
 assertStatusCode(t, Skip, 5)
+ assertStatusCode(t, Pending, 6)


I actually don't see the point of this test, but we could remove it in another PR, if you agree.

100% agree.

alculquicondor · 2023-10-19T17:43:38Z

pkg/scheduler/framework/interface.go

+ // because the scheduling result is used to proceed the Pod's scheduling forward,
+ // that particular scheduling cycle is failed though.
+ // So, Pods rejected by such reasons don't need to suffer a penalty (backoff).
+ // When the scheduling queue requeues Pods, which was rejected with Pending in the last scheduling,


What about this scenario?
A pod failed with two plugins, plugin A returning Pending and plugin B returning Unschedulable.
An event comes and plugin A's hint is Queue, while plugin B's is Skip.

Will this Pod re-enter the queue? I guess it does, and it should, even if it's not schedulable yet. But a scheduling attempt can clear plugin A from the list of failed plugins, right?

Yes, that Pod is moved to activeQ immediately. As you said, in the next scheduling, probably plugin A is removed from the list.

Oops the scenario I had in mind was the opposite:

A pod failed with two plugins, plugin A returning Pending and plugin B returning Unschedulable.
An event comes and plugin B's hint is Queue, while plugin A's is Skip.

But the same reasoning applies.

pkg/scheduler/framework/types.go

pkg/scheduler/internal/queue/scheduling_queue.go

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2023-10-20T16:19:29Z

LGTM label has been added.

Git tree hash: e584ccf8c1d97596524b2fdc504d9c2f8be55c5f

alculquicondor · 2023-10-20T16:19:43Z

/retest
Due to Pod got deleted unexpectedly

k8s-ci-robot · 2023-10-20T16:19:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho

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:

~~pkg/scheduler/OWNERS~~ [alculquicondor,sanposhiho]
~~test/integration/scheduler/OWNERS~~ [alculquicondor,sanposhiho]

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

alculquicondor · 2023-10-20T16:19:53Z

/hold cancel

sanposhiho · 2023-10-22T05:21:23Z

@alculquicondor Thanks.

I opened two follow up PRs:

k8s-ci-robot requested a review from alculquicondor July 22, 2023 15:02

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 22, 2023

k8s-ci-robot requested a review from kerthcet July 22, 2023 15:02

sanposhiho force-pushed the block-status branch 3 times, most recently from 5d9d802 to c0d16fd Compare July 22, 2023 15:09

sanposhiho commented Jul 22, 2023

View reviewed changes

pkg/scheduler/framework/runtime/framework.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2023

sanposhiho force-pushed the block-status branch from c0d16fd to ab000b5 Compare July 22, 2023 15:26

sanposhiho mentioned this pull request Jul 22, 2023

feature(scheduler): implement ClusterEventWithHint to filter out useless events #118551

Merged

sanposhiho force-pushed the block-status branch 3 times, most recently from 71c9512 to 428a670 Compare July 23, 2023 06:36

bart0sh added this to Triage in SIG Node PR Triage Jul 23, 2023

bart0sh moved this from Triage to Needs Approver in SIG Node PR Triage Jul 25, 2023

wackxu reviewed Jul 26, 2023

View reviewed changes

sanposhiho mentioned this pull request Oct 15, 2023

[Umbrella] Implement QueueingHintFn in in-tree plugins #118893

Open

12 tasks

pacoxu moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Oct 17, 2023

sanposhiho force-pushed the block-status branch 4 times, most recently from 6e2aca3 to c8f8820 Compare October 19, 2023 11:00

feature(scheduler): simplify QueueingHint by introducing new statuses

cb5dc46

sanposhiho force-pushed the block-status branch from c8f8820 to cb5dc46 Compare October 19, 2023 11:02

alculquicondor reviewed Oct 19, 2023

View reviewed changes

fix based on reviews

4f5bc7e

alculquicondor reviewed Oct 20, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Oct 20, 2023

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2023

k8s-ci-robot merged commit 8d4ccd6 into kubernetes:master Oct 20, 2023
18 checks passed

SIG Node PR Triage automation moved this from Waiting on Author to Done Oct 20, 2023

k8s-ci-robot added this to the v1.29 milestone Oct 20, 2023

This was referenced Oct 22, 2023

cleanup: remove useless test #121418

Merged

fix(scheduler): don't set plugin in cases of errors #121419

Closed

This was referenced Oct 24, 2023

cleanup: rename failedPlugin to plugin in framework.Status #121469

Merged

QueueingHint for CSILimit when deleting pods #121508

Merged

sanposhiho changed the title ~~feature(scheduler): simplify QueueingHintFn by introducing new statuses~~ feature(scheduler): simplify QueueingHintFn by introducing new status Pending Oct 27, 2023

rayandas mentioned this pull request Oct 29, 2023

Per-plugin callback functions for accurate requeueing in kube-scheduler kubernetes/enhancements#4247

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(scheduler): simplify QueueingHintFn by introducing new status `Pending` #119517

feature(scheduler): simplify QueueingHintFn by introducing new status `Pending` #119517

sanposhiho commented Jul 22, 2023 •

edited

k8s-ci-robot commented Jul 22, 2023

sanposhiho left a comment

alculquicondor commented Jul 26, 2023

alculquicondor commented Oct 18, 2023

sanposhiho commented Oct 19, 2023

alculquicondor Oct 19, 2023

alculquicondor Oct 19, 2023

sanposhiho Oct 20, 2023

alculquicondor Oct 19, 2023

sanposhiho Oct 20, 2023

alculquicondor Oct 19, 2023

sanposhiho Oct 20, 2023

alculquicondor Oct 20, 2023

alculquicondor left a comment

k8s-ci-robot commented Oct 20, 2023

alculquicondor commented Oct 20, 2023

k8s-ci-robot commented Oct 20, 2023

alculquicondor commented Oct 20, 2023

sanposhiho commented Oct 22, 2023

feature(scheduler): simplify QueueingHintFn by introducing new status Pending #119517

feature(scheduler): simplify QueueingHintFn by introducing new status Pending #119517

Conversation

sanposhiho commented Jul 22, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jul 22, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

alculquicondor commented Jul 26, 2023

alculquicondor commented Oct 18, 2023

sanposhiho commented Oct 19, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 20, 2023

alculquicondor commented Oct 20, 2023

k8s-ci-robot commented Oct 20, 2023

alculquicondor commented Oct 20, 2023

sanposhiho commented Oct 22, 2023

feature(scheduler): simplify QueueingHintFn by introducing new status `Pending` #119517

feature(scheduler): simplify QueueingHintFn by introducing new status `Pending` #119517

sanposhiho commented Jul 22, 2023 •

edited