Merge pull request #100477 from chendave/validation_noderesource

Move NodeResourcesFit plugin args validation to apis/config/validation

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()
+}

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)
+			}
+		})
+	}
+}

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)
+	args, ok := plArgs.(*config.NodeResourcesFitArgs)
-	if err != nil {
+	if !ok {
-		return nil, err
+		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

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)
-			}
-		})
-	}
-}