From 7d3bb891541ad553dbd7f66be17f2e9434445e39 Mon Sep 17 00:00:00 2001 From: Michal Szadkowski Date: Thu, 12 Sep 2024 11:31:46 +0200 Subject: [PATCH] Update job_test and logging Signed-off-by: Michal Szadkowski --- pkg/common/util/webhooks.go | 2 +- pkg/controller.v1/common/job_test.go | 24 ++++++++++++++----- pkg/controller.v1/mpi/mpijob_controller.go | 2 +- .../paddlepaddle/paddlepaddle_controller.go | 2 +- .../pytorch/pytorchjob_controller.go | 2 +- .../tensorflow/tfjob_controller.go | 2 +- .../xgboost/xgboostjob_controller.go | 2 +- 7 files changed, 24 insertions(+), 12 deletions(-) diff --git a/pkg/common/util/webhooks.go b/pkg/common/util/webhooks.go index 65511531ba..ea3529d009 100644 --- 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 index e604c715f0..cdd34fdb19 100644 --- 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 index a3c1859108..10682d0981 100644 --- 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 index a9884d4442..1f3cf2230f 100644 --- 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 index f8f2f4d0b5..9af3e4527d 100644 --- 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 index 3095d7a57a..b14c33f36d 100644 --- 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 index 3b051ddc7d..da3889ce02 100644 --- 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 }