From b2d2ec9e76d7bd480d722eff92c4c51e375f93e5 Mon Sep 17 00:00:00 2001 From: Abdullah Gharaibeh Date: Tue, 15 Feb 2022 10:46:13 -0500 Subject: [PATCH] Graduate SuspendJob to GA --- pkg/apis/batch/types.go | 3 -- pkg/apis/batch/v1/defaults.go | 2 +- pkg/apis/batch/v1/defaults_test.go | 11 ++++--- pkg/controller/job/job_controller.go | 4 +-- pkg/controller/job/job_controller_test.go | 12 -------- pkg/features/kube_features.go | 3 +- pkg/registry/batch/job/strategy.go | 13 -------- pkg/registry/batch/job/strategy_test.go | 15 ---------- staging/src/k8s.io/api/batch/v1/types.go | 3 -- test/integration/job/job_test.go | 36 +++-------------------- 10 files changed, 16 insertions(+), 86 deletions(-) diff --git a/pkg/apis/batch/types.go b/pkg/apis/batch/types.go index 529748c2ee3..322cd165a25 100644 --- a/pkg/apis/batch/types.go +++ b/pkg/apis/batch/types.go @@ -208,9 +208,6 @@ type JobSpec struct { // Suspending a Job will reset the StartTime field of the Job, effectively // resetting the ActiveDeadlineSeconds timer too. Defaults to false. // - // This field is beta-level, gated by SuspendJob feature flag (enabled by - // default). - // // +optional Suspend *bool } diff --git a/pkg/apis/batch/v1/defaults.go b/pkg/apis/batch/v1/defaults.go index 14a4c1aeeff..61e83ad77ed 100644 --- a/pkg/apis/batch/v1/defaults.go +++ b/pkg/apis/batch/v1/defaults.go @@ -49,7 +49,7 @@ func SetDefaults_Job(obj *batchv1.Job) { mode := batchv1.NonIndexedCompletion obj.Spec.CompletionMode = &mode } - if utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) && obj.Spec.Suspend == nil { + if obj.Spec.Suspend == nil { obj.Spec.Suspend = utilpointer.BoolPtr(false) } } diff --git a/pkg/apis/batch/v1/defaults_test.go b/pkg/apis/batch/v1/defaults_test.go index 6fccc0be1a8..5862ea90564 100644 --- a/pkg/apis/batch/v1/defaults_test.go +++ b/pkg/apis/batch/v1/defaults_test.go @@ -40,7 +40,6 @@ func TestSetDefaultJob(t *testing.T) { defaultLabels := map[string]string{"default": "default"} tests := map[string]struct { indexedJobEnabled bool - suspendJobEnabled bool original *batchv1.Job expected *batchv1.Job expectLabels bool @@ -58,6 +57,7 @@ func TestSetDefaultJob(t *testing.T) { Completions: pointer.Int32Ptr(1), Parallelism: pointer.Int32Ptr(1), BackoffLimit: pointer.Int32Ptr(6), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, @@ -77,12 +77,12 @@ func TestSetDefaultJob(t *testing.T) { Parallelism: pointer.Int32Ptr(1), BackoffLimit: pointer.Int32Ptr(6), CompletionMode: completionModePtr(batchv1.NonIndexedCompletion), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, }, "All unspecified, suspend job enabled -> sets all to default values": { - suspendJobEnabled: true, original: &batchv1.Job{ Spec: batchv1.JobSpec{ Template: v1.PodTemplateSpec{ @@ -101,7 +101,6 @@ func TestSetDefaultJob(t *testing.T) { expectLabels: true, }, "suspend set, everything else is defaulted": { - suspendJobEnabled: true, original: &batchv1.Job{ Spec: batchv1.JobSpec{ Suspend: pointer.BoolPtr(true), @@ -136,6 +135,7 @@ func TestSetDefaultJob(t *testing.T) { Completions: pointer.Int32Ptr(1), Parallelism: pointer.Int32Ptr(1), BackoffLimit: pointer.Int32Ptr(6), + Suspend: pointer.BoolPtr(false), }, }, }, @@ -152,6 +152,7 @@ func TestSetDefaultJob(t *testing.T) { Spec: batchv1.JobSpec{ Parallelism: pointer.Int32Ptr(0), BackoffLimit: pointer.Int32Ptr(6), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, @@ -169,6 +170,7 @@ func TestSetDefaultJob(t *testing.T) { Spec: batchv1.JobSpec{ Parallelism: pointer.Int32Ptr(2), BackoffLimit: pointer.Int32Ptr(6), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, @@ -187,6 +189,7 @@ func TestSetDefaultJob(t *testing.T) { Completions: pointer.Int32Ptr(2), Parallelism: pointer.Int32Ptr(1), BackoffLimit: pointer.Int32Ptr(6), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, @@ -205,6 +208,7 @@ func TestSetDefaultJob(t *testing.T) { Completions: pointer.Int32Ptr(1), Parallelism: pointer.Int32Ptr(1), BackoffLimit: pointer.Int32Ptr(5), + Suspend: pointer.BoolPtr(false), }, }, expectLabels: true, @@ -265,7 +269,6 @@ func TestSetDefaultJob(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.IndexedJob, test.indexedJobEnabled)() - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, test.suspendJobEnabled)() original := test.original expected := test.expected diff --git a/pkg/controller/job/job_controller.go b/pkg/controller/job/job_controller.go index 424350b6ae1..e0f14144352 100644 --- a/pkg/controller/job/job_controller.go +++ b/pkg/controller/job/job_controller.go @@ -767,7 +767,7 @@ func (jm *Controller) syncJob(ctx context.Context, key string) (forget bool, rEr } if complete { finishedCondition = newCondition(batch.JobComplete, v1.ConditionTrue, "", "") - } else if feature.DefaultFeatureGate.Enabled(features.SuspendJob) && manageJobCalled { + } else if manageJobCalled { // Update the conditions / emit events only if manageJob was called in // this syncJob. Otherwise wait for the right syncJob call to make // updates. @@ -1257,7 +1257,7 @@ func getStatus(job *batch.Job, pods []*v1.Pod, uncounted *uncountedTerminatedPod // jobSuspended returns whether a Job is suspended while taking the feature // gate into account. func jobSuspended(job *batch.Job) bool { - return feature.DefaultFeatureGate.Enabled(features.SuspendJob) && job.Spec.Suspend != nil && *job.Spec.Suspend + return job.Spec.Suspend != nil && *job.Spec.Suspend } // manageJob is the core method responsible for managing the number of running diff --git a/pkg/controller/job/job_controller_test.go b/pkg/controller/job/job_controller_test.go index 45ab34887c2..772864b064c 100644 --- a/pkg/controller/job/job_controller_test.go +++ b/pkg/controller/job/job_controller_test.go @@ -225,7 +225,6 @@ func TestControllerSyncJob(t *testing.T) { // features indexedJobEnabled bool - suspendJobEnabled bool jobReadyPodsEnabled bool }{ "job start": { @@ -659,7 +658,6 @@ func TestControllerSyncJob(t *testing.T) { "suspending a job with satisfied expectations": { // Suspended Job should delete active pods when expectations are // satisfied. - suspendJobEnabled: true, suspend: true, parallelism: 2, activePods: 2, // parallelism == active, expectations satisfied @@ -679,7 +677,6 @@ func TestControllerSyncJob(t *testing.T) { // Job in the syncJob call because the controller will wait for // expectations to be satisfied first. The next syncJob call (not tested // here) will be the same as the previous test. - suspendJobEnabled: true, suspend: true, parallelism: 2, activePods: 3, // active > parallelism, expectations unsatisfied @@ -692,7 +689,6 @@ func TestControllerSyncJob(t *testing.T) { expectedActive: 3, }, "resuming a suspended job": { - suspendJobEnabled: true, wasSuspended: true, suspend: false, parallelism: 2, @@ -711,7 +707,6 @@ func TestControllerSyncJob(t *testing.T) { // cases above), but since this job is being deleted, we don't expect // anything changed here from before the job was suspended. The // JobSuspended condition is also missing. - suspendJobEnabled: true, suspend: true, deleting: true, parallelism: 2, @@ -733,7 +728,6 @@ func TestControllerSyncJob(t *testing.T) { t.Skip("Can't track status if finalizers can't be removed") } defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.IndexedJob, tc.indexedJobEnabled)() - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, tc.suspendJobEnabled)() defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobReadyPods, tc.jobReadyPodsEnabled)() defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobTrackingWithFinalizers, wFinalizers)() @@ -1696,9 +1690,6 @@ func TestSyncJobPastDeadline(t *testing.T) { expectedFailed int32 expectedCondition batch.JobConditionType expectedConditionReason string - - // features - suspendJobEnabled bool }{ "activeDeadlineSeconds less than single pod execution": { parallelism: 1, @@ -1750,7 +1741,6 @@ func TestSyncJobPastDeadline(t *testing.T) { expectedConditionReason: "BackoffLimitExceeded", }, "activeDeadlineSeconds is not triggered when Job is suspended": { - suspendJobEnabled: true, suspend: true, parallelism: 1, completions: 2, @@ -1765,8 +1755,6 @@ func TestSyncJobPastDeadline(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, tc.suspendJobEnabled)() - // job manager setup clientSet := clientset.NewForConfigOrDie(&restclient.Config{Host: "", ContentConfig: restclient.ContentConfig{GroupVersion: &schema.GroupVersion{Group: "", Version: "v1"}}}) manager, sharedInformerFactory := newControllerFromClient(clientSet, controller.NoResyncPeriodFunc) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 0afe6fcebb0..ccef05c1ab1 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -653,6 +653,7 @@ const ( // owner: @adtac // alpha: v1.21 // beta: v1.22 + // GA: v1.24 // // Allows jobs to be created in the suspended state. SuspendJob featuregate.Feature = "SuspendJob" @@ -910,7 +911,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS IngressClassNamespacedParams: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.24 ServiceInternalTrafficPolicy: {Default: true, PreRelease: featuregate.Beta}, LogarithmicScaleDown: {Default: true, PreRelease: featuregate.Beta}, - SuspendJob: {Default: true, PreRelease: featuregate.Beta}, + SuspendJob: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.25 KubeletPodResourcesGetAllocatable: {Default: true, PreRelease: featuregate.Beta}, CSIVolumeHealth: {Default: false, PreRelease: featuregate.Alpha}, WindowsHostProcessContainers: {Default: true, PreRelease: featuregate.Beta}, diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index c7a5ed8805c..427c3aba5f3 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -97,10 +97,6 @@ func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { job.Spec.CompletionMode = nil } - if !utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) { - job.Spec.Suspend = nil - } - if utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) { // Until this feature graduates to GA and soaks in clusters, we use an // annotation to mark whether jobs are tracked with it. @@ -141,15 +137,6 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object newJob.Spec.CompletionMode = nil } - if !utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) { - // There are 3 possible values (nil, true, false) for each flag, so 9 - // combinations. We want to disallow everything except true->false and - // true->nil when the feature gate is disabled. Or, basically allow this - // only when oldJob is true. - if oldJob.Spec.Suspend == nil || !*oldJob.Spec.Suspend { - newJob.Spec.Suspend = oldJob.Spec.Suspend - } - } if !utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) && !hasJobTrackingAnnotation(oldJob) { dropJobTrackingAnnotation(newJob) } diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index ded2126db7c..879fb646646 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -43,16 +43,12 @@ var ignoreErrValueDetail = cmpopts.IgnoreFields(field.Error{}, "BadValue", "Deta func TestJobStrategy(t *testing.T) { cases := map[string]struct { indexedJobEnabled bool - suspendJobEnabled bool trackingWithFinalizersEnabled bool }{ "features disabled": {}, "indexed job enabled": { indexedJobEnabled: true, }, - "suspend job enabled": { - suspendJobEnabled: true, - }, "new job tracking enabled": { trackingWithFinalizersEnabled: true, }, @@ -60,7 +56,6 @@ func TestJobStrategy(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IndexedJob, tc.indexedJobEnabled)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SuspendJob, tc.suspendJobEnabled)() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.JobTrackingWithFinalizers, tc.trackingWithFinalizersEnabled)() testJobStrategy(t) }) @@ -69,7 +64,6 @@ func TestJobStrategy(t *testing.T) { func testJobStrategy(t *testing.T) { indexedJobEnabled := utilfeature.DefaultFeatureGate.Enabled(features.IndexedJob) - suspendJobEnabled := utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) trackingWithFinalizersEnabled := utilfeature.DefaultFeatureGate.Enabled(features.JobTrackingWithFinalizers) ctx := genericapirequest.NewDefaultContext() if !Strategy.NamespaceScoped() { @@ -130,9 +124,6 @@ func testJobStrategy(t *testing.T) { if indexedJobEnabled != (job.Spec.CompletionMode != nil) { t.Errorf("Job should allow setting .spec.completionMode only when %v feature is enabled", features.IndexedJob) } - if !suspendJobEnabled && (job.Spec.Suspend != nil) { - t.Errorf("Job should allow setting .spec.suspend only when %v feature is enabled", features.SuspendJob) - } wantAnnotations := map[string]string{"foo": "bar"} if trackingWithFinalizersEnabled { wantAnnotations[batchv1.JobTrackingFinalizer] = "" @@ -210,14 +201,8 @@ func testJobStrategy(t *testing.T) { // disabled. We don't care about other combinations. job.Spec.Suspend, updatedJob.Spec.Suspend = pointer.BoolPtr(false), pointer.BoolPtr(true) Strategy.PrepareForUpdate(ctx, updatedJob, job) - if !suspendJobEnabled && *updatedJob.Spec.Suspend { - t.Errorf("[SuspendJob=%v] .spec.suspend should not be updated from false to true", suspendJobEnabled) - } job.Spec.Suspend, updatedJob.Spec.Suspend = nil, pointer.BoolPtr(true) Strategy.PrepareForUpdate(ctx, updatedJob, job) - if !suspendJobEnabled && updatedJob.Spec.Suspend != nil { - t.Errorf("[SuspendJob=%v] .spec.suspend should not be updated from nil to non-nil", suspendJobEnabled) - } // Make sure we correctly implement the interface. // Otherwise a typo could silently change the default. diff --git a/staging/src/k8s.io/api/batch/v1/types.go b/staging/src/k8s.io/api/batch/v1/types.go index aec1cad8e20..743f3ce57c6 100644 --- a/staging/src/k8s.io/api/batch/v1/types.go +++ b/staging/src/k8s.io/api/batch/v1/types.go @@ -190,9 +190,6 @@ type JobSpec struct { // Suspending a Job will reset the StartTime field of the Job, effectively // resetting the ActiveDeadlineSeconds timer too. Defaults to false. // - // This field is beta-level, gated by SuspendJob feature flag (enabled by - // default). - // // +optional Suspend *bool `json:"suspend,omitempty" protobuf:"varint,10,opt,name=suspend"` } diff --git a/test/integration/job/job_test.go b/test/integration/job/job_test.go index 659802f421f..9d334855eb7 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -607,32 +607,18 @@ func TestSuspendJob(t *testing.T) { // Exhaustively test all combinations other than trivial true->true and // false->false cases. { - featureGate: true, - create: step{flag: false, wantActive: 2}, - update: step{flag: true, wantActive: 0, wantStatus: v1.ConditionTrue, wantReason: "Suspended"}, + create: step{flag: false, wantActive: 2}, + update: step{flag: true, wantActive: 0, wantStatus: v1.ConditionTrue, wantReason: "Suspended"}, }, { - featureGate: true, - create: step{flag: true, wantActive: 0, wantStatus: v1.ConditionTrue, wantReason: "Suspended"}, - update: step{flag: false, wantActive: 2, wantStatus: v1.ConditionFalse, wantReason: "Resumed"}, - }, - { - featureGate: false, - create: step{flag: false, wantActive: 2}, - update: step{flag: true, wantActive: 2}, - }, - { - featureGate: false, - create: step{flag: true, wantActive: 2}, - update: step{flag: false, wantActive: 2}, + create: step{flag: true, wantActive: 0, wantStatus: v1.ConditionTrue, wantReason: "Suspended"}, + update: step{flag: false, wantActive: 2, wantStatus: v1.ConditionFalse, wantReason: "Resumed"}, }, } for _, tc := range testCases { name := fmt.Sprintf("feature=%v,create=%v,update=%v", tc.featureGate, tc.create.flag, tc.update.flag) t.Run(name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, tc.featureGate)() - closeFn, restConfig, clientSet, ns := setup(t, "suspend") defer closeFn() ctx, cancel := startJobController(restConfig, clientSet) @@ -683,8 +669,6 @@ func TestSuspendJob(t *testing.T) { } func TestSuspendJobControllerRestart(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, true)() - closeFn, restConfig, clientSet, ns := setup(t, "suspend") defer closeFn() ctx, cancel := startJobController(restConfig, clientSet) @@ -705,18 +689,6 @@ func TestSuspendJobControllerRestart(t *testing.T) { validateJobPodsStatus(ctx, t, clientSet, job, podsByStatus{ Active: 0, }, true) - - // Disable feature gate and restart controller to test that pods get created. - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, false)() - cancel() - ctx, cancel = startJobController(restConfig, clientSet) - job, err = clientSet.BatchV1().Jobs(ns.Name).Get(ctx, job.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Failed to get Job: %v", err) - } - validateJobPodsStatus(ctx, t, clientSet, job, podsByStatus{ - Active: 2, - }, true) } func TestNodeSelectorUpdate(t *testing.T) {