From a0d93cc0c898cabc4e5a7acce7546138a22551fa Mon Sep 17 00:00:00 2001 From: Dave Chen Date: Tue, 23 Mar 2021 16:09:53 +0800 Subject: [PATCH] Move NodeResourcesFit plugin args validation to apis/config/validation Signed-off-by: Dave Chen --- .../validation/validation_pluginargs.go | 28 +++++++ .../validation/validation_pluginargs_test.go | 73 +++++++++++++++++++ .../framework/plugins/noderesources/fit.go | 47 ++---------- .../plugins/noderesources/fit_test.go | 72 ------------------ 4 files changed, 106 insertions(+), 114 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index 182f616bf10..7b94446db59 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go @@ -18,6 +18,7 @@ package validation import ( "fmt" + "strings" v1 "k8s.io/api/core/v1" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" @@ -310,3 +311,30 @@ func ValidateVolumeBindingArgs(args *config.VolumeBindingArgs) error { return err } + +func ValidateNodeResourcesFitArgs(args *config.NodeResourcesFitArgs) error { + var allErrs field.ErrorList + resPath := field.NewPath("ignoredResources") + for i, res := range args.IgnoredResources { + path := resPath.Index(i) + if errs := metav1validation.ValidateLabelName(res, path); len(errs) != 0 { + allErrs = append(allErrs, errs...) + } + } + + groupPath := field.NewPath("ignoredResourceGroups") + for i, group := range args.IgnoredResourceGroups { + path := groupPath.Index(i) + if strings.Contains(group, "/") { + allErrs = append(allErrs, field.Invalid(path, group, "resource group name can't contain '/'")) + } + if errs := metav1validation.ValidateLabelName(group, path); len(errs) != 0 { + allErrs = append(allErrs, errs...) + } + } + + if len(allErrs) == 0 { + return nil + } + return allErrs.ToAggregate() +} diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go index 1c9d5c922be..28fb70b5577 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go @@ -17,6 +17,8 @@ limitations under the License. package validation import ( + "fmt" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -984,3 +986,74 @@ func TestValidateVolumeBindingArgs(t *testing.T) { }) } } + +func TestValidateFitArgs(t *testing.T) { + argsTest := []struct { + name string + args config.NodeResourcesFitArgs + expect string + }{ + { + name: "IgnoredResources: too long value", + args: config.NodeResourcesFitArgs{ + IgnoredResources: []string{fmt.Sprintf("longvalue%s", strings.Repeat("a", 64))}, + }, + expect: "name part must be no more than 63 characters", + }, + { + name: "IgnoredResources: name is empty", + args: config.NodeResourcesFitArgs{ + IgnoredResources: []string{"example.com/"}, + }, + expect: "name part must be non-empty", + }, + { + name: "IgnoredResources: name has too many slash", + args: config.NodeResourcesFitArgs{ + IgnoredResources: []string{"example.com/aaa/bbb"}, + }, + expect: "a qualified name must consist of alphanumeric characters", + }, + { + name: "IgnoredResources: valid args", + args: config.NodeResourcesFitArgs{ + IgnoredResources: []string{"example.com"}, + }, + }, + { + name: "IgnoredResourceGroups: valid args ", + args: config.NodeResourcesFitArgs{ + IgnoredResourceGroups: []string{"example.com"}, + }, + }, + { + name: "IgnoredResourceGroups: illegal args", + args: config.NodeResourcesFitArgs{ + IgnoredResourceGroups: []string{"example.com/"}, + }, + expect: "name part must be non-empty", + }, + { + name: "IgnoredResourceGroups: name is too long", + args: config.NodeResourcesFitArgs{ + IgnoredResourceGroups: []string{strings.Repeat("a", 64)}, + }, + expect: "name part must be no more than 63 characters", + }, + { + name: "IgnoredResourceGroups: name cannot be contain slash", + args: config.NodeResourcesFitArgs{ + IgnoredResourceGroups: []string{"example.com/aa"}, + }, + expect: "resource group name can't contain '/'", + }, + } + + for _, test := range argsTest { + t.Run(test.name, func(t *testing.T) { + if err := ValidateNodeResourcesFitArgs(&test.args); err != nil && !strings.Contains(err.Error(), test.expect) { + t.Errorf("case[%v]: error details do not include %v", test.name, err) + } + }) + } +} diff --git a/pkg/scheduler/framework/plugins/noderesources/fit.go b/pkg/scheduler/framework/plugins/noderesources/fit.go index 28c4dc0920b..e634067a7d7 100644 --- a/pkg/scheduler/framework/plugins/noderesources/fit.go +++ b/pkg/scheduler/framework/plugins/noderesources/fit.go @@ -22,14 +22,13 @@ import ( "strings" v1 "k8s.io/api/core/v1" - metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/scheduler/apis/config" + "k8s.io/kubernetes/pkg/scheduler/apis/config/validation" "k8s.io/kubernetes/pkg/scheduler/framework" ) @@ -67,58 +66,22 @@ func (f *Fit) Name() string { return FitName } -func validateFitArgs(args config.NodeResourcesFitArgs) error { - var allErrs field.ErrorList - resPath := field.NewPath("ignoredResources") - for i, res := range args.IgnoredResources { - path := resPath.Index(i) - if errs := metav1validation.ValidateLabelName(res, path); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - } - - groupPath := field.NewPath("ignoredResourceGroups") - for i, group := range args.IgnoredResourceGroups { - path := groupPath.Index(i) - if strings.Contains(group, "/") { - allErrs = append(allErrs, field.Invalid(path, group, "resource group name can't contain '/'")) - } - if errs := metav1validation.ValidateLabelName(group, path); len(errs) != 0 { - allErrs = append(allErrs, errs...) - } - } - - if len(allErrs) == 0 { - return nil - } - return allErrs.ToAggregate() -} - // NewFit initializes a new plugin and returns it. func NewFit(plArgs runtime.Object, _ framework.Handle) (framework.Plugin, error) { - args, err := getFitArgs(plArgs) - if err != nil { - return nil, err + args, ok := plArgs.(*config.NodeResourcesFitArgs) + if !ok { + return nil, fmt.Errorf("want args to be of type NodeResourcesFitArgs, got %T", plArgs) } - if err := validateFitArgs(args); err != nil { + if err := validation.ValidateNodeResourcesFitArgs(args); err != nil { return nil, err } - return &Fit{ ignoredResources: sets.NewString(args.IgnoredResources...), ignoredResourceGroups: sets.NewString(args.IgnoredResourceGroups...), }, nil } -func getFitArgs(obj runtime.Object) (config.NodeResourcesFitArgs, error) { - ptr, ok := obj.(*config.NodeResourcesFitArgs) - if !ok { - return config.NodeResourcesFitArgs{}, fmt.Errorf("want args to be of type NodeResourcesFitArgs, got %T", obj) - } - return *ptr, nil -} - // computePodResourceRequest returns a framework.Resource that covers the largest // width in each resource dimension. Because init-containers run sequentially, we collect // the max in each dimension iteratively. In contrast, we sum the resource vectors for diff --git a/pkg/scheduler/framework/plugins/noderesources/fit_test.go b/pkg/scheduler/framework/plugins/noderesources/fit_test.go index f9529006e71..190fb705bd9 100644 --- a/pkg/scheduler/framework/plugins/noderesources/fit_test.go +++ b/pkg/scheduler/framework/plugins/noderesources/fit_test.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "reflect" - "strings" "testing" v1 "k8s.io/api/core/v1" @@ -571,74 +570,3 @@ func TestStorageRequests(t *testing.T) { } } - -func TestValidateFitArgs(t *testing.T) { - argsTest := []struct { - name string - args config.NodeResourcesFitArgs - expect string - }{ - { - name: "IgnoredResources: too long value", - args: config.NodeResourcesFitArgs{ - IgnoredResources: []string{fmt.Sprintf("longvalue%s", strings.Repeat("a", 64))}, - }, - expect: "name part must be no more than 63 characters", - }, - { - name: "IgnoredResources: name is empty", - args: config.NodeResourcesFitArgs{ - IgnoredResources: []string{"example.com/"}, - }, - expect: "name part must be non-empty", - }, - { - name: "IgnoredResources: name has too many slash", - args: config.NodeResourcesFitArgs{ - IgnoredResources: []string{"example.com/aaa/bbb"}, - }, - expect: "a qualified name must consist of alphanumeric characters", - }, - { - name: "IgnoredResources: valid args", - args: config.NodeResourcesFitArgs{ - IgnoredResources: []string{"example.com"}, - }, - }, - { - name: "IgnoredResourceGroups: valid args ", - args: config.NodeResourcesFitArgs{ - IgnoredResourceGroups: []string{"example.com"}, - }, - }, - { - name: "IgnoredResourceGroups: illegal args", - args: config.NodeResourcesFitArgs{ - IgnoredResourceGroups: []string{"example.com/"}, - }, - expect: "name part must be non-empty", - }, - { - name: "IgnoredResourceGroups: name is too long", - args: config.NodeResourcesFitArgs{ - IgnoredResourceGroups: []string{strings.Repeat("a", 64)}, - }, - expect: "name part must be no more than 63 characters", - }, - { - name: "IgnoredResourceGroups: name cannot be contain slash", - args: config.NodeResourcesFitArgs{ - IgnoredResourceGroups: []string{"example.com/aa"}, - }, - expect: "resource group name can't contain '/'", - }, - } - - for _, test := range argsTest { - t.Run(test.name, func(t *testing.T) { - if err := validateFitArgs(test.args); err != nil && !strings.Contains(err.Error(), test.expect) { - t.Errorf("case[%v]: error details do not include %v", test.name, err) - } - }) - } -}