From d2ddd983e546e10f827b932c23c36c990abca452 Mon Sep 17 00:00:00 2001 From: David Grove Date: Tue, 13 Aug 2024 16:56:07 -0400 Subject: [PATCH] inject AppWrapper finalizer in WebHook Avoid a needless reconciler race between the AppWrapper and Workload controllers when Kueue unsuspends the AppWrapper by updating its Spec "at the same time as" the AppWrapper controller was injecting the finalizer into the metadata. --- internal/controller/appwrapper/appwrapper_controller.go | 7 +++++-- internal/webhook/appwrapper_webhook.go | 5 +++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/controller/appwrapper/appwrapper_controller.go b/internal/controller/appwrapper/appwrapper_controller.go index 6dbd24aa..7c7eb505 100644 --- a/internal/controller/appwrapper/appwrapper_controller.go +++ b/internal/controller/appwrapper/appwrapper_controller.go @@ -154,11 +154,14 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request) switch aw.Status.Phase { - case workloadv1beta2.AppWrapperEmpty: // initial state, inject finalizer - if controllerutil.AddFinalizer(aw, AppWrapperFinalizer) { + case workloadv1beta2.AppWrapperEmpty: // initial state + if !controllerutil.ContainsFinalizer(aw, AppWrapperFinalizer) { + // Belt and Suspenders: should never get here because finalizer is injected by Webhook + controllerutil.AddFinalizer(aw, AppWrapperFinalizer) if err := r.Update(ctx, aw); err != nil { return ctrl.Result{}, err } + log.FromContext(ctx).Info("Finalizer Added By Operator!") } orig := copyForStatusPatch(aw) diff --git a/internal/webhook/appwrapper_webhook.go b/internal/webhook/appwrapper_webhook.go index 36ac3b1c..d281d48a 100644 --- a/internal/webhook/appwrapper_webhook.go +++ b/internal/webhook/appwrapper_webhook.go @@ -33,6 +33,7 @@ import ( utilmaps "sigs.k8s.io/kueue/pkg/util/maps" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -40,6 +41,7 @@ import ( "sigs.k8s.io/kueue/pkg/controller/jobframework" workloadv1beta2 "github.com/project-codeflare/appwrapper/api/v1beta2" + awc "github.com/project-codeflare/appwrapper/internal/controller/appwrapper" wlc "github.com/project-codeflare/appwrapper/internal/controller/workload" "github.com/project-codeflare/appwrapper/pkg/config" "github.com/project-codeflare/appwrapper/pkg/utils" @@ -87,6 +89,9 @@ func (w *AppWrapperWebhook) Default(ctx context.Context, obj runtime.Object) err username := utils.SanitizeLabel(userInfo.Username) aw.Labels = utilmaps.MergeKeepFirst(map[string]string{AppWrapperUsernameLabel: username, AppWrapperUserIDLabel: userInfo.UID}, aw.Labels) + // inject finalizer now (avoid reconcilier errors between the AppWrapper and WorkloadControllers when it is admitted by a ClusterQueue almost immediately) + controllerutil.AddFinalizer(aw, awc.AppWrapperFinalizer) + return nil }