From 4c58a9d19d05fd48f2ec3e3e6533bbc0ab58c1fe Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Mon, 30 Jun 2025 07:57:28 +0200 Subject: [PATCH 1/2] Fix validation for Job with suspend=true,completions=0 to set Complete condition --- pkg/registry/batch/job/strategy.go | 3 ++- pkg/registry/batch/job/strategy_test.go | 30 +++++++++++++++++++++++++ test/integration/job/job_test.go | 23 +++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 1de03e459da..2db435d94fc 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -379,6 +379,7 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt isUncountedTerminatedPodsChanged := !apiequality.Semantic.DeepEqual(oldJob.Status.UncountedTerminatedPods, newJob.Status.UncountedTerminatedPods) isReadyChanged := !ptr.Equal(oldJob.Status.Ready, newJob.Status.Ready) isTerminatingChanged := !ptr.Equal(oldJob.Status.Terminating, newJob.Status.Terminating) + isSuspendedWithZeroCompletions := newJob.Spec.Completions != nil && *newJob.Spec.Completions == 0 && newJob.Spec.Suspend != nil && *newJob.Spec.Suspend return batchvalidation.JobStatusValidationOptions{ // We allow to decrease the counter for succeeded pods for jobs which @@ -394,7 +395,7 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt RejectFailedJobWithoutFailureTarget: isJobFailedChanged || isFailedIndexesChanged, RejectCompleteJobWithoutSuccessCriteriaMet: isJobCompleteChanged || isJobSuccessCriteriaMetChanged, RejectFinishedJobWithActivePods: isJobFinishedChanged || isActiveChanged, - RejectFinishedJobWithoutStartTime: isJobFinishedChanged || isStartTimeChanged, + RejectFinishedJobWithoutStartTime: (isJobFinishedChanged || isStartTimeChanged) && !isSuspendedWithZeroCompletions, RejectFinishedJobWithUncountedTerminatedPods: isJobFinishedChanged || isUncountedTerminatedPodsChanged, RejectStartTimeUpdateForUnsuspendedJob: isStartTimeChanged, RejectCompletionTimeBeforeStartTime: isStartTimeChanged || isCompletionTimeChanged, diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index 18489fa4aeb..227f2ebf256 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -3561,6 +3561,36 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { {Type: field.ErrorTypeInvalid, Field: "status.ready"}, }, }, + "valid transition to Complete for suspended Job with completions=0; without startTime": { + enableJobManagedBy: true, + job: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Completions: ptr.To[int32](0), + Suspend: ptr.To(true), + }, + }, + newJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Completions: ptr.To[int32](0), + Suspend: ptr.To(true), + }, + Status: batch.JobStatus{ + CompletionTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + { + Type: batch.JobComplete, + Status: api.ConditionTrue, + }, + }, + }, + }, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { diff --git a/test/integration/job/job_test.go b/test/integration/job/job_test.go index 1c2062b83d4..d34c50ae7b1 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -2790,6 +2790,29 @@ func TestParallelJobWithCompletions(t *testing.T) { }) } +// TestSuspendedJobWithZeroCompletions verifies the suspended Job with +// completions=0 is marked as Complete. +func TestSuspendedJobWithZeroCompletions(t *testing.T) { + closeFn, restConfig, clientSet, ns := setup(t, "suspended-with-zero-completions") + t.Cleanup(closeFn) + ctx, cancel := startJobControllerAndWaitForCaches(t, restConfig) + t.Cleanup(func() { + cancel() + }) + jobObj, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{ + Spec: batchv1.JobSpec{ + Completions: ptr.To[int32](0), + Suspend: ptr.To(true), + }, + }) + if err != nil { + t.Fatalf("Failed to create Job: %v", err) + } + for _, condition := range []batchv1.JobConditionType{batchv1.JobSuccessCriteriaMet, batchv1.JobComplete} { + validateJobCondition(ctx, t, clientSet, jobObj, condition) + } +} + func TestIndexedJob(t *testing.T) { t.Cleanup(setDurationDuringTest(&jobcontroller.DefaultJobPodFailureBackOff, fastPodFailureBackoff)) closeFn, restConfig, clientSet, ns := setup(t, "indexed") From 339ead430761b4032f7c8368f00e548f4c430ac4 Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Mon, 30 Jun 2025 13:18:53 +0200 Subject: [PATCH 2/2] Review remarks --- pkg/registry/batch/job/strategy.go | 2 +- test/integration/job/job_test.go | 46 +++++++++++++++--------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 2db435d94fc..71cd60c0598 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -379,7 +379,7 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt isUncountedTerminatedPodsChanged := !apiequality.Semantic.DeepEqual(oldJob.Status.UncountedTerminatedPods, newJob.Status.UncountedTerminatedPods) isReadyChanged := !ptr.Equal(oldJob.Status.Ready, newJob.Status.Ready) isTerminatingChanged := !ptr.Equal(oldJob.Status.Terminating, newJob.Status.Terminating) - isSuspendedWithZeroCompletions := newJob.Spec.Completions != nil && *newJob.Spec.Completions == 0 && newJob.Spec.Suspend != nil && *newJob.Spec.Suspend + isSuspendedWithZeroCompletions := ptr.Equal(newJob.Spec.Suspend, ptr.To(true)) && ptr.Equal(newJob.Spec.Completions, ptr.To[int32](0)) return batchvalidation.JobStatusValidationOptions{ // We allow to decrease the counter for succeeded pods for jobs which diff --git a/test/integration/job/job_test.go b/test/integration/job/job_test.go index d34c50ae7b1..74c85baa9dd 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -2790,29 +2790,6 @@ func TestParallelJobWithCompletions(t *testing.T) { }) } -// TestSuspendedJobWithZeroCompletions verifies the suspended Job with -// completions=0 is marked as Complete. -func TestSuspendedJobWithZeroCompletions(t *testing.T) { - closeFn, restConfig, clientSet, ns := setup(t, "suspended-with-zero-completions") - t.Cleanup(closeFn) - ctx, cancel := startJobControllerAndWaitForCaches(t, restConfig) - t.Cleanup(func() { - cancel() - }) - jobObj, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{ - Spec: batchv1.JobSpec{ - Completions: ptr.To[int32](0), - Suspend: ptr.To(true), - }, - }) - if err != nil { - t.Fatalf("Failed to create Job: %v", err) - } - for _, condition := range []batchv1.JobConditionType{batchv1.JobSuccessCriteriaMet, batchv1.JobComplete} { - validateJobCondition(ctx, t, clientSet, jobObj, condition) - } -} - func TestIndexedJob(t *testing.T) { t.Cleanup(setDurationDuringTest(&jobcontroller.DefaultJobPodFailureBackOff, fastPodFailureBackoff)) closeFn, restConfig, clientSet, ns := setup(t, "indexed") @@ -3988,6 +3965,29 @@ func TestSuspendJob(t *testing.T) { } } +// TestSuspendJobWithZeroCompletions verifies the suspended Job with +// completions=0 is marked as Complete. +func TestSuspendJobWithZeroCompletions(t *testing.T) { + closeFn, restConfig, clientSet, ns := setup(t, "suspended-with-zero-completions") + t.Cleanup(closeFn) + ctx, cancel := startJobControllerAndWaitForCaches(t, restConfig) + t.Cleanup(func() { + cancel() + }) + jobObj, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{ + Spec: batchv1.JobSpec{ + Completions: ptr.To[int32](0), + Suspend: ptr.To(true), + }, + }) + if err != nil { + t.Fatalf("Failed to create Job: %v", err) + } + for _, condition := range []batchv1.JobConditionType{batchv1.JobSuccessCriteriaMet, batchv1.JobComplete} { + validateJobCondition(ctx, t, clientSet, jobObj, condition) + } +} + func TestSuspendJobControllerRestart(t *testing.T) { closeFn, restConfig, clientSet, ns := setup(t, "suspend") t.Cleanup(closeFn)