scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin #111206

SataQiu · 2022-07-18T07:25:40Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin

After this patch:

E0718 07:13:34.754796    9974 run.go:74] "command failed" err="initializing profiles: creating profile for scheduler name default-scheduler: initializing plugin \"NodeResourcesFit\": scoring strategy XXXX is not supported, only [LeastAllocated MostAllocated RequestedToCapacityRatio] is accepted"

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin

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

k8s-ci-robot · 2022-07-18T07:25:48Z

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

SataQiu · 2022-07-18T11:50:04Z

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

pkg/scheduler/framework/plugins/noderesources/fit.go

kerthcet · 2022-07-18T13:43:37Z

pkg/scheduler/framework/plugins/noderesources/fit.go

@@ -119,7 +120,14 @@ func NewFit(plArgs runtime.Object, h framework.Handle, fts feature.Features) (fr
 	strategy := args.ScoringStrategy.Type
 	scorePlugin, exists := nodeResourceStrategyTypeMap[strategy]
 	if !exists {
-		return nil, fmt.Errorf("scoring strategy %s is not supported", strategy)
+		supportedScoringStrategyTypes := make([]config.ScoringStrategyType, 0, len(nodeResourceStrategyTypeMap))


Would it be better to move these under ValidateNodeResourcesFitArgs?

That makes sense. The check logic has been moved to ValidateNodeResourcesFitArgs.

k8s-triage-robot · 2022-07-20T11:27:28Z

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.

SataQiu · 2022-07-21T02:10:11Z

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

kerthcet · 2022-07-21T03:14:39Z

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

@@ -31,6 +31,12 @@ import (
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
 )

+var supportedScoringStrategyType = sets.NewString(


supportedScoringStrategyTypes or supportedScoringStrategySet ?

Define a new set here may grow the maintenance costs, like when adding a new type in api package, but considering that the amount of ScoringStrategy types is quite stable, I think it's ok here.

This is a reference to ServiceType:

kubernetes/pkg/apis/core/validation/validation.go

Lines 4485 to 4486 in eb2f56c

var supportedServiceType = sets.NewString(string(core.ServiceTypeClusterIP), string(core.ServiceTypeNodePort),

string(core.ServiceTypeLoadBalancer), string(core.ServiceTypeExternalName))

It is ok, if we want to use supportedScoringStrategyTypes.

Define a new set here may grow the maintenance costs, like when adding a new type in api package, but considering that the amount of ScoringStrategy types is quite stable, I think it's ok here.

If a new type is added and we forget to update the supportedScoringStrategyTypes, I think the scheduler will fail to start with an unsupported type and we can easily find the issue and fix it. Our e2e tests should catch this.

Yes if we have tests cover this when adding a new type. I mentioned this here for you used to fetch the supported types from nodeResourceStrategyTypeMap, now you defined a new variable here. I think it may because of the package circular dependency, right?

Yes, Thanks!

SataQiu · 2022-07-23T06:13:48Z

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

kerthcet · 2022-07-23T12:20:00Z

/lgtm
/assign @ahg-g

ahg-g

For posterity, in general this tighter validation is allowed only with a bump in CC version, but this case should be fine since we fail to instantiate the plugin down stream anyways at https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/noderesources/fit.go#L120-L123

ahg-g · 2022-07-25T14:44:46Z

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

@@ -305,6 +311,9 @@ func ValidateNodeResourcesFitArgs(path *field.Path, args *config.NodeResourcesFi
 	}

 	if args.ScoringStrategy != nil {
+		if !supportedScoringStrategyTypes.Has(string(args.ScoringStrategy.Type)) {


I think we need to add context here to the path to include ScoringStrategy as a parent to the validations below.

strategyPath := path.Child("scoringStrategy")

and then use strategyPath instead of path below.

Thanks @ahg-g that is reasonable. I have updated the PR.

…pe field and print supported ScoringStrategyType list on error

kerthcet · 2022-07-27T03:02:11Z

/lgtm
Leave it to @ahg-g for final approval.

SataQiu · 2022-07-30T03:29:38Z

kindly ping @ahg-g

ahg-g · 2022-07-30T07:30:44Z

/approve

k8s-ci-robot · 2022-07-30T07:31:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, SataQiu

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~~ [ahg-g]

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 18, 2022

k8s-ci-robot requested review from denkensk and sanposhiho July 18, 2022 07:26

ahg-g reviewed Jul 18, 2022

View reviewed changes

pkg/scheduler/framework/plugins/noderesources/fit.go Outdated Show resolved Hide resolved

kerthcet reviewed Jul 18, 2022

View reviewed changes

SataQiu force-pushed the scheduler-20220718 branch from 8e5814e to ae78999 Compare July 20, 2022 10:40

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 20, 2022

SataQiu force-pushed the scheduler-20220718 branch from ae78999 to d600773 Compare July 20, 2022 15:47

kerthcet reviewed Jul 21, 2022

View reviewed changes

SataQiu force-pushed the scheduler-20220718 branch from d600773 to 334d591 Compare July 21, 2022 05:04

k8s-ci-robot assigned ahg-g and kerthcet Jul 23, 2022

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

ahg-g reviewed Jul 25, 2022

View reviewed changes

scheduler: ValidateNodeResourcesFitArgs will check ScoringStrategy.Ty…

107dee6

…pe field and print supported ScoringStrategyType list on error

SataQiu force-pushed the scheduler-20220718 branch from 334d591 to 107dee6 Compare July 26, 2022 05:23

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2022

SataQiu requested review from kerthcet and ahg-g July 27, 2022 02:22

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

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

k8s-ci-robot merged commit 2ae27c8 into kubernetes:master Jul 30, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin #111206

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin #111206

SataQiu commented Jul 18, 2022

k8s-ci-robot commented Jul 18, 2022

SataQiu commented Jul 18, 2022

kerthcet Jul 18, 2022

ahg-g Jul 18, 2022

SataQiu Jul 20, 2022

k8s-triage-robot commented Jul 20, 2022

SataQiu commented Jul 21, 2022

kerthcet Jul 21, 2022

kerthcet Jul 21, 2022

SataQiu Jul 21, 2022

SataQiu Jul 21, 2022

kerthcet Jul 21, 2022 •

edited

SataQiu Jul 21, 2022

SataQiu commented Jul 23, 2022

kerthcet commented Jul 23, 2022

ahg-g left a comment

ahg-g Jul 25, 2022

SataQiu Jul 26, 2022

kerthcet commented Jul 27, 2022

SataQiu commented Jul 30, 2022

ahg-g commented Jul 30, 2022

k8s-ci-robot commented Jul 30, 2022

	var supportedServiceType = sets.NewString(string(core.ServiceTypeClusterIP), string(core.ServiceTypeNodePort),
	string(core.ServiceTypeLoadBalancer), string(core.ServiceTypeExternalName))

Navigation Menu

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin #111206

scheduler: include supported ScoringStrategyType list in error message for NodeResourcesFit plugin #111206

Conversation

SataQiu commented Jul 18, 2022

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 18, 2022

SataQiu commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Jul 20, 2022

SataQiu commented Jul 21, 2022

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 Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SataQiu commented Jul 23, 2022

kerthcet commented Jul 23, 2022

ahg-g 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 Jul 27, 2022

SataQiu commented Jul 30, 2022

ahg-g commented Jul 30, 2022

k8s-ci-robot commented Jul 30, 2022

kerthcet Jul 21, 2022 •

edited