diff --git a/pkg/registry/batch/job/strategy.go b/pkg/registry/batch/job/strategy.go index 71582bed98c..cb4b2b3a5ff 100644 --- a/pkg/registry/batch/job/strategy.go +++ b/pkg/registry/batch/job/strategy.go @@ -92,6 +92,7 @@ func (jobStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { // PrepareForCreate clears the status of a job before creation. func (jobStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { job := obj.(*batch.Job) + generateSelectorIfNeeded(job) job.Status = batch.JobStatus{} job.Generation = 1 @@ -163,9 +164,6 @@ func (jobStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object // Validate validates a new job. func (jobStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { job := obj.(*batch.Job) - if !*job.Spec.ManualSelector { - generateSelector(job) - } opts := validationOptionsForJob(job, nil) return batchvalidation.ValidateJob(job, opts) } @@ -212,6 +210,13 @@ func (jobStrategy) WarningsOnCreate(ctx context.Context, obj runtime.Object) []s return warnings } +// generateSelectorIfNeeded checks the job's manual selector flag and generates selector labels if the flag is true. +func generateSelectorIfNeeded(obj *batch.Job) { + if !*obj.Spec.ManualSelector { + generateSelector(obj) + } +} + // generateSelector adds a selector to a job and labels to its template // which can be used to uniquely identify the pods created by that job, // if the user has requested this behavior. diff --git a/pkg/registry/batch/job/strategy_test.go b/pkg/registry/batch/job/strategy_test.go index aa7abac20f2..4c1058d9e20 100644 --- a/pkg/registry/batch/job/strategy_test.go +++ b/pkg/registry/batch/job/strategy_test.go @@ -460,6 +460,8 @@ func TestJobStrategy_PrepareForUpdate(t *testing.T) { func TestJobStrategy_PrepareForCreate(t *testing.T) { validSelector := getValidLabelSelector() validPodTemplateSpec := getValidPodTemplateSpecForSelector(validSelector) + validSelectorWithBatchLabels := &metav1.LabelSelector{MatchLabels: getValidBatchLabelsWithNonBatch()} + expectedPodTemplateSpec := getValidPodTemplateSpecForSelector(validSelectorWithBatchLabels) podFailurePolicy := &batch.PodFailurePolicy{ Rules: []batch.PodFailurePolicyRule{ @@ -481,12 +483,31 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { job batch.Job wantJob batch.Job }{ + "generate selectors": { + job: batch.Job{ + ObjectMeta: getValidObjectMeta(0), + Spec: batch.JobSpec{ + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: validPodTemplateSpec, + }, + }, + wantJob: batch.Job{ + ObjectMeta: getValidObjectMeta(1), + Spec: batch.JobSpec{ + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, + }, + }, + }, "create job with a new fields; JobBackoffLimitPerIndex enabled": { enableJobBackoffLimitPerIndex: true, job: batch.Job{ ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, BackoffLimitPerIndex: pointer.Int32(1), MaxFailedIndexes: pointer.Int32(1), @@ -496,7 +517,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, BackoffLimitPerIndex: pointer.Int32(1), MaxFailedIndexes: pointer.Int32(1), }, @@ -508,6 +530,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, BackoffLimitPerIndex: pointer.Int32(1), MaxFailedIndexes: pointer.Int32(1), @@ -517,7 +540,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, BackoffLimitPerIndex: nil, MaxFailedIndexes: nil, }, @@ -529,6 +553,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, PodFailurePolicy: podFailurePolicy, }, @@ -537,7 +562,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodFailurePolicy: podFailurePolicy, }, }, @@ -548,6 +574,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, PodReplacementPolicy: podReplacementPolicy(batch.Failed), }, @@ -556,7 +583,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodReplacementPolicy: podReplacementPolicy(batch.Failed), }, }, @@ -567,6 +595,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, PodReplacementPolicy: podReplacementPolicy(batch.Failed), }, @@ -575,7 +604,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodReplacementPolicy: nil, }, }, @@ -586,6 +616,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, PodFailurePolicy: podFailurePolicy, }, @@ -594,7 +625,8 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ Selector: validSelector, - Template: validPodTemplateSpec, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodFailurePolicy: nil, }, }, @@ -603,8 +635,9 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { job: batch.Job{ ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: validPodTemplateSpec, }, Status: batch.JobStatus{ Active: 1, @@ -613,8 +646,9 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { wantJob: batch.Job{ ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, }, }, }, @@ -625,6 +659,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, BackoffLimitPerIndex: pointer.Int32(1), PodFailurePolicy: &batch.PodFailurePolicy{ @@ -643,8 +678,9 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { wantJob: batch.Job{ ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodFailurePolicy: &batch.PodFailurePolicy{ Rules: []batch.PodFailurePolicyRule{}, }, @@ -658,6 +694,7 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { ObjectMeta: getValidObjectMeta(0), Spec: batch.JobSpec{ Selector: validSelector, + ManualSelector: pointer.Bool(false), Template: validPodTemplateSpec, BackoffLimitPerIndex: pointer.Int32(1), PodFailurePolicy: &batch.PodFailurePolicy{ @@ -690,8 +727,9 @@ func TestJobStrategy_PrepareForCreate(t *testing.T) { wantJob: batch.Job{ ObjectMeta: getValidObjectMeta(1), Spec: batch.JobSpec{ - Selector: validSelector, - Template: validPodTemplateSpec, + Selector: validSelector, + ManualSelector: pointer.Bool(false), + Template: expectedPodTemplateSpec, PodFailurePolicy: &batch.PodFailurePolicy{ Rules: []batch.PodFailurePolicyRule{ { @@ -1291,12 +1329,11 @@ func TestJobStrategy_WarningsOnCreate(t *testing.T) { func TestJobStrategy_Validate(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - theUID := types.UID("1a2b3c4d5e6f7g8h9i0k") - validSelector := &metav1.LabelSelector{ - MatchLabels: map[string]string{"a": "b"}, - } - validLabels := map[string]string{batch.LegacyJobNameLabel: "myjob2", batch.JobNameLabel: "myjob2", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} - labelsWithNonBatch := map[string]string{"a": "b", batch.LegacyJobNameLabel: "myjob2", batch.JobNameLabel: "myjob2", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} + theUID := getValidUID() + validSelector := getValidLabelSelector() + batchLabels := getValidBatchLabels() + labelsWithNonBatch := getValidBatchLabelsWithNonBatch() + defaultSelector := &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}} validPodSpec := api.PodSpec{ RestartPolicy: api.RestartPolicyOnFailure, DNSPolicy: api.DNSClusterFirst, @@ -1304,11 +1341,7 @@ func TestJobStrategy_Validate(t *testing.T) { } validPodSpecNever := *validPodSpec.DeepCopy() validPodSpecNever.RestartPolicy = api.RestartPolicyNever - validObjectMeta := metav1.ObjectMeta{ - Name: "myjob2", - Namespace: metav1.NamespaceDefault, - UID: theUID, - } + validObjectMeta := getValidObjectMeta(0) testcases := map[string]struct { enableJobPodFailurePolicy bool enableJobBackoffLimitPerIndex bool @@ -1316,15 +1349,15 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob *batch.Job wantWarningCount int32 }{ - "valid job with labels in pod template": { + "valid job with batch labels in pod template": { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector.MatchLabels, + Labels: batchLabels, }, Spec: validPodSpec, }}, @@ -1332,22 +1365,74 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector.MatchLabels, + Labels: batchLabels, }, Spec: validPodSpec, }}, }, }, + "valid job with batch and non-batch labels in pod template": { + job: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Selector: defaultSelector, + ManualSelector: pointer.Bool(false), + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: labelsWithNonBatch, + }, + Spec: validPodSpec, + }}, + }, + wantJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Selector: defaultSelector, + ManualSelector: pointer.Bool(false), + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: labelsWithNonBatch, + }, + Spec: validPodSpec, + }}, + }, + }, + "job with non-batch labels and without batch labels in pod template": { + job: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Selector: defaultSelector, + ManualSelector: pointer.Bool(false), + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + Spec: validPodSpec, + }}, + }, + wantJob: &batch.Job{ + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Selector: defaultSelector, + ManualSelector: pointer.Bool(false), + Template: api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + Spec: validPodSpec, + }}, + }, + wantWarningCount: 5, + }, "no labels in job": { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, - ManualSelector: pointer.Bool(false), + Selector: defaultSelector, Template: api.PodTemplateSpec{ Spec: validPodSpec, }}, @@ -1355,41 +1440,12 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, - ManualSelector: pointer.Bool(false), + Selector: defaultSelector, Template: api.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: validLabels, - }, - Spec: validPodSpec, - }}, - }, - }, - "labels exist": { - job: &batch.Job{ - ObjectMeta: validObjectMeta, - Spec: batch.JobSpec{ - Selector: nil, - ManualSelector: pointer.Bool(false), - Template: api.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: labelsWithNonBatch, - }, - Spec: validPodSpec, - }}, - }, - wantJob: &batch.Job{ - ObjectMeta: validObjectMeta, - Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, - ManualSelector: pointer.Bool(false), - Template: api.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: labelsWithNonBatch, - }, Spec: validPodSpec, }}, }, + wantWarningCount: 5, }, "manual selector; do not generate labels": { job: &batch.Job{ @@ -1425,11 +1481,11 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector.MatchLabels, + Labels: labelsWithNonBatch, }, Spec: validPodSpec, }, @@ -1442,7 +1498,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1461,11 +1517,11 @@ func TestJobStrategy_Validate(t *testing.T) { job: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: nil, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector.MatchLabels, + Labels: labelsWithNonBatch, }, Spec: api.PodSpec{ RestartPolicy: api.RestartPolicyOnFailure, @@ -1479,7 +1535,7 @@ func TestJobStrategy_Validate(t *testing.T) { wantJob: &batch.Job{ ObjectMeta: validObjectMeta, Spec: batch.JobSpec{ - Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}}, + Selector: defaultSelector, ManualSelector: pointer.Bool(false), Template: api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ @@ -1959,9 +2015,14 @@ func getValidObjectMeta(generation int64) metav1.ObjectMeta { return getValidObjectMetaWithAnnotations(generation, nil) } +func getValidUID() types.UID { + return "1a2b3c4d5e6f7g8h9i0k" +} + func getValidObjectMetaWithAnnotations(generation int64, annotations map[string]string) metav1.ObjectMeta { return metav1.ObjectMeta{ Name: "myjob", + UID: getValidUID(), Namespace: metav1.NamespaceDefault, Generation: generation, Annotations: annotations, @@ -1974,6 +2035,16 @@ func getValidLabelSelector() *metav1.LabelSelector { } } +func getValidBatchLabels() map[string]string { + theUID := getValidUID() + return map[string]string{batch.LegacyJobNameLabel: "myjob", batch.JobNameLabel: "myjob", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} +} + +func getValidBatchLabelsWithNonBatch() map[string]string { + theUID := getValidUID() + return map[string]string{"a": "b", batch.LegacyJobNameLabel: "myjob", batch.JobNameLabel: "myjob", batch.LegacyControllerUidLabel: string(theUID), batch.ControllerUidLabel: string(theUID)} +} + func getValidPodTemplateSpecForSelector(validSelector *metav1.LabelSelector) api.PodTemplateSpec { return api.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{