Repository: rancher/fleet

Issue:
Editing targets for a GitRepo takes some time to delete orphaned BundleDeployments
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current Behavior

When modifying an existing `GitRepo` that is deployed to multiple target clusters, if one of the clusters no longer matches the conditions, Fleet "undeploys" those manifests (`BundleDeployments`) included in the `GitRepo` for that cluster.
However, while the `Bundle` status is updated immediately, the corresponding `BundleDeployments` are not, which indirectly causes the `GitRepo` status to contain mismatching information, since `BundleDeployments` are used to construct it.

After some time, the offending `BundleDeployments` will be garbage-collected, causing every resource to reconcile to the desired state. However, in some cases it can take up to 10 minutes, which is undesirable as it could lead to confusion (e.g. Force Updating the `GitRepo` does not have any effect on this).

### Expected Behavior

Garbage-collection of unneeded `BundleDeployments` should happen on a timely fashion.

### Steps To Reproduce

1. Setup Rancher and at least 2 downstream clusters within the same cluster group.
2. In the `fleet-default` namespace (or where the downstream clusters were placed), add a new `GitRepo` that will deploy the same workload to all downstream clusters
   - Just anything should suffice, e.g. https://github.com/rancher/fleet-test-data/tree/master/simple
3. Wait for the `GitRepo` to become `Ready`, then observe the value that appears for `Resources` count and "ready clusters".
4. Edit the `GItRepo`'s targets configuration to only deploy to one downstream cluster.
5. Wait for `GitRepo` to become `Ready` again, then observe the `Resources` count and "ready clusters" didn't change.
6. After some time, this will eventually decrease to the desired value, meaning that `BundleDeployments` were effectively deleted.

### Environment

```markdown
- Fleet Version: 0.12
```

### Logs

```markdown

```

### Anything else?

I was uncertain of what process was actually deleting the `BundleDeployments` after a few minutes, thinking that it could be some scheduled or background job.
However, after some investigation I found that is only done from the `Cluster` reconciler, which not only triggers on their own events but also based on certain changes on `BundleDeployment` objects. This was causing the difference in behavior I observed.

Hints from issue comments:
@aruiz14: /backport v2.11.2@sbulage: ## System Information

### Before Upgrade:

| Rancher Version | Fleet Version |
|--------|--------|
| v2.11.0 | 106.1.0+up0.12.0 | 

### Steps followed
- Steps followed from: https://github.com/rancher/fleet/issues/3660#issuecomment-2858865335
- By creating `targetCustomization` with `doNotDeploy` options.
- `bundleDeployments` are taking too much time to remove.

---

### After Upgrade

| Rancher Version | Fleet Version |
|--------|--------|
| v2.12-49289cc9c6590b361d64950977dd20b1214908d7-head | 107.0.0+up0.13.0-alpha.3 |

- See video for dynamic removal of `bundleDeployments` with less than 5 seconds.

---

### Video showing upgrade process

<details><summary>Video (Play at x1.5 speed and skip the upgrade part)</summary>

https://github.com/user-attachments/assets/3ec98e27-dfce-452a-a11e-5867cf797ca6

</details> 

Solution patch:
diff --git a/internal/cmd/controller/finalize/finalize.go b/internal/cmd/controller/finalize/finalize.go
index 1cfc3144..f25ee352 100644
--- a/internal/cmd/controller/finalize/finalize.go
+++ b/internal/cmd/controller/finalize/finalize.go
@@ -64,29 +64,6 @@ func PurgeBundles(ctx context.Context, c client.Client, gitrepo types.Namespaced
 	return nil
 }
 
-// PurgeBundleDeployments deletes all BundleDeployments related with the given Bundle namespaced name.
-func PurgeBundleDeployments(ctx context.Context, c client.Client, bundle types.NamespacedName) error {
-	list := &v1alpha1.BundleDeploymentList{}
-	err := c.List(
-		ctx,
-		list,
-		client.MatchingLabels{
-			v1alpha1.BundleLabel:          bundle.Name,
-			v1alpha1.BundleNamespaceLabel: bundle.Namespace,
-		},
-	)
-	if err != nil {
-		return err
-	}
-	for _, bd := range list.Items {
-		if err := PurgeBundleDeployment(ctx, c, bd); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
 // PurgeContent tries to delete the content resource related with the given bundle deployment.
 func PurgeContent(ctx context.Context, c client.Client, name, deplID string) error {
 	contentID, _ := kv.Split(deplID, ":")
@@ -179,47 +156,3 @@ func PurgeNamespace(ctx context.Context, c client.Client, deleteNamespace bool,
 
 	return nil
 }
-
-// PurgeBundleDeploymentList deletes the given list of bundledeployments
-func PurgeBundleDeploymentList(ctx context.Context, c client.Client, bds []types.NamespacedName) error {
-	for _, bdId := range bds {
-		bd := &v1alpha1.BundleDeployment{}
-		if err := c.Get(ctx, bdId, bd); err != nil {
-			return err
-		}
-		if err := PurgeBundleDeployment(ctx, c, *bd); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// PurgeBundleDeployment deletes the given bundle deployment and the content related to it
-func PurgeBundleDeployment(ctx context.Context, c client.Client, bd v1alpha1.BundleDeployment) error {
-	if controllerutil.ContainsFinalizer(&bd, BundleDeploymentFinalizer) { // nolint: gosec // does not store pointer
-		nn := types.NamespacedName{Namespace: bd.Namespace, Name: bd.Name}
-		err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			t := &v1alpha1.BundleDeployment{}
-			if err := c.Get(ctx, nn, t); err != nil {
-				return err
-			}
-
-			controllerutil.RemoveFinalizer(t, BundleDeploymentFinalizer)
-
-			return c.Update(ctx, t)
-		})
-		if err != nil {
-			return err
-		}
-	}
-
-	if err := c.Delete(ctx, &bd); err != nil {
-		return err
-	}
-
-	if err := PurgeContent(ctx, c, bd.Name, bd.Spec.DeploymentID); err != nil {
-		return err
-	}
-
-	return nil
-}
diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go
index caef037b..f0f5afb6 100644
--- a/internal/cmd/controller/reconciler/bundle_controller.go
+++ b/internal/cmd/controller/reconciler/bundle_controller.go
@@ -4,13 +4,13 @@ package reconciler
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
+	"slices"
 	"strconv"
 	"time"
 
-	"github.com/go-logr/logr"
-
 	"github.com/rancher/fleet/internal/cmd/controller/finalize"
 	"github.com/rancher/fleet/internal/cmd/controller/summary"
 	"github.com/rancher/fleet/internal/cmd/controller/target"
@@ -21,11 +21,13 @@ import (
 	"github.com/rancher/fleet/pkg/sharding"
 	"github.com/rancher/wrangler/v3/pkg/genericcondition"
 
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -36,7 +38,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
-const bundleFinalizer = "fleet.cattle.io/bundle-finalizer"
+const (
+	// period after which the Bundle reconciler is re-scheduled, in order to wait for the BundleDeploymentReconciler cleanup to finish
+	requeueAfterBundleDeploymentCleanup = 2 * time.Second
+)
 
 type BundleQuery interface {
 	// BundlesForCluster is used to map from a cluster to bundles
@@ -134,7 +139,6 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	if err := r.Get(ctx, req.NamespacedName, bundle); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	bundleOrig := bundle.DeepCopy()
 
 	if bundle.Labels[fleet.RepoLabel] != "" {
 		logger = logger.WithValues(
@@ -143,10 +147,16 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		)
 	}
 
-	if res, err := r.addOrRemoveFinalizer(ctx, logger, req, bundle); res {
+	if !bundle.DeletionTimestamp.IsZero() {
+		return r.handleDelete(ctx, logger, req, bundle)
+	}
+
+	if err := r.ensureFinalizer(ctx, bundle); err != nil {
 		return ctrl.Result{}, err
 	}
 
+	bundleOrig := bundle.DeepCopy()
+
 	logger.V(1).Info(
 		"Reconciling bundle, checking targets, calculating changes, building objects",
 		"generation",
@@ -244,6 +254,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 	// build BundleDeployments out of targets discarding Status, replacing DependsOn with the
 	// bundle's DependsOn (pure function) and replacing the labels with the bundle's labels
+	bundleDeploymentUIDs := make(sets.Set[types.UID])
 	for _, target := range matchedTargets {
 		if target.Deployment == nil {
 			continue
@@ -262,7 +273,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 		// No need to check the deletion timestamp here before adding a finalizer, since the bundle has just
 		// been created.
-		controllerutil.AddFinalizer(bd, bundleDeploymentFinalizer)
+		controllerutil.AddFinalizer(bd, finalize.BundleDeploymentFinalizer)
 
 		bd.Spec.OCIContents = contentsInOCI
 		bd.Spec.HelmChartOptions = bundle.Spec.HelmAppOptions
@@ -277,27 +288,24 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		if err != nil {
 			return ctrl.Result{}, err
 		}
+		bundleDeploymentUIDs.Insert(bd.UID)
 
 		if err := r.handleDeploymentSecret(ctx, bundle, bd); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
 
-	// purge bundle deployments that exist in the bundle but are no longer targeted
-	notTargeted, err := findNotTargetedBundleDeployments(ctx, r.Client, req.NamespacedName, matchedTargets)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-	if len(notTargeted) > 0 {
-		if err := finalize.PurgeBundleDeploymentList(ctx, r.Client, notTargeted); err != nil {
-			return ctrl.Result{}, err
-		}
+	// the targets configuration may have changed, leaving behind some BundleDeployments that are no longer needed
+	if err := r.cleanupOrphanedBundleDeployments(ctx, bundle, bundleDeploymentUIDs); err != nil {
+		logger.V(1).Error(err, "deleting orphaned bundle deployments", "bundle", bundle.GetName())
 	}
 
 	updateDisplay(&bundle.Status)
-	err = r.updateStatus(ctx, bundleOrig, bundle)
+	if err := r.updateStatus(ctx, bundleOrig, bundle); err != nil {
+		return ctrl.Result{}, err
+	}
 
-	return ctrl.Result{}, err
+	return ctrl.Result{}, nil
 }
 
 func upper(op controllerutil.OperationResult) string {
@@ -317,40 +325,38 @@ func upper(op controllerutil.OperationResult) string {
 	}
 }
 
-// addOrRemoveFinalizer adds a finalizer to a recently created bundle, or removes it on a bundle marked for deletion.
-// It returns a boolean indicating whether the current reconcile loop should stop, along with any error which may have
-// occurred in the process.
-func (r *BundleReconciler) addOrRemoveFinalizer(ctx context.Context, logger logr.Logger, req ctrl.Request, bundle *fleet.Bundle) (bool, error) {
-	if !bundle.DeletionTimestamp.IsZero() {
-		if controllerutil.ContainsFinalizer(bundle, bundleFinalizer) {
-			metrics.BundleCollector.Delete(req.Name, req.Namespace)
-
-			logger.V(1).Info("Bundle not found, purging bundle deployments")
-			if err := finalize.PurgeBundleDeployments(ctx, r.Client, req.NamespacedName); err != nil {
-				// A bundle deployment may have been purged by the GitRepo reconciler, hence we ignore
-				// not-found errors here.
-				return true, client.IgnoreNotFound(err)
-			}
+// handleDelete runs cleanup for resources associated to a Bundle, finally removing the finalizer to unblock the deletion of the object from kubernetes.
+func (r *BundleReconciler) handleDelete(ctx context.Context, logger logr.Logger, req ctrl.Request, bundle *fleet.Bundle) (ctrl.Result, error) {
+	if !controllerutil.ContainsFinalizer(bundle, finalize.BundleFinalizer) {
+		return ctrl.Result{}, nil
+	}
 
-			controllerutil.RemoveFinalizer(bundle, bundleFinalizer)
-			err := r.Update(ctx, bundle)
-			if client.IgnoreNotFound(err) != nil {
-				return true, err
-			}
-		}
+	bds, err := r.listBundleDeploymentsForBundle(ctx, bundle)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
 
-		return true, nil
+	// BundleDeployment deletion happens asynchronously: mark them for deletion and requeue
+	// This ensures the Bundle is kept around until all its BundleDeployments are completely deleted.
+	// Both GitRepo and HelmApp status reconcilers rely on this condition, as they watch Bundles and not BundleDeployments
+	if len(bds) > 0 {
+		logger.V(1).Info("Bundle deleted, purging bundle deployments")
+		return ctrl.Result{RequeueAfter: requeueAfterBundleDeploymentCleanup}, batchDeleteBundleDeployments(ctx, r.Client, bds)
 	}
 
-	if !controllerutil.ContainsFinalizer(bundle, bundleFinalizer) {
-		controllerutil.AddFinalizer(bundle, bundleFinalizer)
-		err := r.Update(ctx, bundle)
-		if client.IgnoreNotFound(err) != nil {
-			return true, err
-		}
+	metrics.BundleCollector.Delete(req.Name, req.Namespace)
+	controllerutil.RemoveFinalizer(bundle, finalize.BundleFinalizer)
+	return ctrl.Result{}, r.Update(ctx, bundle)
+}
+
+// ensureFinalizer adds a finalizer to a recently created bundle.
+func (r *BundleReconciler) ensureFinalizer(ctx context.Context, bundle *fleet.Bundle) error {
+	if controllerutil.ContainsFinalizer(bundle, finalize.BundleFinalizer) {
+		return nil
 	}
 
-	return false, nil
+	controllerutil.AddFinalizer(bundle, finalize.BundleFinalizer)
+	return r.Update(ctx, bundle)
 }
 
 func (r *BundleReconciler) createBundleDeployment(
@@ -499,33 +505,43 @@ func (r *BundleReconciler) updateStatus(ctx context.Context, orig *fleet.Bundle,
 	return nil
 }
 
-func findNotTargetedBundleDeployments(ctx context.Context, c client.Client, bundleId types.NamespacedName, targets []*target.Target) ([]types.NamespacedName, error) {
+func (r *BundleReconciler) listBundleDeploymentsForBundle(ctx context.Context, bundle *fleet.Bundle) ([]fleet.BundleDeployment, error) {
 	list := &fleet.BundleDeploymentList{}
-	err := c.List(
-		ctx,
-		list,
+	if err := r.List(ctx, list,
 		client.MatchingLabels{
-			fleet.BundleLabel:          bundleId.Name,
-			fleet.BundleNamespaceLabel: bundleId.Namespace,
+			fleet.BundleLabel:          bundle.GetName(),
+			fleet.BundleNamespaceLabel: bundle.GetNamespace(),
 		},
-	)
+	); err != nil {
+		return nil, err
+	}
+	return list.Items, nil
+}
+
+// cleanupOrphanedBundleDeployments will delete all existing BundleDeployments which do not have a match in a provided list of UIDs
+func (r *BundleReconciler) cleanupOrphanedBundleDeployments(ctx context.Context, bundle *fleet.Bundle, uidsToKeep sets.Set[types.UID]) error {
+	list, err := r.listBundleDeploymentsForBundle(ctx, bundle)
 	if err != nil {
-		return []types.NamespacedName{}, err
+		return err
 	}
+	toDelete := slices.DeleteFunc(list, func(bd fleet.BundleDeployment) bool {
+		return uidsToKeep.Has(bd.UID)
+	})
+	return batchDeleteBundleDeployments(ctx, r.Client, toDelete)
+}
 
-	var notTargeted []types.NamespacedName
-	for _, bd := range list.Items {
-		found := false
-		for _, t := range targets {
-			if t.Deployment.Name == bd.Name && t.Deployment.Namespace == bd.Namespace {
-				found = true
-				break
-			}
+func batchDeleteBundleDeployments(ctx context.Context, c client.Client, list []fleet.BundleDeployment) error {
+	var errs []error
+	for _, bd := range list {
+		if bd.DeletionTimestamp != nil {
+			// already being deleted
+			continue
 		}
-		if !found {
-			notTargeted = append(notTargeted, types.NamespacedName{Namespace: bd.Namespace, Name: bd.Name})
+		// Mark the object for deletion. The BundleDeployment reconciler will react to that calling PurgeContent and finally removing the finalizer
+		if err := c.Delete(ctx, &bd); client.IgnoreNotFound(err) != nil {
+			errs = append(errs, err)
 		}
 	}
 
-	return notTargeted, nil
+	return errors.Join(errs...)
 }
diff --git a/internal/cmd/controller/reconciler/bundledeployment_controller.go b/internal/cmd/controller/reconciler/bundledeployment_controller.go
index 57897fdb..dd7fcffa 100644
--- a/internal/cmd/controller/reconciler/bundledeployment_controller.go
+++ b/internal/cmd/controller/reconciler/bundledeployment_controller.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"reflect"
 
+	"github.com/rancher/fleet/internal/cmd/controller/finalize"
 	"github.com/rancher/fleet/internal/cmd/controller/summary"
 	"github.com/rancher/fleet/internal/metrics"
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
@@ -24,8 +25,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
-const bundleDeploymentFinalizer = "fleet.cattle.io/bundle-deployment-finalizer"
-
 // BundleDeploymentReconciler reconciles a BundleDeployment object
 type BundleDeploymentReconciler struct {
 	client.Client
@@ -65,17 +64,19 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 	// The bundle reconciler takes care of adding the finalizer when creating a bundle deployment
 	if !bd.DeletionTimestamp.IsZero() {
-		if controllerutil.ContainsFinalizer(bd, bundleDeploymentFinalizer) {
+		if controllerutil.ContainsFinalizer(bd, finalize.BundleDeploymentFinalizer) {
+			if err := finalize.PurgeContent(ctx, r.Client, bd.Name, bd.Spec.DeploymentID); err != nil {
+				return ctrl.Result{}, err
+			}
 			err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
-				t := &fleet.BundleDeployment{}
-				err := r.Get(ctx, req.NamespacedName, t)
+				err := r.Get(ctx, req.NamespacedName, bd)
 				if err != nil {
 					return err
 				}
 
-				controllerutil.RemoveFinalizer(t, bundleDeploymentFinalizer)
+				controllerutil.RemoveFinalizer(bd, finalize.BundleDeploymentFinalizer)
 
-				return r.Update(ctx, t)
+				return r.Update(ctx, bd)
 			})
 			if err != nil {
 				return ctrl.Result{}, err



Test patch:
diff --git a/integrationtests/controller/bundle/bundle_targets_test.go b/integrationtests/controller/bundle/bundle_targets_test.go
index 0d82fe79..257c850c 100644
--- a/integrationtests/controller/bundle/bundle_targets_test.go
+++ b/integrationtests/controller/bundle/bundle_targets_test.go
@@ -38,10 +38,12 @@ var _ = Describe("Bundle targets", Ordered, func() {
 		bundleName                        string
 		bdLabels                          map[string]string
 		expectedNumberOfBundleDeployments int
+		bundle                            *v1alpha1.Bundle
 	)
 
 	JustBeforeEach(func() {
-		bundle, err := utils.CreateBundle(ctx, k8sClient, bundleName, namespace, targets, targetRestrictions)
+		var err error
+		bundle, err = utils.CreateBundle(ctx, k8sClient, bundleName, namespace, targets, targetRestrictions)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(bundle).To(Not(BeNil()))
 	})
@@ -87,6 +89,39 @@ var _ = Describe("Bundle targets", Ordered, func() {
 		})
 	})
 
+	When("a GitRepo targets are updated", func() {
+		BeforeEach(func() {
+			bundleName = "all-then-one"
+			bdLabels = map[string]string{
+				"fleet.cattle.io/bundle-name":      bundleName,
+				"fleet.cattle.io/bundle-namespace": namespace,
+			}
+			expectedNumberOfBundleDeployments = 3
+			// simulate targets in GitRepo. All targets in GitRepo are also added to targetRestrictions, which acts as a white list
+			targets = []v1alpha1.BundleTarget{
+				{
+					ClusterGroup: "all",
+				},
+			}
+			targetRestrictions = make([]v1alpha1.BundleTarget, len(targets))
+			copy(targetRestrictions, targets)
+		})
+
+		It("deletes orphaned BundleDeployments ", func() {
+			verifyBundlesDeploymentsAreCreated(expectedNumberOfBundleDeployments, bdLabels, bundleName)
+
+			By("modifying the Bundle's targets spec")
+			mod := bundle.DeepCopy()
+			mod.Spec.Targets = []v1alpha1.BundleTarget{
+				{ClusterName: "one"},
+			}
+			err := k8sClient.Patch(ctx, mod, client.MergeFrom(bundle))
+			Expect(err).NotTo(HaveOccurred())
+
+			verifyBundlesDeploymentsAreCreated(1, bdLabels, bundleName)
+		})
+	})
+
 	When("a target customization is specified for all clusters", func() {
 		BeforeEach(func() {
 			bundleName = "all-customized"
diff --git a/integrationtests/gitjob/controller/suite_test.go b/integrationtests/gitjob/controller/suite_test.go
index d71ccade..99bc1d3c 100644
--- a/integrationtests/gitjob/controller/suite_test.go
+++ b/integrationtests/gitjob/controller/suite_test.go
@@ -170,6 +170,13 @@ var _ = BeforeSuite(func() {
 	}).SetupWithManager(mgr)
 	Expect(err).ToNot(HaveOccurred(), "failed to set up manager")
 
+	err = (&ctrlreconciler.BundleDeploymentReconciler{
+		Client:  mgr.GetClient(),
+		Scheme:  mgr.GetScheme(),
+		Workers: 50,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred(), "failed to set up manager")
+
 	go func() {
 		defer GinkgoRecover()
 		defer ctlr.Finish()
diff --git a/integrationtests/helmops/controller/suite_test.go b/integrationtests/helmops/controller/suite_test.go
index d5fcfb8d..809dbc54 100644
--- a/integrationtests/helmops/controller/suite_test.go
+++ b/integrationtests/helmops/controller/suite_test.go
@@ -100,6 +100,13 @@ var _ = BeforeSuite(func() {
 	}).SetupWithManager(mgr)
 	Expect(err).ToNot(HaveOccurred(), "failed to set up manager")
 
+	err = (&ctrlreconciler.BundleDeploymentReconciler{
+		Client:  mgr.GetClient(),
+		Scheme:  mgr.GetScheme(),
+		Workers: 50,
+	}).SetupWithManager(mgr)
+	Expect(err).ToNot(HaveOccurred(), "failed to set up manager")
+
 	go func() {
 		defer GinkgoRecover()
 		defer ctlr.Finish()