Add validation webhooks for maxFailedTrialCount and parallelTrialCount (

#1936) * add validation webhooks for maxFailedTrialCount and parallelTrialCount * [review] simplify validation logic
kubeflow · Aug 22, 2022 · 9c7d797 · 9c7d797
1 parent fe4d6e7
commit 9c7d797
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 2 deletions.
diff --git a/.github/workflows/template-setup-e2e-test/action.yaml b/.github/workflows/template-setup-e2e-test/action.yaml
@@ -20,6 +20,6 @@ runs:
       uses: docker/setup-buildx-action@v2
 
     - name: Set Up Go env
-      uses: actions/setup-go@v2
+      uses: actions/setup-go@v3
       with:
-        go-version: 1.17.10
+        go-version-file: go.mod
diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go
@@ -83,6 +83,18 @@ func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1be
 	if instance.Spec.ParallelTrialCount != nil && *instance.Spec.ParallelTrialCount <= 0 {
 		return fmt.Errorf("spec.parallelTrialCount must be greater than 0")
 	}
+
+	if instance.Spec.MaxFailedTrialCount != nil && instance.Spec.MaxTrialCount != nil {
+		if *instance.Spec.MaxFailedTrialCount > *instance.Spec.MaxTrialCount {
+			return fmt.Errorf("spec.maxFailedTrialCount should be less than or equal to spec.maxTrialCount")
+		}
+	}
+	if instance.Spec.ParallelTrialCount != nil && instance.Spec.MaxTrialCount != nil {
+		if *instance.Spec.ParallelTrialCount > *instance.Spec.MaxTrialCount {
+			return fmt.Errorf("spec.paralelTrialCount should be less than or equal to spec.maxTrialCount")
+		}
+	}
+
 	if oldInst != nil {
 		// We should validate restart only if appropriate fields are changed.
 		// Otherwise check below is triggered when experiment is deleted.

diff --git a/pkg/webhook/v1beta1/experiment/validator/validator_test.go b/pkg/webhook/v1beta1/experiment/validator/validator_test.go
@@ -284,6 +284,54 @@ func TestValidateExperiment(t *testing.T) {
 			Err:             true,
 			testDescription: "Invalid feasible space in parameters",
 		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				maxTrialCount := int32(5)
+				invalidMaxFailedTrialCount := int32(6)
+				i := newFakeInstance()
+				i.Spec.MaxTrialCount = &maxTrialCount
+				i.Spec.MaxFailedTrialCount = &invalidMaxFailedTrialCount
+				return i
+			}(),
+			Err:             true,
+			testDescription: "maxFailedTrialCount greater than maxTrialCount",
+		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				maxTrialCount := int32(5)
+				validMaxFailedTrialCount := int32(5)
+				i := newFakeInstance()
+				i.Spec.MaxTrialCount = &maxTrialCount
+				i.Spec.MaxFailedTrialCount = &validMaxFailedTrialCount
+				return i
+			}(),
+			Err:             false,
+			testDescription: "maxFailedTrialCount equal to maxTrialCount",
+		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				maxTrialCount := int32(5)
+				invalidParallelTrialCount := int32(6)
+				i := newFakeInstance()
+				i.Spec.MaxTrialCount = &maxTrialCount
+				i.Spec.ParallelTrialCount = &invalidParallelTrialCount
+				return i
+			}(),
+			Err:             true,
+			testDescription: "parallelTrialCount greater than maxTrialCount",
+		},
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				maxTrialCount := int32(5)
+				validParallelTrialCount := int32(5)
+				i := newFakeInstance()
+				i.Spec.MaxTrialCount = &maxTrialCount
+				i.Spec.ParallelTrialCount = &validParallelTrialCount
+				return i
+			}(),
+			Err:             false,
+			testDescription: "parallelTrialCount equal to maxTrialCount",
+		},
 	}
 
 	for _, tc := range tcs {