From 4d59b1fc218fa88748f7672e96e54057857d949e Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sun, 21 Aug 2022 17:54:59 +0900 Subject: [PATCH 1/2] add validation webhooks for maxFailedTrialCount and parallelTrialCount --- .../template-setup-e2e-test/action.yaml | 4 +- .../v1beta1/experiment/validator/validator.go | 19 ++++++-- .../experiment/validator/validator_test.go | 48 +++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) diff --git a/.github/workflows/template-setup-e2e-test/action.yaml b/.github/workflows/template-setup-e2e-test/action.yaml index 10e66b2b430..630efa3fc13 100644 --- 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 index 47a766a3260..5b3553ee01a 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -74,15 +74,26 @@ func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1be return fmt.Errorf(msg) } - if instance.Spec.MaxFailedTrialCount != nil && *instance.Spec.MaxFailedTrialCount < 0 { - return fmt.Errorf("spec.maxFailedTrialCount should not be less than 0") + if instance.Spec.MaxFailedTrialCount != nil { + if *instance.Spec.MaxFailedTrialCount < 0 { + return fmt.Errorf("spec.maxFailedTrialCount should not be less than 0") + } + if instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxFailedTrialCount > *instance.Spec.MaxTrialCount { + return fmt.Errorf("spec.maxFailedTrialCount should be less than or equal to spec.maxTrialCount") + } } if instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxTrialCount <= 0 { return fmt.Errorf("spec.maxTrialCount must be greater than 0") } - if instance.Spec.ParallelTrialCount != nil && *instance.Spec.ParallelTrialCount <= 0 { - return fmt.Errorf("spec.parallelTrialCount must be greater than 0") + if instance.Spec.ParallelTrialCount != nil { + if *instance.Spec.ParallelTrialCount <= 0 { + return fmt.Errorf("spec.parallelTrialCount must be greater than 0") + } + if instance.Spec.MaxTrialCount != nil && *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 index b2b3f786ddd..916ef599a96 100644 --- 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 { From 6398e6f602bf3f5e488bf5218c5041e8ddea9bff Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sun, 21 Aug 2022 20:30:59 +0900 Subject: [PATCH 2/2] [review] simplify validation logic --- .../v1beta1/experiment/validator/validator.go | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go index 5b3553ee01a..aeaf911904a 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -74,22 +74,23 @@ func (g *DefaultValidator) ValidateExperiment(instance, oldInst *experimentsv1be return fmt.Errorf(msg) } - if instance.Spec.MaxFailedTrialCount != nil { - if *instance.Spec.MaxFailedTrialCount < 0 { - return fmt.Errorf("spec.maxFailedTrialCount should not be less than 0") - } - if instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxFailedTrialCount > *instance.Spec.MaxTrialCount { - return fmt.Errorf("spec.maxFailedTrialCount should be less than or equal to spec.maxTrialCount") - } + if instance.Spec.MaxFailedTrialCount != nil && *instance.Spec.MaxFailedTrialCount < 0 { + return fmt.Errorf("spec.maxFailedTrialCount should not be less than 0") } if instance.Spec.MaxTrialCount != nil && *instance.Spec.MaxTrialCount <= 0 { return fmt.Errorf("spec.maxTrialCount must be greater than 0") } - if instance.Spec.ParallelTrialCount != nil { - if *instance.Spec.ParallelTrialCount <= 0 { - return fmt.Errorf("spec.parallelTrialCount must be greater than 0") + 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.MaxTrialCount != nil && *instance.Spec.ParallelTrialCount > *instance.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") } }