Change implementation of JobSet controller manager to better align wi…

…th how Job is implemented.
kubernetes-sigs · Mar 29, 2024 · 66b55c4 · 66b55c4
1 parent a82e440
commit 66b55c4
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 24 deletions.
diff --git a/api/jobset/v1alpha2/jobset_types.go b/api/jobset/v1alpha2/jobset_types.go
@@ -40,9 +40,9 @@ const (
 	NamespacedJobKey        string = "alpha.jobset.sigs.k8s.io/namespaced-job"
 	NoScheduleTaintKey      string = "alpha.jobset.sigs.k8s.io/no-schedule"
 
-	// JobSetManager is used as the value for ManagedBy to identify the jobset controller manager
-	// as the manager of a specific JobSet.
-	JobSetManager = "jobset"
+	// JobSetControllerName is the reserved value for the managedBy field for the built-in
+	// JobSet controller.
+	JobSetControllerName = "jobset.sigs.k8s.io/jobset-controller"
 )
 
 type JobSetConditionType string

diff --git a/api/jobset/v1alpha2/jobset_webhook.go b/api/jobset/v1alpha2/jobset_webhook.go
@@ -94,7 +94,7 @@ func (js *JobSet) Default() {
 	}
 
 	if js.Spec.ManagedBy == nil {
-		js.Spec.ManagedBy = ptr.To(JobSetManager)
+		js.Spec.ManagedBy = ptr.To(JobSetControllerName)
 	}
 }
 

diff --git a/api/jobset/v1alpha2/jobset_webhook_test.go b/api/jobset/v1alpha2/jobset_webhook_test.go
@@ -52,7 +52,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -70,7 +70,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -90,7 +90,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -108,7 +108,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -128,7 +128,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -146,7 +146,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -167,7 +167,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -185,7 +185,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -208,7 +208,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -230,7 +230,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -255,7 +255,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -277,7 +277,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -301,7 +301,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -323,7 +323,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -350,7 +350,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 			want: &JobSet{
@@ -374,7 +374,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -428,7 +428,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},
@@ -465,7 +465,7 @@ func TestJobSetDefaulting(t *testing.T) {
 							},
 						},
 					},
-					ManagedBy: ptr.To(JobSetManager),
+					ManagedBy: ptr.To(JobSetControllerName),
 				},
 			},
 		},

diff --git a/pkg/controllers/jobset_controller.go b/pkg/controllers/jobset_controller.go
@@ -86,7 +86,7 @@ func (r *JobSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	log := ctrl.LoggerFrom(ctx).WithValues("jobset", klog.KObj(&js))
 	ctx = ctrl.LoggerInto(ctx, log)
 
-	if manager := ptr.Deref(js.Spec.ManagedBy, ""); js.Spec.ManagedBy != nil && manager != jobset.JobSetManager {
+	if manager := managedByExternalController(js); manager != nil {
 		log.V(5).Info("Skipping JobSet managed by a different controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}
@@ -936,3 +936,10 @@ func findJobFailureTime(job *batchv1.Job) *metav1.Time {
 	}
 	return nil
 }
+
+func managedByExternalController(js jobset.JobSet) *string {
+	if controllerName := js.Spec.ManagedBy; controllerName != nil && *controllerName != jobset.JobSetControllerName {
+		return controllerName
+	}
+	return nil
+}
diff --git a/test/integration/webhook/jobset_webhook_test.go b/test/integration/webhook/jobset_webhook_test.go
@@ -347,7 +347,7 @@ var _ = ginkgo.Describe("jobset webhook defaulting", func() {
 						Obj())
 			},
 			defaultsApplied: func(js *jobset.JobSet) bool {
-				return ptr.Deref(js.Spec.ManagedBy, "") == jobset.JobSetManager
+				return ptr.Deref(js.Spec.ManagedBy, "") == jobset.JobSetControllerName
 			},
 			updateJobSet: func(js *jobset.JobSet) {
 				js.Spec.ManagedBy = ptr.To("new-manager")
-Original file line number
+Diff line change
@@ Expand Up / @@ -94,7 +94,7 @@ func (js *JobSet) Default() { @@
     	}
     	if js.Spec.ManagedBy == nil {
-    		js.Spec.ManagedBy = ptr.To(JobSetManager)
+    		js.Spec.ManagedBy = ptr.To(JobSetControllerName)
     	}
     }
@@ Expand Down @@