Repository: furiko-io/furiko (go)

Issue:
Bug: Changing defaultPendingTimeoutSeconds should not affect already created jobs
When updating the `defaultPendingTimeoutSeconds`, any already ongoing jobs will be affected by the change. This is probably undesirable, especially if the cluster administrator shortens the timeout from a longer one to a shorter one, it may cause previously created jobs to be killed. We should uphold a principle such that old resources should NOT be affected by newly updated defaults.

As such, we should add `pendingTimeoutSeconds` via the JobMutatingWebhook if it is not specified, so that the controller does not wrongly use the new global configuration.

Pull request:
fix(mutation): Add taskPendingTimeoutSeconds in mutation webhook
Closes #54.

Patch:
diff --git a/pkg/execution/mutation/mutation.go b/pkg/execution/mutation/mutation.go
index af37f93..0716688 100644
--- a/pkg/execution/mutation/mutation.go
+++ b/pkg/execution/mutation/mutation.go
@@ -298,7 +298,7 @@ func (m *Mutator) evaluateOptionValues(rj *v1alpha1.Job, rjc *v1alpha1.JobConfig
 // MutateJobTemplateSpec mutates a JobTemplate in-place.
 func (m *Mutator) MutateJobTemplateSpec(
 	spec *v1alpha1.JobTemplate,
-	taskTemplate bool,
+	mutateTaskTemplate bool,
 	fldPath *field.Path,
 ) *webhook.Result {
 	result := webhook.NewResult()
@@ -306,10 +306,18 @@ func (m *Mutator) MutateJobTemplateSpec(
 	if spec.MaxAttempts == nil {
 		spec.MaxAttempts = pointer.Int64(1)
 	}
+	if spec.TaskPendingTimeoutSeconds == nil {
+		cfg, err := m.ctrlContext.Configs().Jobs()
+		if err != nil {
+			result.Errors = append(result.Errors, field.InternalError(field.NewPath(""), errors.Wrapf(err, "cannot load dynamic configuration")))
+		} else {
+			spec.TaskPendingTimeoutSeconds = cfg.DefaultPendingTimeoutSeconds
+		}
+	}
 	if spec.Parallelism != nil {
 		result.Merge(m.MutateParallelismSpec(spec.Parallelism, fldPath.Child("parallelism")))
 	}
-	if taskTemplate {
+	if mutateTaskTemplate {
 		result.Merge(m.MutateTaskTemplate(&spec.TaskTemplate, fldPath.Child("taskTemplate")))
 	}
 


Test patch:
diff --git a/pkg/execution/mutation/mutation_test.go b/pkg/execution/mutation/mutation_test.go
index 5158396..6f725d8 100644
--- a/pkg/execution/mutation/mutation_test.go
+++ b/pkg/execution/mutation/mutation_test.go
@@ -167,7 +167,8 @@ var (
 			TaskTemplate: v1alpha1.TaskTemplate{
 				Pod: &podTemplateSpecBasic,
 			},
-			MaxAttempts: pointer.Int64(1),
+			TaskPendingTimeoutSeconds: pointer.Int64(900),
+			MaxAttempts:               pointer.Int64(1),
 		},
 	}
 
@@ -221,6 +222,7 @@ var (
 func TestMutator_MutateJobConfig(t *testing.T) {
 	tests := []struct {
 		name         string
+		cfgs         map[configv1alpha1.ConfigName]runtime.Object
 		rjc          *v1alpha1.JobConfig
 		want         *v1alpha1.JobConfig
 		wantErrors   string
@@ -245,7 +247,8 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 							TaskTemplate: v1alpha1.TaskTemplate{
 								Pod: &podTemplateSpecBare,
 							},
-							MaxAttempts: pointer.Int64(1),
+							MaxAttempts:               pointer.Int64(1),
+							TaskPendingTimeoutSeconds: pointer.Int64(900),
 						},
 					},
 				},
@@ -276,7 +279,8 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 							TaskTemplate: v1alpha1.TaskTemplate{
 								Pod: &podTemplateSpecBare,
 							},
-							MaxAttempts: pointer.Int64(1),
+							MaxAttempts:               pointer.Int64(1),
+							TaskPendingTimeoutSeconds: pointer.Int64(900),
 							Parallelism: &v1alpha1.ParallelismSpec{
 								WithCount:          pointer.Int64(3),
 								CompletionStrategy: v1alpha1.AllSuccessful,
@@ -324,7 +328,7 @@ func TestMutator_MutateJobConfig(t *testing.T) {
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			mutator := mutation.NewMutator(mock.NewContext())
+			mutator := setup(t, tt.cfgs, nil)
 			newRjc := tt.rjc.DeepCopy()
 			resp := mutator.MutateJobConfig(newRjc)
 
@@ -686,7 +690,8 @@ func TestMutator_MutateJob(t *testing.T) {
 						TaskTemplate: v1alpha1.TaskTemplate{
 							Pod: &podTemplateSpecBasic,
 						},
-						MaxAttempts: pointer.Int64(1),
+						MaxAttempts:               pointer.Int64(1),
+						TaskPendingTimeoutSeconds: pointer.Int64(900),
 					},
 					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,
 				},
@@ -702,7 +707,8 @@ func TestMutator_MutateJob(t *testing.T) {
 						TaskTemplate: v1alpha1.TaskTemplate{
 							Pod: &podTemplateSpecBasic,
 						},
-						MaxAttempts: pointer.Int64(1),
+						MaxAttempts:               pointer.Int64(1),
+						TaskPendingTimeoutSeconds: pointer.Int64(900),
 					},
 					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,
 				},
diff --git a/pkg/execution/mutation/patcher_jobs_test.go b/pkg/execution/mutation/patcher_jobs_test.go
index 212c399..6a0c44f 100644
--- a/pkg/execution/mutation/patcher_jobs_test.go
+++ b/pkg/execution/mutation/patcher_jobs_test.go
@@ -66,7 +66,8 @@ func TestNewJobPatcher(t *testing.T) {
 								},
 							},
 						},
-						MaxAttempts: pointer.Int64(1),
+						MaxAttempts:               pointer.Int64(1),
+						TaskPendingTimeoutSeconds: pointer.Int64(900),
 					},
 					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,
 				},
@@ -86,8 +87,9 @@ func TestNewJobPatcher(t *testing.T) {
 				Spec: v1alpha1.JobSpec{
 					Type: v1alpha1.JobTypeAdhoc,
 					Template: &v1alpha1.JobTemplate{
-						TaskTemplate: jobTemplateSpecBasic.Spec.TaskTemplate,
-						MaxAttempts:  pointer.Int64(1),
+						TaskTemplate:              jobTemplateSpecBasic.Spec.TaskTemplate,
+						MaxAttempts:               pointer.Int64(1),
+						TaskPendingTimeoutSeconds: pointer.Int64(900),
 					},
 					StartPolicy:             &startPolicyBasic,
 					TTLSecondsAfterFinished: config.DefaultJobExecutionConfig.DefaultTTLSecondsAfterFinished,