add validate for NodeResourcesAllocatable and Coscheduling plugin

Signed-off-by: googs1025 <[email protected]>

Author: googs1025

apis/config/validation/validation_pluginargs.go

@@ -17,8 +17,11 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
+
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	schedconfig "k8s.io/kubernetes/pkg/scheduler/apis/config"
 
 	"sigs.k8s.io/scheduler-plugins/apis/config"
 )
@@ -46,3 +49,56 @@ func validateScoringStrategyType(scoringStrategy config.ScoringStrategyType, pat
 	}
 	return nil
 }
+
+func validateResources(resources []schedconfig.ResourceSpec, p *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+	for i, resource := range resources {
+		if resource.Weight <= 0 {
+			msg := fmt.Sprintf("resource weight of %v should be a positive value, got :%v", resource.Name, resource.Weight)
+			allErrs = append(allErrs, field.Invalid(p.Index(i).Child("weight"), resource.Weight, msg))
+		}
+	}
+	return allErrs
+}
+
+var supportNodeResourcesMode = sets.NewString(
+	string(config.Least),
+	string(config.Most),
+)
+
+func validateNodeResourcesModeType(mode config.ModeType, path *field.Path) *field.Error {
+	if !supportNodeResourcesMode.Has(string(mode)) {
+		return field.Invalid(path, mode, "invalid support ModeType")
+	}
+	return nil
+}
+
+func ValidateNodeResourcesAllocatableArgs(args *config.NodeResourcesAllocatableArgs, path *field.Path) error {
+	var allErrs field.ErrorList
+	if args.Resources != nil {
+		allErrs = append(allErrs, validateResources(args.Resources, path.Child("resources"))...)
+	}
+	if err := validateNodeResourcesModeType(args.Mode, path.Child("mode")); err != nil {
+		allErrs = append(allErrs, err)
+	}
+	if len(allErrs) == 0 {
+		return nil
+	}
+	return allErrs.ToAggregate()
+}
+
+func ValidateCoschedulingArgs(args *config.CoschedulingArgs, _ *field.Path) error {
+	var allErrs field.ErrorList
+	if args.PermitWaitingTimeSeconds < 0 {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("permitWaitingTimeSeconds"),
+			args.PermitWaitingTimeSeconds, "must be greater than 0"))
+	}
+	if args.PodGroupBackoffSeconds < 0 {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("podGroupBackoffSeconds"),
+			args.PodGroupBackoffSeconds, "must be greater than 0"))
+	}
+	if len(allErrs) == 0 {
+		return nil
+	}
+	return allErrs.ToAggregate()
+}

apis/config/validation/validation_plugingargs_test.go

@@ -21,6 +21,10 @@ import (
 	"strings"
 	"testing"
 
+	gocmp "github.com/google/go-cmp/cmp"
+
+	schedconfig "k8s.io/kubernetes/pkg/scheduler/apis/config"
+
 	"sigs.k8s.io/scheduler-plugins/apis/config"
 )
 
@@ -67,3 +71,125 @@ func TestValidateNodeResourceTopologyMatchArgs(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateCoschedulingArgs(t *testing.T) {
+	testCases := []struct {
+		args        *config.CoschedulingArgs
+		expectedErr error
+		description string
+	}{
+		{
+			description: "correct config with valid values",
+			args: &config.CoschedulingArgs{
+				PermitWaitingTimeSeconds: 30,
+				PodGroupBackoffSeconds:   60,
+			},
+			expectedErr: nil,
+		},
+		{
+			description: "invalid PermitWaitingTimeSeconds (negative value)",
+			args: &config.CoschedulingArgs{
+				PermitWaitingTimeSeconds: -10,
+				PodGroupBackoffSeconds:   60,
+			},
+			expectedErr: fmt.Errorf("permitWaitingTimeSeconds: Invalid value: %v: must be greater than 0", -10),
+		},
+		{
+			description: "invalid PodGroupBackoffSeconds (negative value)",
+			args: &config.CoschedulingArgs{
+				PermitWaitingTimeSeconds: 30,
+				PodGroupBackoffSeconds:   -20,
+			},
+			expectedErr: fmt.Errorf("podGroupBackoffSeconds: Invalid value: %v: must be greater than 0", -20),
+		},
+		{
+			description: "both PermitWaitingTimeSeconds and PodGroupBackoffSeconds are negative",
+			args: &config.CoschedulingArgs{
+				PermitWaitingTimeSeconds: -30,
+				PodGroupBackoffSeconds:   -20,
+			},
+			expectedErr: fmt.Errorf(
+				"[permitWaitingTimeSeconds: Invalid value: %v: must be greater than 0, podGroupBackoffSeconds: Invalid value: %v: must be greater than 0]",
+				-30, -20,
+			),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			err := ValidateCoschedulingArgs(testCase.args, nil)
+			if testCase.expectedErr != nil {
+				if err == nil {
+					t.Fatalf("expected err to equal %v not nil", testCase.expectedErr)
+				}
+				if diff := gocmp.Diff(err.Error(), testCase.expectedErr.Error()); diff != "" {
+					t.Fatalf("expected err to contain %s in error message: %s", testCase.expectedErr.Error(), err.Error())
+
+				}
+			}
+			if testCase.expectedErr == nil && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
+func TestValidateNodeResourcesAllocatableArgs(t *testing.T) {
+	testCases := []struct {
+		args        *config.NodeResourcesAllocatableArgs
+		expectedErr error
+		description string
+	}{
+		{
+			description: "correct config with valid resources and mode",
+			args: &config.NodeResourcesAllocatableArgs{
+				Resources: []schedconfig.ResourceSpec{
+					{Name: "cpu", Weight: 1},
+					{Name: "memory", Weight: 2},
+				},
+				Mode: config.Least,
+			},
+			expectedErr: nil,
+		},
+		{
+			description: "invalid resource weight (non-positive value)",
+			args: &config.NodeResourcesAllocatableArgs{
+				Resources: []schedconfig.ResourceSpec{
+					{Name: "cpu", Weight: 0},
+					{Name: "memory", Weight: -1},
+				},
+				Mode: config.Least,
+			},
+			expectedErr: fmt.Errorf("[resources[0].weight: Invalid value: %v: resource weight of cpu should be a positive value, got :%v, resources[1].weight: Invalid value: %v: resource weight of memory should be a positive value, got :%v]", 0, 0, -1, -1),
+		},
+		{
+			description: "invalid ModeType",
+			args: &config.NodeResourcesAllocatableArgs{
+				Resources: []schedconfig.ResourceSpec{
+					{Name: "cpu", Weight: 1},
+					{Name: "memory", Weight: 2},
+				},
+				Mode: "not existent",
+			},
+			expectedErr: fmt.Errorf("mode: Invalid value: \"%s\": invalid support ModeType", "not existent"),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			err := ValidateNodeResourcesAllocatableArgs(testCase.args, nil)
+			if testCase.expectedErr != nil {
+				if err == nil {
+					t.Fatalf("expected err to equal %v not nil", testCase.expectedErr)
+				}
+				if diff := gocmp.Diff(err.Error(), testCase.expectedErr.Error()); diff != "" {
+					fmt.Println(diff)
+					t.Fatalf("expected err to contain %s in error message: %s", testCase.expectedErr.Error(), err.Error())
+				}
+			}
+			if testCase.expectedErr == nil && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}

pkg/coscheduling/coscheduling.go

@@ -32,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"sigs.k8s.io/scheduler-plugins/apis/config"
+	"sigs.k8s.io/scheduler-plugins/apis/config/validation"
 	"sigs.k8s.io/scheduler-plugins/apis/scheduling"
 	"sigs.k8s.io/scheduler-plugins/apis/scheduling/v1alpha1"
 	"sigs.k8s.io/scheduler-plugins/pkg/coscheduling/core"
@@ -71,6 +72,10 @@ func New(ctx context.Context, obj runtime.Object, handle framework.Handle) (fram
 		return nil, fmt.Errorf("want args to be of type CoschedulingArgs, got %T", obj)
 	}
 
+	if err := validation.ValidateCoschedulingArgs(args, nil); err != nil {
+		return nil, err
+	}
+
 	scheme := runtime.NewScheme()
 	_ = clientscheme.AddToScheme(scheme)
 	_ = v1.AddToScheme(scheme)

pkg/noderesources/allocatable.go

@@ -19,15 +19,14 @@ package noderesources
 import (
 	"context"
 	"fmt"
-	"math"
-
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/klog/v2"
 	schedulerconfig "k8s.io/kubernetes/pkg/scheduler/apis/config"
 	"k8s.io/kubernetes/pkg/scheduler/framework"
-
+	"math"
 	"sigs.k8s.io/scheduler-plugins/apis/config"
+	"sigs.k8s.io/scheduler-plugins/apis/config/validation"
 )
 
 // Allocatable is a score plugin that favors nodes based on their allocatable
@@ -82,7 +81,7 @@ func (alloc *Allocatable) ScoreExtensions() framework.ScoreExtensions {
 func NewAllocatable(ctx context.Context, allocArgs runtime.Object, h framework.Handle) (framework.Plugin, error) {
 	logger := klog.FromContext(ctx).WithValues("plugin", AllocatableName)
 	// Start with default values.
-	mode := config.Least
+	var mode config.ModeType
 	resToWeightMap := defaultResourcesToWeightMap
 
 	// Update values from args, if specified.
@@ -91,23 +90,19 @@ func NewAllocatable(ctx context.Context, allocArgs runtime.Object, h framework.H
 		if !ok {
 			return nil, fmt.Errorf("want args to be of type NodeResourcesAllocatableArgs, got %T", allocArgs)
 		}
-		if args.Mode != "" {
-			mode = args.Mode
-			if mode != config.Least && mode != config.Most {
-				return nil, fmt.Errorf("invalid mode, got %s", mode)
-			}
+		if args.Mode == "" {
+			args.Mode = config.Least
+		}
+		if err := validation.ValidateNodeResourcesAllocatableArgs(args, nil); err != nil {
+			return nil, err
 		}
-
 		if len(args.Resources) > 0 {
-			if err := validateResources(args.Resources); err != nil {
-				return nil, err
-			}
-
 			resToWeightMap = make(resourceToWeightMap)
 			for _, resource := range args.Resources {
 				resToWeightMap[v1.ResourceName(resource.Name)] = resource.Weight
 			}
 		}
+		mode = args.Mode
 	}
 
 	return &Allocatable{

pkg/noderesources/allocatable_test.go

@@ -19,6 +19,7 @@ package noderesources
 import (
 	"context"
 	"reflect"
+	"sort"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -221,15 +222,15 @@ func TestNodeResourcesAllocatable(t *testing.T) {
 			pod:       cpuAndMemory,
 			nodeInfos: []*framework.NodeInfo{makeNodeInfo("machine", 4000, 10000)},
 			args:      config.NodeResourcesAllocatableArgs{Resources: []schedulerconfig.ResourceSpec{{Name: "memory", Weight: -1}, {Name: "cpu", Weight: 1}}},
-			wantErr:   "resource Weight of memory should be a positive value, got -1",
+			wantErr:   "resources[0].weight: Invalid value: -1: resource weight of memory should be a positive value, got :-1",
 			name:      "resource with negative weight",
 		},
 		{
 			// resource with zero weight is not allowed
 			pod:       cpuAndMemory,
 			nodeInfos: []*framework.NodeInfo{makeNodeInfo("machine", 4000, 10000)},
 			args:      config.NodeResourcesAllocatableArgs{Resources: []schedulerconfig.ResourceSpec{{Name: "memory", Weight: 1}, {Name: "cpu", Weight: 0}}},
-			wantErr:   "resource Weight of cpu should be a positive value, got 0",
+			wantErr:   "resources[1].weight: Invalid value: 0: resource weight of cpu should be a positive value, got :0",
 			name:      "resource with zero weight",
 		},
 	}
@@ -293,9 +294,13 @@ func TestNodeResourcesAllocatable(t *testing.T) {
 			if !status.IsSuccess() {
 				t.Errorf("unexpected error: %v", status)
 			}
+			sort.Slice(gotList, func(i, j int) bool {
+				return gotList[i].Name < gotList[j].Name
+			})
 
 			for i := range gotList {
 				if !reflect.DeepEqual(test.expectedList[i].Score, gotList[i].Score) {
+					t.Errorf("expected %#v, got %#v", test.expectedList[i].Name, gotList[i].Name)
 					t.Errorf("expected %#v, got %#v", test.expectedList[i].Score, gotList[i].Score)
 				}
 			}