Merge pull request #91446 from pancernik/scheduler-plugin-args-validation-nr

Move Node Resources scheduler plugin args validation to apis/config/validation

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

@@ -133,3 +133,82 @@ func validateConstraintNotRepeat(path *field.Path, constraints []v1.TopologySpre
 	}
 	return nil
 }
+
+// ValidateRequestedToCapacityRatioArgs validates that RequestedToCapacityRatioArgs are correct.
+func ValidateRequestedToCapacityRatioArgs(args config.RequestedToCapacityRatioArgs) error {
+	if err := validateFunctionShape(args.Shape); err != nil {
+		return err
+	}
+	if err := validateResourcesNoMax(args.Resources); err != nil {
+		return err
+	}
+	return nil
+}
+
+func validateFunctionShape(shape []config.UtilizationShapePoint) error {
+	const (
+		minUtilization = 0
+		maxUtilization = 100
+		minScore       = 0
+		maxScore       = int32(config.MaxCustomPriorityScore)
+	)
+
+	if len(shape) == 0 {
+		return fmt.Errorf("at least one point must be specified")
+	}
+
+	for i := 1; i < len(shape); i++ {
+		if shape[i-1].Utilization >= shape[i].Utilization {
+			return fmt.Errorf("utilization values must be sorted. Utilization[%d]==%d >= Utilization[%d]==%d", i-1, shape[i-1].Utilization, i, shape[i].Utilization)
+		}
+	}
+
+	for i, point := range shape {
+		if point.Utilization < minUtilization {
+			return fmt.Errorf("utilization values must not be less than %d. Utilization[%d]==%d", minUtilization, i, point.Utilization)
+		}
+		if point.Utilization > maxUtilization {
+			return fmt.Errorf("utilization values must not be greater than %d. Utilization[%d]==%d", maxUtilization, i, point.Utilization)
+		}
+		if point.Score < minScore {
+			return fmt.Errorf("score values must not be less than %d. Score[%d]==%d", minScore, i, point.Score)
+		}
+		if point.Score > maxScore {
+			return fmt.Errorf("score values must not be greater than %d. Score[%d]==%d", maxScore, i, point.Score)
+		}
+	}
+
+	return nil
+}
+
+// TODO potentially replace with validateResources
+func validateResourcesNoMax(resources []config.ResourceSpec) error {
+	for _, r := range resources {
+		if r.Weight < 1 {
+			return fmt.Errorf("resource %s weight %d must not be less than 1", string(r.Name), r.Weight)
+		}
+	}
+	return nil
+}
+
+// ValidateNodeResourcesLeastAllocatedArgs validates that NodeResourcesLeastAllocatedArgs are correct.
+func ValidateNodeResourcesLeastAllocatedArgs(args *config.NodeResourcesLeastAllocatedArgs) error {
+	return validateResources(args.Resources)
+}
+
+// ValidateNodeResourcesMostAllocatedArgs validates that NodeResourcesMostAllocatedArgs are correct.
+func ValidateNodeResourcesMostAllocatedArgs(args *config.NodeResourcesMostAllocatedArgs) error {
+	return validateResources(args.Resources)
+}
+
+func validateResources(resources []config.ResourceSpec) error {
+	for _, resource := range resources {
+		if resource.Weight <= 0 {
+			return fmt.Errorf("resource Weight of %v should be a positive value, got %v", resource.Name, resource.Weight)
+		}
+		if resource.Weight > 100 {
+			return fmt.Errorf("resource Weight of %v should be less than 100, got %v", resource.Name, resource.Weight)
+		}
+	}
+	return nil
+}

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

@@ -114,7 +114,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) {
 				},
 			},
 		},
-		"max skew less than zero": {
+		"maxSkew less than zero": {
 			args: &config.PodTopologySpreadArgs{
 				DefaultConstraints: []v1.TopologySpreadConstraint{
 					{
@@ -138,7 +138,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) {
 			},
 			wantErr: `defaultConstraints[0].topologyKey: Required value: can not be empty`,
 		},
-		"WhenUnsatisfiable is empty": {
+		"whenUnsatisfiable is empty": {
 			args: &config.PodTopologySpreadArgs{
 				DefaultConstraints: []v1.TopologySpreadConstraint{
 					{
@@ -150,7 +150,7 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) {
 			},
 			wantErr: `defaultConstraints[0].whenUnsatisfiable: Required value: can not be empty`,
 		},
-		"WhenUnsatisfiable contains unsupported action": {
+		"whenUnsatisfiable contains unsupported action": {
 			args: &config.PodTopologySpreadArgs{
 				DefaultConstraints: []v1.TopologySpreadConstraint{
 					{
@@ -206,17 +206,285 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) {
 	}
 }
 
+func TestValidateRequestedToCapacityRatioArgs(t *testing.T) {
+	cases := map[string]struct {
+		args    config.RequestedToCapacityRatioArgs
+		wantErr string
+	}{
+		"valid config": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 20,
+						Score:       5,
+					},
+					{
+						Utilization: 30,
+						Score:       3,
+					},
+					{
+						Utilization: 50,
+						Score:       2,
+					},
+				},
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "custom-resource",
+						Weight: 5,
+					},
+				},
+			},
+		},
+		"no shape points": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{},
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "custom",
+						Weight: 5,
+					},
+				},
+			},
+			wantErr: `at least one point must be specified`,
+		},
+		"utilization less than min": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: -10,
+						Score:       3,
+					},
+					{
+						Utilization: 10,
+						Score:       2,
+					},
+				},
+			},
+			wantErr: `utilization values must not be less than 0. Utilization[0]==-10`,
+		},
+		"utilization greater than max": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 10,
+						Score:       3,
+					},
+					{
+						Utilization: 110,
+						Score:       2,
+					},
+				},
+			},
+			wantErr: `utilization values must not be greater than 100. Utilization[1]==110`,
+		},
+		"Utilization values in non-increasing order": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 30,
+						Score:       3,
+					},
+					{
+						Utilization: 20,
+						Score:       2,
+					},
+					{
+						Utilization: 10,
+						Score:       1,
+					},
+				},
+			},
+			wantErr: `utilization values must be sorted. Utilization[0]==30 >= Utilization[1]==20`,
+		},
+		"duplicated utilization values": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 10,
+						Score:       3,
+					},
+					{
+						Utilization: 20,
+						Score:       2,
+					},
+					{
+						Utilization: 20,
+						Score:       1,
+					},
+				},
+			},
+			wantErr: `utilization values must be sorted. Utilization[1]==20 >= Utilization[2]==20`,
+		},
+		"score less than min": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 10,
+						Score:       -1,
+					},
+					{
+						Utilization: 20,
+						Score:       2,
+					},
+				},
+			},
+			wantErr: `score values must not be less than 0. Score[0]==-1`,
+		},
+		"score greater than max": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 10,
+						Score:       3,
+					},
+					{
+						Utilization: 20,
+						Score:       11,
+					},
+				},
+			},
+			wantErr: `score values must not be greater than 10. Score[1]==11`,
+		},
+		"resources weight less than 1": {
+			args: config.RequestedToCapacityRatioArgs{
+				Shape: []config.UtilizationShapePoint{
+					{
+						Utilization: 10,
+						Score:       1,
+					},
+				},
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "custom",
+						Weight: 0,
+					},
+				},
+			},
+			wantErr: `resource custom weight 0 must not be less than 1`,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := ValidateRequestedToCapacityRatioArgs(tc.args)
+			assertErr(t, tc.wantErr, err)
+		})
+	}
+}
+
+func TestValidateNodeResourcesLeastAllocatedArgs(t *testing.T) {
+	cases := map[string]struct {
+		args    *config.NodeResourcesLeastAllocatedArgs
+		wantErr string
+	}{
+		"valid config": {
+			args: &config.NodeResourcesLeastAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "cpu",
+						Weight: 50,
+					},
+					{
+						Name:   "memory",
+						Weight: 30,
+					},
+				},
+			},
+		},
+		"weight less than min": {
+			args: &config.NodeResourcesLeastAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "cpu",
+						Weight: 0,
+					},
+				},
+			},
+			wantErr: `resource Weight of cpu should be a positive value, got 0`,
+		},
+		"weight more than max": {
+			args: &config.NodeResourcesLeastAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "memory",
+						Weight: 101,
+					},
+				},
+			},
+			wantErr: `resource Weight of memory should be less than 100, got 101`,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := ValidateNodeResourcesLeastAllocatedArgs(tc.args)
+			assertErr(t, tc.wantErr, err)
+		})
+	}
+}
+
+func TestValidateNodeResourcesMostAllocatedArgs(t *testing.T) {
+	cases := map[string]struct {
+		args    *config.NodeResourcesMostAllocatedArgs
+		wantErr string
+	}{
+		"valid config": {
+			args: &config.NodeResourcesMostAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "cpu",
+						Weight: 70,
+					},
+					{
+						Name:   "memory",
+						Weight: 40,
+					},
+				},
+			},
+		},
+		"weight less than min": {
+			args: &config.NodeResourcesMostAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "cpu",
+						Weight: -1,
+					},
+				},
+			},
+			wantErr: `resource Weight of cpu should be a positive value, got -1`,
+		},
+		"weight more than max": {
+			args: &config.NodeResourcesMostAllocatedArgs{
+				Resources: []config.ResourceSpec{
+					{
+						Name:   "memory",
+						Weight: 110,
+					},
+				},
+			},
+			wantErr: `resource Weight of memory should be less than 100, got 110`,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := ValidateNodeResourcesMostAllocatedArgs(tc.args)
+			assertErr(t, tc.wantErr, err)
+		})
+	}
+}
+
 func assertErr(t *testing.T, wantErr string, gotErr error) {
 	if wantErr == "" {
 		if gotErr != nil {
-			t.Fatalf("wanted err to be: 'nil', got: '%s'", gotErr.Error())
+			t.Fatalf("\nwant err to be:\n\tnil\ngot:\n\t%s", gotErr.Error())
 		}
 	} else {
 		if gotErr == nil {
-			t.Fatalf("wanted err to be: '%s', got: nil", wantErr)
+			t.Fatalf("\nwant err to be:\n\t%s\ngot:\n\tnil", wantErr)
 		}
 		if gotErr.Error() != wantErr {
-			t.Errorf("wanted err to be: '%s', got '%s'", wantErr, gotErr.Error())
+			t.Errorf("\nwant err to be:\n\t%s\ngot:\n\t%s", wantErr, gotErr.Error())
 		}
 	}
 }

pkg/scheduler/framework/plugins/noderesources/BUILD

@@ -18,6 +18,7 @@ go_library(
         "//pkg/apis/core/v1/helper:go_default_library",
         "//pkg/features:go_default_library",
         "//pkg/scheduler/apis/config:go_default_library",
+        "//pkg/scheduler/apis/config/validation:go_default_library",
         "//pkg/scheduler/framework/v1alpha1:go_default_library",
         "//pkg/scheduler/util:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",

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

@@ -23,6 +23,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
+	"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
 	framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1"
 )
 
@@ -70,14 +71,12 @@ func NewLeastAllocated(laArgs runtime.Object, h framework.FrameworkHandle) (fram
 		return nil, fmt.Errorf("want args to be of type NodeResourcesLeastAllocatedArgs, got %T", laArgs)
 	}
 
+	if err := validation.ValidateNodeResourcesLeastAllocatedArgs(args); err != nil {
+		return nil, err
+	}
+
 	resToWeightMap := make(resourceToWeightMap)
 	for _, resource := range (*args).Resources {
-		if resource.Weight <= 0 {
-			return nil, fmt.Errorf("resource Weight of %v should be a positive value, got %v", resource.Name, resource.Weight)
-		}
-		if resource.Weight > framework.MaxNodeScore {
-			return nil, fmt.Errorf("resource Weight of %v should be less than 100, got %v", resource.Name, resource.Weight)
-		}
 		resToWeightMap[v1.ResourceName(resource.Name)] = resource.Weight
 	}
 

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

@@ -23,6 +23,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
+	"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
 	framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1"
 )
 
@@ -68,15 +69,12 @@ func NewMostAllocated(maArgs runtime.Object, h framework.FrameworkHandle) (frame
 		return nil, fmt.Errorf("want args to be of type NodeResourcesMostAllocatedArgs, got %T", args)
 	}
 
-	resToWeightMap := make(resourceToWeightMap)
+	if err := validation.ValidateNodeResourcesMostAllocatedArgs(args); err != nil {
+		return nil, err
+	}
 
+	resToWeightMap := make(resourceToWeightMap)
 	for _, resource := range (*args).Resources {
-		if resource.Weight <= 0 {
-			return nil, fmt.Errorf("resource Weight of %v should be a positive value, got %v", resource.Name, resource.Weight)
-		}
-		if resource.Weight > framework.MaxNodeScore {
-			return nil, fmt.Errorf("resource Weight of %v should be less than 100, got %v", resource.Name, resource.Weight)
-		}
 		resToWeightMap[v1.ResourceName(resource.Name)] = resource.Weight
 	}
 

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

@@ -23,8 +23,8 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/pkg/scheduler/apis/config"
+	"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
 	framework "k8s.io/kubernetes/pkg/scheduler/framework/v1alpha1"
 )
 
@@ -33,8 +33,6 @@ const (
 	RequestedToCapacityRatioName = "RequestedToCapacityRatio"
 	minUtilization               = 0
 	maxUtilization               = 100
-	minScore                     = 0
-	maxScore                     = framework.MaxNodeScore
 )
 
 type functionShape []functionShapePoint
@@ -53,6 +51,10 @@ func NewRequestedToCapacityRatio(plArgs runtime.Object, handle framework.Framewo
 		return nil, err
 	}
 
+	if err := validation.ValidateRequestedToCapacityRatioArgs(args); err != nil {
+		return nil, err
+	}
+
 	shape := make([]functionShapePoint, 0, len(args.Shape))
 	for _, point := range args.Shape {
 		shape = append(shape, functionShapePoint{
@@ -64,10 +66,6 @@ func NewRequestedToCapacityRatio(plArgs runtime.Object, handle framework.Framewo
 		})
 	}
 
-	if err := validateFunctionShape(shape); err != nil {
-		return nil, err
-	}
-
 	resourceToWeightMap := make(resourceToWeightMap)
 	for _, resource := range args.Resources {
 		resourceToWeightMap[v1.ResourceName(resource.Name)] = resource.Weight
@@ -123,54 +121,8 @@ func (pl *RequestedToCapacityRatio) ScoreExtensions() framework.ScoreExtensions
 	return nil
 }
 
-func validateFunctionShape(shape functionShape) error {
-	if len(shape) == 0 {
-		return fmt.Errorf("at least one point must be specified")
-	}
-
-	for i := 1; i < len(shape); i++ {
-		if shape[i-1].utilization >= shape[i].utilization {
-			return fmt.Errorf("utilization values must be sorted. Utilization[%d]==%d >= Utilization[%d]==%d", i-1, shape[i-1].utilization, i, shape[i].utilization)
-		}
-	}
-
-	for i, point := range shape {
-		if point.utilization < minUtilization {
-			return fmt.Errorf("utilization values must not be less than %d. Utilization[%d]==%d", minUtilization, i, point.utilization)
-		}
-		if point.utilization > maxUtilization {
-			return fmt.Errorf("utilization values must not be greater than %d. Utilization[%d]==%d", maxUtilization, i, point.utilization)
-		}
-		if point.score < minScore {
-			return fmt.Errorf("score values must not be less than %d. Score[%d]==%d", minScore, i, point.score)
-		}
-		if int64(point.score) > maxScore {
-			return fmt.Errorf("score values not be greater than %d. Score[%d]==%d", maxScore, i, point.score)
-		}
-	}
-
-	return nil
-}
-
-func validateResourceWeightMap(resourceToWeightMap resourceToWeightMap) error {
-	if len(resourceToWeightMap) == 0 {
-		return fmt.Errorf("resourceToWeightMap cannot be nil")
-	}
-
-	for resource, weight := range resourceToWeightMap {
-		if weight < 1 {
-			return fmt.Errorf("resource %s weight %d must not be less than 1", string(resource), weight)
-		}
-	}
-	return nil
-}
-
 func buildRequestedToCapacityRatioScorerFunction(scoringFunctionShape functionShape, resourceToWeightMap resourceToWeightMap) func(resourceToValueMap, resourceToValueMap, bool, int, int) int64 {
 	rawScoringFunction := buildBrokenLinearFunction(scoringFunctionShape)
-	err := validateResourceWeightMap(resourceToWeightMap)
-	if err != nil {
-		klog.Error(err)
-	}
 	resourceScoringFunction := func(requested, capacity int64) int64 {
 		if capacity == 0 || requested > capacity {
 			return rawScoringFunction(maxUtilization)

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

@@ -116,47 +116,6 @@ func makePod(node string, milliCPU, memory int64) *v1.Pod {
 	}
 }
 
-func TestCreatingFunctionShapeErrorsIfEmptyPoints(t *testing.T) {
-	var err error
-	err = validateFunctionShape([]functionShapePoint{})
-	assert.Equal(t, "at least one point must be specified", err.Error())
-}
-
-func TestCreatingResourceNegativeWeight(t *testing.T) {
-	err := validateResourceWeightMap(resourceToWeightMap{v1.ResourceCPU: -1})
-	assert.Equal(t, "resource cpu weight -1 must not be less than 1", err.Error())
-}
-
-func TestCreatingResourceDefaultWeight(t *testing.T) {
-	err := validateResourceWeightMap(resourceToWeightMap{})
-	assert.Equal(t, "resourceToWeightMap cannot be nil", err.Error())
-
-}
-
-func TestCreatingFunctionShapeErrorsIfXIsNotSorted(t *testing.T) {
-	var err error
-	err = validateFunctionShape([]functionShapePoint{{10, 1}, {15, 2}, {20, 3}, {19, 4}, {25, 5}})
-	assert.Equal(t, "utilization values must be sorted. Utilization[2]==20 >= Utilization[3]==19", err.Error())
-
-	err = validateFunctionShape([]functionShapePoint{{10, 1}, {20, 2}, {20, 3}, {22, 4}, {25, 5}})
-	assert.Equal(t, "utilization values must be sorted. Utilization[1]==20 >= Utilization[2]==20", err.Error())
-}
-
-func TestCreatingFunctionPointNotInAllowedRange(t *testing.T) {
-	var err error
-	err = validateFunctionShape([]functionShapePoint{{-1, 0}, {100, 100}})
-	assert.Equal(t, "utilization values must not be less than 0. Utilization[0]==-1", err.Error())
-
-	err = validateFunctionShape([]functionShapePoint{{0, 0}, {101, 100}})
-	assert.Equal(t, "utilization values must not be greater than 100. Utilization[1]==101", err.Error())
-
-	err = validateFunctionShape([]functionShapePoint{{0, -1}, {100, 100}})
-	assert.Equal(t, "score values must not be less than 0. Score[0]==-1", err.Error())
-
-	err = validateFunctionShape([]functionShapePoint{{0, 0}, {100, 101}})
-	assert.Equal(t, "score values not be greater than 100. Score[1]==101", err.Error())
-}
-
 func TestBrokenLinearFunction(t *testing.T) {
 	type Assertion struct {
 		p        int64