add more tests

kubernetes-sigs · Oct 23, 2023 · 7799023 · 7799023
1 parent 0943f32
commit 7799023
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 13 deletions.
diff --git a/pkg/webhooks/workload_webhook.go b/pkg/webhooks/workload_webhook.go
@@ -190,7 +190,7 @@ func validateAdmissionChecks(obj *kueue.Workload, basePath *field.Path) field.Er
 	for i := range obj.Status.AdmissionChecks {
 		admissionChecksPath := basePath.Index(i)
 		ac := &obj.Status.AdmissionChecks[i]
-		if len(ac.PodSetUpdates) > 0 && (len(ac.PodSetUpdates) != len(obj.Spec.PodSets)) {
+		if len(ac.PodSetUpdates) > 0 && len(ac.PodSetUpdates) != len(obj.Spec.PodSets) {
 			allErrs = append(allErrs, field.Invalid(admissionChecksPath.Child("podSetUpdates"), field.OmitValueType{}, "must have the same number of podSetUpdates as the podSets"))
 		}
 		allErrs = append(allErrs, validatePodSetUpdates(ac, obj, admissionChecksPath.Child("podSetUpdates"))...)
@@ -208,28 +208,28 @@ func validatePodSetUpdates(acs *kueue.AdmissionCheckState, obj *kueue.Workload,
 	for i := range acs.PodSetUpdates {
 		psu := &acs.PodSetUpdates[i]
 		psuPath := basePath.Index(i)
-		if found := knowPodSets.Has(psu.Name); !found {
+		if !knowPodSets.Has(psu.Name) {
 			allErrs = append(allErrs, field.NotSupported(psuPath.Child("name"), psu.Name, knowPodSets.UnsortedList()))
 		}
 		allErrs = append(allErrs, validateTolerations(psu.Tolerations, psuPath.Child("tolerations"))...)
 		allErrs = append(allErrs, apivalidation.ValidateAnnotations(psu.Annotations, psuPath.Child("annotations"))...)
+		allErrs = append(allErrs, metav1validation.ValidateLabels(psu.NodeSelector, psuPath.Child("nodeSelector"))...)
 		allErrs = append(allErrs, metav1validation.ValidateLabels(psu.Labels, psuPath.Child("labels"))...)
 	}
 	return allErrs
 }
 
 func validateImmutablePodSetUpdates(newObj, oldObj *kueue.Workload, basePath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
-	for i := range newObj.Status.AdmissionChecks {
-		newAc := &newObj.Status.AdmissionChecks[i]
-		for i := range oldObj.Status.AdmissionChecks {
-			oldAc := &oldObj.Status.AdmissionChecks[i]
-			if newAc.Name != oldAc.Name {
-				continue
-			}
-			if oldAc.State == kueue.CheckStateReady && newAc.State == kueue.CheckStateReady {
-				allErrs = append(allErrs, apivalidation.ValidateImmutableField(newAc.PodSetUpdates, oldAc.PodSetUpdates, basePath.Index(i).Child("podSetUpdates"))...)
-			}
+	newAcs := slices.ToRefMap(newObj.Status.AdmissionChecks, func(f *kueue.AdmissionCheckState) string { return f.Name })
+	for i := range oldObj.Status.AdmissionChecks {
+		oldAc := &oldObj.Status.AdmissionChecks[i]
+		newAc, found := newAcs[oldAc.Name]
+		if !found {
+			continue
+		}
+		if oldAc.State == kueue.CheckStateReady && newAc.State == kueue.CheckStateReady {
+			allErrs = append(allErrs, apivalidation.ValidateImmutableField(newAc.PodSetUpdates, oldAc.PodSetUpdates, basePath.Index(i).Child("podSetUpdates"))...)
 		}
 	}
 	return allErrs

diff --git a/pkg/webhooks/workload_webhook_test.go b/pkg/webhooks/workload_webhook_test.go
@@ -234,6 +234,10 @@ func TestValidateWorkload(t *testing.T) {
 				field.Invalid(firstPodSetSpecPath.Child("containers").Index(0).Child("resources", "requests").Key(string(corev1.ResourcePods)), nil, ""),
 			},
 		},
+		"empty podSetUpdates": {
+			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).AdmissionChecks(kueue.AdmissionCheckState{}).Obj(),
+			wantErr:  nil,
+		},
 		"should podSetUpdates have the same number of podSets": {
 			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets(
 				*testingutil.MakePodSet("first", 1).Obj(),
@@ -245,7 +249,7 @@ func TestValidateWorkload(t *testing.T) {
 				field.Invalid(podSetUpdatePath, nil, "must have the same number of podSetUpdates as the podSets"),
 			},
 		},
-		"should the set of names in podSetUpdates to be matching with podSets": {
+		"mismatched names in podSetUpdates with names in podSets": {
 			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets(
 				*testingutil.MakePodSet("first", 1).Obj(),
 				*testingutil.MakePodSet("second", 1).Obj(),
@@ -256,6 +260,14 @@ func TestValidateWorkload(t *testing.T) {
 				field.NotSupported(firstAdmissionChecksPath.Child("podSetUpdates").Index(1).Child("name"), nil, nil),
 			},
 		},
+		"matched names in podSetUpdates with names in podSets": {
+			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets(
+				*testingutil.MakePodSet("first", 1).Obj(),
+				*testingutil.MakePodSet("second", 1).Obj(),
+			).AdmissionChecks(
+				kueue.AdmissionCheckState{PodSetUpdates: []kueue.PodSetUpdate{{Name: "first"}, {Name: "second"}}},
+			).Obj(),
+		},
 		"invalid label name of podSetUpdate": {
 			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				AdmissionChecks(
@@ -266,6 +278,16 @@ func TestValidateWorkload(t *testing.T) {
 				field.Invalid(podSetUpdatePath.Index(0).Child("labels"), "@abc", ""),
 			},
 		},
+		"invalid node selector name of podSetUpdate": {
+			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
+				AdmissionChecks(
+					kueue.AdmissionCheckState{PodSetUpdates: []kueue.PodSetUpdate{{Name: "main", NodeSelector: map[string]string{"@abc": "foo"}}}},
+				).
+				Obj(),
+			wantErr: field.ErrorList{
+				field.Invalid(podSetUpdatePath.Index(0).Child("nodeSelector"), "@abc", ""),
+			},
+		},
 		"invalid label value of podSetUpdate": {
 			workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).
 				AdmissionChecks(