fix: ResourceGroup unnecessary condition updates (#1571)

- Deprecate the Reconciling condition on the ResourceGroup, because
  it doesn't actually reflect the aggregate reconcile status of the
  objects synced by Config Sync from the source of truth. This
  condition will now always have a status of False and a reason of
  FinishReconciling to avoid breaking any clients that were waiting
  for that state. This change reduces churn on the ResourceGroup
  objects, which had been frequently flipping back and forth between
  a status of True and False. To learn how to wait for all the managed
  objects to be synced and reconciled, see
  https://cloud.google.com/kubernetes-engine/enterprise/config-sync/docs/how-to/monitor-rootsync-reposync#confirm_resources_in_the_commit_are_reconciled
- This change also removes the case that was setting the Reconciling
  condition reason to ExceedTimeout, which is an implementation
  detail of the ResourceGroup controller, has no value to the user
  and doesn't need to be exposed.
- Docs change: cl/731842416

Fixes: b/399705420

Author: karlkfi

pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller.go

@@ -99,16 +99,17 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
-	newStatus := r.startReconcilingStatus(rgObj.Status)
-	if err := r.updateStatusKptGroup(ctx, rgObj, newStatus); err != nil {
-		r.Logger(ctx).Error(err, "failed to update ResourceGroup to start reconciling")
-		return ctrl.Result{Requeue: true}, err
-	}
+	// Since this controller doesn't perform any writes in any external systems,
+	// it doesn't need to set the Reconciling condition status to True before
+	// building the new status, because there are no possible side-effects that
+	// the user might need to know about when reconciling fails.
+	// So skip initializing the Reconciling & Stalled conditions and only set
+	// them when reconciling succeeds or fails.
 
 	id := getInventoryID(rgObj.Labels)
-	newStatus = r.endReconcilingStatus(ctx, id, req.NamespacedName, rgObj.Spec, rgObj.Status, rgObj.Generation)
+	newStatus := r.endReconcilingStatus(ctx, id, req.NamespacedName, rgObj.Spec, rgObj.Status, rgObj.Generation)
 	if err := r.updateStatusKptGroup(ctx, rgObj, newStatus); err != nil {
-		r.Logger(ctx).Error(err, "failed to update ResourceGroup to finish reconciling")
+		r.Logger(ctx).Error(err, "failed to update ResourceGroup")
 		return ctrl.Result{Requeue: true}, err
 	}
 
@@ -169,19 +170,6 @@ func (r *reconciler) updateStatusKptGroup(ctx context.Context, resgroup *v1alpha
 	return r.client.Status().Update(ctx, resgroup, client.FieldOwner(FieldManager))
 }
 
-func (r *reconciler) startReconcilingStatus(status v1alpha1.ResourceGroupStatus) v1alpha1.ResourceGroupStatus {
-	// Preserve existing status and only update fields that need to change.
-	// This avoids repeatedly updating the status just to update a timestamp.
-	// It also prevents fighting with other controllers updating the status.
-	newStatus := *status.DeepCopy()
-	// Update conditions, if they've changed
-	newStatus.Conditions = updateCondition(newStatus.Conditions,
-		newReconcilingCondition(v1alpha1.TrueConditionStatus, StartReconciling, startReconcilingMsg))
-	newStatus.Conditions = updateCondition(newStatus.Conditions,
-		newStalledCondition(v1alpha1.FalseConditionStatus, "", ""))
-	return newStatus
-}
-
 func (r *reconciler) endReconcilingStatus(
 	ctx context.Context,
 	id string,
@@ -205,20 +193,24 @@ func (r *reconciler) endReconcilingStatus(
 	}()
 	select {
 	case <-finish:
+		// Updating the ObservedGeneration should be a no-op, because the
+		// applier should have updated it already.
 		newStatus.ObservedGeneration = generation
-		// Update conditions, if they've changed
-		newStatus.Conditions = updateCondition(newStatus.Conditions,
-			newReconcilingCondition(v1alpha1.FalseConditionStatus, FinishReconciling, finishReconcilingMsg))
 		newStatus.Conditions = updateCondition(newStatus.Conditions,
 			aggregateResourceStatuses(newStatus.ResourceStatuses))
 	case <-time.After(reconcileTimeout):
-		// Update conditions, if they've changed
-		newStatus.Conditions = updateCondition(newStatus.Conditions,
-			newReconcilingCondition(v1alpha1.FalseConditionStatus, ExceedTimeout, exceedTimeoutMsg))
 		newStatus.Conditions = updateCondition(newStatus.Conditions,
 			newStalledCondition(v1alpha1.TrueConditionStatus, ExceedTimeout, exceedTimeoutMsg))
 	}
 
+	// Previously, the Reconciling condition toggled between True & False while
+	// this controller was reconciling, but this caused unnecessary churn and
+	// confusion, because it didn't reflect the reconciliation status of the
+	// managed objects in status.resourceStatuses. So now it's always set to
+	// False, for reverse compatibility, in case any clients were waiting it.
+	newStatus.Conditions = updateCondition(newStatus.Conditions,
+		newReconcilingCondition(v1alpha1.FalseConditionStatus, FinishReconciling, finishReconcilingMsg))
+
 	metrics.RecordReconcileDuration(ctx, newStatus.Conditions[1].Reason, startTime)
 	updateResourceMetrics(ctx, namespacedName, newStatus.ResourceStatuses)
 	return newStatus