fix: ResourceGroup resource status bugs (#1576)

- Fixed a bug that was causing the status field to remain Current
  after the object had successfully reconciled, even if the object
  later became InProgress, Terminating, or Failed. This will cause
  nomos status and the Cloud Console UI to correctly show the current
  status after initial reconciliation, rather than just the status
  based on the last time the Applier ran.
- Fixed a bug that was causing the status field to become Unknown
  when the object is actually Terminating when the actuation is
  empty/unknown, pending, skipped, or failed. Now, the Terminating
  and NotFound statuses should be correctly exposed for previously
  applied objects, even if the object spec has not been updated to
  match the source of truth yet.
- Refactored ActuationStatusToLegacy to be easier to read, with extra
  comments to explain the reasoning.
- Adjusted the test expectations for the new behavior

Author: karlkfi

pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller.go

@@ -327,16 +327,16 @@ func (r *reconciler) computeStatus(
 			hasErr = true
 		}
 
-		// Update the legacy status field based on the actuation, strategy and reconcile
-		// statuses set by cli-utils. If the actuation is not successful, update the legacy
-		// status field to be of unknown status.
+		// Keep the existing object statuses written by cli-utils
 		aStatus, exists := actuationStatuses[resStatus.ObjMetadata]
 		if exists {
 			resStatus.Actuation = aStatus.Actuation
 			resStatus.Strategy = aStatus.Strategy
 			resStatus.Reconcile = aStatus.Reconcile
 
-			resStatus.Status = ActuationStatusToLegacy(resStatus)
+			// Update the status field to reflect source -> spec -> status,
+			// not just spec -> status.
+			resStatus.Status = UpdateStatusToReflectActuation(resStatus)
 		}
 
 		// add the resource status into resgroup
@@ -350,23 +350,45 @@ func (r *reconciler) computeStatus(
 	return statuses
 }
 
-// ActuationStatusToLegacy contains the logic/rules to convert from the actuation statuses
-// to the legacy status field. If conversion is not needed, the original status field is returned
-// instead.
-func ActuationStatusToLegacy(s v1alpha1.ResourceStatus) v1alpha1.Status {
-	if s.Status == v1alpha1.NotFound {
-		return v1alpha1.NotFound
+// UpdateStatusToReflectActuation updates the latest computed kstatus to reflect
+// the desired state in the source of truth, as much as possible, not just the
+// desired state that has been persisted to the Kubernetes API.
+//
+// This is necessary, because kstatus only reflects whether the object status
+// reflects the object spec. But in Config Sync, the desired state is actually
+// in the source or truth. So if the latest desired state has not yet been
+// successfully actuated (applied or deleted), then the current status is
+// Unknown, because neither Kubernetes nor this ResourceGroup controller knows
+// what the desired state is.
+func UpdateStatusToReflectActuation(s v1alpha1.ResourceStatus) v1alpha1.Status {
+	// When actuation is unknown, return the latest computed status.
+	// This should only ever happen when upgrading from an old version.
+	// It means the applier was last run with code that predates the addition
+	// of the strategy, actuation, and reconcile fields.
+	if s.Actuation == "" {
+		return s.Status
 	}
 
-	if s.Actuation != "" &&
-		s.Actuation != v1alpha1.ActuationSucceeded {
-		return v1alpha1.Unknown
+	// When actuation has succeeded, we know the object spec is up to date with
+	// the latest source, so the latest computed status should be correct.
+	if s.Actuation == v1alpha1.ActuationSucceeded {
+		return s.Status
 	}
 
-	if s.Actuation == v1alpha1.ActuationSucceeded && s.Reconcile == v1alpha1.ReconcileSucceeded {
-		return v1alpha1.Current
+	// When the latest computed status is Terminating or NotFound, keep it,
+	// even if actuation is Pending, Skipped, or Failed, because Terminating and
+	// NotFound are independent of the desired state in the source of truth,
+	// while InProgress, Current, and Failed depend on actuation having
+	// successfully updated the spec on the cluster.
+	if s.Status == v1alpha1.NotFound || s.Status == v1alpha1.Terminating {
+		return s.Status
 	}
-	return s.Status
+
+	// When actuation is not successful (Pending, Skipped, Failed), that means
+	// the object spec has not been updated to the desired spec from source yet,
+	// so the computed status can be ignored and the real status is Unknown,
+	// unless the latest computed status is NotFound or Terminating.
+	return v1alpha1.Unknown
 }
 
 // setResStatus updates a resource status struct using values within the cached status struct.

pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller_test.go

@@ -474,65 +474,106 @@ func TestReconcileTimeout(t *testing.T) {
 	}
 }
 
-func TestActuationStatusToLegacy(t *testing.T) {
+func TestUpdateStatusToReflectActuation(t *testing.T) {
 	tests := []struct {
 		name      string
 		resStatus v1alpha1.ResourceStatus
 		want      v1alpha1.Status
 	}{
 		{
-			"Status should equal current status when actuation is status is successful",
-			v1alpha1.ResourceStatus{
+			name: "Status should equal current status when actuation is status is successful",
+			resStatus: v1alpha1.ResourceStatus{
 				Status:    v1alpha1.Current,
 				Actuation: v1alpha1.ActuationSucceeded,
 			},
-			v1alpha1.Current,
+			want: v1alpha1.Current,
 		},
 		{
-			"Return status field when actuation is status is empty",
-			v1alpha1.ResourceStatus{
+			name: "Return status field when actuation is status is empty",
+			resStatus: v1alpha1.ResourceStatus{
 				Status: v1alpha1.InProgress,
 			},
-			v1alpha1.InProgress,
+			want: v1alpha1.InProgress,
 		},
 		{
-			"Return unknown when actuation is not successful",
-			v1alpha1.ResourceStatus{
+			name: "Return unknown when actuation is not successful",
+			resStatus: v1alpha1.ResourceStatus{
 				Actuation: v1alpha1.ActuationPending,
 			},
-			v1alpha1.Unknown,
+			want: v1alpha1.Unknown,
 		},
 		{
-			"Return not found when status is not found already",
-			v1alpha1.ResourceStatus{
+			name: "Return not found when status is not found already",
+			resStatus: v1alpha1.ResourceStatus{
 				Status:    v1alpha1.NotFound,
 				Actuation: v1alpha1.ActuationPending,
 			},
-			v1alpha1.NotFound,
+			want: v1alpha1.NotFound,
 		},
 		{
-			"Return not found when status is not found already - disregard actuation success",
-			v1alpha1.ResourceStatus{
+			name: "Return not found when status is not found already - disregard actuation success",
+			resStatus: v1alpha1.ResourceStatus{
 				Status:    v1alpha1.NotFound,
 				Actuation: v1alpha1.ActuationSucceeded,
 			},
-			v1alpha1.NotFound,
+			want: v1alpha1.NotFound,
 		},
 		{
-			"Return Current if both Actuation and Reconcile succeeded",
-			v1alpha1.ResourceStatus{
-				Status:    v1alpha1.Unknown,
+			name: "Return Terminating when Terminating even if apply and reconcile previously succeeded",
+			resStatus: v1alpha1.ResourceStatus{
+				Status:    v1alpha1.Terminating,
+				Strategy:  v1alpha1.Apply,
 				Actuation: v1alpha1.ActuationSucceeded,
 				Reconcile: v1alpha1.ReconcileSucceeded,
 			},
-			v1alpha1.Current,
+			want: v1alpha1.Terminating,
+		},
+		{
+			name: "Return Terminating when Terminating even if delete and reconcile previously succeeded",
+			resStatus: v1alpha1.ResourceStatus{
+				Status:    v1alpha1.Terminating,
+				Strategy:  v1alpha1.Delete,
+				Actuation: v1alpha1.ActuationSucceeded,
+				Reconcile: v1alpha1.ReconcileSucceeded,
+			},
+			want: v1alpha1.Terminating,
+		},
+		{
+			name: "Return NotFound when NotFound even if apply and reconcile previously succeeded",
+			resStatus: v1alpha1.ResourceStatus{
+				Status:    v1alpha1.NotFound,
+				Strategy:  v1alpha1.Apply,
+				Actuation: v1alpha1.ActuationSucceeded,
+				Reconcile: v1alpha1.ReconcileSucceeded,
+			},
+			want: v1alpha1.NotFound,
+		},
+		{
+			name: "Return NotFound when NotFound when strategy is delete",
+			resStatus: v1alpha1.ResourceStatus{
+				Status:    v1alpha1.NotFound,
+				Strategy:  v1alpha1.Delete,
+				Actuation: v1alpha1.ActuationSucceeded,
+				Reconcile: v1alpha1.ReconcileSucceeded,
+			},
+			want: v1alpha1.NotFound,
+		},
+		{
+			name: "Return Terminating if status is Terminating and strategy is delete, even if reconcile previously succeeded",
+			resStatus: v1alpha1.ResourceStatus{
+				Status:    v1alpha1.Terminating,
+				Strategy:  v1alpha1.Delete,
+				Actuation: v1alpha1.ActuationSucceeded,
+				Reconcile: v1alpha1.ReconcileSucceeded,
+			},
+			want: v1alpha1.Terminating,
 		},
 	}
 	for _, tc := range tests {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			if got := ActuationStatusToLegacy(tc.resStatus); got != tc.want {
+			if got := UpdateStatusToReflectActuation(tc.resStatus); got != tc.want {
 				t.Errorf("ActuationStatusToLegacy() = %v, want %v", got, tc.want)
 			}
 		})

pkg/resourcegroup/controllers/root/root_controller.go

@@ -267,11 +267,11 @@ func (ResourceGroupPredicate) Update(e event.UpdateEvent) bool {
 	return statusNeedsUpdate(rgNew.Status.ResourceStatuses)
 }
 
-// statusNeedsUpdate checks each resource status to ensure the legacy status field
-// aligns with the new actuation/reconcile status fields.
+// statusNeedsUpdate checks each resource status to ensure the status field
+// reflects the actuation status.
 func statusNeedsUpdate(statuses []v1alpha1.ResourceStatus) bool {
 	for _, s := range statuses {
-		if resourcegroup.ActuationStatusToLegacy(s) != s.Status {
+		if resourcegroup.UpdateStatusToReflectActuation(s) != s.Status {
 			return true
 		}
 	}