Skip to content

Commit

Permalink
inject AppWrapper finalizer in WebHook (#228)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dgrove-oss authored Aug 13, 2024
1 parent 4f0f9b7 commit 96563f6
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
7 changes: 5 additions & 2 deletions internal/controller/appwrapper/appwrapper_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions internal/webhook/appwrapper_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@ 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"

"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"
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit 96563f6

Please sign in to comment.