address comments & update implementation

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

Author: britaniar

cmd/memberagent/main.go

@@ -91,6 +91,7 @@ var (
 	driftDetectionInterval       = flag.Int("drift-detection-interval", 15, "The interval in seconds between attempts to detect configuration drifts in the cluster.")
 	watchWorkWithPriorityQueue   = flag.Bool("enable-watch-work-with-priority-queue", false, "If set, the apply_work controller will watch/reconcile work objects that are created new or have recent updates")
 	watchWorkReconcileAgeMinutes = flag.Int("watch-work-reconcile-age", 60, "maximum age (in minutes) of work objects for apply_work controller to watch/reconcile")
+	deletionWaitTime             = flag.Int("deletion-wait-time", 5, "The time the work-applier will wait for work object to be deleted before updating the applied work owner reference")
 	enablePprof                  = flag.Bool("enable-pprof", false, "enable pprof profiling")
 	pprofPort                    = flag.Int("pprof-port", 6065, "port for pprof profiling")
 	hubPprofPort                 = flag.Int("hub-pprof-port", 6066, "port for hub pprof profiling")
@@ -395,6 +396,7 @@ func Start(ctx context.Context, hubCfg, memberConfig *rest.Config, hubOpts, memb
 			parallelizer.DefaultNumOfWorkers,
 			time.Second*time.Duration(*availabilityCheckInterval),
 			time.Second*time.Duration(*driftDetectionInterval),
+			time.Minute*time.Duration(*deletionWaitTime),
 			*watchWorkWithPriorityQueue,
 			*watchWorkReconcileAgeMinutes,
 		)

pkg/controllers/internalmembercluster/v1beta1/member_suite_test.go

@@ -379,7 +379,7 @@ var _ = BeforeSuite(func() {
 
 	// This controller is created for testing purposes only; no reconciliation loop is actually
 	// run.
-	workApplier1 = workapplier.NewReconciler(hubClient, member1ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, true, 60)
+	workApplier1 = workapplier.NewReconciler(hubClient, member1ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, time.Minute, true, 60)
 
 	propertyProvider1 = &manuallyUpdatedProvider{}
 	member1Reconciler, err := NewReconciler(ctx, hubClient, member1Cfg, member1Client, workApplier1, propertyProvider1)
@@ -402,7 +402,7 @@ var _ = BeforeSuite(func() {
 
 	// This controller is created for testing purposes only; no reconciliation loop is actually
 	// run.
-	workApplier2 = workapplier.NewReconciler(hubClient, member2ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, true, 60)
+	workApplier2 = workapplier.NewReconciler(hubClient, member2ReservedNSName, nil, nil, nil, nil, 0, 1, time.Second*5, time.Second*5, time.Minute, true, 60)
 
 	member2Reconciler, err := NewReconciler(ctx, hubClient, member2Cfg, member2Client, workApplier2, nil)
 	Expect(err).NotTo(HaveOccurred())

pkg/controllers/workapplier/apply.go

@@ -517,10 +517,7 @@ func validateOwnerReferences(
 	// expected AppliedWork object. For safety reasons, Fleet will still do a sanity check.
 	found := false
 	for _, ownerRef := range inMemberClusterObjOwnerRefs {
-		if ownerRef.UID == expectedAppliedWorkOwnerRef.UID &&
-			ownerRef.Name == expectedAppliedWorkOwnerRef.Name &&
-			ownerRef.Kind == expectedAppliedWorkOwnerRef.Kind &&
-			ownerRef.APIVersion == expectedAppliedWorkOwnerRef.APIVersion {
+		if ownerRefEqualsExpected(&ownerRef, expectedAppliedWorkOwnerRef) {
 			found = true
 			break
 		}

pkg/controllers/workapplier/controller.go

@@ -197,6 +197,7 @@ type Reconciler struct {
 
 	availabilityCheckRequeueAfter time.Duration
 	driftCheckRequeueAfter        time.Duration
+	deletionWaitTime              time.Duration
 }
 
 func NewReconciler(
@@ -207,6 +208,7 @@ func NewReconciler(
 	workerCount int,
 	availabilityCheckRequestAfter time.Duration,
 	driftCheckRequestAfter time.Duration,
+	deletionWaitTime time.Duration,
 	watchWorkWithPriorityQueue bool,
 	watchWorkReconcileAgeMinutes int,
 ) *Reconciler {
@@ -236,6 +238,7 @@ func NewReconciler(
 		joined:                        atomic.NewBool(false),
 		availabilityCheckRequeueAfter: acRequestAfter,
 		driftCheckRequeueAfter:        dcRequestAfter,
+		deletionWaitTime:              deletionWaitTime,
 	}
 }
 
@@ -470,74 +473,78 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	return ctrl.Result{RequeueAfter: r.driftCheckRequeueAfter}, nil
 }
 
-// garbageCollectAppliedWork deletes the appliedWork and all the manifests associated with it from the cluster.
 func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv1beta1.Work) (ctrl.Result, error) {
 	deletePolicy := metav1.DeletePropagationForeground
 	if !controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) {
 		return ctrl.Result{}, nil
 	}
-	// Delete the appliedWork which will remove all the manifests associated with it, unless they have
-	// other owner references.
-	appliedWork := fleetv1beta1.AppliedWork{
+	appliedWork := &fleetv1beta1.AppliedWork{
 		ObjectMeta: metav1.ObjectMeta{Name: work.Name},
 	}
 	// Get the AppliedWork object
-	if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: work.Name}, &appliedWork); err != nil {
+	if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: work.Name}, appliedWork); err != nil {
 		if apierrors.IsNotFound(err) {
 			klog.V(2).InfoS("The appliedWork is already deleted, removing the finalizer from the work", "appliedWork", work.Name)
 			return r.removeWorkFinalizer(ctx, work)
 		}
 		klog.ErrorS(err, "Failed to get AppliedWork", "appliedWork", work.Name)
-		return ctrl.Result{Requeue: true}, controller.NewAPIServerError(false, err)
+		return ctrl.Result{}, controller.NewAPIServerError(false, err)
 	}
 
-	// Delete the appliedWork object.
-	if appliedWork.DeletionTimestamp.IsZero() {
-		// Update the owner reference in the AppliedWork object.
-		if err := r.updateOwnerReference(ctx, &appliedWork); err != nil {
-			klog.ErrorS(err, "Failed to update owner reference in the appliedWork", "appliedWork", work.Name)
+	// Handle stuck deletion after 5 minutes where the other owner references might not exist or are invalid.
+	if !appliedWork.DeletionTimestamp.IsZero() && time.Since(appliedWork.DeletionTimestamp.Time) >= r.deletionWaitTime {
+		klog.V(2).InfoS("AppliedWork deletion appears stuck; attempting to patch owner references", "appliedWork", work.Name)
+		if err := r.updateOwnerReference(ctx, work, appliedWork); err != nil {
+			klog.ErrorS(err, "Failed to update owner references for AppliedWork", "appliedWork", work.Name)
 			return ctrl.Result{}, controller.NewAPIServerError(false, err)
 		}
-		klog.V(2).InfoS("Deleting the appliedWork", "appliedWork", work.Name)
-		if err := r.spokeClient.Delete(ctx, &appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy}); err != nil {
-			if apierrors.IsNotFound(err) {
-				klog.V(2).InfoS("The appliedWork is already deleted", "appliedWork", work.Name)
-				return r.removeWorkFinalizer(ctx, work)
-			}
-			klog.V(2).ErrorS(err, "Failed to delete the appliedWork", "appliedWork", work.Name)
-			return ctrl.Result{}, controller.NewAPIServerError(false, err)
+	}
+
+	if err := r.spokeClient.Delete(ctx, appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy}); err != nil {
+		if apierrors.IsNotFound(err) {
+			klog.V(2).InfoS("AppliedWork already deleted", "appliedWork", work.Name)
+			return r.removeWorkFinalizer(ctx, work)
 		}
+		klog.V(2).ErrorS(err, "Failed to delete the appliedWork", "appliedWork", work.Name)
+		return ctrl.Result{}, controller.NewAPIServerError(false, err)
 	}
+
 	klog.V(2).InfoS("AppliedWork deletion in progress", "appliedWork", work.Name)
-	return ctrl.Result{Requeue: true}, fmt.Errorf("AppliedWork %s is being deleted, waiting for the deletion to complete", work.Name)
+	return ctrl.Result{}, fmt.Errorf("AppliedWork %s is being deleted, waiting for the deletion to complete", work.Name)
 }
 
 // updateOwnerReference updates the AppliedWork owner reference in the manifest objects.
-// It changes the `blockOwnerDeletion` field to true, so that the AppliedWork can be deleted in cases where
+// It changes the blockOwnerDeletion field to false, so that the AppliedWork can be deleted in cases where
 // the other owner references do not exist or are invalid.
 // https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/#owner-references-in-object-specifications
-func (r *Reconciler) updateOwnerReference(ctx context.Context, appliedWork *fleetv1beta1.AppliedWork) error {
+func (r *Reconciler) updateOwnerReference(ctx context.Context, work *fleetv1beta1.Work, appliedWork *fleetv1beta1.AppliedWork) error {
 	appliedWorkOwnerRef := &metav1.OwnerReference{
 		APIVersion: fleetv1beta1.GroupVersion.String(),
 		Kind:       "AppliedWork",
 		Name:       appliedWork.Name,
 		UID:        appliedWork.UID,
 	}
 
-	for _, res := range appliedWork.Status.AppliedResources {
+	if err := r.hubClient.Get(ctx, types.NamespacedName{Name: work.Name, Namespace: work.Namespace}, work); err != nil {
+		if apierrors.IsNotFound(err) {
+			klog.V(2).InfoS("Work object not found, skipping owner reference update", "work", work.Name, "namespace", work.Namespace)
+			return nil
+		}
+		klog.ErrorS(err, "Failed to get Work object for owner reference update", "work", work.Name, "namespace", work.Namespace)
+		return controller.NewAPIServerError(false, err)
+	}
+
+	for _, cond := range work.Status.ManifestConditions {
+		res := cond.Identifier
 		gvr := schema.GroupVersionResource{
 			Group:    res.Group,
 			Version:  res.Version,
 			Resource: res.Resource,
 		}
+
 		var obj *unstructured.Unstructured
 		var err error
-		if res.Namespace != "" {
-			obj, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Get(ctx, res.Name, metav1.GetOptions{})
-		} else {
-			obj, err = r.spokeDynamicClient.Resource(gvr).Get(ctx, res.Name, metav1.GetOptions{})
-		}
-		if err != nil {
+		if obj, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Get(ctx, res.Name, metav1.GetOptions{}); err != nil {
 			if apierrors.IsNotFound(err) {
 				continue
 			}
@@ -548,37 +555,20 @@ func (r *Reconciler) updateOwnerReference(ctx context.Context, appliedWork *flee
 		// Otherwise, at least one other owner reference exists, and we need to leave resource alone.
 		if len(obj.GetOwnerReferences()) > 1 {
 			ownerRefs := obj.GetOwnerReferences()
-
-			// Re-build the owner reference; update the blockOwnerDeletion field to false.
 			updated := false
 			for idx := range ownerRefs {
-				if ownerRefs[idx].UID == appliedWorkOwnerRef.UID &&
-					ownerRefs[idx].Name == appliedWorkOwnerRef.Name &&
-					ownerRefs[idx].Kind == appliedWorkOwnerRef.Kind &&
-					ownerRefs[idx].APIVersion == appliedWorkOwnerRef.APIVersion &&
-					*ownerRefs[idx].BlockOwnerDeletion {
+				if ownerRefEqualsExpected(&ownerRefs[idx], appliedWorkOwnerRef) {
 					ownerRefs[idx].BlockOwnerDeletion = ptr.To(false)
 					updated = true
 				}
 			}
 			if updated {
 				obj.SetOwnerReferences(ownerRefs)
-
-				if res.Namespace != "" {
-					_, err = r.spokeDynamicClient.Resource(gvr).Namespace(res.Namespace).Update(ctx, obj, metav1.UpdateOptions{})
-					if err != nil {
-						klog.ErrorS(err, "Failed to update manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
-						return err
-					}
-					klog.V(4).InfoS("Updated manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
-				} else {
-					_, err = r.spokeDynamicClient.Resource(gvr).Update(ctx, obj, metav1.UpdateOptions{})
-					if err != nil {
-						klog.ErrorS(err, "Failed to update manifest owner references", "gvr", gvr, "name", res.Name)
-						return err
-					}
-					klog.V(4).InfoS("Updated manifest owner references", "gvr", gvr, "name", res.Name)
+				if _, err = r.spokeDynamicClient.Resource(gvr).Namespace(obj.GetNamespace()).Update(ctx, obj, metav1.UpdateOptions{}); err != nil {
+					klog.ErrorS(err, "Failed to update manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
+					return err
 				}
+				klog.V(4).InfoS("Patched manifest owner references", "gvr", gvr, "name", res.Name, "namespace", res.Namespace)
 			}
 		}
 	}

pkg/controllers/workapplier/controller_integration_test.go

@@ -475,9 +475,13 @@ func workRemovedActual(workName string) func() error {
 	// Wait for the removal of the Work object.
 	return func() error {
 		work := &fleetv1beta1.Work{}
-		if err := hubClient.Get(ctx, client.ObjectKey{Name: workName, Namespace: memberReservedNSName}, work); !errors.IsNotFound(err) {
+		if err := hubClient.Get(ctx, client.ObjectKey{Name: workName, Namespace: memberReservedNSName}, work); !errors.IsNotFound(err) && err != nil {
 			return fmt.Errorf("work object still exists or an unexpected error occurred: %w", err)
 		}
+		if controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) {
+			// The Work object is being deleted, but the finalizer is still present.
+			return fmt.Errorf("work object is being deleted, but the finalizer is still present")
+		}
 		return nil
 	}
 }
@@ -1672,10 +1676,13 @@ var _ = Describe("work applier garbage collection", func() {
 				}
 				return nil
 			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object")
+
+			// Explicitly wait a minute to let the deletion timestamp progress
+			time.Sleep(1 * time.Minute) // Adjust based on your real-world scenario
 		})
 
 		It("should update owner reference from the Deployment object", func() {
-			// Ensure that the Deployment object has been updated to remove the owner reference.
+			// Ensure that the Deployment object has been updated applied owner reference with blockOwnerDeletion to false.
 			Eventually(func() error {
 				// Retrieve the Deployment object again.
 				gotDeploy := &appsv1.Deployment{}
@@ -1694,7 +1701,7 @@ var _ = Describe("work applier garbage collection", func() {
 					}
 				}
 				return nil
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment")
+			}, 3*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment")
 		})
 
 		AfterAll(func() {
@@ -1707,7 +1714,7 @@ var _ = Describe("work applier garbage collection", func() {
 			Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object")
 
 			workRemovedActual := workRemovedActual(workName)
-			Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
+			Eventually(workRemovedActual, 3*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
 
 			// Ensure that the Deployment object still exists.
 			Consistently(func() error {
@@ -1985,6 +1992,9 @@ var _ = Describe("work applier garbage collection", func() {
 				}
 				return nil
 			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object")
+
+			// Explicitly wait a minute to let the deletion timestamp progress
+			time.Sleep(1 * time.Minute) // Adjust based on your real-world scenario
 		})
 
 		It("should update owner reference from the ClusterRole object", func() {
@@ -2007,7 +2017,7 @@ var _ = Describe("work applier garbage collection", func() {
 				}
 
 				return nil
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from ClusterRole")
+			}, 3*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from ClusterRole")
 		})
 
 		AfterAll(func() {
@@ -2024,7 +2034,7 @@ var _ = Describe("work applier garbage collection", func() {
 			Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object")
 
 			workRemovedActual := workRemovedActual(workName)
-			Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
+			Eventually(workRemovedActual, 3*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
 
 			// Ensure that the ClusterRole object still exists.
 			Consistently(func() error {
@@ -2300,6 +2310,9 @@ var _ = Describe("work applier garbage collection", func() {
 				}
 				return nil
 			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to start deleting the AppliedWork object")
+
+			// Explicitly wait a minute to let the deletion timestamp progress
+			time.Sleep(1 * time.Minute) // Adjust based on your real-world scenario
 		})
 
 		It("should update owner reference from the Deployment object", func() {
@@ -2321,7 +2334,7 @@ var _ = Describe("work applier garbage collection", func() {
 					}
 				}
 				return nil
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment")
+			}, 3*eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove owner reference from Deployment")
 		})
 
 		AfterAll(func() {
@@ -2338,7 +2351,7 @@ var _ = Describe("work applier garbage collection", func() {
 			Eventually(appliedWorkRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the AppliedWork object")
 
 			workRemovedActual := workRemovedActual(workName)
-			Eventually(workRemovedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
+			Eventually(workRemovedActual, 3*time.Minute, eventuallyInterval).Should(Succeed(), "Failed to remove the Work object")
 
 			// Ensure that the Deployment object still exists.
 			Consistently(func() error {

pkg/controllers/workapplier/preprocess.go

@@ -543,10 +543,7 @@ func isInMemberClusterObjectDerivedFromManifestObj(inMemberClusterObj *unstructu
 	// Verify if the owner reference still stands.
 	curOwners := inMemberClusterObj.GetOwnerReferences()
 	for idx := range curOwners {
-		if curOwners[idx].UID == expectedAppliedWorkOwnerRef.UID &&
-			curOwners[idx].Name == expectedAppliedWorkOwnerRef.Name &&
-			curOwners[idx].Kind == expectedAppliedWorkOwnerRef.Kind &&
-			curOwners[idx].APIVersion == expectedAppliedWorkOwnerRef.APIVersion {
+		if ownerRefEqualsExpected(&curOwners[idx], expectedAppliedWorkOwnerRef) {
 			return true
 		}
 	}
@@ -560,14 +557,19 @@ func removeOwnerRef(obj *unstructured.Unstructured, expectedAppliedWorkOwnerRef
 
 	// Re-build the owner references; remove the given one from the list.
 	for idx := range ownerRefs {
-		if ownerRefs[idx].UID == expectedAppliedWorkOwnerRef.UID &&
-			ownerRefs[idx].Name == expectedAppliedWorkOwnerRef.Name &&
-			ownerRefs[idx].Kind == expectedAppliedWorkOwnerRef.Kind &&
-			ownerRefs[idx].APIVersion == expectedAppliedWorkOwnerRef.APIVersion {
+		if ownerRefEqualsExpected(&ownerRefs[idx], expectedAppliedWorkOwnerRef) {
 			// Skip the expected owner reference.
 			continue
 		}
 		updatedOwnerRefs = append(updatedOwnerRefs, ownerRefs[idx])
 	}
 	obj.SetOwnerReferences(updatedOwnerRefs)
 }
+
+// ownerRefEquals returns true if two owner references are equal based on UID, Name, Kind, and APIVersion.
+func ownerRefEqualsExpected(a, b *metav1.OwnerReference) bool {
+	return a.UID == b.UID &&
+		a.Name == b.Name &&
+		a.Kind == b.Kind &&
+		a.APIVersion == b.APIVersion
+}

pkg/controllers/workapplier/process.go

@@ -356,10 +356,7 @@ func canApplyWithOwnership(inMemberClusterObj *unstructured.Unstructured, expect
 	// Verify if the object is owned by Fleet.
 	curOwners := inMemberClusterObj.GetOwnerReferences()
 	for idx := range curOwners {
-		if curOwners[idx].UID == expectedAppliedWorkOwnerRef.UID &&
-			curOwners[idx].Name == expectedAppliedWorkOwnerRef.Name &&
-			curOwners[idx].Kind == expectedAppliedWorkOwnerRef.Kind &&
-			curOwners[idx].APIVersion == expectedAppliedWorkOwnerRef.APIVersion {
+		if ownerRefEqualsExpected(&curOwners[idx], expectedAppliedWorkOwnerRef) {
 			return true
 		}
 	}

pkg/controllers/workapplier/suite_test.go

@@ -165,6 +165,7 @@ var _ = BeforeSuite(func() {
 		workerCount,
 		time.Second*5,
 		time.Second*5,
+		time.Minute,
 		true,
 		60,
 	)