Graduate SuspendJob to GA

This commit is contained in:
Abdullah Gharaibeh 2022-02-15 10:46:13 -05:00
parent d12787bc2c
commit b2d2ec9e76
10 changed files with 16 additions and 86 deletions

View File

@ -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
}

View File

@ -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)
}
}

View File

@ -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

View File

@ -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

View File

@ -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)

View File

@ -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},

View File

@ -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)
}

View File

@ -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.

View File

@ -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"`
}

View File

@ -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) {