From c63f4484510832f3fe7b53e18ba48e25fdd3f8d9 Mon Sep 17 00:00:00 2001 From: Daniel Vega-Myhre Date: Fri, 17 Feb 2023 23:02:05 +0000 Subject: [PATCH] change test names and address other comments --- pkg/apis/batch/validation/validation.go | 8 ++-- test/integration/job/job_test.go | 61 ++++++++++--------------- 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index 0bac3f6fbb1..22a531031d3 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -568,13 +568,15 @@ func ValidateJobTemplateSpec(spec *batch.JobTemplateSpec, fldPath *field.Path, o } func validateCompletions(spec, oldSpec batch.JobSpec, fldPath *field.Path) field.ErrorList { - var allErrs field.ErrorList - + if !feature.DefaultFeatureGate.Enabled(features.ElasticIndexedJob) { + return apivalidation.ValidateImmutableField(spec.Completions, oldSpec.Completions, fldPath) + } // Completions is immutable for non-indexed jobs. // For Indexed Jobs, if ElasticIndexedJob feature gate is not enabled, // fall back to validating that spec.Completions is always immutable. + var allErrs field.ErrorList isIndexedJob := spec.CompletionMode != nil && *spec.CompletionMode == batch.IndexedCompletion - if !isIndexedJob || !feature.DefaultFeatureGate.Enabled(features.ElasticIndexedJob) { + if !isIndexedJob { return apivalidation.ValidateImmutableField(spec.Completions, oldSpec.Completions, fldPath) } diff --git a/test/integration/job/job_test.go b/test/integration/job/job_test.go index d12bf4203a4..0e118c83efc 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -1030,12 +1030,9 @@ func TestIndexedJob(t *testing.T) { } func TestElasticIndexedJob(t *testing.T) { - const ( - noCompletionsUpdate = -1 - initialCompletions = 3 - ) + const initialCompletions int32 = 3 type jobUpdate struct { - completions int + completions *int32 succeedIndexes []int failIndexes []int wantSucceededIndexes string @@ -1051,7 +1048,7 @@ func TestElasticIndexedJob(t *testing.T) { "feature flag off, mutation not allowed": { jobUpdates: []jobUpdate{ { - completions: 4, + completions: pointer.Int32Ptr(4), }, }, wantErr: apierrors.NewInvalid( @@ -1060,23 +1057,22 @@ func TestElasticIndexedJob(t *testing.T) { field.ErrorList{field.Invalid(field.NewPath("spec", "completions"), 4, "field is immutable")}, ), }, - "scale up, verify that the range expands, and the job finishes successfully when indexes including the ones in the new range exit successfully": { + "scale up": { featureGate: true, jobUpdates: []jobUpdate{ { // Scale up completions 3->4 then succeed indexes 0-3 - completions: 4, + completions: pointer.Int32Ptr(4), succeedIndexes: []int{0, 1, 2, 3}, wantSucceededIndexes: "0-3", }, }, }, - "scale down, verify that the range shrinks, and the job finishes successfully when indexes only in the smaller range finishes successfully, and verify that failures that happened for indexes that are now outside the range still count": { + "scale down": { featureGate: true, jobUpdates: []jobUpdate{ // First succeed index 1 and fail index 2 while completions is still original value (3). { - completions: noCompletionsUpdate, succeedIndexes: []int{1}, failIndexes: []int{2}, wantSucceededIndexes: "1", @@ -1087,19 +1083,18 @@ func TestElasticIndexedJob(t *testing.T) { // Scale down completions 3->1, verify prev failure out of range still counts // but succeeded out of range does not. { - completions: 1, + completions: pointer.Int32Ptr(1), succeedIndexes: []int{0}, wantSucceededIndexes: "0", wantFailed: 1, }, }, }, - "index finishes successfully, scale down to exclude the index, then scale up to include it back. verify that the index was restarted and job finishes successfully after all indexes complete": { + "index finishes successfully, scale down, scale up": { featureGate: true, jobUpdates: []jobUpdate{ // First succeed index 2 while completions is still original value (3). { - completions: noCompletionsUpdate, succeedIndexes: []int{2}, wantSucceededIndexes: "2", wantRemainingIndexes: sets.NewInt(0, 1), @@ -1107,13 +1102,13 @@ func TestElasticIndexedJob(t *testing.T) { }, // Scale completions down 3->2 to exclude previously succeeded index. { - completions: 2, + completions: pointer.Int32Ptr(2), wantRemainingIndexes: sets.NewInt(0, 1), wantActivePods: 2, }, // Scale completions back up to include previously succeeded index that was temporarily out of range. { - completions: 3, + completions: pointer.Int32Ptr(3), succeedIndexes: []int{0, 1, 2}, wantSucceededIndexes: "0-2", }, @@ -1122,7 +1117,9 @@ func TestElasticIndexedJob(t *testing.T) { "scale down to 0, verify that the job succeeds": { featureGate: true, jobUpdates: []jobUpdate{ - {}, + { + completions: pointer.Int32Ptr(0), + }, }, }, } @@ -1141,8 +1138,8 @@ func TestElasticIndexedJob(t *testing.T) { mode := batchv1.IndexedCompletion jobObj, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{ Spec: batchv1.JobSpec{ - Parallelism: pointer.Int32Ptr(int32(initialCompletions)), - Completions: pointer.Int32Ptr(int32(initialCompletions)), + Parallelism: pointer.Int32Ptr(initialCompletions), + Completions: pointer.Int32Ptr(initialCompletions), CompletionMode: &mode, }, }) @@ -1168,16 +1165,12 @@ func TestElasticIndexedJob(t *testing.T) { for _, update := range tc.jobUpdates { // Update Job spec if necessary. - if update.completions != noCompletionsUpdate { + if update.completions != nil { if jobObj, err = updateJob(ctx, jobClient, jobObj.Name, func(j *batchv1.Job) { - j.Spec.Completions = pointer.Int32Ptr(int32(update.completions)) - j.Spec.Parallelism = pointer.Int32Ptr(int32(update.completions)) + j.Spec.Completions = update.completions + j.Spec.Parallelism = update.completions }); err != nil { - if tc.wantErr == nil { - t.Fatalf("Failed to update Job: %v", err) - } - statusErr := err.(*apierrors.StatusError) - if diff := cmp.Diff(tc.wantErr, statusErr); diff != "" { + if diff := cmp.Diff(tc.wantErr, err); diff != "" { t.Fatalf("Unexpected or missing errors (-want/+got): %s", diff) } return @@ -1185,20 +1178,16 @@ func TestElasticIndexedJob(t *testing.T) { } // Succeed specified indexes. - if update.succeedIndexes != nil { - for _, idx := range update.succeedIndexes { - if err := setJobPhaseForIndex(ctx, clientSet, jobObj, v1.PodSucceeded, idx); err != nil { - t.Fatalf("Failed trying to succeed pod with index %d: %v", idx, err) - } + for _, idx := range update.succeedIndexes { + if err := setJobPhaseForIndex(ctx, clientSet, jobObj, v1.PodSucceeded, idx); err != nil { + t.Fatalf("Failed trying to succeed pod with index %d: %v", idx, err) } } // Fail specified indexes. - if update.failIndexes != nil { - for _, idx := range update.failIndexes { - if err := setJobPhaseForIndex(ctx, clientSet, jobObj, v1.PodFailed, idx); err != nil { - t.Fatalf("Failed trying to fail pod with index %d: %v", idx, err) - } + for _, idx := range update.failIndexes { + if err := setJobPhaseForIndex(ctx, clientSet, jobObj, v1.PodFailed, idx); err != nil { + t.Fatalf("Failed trying to fail pod with index %d: %v", idx, err) } }