bug fix in managedBy logic (#307)

defaulting webhooks can run in any order, therefore we
can't rely using the value of managedBy seen by our defaulter
to figure out whether or not we should add the appwrapper finalizer.

To reduce reconciliation conflicts with Kueue, defer adding the
finalizer until we are taking the Suspended to Resuming transition
(which will only happen after Kueue's workload controller successfully
updates Spec.Suspend).

Author: dgrove-oss

internal/controller/appwrapper/appwrapper_controller.go

@@ -40,11 +40,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
-	"sigs.k8s.io/kueue/pkg/controller/jobframework"
-	utilmaps "sigs.k8s.io/kueue/pkg/util/maps"
 
 	workloadv1beta2 "github.com/project-codeflare/appwrapper/api/v1beta2"
-	wlc "github.com/project-codeflare/appwrapper/internal/controller/workload"
 	"github.com/project-codeflare/appwrapper/internal/metrics"
 	"github.com/project-codeflare/appwrapper/pkg/config"
 	"github.com/project-codeflare/appwrapper/pkg/utils"
@@ -162,30 +159,6 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	switch aw.Status.Phase {
 
 	case workloadv1beta2.AppWrapperEmpty: // initial state
-		if !controllerutil.ContainsFinalizer(aw, AppWrapperFinalizer) {
-			// The AppWrapperFinalizer is added by our webhook, so if we get here it means that we are
-			// running in dev mode (`make run`) which disables the webhook. To make dev mode as
-			// useful as possible, replicate as much of AppWrapperWebhook.Default() as we can without having the admission.Request.
-			if r.Config.EnableKueueIntegrations {
-				if r.Config.DefaultQueueName != "" {
-					aw.Labels = utilmaps.MergeKeepFirst(aw.Labels, map[string]string{"kueue.x-k8s.io/queue-name": r.Config.DefaultQueueName})
-				}
-				nsSelector, err := metav1.LabelSelectorAsSelector(r.Config.KueueJobReconciller.ManageJobsNamespaceSelector)
-				if err != nil {
-					return ctrl.Result{}, err
-				}
-				err = jobframework.ApplyDefaultForSuspend(ctx, (*wlc.AppWrapper)(aw), r.Client, r.Config.KueueJobReconciller.ManageJobsWithoutQueueName, nsSelector)
-				if err != nil {
-					return ctrl.Result{}, err
-				}
-			}
-			controllerutil.AddFinalizer(aw, AppWrapperFinalizer)
-			if err := r.Update(ctx, aw); err != nil {
-				return ctrl.Result{}, err
-			}
-			log.FromContext(ctx).Info("No webhook: applied default initializations")
-		}
-
 		orig := copyForStatusPatch(aw)
 		if err := utils.EnsureComponentStatusInitialized(aw); err != nil {
 			return ctrl.Result{}, err
@@ -198,6 +171,14 @@ func (r *AppWrapperReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			return ctrl.Result{}, nil // remain suspended
 		}
 
+		// ensure our finalizer is present before we deploy any resources
+		if controllerutil.AddFinalizer(aw, AppWrapperFinalizer) {
+			if err := r.Update(ctx, aw); err != nil {
+				return ctrl.Result{}, err
+			}
+			log.FromContext(ctx).Info("Finalizer Added")
+		}
+
 		// begin deployment
 		orig := copyForStatusPatch(aw)
 		meta.SetStatusCondition(&aw.Status.Conditions, metav1.Condition{

internal/controller/appwrapper/appwrapper_controller_test.go

@@ -81,7 +81,6 @@ var _ = Describe("AppWrapper Controller", func() {
 
 		aw = getAppWrapper(awName)
 		Expect(aw.Status.Phase).Should(Equal(workloadv1beta2.AppWrapperSuspended))
-		Expect(controllerutil.ContainsFinalizer(aw, AppWrapperFinalizer)).Should(BeTrue())
 
 		By("Updating aw.Spec by invoking RunWithPodSetsInfo")
 		Expect((*workload.AppWrapper)(aw).RunWithPodSetsInfo([]podset.PodSetInfo{markerPodSet, markerPodSet})).To(Succeed())
@@ -94,6 +93,7 @@ var _ = Describe("AppWrapper Controller", func() {
 
 		aw = getAppWrapper(awName)
 		Expect(aw.Status.Phase).Should(Equal(workloadv1beta2.AppWrapperResuming))
+		Expect(controllerutil.ContainsFinalizer(aw, AppWrapperFinalizer)).Should(BeTrue())
 		Expect(meta.IsStatusConditionTrue(aw.Status.Conditions, string(workloadv1beta2.ResourcesDeployed))).Should(BeTrue())
 		Expect(meta.IsStatusConditionTrue(aw.Status.Conditions, string(workloadv1beta2.QuotaReserved))).Should(BeTrue())
 		Expect((*workload.AppWrapper)(aw).IsActive()).Should(BeTrue())

internal/webhook/appwrapper_webhook.go

@@ -35,15 +35,13 @@ import (
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"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"
@@ -105,14 +103,6 @@ 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)
 
-	// do not inject finalizer if managed by another controller
-	if aw.Spec.ManagedBy != nil && *aw.Spec.ManagedBy != workloadv1beta2.AppWrapperControllerName {
-		return nil
-	}
-
-	// 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
 }