scheduler: move percentagesOfNodesToScore to the scheduler profile #97263

SataQiu · 2020-12-13T10:28:18Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
scheduler: move percentagesOfNodesToScore to the scheduler profile

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

scheduler: support profile-level PercentagesOfNodesToScore parameter

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

k8s-ci-robot · 2020-12-13T10:28:25Z

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

k8s-ci-robot · 2020-12-13T10:28:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SataQiu
To complete the pull request process, please assign damemi, lavalamp after the PR has been reviewed.
You can assign the PR to them by writing /assign @damemi @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

fejta-bot · 2020-12-13T10:48:44Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

adtac · 2020-12-14T14:17:49Z

/assign

adtac · 2020-12-14T20:56:16Z

pkg/scheduler/apis/config/types.go

+	// Example: if the cluster size is 500 nodes and the value of this flag is 30,
+	// then scheduler stops finding further feasible nodes once it finds 150 feasible ones.
+	// When the value is 0, the PercentageOfNodesToScore in the top-level KubeSchedulerConfiguration
+	// will be used.


Worth re-stating here too that this will override the top-level configuration

pkg/scheduler/apis/config/validation/validation.go

adtac · 2020-12-14T21:12:29Z

pkg/scheduler/core/generic_scheduler.go

-	if numAllNodes < minFeasibleNodesToFind || g.percentageOfNodesToScore >= 100 {
+func (g *genericScheduler) numFeasibleNodesToFind(fwk framework.Framework, numAllNodes int32) (numNodes int32) {
+	adaptivePercentage := g.percentageOfNodesToScore
+	if p := fwk.PercentageOfNodesToScore(); p != 0 {


this defaulting should probably happen in the v1beta1/defaults.go file; then we can just use the value as-is without the p != 0 check. also helps to have all the defaulting in the same place and also in the future when we decide to remove the top-level field.

+1, defaulting needs to happen in v1beta1/defaults.go. Basically when not set we default the value to the higher level field.

pkg/scheduler/apis/config/types.go

SataQiu · 2020-12-20T07:53:03Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

SataQiu · 2020-12-21T12:58:21Z

/cc @Huang-Wei

ahg-g · 2021-01-06T15:12:46Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types.go

+	// DEPRECATED: Please use the profile-level PercentageOfNodesToScore to configure this instead.
+	// TODO(#95446): Remove PercentageOfNodesToScore from KubeSchedulerConfiguration once v1beta1 is removed.


This will still be valid in v1beta1, so I don't think we should say "deprecated" here.

ahg-g · 2021-01-06T15:13:04Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types.go

@@ -72,6 +72,9 @@ type KubeSchedulerConfiguration struct {
 	// then scheduler stops finding further feasible nodes once it finds 150 feasible ones.
 	// When the value is 0, default percentage (5%--50% based on the size of the cluster) of the
 	// nodes will be scored.
+	// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when that is not zero.


Suggested change

// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when that is not zero.

// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when set.

ahg-g · 2021-01-06T15:32:26Z

staging/src/k8s.io/kube-scheduler/config/v1beta1/types.go

+	// When the field is unset or the value is 0, the PercentageOfNodesToScore in the top-level
+	// KubeSchedulerConfiguration will be used.
+	// Note: This field will override the top-level PercentageOfNodesToScore when it is not zero.
+	PercentageOfNodesToScore *int32 `json:"percentageOfNodesToScore,omitempty"`


@liggitt do we have precedence on defaulting a field based on another? in this case, this is a per-profile field that when not set should be defaulted to the component configuration level field above (which we are planning to remove in v1beta2).

@liggitt do we have precedence on defaulting a field based on another? in this case, this is a per-profile field that when not set should be defaulted to the component configuration level field above (which we are planning to remove in v1beta2).

When we've done it in the past for REST APIs, it hasn't worked well, because of the following:

user created object with field A set and field B unset

server defaults field B based on field A

user updates object changing field A's value to something else

the previously defaulted value for field B is not valid with the new value of field A, so the user gets an error about an invalid value for a field they never set.

However, for a config file which is read only (and never has to handle update scenarios), this is probably fine.

ahg-g · 2021-01-06T15:46:02Z

pkg/scheduler/apis/config/types.go

@@ -88,6 +88,9 @@ type KubeSchedulerConfiguration struct {
 	// then scheduler stops finding further feasible nodes once it finds 150 feasible ones.
 	// When the value is 0, default percentage (5%--50% based on the size of the cluster) of the
 	// nodes will be scored.
+	// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when that is not zero.
+	// DEPRECATED: Please use the profile-level PercentageOfNodesToScore to configure this instead.
+	// TODO(#95446): Remove PercentageOfNodesToScore from KubeSchedulerConfiguration once v1beta1 is removed.


I think we can delete this field now, we don't need it in the internal representation since we will directly default the per-profile field based on this one in defaults.go

+1 to keep profile-pertentage only, particularly as Liggitt commented that obtaining value from another field doesn't work well.

Huang-Wei · 2021-01-06T20:18:11Z

pkg/scheduler/core/generic_scheduler.go

@@ -274,7 +278,7 @@ func (g *genericScheduler) findNodesThatPassFilters(ctx context.Context, fwk fra
 		return nil, err
 	}

-	numNodesToFind := g.numFeasibleNodesToFind(int32(len(allNodes)))
+	numNodesToFind := g.numFeasibleNodesToFind(fwk, int32(len(allNodes)))


If we followed @ahg-g's comment at https://github.com/kubernetes/kubernetes/pull/97263/files#r552730847 to remove the global percentage, percentageOfNodesToScore would be a profile-level field only, semantically its logic shouldn't be tightened to g (generic scheduler). I'd suggest to

either expose NodesNumToScore() in framework/inteface.go, and hence here we just call fwk.NodesNumToScore(int32(len(allNodes))).

or (preferrable IMO), make the function stateless, i.e., NodesNumToScore() to accept two parameters: percentage and num of all nodes.

Huang-Wei · 2021-01-06T20:28:29Z

pkg/scheduler/apis/config/types.go

@@ -88,6 +88,9 @@ type KubeSchedulerConfiguration struct {
 	// then scheduler stops finding further feasible nodes once it finds 150 feasible ones.
 	// When the value is 0, default percentage (5%--50% based on the size of the cluster) of the
 	// nodes will be scored.
+	// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when that is not zero.
+	// DEPRECATED: Please use the profile-level PercentageOfNodesToScore to configure this instead.
+	// TODO(#95446): Remove PercentageOfNodesToScore from KubeSchedulerConfiguration once v1beta1 is removed.


+1 to keep profile-pertentage only, particularly as Liggitt commented that obtaining value from another field doesn't work well.

k8s-ci-robot · 2021-02-06T13:13:19Z

@SataQiu: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`4b00fdf`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-kind-ipv6	`4b00fdf`	link	`/test pull-kubernetes-e2e-kind-ipv6`
pull-kubernetes-e2e-kind	`4b00fdf`	link	`/test pull-kubernetes-e2e-kind`

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.

SataQiu · 2021-03-26T06:18:17Z

/close

k8s-ci-robot · 2021-03-26T06:18:24Z

@SataQiu: PR needs rebase.

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.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 13, 2020

k8s-ci-robot requested review from damemi and dims December 13, 2020 10:29

SataQiu force-pushed the move-PercentageOfNodesToScore-20201213 branch from 0d253ac to 6cc9bf8 Compare December 13, 2020 11:14

k8s-ci-robot assigned adtac Dec 14, 2020

adtac reviewed Dec 14, 2020

View reviewed changes

SataQiu force-pushed the move-PercentageOfNodesToScore-20201213 branch from 6cc9bf8 to 1740bda Compare December 19, 2020 09:24

k8s-ci-robot requested a review from Huang-Wei December 21, 2020 12:58

dims removed their request for review January 4, 2021 19:40

ahg-g reviewed Jan 6, 2021

View reviewed changes

damemi mentioned this pull request Jan 6, 2021

Allow dynamic change of scheduler plugins in a profile kubernetes-sigs/scheduler-plugins#137

Closed

ahg-g reviewed Jan 6, 2021

View reviewed changes

Huang-Wei reviewed Jan 6, 2021

View reviewed changes

SataQiu force-pushed the move-PercentageOfNodesToScore-20201213 branch 2 times, most recently from 3e8766f to 4559909 Compare February 6, 2021 12:34

SataQiu force-pushed the move-PercentageOfNodesToScore-20201213 branch from 4559909 to d3ff601 Compare February 6, 2021 12:40

scheduler: move percentagesOfNodesToScore to the scheduler profile

4b00fdf

SataQiu force-pushed the move-PercentageOfNodesToScore-20201213 branch from d3ff601 to 4b00fdf Compare February 6, 2021 12:43

SataQiu closed this Mar 26, 2021

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

This was referenced Sep 14, 2022

scheduler: support scheduling profile-level configuration parameters #93270

Closed

Add a scheduler profile level parameter percentageOfNodesToScore #112521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: move percentagesOfNodesToScore to the scheduler profile #97263

scheduler: move percentagesOfNodesToScore to the scheduler profile #97263

SataQiu commented Dec 13, 2020

k8s-ci-robot commented Dec 13, 2020

k8s-ci-robot commented Dec 13, 2020

fejta-bot commented Dec 13, 2020

adtac commented Dec 14, 2020

adtac Dec 14, 2020

SataQiu Dec 19, 2020

adtac Dec 14, 2020

ahg-g Jan 6, 2021

SataQiu commented Dec 20, 2020

SataQiu commented Dec 21, 2020

ahg-g Jan 6, 2021

ahg-g Jan 6, 2021

ahg-g Jan 6, 2021

liggitt Jan 6, 2021

ahg-g Jan 6, 2021

Huang-Wei Jan 6, 2021

Huang-Wei Jan 6, 2021

Huang-Wei Jan 6, 2021

k8s-ci-robot commented Feb 6, 2021

SataQiu commented Mar 26, 2021

k8s-ci-robot commented Mar 26, 2021

		// DEPRECATED: Please use the profile-level PercentageOfNodesToScore to configure this instead.
		// TODO(#95446): Remove PercentageOfNodesToScore from KubeSchedulerConfiguration once v1beta1 is removed.

	// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when that is not zero.
	// Note: This field will be overridden by the profile-level PercentageOfNodesToScore when set.

scheduler: move percentagesOfNodesToScore to the scheduler profile #97263

scheduler: move percentagesOfNodesToScore to the scheduler profile #97263

Conversation

SataQiu commented Dec 13, 2020

k8s-ci-robot commented Dec 13, 2020

k8s-ci-robot commented Dec 13, 2020

fejta-bot commented Dec 13, 2020

adtac commented Dec 14, 2020

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

SataQiu commented Dec 20, 2020

SataQiu commented Dec 21, 2020

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

k8s-ci-robot commented Feb 6, 2021

SataQiu commented Mar 26, 2021

k8s-ci-robot commented Mar 26, 2021