diff --git a/internal/controller/appwrapper/appwrapper_controller.go b/internal/controller/appwrapper/appwrapper_controller.go index 7e3d005..2256f04 100644 --- a/internal/controller/appwrapper/appwrapper_controller.go +++ b/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{ diff --git a/internal/controller/appwrapper/appwrapper_controller_test.go b/internal/controller/appwrapper/appwrapper_controller_test.go index 77bafad..d16a1da 100644 --- a/internal/controller/appwrapper/appwrapper_controller_test.go +++ b/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()) diff --git a/internal/webhook/appwrapper_webhook.go b/internal/webhook/appwrapper_webhook.go index 348cfb4..abcc0a3 100644 --- a/internal/webhook/appwrapper_webhook.go +++ b/internal/webhook/appwrapper_webhook.go @@ -35,7 +35,6 @@ 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" @@ -43,7 +42,6 @@ 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" @@ -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 }