the scheduling queue logs the error and treats it as QueueAfterBackoff #119290

carlory · 2023-07-13T13:48:13Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

See #119155 (comment)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

QueueingHint got error in its returning value. If QueueingHint returns error, the scheduler logs the error and treats the event as QueueAfterBackoff so that the Pod wouldn't be stuck in the unschedulable pod pool.

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

sanposhiho

Thanks for a prompt action!

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

pkg/scheduler/internal/queue/scheduling_queue.go

pkg/scheduler/internal/queue/scheduling_queue_test.go

sanposhiho · 2023-07-13T14:06:18Z

/assign

carlory · 2023-07-13T14:44:37Z

@sanposhiho updated.

pkg/scheduler/framework/types.go

pkg/scheduler/internal/queue/scheduling_queue.go

carlory · 2023-07-14T03:47:41Z

@sanposhiho I pushed some changes. please review again.

codefromthecrypt · 2023-07-14T07:47:27Z

pkg/scheduler/framework/types.go

 //
 // - `pod`: the Pod to be enqueued, which is rejected by this plugin in the past.
 // - `oldObj` `newObj`: the object involved in that event.
 // - For example, the given event is "Node deleted", the `oldObj` will be that deleted Node.
 // - `oldObj` is nil if the event is add event.
 // - `newObj` is nil if the event is delete event.
-type QueueingHintFn func(logger klog.Logger, pod *v1.Pod, oldObj, newObj interface{}) QueueingHint
+type QueueingHintFn func(logger klog.Logger, pod *v1.Pod, oldObj, newObj interface{}) (QueueingHint, error)


since breaking api anyway, we can add ctx as a parameter, since almost everything in the framework accepts one (except this and one other place I noticed)

cc @sanposhiho what do you think?

There's some discuss about logger and ctx. #119155 (comment)

Ah, maybe Aldo missed my followup question on ctx in the original thread. Let me follow up again in the original PR.

I'd like to make this PR's scope only change for error. We can easily have another change for ctx if we conclude we have.

https://github.com/kubernetes/kubernetes/pull/119155/files#r1263484074

sanposhiho

tiny nits only. will /lgtm after them

/approve

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go

pkg/scheduler/internal/queue/scheduling_queue.go

carlory · 2023-08-07T03:10:12Z

/test pull-kubernetes-e2e-gce

sanposhiho · 2023-08-07T15:11:14Z

pkg/scheduler/scheduler_test.go

@@ -813,6 +813,54 @@ func Test_buildQueueingHintMap(t *testing.T) {
 },
 },
 },
+ {
+ name: "node and pod plugin (QueueingHintFn returns error)",


Why is this test case needed?
Test_buildQueueingHintMap is literary to test buildQueueingHintMap function, and the returning value from QueueingHintFn is completely not related to this test.

The reason we're comparing the result like this is Golang cannot compare the function is the same.
So, we change the result coming from QueueingHintFn in every fake plugin and check registered functions are expected ones by checking the result from QueueingHint

if fn.QueueingHintFn(logger, nil, nil, nil) != wantfns[i].QueueingHintFn(logger, nil, nil, nil) {

Then no need to compare QueueingHintFn also, because pluginName tells everything.

sanposhiho · 2023-08-07T15:16:05Z

pkg/scheduler/util/utils.go

-func As[T runtime.Object](oldObj, newobj interface{}) (T, T, error) {
+func As[T any](oldObj, newobj interface{}) (T, T, error) {


Let's add one unit test case to confirm we can convert objects to klog.KMetadata

@sanposhiho added.

…nd treats it as QueueAfterBackoff. Co-authored-by: Kensei Nakada <[email protected]> Co-authored-by: Kante Yin <[email protected]> Co-authored-by: XsWack <[email protected]>

sanposhiho

Perfect, thanks for a long journey here!

/lgtm
/hold

Please add the release note. (someone may already be using the queueing hint in out-of-tree plugins) Once done, I'll /approve.

k8s-ci-robot · 2023-09-22T13:59:54Z

LGTM label has been added.

Git tree hash: 38fb9f9ee3a9accb042704f26130e4207475ed32

sanposhiho · 2023-09-22T14:05:02Z

/release-note-edit release-note QueueingHint's function got `error` in returning value. If QueueingHint returns error, the scheduler logs the error and treats the event as QueueAfterBackoff so that the Pod wouldn't be stuck in the unschedulable pod pool.

sanposhiho · 2023-09-22T14:05:59Z

/release-note-edit

QueueingHint got error in its returning value. If QueueingHint returns error, the scheduler logs the error and treats the event as QueueAfterBackoff so that the Pod wouldn't be stuck in the unschedulable pod pool.

sanposhiho

It works.

/approve

k8s-ci-robot · 2023-09-22T14:06:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, 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~~ [sanposhiho]
~~test/integration/scheduler/OWNERS~~ [sanposhiho]

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

sanposhiho · 2023-09-22T14:06:50Z

/unhold

k8s-ci-robot requested review from chendave and denkensk July 13, 2023 13:49

carlory mentioned this pull request Jul 13, 2023

nodeaffinity: scheduler queueing hints #119155

Merged

sanposhiho reviewed Jul 13, 2023

View reviewed changes

k8s-ci-robot assigned sanposhiho Jul 13, 2023

carlory force-pushed the add-logger branch from c465cff to 5eaed78 Compare July 13, 2023 14:41

sanposhiho reviewed Jul 13, 2023

View reviewed changes

carlory force-pushed the add-logger branch 3 times, most recently from fcfad44 to 080dfa0 Compare July 14, 2023 02:22

codefromthecrypt reviewed Jul 14, 2023

View reviewed changes

sanposhiho reviewed Jul 14, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2023

carlory force-pushed the add-logger branch 2 times, most recently from b9dd8b7 to 7fd4897 Compare July 16, 2023 13:01

carlory force-pushed the add-logger branch from 718c158 to dc10c29 Compare July 26, 2023 09:00

sanposhiho reviewed Aug 5, 2023

View reviewed changes

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

carlory force-pushed the add-logger branch from dc10c29 to 09df977 Compare August 7, 2023 02:05

sanposhiho reviewed Aug 7, 2023

View reviewed changes

carlory force-pushed the add-logger branch 5 times, most recently from 3b88220 to 6eefacb Compare September 20, 2023 09:23

when the hint fn returns error, the scheduling queue logs the error a…

0105a00

…nd treats it as QueueAfterBackoff. Co-authored-by: Kensei Nakada <[email protected]> Co-authored-by: Kante Yin <[email protected]> Co-authored-by: XsWack <[email protected]>

carlory force-pushed the add-logger branch from 6eefacb to 0105a00 Compare September 21, 2023 01:40

sanposhiho reviewed Sep 22, 2023

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 22, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 22, 2023

sanposhiho approved these changes Sep 22, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 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 Sep 22, 2023

sanposhiho mentioned this pull request Sep 22, 2023

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

Open

12 tasks

k8s-ci-robot merged commit 3ac83f5 into kubernetes:master Sep 22, 2023
18 checks passed

SIG Node PR Triage automation moved this from Needs Reviewer to Done Sep 22, 2023

k8s-ci-robot added this to the v1.29 milestone Sep 22, 2023

carlory deleted the add-logger branch September 22, 2023 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

the scheduling queue logs the error and treats it as QueueAfterBackoff #119290

the scheduling queue logs the error and treats it as QueueAfterBackoff #119290

carlory commented Jul 13, 2023 •

edited by k8s-ci-robot

sanposhiho left a comment

sanposhiho commented Jul 13, 2023

carlory commented Jul 13, 2023

carlory commented Jul 14, 2023

codefromthecrypt Jul 14, 2023

carlory Jul 14, 2023

sanposhiho Jul 14, 2023

sanposhiho Jul 14, 2023

sanposhiho left a comment

carlory commented Aug 7, 2023

sanposhiho Aug 7, 2023

sanposhiho Aug 7, 2023

kerthcet Sep 20, 2023

sanposhiho Aug 7, 2023

carlory Sep 20, 2023

sanposhiho left a comment

k8s-ci-robot commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

sanposhiho left a comment

k8s-ci-robot commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

		func As[T runtime.Object](oldObj, newobj interface{}) (T, T, error) {
		func As[T any](oldObj, newobj interface{}) (T, T, error) {

the scheduling queue logs the error and treats it as QueueAfterBackoff #119290

the scheduling queue logs the error and treats it as QueueAfterBackoff #119290

Conversation

carlory commented Jul 13, 2023 • edited by k8s-ci-robot

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

sanposhiho left a comment

Choose a reason for hiding this comment

sanposhiho commented Jul 13, 2023

carlory commented Jul 13, 2023

carlory commented Jul 14, 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

sanposhiho left a comment

Choose a reason for hiding this comment

carlory commented Aug 7, 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

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 22, 2023

sanposhiho commented Sep 22, 2023

carlory commented Jul 13, 2023 •

edited by k8s-ci-robot