From 77f0f442a187c81dbf15e7327219608074474874 Mon Sep 17 00:00:00 2001 From: Ling Samuel Date: Fri, 20 Nov 2020 17:39:51 +0800 Subject: [PATCH] Use field.Error(s) in scheduler plugin args validation Signed-off-by: Ling Samuel --- .../validation/validation_pluginargs.go | 151 ++++--- .../validation/validation_pluginargs_test.go | 399 +++++++++++++++--- .../framework/plugins/noderesources/BUILD | 3 + .../noderesources/least_allocated_test.go | 63 ++- .../noderesources/most_allocated_test.go | 63 ++- 5 files changed, 506 insertions(+), 173 deletions(-) diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs.go b/pkg/scheduler/apis/config/validation/validation_pluginargs.go index b60765b6c92..61647944771 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs.go @@ -29,39 +29,46 @@ import ( // ValidateDefaultPreemptionArgs validates that DefaultPreemptionArgs are correct. func ValidateDefaultPreemptionArgs(args config.DefaultPreemptionArgs) error { - if err := validateMinCandidateNodesPercentage(args.MinCandidateNodesPercentage); err != nil { - return err + var path *field.Path + var allErrs field.ErrorList + percentagePath := path.Child("minCandidateNodesPercentage") + absolutePath := path.Child("minCandidateNodesAbsolute") + if err := validateMinCandidateNodesPercentage(args.MinCandidateNodesPercentage, percentagePath); err != nil { + allErrs = append(allErrs, err) } - if err := validateMinCandidateNodesAbsolute(args.MinCandidateNodesAbsolute); err != nil { - return err + if err := validateMinCandidateNodesAbsolute(args.MinCandidateNodesAbsolute, absolutePath); err != nil { + allErrs = append(allErrs, err) } if args.MinCandidateNodesPercentage == 0 && args.MinCandidateNodesAbsolute == 0 { - return fmt.Errorf("both minCandidateNodesPercentage and minCandidateNodesAbsolute cannot be zero") + allErrs = append(allErrs, + field.Invalid(percentagePath, args.MinCandidateNodesPercentage, "cannot be zero at the same time as minCandidateNodesAbsolute"), + field.Invalid(absolutePath, args.MinCandidateNodesAbsolute, "cannot be zero at the same time as minCandidateNodesPercentage")) } - return nil + return allErrs.ToAggregate() } // validateMinCandidateNodesPercentage validates that // minCandidateNodesPercentage is within the allowed range. -func validateMinCandidateNodesPercentage(minCandidateNodesPercentage int32) error { +func validateMinCandidateNodesPercentage(minCandidateNodesPercentage int32, p *field.Path) *field.Error { if minCandidateNodesPercentage < 0 || minCandidateNodesPercentage > 100 { - return fmt.Errorf("minCandidateNodesPercentage is not in the range [0, 100]") + return field.Invalid(p, minCandidateNodesPercentage, "not in valid range [0, 100]") } return nil } // validateMinCandidateNodesAbsolute validates that minCandidateNodesAbsolute // is within the allowed range. -func validateMinCandidateNodesAbsolute(minCandidateNodesAbsolute int32) error { +func validateMinCandidateNodesAbsolute(minCandidateNodesAbsolute int32, p *field.Path) *field.Error { if minCandidateNodesAbsolute < 0 { - return fmt.Errorf("minCandidateNodesAbsolute is not in the range [0, inf)") + return field.Invalid(p, minCandidateNodesAbsolute, "not in valid range [0, inf)") } return nil } // ValidateInterPodAffinityArgs validates that InterPodAffinityArgs are correct. func ValidateInterPodAffinityArgs(args config.InterPodAffinityArgs) error { - return ValidateHardPodAffinityWeight(field.NewPath("hardPodAffinityWeight"), args.HardPodAffinityWeight) + var path *field.Path + return ValidateHardPodAffinityWeight(path.Child("hardPodAffinityWeight"), args.HardPodAffinityWeight) } // ValidateHardPodAffinityWeight validates that weight is within allowed range. @@ -72,7 +79,7 @@ func ValidateHardPodAffinityWeight(path *field.Path, w int32) error { ) if w < minHardPodAffinityWeight || w > maxHardPodAffinityWeight { - msg := fmt.Sprintf("not in valid range [%d-%d]", minHardPodAffinityWeight, maxHardPodAffinityWeight) + msg := fmt.Sprintf("not in valid range [%d, %d]", minHardPodAffinityWeight, maxHardPodAffinityWeight) return field.Invalid(path, w, msg) } return nil @@ -80,44 +87,50 @@ func ValidateHardPodAffinityWeight(path *field.Path, w int32) error { // ValidateNodeLabelArgs validates that NodeLabelArgs are correct. func ValidateNodeLabelArgs(args config.NodeLabelArgs) error { - if err := validateNoConflict(args.PresentLabels, args.AbsentLabels); err != nil { - return err - } - if err := validateNoConflict(args.PresentLabelsPreference, args.AbsentLabelsPreference); err != nil { - return err - } - return nil + var path *field.Path + var allErrs field.ErrorList + + allErrs = append(allErrs, validateNoConflict(args.PresentLabels, args.AbsentLabels, + path.Child("presentLabels"), path.Child("absentLabels"))...) + allErrs = append(allErrs, validateNoConflict(args.PresentLabelsPreference, args.AbsentLabelsPreference, + path.Child("presentLabelsPreference"), path.Child("absentLabelsPreference"))...) + + return allErrs.ToAggregate() } // validateNoConflict validates that presentLabels and absentLabels do not conflict. -func validateNoConflict(presentLabels []string, absentLabels []string) error { - m := make(map[string]struct{}, len(presentLabels)) - for _, l := range presentLabels { - m[l] = struct{}{} +func validateNoConflict(presentLabels, absentLabels []string, presentPath, absentPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + m := make(map[string]int, len(presentLabels)) // label -> index + for i, l := range presentLabels { + m[l] = i } - for _, l := range absentLabels { - if _, ok := m[l]; ok { - return fmt.Errorf("detecting at least one label (e.g., %q) that exist in both the present(%+v) and absent(%+v) label list", l, presentLabels, absentLabels) + for i, l := range absentLabels { + if j, ok := m[l]; ok { + allErrs = append(allErrs, field.Invalid(presentPath.Index(j), l, + fmt.Sprintf("conflict with %v", absentPath.Index(i).String()))) } } - return nil + return allErrs } // ValidatePodTopologySpreadArgs validates that PodTopologySpreadArgs are correct. // It replicates the validation from pkg/apis/core/validation.validateTopologySpreadConstraints // with an additional check for .labelSelector to be nil. func ValidatePodTopologySpreadArgs(args *config.PodTopologySpreadArgs) error { + var path *field.Path var allErrs field.ErrorList - if err := validateDefaultingType(field.NewPath("defaultingType"), args.DefaultingType, args.DefaultConstraints); err != nil { + if err := validateDefaultingType(path.Child("defaultingType"), args.DefaultingType, args.DefaultConstraints); err != nil { allErrs = append(allErrs, err) } - path := field.NewPath("defaultConstraints") + defaultConstraintsPath := path.Child("defaultConstraints") for i, c := range args.DefaultConstraints { - p := path.Index(i) + p := defaultConstraintsPath.Index(i) if c.MaxSkew <= 0 { f := p.Child("maxSkew") - allErrs = append(allErrs, field.Invalid(f, c.MaxSkew, "must be greater than zero")) + allErrs = append(allErrs, field.Invalid(f, c.MaxSkew, "not in valid range (0, inf)")) } allErrs = append(allErrs, validateTopologyKey(p.Child("topologyKey"), c.TopologyKey)...) if err := validateWhenUnsatisfiable(p.Child("whenUnsatisfiable"), c.WhenUnsatisfiable); err != nil { @@ -127,7 +140,7 @@ func ValidatePodTopologySpreadArgs(args *config.PodTopologySpreadArgs) error { f := field.Forbidden(p.Child("labelSelector"), "constraint must not define a selector, as they deduced for each pod") allErrs = append(allErrs, f) } - if err := validateConstraintNotRepeat(path, args.DefaultConstraints, i); err != nil { + if err := validateConstraintNotRepeat(defaultConstraintsPath, args.DefaultConstraints, i); err != nil { allErrs = append(allErrs, err) } } @@ -139,7 +152,7 @@ func ValidatePodTopologySpreadArgs(args *config.PodTopologySpreadArgs) error { func validateDefaultingType(p *field.Path, v config.PodTopologySpreadConstraintsDefaulting, constraints []v1.TopologySpreadConstraint) *field.Error { if v != config.SystemDefaulting && v != config.ListDefaulting { - return field.Invalid(p, v, fmt.Sprintf("must be one of {%q, %q}", config.SystemDefaulting, config.ListDefaulting)) + return field.NotSupported(p, v, []string{string(config.SystemDefaulting), string(config.ListDefaulting)}) } if v == config.SystemDefaulting && len(constraints) > 0 { return field.Invalid(p, v, "when .defaultConstraints are not empty") @@ -182,16 +195,14 @@ func validateConstraintNotRepeat(path *field.Path, constraints []v1.TopologySpre // 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 + var path *field.Path + var allErrs field.ErrorList + allErrs = append(allErrs, validateFunctionShape(args.Shape, path.Child("shape"))...) + allErrs = append(allErrs, validateResourcesNoMax(args.Resources, path.Child("resources"))...) + return allErrs.ToAggregate() } -func validateFunctionShape(shape []config.UtilizationShapePoint) error { +func validateFunctionShape(shape []config.UtilizationShapePoint, path *field.Path) field.ErrorList { const ( minUtilization = 0 maxUtilization = 100 @@ -199,64 +210,68 @@ func validateFunctionShape(shape []config.UtilizationShapePoint) error { maxScore = int32(config.MaxCustomPriorityScore) ) + var allErrs field.ErrorList + if len(shape) == 0 { - return fmt.Errorf("at least one point must be specified") + allErrs = append(allErrs, field.Required(path, "at least one point must be specified")) + return allErrs } 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) + allErrs = append(allErrs, field.Invalid(path.Index(i).Child("utilization"), shape[i].Utilization, "utilization values must be sorted in increasing order")) + break } } 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 < minUtilization || point.Utilization > maxUtilization { + msg := fmt.Sprintf("not in valid range [%d, %d]", minUtilization, maxUtilization) + allErrs = append(allErrs, field.Invalid(path.Index(i).Child("utilization"), point.Utilization, msg)) } - 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) + + if point.Score < minScore || point.Score > maxScore { + msg := fmt.Sprintf("not in valid range [%d, %d]", minScore, maxScore) + allErrs = append(allErrs, field.Invalid(path.Index(i).Child("score"), point.Score, msg)) } } - return nil + return allErrs } // TODO potentially replace with validateResources -func validateResourcesNoMax(resources []config.ResourceSpec) error { - for _, r := range resources { +func validateResourcesNoMax(resources []config.ResourceSpec, p *field.Path) field.ErrorList { + var allErrs field.ErrorList + for i, 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) + allErrs = append(allErrs, field.Invalid(p.Index(i).Child("weight"), r.Weight, + fmt.Sprintf("resource weight of %s not in valid range [1, inf)", r.Name))) } } - return nil + return allErrs } // ValidateNodeResourcesLeastAllocatedArgs validates that NodeResourcesLeastAllocatedArgs are correct. func ValidateNodeResourcesLeastAllocatedArgs(args *config.NodeResourcesLeastAllocatedArgs) error { - return validateResources(args.Resources) + var path *field.Path + return validateResources(args.Resources, path.Child("resources")).ToAggregate() } // ValidateNodeResourcesMostAllocatedArgs validates that NodeResourcesMostAllocatedArgs are correct. func ValidateNodeResourcesMostAllocatedArgs(args *config.NodeResourcesMostAllocatedArgs) error { - return validateResources(args.Resources) + var path *field.Path + return validateResources(args.Resources, path.Child("resources")).ToAggregate() } -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) +func validateResources(resources []config.ResourceSpec, p *field.Path) field.ErrorList { + var allErrs field.ErrorList + for i, resource := range resources { + if resource.Weight <= 0 || resource.Weight > 100 { + msg := fmt.Sprintf("resource weight of %v not in valid range (0, 100]", resource.Name) + allErrs = append(allErrs, field.Invalid(p.Index(i).Child("weight"), resource.Weight, msg)) } } - return nil + return allErrs } // ValidateNodeAffinityArgs validates that NodeAffinityArgs are correct. diff --git a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go index 681b49ba1a8..cb1fa65b90b 100644 --- a/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go +++ b/pkg/scheduler/apis/config/validation/validation_pluginargs_test.go @@ -33,8 +33,8 @@ var ( func TestValidateDefaultPreemptionArgs(t *testing.T) { cases := map[string]struct { - args config.DefaultPreemptionArgs - wantErr string + args config.DefaultPreemptionArgs + wantErrs field.ErrorList }{ "valid args (default)": { args: config.DefaultPreemptionArgs{ @@ -47,35 +47,75 @@ func TestValidateDefaultPreemptionArgs(t *testing.T) { MinCandidateNodesPercentage: -1, MinCandidateNodesAbsolute: 100, }, - wantErr: "minCandidateNodesPercentage is not in the range [0, 100]", + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesPercentage", + }, + }, }, "minCandidateNodesPercentage over 100": { args: config.DefaultPreemptionArgs{ MinCandidateNodesPercentage: 900, MinCandidateNodesAbsolute: 100, }, - wantErr: "minCandidateNodesPercentage is not in the range [0, 100]", + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesPercentage", + }, + }, }, "negative minCandidateNodesAbsolute": { args: config.DefaultPreemptionArgs{ MinCandidateNodesPercentage: 20, MinCandidateNodesAbsolute: -1, }, - wantErr: "minCandidateNodesAbsolute is not in the range [0, inf)", + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesAbsolute", + }, + }, }, "all zero": { args: config.DefaultPreemptionArgs{ MinCandidateNodesPercentage: 0, MinCandidateNodesAbsolute: 0, }, - wantErr: "both minCandidateNodesPercentage and minCandidateNodesAbsolute cannot be zero", + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesPercentage", + }, &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesAbsolute", + }, + }, + }, + "both negative": { + args: config.DefaultPreemptionArgs{ + MinCandidateNodesPercentage: -1, + MinCandidateNodesAbsolute: -1, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesPercentage", + }, &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "minCandidateNodesAbsolute", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateDefaultPreemptionArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateDefaultPreemptionArgs returned err (-want,+got):\n%s", diff) + } }) } } @@ -83,7 +123,7 @@ func TestValidateDefaultPreemptionArgs(t *testing.T) { func TestValidateInterPodAffinityArgs(t *testing.T) { cases := map[string]struct { args config.InterPodAffinityArgs - wantErr string + wantErr error }{ "valid args": { args: config.InterPodAffinityArgs{ @@ -94,28 +134,36 @@ func TestValidateInterPodAffinityArgs(t *testing.T) { args: config.InterPodAffinityArgs{ HardPodAffinityWeight: -1, }, - wantErr: `hardPodAffinityWeight: Invalid value: -1: not in valid range [0-100]`, + wantErr: &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "hardPodAffinityWeight", + }, }, "hardPodAffinityWeight more than max": { args: config.InterPodAffinityArgs{ HardPodAffinityWeight: 101, }, - wantErr: `hardPodAffinityWeight: Invalid value: 101: not in valid range [0-100]`, + wantErr: &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "hardPodAffinityWeight", + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateInterPodAffinityArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErr, err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateInterPodAffinityArgs returned err (-want,+got):\n%s", diff) + } }) } } func TestValidateNodeLabelArgs(t *testing.T) { cases := map[string]struct { - args config.NodeLabelArgs - wantErr string + args config.NodeLabelArgs + wantErrs field.ErrorList }{ "valid config": { args: config.NodeLabelArgs{ @@ -130,29 +178,73 @@ func TestValidateNodeLabelArgs(t *testing.T) { PresentLabels: []string{"label"}, AbsentLabels: []string{"label"}, }, - wantErr: `detecting at least one label (e.g., "label") that exist in both the present([label]) and absent([label]) label list`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabels[0]", + }, + }, + }, + "multiple labels conflict": { + args: config.NodeLabelArgs{ + PresentLabels: []string{"label", "label3"}, + AbsentLabels: []string{"label", "label2", "label3"}, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabels[0]", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabels[1]", + }, + }, }, "labels preference conflict": { args: config.NodeLabelArgs{ PresentLabelsPreference: []string{"label"}, AbsentLabelsPreference: []string{"label"}, }, - wantErr: `detecting at least one label (e.g., "label") that exist in both the present([label]) and absent([label]) label list`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabelsPreference[0]", + }, + }, + }, + "multiple labels preference conflict": { + args: config.NodeLabelArgs{ + PresentLabelsPreference: []string{"label", "label3"}, + AbsentLabelsPreference: []string{"label", "label2", "label3"}, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabelsPreference[0]", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "presentLabelsPreference[1]", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateNodeLabelArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateNodeLabelArgs returned err (-want,+got):\n%s", diff) + } }) } } func TestValidatePodTopologySpreadArgs(t *testing.T) { cases := map[string]struct { - args *config.PodTopologySpreadArgs - wantErr string + args *config.PodTopologySpreadArgs + wantErrs field.ErrorList }{ "valid config": { args: &config.PodTopologySpreadArgs{ @@ -182,7 +274,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[0].maxSkew: Invalid value: -1: must be greater than zero`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "defaultConstraints[0].maxSkew", + }, + }, }, "empty topology key": { args: &config.PodTopologySpreadArgs{ @@ -195,7 +292,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[0].topologyKey: Required value: can not be empty`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeRequired, + Field: "defaultConstraints[0].topologyKey", + }, + }, }, "whenUnsatisfiable is empty": { args: &config.PodTopologySpreadArgs{ @@ -208,7 +310,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[0].whenUnsatisfiable: Required value: can not be empty`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeRequired, + Field: "defaultConstraints[0].whenUnsatisfiable", + }, + }, }, "whenUnsatisfiable contains unsupported action": { args: &config.PodTopologySpreadArgs{ @@ -221,7 +328,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[0].whenUnsatisfiable: Unsupported value: "unknown action": supported values: "DoNotSchedule", "ScheduleAnyway"`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeNotSupported, + Field: "defaultConstraints[0].whenUnsatisfiable", + }, + }, }, "duplicated constraints": { args: &config.PodTopologySpreadArgs{ @@ -239,7 +351,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[1]: Duplicate value: "{node, DoNotSchedule}"`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeDuplicate, + Field: "defaultConstraints[1]", + }, + }, }, "label selector present": { args: &config.PodTopologySpreadArgs{ @@ -257,7 +374,12 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.ListDefaulting, }, - wantErr: `defaultConstraints[0].labelSelector: Forbidden: constraint must not define a selector, as they deduced for each pod`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeForbidden, + Field: "defaultConstraints[0].labelSelector", + }, + }, }, "list default constraints, no constraints": { args: &config.PodTopologySpreadArgs{ @@ -269,6 +391,17 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { DefaultingType: config.SystemDefaulting, }, }, + "wrong constraints": { + args: &config.PodTopologySpreadArgs{ + DefaultingType: "unknown", + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeNotSupported, + Field: "defaultingType", + }, + }, + }, "system default constraints, but has constraints": { args: &config.PodTopologySpreadArgs{ DefaultConstraints: []v1.TopologySpreadConstraint{ @@ -280,22 +413,29 @@ func TestValidatePodTopologySpreadArgs(t *testing.T) { }, DefaultingType: config.SystemDefaulting, }, - wantErr: `defaultingType: Invalid value: "System": when .defaultConstraints are not empty`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "defaultingType", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidatePodTopologySpreadArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidatePodTopologySpreadArgs returned err (-want,+got):\n%s", diff) + } }) } } func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { cases := map[string]struct { - args config.RequestedToCapacityRatioArgs - wantErr string + args config.RequestedToCapacityRatioArgs + wantErrs field.ErrorList }{ "valid config": { args: config.RequestedToCapacityRatioArgs{ @@ -331,7 +471,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `at least one point must be specified`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeRequired, + Field: "shape", + }, + }, }, "utilization less than min": { args: config.RequestedToCapacityRatioArgs{ @@ -346,7 +491,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `utilization values must not be less than 0. Utilization[0]==-10`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[0].utilization", + }, + }, }, "utilization greater than max": { args: config.RequestedToCapacityRatioArgs{ @@ -361,9 +511,14 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `utilization values must not be greater than 100. Utilization[1]==110`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[1].utilization", + }, + }, }, - "Utilization values in non-increasing order": { + "utilization values in non-increasing order": { args: config.RequestedToCapacityRatioArgs{ Shape: []config.UtilizationShapePoint{ { @@ -380,7 +535,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `utilization values must be sorted. Utilization[0]==30 >= Utilization[1]==20`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[1].utilization", + }, + }, }, "duplicated utilization values": { args: config.RequestedToCapacityRatioArgs{ @@ -399,7 +559,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `utilization values must be sorted. Utilization[1]==20 >= Utilization[2]==20`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[2].utilization", + }, + }, }, "score less than min": { args: config.RequestedToCapacityRatioArgs{ @@ -414,7 +579,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `score values must not be less than 0. Score[0]==-1`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[0].score", + }, + }, }, "score greater than max": { args: config.RequestedToCapacityRatioArgs{ @@ -429,7 +599,12 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `score values must not be greater than 10. Score[1]==11`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[1].score", + }, + }, }, "resources weight less than 1": { args: config.RequestedToCapacityRatioArgs{ @@ -446,22 +621,63 @@ func TestValidateRequestedToCapacityRatioArgs(t *testing.T) { }, }, }, - wantErr: `resource custom weight 0 must not be less than 1`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, + }, + "multiple errors": { + args: config.RequestedToCapacityRatioArgs{ + Shape: []config.UtilizationShapePoint{ + { + Utilization: 20, + Score: -1, + }, + { + Utilization: 10, + Score: 2, + }, + }, + Resources: []config.ResourceSpec{ + { + Name: "custom", + Weight: 0, + }, + }, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[1].utilization", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "shape[0].score", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateRequestedToCapacityRatioArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateRequestedToCapacityRatioArgs returned err (-want,+got):\n%s", diff) + } }) } } func TestValidateNodeResourcesLeastAllocatedArgs(t *testing.T) { cases := map[string]struct { - args *config.NodeResourcesLeastAllocatedArgs - wantErr string + args *config.NodeResourcesLeastAllocatedArgs + wantErrs field.ErrorList }{ "valid config": { args: &config.NodeResourcesLeastAllocatedArgs{ @@ -486,7 +702,12 @@ func TestValidateNodeResourcesLeastAllocatedArgs(t *testing.T) { }, }, }, - wantErr: `resource Weight of cpu should be a positive value, got 0`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, }, "weight more than max": { args: &config.NodeResourcesLeastAllocatedArgs{ @@ -497,22 +718,53 @@ func TestValidateNodeResourcesLeastAllocatedArgs(t *testing.T) { }, }, }, - wantErr: `resource Weight of memory should be less than 100, got 101`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, + }, + "multiple error": { + args: &config.NodeResourcesLeastAllocatedArgs{ + Resources: []config.ResourceSpec{ + { + Name: "memory", + Weight: 0, + }, + { + Name: "cpu", + Weight: 101, + }, + }, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[1].weight", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateNodeResourcesLeastAllocatedArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) + } }) } } func TestValidateNodeResourcesMostAllocatedArgs(t *testing.T) { cases := map[string]struct { - args *config.NodeResourcesMostAllocatedArgs - wantErr string + args *config.NodeResourcesMostAllocatedArgs + wantErrs field.ErrorList }{ "valid config": { args: &config.NodeResourcesMostAllocatedArgs{ @@ -537,7 +789,12 @@ func TestValidateNodeResourcesMostAllocatedArgs(t *testing.T) { }, }, }, - wantErr: `resource Weight of cpu should be a positive value, got -1`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, }, "weight more than max": { args: &config.NodeResourcesMostAllocatedArgs{ @@ -548,14 +805,45 @@ func TestValidateNodeResourcesMostAllocatedArgs(t *testing.T) { }, }, }, - wantErr: `resource Weight of memory should be less than 100, got 110`, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }, + }, + "multiple error": { + args: &config.NodeResourcesMostAllocatedArgs{ + Resources: []config.ResourceSpec{ + { + Name: "memory", + Weight: -1, + }, + { + Name: "cpu", + Weight: 110, + }, + }, + }, + wantErrs: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[1].weight", + }, + }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { err := ValidateNodeResourcesMostAllocatedArgs(tc.args) - assertErr(t, tc.wantErr, err) + if diff := cmp.Diff(tc.wantErrs.ToAggregate(), err, ignoreBadValueDetail); diff != "" { + t.Fatalf("ValidateNodeResourcesLeastAllocatedArgs returned err (-want,+got):\n%s", diff) + } }) } } @@ -651,18 +939,3 @@ func TestValidateNodeAffinityArgs(t *testing.T) { }) } } - -func assertErr(t *testing.T, wantErr string, gotErr error) { - if wantErr == "" { - if gotErr != nil { - t.Fatalf("\nwant err to be:\n\tnil\ngot:\n\t%s", gotErr.Error()) - } - } else { - if gotErr == nil { - t.Fatalf("\nwant err to be:\n\t%s\ngot:\n\tnil", wantErr) - } - if gotErr.Error() != wantErr { - t.Errorf("\nwant err to be:\n\t%s\ngot:\n\t%s", wantErr, gotErr.Error()) - } - } -} diff --git a/pkg/scheduler/framework/plugins/noderesources/BUILD b/pkg/scheduler/framework/plugins/noderesources/BUILD index 0396ab794a5..2644db2da3e 100644 --- a/pkg/scheduler/framework/plugins/noderesources/BUILD +++ b/pkg/scheduler/framework/plugins/noderesources/BUILD @@ -66,9 +66,12 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/component-base/featuregate:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/github.com/google/go-cmp/cmp/cmpopts:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/pkg/scheduler/framework/plugins/noderesources/least_allocated_test.go b/pkg/scheduler/framework/plugins/noderesources/least_allocated_test.go index 330a919a154..c4530d8f1ad 100644 --- a/pkg/scheduler/framework/plugins/noderesources/least_allocated_test.go +++ b/pkg/scheduler/framework/plugins/noderesources/least_allocated_test.go @@ -21,9 +21,12 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/scheduler/apis/config" "k8s.io/kubernetes/pkg/scheduler/framework" "k8s.io/kubernetes/pkg/scheduler/framework/runtime" @@ -101,7 +104,7 @@ func TestNodeResourcesLeastAllocated(t *testing.T) { pods []*v1.Pod nodes []*v1.Node args config.NodeResourcesLeastAllocatedArgs - wantErr string + wantErr error expectedList framework.NodeScoreList name string }{ @@ -260,27 +263,42 @@ func TestNodeResourcesLeastAllocated(t *testing.T) { }, { // resource with negtive weight is not allowed - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: -1}, {Name: "cpu", Weight: 1}}}, - wantErr: "resource Weight of memory should be a positive value, got -1", - name: "resource with negtive weight", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: -1}, {Name: "cpu", Weight: 1}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }.ToAggregate(), + name: "resource with negtive weight", }, { // resource with zero weight is not allowed - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 1}, {Name: "cpu", Weight: 0}}}, - wantErr: "resource Weight of cpu should be a positive value, got 0", - name: "resource with zero weight", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 1}, {Name: "cpu", Weight: 0}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[1].weight", + }, + }.ToAggregate(), + name: "resource with zero weight", }, { // resource weight should be less than MaxNodeScore - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 120}}}, - wantErr: "resource Weight of memory should be less than 100, got 120", - name: "resource weight larger than MaxNodeScore", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesLeastAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 120}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }.ToAggregate(), + name: "resource weight larger than MaxNodeScore", }, } @@ -290,16 +308,19 @@ func TestNodeResourcesLeastAllocated(t *testing.T) { fh, _ := runtime.NewFramework(nil, nil, nil, runtime.WithSnapshotSharedLister(snapshot)) p, err := NewLeastAllocated(&test.args, fh) - if len(test.wantErr) != 0 { - if err != nil && test.wantErr != err.Error() { - t.Fatalf("got err %v, want %v", err.Error(), test.wantErr) - } else if err == nil { + if test.wantErr != nil { + if err != nil { + diff := cmp.Diff(test.wantErr, err, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")) + if diff != "" { + t.Fatalf("got err (-want,+got):\n%s", diff) + } + } else { t.Fatalf("no error produced, wanted %v", test.wantErr) } return } - if err != nil && len(test.wantErr) == 0 { + if err != nil && test.wantErr == nil { t.Fatalf("failed to initialize plugin NodeResourcesLeastAllocated, got error: %v", err) } diff --git a/pkg/scheduler/framework/plugins/noderesources/most_allocated_test.go b/pkg/scheduler/framework/plugins/noderesources/most_allocated_test.go index 0f9a17ff3a1..fdbbe2d800c 100644 --- a/pkg/scheduler/framework/plugins/noderesources/most_allocated_test.go +++ b/pkg/scheduler/framework/plugins/noderesources/most_allocated_test.go @@ -21,9 +21,12 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/scheduler/apis/config" "k8s.io/kubernetes/pkg/scheduler/framework" "k8s.io/kubernetes/pkg/scheduler/framework/runtime" @@ -116,7 +119,7 @@ func TestNodeResourcesMostAllocated(t *testing.T) { pods []*v1.Pod nodes []*v1.Node args config.NodeResourcesMostAllocatedArgs - wantErr string + wantErr error expectedList framework.NodeScoreList name string }{ @@ -220,27 +223,42 @@ func TestNodeResourcesMostAllocated(t *testing.T) { }, { // resource with negtive weight is not allowed - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: -1}, {Name: "cpu", Weight: 1}}}, - wantErr: "resource Weight of memory should be a positive value, got -1", - name: "resource with negtive weight", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: -1}, {Name: "cpu", Weight: 1}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }.ToAggregate(), + name: "resource with negtive weight", }, { // resource with zero weight is not allowed - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 1}, {Name: "cpu", Weight: 0}}}, - wantErr: "resource Weight of cpu should be a positive value, got 0", - name: "resource with zero weight", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 1}, {Name: "cpu", Weight: 0}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[1].weight", + }, + }.ToAggregate(), + name: "resource with zero weight", }, { // resource weight should be less than MaxNodeScore - pod: &v1.Pod{Spec: cpuAndMemory}, - nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, - args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 120}}}, - wantErr: "resource Weight of memory should be less than 100, got 120", - name: "resource weight larger than MaxNodeScore", + pod: &v1.Pod{Spec: cpuAndMemory}, + nodes: []*v1.Node{makeNode("machine", 4000, 10000)}, + args: config.NodeResourcesMostAllocatedArgs{Resources: []config.ResourceSpec{{Name: "memory", Weight: 120}}}, + wantErr: field.ErrorList{ + &field.Error{ + Type: field.ErrorTypeInvalid, + Field: "resources[0].weight", + }, + }.ToAggregate(), + name: "resource weight larger than MaxNodeScore", }, } @@ -250,16 +268,19 @@ func TestNodeResourcesMostAllocated(t *testing.T) { fh, _ := runtime.NewFramework(nil, nil, nil, runtime.WithSnapshotSharedLister(snapshot)) p, err := NewMostAllocated(&test.args, fh) - if len(test.wantErr) != 0 { - if err != nil && test.wantErr != err.Error() { - t.Fatalf("got err %v, want %v", err.Error(), test.wantErr) - } else if err == nil { + if test.wantErr != nil { + if err != nil { + diff := cmp.Diff(test.wantErr, err, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")) + if diff != "" { + t.Fatalf("got err (-want,+got):\n%s", diff) + } + } else { t.Fatalf("no error produced, wanted %v", test.wantErr) } return } - if err != nil && len(test.wantErr) == 0 { + if err != nil && test.wantErr == nil { t.Fatalf("failed to initialize plugin NodeResourcesMostAllocated, got error: %v", err) }