Retry reconcile when we cannot add finalizer to content resource

integrationtests/agent/bundle_deployment_diffs_test.go

@@ -128,7 +128,6 @@ var _ = Describe("BundleDeployment diff", func() {
 						context.TODO(),
 						types.NamespacedName{Namespace: clusterNS, Name: name},
 						bd,
-						&client.GetOptions{},
 					)
 					g.Expect(err).NotTo(HaveOccurred())
 					g.Expect(bd.Status.NonModified).To(BeTrue())
@@ -155,7 +154,6 @@ var _ = Describe("BundleDeployment diff", func() {
 						context.TODO(),
 						types.NamespacedName{Namespace: clusterNS, Name: name},
 						bd,
-						&client.GetOptions{},
 					)
 					g.Expect(err).NotTo(HaveOccurred())
 					g.Expect(bd.Status.NonModified).To(BeTrue())

integrationtests/agent/suite_test.go

@@ -69,6 +69,7 @@ func TestFleet(t *testing.T) {
 
 var _ = BeforeSuite(func() {
 	SetDefaultEventuallyTimeout(timeout)
+	SetDefaultEventuallyPollingInterval(1 * time.Second)
 
 	ctx, cancel = context.WithCancel(context.TODO())
 	testEnv = utils.NewEnvTest("../..")
@@ -257,7 +258,7 @@ type specEnv struct {
 
 func (se specEnv) isNotReadyAndModified(g Gomega, name string, modifiedStatus v1alpha1.ModifiedStatus, message string) {
 	bd := &v1alpha1.BundleDeployment{}
-	err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterNS, Name: name}, bd, &client.GetOptions{})
+	err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterNS, Name: name}, bd)
 
 	g.Expect(err).ToNot(HaveOccurred())
 
@@ -269,7 +270,7 @@ func (se specEnv) isNotReadyAndModified(g Gomega, name string, modifiedStatus v1
 
 func (se specEnv) isBundleDeploymentReadyAndNotModified(name string) bool {
 	bd := &v1alpha1.BundleDeployment{}
-	err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterNS, Name: name}, bd, &client.GetOptions{})
+	err := k8sClient.Get(context.TODO(), types.NamespacedName{Namespace: clusterNS, Name: name}, bd)
 	if err != nil {
 		return false
 	}

internal/cmd/agent/controller/bundledeployment_controller.go

@@ -134,6 +134,7 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 		merr = append(merr, fmt.Errorf("failed deploying bundle: %w", err))
 	} else {
+		logger.V(1).Info("Bundle deployed", "status", status)
 		bd.Status = setCondition(status, nil, monitor.Cond(fleetv1.BundleDeploymentConditionDeployed))
 	}
 

internal/cmd/agent/controller/drift_controller.go

@@ -88,6 +88,7 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 
 	// return early if the bundledeployment is still being installed
 	if !monitor.ShouldUpdateStatus(bd) {
+		logger.V(1).Info("BundleDeployment is still being installed")
 		return ctrl.Result{}, nil
 	}
 
@@ -99,6 +100,7 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 
 	// run drift correction
 	if len(bd.Status.ModifiedStatus) > 0 && bd.Spec.CorrectDrift != nil && bd.Spec.CorrectDrift.Enabled {
+		logger.V(1).Info("Removing external changes")
 		if release, err := r.Deployer.RemoveExternalChanges(ctx, bd); err != nil {
 			merr = append(merr, fmt.Errorf("failed reconciling drift: %w", err))
 			// Propagate drift correction error to bundle deployment status.
@@ -109,22 +111,15 @@ func (r *DriftReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
 	}
 
 	// final status update
-	logger.V(1).Info("Reconcile finished, updating the bundledeployment status")
-	err = r.updateStatus(ctx, orig, bd)
+	statusPatch := client.MergeFrom(orig)
+	err = r.Status().Patch(ctx, bd, statusPatch)
 	if apierrors.IsNotFound(err) {
 		merr = append(merr, fmt.Errorf("bundledeployment has been deleted: %w", err))
 	} else if err != nil {
 		merr = append(merr, fmt.Errorf("failed final update to bundledeployment status: %w", err))
+	} else {
+		logger.V(1).Info("Reconcile finished, updating the bundledeployment status", "newStatus", bd.Status)
 	}
 
 	return ctrl.Result{}, errutil.NewAggregate(merr)
 }
-
-func (r *DriftReconciler) updateStatus(
-	ctx context.Context,
-	orig *fleetv1.BundleDeployment,
-	obj *fleetv1.BundleDeployment,
-) error {
-	statusPatch := client.MergeFrom(orig)
-	return r.Status().Patch(ctx, obj, statusPatch)
-}

internal/cmd/agent/deployer/monitor/updatestatus.go

@@ -67,6 +67,8 @@ func isAgent(bd *fleet.BundleDeployment) bool {
 	return strings.HasPrefix(bd.Name, "fleet-agent")
 }
 
+// ShouldUpdateStatus skips resource and ready status updates if the bundle
+// deployment is unchanged or not installed yet.
 func ShouldUpdateStatus(bd *fleet.BundleDeployment) bool {
 	if bd.Spec.DeploymentID != bd.Status.AppliedDeploymentID {
 		return false
@@ -81,6 +83,9 @@ func ShouldUpdateStatus(bd *fleet.BundleDeployment) bool {
 	return true
 }
 
+// UpdateStatus updates the status of the bundledeployment based on the resources from the helm release history and the live state.
+// In the status it updates: Ready, NonReadyStatus, IncompleteState, NonReadyStatus, NonModified, ModifiedStatus, Resources and ResourceCounts fields.
+// Additionally it sets the Ready condition either from the NonReadyStatus or the NonModified status field.
 func (m *Monitor) UpdateStatus(ctx context.Context, bd *fleet.BundleDeployment, resources *helmdeployer.Resources) (fleet.BundleDeploymentStatus, error) {
 	logger := log.FromContext(ctx).WithName("update-status")
 	ctx = log.IntoContext(ctx, logger)
@@ -149,6 +154,7 @@ func readyError(status fleet.BundleDeploymentStatus) error {
 
 // updateFromPreviousDeployment updates the status with information from the
 // helm release history and an apply dry run.
+// Modified resources are resources that have changed from the previous helm release.
 func (m *Monitor) updateFromPreviousDeployment(ctx context.Context, bd *fleet.BundleDeployment, resources *helmdeployer.Resources) error {
 	resourcesPreviousRelease, err := m.deployer.ResourcesFromPreviousReleaseVersion(bd.Name, bd.Status.Release)
 	if err != nil {

internal/cmd/controller/reconciler/bundle_controller.go

@@ -218,6 +218,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			return ctrl.Result{}, err
 		}
 	}
+	logger = logger.WithValues("manifestID", manifestID)
 
 	if err := resetStatus(&bundle.Status, matchedTargets); err != nil {
 		return ctrl.Result{}, err
@@ -332,10 +333,8 @@ func (r *BundleReconciler) createBundleDeployment(
 	}
 	if target.Deployment.Namespace == "" {
 		logger.V(1).Info(
-			"Skipping bundledeployment with empty namespace, "+
-				"waiting for agentmanagement to set cluster.status.namespace",
-			"bundledeployment",
-			target.Deployment,
+			"Skipping bundledeployment with empty namespace, waiting for agentmanagement to set cluster.status.namespace",
+			"bundledeployment", target.Deployment,
 		)
 		return nil, nil
 	}
@@ -356,15 +355,13 @@ func (r *BundleReconciler) createBundleDeployment(
 	// When content resources are stored in etcd, we need to add finalizers.
 	if !contentsInOCI && !contentsInHelmChart {
 		content := &fleet.Content{}
-		err := r.Get(ctx, types.NamespacedName{Name: manifestID}, content)
-		if client.IgnoreNotFound(err) != nil {
-			logger.Error(err, "Reconcile failed to get content", "content ID", manifestID)
-			return nil, err
+		if err := r.Get(ctx, types.NamespacedName{Name: manifestID}, content); err != nil {
+			return nil, fmt.Errorf("failed to get content resource: %w", err)
 		}
 
 		if added := controllerutil.AddFinalizer(content, bd.Name); added {
 			if err := r.Update(ctx, content); err != nil {
-				logger.Error(err, "Reconcile failed to add content finalizer", "content ID", manifestID)
+				return nil, fmt.Errorf("could not add finalizer to content resource: %w", err)
 			}
 		}
 	}

pkg/apis/fleet.cattle.io/v1alpha1/bundle_types.go

@@ -312,9 +312,7 @@ const (
 	// BundleDeploymentConditionInstalled indicates the bundledeployment
 	// has been installed.
 	BundleDeploymentConditionInstalled = "Installed"
-	// BundleDeploymentConditionDeployed is used by the bundledeployment
-	// controller. It is true if the handler returns no error and false if
-	// an error is returned.
+	// BundleDeploymentConditionDeployed indicates the deployment failed.
 	BundleDeploymentConditionDeployed  = "Deployed"
 	BundleDeploymentConditionMonitored = "Monitored"
 )