fix: infinite reconciliation loop when app is in error (argoproj#23067)

Signed-off-by: Alexandre Gaudreault <[email protected]>

Author: agaudreault

controller/appcontroller.go

@@ -1681,11 +1681,9 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
 
 	project, hasErrors := ctrl.refreshAppConditions(app)
 	ts.AddCheckpoint("refresh_app_conditions_ms")
-	now := metav1.Now()
 	if hasErrors {
 		app.Status.Sync.Status = appv1.SyncStatusCodeUnknown
 		app.Status.Health.Status = health.HealthStatusUnknown
-		app.Status.Health.LastTransitionTime = &now
 		patchMs = ctrl.persistAppStatus(origApp, &app.Status)
 
 		if err := ctrl.cache.SetAppResourcesTree(app.InstanceName(ctrl.namespace), &appv1.ApplicationTree{}); err != nil {
@@ -1782,6 +1780,7 @@ func (ctrl *ApplicationController) processAppRefreshQueueItem() (processNext boo
 	ts.AddCheckpoint("auto_sync_ms")
 
 	if app.Status.ReconciledAt == nil || comparisonLevel >= CompareWithLatest {
+		now := metav1.Now()
 		app.Status.ReconciledAt = &now
 	}
 	app.Status.Sync = *compareResult.syncStatus
@@ -2012,9 +2011,15 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 		ctrl.logAppEvent(context.TODO(), orig, argo.EventInfo{Reason: argo.EventReasonResourceUpdated, Type: corev1.EventTypeNormal}, message)
 	}
 	if orig.Status.Health.Status != newStatus.Health.Status {
+		now := metav1.Now()
+		newStatus.Health.LastTransitionTime = &now
 		message := fmt.Sprintf("Updated health status: %s -> %s", orig.Status.Health.Status, newStatus.Health.Status)
 		ctrl.logAppEvent(context.TODO(), orig, argo.EventInfo{Reason: argo.EventReasonResourceUpdated, Type: corev1.EventTypeNormal}, message)
+	} else {
+		// make sure the last transition time is the same and populated if the health is the same
+		newStatus.Health.LastTransitionTime = orig.Status.Health.LastTransitionTime
 	}
+
 	var newAnnotations map[string]string
 	if orig.GetAnnotations() != nil {
 		newAnnotations = make(map[string]string)

controller/appcontroller_test.go

@@ -1826,7 +1826,7 @@ apps/Deployment:
     hs = {}
     hs.status = ""
     hs.message = ""
-    
+
     if obj.metadata ~= nil then
       if obj.metadata.labels ~= nil then
         current_status = obj.metadata.labels["status"]

controller/health.go

@@ -8,7 +8,6 @@ import (
 	"github.com/argoproj/gitops-engine/pkg/sync/ignore"
 	kubeutil "github.com/argoproj/gitops-engine/pkg/utils/kube"
 	log "github.com/sirupsen/logrus"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/argoproj/argo-cd/v3/common"
@@ -22,7 +21,9 @@ func setApplicationHealth(resources []managedResource, statuses []appv1.Resource
 	var savedErr error
 	var errCount uint
 
-	appHealth := appv1.HealthStatus{Status: health.HealthStatusHealthy}
+	appHealth := app.Status.Health.DeepCopy()
+	appHealth.Status = health.HealthStatusHealthy
+
 	for i, res := range resources {
 		if res.Target != nil && hookutil.Skip(res.Target) {
 			continue
@@ -80,13 +81,6 @@ func setApplicationHealth(resources []managedResource, statuses []appv1.Resource
 			appHealth.Status = healthStatus.Status
 		}
 	}
-	// if the status didn't change, don't update the timestamp
-	if app.Status.Health.Status == appHealth.Status && app.Status.Health.LastTransitionTime != nil {
-		appHealth.LastTransitionTime = app.Status.Health.LastTransitionTime
-	} else {
-		now := metav1.Now()
-		appHealth.LastTransitionTime = &now
-	}
 	if persistResourceHealth {
 		app.Status.ResourceHealthSource = appv1.ResourceHealthLocationInline
 	} else {
@@ -95,5 +89,5 @@ func setApplicationHealth(resources []managedResource, statuses []appv1.Resource
 	if savedErr != nil && errCount > 1 {
 		savedErr = fmt.Errorf("see application-controller logs for %d other errors; most recent error was: %w", errCount-1, savedErr)
 	}
-	return &appHealth, savedErr
+	return appHealth, savedErr
 }

controller/health_test.go

@@ -73,17 +73,15 @@ func TestSetApplicationHealth(t *testing.T) {
 	assert.NotNil(t, healthStatus.LastTransitionTime)
 	assert.Nil(t, resourceStatuses[0].Health.LastTransitionTime)
 	assert.Nil(t, resourceStatuses[1].Health.LastTransitionTime)
-	previousLastTransitionTime := healthStatus.LastTransitionTime
 	app.Status.Health = *healthStatus
 
 	// now mark the job as a hook and retry. it should ignore the hook and consider the app healthy
 	failedJob.SetAnnotations(map[string]string{synccommon.AnnotationKeyHook: "PreSync"})
 	healthStatus, err = setApplicationHealth(resources, resourceStatuses, nil, app, true)
 	require.NoError(t, err)
 	assert.Equal(t, health.HealthStatusHealthy, healthStatus.Status)
-	// change in health, timestamp should change
-	assert.NotEqual(t, *previousLastTransitionTime, *healthStatus.LastTransitionTime)
-	previousLastTransitionTime = healthStatus.LastTransitionTime
+	// timestamp should be the same in case health did not change
+	assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 	app.Status.Health = *healthStatus
 
 	// now we set the `argocd.argoproj.io/ignore-healthcheck: "true"` annotation on the job's target.
@@ -94,8 +92,7 @@ func TestSetApplicationHealth(t *testing.T) {
 	healthStatus, err = setApplicationHealth(resources, resourceStatuses, nil, app, true)
 	require.NoError(t, err)
 	assert.Equal(t, health.HealthStatusHealthy, healthStatus.Status)
-	// no change in health, timestamp shouldn't change
-	assert.Equal(t, *previousLastTransitionTime, *healthStatus.LastTransitionTime)
+	assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 }
 
 func TestSetApplicationHealth_ResourceHealthNotPersisted(t *testing.T) {
@@ -125,7 +122,7 @@ func TestSetApplicationHealth_MissingResource(t *testing.T) {
 	healthStatus, err := setApplicationHealth(resources, resourceStatuses, lua.ResourceHealthOverrides{}, app, true)
 	require.NoError(t, err)
 	assert.Equal(t, health.HealthStatusMissing, healthStatus.Status)
-	assert.False(t, healthStatus.LastTransitionTime.IsZero())
+	assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 }
 
 func TestSetApplicationHealth_HealthImproves(t *testing.T) {
@@ -157,7 +154,7 @@ func TestSetApplicationHealth_HealthImproves(t *testing.T) {
 			healthStatus, err := setApplicationHealth(resources, resourceStatuses, overrides, app, true)
 			require.NoError(t, err)
 			assert.Equal(t, tc.newStatus, healthStatus.Status)
-			assert.NotEqual(t, testTimestamp, *healthStatus.LastTransitionTime)
+			assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 		})
 	}
 }
@@ -174,6 +171,7 @@ func TestSetApplicationHealth_MissingResourceNoBuiltHealthCheck(t *testing.T) {
 		healthStatus, err := setApplicationHealth(resources, resourceStatuses, lua.ResourceHealthOverrides{}, app, true)
 		require.NoError(t, err)
 		assert.Equal(t, health.HealthStatusHealthy, healthStatus.Status)
+		assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 		assert.Equal(t, health.HealthStatusMissing, resourceStatuses[0].Health.Status)
 	})
 
@@ -185,7 +183,7 @@ func TestSetApplicationHealth_MissingResourceNoBuiltHealthCheck(t *testing.T) {
 		}, app, true)
 		require.NoError(t, err)
 		assert.Equal(t, health.HealthStatusMissing, healthStatus.Status)
-		assert.False(t, healthStatus.LastTransitionTime.IsZero())
+		assert.Equal(t, app.Status.Health.LastTransitionTime, healthStatus.LastTransitionTime)
 	})
 }
 

controller/state.go

@@ -518,15 +518,14 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 
 	// return unknown comparison result if basic comparison settings cannot be loaded
 	if err != nil {
-		now := metav1.Now()
 		if hasMultipleSources {
 			return &comparisonResult{
 				syncStatus: &v1alpha1.SyncStatus{
 					ComparedTo: app.Spec.BuildComparedToStatus(),
 					Status:     v1alpha1.SyncStatusCodeUnknown,
 					Revisions:  revisions,
 				},
-				healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown, LastTransitionTime: &now},
+				healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown},
 			}, nil
 		}
 		return &comparisonResult{
@@ -535,7 +534,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
 				Status:     v1alpha1.SyncStatusCodeUnknown,
 				Revision:   revisions[0],
 			},
-			healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown, LastTransitionTime: &now},
+			healthStatus: &v1alpha1.HealthStatus{Status: health.HealthStatusUnknown},
 		}, nil
 	}
 

controller/state_test.go

@@ -718,7 +718,7 @@ func TestSetHealth(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status)
-	assert.False(t, compRes.healthStatus.LastTransitionTime.IsZero())
+	assert.Equal(t, app.Status.Health.LastTransitionTime, compRes.healthStatus.LastTransitionTime)
 }
 
 func TestPreserveStatusTimestamp(t *testing.T) {
@@ -793,7 +793,7 @@ func TestSetHealthSelfReferencedApp(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Equal(t, health.HealthStatusHealthy, compRes.healthStatus.Status)
-	assert.False(t, compRes.healthStatus.LastTransitionTime.IsZero())
+	assert.Equal(t, app.Status.Health.LastTransitionTime, compRes.healthStatus.LastTransitionTime)
 }
 
 func TestSetManagedResourcesWithOrphanedResources(t *testing.T) {
@@ -869,7 +869,7 @@ func TestReturnUnknownComparisonStateOnSettingLoadError(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Equal(t, health.HealthStatusUnknown, compRes.healthStatus.Status)
-	assert.False(t, compRes.healthStatus.LastTransitionTime.IsZero())
+	assert.Equal(t, app.Status.Health.LastTransitionTime, compRes.healthStatus.LastTransitionTime)
 	assert.Equal(t, v1alpha1.SyncStatusCodeUnknown, compRes.syncStatus.Status)
 }
 

test/e2e/app_management_test.go

@@ -3029,3 +3029,46 @@ func TestDeletionConfirmation(t *testing.T) {
 		When().ConfirmDeletion().
 		Then().Expect(DoesNotExist())
 }
+
+func TestLastTransitionTimeUnchangedError(t *testing.T) {
+	// Ensure that, if the health status hasn't changed, the lastTransitionTime is not updated.
+
+	ctx := Given(t)
+	ctx.
+		Path(guestbookPath).
+		When().
+		And(func() {
+			// Manually create an application with an outdated reconciledAt field
+			manifest := fmt.Sprintf(`
+apiVersion: argoproj.io/v1alpha1
+kind: Application
+metadata:
+  name: %s
+spec:
+  project: default
+  source:
+    repoURL: %s
+    path: guestbook
+    targetRevision: HEAD
+  destination:
+    server: https://non-existent-cluster
+    namespace: default
+status:
+  reconciledAt: "2023-01-01T00:00:00Z"
+  health:
+    status: Unknown
+    lastTransitionTime: "2023-01-01T00:00:00Z"
+`, ctx.AppName(), fixture.RepoURL(fixture.RepoURLTypeFile))
+			_, err := fixture.RunWithStdin(manifest, "", "kubectl", "apply", "-n", fixture.ArgoCDNamespace, "-f", "-")
+			require.NoError(t, err)
+		}).
+		Refresh(RefreshTypeNormal).
+		Then().
+		And(func(app *Application) {
+			// Verify the health status is still Unknown
+			assert.Equal(t, health.HealthStatusUnknown, app.Status.Health.Status)
+
+			// Verify the lastTransitionTime has not been updated
+			assert.Equal(t, "2023-01-01T00:00:00Z", app.Status.Health.LastTransitionTime.UTC().Format(time.RFC3339))
+		})
+}