Graduate kube-scheduler NodeInclusionPolicy to beta #113500

kerthcet · 2022-11-01T04:04:30Z

Signed-off-by: kerthcet kerthcet@gmail.com

What type of PR is this?

/kind feature
/sig scheduling

What this PR does / why we need it:

Graduate NodeInclusionPolicy to beta, only api part.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NodeInclusionPolicy in podTopologySpread plugin is enabled by default.

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

- KEP: https://github.com/kubernetes/enhancements/issues/3094

kerthcet · 2022-11-01T04:07:20Z

/remove-sig apps api-machinery

kerthcet · 2022-11-01T07:57:39Z

/test pull-kubernetes-unit

Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet · 2022-11-01T09:18:28Z

/retest

kerthcet · 2022-11-02T10:53:34Z

This is the benchmark results for nodeInclusionPolicy:

1) 500 nodes:
NodeInclusionPolicy disabled:
BenchmarkPerfScheduling/SchedulingWithNodeInclusionPolicy/500Nodes-8     1    42853905745 ns/op
ok  	k8s.io/kubernetes/test/integration/scheduler_perf    62.579s

NodeInclusionPolicy enabled:
BenchmarkPerfScheduling/SchedulingWithNodeInclusionPolicy/500Nodes-8     1    42976119234 ns/op
ok  	k8s.io/kubernetes/test/integration/scheduler_perf    62.711s

2) 5000 nodes:
NodeInclusionPolicy disabled:
BenchmarkPerfScheduling/SchedulingWithNodeInclusionPolicy/5000Nodes-8     1    383803119491 ns/op
ok  	k8s.io/kubernetes/test/integration/scheduler_perf    555.560s

NodeInclusionPolicy enabled:
BenchmarkPerfScheduling/SchedulingWithNodeInclusionPolicy/5000Nodes-8     1    388210043641 ns/op
ok  	k8s.io/kubernetes/test/integration/scheduler_perf    560.219s

liggitt

API plumbing lgtm, will defer benchmark review and test change to scheduling reviewer

liggitt · 2022-11-02T13:44:32Z

test/integration/framework/perf_utils.go

@@ -119,7 +119,7 @@ func (p *IntegrationTestNodePreparer) PrepareNodes(nextNodeIndex int) error {
 func (p *IntegrationTestNodePreparer) CleanupNodes() error {
 	// TODO(#93794): make CleanupNodes only clean up the nodes created by this
 	// IntegrationTestNodePreparer to make this more intuitive.
-	nodes, err := GetReadySchedulableNodes(p.client)


curious why this change was needed?

When creating nodes with taints, all these nodes are unschedulable to normal pods, so we'll always return error after calling Filter(). That means no node satisfied.

checkWaitListSchedulableNodes will return all nodes with spec.unschedulable is false, the only difference here is we'll patch more nodes than before, who will not be scheduled pods. So I think it's ok.

This actually impacts all existing tests' behavior to skip nodes' health check. I'm a bit concerned on this. Can you add a param in workloadTemplate to make the behavior of "ignoring node health check or not" an option?

Should we make the parameter ignore taints, instead of ignoring isNodeSchedulable?

although... why did we even care about getting Ready nodes in the first place? I don't think we have any test cases with non-ready nodes.

Add a param is one approach, but try to figure out the reason why we want to filter out taint nodes here. They're actually schedulable if pod tolerates the taints.

kubernetes/test/integration/framework/util.go

Lines 75 to 77 in ee0b179

Filter(nodes, func(node v1.Node) bool {

return IsNodeSchedulable(&node) && isNodeUntainted(&node)

})

I think the change won't impact present tests for we have no taint-related or node-not-ready cases. But in theory, filtering out non-ready nodes when updating patches makes sense to me for the sake of performance especially we have thousands of nodes.

Another question here is when cleaning up, we only remove the readySchedulableNodes nodes, but we should remove all nodes created by itself. I used to submit a PR to fix this here #112237.

So in summary:

When calling PrepareNodes, I'll get ready and schedulable nodes including nodes with taints

When calling CleanupNodes, I'll remove all nodes created by itself as Remove only prepared nodes in performance tests #112237.

WDYT?

I think we can prepare and cleanup all nodes, given that we always create them as ready (there are no kubelets running in our tests).

I think we can prepare and cleanup all nodes, given that we always create them as ready (there are no kubelets running in our tests).

I'm ok with this. Also looking forward to @Huang-Wei 's advices.

Created a separate PR here #113615 to make this a pure one, only focus on the API part.

test/integration/framework/util.go

kerthcet · 2022-11-02T14:57:58Z

The test pull-kubernetes-e2e-kind failed mostly related to #113547

alculquicondor

/approve
/hold for nits

alculquicondor · 2022-11-02T20:27:50Z

test/integration/framework/perf_utils.go

@@ -119,7 +119,7 @@ func (p *IntegrationTestNodePreparer) PrepareNodes(nextNodeIndex int) error {
 func (p *IntegrationTestNodePreparer) CleanupNodes() error {
 	// TODO(#93794): make CleanupNodes only clean up the nodes created by this
 	// IntegrationTestNodePreparer to make this more intuitive.
-	nodes, err := GetReadySchedulableNodes(p.client)


Should we make the parameter ignore taints, instead of ignoring isNodeSchedulable?

alculquicondor · 2022-11-02T20:30:01Z

test/integration/framework/perf_utils.go

@@ -119,7 +119,7 @@ func (p *IntegrationTestNodePreparer) PrepareNodes(nextNodeIndex int) error {
 func (p *IntegrationTestNodePreparer) CleanupNodes() error {
 	// TODO(#93794): make CleanupNodes only clean up the nodes created by this
 	// IntegrationTestNodePreparer to make this more intuitive.
-	nodes, err := GetReadySchedulableNodes(p.client)


although... why did we even care about getting Ready nodes in the first place? I don't think we have any test cases with non-ready nodes.

test/integration/framework/util.go

kerthcet · 2022-11-04T03:47:15Z

Hold until #113615 got merged first.
/hold

alculquicondor · 2022-11-04T13:59:12Z

/lgtm

kerthcet · 2022-11-07T17:27:06Z

/hold cancel

kerthcet · 2022-11-07T17:27:56Z

/assign @liggitt

liggitt · 2022-11-07T20:43:38Z

/approve
for API bits

k8s-ci-robot · 2022-11-07T20:44:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, kerthcet, 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:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

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

fedebongio · 2022-11-08T21:13:40Z

/triage accepted

kerthcet mentioned this pull request Nov 1, 2022

Take taints/tolerations into consideration when calculating PodTopologySpread skew kubernetes/enhancements#3094

Open

8 tasks

k8s-ci-robot requested review from liggitt and mikedanese November 1, 2022 04:05

k8s-ci-robot removed sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Nov 1, 2022

kerthcet force-pushed the feat/graduate-nodeInclusionPoplicy-to-beta branch from f609164 to b036034 Compare November 1, 2022 06:58

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Nov 1, 2022

Feat: graduate NodeInclusionPolicy to beta

8d78b37

Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet force-pushed the feat/graduate-nodeInclusionPoplicy-to-beta branch from b036034 to 8d78b37 Compare November 1, 2022 08:07

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2022

kerthcet changed the title ~~WIP: Graduate NodeInclusionPolicy to beta~~ Graduate NodeInclusionPolicy to beta Nov 2, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2022

liggitt assigned liggitt and alculquicondor Nov 2, 2022

liggitt added this to Assigned in API Reviews Nov 2, 2022

liggitt reviewed Nov 2, 2022

View reviewed changes

liggitt moved this from Assigned to API review completed, 1.26 in API Reviews Nov 2, 2022

liggitt added the api-review Categorizes an issue or PR as actively needing an API review. label Nov 2, 2022

kerthcet force-pushed the feat/graduate-nodeInclusionPoplicy-to-beta branch from 6bd7dcb to 64adc8b Compare November 2, 2022 14:14

alculquicondor reviewed Nov 2, 2022

View reviewed changes

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

This was referenced Nov 3, 2022

Graduate scheduler nodeInclusionPolicy to beta kubernetes/website#37688

Merged

Add nodeInclusionPolicy benchmark tests to scheduler_perf #113615

Merged

kerthcet force-pushed the feat/graduate-nodeInclusionPoplicy-to-beta branch from 64adc8b to 8d78b37 Compare November 4, 2022 03:45

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022

k8s-ci-robot merged commit c326b27 into kubernetes:master Nov 8, 2022

kerthcet deleted the feat/graduate-nodeInclusionPoplicy-to-beta branch November 8, 2022 00:01

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graduate kube-scheduler NodeInclusionPolicy to beta #113500

Graduate kube-scheduler NodeInclusionPolicy to beta #113500

kerthcet commented Nov 1, 2022 •

edited

kerthcet commented Nov 1, 2022

kerthcet commented Nov 1, 2022

kerthcet commented Nov 1, 2022

kerthcet commented Nov 2, 2022 •

edited

liggitt left a comment

liggitt Nov 2, 2022

kerthcet Nov 2, 2022

Huang-Wei Nov 2, 2022

alculquicondor Nov 2, 2022

alculquicondor Nov 2, 2022

kerthcet Nov 3, 2022 •

edited

alculquicondor Nov 3, 2022

kerthcet Nov 3, 2022

kerthcet Nov 4, 2022

kerthcet commented Nov 2, 2022

alculquicondor left a comment

alculquicondor Nov 2, 2022

alculquicondor Nov 2, 2022

kerthcet commented Nov 4, 2022

alculquicondor commented Nov 4, 2022

kerthcet commented Nov 7, 2022

kerthcet commented Nov 7, 2022

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

fedebongio commented Nov 8, 2022

	Filter(nodes, func(node v1.Node) bool {
	return IsNodeSchedulable(&node) && isNodeUntainted(&node)
	})

Graduate kube-scheduler NodeInclusionPolicy to beta #113500

Graduate kube-scheduler NodeInclusionPolicy to beta #113500

Conversation

kerthcet commented Nov 1, 2022 • 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.:

kerthcet commented Nov 1, 2022

kerthcet commented Nov 1, 2022

kerthcet commented Nov 1, 2022

kerthcet commented Nov 2, 2022 • edited

liggitt left a 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

kerthcet Nov 3, 2022 • edited

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

kerthcet commented Nov 2, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Nov 4, 2022

alculquicondor commented Nov 4, 2022

kerthcet commented Nov 7, 2022

kerthcet commented Nov 7, 2022

liggitt commented Nov 7, 2022

k8s-ci-robot commented Nov 7, 2022

fedebongio commented Nov 8, 2022

kerthcet commented Nov 1, 2022 •

edited

kerthcet commented Nov 2, 2022 •

edited

kerthcet Nov 3, 2022 •

edited