Merge pull request #119877 from dejanzele/cleanup-validate-selector

cleanup: extract generateSelector from Validate method in job strategy

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.

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{