nodeports: scheduler queueing hints #119176

carlory · 2023-07-08T12:08:28Z

What type of PR is this?

/kind feature

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?

NONE

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

k8s-ci-robot · 2023-07-08T12:08:36Z

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.

carlory · 2023-07-08T12:40:22Z

/assign @sanposhiho

carlory · 2023-07-10T02:57:10Z

@sanposhiho It's ready for review.

pkg/scheduler/framework/plugins/nodeports/node_ports.go

pkg/scheduler/framework/plugins/nodeports/node_ports_test.go

pkg/scheduler/framework/plugins/nodeports/node_ports.go

carlory · 2023-09-21T04:50:27Z

/test pull-kubernetes-e2e-gce

carlory · 2023-09-25T08:38:14Z

Can you take a look at it?

/cc @sanposhiho

pkg/scheduler/framework/plugins/nodeports/node_ports.go

carlory · 2023-09-25T23:12:59Z

/retest

sanposhiho

/lgtm

k8s-ci-robot · 2023-10-01T13:40:35Z

LGTM label has been added.

Git tree hash: d5b987cf6c70d0e1d763130060171d279b87524b

sanposhiho · 2023-10-01T13:42:29Z

@kubernetes/sig-scheduling-approvers I /lgtmed it but I'm assigned to the approver on this PR as well.
Please anyone else give a review for /approve.

alculquicondor

/approve
/hold
to update comment

pkg/scheduler/framework/plugins/nodeports/node_ports.go

alculquicondor · 2023-10-05T19:25:17Z

pkg/scheduler/framework/plugins/nodeports/node_ports.go

+		// TODO: Ideally, it's supposed to register only NodeCreated, because NodeUpdated event never means to have any free ports for the Pod.
+		// But, we may miss NodeCreated event due to preCheck.
+		// See: https://github.com/kubernetes/kubernetes/issues/109437
+		// And, we can remove NodeUpdated event once https://github.com/kubernetes/kubernetes/issues/110175 is solved.


Is there already an ongoing PR for scheduling hints for node affinity?

With that implementation, maybe we can skip nodeaffinity precheck if the SchedulingHint feature gate is enabled. And we can remove Update here.

Is there already an ongoing PR for scheduling hints for node affinity?
@alculquicondor Yes, please review #119155

With that implementation, maybe we can skip nodeaffinity precheck if the SchedulingHint feature gate is enabled. And we can remove Update here.

@sanposhiho what do you think ?

maybe we can skip nodeaffinity precheck if the SchedulingHint feature gate is enabled. And we can remove Update here.

The existing preCheck checks more than just NodeAffinity.

kubernetes/pkg/scheduler/eventhandlers.go

Line 572 in 10827a1

func preCheckForNode(nodeInfo *framework.NodeInfo) queue.PreEnqueueCheck {

For example, unready Node is created → the Pod was failed by nodeport, but nodeport plugin doesn't get informed of this NodeCreated event because preCheck filters out such events for Node which is tainted. → Node is now ready and untainted and this event should be delivered to plugins (unless other things in preCheck still reject it), but as NodeUpdate.
This kind of scenarios could still happen. So, I'd suggest we keep it until we completely eliminate a whole preCheck.

Right, so once we have the implementation for every plugin that is related to the existing prechecks, we can disable them based on the feature gate.

WDYT?

Regardless, we can proceed with this PR
/approve

/hold cancel

Right, so once we have the implementation for every plugin that is related to the existing prechecks, we can disable them based on the feature gate.

Yes.

Co-authored-by: Kensei Nakada <handbomusic@gmail.com> Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

carlory · 2023-10-08T04:10:57Z

/test pull-kubernetes-e2e-kind

carlory · 2023-10-09T01:17:37Z

/cc @alculquicondor @sanposhiho

sanposhiho

/lgtm

k8s-ci-robot · 2023-10-09T01:33:15Z

LGTM label has been added.

Git tree hash: 872b2e065b0b7b407c100b46312d13a2013e4e6b

k8s-ci-robot · 2023-10-10T15:22:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 8, 2023

k8s-ci-robot requested review from chendave and damemi July 8, 2023 12:09

carlory force-pushed the fix-118893-2 branch from 6bf7f5e to e1293a7 Compare July 8, 2023 12:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2023

k8s-ci-robot assigned sanposhiho Jul 8, 2023

carlory force-pushed the fix-118893-2 branch from e1293a7 to 4dd90c1 Compare July 10, 2023 02:55

carlory force-pushed the fix-118893-2 branch from 4dd90c1 to c4a579b Compare July 10, 2023 02:58

carlory commented Jul 10, 2023

View reviewed changes

pkg/scheduler/framework/plugins/nodeports/node_ports.go Show resolved Hide resolved

carlory force-pushed the fix-118893-2 branch from c4a579b to 623e7cf Compare July 10, 2023 08:41

wackxu reviewed Sep 15, 2023

View reviewed changes

carlory force-pushed the fix-118893-2 branch from 623e7cf to 276e642 Compare September 20, 2023 09:09

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

carlory force-pushed the fix-118893-2 branch from 276e642 to 328d603 Compare September 21, 2023 01:51

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

carlory force-pushed the fix-118893-2 branch from 328d603 to f5fbaba Compare September 25, 2023 01:25

k8s-ci-robot requested a review from sanposhiho September 25, 2023 08:38

carlory commented Sep 25, 2023

View reviewed changes

pkg/scheduler/framework/plugins/nodeports/node_ports.go Outdated Show resolved Hide resolved

sanposhiho reviewed Sep 25, 2023

View reviewed changes

pkg/scheduler/framework/plugins/nodeports/node_ports.go Outdated Show resolved Hide resolved

carlory force-pushed the fix-118893-2 branch 3 times, most recently from f4d60aa to 7f1f07e Compare September 25, 2023 10:02

sanposhiho reviewed Oct 1, 2023

View reviewed changes

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

alculquicondor reviewed Oct 5, 2023

View reviewed changes

k8s-ci-robot requested a review from sanposhiho October 8, 2023 03:31

nodeports: scheduler queueing hints

7cba35f

Co-authored-by: Kensei Nakada <handbomusic@gmail.com> Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

carlory force-pushed the fix-118893-2 branch from 04e10cb to 7cba35f Compare October 8, 2023 03:35

k8s-ci-robot requested a review from alculquicondor October 9, 2023 01:17

sanposhiho reviewed Oct 9, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 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 10, 2023

k8s-ci-robot merged commit 46c3078 into kubernetes:master Oct 10, 2023
12 of 14 checks passed

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

sanposhiho mentioned this pull request Dec 5, 2023

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

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nodeports: scheduler queueing hints #119176

nodeports: scheduler queueing hints #119176

carlory commented Jul 8, 2023

k8s-ci-robot commented Jul 8, 2023

carlory commented Jul 8, 2023

carlory commented Jul 10, 2023

carlory commented Sep 21, 2023

carlory commented Sep 25, 2023

carlory commented Sep 25, 2023

sanposhiho left a comment

k8s-ci-robot commented Oct 1, 2023

sanposhiho commented Oct 1, 2023

alculquicondor left a comment

alculquicondor Oct 5, 2023

carlory Oct 8, 2023

sanposhiho Oct 8, 2023 •

edited

alculquicondor Oct 10, 2023

alculquicondor Oct 10, 2023

sanposhiho Nov 21, 2023

carlory commented Oct 8, 2023

carlory commented Oct 9, 2023

sanposhiho left a comment

k8s-ci-robot commented Oct 9, 2023

k8s-ci-robot commented Oct 10, 2023

nodeports: scheduler queueing hints #119176

nodeports: scheduler queueing hints #119176

Conversation

carlory commented Jul 8, 2023

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 8, 2023

carlory commented Jul 8, 2023

carlory commented Jul 10, 2023

carlory commented Sep 21, 2023

carlory commented Sep 25, 2023

carlory commented Sep 25, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 1, 2023

sanposhiho commented Oct 1, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Oct 5, 2023

Choose a reason for hiding this comment

carlory Oct 8, 2023

Choose a reason for hiding this comment

sanposhiho Oct 8, 2023 • edited

Choose a reason for hiding this comment

alculquicondor Oct 10, 2023

Choose a reason for hiding this comment

alculquicondor Oct 10, 2023

Choose a reason for hiding this comment

sanposhiho Nov 21, 2023

Choose a reason for hiding this comment

carlory commented Oct 8, 2023

carlory commented Oct 9, 2023

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2023

k8s-ci-robot commented Oct 10, 2023

sanposhiho Oct 8, 2023 •

edited