From 6e0e6c70d3b232ff7d01bb4a552d55a716675fb1 Mon Sep 17 00:00:00 2001 From: Irvin Lim Date: Tue, 12 Apr 2022 23:44:51 +0800 Subject: [PATCH] feat: Change maxRetryAttempts to maxAttempts --- apis/execution/v1alpha1/job_types.go | 14 +- .../v1alpha1/zz_generated.deepcopy.go | 4 +- .../bases/execution.furiko.io_jobconfigs.yaml | 6 +- .../crd/bases/execution.furiko.io_jobs.yaml | 6 +- config/samples/execution_v1alpha1_job.yaml | 4 +- .../utils.go => core/validation/generic.go} | 45 +++- pkg/core/validation/generic_test.go | 241 ++++++++++++++++++ .../controllers/jobcontroller/reconciler.go | 1 - pkg/execution/mutation/mutation.go | 4 + pkg/execution/mutation/mutation_test.go | 24 +- pkg/execution/validation/corev1.go | 6 +- pkg/execution/validation/validation.go | 24 +- pkg/execution/validation/validation_test.go | 20 +- pkg/execution/variablecontext/provider.go | 4 +- pkg/utils/execution/job/condition_test.go | 18 +- pkg/utils/execution/job/job_utils_test.go | 12 +- pkg/utils/execution/job/task_test.go | 2 +- pkg/utils/execution/job/task_utils.go | 8 +- pkg/utils/execution/job/task_utils_test.go | 16 +- 19 files changed, 377 insertions(+), 82 deletions(-) rename pkg/{execution/validation/utils.go => core/validation/generic.go} (50%) create mode 100644 pkg/core/validation/generic_test.go diff --git a/apis/execution/v1alpha1/job_types.go b/apis/execution/v1alpha1/job_types.go index 8b1a86f..3c62940 100644 --- a/apis/execution/v1alpha1/job_types.go +++ b/apis/execution/v1alpha1/job_types.go @@ -138,21 +138,19 @@ type JobTemplateSpec struct { // Describes the tasks to be created for the Job. Task JobTaskSpec `json:"task"` - // Specifies maximum number of retry attempts for the Job if the job exceeds its - // pending timeout or active deadline. Each retry attempt will create a single - // task at a time. The controller will create up to MaxRetryAttempts+1 tasks for - // the job, before terminating in RetryLimitExceeded or PendingTimeout. Defaults - // to 0, which means no retry. Value must be a non-negative integer. + // Specifies maximum number of attempts for the Job. Each attempt will create a + // single task at a time, and if the task fails, the controller will wait + // retryDelaySeconds before creating the next task attempt. Once maxAttempts is + // reached, the Job terminates in RetryLimitExceeded. Value must be a positive + // integer. Defaults to 1. // - // +kubebuilder:validation:Minimum=0 // +optional - MaxRetryAttempts *int32 `json:"maxRetryAttempts,omitempty"` + MaxAttempts *int32 `json:"maxAttempts,omitempty"` // Optional duration in seconds to wait between retries. If left empty or zero, // it means no delay (i.e. retry immediately). Value must be a non-negative // integer. // - // +kubebuilder:validation:Minimum=0 // +optional RetryDelaySeconds *int64 `json:"retryDelaySeconds,omitempty"` } diff --git a/apis/execution/v1alpha1/zz_generated.deepcopy.go b/apis/execution/v1alpha1/zz_generated.deepcopy.go index 7d777fc..de69105 100644 --- a/apis/execution/v1alpha1/zz_generated.deepcopy.go +++ b/apis/execution/v1alpha1/zz_generated.deepcopy.go @@ -494,8 +494,8 @@ func (in *JobTemplate) DeepCopy() *JobTemplate { func (in *JobTemplateSpec) DeepCopyInto(out *JobTemplateSpec) { *out = *in in.Task.DeepCopyInto(&out.Task) - if in.MaxRetryAttempts != nil { - in, out := &in.MaxRetryAttempts, &out.MaxRetryAttempts + if in.MaxAttempts != nil { + in, out := &in.MaxAttempts, &out.MaxAttempts *out = new(int32) **out = **in } diff --git a/config/crd/bases/execution.furiko.io_jobconfigs.yaml b/config/crd/bases/execution.furiko.io_jobconfigs.yaml index 071c3b8..7568a36 100644 --- a/config/crd/bases/execution.furiko.io_jobconfigs.yaml +++ b/config/crd/bases/execution.furiko.io_jobconfigs.yaml @@ -199,15 +199,13 @@ spec: spec: description: Specification of the desired behavior of the job. properties: - maxRetryAttempts: - description: Specifies maximum number of retry attempts for the Job if the job exceeds its pending timeout or active deadline. Each retry attempt will create a single task at a time. The controller will create up to MaxRetryAttempts+1 tasks for the job, before terminating in RetryLimitExceeded or PendingTimeout. Defaults to 0, which means no retry. Value must be a non-negative integer. + maxAttempts: + description: Specifies maximum number of attempts for the Job. Each attempt will create a single task at a time, and if the task fails, the controller will wait retryDelaySeconds before creating the next task attempt. Once maxAttempts is reached, the Job terminates in RetryLimitExceeded. format: int32 - minimum: 0 type: integer retryDelaySeconds: description: Optional duration in seconds to wait between retries. If left empty or zero, it means no delay (i.e. retry immediately). Value must be a non-negative integer. format: int64 - minimum: 0 type: integer task: description: Describes the tasks to be created for the Job. diff --git a/config/crd/bases/execution.furiko.io_jobs.yaml b/config/crd/bases/execution.furiko.io_jobs.yaml index 5d6f688..0510933 100644 --- a/config/crd/bases/execution.furiko.io_jobs.yaml +++ b/config/crd/bases/execution.furiko.io_jobs.yaml @@ -81,15 +81,13 @@ spec: template: description: Template specifies how to create the Job. properties: - maxRetryAttempts: - description: Specifies maximum number of retry attempts for the Job if the job exceeds its pending timeout or active deadline. Each retry attempt will create a single task at a time. The controller will create up to MaxRetryAttempts+1 tasks for the job, before terminating in RetryLimitExceeded or PendingTimeout. Defaults to 0, which means no retry. Value must be a non-negative integer. + maxAttempts: + description: Specifies maximum number of attempts for the Job. Each attempt will create a single task at a time, and if the task fails, the controller will wait retryDelaySeconds before creating the next task attempt. Once maxAttempts is reached, the Job terminates in RetryLimitExceeded. format: int32 - minimum: 0 type: integer retryDelaySeconds: description: Optional duration in seconds to wait between retries. If left empty or zero, it means no delay (i.e. retry immediately). Value must be a non-negative integer. format: int64 - minimum: 0 type: integer task: description: Describes the tasks to be created for the Job. diff --git a/config/samples/execution_v1alpha1_job.yaml b/config/samples/execution_v1alpha1_job.yaml index cd00545..7baaeee 100644 --- a/config/samples/execution_v1alpha1_job.yaml +++ b/config/samples/execution_v1alpha1_job.yaml @@ -40,8 +40,8 @@ spec: # Maximum duration that a task can be Pending for. pendingTimeoutSeconds: 1800 - # Defines how many additional tasks to retry in case of failure. - maxRetryAttempts: 2 + # Defines how many tasks can be created in case of failure. + maxAttempts: 3 # Defines how long to wait between retries. retryDelaySeconds: 15 diff --git a/pkg/execution/validation/utils.go b/pkg/core/validation/generic.go similarity index 50% rename from pkg/execution/validation/utils.go rename to pkg/core/validation/generic.go index d8b4c05..5ac8bda 100644 --- a/pkg/execution/validation/utils.go +++ b/pkg/core/validation/generic.go @@ -23,11 +23,14 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// ToInternalErrorList converts an error into an ErrorList with a single InternalError. +// ToInternalErrorList converts an error into an ErrorList with a single +// InternalError if it is not nil, otherwise returns an empty list. func ToInternalErrorList(fldPath *field.Path, err error) field.ErrorList { - return field.ErrorList{ - field.InternalError(fldPath, err), + allErrs := field.ErrorList{} + if err != nil { + allErrs = append(allErrs, field.InternalError(fldPath, err)) } + return allErrs } // ValidateImmutableField validates the new value and the old value are deeply @@ -48,3 +51,39 @@ func ValidateMaxLength(val string, maxLen int, fldPath *field.Path) field.ErrorL } return allErrs } + +// ValidateGTE validates the given value must be greater than or equal to the minimum value. +func ValidateGTE(value, minimum int64, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value < minimum { + allErrs = append(allErrs, field.Invalid(fldPath, value, fmt.Sprintf("must be greater than or equal to %v", minimum))) + } + return allErrs +} + +// ValidateGT validates the given value must be greater than the minimum value. +func ValidateGT(value, minimum int64, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value <= minimum { + allErrs = append(allErrs, field.Invalid(fldPath, value, fmt.Sprintf("must be greater than %v", minimum))) + } + return allErrs +} + +// ValidateLTE validates the given value must be less than or equal to the maximum value. +func ValidateLTE(value, maximum int64, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value > maximum { + allErrs = append(allErrs, field.Invalid(fldPath, value, fmt.Sprintf("must be less than or equal to %v", maximum))) + } + return allErrs +} + +// ValidateLT validates the given value must be less than the maximum value. +func ValidateLT(value, maximum int64, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value >= maximum { + allErrs = append(allErrs, field.Invalid(fldPath, value, fmt.Sprintf("must be less than %v", maximum))) + } + return allErrs +} diff --git a/pkg/core/validation/generic_test.go b/pkg/core/validation/generic_test.go new file mode 100644 index 0000000..28d25ef --- /dev/null +++ b/pkg/core/validation/generic_test.go @@ -0,0 +1,241 @@ +/* + * Copyright 2022 The Furiko Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package validation + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +var ( + fldPath = field.NewPath("spec") +) + +func TestToInternalErrorList(t *testing.T) { + tests := []struct { + name string + err error + want field.ErrorList + }{ + { + name: "nil error", + }, + { + name: "non-nil error", + err: assert.AnError, + want: field.ErrorList{ + field.InternalError(fldPath, assert.AnError), + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got := ToInternalErrorList(fldPath, tt.err) + if !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ToInternalErrorList() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestValidateImmutableField(t *testing.T) { + tests := []struct { + name string + newVal interface{} + oldVal interface{} + msg string + want field.ErrorList + }{ + { + name: "nil values", + }, + { + name: "different integer values", + newVal: 1, + oldVal: 0, + msg: "custom error message", + want: field.ErrorList{ + field.Invalid(fldPath, 1, "custom error message"), + }, + }, + { + name: "equal integer values", + newVal: 1, + oldVal: 1, + msg: "custom error message", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateImmutableField(tt.newVal, tt.oldVal, fldPath, tt.msg); !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ValidateImmutableField(%v, %v, %v, %v) = %v, want %v", + tt.newVal, tt.oldVal, fldPath, tt.msg, got, tt.want) + } + }) + } +} + +func TestValidateGTE(t *testing.T) { + tests := []struct { + name string + value int64 + minimum int64 + want field.ErrorList + }{ + { + name: "greater than", + value: 11, + minimum: 10, + }, + { + name: "equal to", + value: 10, + minimum: 10, + }, + { + name: "less than", + value: 9, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(9), "must be greater than or equal to 10"), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateGTE(tt.value, tt.minimum, fldPath); !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ValidateGTE(%v, %v, %v) = %v, want %v", tt.value, tt.minimum, fldPath, got, tt.want) + } + }) + } +} + +func TestValidateGT(t *testing.T) { + tests := []struct { + name string + value int64 + minimum int64 + want field.ErrorList + }{ + { + name: "greater than", + value: 11, + minimum: 10, + }, + { + name: "equal to", + value: 10, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(10), "must be greater than 10"), + }, + }, + { + name: "less than", + value: 9, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(9), "must be greater than 10"), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateGT(tt.value, tt.minimum, fldPath); !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ValidateGT(%v, %v, %v) = %v, want %v", tt.value, tt.minimum, fldPath, got, tt.want) + } + }) + } +} + +func TestValidateLTE(t *testing.T) { + tests := []struct { + name string + value int64 + minimum int64 + want field.ErrorList + }{ + { + name: "greater than", + value: 11, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(11), "must be less than or equal to 10"), + }, + }, + { + name: "equal to", + value: 10, + minimum: 10, + }, + { + name: "less than", + value: 9, + minimum: 10, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateLTE(tt.value, tt.minimum, fldPath); !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ValidateLTE(%v, %v, %v) = %v, want %v", tt.value, tt.minimum, fldPath, got, tt.want) + } + }) + } +} + +func TestValidateLT(t *testing.T) { + tests := []struct { + name string + value int64 + minimum int64 + want field.ErrorList + }{ + { + name: "greater than", + value: 11, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(11), "must be less than 10"), + }, + }, + { + name: "equal to", + value: 10, + minimum: 10, + want: field.ErrorList{ + field.Invalid(fldPath, int64(10), "must be less than 10"), + }, + }, + { + name: "less than", + value: 9, + minimum: 10, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ValidateLT(tt.value, tt.minimum, fldPath); !cmp.Equal(got, tt.want, cmpopts.EquateEmpty()) { + t.Errorf("ValidateLT(%v, %v, %v) = %v, want %v", tt.value, tt.minimum, fldPath, got, tt.want) + } + }) + } +} diff --git a/pkg/execution/controllers/jobcontroller/reconciler.go b/pkg/execution/controllers/jobcontroller/reconciler.go index 0a74247..f5a1cd8 100644 --- a/pkg/execution/controllers/jobcontroller/reconciler.go +++ b/pkg/execution/controllers/jobcontroller/reconciler.go @@ -168,7 +168,6 @@ func (w *Reconciler) syncJobTasks( return nil, errors.Wrapf(err, "cannot create task manager") } - // Note that max number of allowed tasks is maxRetries + 1. maxAllowedTasks := jobutil.GetMaxAllowedTasks(rj) // Get all tasks for job from cache. diff --git a/pkg/execution/mutation/mutation.go b/pkg/execution/mutation/mutation.go index 0aec797..35c211c 100644 --- a/pkg/execution/mutation/mutation.go +++ b/pkg/execution/mutation/mutation.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/pointer" "github.com/furiko-io/furiko/apis/execution" "github.com/furiko-io/furiko/apis/execution/v1alpha1" @@ -137,6 +138,9 @@ func (m *Mutator) MutateJob(rj *v1alpha1.Job) *webhook.Result { if rj.Spec.TTLSecondsAfterFinished == nil { rj.Spec.TTLSecondsAfterFinished = cfg.DefaultTTLSecondsAfterFinished } + if rj.Spec.Template.MaxAttempts == nil { + rj.Spec.Template.MaxAttempts = pointer.Int32(1) + } return result } diff --git a/pkg/execution/mutation/mutation_test.go b/pkg/execution/mutation/mutation_test.go index bee898e..40a9071 100644 --- a/pkg/execution/mutation/mutation_test.go +++ b/pkg/execution/mutation/mutation_test.go @@ -595,14 +595,23 @@ func TestMutator_MutateJob(t *testing.T) { rj: &v1alpha1.Job{ ObjectMeta: objectMetaJob, Spec: v1alpha1.JobSpec{ - Template: &jobTemplateSpecBasic.Spec, + Template: &v1alpha1.JobTemplateSpec{ + Task: v1alpha1.JobTaskSpec{ + Template: podTemplateSpecBasic, + }, + }, }, }, want: &v1alpha1.Job{ ObjectMeta: objectMetaJob, Spec: v1alpha1.JobSpec{ - Type: v1alpha1.JobTypeAdhoc, - Template: &jobTemplateSpecBasic.Spec, + Type: v1alpha1.JobTypeAdhoc, + Template: &v1alpha1.JobTemplateSpec{ + Task: v1alpha1.JobTaskSpec{ + Template: podTemplateSpecBasic, + }, + MaxAttempts: pointer.Int32(1), + }, TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished, }, }, @@ -612,8 +621,13 @@ func TestMutator_MutateJob(t *testing.T) { rj: &v1alpha1.Job{ ObjectMeta: objectMetaJob, Spec: v1alpha1.JobSpec{ - Type: v1alpha1.JobTypeAdhoc, - Template: &jobTemplateSpecBasic.Spec, + Type: v1alpha1.JobTypeAdhoc, + Template: &v1alpha1.JobTemplateSpec{ + Task: v1alpha1.JobTaskSpec{ + Template: podTemplateSpecBasic, + }, + MaxAttempts: pointer.Int32(1), + }, TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished, }, }, diff --git a/pkg/execution/validation/corev1.go b/pkg/execution/validation/corev1.go index 35164c2..45fdfa8 100644 --- a/pkg/execution/validation/corev1.go +++ b/pkg/execution/validation/corev1.go @@ -26,6 +26,8 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kubernetes/pkg/apis/core" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" + + "github.com/furiko-io/furiko/pkg/core/validation" ) var ( @@ -52,7 +54,7 @@ func ValidatePodTemplateSpec(spec *corev1.PodTemplateSpec, fieldPath *field.Path err = corev1.SchemeBuilder.AddToScheme(scheme) }) if err != nil { - return ToInternalErrorList(fieldPath, errors.Wrapf(err, "cannot add to scheme")) + return validation.ToInternalErrorList(fieldPath, errors.Wrapf(err, "cannot add to scheme")) } // Wrap into a new PodTemplate and perform defaulting. @@ -65,7 +67,7 @@ func ValidatePodTemplateSpec(spec *corev1.PodTemplateSpec, fieldPath *field.Path // corev1.PodTemplateSpec to core.PodTemplateSpec. var corePodTemplateSpec core.PodTemplateSpec if err := scheme.Convert(&podTemplate.Template, &corePodTemplateSpec, &conversion.Meta{}); err != nil { - return ToInternalErrorList(fieldPath, errors.Wrapf(err, "conversion error")) + return validation.ToInternalErrorList(fieldPath, errors.Wrapf(err, "conversion error")) } // Validate core.PodTemplateSpec using default Kubernetes validators. diff --git a/pkg/execution/validation/validation.go b/pkg/execution/validation/validation.go index ed2be1e..5272f10 100644 --- a/pkg/execution/validation/validation.go +++ b/pkg/execution/validation/validation.go @@ -32,6 +32,7 @@ import ( "github.com/furiko-io/furiko/apis/execution/v1alpha1" "github.com/furiko-io/furiko/pkg/core/options" "github.com/furiko-io/furiko/pkg/core/tzutils" + "github.com/furiko-io/furiko/pkg/core/validation" "github.com/furiko-io/furiko/pkg/execution/util/cronparser" executionlister "github.com/furiko-io/furiko/pkg/generated/listers/execution/v1alpha1" "github.com/furiko-io/furiko/pkg/runtime/controllercontext" @@ -66,7 +67,7 @@ func NewValidator(ctrlContext controllercontext.Context) *Validator { // ValidateJobConfig validates a *v1alpha1.JobConfig. func (v *Validator) ValidateJobConfig(rjc *v1alpha1.JobConfig) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidateMaxLength(rjc.Name, maxJobConfigNameLen, field.NewPath("metadata").Child("name"))...) + allErrs = append(allErrs, validation.ValidateMaxLength(rjc.Name, maxJobConfigNameLen, field.NewPath("metadata").Child("name"))...) allErrs = append(allErrs, v.ValidateJobConfigSpec(&rjc.Spec, field.NewPath("spec"))...) return allErrs } @@ -94,7 +95,7 @@ func (v *Validator) ValidateJob(rj *v1alpha1.Job) field.ErrorList { // ValidateJobMetadata validates the metadata of a *v1alpha1.Job. func (v *Validator) ValidateJobMetadata(metadata *metav1.ObjectMeta, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, ValidateMaxLength(metadata.Name, maxJobNameLen, fldPath.Child("name"))...) + allErrs = append(allErrs, validation.ValidateMaxLength(metadata.Name, maxJobNameLen, fldPath.Child("name"))...) return allErrs } @@ -160,7 +161,7 @@ func (v *Validator) ValidateJobUpdate(oldRj, rj *v1alpha1.Job) field.ErrorList { // Once Job is started, not allowed to update startPolicy. if !rj.Status.StartTime.IsZero() { - allErrs = append(allErrs, ValidateImmutableField(rj.Spec.StartPolicy, oldRj.Spec.StartPolicy, + allErrs = append(allErrs, validation.ValidateImmutableField(rj.Spec.StartPolicy, oldRj.Spec.StartPolicy, field.NewPath("spec.startPolicy"), "cannot update startPolicy once Job is started")...) } @@ -332,7 +333,7 @@ func (v *Validator) ValidateJobSpecUpdate(oldSpec, spec *v1alpha1.JobSpec, fldPa func (v *Validator) ValidateJobTemplateSpecImmutable(oldTemplate, template *v1alpha1.JobTemplateSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateImmutableField(template.Task, oldTemplate.Task, fldPath.Child("task"))...) - allErrs = append(allErrs, apivalidation.ValidateImmutableField(template.MaxRetryAttempts, oldTemplate.MaxRetryAttempts, fldPath.Child("maxRetryAttempts"))...) + allErrs = append(allErrs, apivalidation.ValidateImmutableField(template.MaxAttempts, oldTemplate.MaxAttempts, fldPath.Child("maxAttempts"))...) allErrs = append(allErrs, apivalidation.ValidateImmutableField(template.RetryDelaySeconds, oldTemplate.RetryDelaySeconds, fldPath.Child("retryDelaySeconds"))...) return allErrs } @@ -381,8 +382,8 @@ func (v *Validator) ValidateStartPolicySpec(spec *v1alpha1.StartPolicySpec, fldP func (v *Validator) ValidateJobTemplateSpec(template *v1alpha1.JobTemplateSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, v.ValidateJobTaskSpec(&template.Task, fldPath.Child("task"))...) - if template.MaxRetryAttempts != nil { - allErrs = append(allErrs, v.ValidateMaxRetryAttempts(*template.MaxRetryAttempts, fldPath.Child("maxRetryAttempts"))...) + if template.MaxAttempts != nil { + allErrs = append(allErrs, v.ValidateMaxRetryAttempts(*template.MaxAttempts, fldPath.Child("maxAttempts"))...) } if template.RetryDelaySeconds != nil { allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(*template.RetryDelaySeconds, fldPath.Child("retryDelaySeconds"))...) @@ -392,12 +393,13 @@ func (v *Validator) ValidateJobTemplateSpec(template *v1alpha1.JobTemplateSpec, func (v *Validator) ValidateMaxRetryAttempts(attempts int32, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(attempts), fldPath)...) - // Set a limit of 49 retries (excluding first attempt) arbitrarily. - if attempts > 49 { - allErrs = append(allErrs, field.Invalid(fldPath, attempts, "maxRetryAttempts must be at most 49")) - } + // Must be greater than 0. + allErrs = append(allErrs, validation.ValidateGT(int64(attempts), 0, fldPath)...) + + // Set an arbitrary limit of 50 attempts. + // TODO(irvinlim): Support configuring this value + allErrs = append(allErrs, validation.ValidateLTE(int64(attempts), 50, fldPath)...) return allErrs } diff --git a/pkg/execution/validation/validation_test.go b/pkg/execution/validation/validation_test.go index 883d33a..1c1c5c2 100644 --- a/pkg/execution/validation/validation_test.go +++ b/pkg/execution/validation/validation_test.go @@ -95,7 +95,7 @@ var ( Template: podTemplateSpecBasic, PendingTimeoutSeconds: pointer.Int64(1800), }, - MaxRetryAttempts: pointer.Int32(5), + MaxAttempts: pointer.Int32(5), RetryDelaySeconds: pointer.Int64(60), }, } @@ -107,7 +107,7 @@ var ( Template: podTemplateSpecBasic, PendingTimeoutSeconds: pointer.Int64(3600), }, - MaxRetryAttempts: jobTemplateSpecBasic.Spec.MaxRetryAttempts, + MaxAttempts: jobTemplateSpecBasic.Spec.MaxAttempts, RetryDelaySeconds: jobTemplateSpecBasic.Spec.RetryDelaySeconds, }, } @@ -116,7 +116,7 @@ var ( ObjectMeta: jobTemplateSpecBasic.ObjectMeta, Spec: v1alpha1.JobTemplateSpec{ Task: jobTemplateSpecBasic.Spec.Task, - MaxRetryAttempts: pointer.Int32(10), + MaxAttempts: pointer.Int32(10), RetryDelaySeconds: jobTemplateSpecBasic.Spec.RetryDelaySeconds, }, } @@ -125,7 +125,7 @@ var ( ObjectMeta: jobTemplateSpecBasic.ObjectMeta, Spec: v1alpha1.JobTemplateSpec{ Task: jobTemplateSpecBasic.Spec.Task, - MaxRetryAttempts: pointer.Int32(100), + MaxAttempts: pointer.Int32(100), RetryDelaySeconds: jobTemplateSpecBasic.Spec.RetryDelaySeconds, }, } @@ -339,7 +339,7 @@ func TestValidateJobConfig(t *testing.T) { wantErr: "spec.schedule.cron.timezone: Invalid value: \"Invalid/Time/Zone\": cannot parse timezone", }, { - name: "maxRetryAttempts too large", + name: "maxAttempts too large", rjc: &v1alpha1.JobConfig{ Spec: v1alpha1.JobConfigSpec{ Template: jobTemplateSpecTooManyRetries, @@ -347,7 +347,7 @@ func TestValidateJobConfig(t *testing.T) { Schedule: &scheduleSpecBasic, }, }, - wantErr: "spec.template.spec.maxRetryAttempts: Invalid value: 100: maxRetryAttempts must be at most 49", + wantErr: "spec.template.spec.maxAttempts: Invalid value: 100: must be less than or equal to 50", }, { name: "invalid options", @@ -519,7 +519,7 @@ func TestValidateJob(t *testing.T) { wantErr: "spec.ttlSecondsAfterFinished: Invalid value: -300", }, { - name: "maxRetryAttempts too large", + name: "maxAttempts too large", rj: &v1alpha1.Job{ ObjectMeta: objectMetaJob, Spec: v1alpha1.JobSpec{ @@ -527,7 +527,7 @@ func TestValidateJob(t *testing.T) { Template: &jobTemplateSpecTooManyRetries.Spec, }, }, - wantErr: "spec.template.maxRetryAttempts: Invalid value: 100: maxRetryAttempts must be at most 49", + wantErr: "spec.template.maxAttempts: Invalid value: 100: must be less than or equal to 50", }, { name: "invalid pod template", @@ -720,7 +720,7 @@ func TestValidateJobUpdate(t *testing.T) { wantErr: "spec.template.task: Invalid value", }, { - name: "immutable field maxRetryAttempts", + name: "immutable field maxAttempts", oldRj: &v1alpha1.Job{ ObjectMeta: objectMetaJob, Spec: v1alpha1.JobSpec{ @@ -735,7 +735,7 @@ func TestValidateJobUpdate(t *testing.T) { Template: &jobTemplateSpecMoreRetries.Spec, }, }, - wantErr: "spec.template.maxRetryAttempts: Invalid value: 10: field is immutable", + wantErr: "spec.template.maxAttempts: Invalid value: 10: field is immutable", }, { name: "can set KillTimestamp", diff --git a/pkg/execution/variablecontext/provider.go b/pkg/execution/variablecontext/provider.go index f461075..cd5fdf1 100644 --- a/pkg/execution/variablecontext/provider.go +++ b/pkg/execution/variablecontext/provider.go @@ -81,8 +81,8 @@ func (c *defaultProvider) MakeVariablesFromJob(rj *execution.Job) map[string]str "job.type": string(rj.Spec.Type), } - if maxRetries := rj.Spec.Template.MaxRetryAttempts; maxRetries != nil { - subs["job.max_retries"] = strconv.Itoa(int(*maxRetries)) + if maxAttempts := rj.Spec.Template.MaxAttempts; maxAttempts != nil { + subs["job.max_attempts"] = strconv.Itoa(int(*maxAttempts)) } return subs diff --git a/pkg/utils/execution/job/condition_test.go b/pkg/utils/execution/job/condition_test.go index 15e09f0..87f26f5 100644 --- a/pkg/utils/execution/job/condition_test.go +++ b/pkg/utils/execution/job/condition_test.go @@ -309,7 +309,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -374,7 +374,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -442,7 +442,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1", "task2"), @@ -482,7 +482,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1", "task2"), @@ -541,7 +541,7 @@ func TestGetCondition(t *testing.T) { Spec: execution.JobSpec{ KillTimestamp: &killTime, Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: execution.JobStatus{ @@ -585,7 +585,7 @@ func TestGetCondition(t *testing.T) { Spec: execution.JobSpec{ KillTimestamp: &killTime, Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: execution.JobStatus{ @@ -662,7 +662,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -683,7 +683,7 @@ func TestGetCondition(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: execution.JobStatus{ @@ -720,7 +720,7 @@ func TestGetCondition(t *testing.T) { Spec: execution.JobSpec{ KillTimestamp: &killTime, Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), diff --git a/pkg/utils/execution/job/job_utils_test.go b/pkg/utils/execution/job/job_utils_test.go index 2d1ed9e..150e40b 100644 --- a/pkg/utils/execution/job/job_utils_test.go +++ b/pkg/utils/execution/job/job_utils_test.go @@ -48,7 +48,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -70,7 +70,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -96,7 +96,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -124,7 +124,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -176,7 +176,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { Spec: execution.JobSpec{ KillTimestamp: &killTime, Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, }, }, Status: createTaskRefsStatus("task1"), @@ -204,7 +204,7 @@ func TestAllowedToCreateNewTask(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, }, }, Status: createTaskRefsStatus("task1", "task2"), diff --git a/pkg/utils/execution/job/task_test.go b/pkg/utils/execution/job/task_test.go index 565e12d..53929e2 100644 --- a/pkg/utils/execution/job/task_test.go +++ b/pkg/utils/execution/job/task_test.go @@ -43,8 +43,8 @@ var ( finishTime = metav1.NewTime(stdFinishTime) finishTime2 = metav1.NewTime(stdFinishTime.Add(time.Minute)) killTime = metav1.NewTime(stdKillTime) - one = int32(1) two = int32(2) + three = int32(3) ) type stubTask struct { diff --git a/pkg/utils/execution/job/task_utils.go b/pkg/utils/execution/job/task_utils.go index c959f3b..2af2168 100644 --- a/pkg/utils/execution/job/task_utils.go +++ b/pkg/utils/execution/job/task_utils.go @@ -49,13 +49,13 @@ func IsTaskFinished(task jobtasks.Task) bool { // GetMaxAllowedTasks returns the maximum number of allowed tasks that a Job can have. func GetMaxAllowedTasks(rj *execution.Job) int64 { - var maxRetries int64 + var maxAttempts int64 = 1 if template := rj.Spec.Template; template != nil { - if rj.Spec.Template.MaxRetryAttempts != nil { - maxRetries = int64(*rj.Spec.Template.MaxRetryAttempts) + if rj.Spec.Template.MaxAttempts != nil { + maxAttempts = int64(*rj.Spec.Template.MaxAttempts) } } - return maxRetries + 1 + return maxAttempts } // GetNextAllowedRetry checks if we can create a new task, and if so, returns diff --git a/pkg/utils/execution/job/task_utils_test.go b/pkg/utils/execution/job/task_utils_test.go index 93dfe12..a078cf8 100644 --- a/pkg/utils/execution/job/task_utils_test.go +++ b/pkg/utils/execution/job/task_utils_test.go @@ -38,7 +38,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, }, }, }, @@ -49,7 +49,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, RetryDelaySeconds: pointer.Int64(30), }, }, @@ -82,7 +82,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, }, }, Status: execution.JobStatus{ @@ -107,7 +107,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, RetryDelaySeconds: pointer.Int64(30), }, }, @@ -133,7 +133,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, RetryDelaySeconds: pointer.Int64(30), }, }, @@ -158,7 +158,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &two, + MaxAttempts: &three, RetryDelaySeconds: pointer.Int64(30), }, KillTimestamp: &killTime, @@ -185,7 +185,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, RetryDelaySeconds: pointer.Int64(30), }, }, @@ -220,7 +220,7 @@ func TestGetNextAllowedRetry(t *testing.T) { rj: &execution.Job{ Spec: execution.JobSpec{ Template: &execution.JobTemplateSpec{ - MaxRetryAttempts: &one, + MaxAttempts: &two, RetryDelaySeconds: pointer.Int64(30), }, },