diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index e442b722ea0..d731732a1ba 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -518,6 +518,16 @@ func validateJobStatus(job *batch.Job, fldPath *field.Path, opts JobStatusValida allErrs = append(allErrs, field.Invalid(fldPath.Child("completionTime"), status.CompletionTime, "completionTime cannot be set before startTime")) } } + if opts.RejectFailedJobWithoutFailureTarget { + if IsJobFailed(job) && !isJobFailureTarget(job) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("conditions"), field.OmitValueType{}, "cannot set Failed=True condition without the FailureTarget=true condition")) + } + } + if opts.RejectCompleteJobWithoutSuccessCriteriaMet { + if IsJobComplete(job) && !isJobSuccessCriteriaMet(job) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("conditions"), field.OmitValueType{}, "cannot set Complete=True condition without the SuccessCriteriaMet=true condition")) + } + } isJobFinished := IsJobFinished(job) if opts.RejectFinishedJobWithActivePods { if status.Active > 0 && isJobFinished { @@ -568,6 +578,16 @@ func validateJobStatus(job *batch.Job, fldPath *field.Path, opts JobStatusValida } } } + if opts.RejectFinishedJobWithTerminatingPods { + if status.Terminating != nil && *status.Terminating > 0 && isJobFinished { + allErrs = append(allErrs, field.Invalid(fldPath.Child("terminating"), status.Terminating, "terminating>0 is invalid for finished job")) + } + } + if opts.RejectMoreReadyThanActivePods { + if status.Ready != nil && *status.Ready > status.Active { + allErrs = append(allErrs, field.Invalid(fldPath.Child("ready"), *status.Ready, "cannot set more ready pods than active")) + } + } if !opts.AllowForSuccessCriteriaMetInExtendedScope && ptr.Deref(job.Spec.CompletionMode, batch.NonIndexedCompletion) != batch.IndexedCompletion && isJobSuccessCriteriaMet(job) { allErrs = append(allErrs, field.Invalid(fldPath.Child("conditions"), field.OmitValueType{}, "cannot set SuccessCriteriaMet to NonIndexed Job")) } @@ -1005,6 +1025,8 @@ type JobStatusValidationOptions struct { RejectFailedIndexesOverlappingCompleted bool RejectCompletedIndexesForNonIndexedJob bool RejectFailedIndexesForNoBackoffLimitPerIndex bool + RejectFailedJobWithoutFailureTarget bool + RejectCompleteJobWithoutSuccessCriteriaMet bool RejectFinishedJobWithActivePods bool RejectFinishedJobWithoutStartTime bool RejectFinishedJobWithUncountedTerminatedPods bool @@ -1016,4 +1038,6 @@ type JobStatusValidationOptions struct { RejectCompleteJobWithFailedCondition bool RejectCompleteJobWithFailureTargetCondition bool AllowForSuccessCriteriaMetInExtendedScope bool + RejectMoreReadyThanActivePods bool + RejectFinishedJobWithTerminatingPods bool } diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index bf0745474d9..333c98b45b6 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -376,12 +376,15 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt isJobCompleteChanged := batchvalidation.IsJobComplete(oldJob) != batchvalidation.IsJobComplete(newJob) isJobFailedChanged := batchvalidation.IsJobFailed(oldJob) != batchvalidation.IsJobFailed(newJob) isJobFailureTargetChanged := batchvalidation.IsConditionTrue(oldJob.Status.Conditions, batch.JobFailureTarget) != batchvalidation.IsConditionTrue(newJob.Status.Conditions, batch.JobFailureTarget) + isJobSuccessCriteriaMetChanged := batchvalidation.IsConditionTrue(oldJob.Status.Conditions, batch.JobSuccessCriteriaMet) != batchvalidation.IsConditionTrue(newJob.Status.Conditions, batch.JobSuccessCriteriaMet) isCompletedIndexesChanged := oldJob.Status.CompletedIndexes != newJob.Status.CompletedIndexes isFailedIndexesChanged := !ptr.Equal(oldJob.Status.FailedIndexes, newJob.Status.FailedIndexes) isActiveChanged := oldJob.Status.Active != newJob.Status.Active isStartTimeChanged := !ptr.Equal(oldJob.Status.StartTime, newJob.Status.StartTime) isCompletionTimeChanged := !ptr.Equal(oldJob.Status.CompletionTime, newJob.Status.CompletionTime) 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) return batchvalidation.JobStatusValidationOptions{ // We allow to decrease the counter for succeeded pods for jobs which @@ -394,6 +397,8 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt RejectCompletedIndexesForNonIndexedJob: isCompletedIndexesChanged, RejectFailedIndexesForNoBackoffLimitPerIndex: isFailedIndexesChanged, RejectFailedIndexesOverlappingCompleted: isFailedIndexesChanged || isCompletedIndexesChanged, + RejectFailedJobWithoutFailureTarget: isJobFailedChanged || isFailedIndexesChanged, + RejectCompleteJobWithoutSuccessCriteriaMet: isJobCompleteChanged || isJobSuccessCriteriaMetChanged, RejectFinishedJobWithActivePods: isJobFinishedChanged || isActiveChanged, RejectFinishedJobWithoutStartTime: isJobFinishedChanged || isStartTimeChanged, RejectFinishedJobWithUncountedTerminatedPods: isJobFinishedChanged || isUncountedTerminatedPodsChanged, @@ -405,6 +410,8 @@ func getStatusValidationOptions(newJob, oldJob *batch.Job) batchvalidation.JobSt RejectCompleteJobWithFailedCondition: isJobCompleteChanged || isJobFailedChanged, RejectCompleteJobWithFailureTargetCondition: isJobCompleteChanged || isJobFailureTargetChanged, AllowForSuccessCriteriaMetInExtendedScope: true, + RejectMoreReadyThanActivePods: isReadyChanged || isActiveChanged, + RejectFinishedJobWithTerminatingPods: isJobFinishedChanged || isTerminatingChanged, } } if utilfeature.DefaultFeatureGate.Enabled(features.JobPodReplacementPolicy) { diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index 3cd75ea8e00..9d87531ddc2 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -2155,6 +2155,51 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { }, }, }, + wantErrs: field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, + {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, + {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, + }, + }, + "invalid addition of Complete=True without SuccessCriteriaMet=True": { + enableJobManagedBy: true, + job: &batch.Job{ + ObjectMeta: validObjectMeta, + }, + newJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + CompletionTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobComplete, + Status: api.ConditionTrue, + }, + }, + }, + }, + wantErrs: field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, + }, + }, + "invalid addition of Failed=True without FailureTarget=True": { + enableJobManagedBy: true, + job: &batch.Job{ + ObjectMeta: validObjectMeta, + }, + newJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailed, + Status: api.ConditionTrue, + }, + }, + }, + }, wantErrs: field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, }, @@ -2179,11 +2224,23 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, Status: batch.JobStatus{ Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, { Type: batch.JobFailed, Status: api.ConditionTrue, @@ -2199,12 +2256,24 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, Status: batch.JobStatus{ CompletionTime: &now, Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, { Type: batch.JobComplete, Status: api.ConditionTrue, @@ -2220,6 +2289,16 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Active: 1, + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, @@ -2228,6 +2307,10 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { CompletionTime: &now, Active: 1, Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, { Type: batch.JobComplete, Status: api.ConditionTrue, @@ -2239,30 +2322,94 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { {Type: field.ErrorTypeInvalid, Field: "status.active"}, }, }, - "transition to Failed condition with terminating>0 and ready>0": { + "invalid attempt to transition to Failed=True with terminating > 0": { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, + }, + Terminating: ptr.To[int32](1), + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, Status: batch.JobStatus{ StartTime: &now, Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, { Type: batch.JobFailed, Status: api.ConditionTrue, }, }, Terminating: ptr.To[int32](1), - Ready: ptr.To[int32](1), }, }, + wantErrs: field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "status.terminating"}, + }, + }, + "invalid attempt to transition to Failed=True with active > 0": { + enableJobManagedBy: true, + job: &batch.Job{ + ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, + }, + Active: 1, + }, + }, + newJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, + { + Type: batch.JobFailed, + Status: api.ConditionTrue, + }, + }, + Active: 1, + }, + }, + wantErrs: field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "status.active"}, + }, }, "invalid attempt to transition to Failed=True with uncountedTerminatedPods.Failed>0": { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + UncountedTerminatedPods: &batch.UncountedTerminatedPods{ + Failed: []types.UID{"a"}, + }, + Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, @@ -2272,6 +2419,10 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { Failed: []types.UID{"a"}, }, Conditions: []batch.JobCondition{ + { + Type: batch.JobFailureTarget, + Status: api.ConditionTrue, + }, { Type: batch.JobFailed, Status: api.ConditionTrue, @@ -2364,6 +2515,18 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + UncountedTerminatedPods: &batch.UncountedTerminatedPods{ + Succeeded: []types.UID{"a"}, + }, + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, @@ -2374,6 +2537,10 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { Succeeded: []types.UID{"a"}, }, Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, { Type: batch.JobComplete, Status: api.ConditionTrue, @@ -2389,12 +2556,25 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { enableJobManagedBy: true, job: &batch.Job{ ObjectMeta: validObjectMeta, + Status: batch.JobStatus{ + StartTime: &now, + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + }, + }, }, newJob: &batch.Job{ ObjectMeta: validObjectMeta, Status: batch.JobStatus{ StartTime: &now, Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, { Type: batch.JobComplete, Status: api.ConditionTrue, @@ -2500,6 +2680,12 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { ObjectMeta: validObjectMeta, Status: batch.JobStatus{ StartTime: &nowPlusMinute, + Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, + }, }, }, newJob: &batch.Job{ @@ -2508,6 +2694,10 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { StartTime: &nowPlusMinute, CompletionTime: &now, Conditions: []batch.JobCondition{ + { + Type: batch.JobSuccessCriteriaMet, + Status: api.ConditionTrue, + }, { Type: batch.JobComplete, Status: api.ConditionTrue, @@ -2942,6 +3132,7 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { }, wantErrs: field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, + {Type: field.ErrorTypeInvalid, Field: "status.conditions"}, }, }, "invalid failedIndexes, which overlap with completedIndexes": { @@ -3440,6 +3631,28 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { }, }, }, + "invalid attempt to set more ready pods than active": { + enableJobManagedBy: true, + job: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Completions: ptr.To[int32](5), + }, + }, + newJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Completions: ptr.To[int32](5), + }, + Status: batch.JobStatus{ + Active: 1, + Ready: ptr.To[int32](2), + }, + }, + wantErrs: field.ErrorList{ + {Type: field.ErrorTypeInvalid, Field: "status.ready"}, + }, + }, } 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 70fa382dd3e..e924030d713 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -2262,16 +2262,14 @@ func TestManagedBy_UsingReservedJobFinalizers(t *testing.T) { t.Fatalf("Error %v when marking the %q pod as succeeded", err, klog.KObj(podObj)) } - // Mark the job as finished so that the built-in controller receives the + // Trigger termination for the Job so that the built-in controller receives the // UpdateJob event in reaction to each it would remove the pod's finalizer, // if not for the custom managedBy field. jobObj.Status.Conditions = append(jobObj.Status.Conditions, batchv1.JobCondition{ - Type: batchv1.JobComplete, + Type: batchv1.JobSuccessCriteriaMet, Status: v1.ConditionTrue, }) jobObj.Status.StartTime = ptr.To(metav1.Now()) - jobObj.Status.CompletionTime = ptr.To(metav1.Now()) - if jobObj, err = clientSet.BatchV1().Jobs(jobObj.Namespace).UpdateStatus(ctx, jobObj, metav1.UpdateOptions{}); err != nil { t.Fatalf("Error %v when updating the job as finished %v", err, klog.KObj(jobObj)) }