Merge pull request #114930 from kannon92/add-new-labels

Add batch.kubernetes.io to labels created in the Job controller.

pkg/apis/batch/types.go

@@ -22,16 +22,29 @@ import (
 	api "k8s.io/kubernetes/pkg/apis/core"
 )
 
-// JobTrackingFinalizer is a finalizer for Job's pods. It prevents them from
+const (
-// being deleted before being accounted in the Job status.
+	// Unprefixed labels are reserved for end-users
-//
+	// so we will add a batch.kubernetes.io to designate these labels as official Kubernetes labels.
-// Additionally, the apiserver and job controller use this string as a Job
+	// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions
-// annotation, to mark Jobs that are being tracked using pod finalizers.
+	labelPrefix = "batch.kubernetes.io/"
-// However, this behavior is deprecated in kubernetes 1.26. This means that, in
+	// JobTrackingFinalizer is a finalizer for Job's pods. It prevents them from
-// 1.27+, one release after JobTrackingWithFinalizers graduates to GA, the
+	// being deleted before being accounted in the Job status.
-// apiserver and job controller will ignore this annotation and they will
+	//
-// always track jobs using finalizers.
+	// Additionally, the apiserver and job controller use this string as a Job
-const JobTrackingFinalizer = "batch.kubernetes.io/job-tracking"
+	// annotation, to mark Jobs that are being tracked using pod finalizers.
+	// However, this behavior is deprecated in kubernetes 1.26. This means that, in
+	// 1.27+, one release after JobTrackingWithFinalizers graduates to GA, the
+	// apiserver and job controller will ignore this annotation and they will
+	// always track jobs using finalizers.
+	JobTrackingFinalizer = labelPrefix + "job-tracking"
+	// LegacyJobName and LegacyControllerUid are legacy labels that were set using unprefixed labels.
+	LegacyJobNameLabel       = "job-name"
+	LegacyControllerUidLabel = "controller-uid"
+	// JobName is a user friendly way to refer to jobs and is set in the labels for jobs.
+	JobNameLabel = labelPrefix + LegacyJobNameLabel
+	// Controller UID is used for selectors and labels for jobs
+	ControllerUidLabel = labelPrefix + LegacyControllerUidLabel
+)
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 

pkg/apis/batch/validation/validation.go

@@ -70,12 +70,12 @@ var (
 		string(v1.ConditionUnknown))
 )
 
-// ValidateGeneratedSelector validates that the generated selector on a controller object match the controller object
+// validateGeneratedSelector validates that the generated selector on a controller object match the controller object
 // metadata, and the labels on the pod template are as generated.
 //
 // TODO: generalize for other controller objects that will follow the same pattern, such as ReplicaSet and DaemonSet, and
 // move to new location.  Replace batch.Job with an interface.
-func ValidateGeneratedSelector(obj *batch.Job) field.ErrorList {
+func validateGeneratedSelector(obj *batch.Job, validateBatchLabels bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if obj.Spec.ManualSelector != nil && *obj.Spec.ManualSelector {
 		return allErrs
@@ -100,12 +100,20 @@ func ValidateGeneratedSelector(obj *batch.Job) field.ErrorList {
 	// have placed certain labels on the pod, but this could have failed if
 	// the user added coflicting labels.  Validate that the expected
 	// generated ones are there.
-
+	allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), batch.LegacyControllerUidLabel, string(obj.UID))...)
-	allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), "controller-uid", string(obj.UID))...)
+	allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), batch.LegacyJobNameLabel, string(obj.Name))...)
-	allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), "job-name", string(obj.Name))...)
 	expectedLabels := make(map[string]string)
-	expectedLabels["controller-uid"] = string(obj.UID)
+	if validateBatchLabels {
-	expectedLabels["job-name"] = string(obj.Name)
+		allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), batch.ControllerUidLabel, string(obj.UID))...)
+		allErrs = append(allErrs, apivalidation.ValidateHasLabel(obj.Spec.Template.ObjectMeta, field.NewPath("spec").Child("template").Child("metadata"), batch.JobNameLabel, string(obj.Name))...)
+		expectedLabels[batch.ControllerUidLabel] = string(obj.UID)
+		expectedLabels[batch.JobNameLabel] = string(obj.Name)
+	}
+	// Labels created by the Kubernetes project should have a Kubernetes prefix.
+	// These labels are set due to legacy reasons.
+
+	expectedLabels[batch.LegacyControllerUidLabel] = string(obj.UID)
+	expectedLabels[batch.LegacyJobNameLabel] = string(obj.Name)
 	// Whether manually or automatically generated, the selector of the job must match the pods it will produce.
 	if selector, err := metav1.LabelSelectorAsSelector(obj.Spec.Selector); err == nil {
 		if !selector.Matches(labels.Set(expectedLabels)) {
@@ -120,7 +128,7 @@ func ValidateGeneratedSelector(obj *batch.Job) field.ErrorList {
 func ValidateJob(job *batch.Job, opts JobValidationOptions) field.ErrorList {
 	// Jobs and rcs have the same name validation
 	allErrs := apivalidation.ValidateObjectMeta(&job.ObjectMeta, true, apivalidation.ValidateReplicationControllerName, field.NewPath("metadata"))
-	allErrs = append(allErrs, ValidateGeneratedSelector(job)...)
+	allErrs = append(allErrs, validateGeneratedSelector(job, opts.RequirePrefixedLabels)...)
 	allErrs = append(allErrs, ValidateJobSpec(&job.Spec, field.NewPath("spec"), opts.PodValidationOptions)...)
 	if !opts.AllowTrackingAnnotation && hasJobTrackingAnnotation(job) {
 		allErrs = append(allErrs, field.Forbidden(field.NewPath("metadata").Child("annotations").Key(batch.JobTrackingFinalizer), "cannot add this annotation"))
@@ -596,4 +604,6 @@ type JobValidationOptions struct {
 	AllowMutableSchedulingDirectives bool
 	// Allow elastic indexed jobs
 	AllowElasticIndexedJobs bool
+	// Require Job to have the label on batch.kubernetes.io/job-name and batch.kubernetes.io/controller-uid
+	RequirePrefixedLabels bool
 }

pkg/registry/batch/job/strategy.go

@@ -170,6 +170,7 @@ func validationOptionsForJob(newJob, oldJob *batch.Job) batchvalidation.JobValid
 		PodValidationOptions:    pod.GetValidationOptionsFromPodTemplate(newPodTemplate, oldPodTemplate),
 		AllowTrackingAnnotation: true,
 		AllowElasticIndexedJobs: utilfeature.DefaultFeatureGate.Enabled(features.ElasticIndexedJob),
+		RequirePrefixedLabels:   true,
 	}
 	if oldJob != nil {
 		opts.AllowInvalidLabelValueInSelector = opts.AllowInvalidLabelValueInSelector || metav1validation.LabelSelectorHasInvalidLabelValue(oldJob.Spec.Selector)
@@ -184,6 +185,12 @@ func validationOptionsForJob(newJob, oldJob *batch.Job) batchvalidation.JobValid
 		suspended := oldJob.Spec.Suspend != nil && *oldJob.Spec.Suspend
 		notStarted := oldJob.Status.StartTime == nil
 		opts.AllowMutableSchedulingDirectives = suspended && notStarted
+
+		// Validation should not fail jobs if they don't have the new labels.
+		// This can be removed once we have high confidence that both labels exist (1.30 at least)
+		_, hadJobName := oldJob.Spec.Template.Labels[batch.JobNameLabel]
+		_, hadControllerUid := oldJob.Spec.Template.Labels[batch.ControllerUidLabel]
+		opts.RequirePrefixedLabels = hadJobName && hadControllerUid
 	}
 	return opts
 }
@@ -209,21 +216,28 @@ func generateSelector(obj *batch.Job) {
 	// The job-name label is unique except in cases that are expected to be
 	// quite uncommon, and is more user friendly than uid.  So, we add it as
 	// a label.
-	_, found := obj.Spec.Template.Labels["job-name"]
+	jobNameLabels := []string{batch.LegacyJobNameLabel, batch.JobNameLabel}
-	if found {
+	for _, value := range jobNameLabels {
-		// User asked us to automatically generate a selector, but set manual labels.
+		_, found := obj.Spec.Template.Labels[value]
-		// If there is a conflict, we will reject in validation.
+		if found {
-	} else {
+			// User asked us to automatically generate a selector, but set manual labels.
-		obj.Spec.Template.Labels["job-name"] = string(obj.ObjectMeta.Name)
+			// If there is a conflict, we will reject in validation.
+		} else {
+			obj.Spec.Template.Labels[value] = string(obj.ObjectMeta.Name)
+		}
 	}
+
 	// The controller-uid label makes the pods that belong to this job
 	// only match this job.
-	_, found = obj.Spec.Template.Labels["controller-uid"]
+	controllerUidLabels := []string{batch.LegacyControllerUidLabel, batch.ControllerUidLabel}
-	if found {
+	for _, value := range controllerUidLabels {
-		// User asked us to automatically generate a selector, but set manual labels.
+		_, found := obj.Spec.Template.Labels[value]
-		// If there is a conflict, we will reject in validation.
+		if found {
-	} else {
+			// User asked us to automatically generate a selector, but set manual labels.
-		obj.Spec.Template.Labels["controller-uid"] = string(obj.ObjectMeta.UID)
+			// If there is a conflict, we will reject in validation.
+		} else {
+			obj.Spec.Template.Labels[value] = string(obj.ObjectMeta.UID)
+		}
 	}
 	// Select the controller-uid label.  This is sufficient for uniqueness.
 	if obj.Spec.Selector == nil {
@@ -232,8 +246,9 @@ func generateSelector(obj *batch.Job) {
 	if obj.Spec.Selector.MatchLabels == nil {
 		obj.Spec.Selector.MatchLabels = make(map[string]string)
 	}
-	if _, found := obj.Spec.Selector.MatchLabels["controller-uid"]; !found {
+
-		obj.Spec.Selector.MatchLabels["controller-uid"] = string(obj.ObjectMeta.UID)
+	if _, found := obj.Spec.Selector.MatchLabels[batch.ControllerUidLabel]; !found {
+		obj.Spec.Selector.MatchLabels[batch.ControllerUidLabel] = string(obj.ObjectMeta.UID)
 	}
 	// If the user specified matchLabel controller-uid=$WRONGUID, then it should fail
 	// in validation, either because the selector does not match the pod template

pkg/registry/batch/job/strategy_test.go

@@ -670,6 +670,66 @@ func TestJobStrategy_ValidateUpdate(t *testing.T) {
 				job.Annotations["hello"] = "world"
 			},
 		},
+		"old job has no batch.kubernetes.io labels": {
+			job: &batch.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "myjob",
+					UID:             "test",
+					Namespace:       metav1.NamespaceDefault,
+					ResourceVersion: "10",
+					Annotations:     map[string]string{"hello": "world"},
+				},
+				Spec: batch.JobSpec{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{batch.LegacyControllerUidLabel: "test"},
+					},
+					Parallelism: pointer.Int32(4),
+					Template: api.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{batch.LegacyJobNameLabel: "myjob", batch.LegacyControllerUidLabel: "test"},
+						},
+						Spec: api.PodSpec{
+							RestartPolicy: api.RestartPolicyOnFailure,
+							DNSPolicy:     api.DNSClusterFirst,
+							Containers:    []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}},
+						},
+					},
+				},
+			},
+			update: func(job *batch.Job) {
+				job.Annotations["hello"] = "world"
+			},
+		},
+		"old job has all labels": {
+			job: &batch.Job{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "myjob",
+					UID:             "test",
+					Namespace:       metav1.NamespaceDefault,
+					ResourceVersion: "10",
+					Annotations:     map[string]string{"foo": "bar"},
+				},
+				Spec: batch.JobSpec{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{batch.ControllerUidLabel: "test"},
+					},
+					Parallelism: pointer.Int32(4),
+					Template: api.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{batch.LegacyJobNameLabel: "myjob", batch.JobNameLabel: "myjob", batch.LegacyControllerUidLabel: "test", batch.ControllerUidLabel: "test"},
+						},
+						Spec: api.PodSpec{
+							RestartPolicy: api.RestartPolicyOnFailure,
+							DNSPolicy:     api.DNSClusterFirst,
+							Containers:    []api.Container{{Name: "abc", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: api.TerminationMessageReadFile}},
+						},
+					},
+				},
+			},
+			update: func(job *batch.Job) {
+				job.Annotations["hello"] = "world"
+			},
+		},
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
@@ -872,7 +932,8 @@ func TestJobStrategy_Validate(t *testing.T) {
 	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)}
 	validPodSpec := api.PodSpec{
 		RestartPolicy: api.RestartPolicyOnFailure,
 		DNSPolicy:     api.DNSClusterFirst,
@@ -903,7 +964,7 @@ func TestJobStrategy_Validate(t *testing.T) {
 			wantJob: &batch.Job{
 				ObjectMeta: validObjectMeta,
 				Spec: batch.JobSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"controller-uid": string(theUID)}},
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}},
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
 							Labels: validSelector.MatchLabels,
@@ -924,10 +985,10 @@ func TestJobStrategy_Validate(t *testing.T) {
 			wantJob: &batch.Job{
 				ObjectMeta: validObjectMeta,
 				Spec: batch.JobSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"controller-uid": string(theUID)}},
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}},
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"job-name": "myjob2", "controller-uid": string(theUID)},
+							Labels: validLabels,
 						},
 						Spec: validPodSpec,
 					}},
@@ -940,7 +1001,7 @@ func TestJobStrategy_Validate(t *testing.T) {
 					Selector: nil,
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"a": "b", "job-name": "myjob2", "controller-uid": string(theUID)},
+							Labels: labelsWithNonBatch,
 						},
 						Spec: validPodSpec,
 					}},
@@ -948,10 +1009,10 @@ func TestJobStrategy_Validate(t *testing.T) {
 			wantJob: &batch.Job{
 				ObjectMeta: validObjectMeta,
 				Spec: batch.JobSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"controller-uid": string(theUID)}},
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}},
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"a": "b", "job-name": "myjob2", "controller-uid": string(theUID)},
+							Labels: labelsWithNonBatch,
 						},
 						Spec: validPodSpec,
 					}},
@@ -1007,10 +1068,10 @@ func TestJobStrategy_Validate(t *testing.T) {
 			wantJob: &batch.Job{
 				ObjectMeta: validObjectMeta,
 				Spec: batch.JobSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"controller-uid": string(theUID)}},
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}},
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"a": "b", "job-name": "myjob2", "controller-uid": string(theUID)},
+							Labels: labelsWithNonBatch,
 						},
 						Spec: validPodSpec,
 					},
@@ -1042,10 +1103,10 @@ func TestJobStrategy_Validate(t *testing.T) {
 			wantJob: &batch.Job{
 				ObjectMeta: validObjectMeta,
 				Spec: batch.JobSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"controller-uid": string(theUID)}},
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{batch.ControllerUidLabel: string(theUID)}},
 					Template: api.PodTemplateSpec{
 						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"a": "b", "job-name": "myjob2", "controller-uid": string(theUID)},
+							Labels: labelsWithNonBatch,
 						},
 						Spec: api.PodSpec{
 							RestartPolicy: api.RestartPolicyOnFailure,

staging/src/k8s.io/api/batch/v1/types.go

@@ -23,8 +23,11 @@ import (
 )
 
 const (
-	JobCompletionIndexAnnotation = "batch.kubernetes.io/job-completion-index"
+	// All Kubernetes labels need to be prefixed with Kubernetes to distinguish them from end-user labels
+	// More info: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions
+	labelPrefix = "batch.kubernetes.io/"
 
+	JobCompletionIndexAnnotation = labelPrefix + "job-completion-index"
 	// JobTrackingFinalizer is a finalizer for Job's pods. It prevents them from
 	// being deleted before being accounted in the Job status.
 	//
@@ -34,7 +37,14 @@ const (
 	// 1.27+, one release after JobTrackingWithFinalizers graduates to GA, the
 	// apiserver and job controller will ignore this annotation and they will
 	// always track jobs using finalizers.
-	JobTrackingFinalizer = "batch.kubernetes.io/job-tracking"
+	JobTrackingFinalizer = labelPrefix + "job-tracking"
+	// The Job labels will use batch.kubernetes.io as a prefix for all labels
+	// Historically the job controller uses unprefixed labels for job-name and controller-uid and
+	// Kubernetes continutes to recognize those unprefixed labels for consistency.
+	JobNameLabel = labelPrefix + "job-name"
+	// ControllerUid is used to programatically get pods corresponding to a Job.
+	// There is a corresponding label without the batch.kubernetes.io that we support for legacy reasons.
+	ControllerUidLabel = labelPrefix + "controller-uid"
 )
 
 // +genclient