From 2ecd24011aa783af244b35697a8833f0ce1b28be Mon Sep 17 00:00:00 2001 From: ahg-g Date: Tue, 28 Feb 2023 15:31:24 +0000 Subject: [PATCH] Graduate JobMutableNodeSchedulingDirectives feature to GA --- pkg/features/kube_features.go | 3 +- pkg/registry/batch/job/strategy.go | 3 +- pkg/registry/batch/job/strategy_test.go | 35 +------ test/integration/job/job_test.go | 125 +++++++++++------------- 4 files changed, 61 insertions(+), 105 deletions(-) diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 411b9318d5c..efc73fe71c4 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -402,6 +402,7 @@ const ( // owner: @ahg // beta: v1.23 + // stable: v1.27 // // Allow updating node scheduling directives in the pod template of jobs. Specifically, // node affinity, selector and tolerations. This is allowed only for suspended jobs @@ -963,7 +964,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS JobPodFailurePolicy: {Default: true, PreRelease: featuregate.Beta}, - JobMutableNodeSchedulingDirectives: {Default: true, PreRelease: featuregate.Beta}, + JobMutableNodeSchedulingDirectives: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.29 JobReadyPods: {Default: true, PreRelease: featuregate.Beta}, diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 71a9c7eafca..2dfae56c236 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -183,8 +183,7 @@ func validationOptionsForJob(newJob, oldJob *batch.Job) batchvalidation.JobValid // only for suspended jobs that never started before. suspended := oldJob.Spec.Suspend != nil && *oldJob.Spec.Suspend notStarted := oldJob.Status.StartTime == nil - opts.AllowMutableSchedulingDirectives = utilfeature.DefaultFeatureGate.Enabled(features.JobMutableNodeSchedulingDirectives) && - suspended && notStarted + opts.AllowMutableSchedulingDirectives = suspended && notStarted } return opts } diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index e92274af47f..981fdb80425 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -472,10 +472,9 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) { } now := metav1.Now() cases := map[string]struct { - job *batch.Job - update func(*batch.Job) - wantErrs field.ErrorList - mutableSchedulingDirectivesEnabled bool + job *batch.Job + update func(*batch.Job) + wantErrs field.ErrorList }{ "update parallelism": { job: &batch.Job{ @@ -603,7 +602,6 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) { wantErrs: field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "spec.template"}, }, - mutableSchedulingDirectivesEnabled: true, }, "updating node selector for suspended but previously started job disallowed": { job: &batch.Job{ @@ -630,7 +628,6 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) { wantErrs: field.ErrorList{ {Type: field.ErrorTypeInvalid, Field: "spec.template"}, }, - mutableSchedulingDirectivesEnabled: true, }, "updating node selector for suspended and not previously started job allowed": { job: &batch.Job{ @@ -651,31 +648,6 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) { update: func(job *batch.Job) { job.Spec.Template.Spec.NodeSelector = map[string]string{"foo": "bar"} }, - mutableSchedulingDirectivesEnabled: true, - }, - "updating node selector whilte gate disabled disallowed": { - job: &batch.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myjob", - Namespace: metav1.NamespaceDefault, - ResourceVersion: "0", - Annotations: map[string]string{"foo": "bar"}, - }, - Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, - ManualSelector: pointer.BoolPtr(true), - Parallelism: pointer.Int32Ptr(1), - Suspend: pointer.BoolPtr(true), - }, - }, - update: func(job *batch.Job) { - job.Spec.Template.Spec.NodeSelector = map[string]string{"foo": "bar"} - }, - wantErrs: field.ErrorList{ - {Type: field.ErrorTypeInvalid, Field: "spec.template"}, - }, - mutableSchedulingDirectivesEnabled: false, }, "invalid label selector": { job: &batch.Job{ @@ -701,7 +673,6 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.JobMutableNodeSchedulingDirectives, tc.mutableSchedulingDirectivesEnabled)() newJob := tc.job.DeepCopy() tc.update(newJob) errs := Strategy.ValidateUpdate(ctx, newJob, tc.job) diff --git a/test/integration/job/job_test.go b/test/integration/job/job_test.go index 7f6975539f4..b1b857e5704 100644 --- a/test/integration/job/job_test.go +++ b/test/integration/job/job_test.go @@ -1574,78 +1574,63 @@ func TestSuspendJobControllerRestart(t *testing.T) { } func TestNodeSelectorUpdate(t *testing.T) { - for name, featureGate := range map[string]bool{ - "feature gate disabled": false, - "feature gate enabled": true, - } { - t.Run(name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobMutableNodeSchedulingDirectives, featureGate)() + closeFn, restConfig, clientSet, ns := setup(t, "suspend") + defer closeFn() + ctx, cancel := startJobControllerAndWaitForCaches(restConfig) + defer cancel() - closeFn, restConfig, clientSet, ns := setup(t, "suspend") - defer closeFn() - ctx, cancel := startJobControllerAndWaitForCaches(restConfig) - defer cancel() - - job, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{Spec: batchv1.JobSpec{ - Parallelism: pointer.Int32(1), - Suspend: pointer.BoolPtr(true), - }}) - if err != nil { - t.Fatalf("Failed to create Job: %v", err) - } - jobName := job.Name - jobNamespace := job.Namespace - jobClient := clientSet.BatchV1().Jobs(jobNamespace) - - // (1) Unsuspend and set node selector in the same update. - nodeSelector := map[string]string{"foo": "bar"} - _, err = updateJob(ctx, jobClient, jobName, func(j *batchv1.Job) { - j.Spec.Template.Spec.NodeSelector = nodeSelector - j.Spec.Suspend = pointer.BoolPtr(false) - }) - if !featureGate { - if err == nil || !strings.Contains(err.Error(), "spec.template: Invalid value") { - t.Errorf("Expected \"spec.template: Invalid value\" error, got: %v", err) - } - } else if featureGate && err != nil { - t.Errorf("Unexpected error: %v", err) - } - - // (2) Check that the pod was created using the expected node selector. - if featureGate { - var pod *v1.Pod - if err := wait.PollImmediate(waitInterval, wait.ForeverTestTimeout, func() (bool, error) { - pods, err := clientSet.CoreV1().Pods(jobNamespace).List(ctx, metav1.ListOptions{}) - if err != nil { - t.Fatalf("Failed to list Job Pods: %v", err) - } - if len(pods.Items) == 0 { - return false, nil - } - pod = &pods.Items[0] - return true, nil - }); err != nil || pod == nil { - t.Fatalf("pod not found: %v", err) - } - - // if the feature gate is enabled, then the job should now be unsuspended and - // the pod has the node selector. - if diff := cmp.Diff(nodeSelector, pod.Spec.NodeSelector); diff != "" { - t.Errorf("Unexpected nodeSelector (-want,+got):\n%s", diff) - } - } - - // (3) Update node selector again. It should fail since the job is unsuspended. - _, err = updateJob(ctx, jobClient, jobName, func(j *batchv1.Job) { - j.Spec.Template.Spec.NodeSelector = map[string]string{"foo": "baz"} - }) - - if err == nil || !strings.Contains(err.Error(), "spec.template: Invalid value") { - t.Errorf("Expected \"spec.template: Invalid value\" error, got: %v", err) - } - - }) + job, err := createJobWithDefaults(ctx, clientSet, ns.Name, &batchv1.Job{Spec: batchv1.JobSpec{ + Parallelism: pointer.Int32(1), + Suspend: pointer.BoolPtr(true), + }}) + if err != nil { + t.Fatalf("Failed to create Job: %v", err) } + jobName := job.Name + jobNamespace := job.Namespace + jobClient := clientSet.BatchV1().Jobs(jobNamespace) + + // (1) Unsuspend and set node selector in the same update. + nodeSelector := map[string]string{"foo": "bar"} + if _, err := updateJob(ctx, jobClient, jobName, func(j *batchv1.Job) { + j.Spec.Template.Spec.NodeSelector = nodeSelector + j.Spec.Suspend = pointer.BoolPtr(false) + }); err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // (2) Check that the pod was created using the expected node selector. + + var pod *v1.Pod + if err := wait.PollImmediate(waitInterval, wait.ForeverTestTimeout, func() (bool, error) { + pods, err := clientSet.CoreV1().Pods(jobNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failed to list Job Pods: %v", err) + } + if len(pods.Items) == 0 { + return false, nil + } + pod = &pods.Items[0] + return true, nil + }); err != nil || pod == nil { + t.Fatalf("pod not found: %v", err) + } + + // if the feature gate is enabled, then the job should now be unsuspended and + // the pod has the node selector. + if diff := cmp.Diff(nodeSelector, pod.Spec.NodeSelector); diff != "" { + t.Errorf("Unexpected nodeSelector (-want,+got):\n%s", diff) + } + + // (3) Update node selector again. It should fail since the job is unsuspended. + _, err = updateJob(ctx, jobClient, jobName, func(j *batchv1.Job) { + j.Spec.Template.Spec.NodeSelector = map[string]string{"foo": "baz"} + }) + + if err == nil || !strings.Contains(err.Error(), "spec.template: Invalid value") { + t.Errorf("Expected \"spec.template: Invalid value\" error, got: %v", err) + } + } type podsByStatus struct {