update to workapplier to use foreground deletion, add and fix tests

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

Author: britaniar

pkg/controllers/workapplier/controller.go

@@ -417,7 +417,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		Kind:               fleetv1beta1.AppliedWorkKind,
 		Name:               appliedWork.GetName(),
 		UID:                appliedWork.GetUID(),
-		BlockOwnerDeletion: ptr.To(false),
+		BlockOwnerDeletion: ptr.To(true),
 	}
 
 	// Set the default values for the Work object to avoid additional validation logic in the
@@ -487,7 +487,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 // 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.DeletePropagationBackground
+	deletePolicy := metav1.DeletePropagationForeground
 	if !controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) {
 		return ctrl.Result{}, nil
 	}
@@ -496,18 +496,22 @@ func (r *Reconciler) garbageCollectAppliedWork(ctx context.Context, work *fleetv
 	appliedWork := fleetv1beta1.AppliedWork{
 		ObjectMeta: metav1.ObjectMeta{Name: work.Name},
 	}
-	err := r.spokeClient.Delete(ctx, &appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy})
-	switch {
-	case apierrors.IsNotFound(err):
-		klog.V(2).InfoS("The appliedWork is already deleted", "appliedWork", work.Name)
-	case err != nil:
-		klog.ErrorS(err, "Failed to delete 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)
-	default:
-		klog.InfoS("Successfully deleted the appliedWork", "appliedWork", work.Name)
 	}
-	controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer)
 
+	klog.V(2).InfoS("Deletion for appliedWork is in progress", "appliedWork", work.Name)
+	return ctrl.Result{Requeue: true}, nil
+}
+
+// removeWorkFinalizer removes the finalizer from the work and updates it in the hub.
+func (r *Reconciler) removeWorkFinalizer(ctx context.Context, work *fleetv1beta1.Work) (ctrl.Result, error) {
+	controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer)
 	if err := r.hubClient.Update(ctx, work, &client.UpdateOptions{}); err != nil {
 		klog.ErrorS(err, "Failed to remove the finalizer from the work", "work", klog.KObj(work))
 		return ctrl.Result{}, controller.NewAPIServerError(false, err)
@@ -557,6 +561,10 @@ func (r *Reconciler) ensureAppliedWork(ctx context.Context, work *fleetv1beta1.W
 			return nil, controller.NewAPIServerError(false, err)
 		}
 	}
+	if err := r.spokeClient.Get(ctx, types.NamespacedName{Name: workRef.Name}, appliedWork); err != nil {
+		klog.ErrorS(err, "Failed to get the appliedWork", "appliedWork", appliedWork.Name)
+		return nil, controller.NewAPIServerError(false, err)
+	}
 	klog.InfoS("Recreated the appliedWork resource", "appliedWork", workRef.Name)
 	return appliedWork, nil
 }

pkg/controllers/workapplier/controller_integration_test.go

@@ -178,7 +178,7 @@ func prepareAppliedWorkOwnerRef(workName string) *metav1.OwnerReference {
 		Kind:               "AppliedWork",
 		Name:               appliedWork.Name,
 		UID:                appliedWork.GetUID(),
-		BlockOwnerDeletion: ptr.To(false),
+		BlockOwnerDeletion: ptr.To(true),
 	}
 }
 
@@ -451,6 +451,10 @@ func cleanupWorkObject(workName string) {
 	workRemovedActual := func() error {
 		work := &fleetv1beta1.Work{}
 		if err := hubClient.Get(ctx, client.ObjectKey{Name: workName, Namespace: memberReservedNSName}, work); !errors.IsNotFound(err) {
+			if controllerutil.ContainsFinalizer(work, fleetv1beta1.WorkFinalizer) {
+				controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer)
+				Expect(hubClient.Update(ctx, work)).To(Succeed(), "Failed to remove the finalizer from the Work object")
+			}
 			return fmt.Errorf("work object still exists or an unexpected error occurred: %w", err)
 		}
 		return nil
@@ -459,6 +463,15 @@ func cleanupWorkObject(workName string) {
 }
 
 func appliedWorkRemovedActual(workName string) func() error {
+	// Retrieve the AppliedWork object.
+	currentAppliedWork := &fleetv1beta1.AppliedWork{}
+	if err := memberClient.Get(ctx, client.ObjectKey{Name: workName}, currentAppliedWork); err == nil {
+		if controllerutil.ContainsFinalizer(currentAppliedWork, metav1.FinalizerDeleteDependents) {
+			controllerutil.RemoveFinalizer(currentAppliedWork, metav1.FinalizerDeleteDependents)
+			Expect(memberClient.Update(ctx, currentAppliedWork)).To(Succeed(), "Failed to remove the finalizer from the AppliedWork object")
+		}
+	}
+
 	return func() error {
 		// Retrieve the AppliedWork object.
 		appliedWork := &fleetv1beta1.AppliedWork{}

test/e2e/placement_apply_strategy_test.go

@@ -23,6 +23,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -77,11 +78,21 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		})
 
 		AfterAll(func() {
+			By("deleting created work resources on member cluster")
+			cleanWorkResourcesOnCluster(allMemberClusters[0])
+
 			By(fmt.Sprintf("deleting placement %s", crpName))
 			cleanupCRP(crpName)
 
-			By("deleting created work resources on member cluster")
-			cleanWorkResourcesOnCluster(allMemberClusters[0])
+			// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
+			// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
+			// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
+			// and flakiness in subsequent tests.
+			By("Check if work is deleted")
+			Eventually(func() bool {
+				work := &placementv1beta1.Work{}
+				return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
+			}, 6*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
 		})
 
 		It("should update CRP status as expected", func() {
@@ -119,7 +130,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		It("namespace should be kept on member cluster", func() {
 			Consistently(func() error {
 				ns := &corev1.Namespace{}
-				return allMemberClusters[0].KubeClient.Get(ctx, types.NamespacedName{Name: workNamespaceName}, ns)
+				return hubClient.Get(ctx, types.NamespacedName{Name: workNamespaceName}, ns)
 			}, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Namespace which is not owned by the CRP should not be deleted")
 		})
 	})
@@ -139,6 +150,16 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		})
 
 		AfterAll(func() {
+			// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
+			// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
+			// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
+			// and flakiness in subsequent tests.
+			By("Check if work is deleted")
+			Eventually(func() bool {
+				work := &placementv1beta1.Work{}
+				return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
+			}, 6*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
+
 			By(fmt.Sprintf("deleting placement %s", crpName))
 			cleanupCRP(crpName)
 		})
@@ -194,6 +215,7 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		AfterAll(func() {
 			By(fmt.Sprintf("deleting placement %s", crpName))
 			cleanupCRP(crpName)
+
 		})
 
 		It("should update CRP status as expected", func() {
@@ -250,11 +272,21 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		})
 
 		AfterAll(func() {
+			By("deleting created work resources on member cluster")
+			cleanWorkResourcesOnCluster(allMemberClusters[0])
+
 			By(fmt.Sprintf("deleting placement %s", crpName))
 			cleanupCRP(crpName)
 
-			By("deleting created work resources on member cluster")
-			cleanWorkResourcesOnCluster(allMemberClusters[0])
+			// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
+			// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
+			// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
+			// and flakiness in subsequent tests.
+			By("Check if work is deleted")
+			Eventually(func() bool {
+				work := &placementv1beta1.Work{}
+				return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
+			}, 6*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
 		})
 
 		It("should update CRP status as expected", func() {
@@ -385,11 +417,21 @@ var _ = Describe("validating CRP when resources exists", Ordered, func() {
 		})
 
 		AfterAll(func() {
+			By("deleting created work resources on member cluster")
+			cleanWorkResourcesOnCluster(allMemberClusters[0])
+
 			By(fmt.Sprintf("deleting placement %s", crpName))
 			cleanupCRP(crpName)
 
-			By("deleting created work resources on member cluster")
-			cleanWorkResourcesOnCluster(allMemberClusters[0])
+			// Check if work is deleted. Needed to ensure that the Work resource is cleaned up before the next CRP is created.
+			// This is because the Work resource is created with a finalizer that blocks deletion until the all applied work
+			// and applied work itself is successfully deleted. If the Work resource is not deleted, it can cause resource overlap
+			// and flakiness in subsequent tests.
+			By("Check if work is deleted")
+			Eventually(func() bool {
+				work := &placementv1beta1.Work{}
+				return errors.IsNotFound(hubClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-work", crpName), Namespace: fmt.Sprintf("fleet-member-%s", allMemberClusterNames[0])}, work))
+			}, 7*eventuallyDuration, eventuallyInterval).Should(BeTrue(), "Work resource should be deleted from hub")
 		})
 
 		It("should update CRP status as expected", func() {

test/e2e/placement_pickfixed_test.go

@@ -21,9 +21,11 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	"github.com/kubefleet-dev/kubefleet/test/e2e/framework"
@@ -190,4 +192,105 @@ var _ = Describe("placing resources using a CRP of PickFixed placement type", fu
 			ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{})
 		})
 	})
+
+	Context("switch to another cluster to simulate stuck deleting works", Ordered, func() {
+		crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
+		workNamespaceName := fmt.Sprintf(workNamespaceNameTemplate, GinkgoParallelProcess())
+		appConfigMapName := fmt.Sprintf(appConfigMapNameTemplate, GinkgoParallelProcess())
+		var currentConfigMap corev1.ConfigMap
+
+		BeforeAll(func() {
+			// Create the resources.
+			createWorkResources()
+
+			// Create the CRP.
+			crp := &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: crpName,
+					// Add a custom finalizer; this would allow us to better observe
+					// the behavior of the controllers.
+					Finalizers: []string{customDeletionBlockerFinalizer},
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: workResourceSelector(),
+					Strategy: placementv1beta1.RolloutStrategy{
+						Type: placementv1beta1.RollingUpdateRolloutStrategyType,
+						RollingUpdate: &placementv1beta1.RollingUpdateConfig{
+							UnavailablePeriodSeconds: ptr.To(2),
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+						ClusterNames: []string{
+							memberCluster1EastProdName,
+						},
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP")
+		})
+
+		It("should update CRP status as expected", func() {
+			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster1EastProdName}, nil, "0")
+			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+		})
+
+		It("should place resources on specified clusters", func() {
+			resourcePlacedActual := workNamespaceAndConfigMapPlacedOnClusterActual(memberCluster1EastProd)
+			Eventually(resourcePlacedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to place resources on specified clusters")
+		})
+
+		It("should add finalizer to work resources on the specified clusters", func() {
+			Eventually(func() error {
+				if err := memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, &currentConfigMap); err != nil {
+					return err
+				}
+				return nil
+			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to get configmap")
+			// Add finalizer to block deletion to simulate work stuck
+			controllerutil.AddFinalizer(&currentConfigMap, "example.com/finalizer")
+			Expect(memberCluster1EastProd.KubeClient.Update(ctx, &currentConfigMap)).To(Succeed(), "Failed to update configmap with finalizer")
+		})
+
+		It("update crp to pick another cluster", func() {
+			Eventually(func() error {
+				crp := &placementv1beta1.ClusterResourcePlacement{}
+				if err := hubClient.Get(ctx, types.NamespacedName{Name: crpName}, crp); err != nil {
+					return err
+				}
+				crp.Spec.Policy.ClusterNames = []string{memberCluster2EastCanaryName}
+				return hubClient.Update(ctx, crp)
+			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP")
+		})
+
+		It("should update CRP status as expected", func() {
+			// should successfully apply to the new cluster
+			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster2EastCanaryName}, nil, "0")
+			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+		})
+
+		It("configmap should still exists on previously specified cluster", func() {
+			Expect(memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, &currentConfigMap)).Should(BeNil(), "ConfigMap should still exists on previously specified cluster")
+		})
+
+		It("should remove finalizer from work resources on the specified clusters", func() {
+			configMap := &corev1.ConfigMap{}
+			Expect(memberCluster1EastProd.KubeClient.Get(ctx, types.NamespacedName{Namespace: workNamespaceName, Name: appConfigMapName}, configMap)).Should(Succeed(), "Failed to get configmap")
+			controllerutil.RemoveFinalizer(configMap, "example.com/finalizer")
+			Expect(memberCluster1EastProd.KubeClient.Update(ctx, configMap)).To(Succeed(), "Failed to update configmap with finalizer")
+		})
+
+		It("should remove resources from previously specified clusters", func() {
+			checkIfRemovedWorkResourcesFromMemberClusters([]*framework.Cluster{memberCluster1EastProd})
+		})
+
+		It("should update CRP status as expected", func() {
+			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster2EastCanaryName}, nil, "0")
+			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+		})
+
+		AfterAll(func() {
+			ensureCRPAndRelatedResourcesDeleted(crpName, []*framework.Cluster{memberCluster1EastProd, memberCluster2EastCanary})
+		})
+	})
 })

test/e2e/resources/test-job.yaml

@@ -2,7 +2,9 @@ apiVersion: batch/v1
 kind: Job
 metadata:
   name: pi
+  namespace: test-ns
 spec:
+  suspend: true
   template:
     spec:
       containers:

test/e2e/rollout_test.go

@@ -718,8 +718,8 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() {
 		})
 		// job is not trackable, so we need to wait for a bit longer for each roll out
 		It("should update CRP status as expected", func() {
-			crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, allMemberClusterNames, nil, "1", false)
-			Eventually(crpStatusUpdatedActual, 5*time.Duration(unAvailablePeriodSeconds)*time.Second, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
+			crpStatusUpdatedActual := customizedCRPStatusUpdatedActual(crpName, wantSelectedResources, allMemberClusterNames, nil, "0", false)
+			Eventually(crpStatusUpdatedActual, 6*time.Duration(unAvailablePeriodSeconds)*time.Second, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected")
 		})
 
 		AfterAll(func() {

test/e2e/utils_test.go

@@ -1265,7 +1265,7 @@ func buildOwnerReference(cluster *framework.Cluster, crpName string) *metav1.Own
 		Kind:               "AppliedWork",
 		Name:               workName,
 		UID:                appliedWork.UID,
-		BlockOwnerDeletion: ptr.To(false),
+		BlockOwnerDeletion: ptr.To(true),
 	}
 }