diff --git a/pkg/apis/batch/validation/validation.go b/pkg/apis/batch/validation/validation.go index a11886703d4..9209f699483 100644 --- a/pkg/apis/batch/validation/validation.go +++ b/pkg/apis/batch/validation/validation.go @@ -741,7 +741,7 @@ func ValidateJobStatusUpdate(job, oldJob *batch.Job, opts JobStatusValidationOpt // Note that we check `oldJob.Status.StartTime != nil` to allow transitioning from // startTime = nil to startTime != nil for unsuspended jobs, which is a desired transition. if oldJob.Status.StartTime != nil && !ptr.Equal(oldJob.Status.StartTime, job.Status.StartTime) && !ptr.Deref(job.Spec.Suspend, false) { - allErrs = append(allErrs, field.Required(statusFld.Child("startTime"), "startTime cannot be removed for unsuspended job")) + allErrs = append(allErrs, field.Invalid(statusFld.Child("startTime"), job.Status.StartTime, "field is immutable for unsuspended job once set")) } } if isJobSuccessCriteriaMet(oldJob) && !isJobSuccessCriteriaMet(job) { diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 3bf0791852d..63b36dd86e3 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/pkg/apis/batch/validation/validation_test.go @@ -21,6 +21,7 @@ import ( "fmt" "strings" "testing" + "time" _ "time/tzdata" "github.com/google/go-cmp/cmp" @@ -2583,12 +2584,16 @@ func TestValidateJobUpdate(t *testing.T) { } func TestValidateJobUpdateStatus(t *testing.T) { + now := time.Now() + cases := map[string]struct { opts JobStatusValidationOptions old batch.Job update batch.Job wantErrs field.ErrorList + + cmpopts cmp.Options }{ "valid": { old: batch.Job{ @@ -2680,6 +2685,7 @@ func TestValidateJobUpdateStatus(t *testing.T) { {Type: field.ErrorTypeInvalid, Field: "status.ready"}, {Type: field.ErrorTypeInvalid, Field: "status.terminating"}, }, + cmpopts: cmp.Options{ignoreErrValueDetail}, }, "empty and duplicated uncounted pods": { old: batch.Job{ @@ -2709,12 +2715,105 @@ func TestValidateJobUpdateStatus(t *testing.T) { {Type: field.ErrorTypeDuplicate, Field: "status.uncountedTerminatedPods.failed[3]"}, {Type: field.ErrorTypeInvalid, Field: "status.uncountedTerminatedPods.failed[4]"}, }, + cmpopts: cmp.Options{ignoreErrValueDetail}, + }, + "immutable startTime for unsuspended job: with non-nil startTime": { + opts: JobStatusValidationOptions{ + RejectStartTimeUpdateForUnsuspendedJob: true, + }, + old: batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "1", + }, + Spec: batch.JobSpec{ + Suspend: ptr.To(false), + }, + Status: batch.JobStatus{ + StartTime: &metav1.Time{ + Time: now, + }, + Active: 1, + }, + }, + update: batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "1", + }, + Spec: batch.JobSpec{ + Suspend: ptr.To(false), + }, + Status: batch.JobStatus{ + StartTime: &metav1.Time{ + Time: now.Add(time.Second), // Attempt to change startTime + }, + Active: 1, + }, + }, + wantErrs: field.ErrorList{ + { + Type: field.ErrorTypeInvalid, + Field: "status.startTime", + BadValue: &metav1.Time{ + Time: now.Add(time.Second), + }, + Detail: "field is immutable for unsuspended job once set", + }, + }, + cmpopts: cmp.Options{cmpopts.IgnoreFields(field.Error{}, "Origin")}, + }, + "immutable startTime for unsuspended job: with nil startTime": { + opts: JobStatusValidationOptions{ + RejectStartTimeUpdateForUnsuspendedJob: true, + }, + old: batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "1", + }, + Spec: batch.JobSpec{ + Suspend: ptr.To(false), + }, + Status: batch.JobStatus{ + StartTime: &metav1.Time{ + Time: now, + }, + Active: 1, + }, + }, + update: batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc", + Namespace: metav1.NamespaceDefault, + ResourceVersion: "1", + }, + Spec: batch.JobSpec{ + Suspend: ptr.To(false), + }, + Status: batch.JobStatus{ + StartTime: nil, + Active: 1, + }, + }, + wantErrs: field.ErrorList{ + { + Type: field.ErrorTypeInvalid, + Field: "status.startTime", + BadValue: (*metav1.Time)(nil), + Detail: "field is immutable for unsuspended job once set", + }, + }, + cmpopts: cmp.Options{cmpopts.IgnoreFields(field.Error{}, "Origin")}, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { errs := ValidateJobUpdateStatus(&tc.update, &tc.old, tc.opts) - if diff := cmp.Diff(tc.wantErrs, errs, ignoreErrValueDetail); diff != "" { + if diff := cmp.Diff(tc.wantErrs, errs, tc.cmpopts); diff != "" { t.Errorf("Unexpected errors (-want,+got):\n%s", diff) } }) diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index deafa30ef5c..1b7aa158558 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -2844,7 +2844,7 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { }, }, wantErrs: field.ErrorList{ - {Type: field.ErrorTypeRequired, Field: "status.startTime"}, + {Type: field.ErrorTypeInvalid, Field: "status.startTime"}, }, }, "verify startTime cannot be updated for unsuspended job": { @@ -2862,7 +2862,7 @@ func TestStatusStrategy_ValidateUpdate(t *testing.T) { }, }, wantErrs: field.ErrorList{ - {Type: field.ErrorTypeRequired, Field: "status.startTime"}, + {Type: field.ErrorTypeInvalid, Field: "status.startTime"}, }, }, "verify startTime can be updated when resuming job (JobSuspended: True -> False)": {