Update job_test and logging

kubeflow · Sep 12, 2024 · a822a17 · a822a17
1 parent de5d02b
commit a822a17
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 12 deletions.
diff --git a/pkg/common/util/webhooks.go b/pkg/common/util/webhooks.go
@@ -14,8 +14,8 @@ var supportedJobControllers = sets.New(
 func ValidateManagedBy(runPolicy *v1.RunPolicy, allErrs field.ErrorList) field.ErrorList {
 	if runPolicy.ManagedBy != nil {
 		manager := *runPolicy.ManagedBy
-		fieldPath := field.NewPath("spec", "managedBy")
 		if !supportedJobControllers.Has(manager) {
+			fieldPath := field.NewPath("spec", "managedBy")
 			allErrs = append(allErrs, field.NotSupported(fieldPath, manager, supportedJobControllers.UnsortedList()))
 		}
 	}

diff --git a/pkg/controller.v1/common/job_test.go b/pkg/controller.v1/common/job_test.go
@@ -220,21 +220,30 @@ func TestPastActiveDeadline(T *testing.T) {
 	}
 }
 
-func TestManagedBy(T *testing.T) {
+func TestManagedByExternalController(T *testing.T) {
 	cases := map[string]struct {
-		managedBy *string
+		managedBy  *string
+		wantResult bool
 	}{
+		"managedBy is nil": {
+			managedBy:  nil,
+			wantResult: false,
+		},
 		"managedBy is empty": {
-			managedBy: ptr.To[string](""),
+			managedBy:  ptr.To[string](""),
+			wantResult: true,
 		},
 		"managedBy is training-operator controller": {
-			managedBy: ptr.To[string](apiv1.KubeflowJobsController),
+			managedBy:  ptr.To[string](apiv1.KubeflowJobsController),
+			wantResult: false,
 		},
 		"managedBy is not the training-operator controller": {
-			managedBy: ptr.To[string]("kueue.x-k8s.io/multikueue"),
+			managedBy:  ptr.To[string]("kueue.x-k8s.io/multikueue"),
+			wantResult: true,
 		},
 		"managedBy is other value": {
-			managedBy: ptr.To[string]("other-job-controller"),
+			managedBy:  ptr.To[string]("other-job-controller"),
+			wantResult: true,
 		},
 	}
 	for name, tc := range cases {
@@ -245,6 +254,9 @@ func TestManagedBy(T *testing.T) {
 			}
 
 			if got := jobController.ManagedByExternalController(*runPolicy); got != nil {
+				if !tc.wantResult {
+					t.Errorf("Unwanted manager controller: %s\n", *got)
+				}
 				if diff := cmp.Diff(tc.managedBy, got); diff != "" {
 					t.Errorf("Unexpected manager controller (-want +got):\n%s", diff)
 				}

diff --git a/pkg/controller.v1/mpi/mpijob_controller.go b/pkg/controller.v1/mpi/mpijob_controller.go
@@ -136,7 +136,7 @@ func (jc *MPIJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	}
 
 	if manager := jc.ManagedByExternalController(mpijob.Spec.RunPolicy); manager != nil {
-		logger.Info("Skipping MPIJob managed by a different controller", "managed-by", manager)
+		logger.Info("Skipping MPIJob managed by a custom controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}
 

diff --git a/pkg/controller.v1/paddlepaddle/paddlepaddle_controller.go b/pkg/controller.v1/paddlepaddle/paddlepaddle_controller.go
@@ -132,7 +132,7 @@ func (r *PaddleJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	if manager := r.ManagedByExternalController(paddlejob.Spec.RunPolicy); manager != nil {
-		logger.Info("Skipping PaddleJob managed by a different controller", "managed-by", manager)
+		logger.Info("Skipping PaddleJob managed by a custom controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}
 

diff --git a/pkg/controller.v1/pytorch/pytorchjob_controller.go b/pkg/controller.v1/pytorch/pytorchjob_controller.go
@@ -133,7 +133,7 @@ func (r *PyTorchJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	if manager := r.ManagedByExternalController(pytorchjob.Spec.RunPolicy); manager != nil {
-		logger.Info("Skipping PyTorchJob managed by a different controller", "managed-by", manager)
+		logger.Info("Skipping PyTorchJob managed by a custom controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}
 

diff --git a/pkg/controller.v1/tensorflow/tfjob_controller.go b/pkg/controller.v1/tensorflow/tfjob_controller.go
@@ -128,7 +128,7 @@ func (r *TFJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	}
 
 	if manager := r.ManagedByExternalController(tfjob.Spec.RunPolicy); manager != nil {
-		logger.Info("Skipping TFJob managed by a different controller", "managed-by", manager)
+		logger.Info("Skipping TFJob managed by a custom controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}
 

diff --git a/pkg/controller.v1/xgboost/xgboostjob_controller.go b/pkg/controller.v1/xgboost/xgboostjob_controller.go
@@ -131,7 +131,7 @@ func (r *XGBoostJobReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	if manager := r.ManagedByExternalController(xgboostjob.Spec.RunPolicy); manager != nil {
-		logger.Info("Skipping XGBoostJob managed by a different controller", "managed-by", manager)
+		logger.Info("Skipping XGBoostJob managed by a custom controller", "managed-by", manager)
 		return ctrl.Result{}, nil
 	}