scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugins/feature #103493

cofyc · 2021-07-05T16:23:08Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

fixes #103431

/sig scheduling

Which issue(s) this PR fixes:

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 · 2021-07-05T16:23:15Z

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

cofyc · 2021-07-05T16:24:10Z

/assign @Huang-Wei
this is based on #100003, I'll rebase after #100003 is merged.

Huang-Wei · 2021-07-05T16:26:58Z

this is based on #100003, I'll rebase after #100003 is merged.

Thanks, I just approved #100003.

cofyc · 2021-07-06T06:10:33Z

/skip
/retest

alculquicondor · 2021-07-06T13:12:54Z

pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go

@@ -660,7 +661,11 @@ func TestVolumeBinding(t *testing.T) {
 				}
 			}

-			pl, err := New(args, fh)
+			fts := feature.Features{
+				EnableVolumeCapacityPriority: utilfeature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority),


instead, change the test cases to have a feature.Features struct. If we are decoupling, there is no reason for the unit tests to depend on the feature gates.

fixed in 1a012fc
because the validation code depends on feature gates, I need to add a new function which we can pass feature.Features struct

fejta-bot · 2021-07-08T04:18:52Z

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.

cofyc · 2021-07-08T06:47:27Z

/retest

alculquicondor · 2021-07-08T13:50:35Z

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

@@ -29,6 +29,7 @@ import (
 	"k8s.io/component-helpers/scheduling/corev1/nodeaffinity"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
+	"k8s.io/kubernetes/pkg/scheduler/framework/plugins/feature"


This is a big no.

A better solution is to have this file define VolumeBindingArgsValidationOptions. Somewhat similar to this https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#new-enum-value-in-existing-field

cofyc · 2021-07-09T03:41:22Z

/assign alculquicondor

cofyc · 2021-07-09T04:17:20Z

/retest

cofyc · 2021-07-09T05:26:25Z

/retest

Huang-Wei · 2021-08-23T17:06:01Z

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

@@ -311,15 +311,27 @@ func ValidateNodeAffinityArgs(path *field.Path, args *config.NodeAffinityArgs) e
 	return errors.Flatten(errors.NewAggregate(errs))
 }

+// VolumeBindingArgsValidationOptions contains the different settings for validation.
+type VolumeBindingArgsValidationOptions struct {


What's the motivation for introducing this? If it's to eliminate the dependency on utilfeature, L322 still depends on it; if not, the old logic that directly checks the feature gate seems more straightforward.

This was inspired by my initial recommendation. There are 2 options:

Keep the Options but populate it outside of this package.

Leave the old logic (we probably don't care enough about the dependency). Just don't import k8s.io/kubernetes/pkg/scheduler/framework/plugins/feature here.

The 2nd option sounds neat to me.

hi, @Huang-Wei

I'd prefer keeping this option struct. It's required if we want to cut the dependency on the feature gates (typically the global utilfeature.DefaultFeatureGate). Without this option struct, we need to pass fts.Features to utilfeature.DefaultFeatureGate, because we need to call validation function in volumebinding plugin if the validation function check the features in utilfeature.DefaultFeatureGate. This will be weird.

The above comment is not for pkg/scheduler/framework/plugins/volumebinding/volume_binding.go - that refactoring is absolutely fine.

My point is it doesn't seem necessary to introduce the option in validation logic. There are no differences in terms of dependency as both depend on utilfeature, isn't it?

So I'd prefer to revert the changes in this file.

diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index 70fdabe20b1..3202841c579 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go @@ -311,27 +311,15 @@ func ValidateNodeAffinityArgs(path *field.Path, args *config.NodeAffinityArgs) e return errors.Flatten(errors.NewAggregate(errs)) } -// VolumeBindingArgsValidationOptions contains the different settings for validation. -type VolumeBindingArgsValidationOptions struct { - AllowVolumeCapacityPriority bool -} - // ValidateVolumeBindingArgs validates that VolumeBindingArgs are set correctly. func ValidateVolumeBindingArgs(path *field.Path, args *config.VolumeBindingArgs) error { - return ValidateVolumeBindingArgsWithOptions(path, args, VolumeBindingArgsValidationOptions{ - AllowVolumeCapacityPriority: utilfeature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority), - }) -} - -// ValidateVolumeBindingArgs validates that VolumeBindingArgs with scheduler features. -func ValidateVolumeBindingArgsWithOptions(path *field.Path, args *config.VolumeBindingArgs, opts VolumeBindingArgsValidationOptions) error { var allErrs field.ErrorList if args.BindTimeoutSeconds < 0 { allErrs = append(allErrs, field.Invalid(path.Child("bindTimeoutSeconds"), args.BindTimeoutSeconds, "invalid BindTimeoutSeconds, should not be a negative value")) } - if opts.AllowVolumeCapacityPriority { + if utilfeature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority) { allErrs = append(allErrs, validateFunctionShape(args.Shape, path.Child("shape"))...) } else if args.Shape != nil { // When the feature is off, return an error if the config is not nil.

The new option struct is introduced because we need to pass the fts.Features to the validation function. Especially in unit testing, if we don't the test will fail.

However, the fts.Features is populated from the global utilfeature.DefaultFeatureGate in kube-scheduler. This is the place why I think it will be weird.

I think the struct is fine. We could try to get rid of the original ValidateVolumeBindingArgs as well, but we would need to pass the options to ValidateKubeSchedulerConfiguration or something like that.

The new option struct is introduced because we need to pass the fts.Features to the validation function. Especially in unit testing, if we don't the test will fail.

That's fair.

We could try to get rid of the original ValidateVolumeBindingArgs as well

That sounds great so we don't need to build another options struct - just use plugins/../feature.go#Features. Also, we would change the validation function signature to func(path, args, options) and then parse the signature properly. Let's pursue it in a follow-up.

Let's pursue it in a follow-up.

Hmm... it seems we cannot achieve it as the fts can only be obtained from New(), which happens after validation (Config()).

alculquicondor

/lgtm

I'll leave the approval to @Huang-Wei

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

…ns/feature

Huang-Wei · 2021-09-13T18:37:05Z

/approve

Huang-Wei · 2021-09-13T18:37:14Z

/lgtm

k8s-ci-robot · 2021-09-13T18:40:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, Huang-Wei

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~~ [Huang-Wei]

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 requested review from alculquicondor and xing-yang July 5, 2021 16:23

k8s-ci-robot assigned Huang-Wei Jul 5, 2021

cofyc force-pushed the fix103431 branch from 453b0fd to dfde680 Compare July 6, 2021 04:40

alculquicondor reviewed Jul 6, 2021

View reviewed changes

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

alculquicondor reviewed Jul 8, 2021

View reviewed changes

cofyc force-pushed the fix103431 branch from 1a012fc to 3fc4b41 Compare July 9, 2021 03:40

k8s-ci-robot assigned alculquicondor Jul 9, 2021

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

cofyc force-pushed the fix103431 branch from 3fc4b41 to e1e0184 Compare August 23, 2021 02:07

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

Huang-Wei reviewed Aug 23, 2021

View reviewed changes

cofyc force-pushed the fix103431 branch from e1e0184 to 94d89dc Compare September 10, 2021 10:32

alculquicondor reviewed Sep 10, 2021

View reviewed changes

pkg/scheduler/apis/config/validation/validation_pluginargs.go Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2021

scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugi…

82b50dc

…ns/feature

cofyc force-pushed the fix103431 branch from 94d89dc to 82b50dc Compare September 11, 2021 02:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2021

k8s-ci-robot merged commit c6dfe73 into kubernetes:master Sep 13, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 13, 2021

cofyc deleted the fix103431 branch October 5, 2021 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugins/feature #103493

scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugins/feature #103493

cofyc commented Jul 5, 2021 •

edited

k8s-ci-robot commented Jul 5, 2021

cofyc commented Jul 5, 2021 •

edited

Huang-Wei commented Jul 5, 2021

cofyc commented Jul 6, 2021

alculquicondor Jul 6, 2021

cofyc Jul 8, 2021

fejta-bot commented Jul 8, 2021

cofyc commented Jul 8, 2021

alculquicondor Jul 8, 2021

cofyc Jul 9, 2021

cofyc commented Jul 9, 2021

cofyc commented Jul 9, 2021

cofyc commented Jul 9, 2021

Huang-Wei Aug 23, 2021

alculquicondor Aug 23, 2021

Huang-Wei Aug 23, 2021

cofyc Sep 10, 2021

Huang-Wei Sep 12, 2021

cofyc Sep 13, 2021

alculquicondor Sep 13, 2021

Huang-Wei Sep 13, 2021

Huang-Wei Sep 13, 2021

alculquicondor left a comment

Huang-Wei commented Sep 13, 2021

Huang-Wei commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugins/feature #103493

scheduler/volumebinding: migrate to use pkg/scheduler/framework/plugins/feature #103493

Conversation

cofyc commented Jul 5, 2021 • 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.:

k8s-ci-robot commented Jul 5, 2021

cofyc commented Jul 5, 2021 • edited

Huang-Wei commented Jul 5, 2021

cofyc commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Jul 8, 2021

cofyc commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cofyc commented Jul 9, 2021

cofyc commented Jul 9, 2021

cofyc commented Jul 9, 2021

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

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

Huang-Wei commented Sep 13, 2021

Huang-Wei commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

cofyc commented Jul 5, 2021 •

edited

cofyc commented Jul 5, 2021 •

edited