fix(applicationset): ApplicationSets with rolling sync stuck in Pending

In issue argoproj#19535 we have discovered a bug where ApplicationSet Progressive Sync feature gets stuck and it never recovers and get stuck forever unless you manually delete the existing `applicationsStatus` of the ApplicationSet affected or trigger a manual sync.

When the ApplicationSet is performing a progressive sync, the apps in the step being synced get the status `Pending` in the ApplicationSet `applicationStatus `. This means that the apps are gonna be synced and is waiting for the sync to start progressing.

Let's set an example, applicationset generates 3 applications. In the
initial moment applicationset points to commit A

applicationset will generate those 3 applications and start the
progressive sync, then application 2 is in Pending status, the
applicationset status for application 2 is marked to targetrevision for
app2 to A

At this moment in time applicationset gets updated to point to commit B,
since app2 is in pending state the progressive sync allows it to the app
to be synced and hence the app2 is synced to commit B

since applicationset targetrevision for app2 expects to be A but it's B
will never move app2 from 'Pending' to 'Progressing' state.

[Here](https://github.com/argoproj/argo-cd/blob/master/applicationset/controllers/applicationset_controller.go#L1078) is the logic that performs this check.

This new check was introduced in ArgoCD 2.12 causing this bug when a progressive sync is already being performed.

- Because first it will get the `applicationStatus` from the existing applicationStatus which is the one that has the [old revision](https://github.com/argoproj/argo-cd/blob/master/applicationset/controllers/applicationset_controller.go#L1045)
- And since the app inside the ApplicationSet `applicationStatus` is in "Pending" the revision is never updated when it enters the [if statement](https://github.com/argoproj/argo-cd/blob/master/applicationset/controllers/applicationset_controller.go#L1069-L1092) (see how currentAppStatus.TargetRevision never will be updated)

This means that the ApplicationSet will always think that the app is not being synced to the latest version and never progress, but in reality the app is actually in a later version but tha ApplicationSet never updated it in the apps that are in "Pending".

- This PR fixes this bug changing the logic that checks when an
  applications needs to be moved from Pending to Progressing, instead of
rely on the targetrevision we actually rely just in the application
being synced to move it. This also don't introduce a prior bug where it
was cheched that the application was synced in a certain moment in time
to ensure it was triggered by the applicationset controller.

- Note that if someone manually sync one application of the
  applicationset while it's being progresively synced after merging this
PR the applciationset controller will continue the rollout

- Ensure that a certain revision is applied orderly in all applications
  generated from the applicationset it's certainly possible that a given
application can be synced in a different revision than the one
explicitly set in the appset

Co-authored-by: Thibault Jamet <[email protected]>
Co-authored-by: Carlos Rejano <[email protected]>

applicationset/controllers/applicationset_controller.go

@@ -1038,18 +1038,21 @@ func (r *ApplicationSetReconciler) updateApplicationSetApplicationStatus(ctx con
 				Message:            "No Application status found, defaulting status to Waiting.",
 				Status:             "Waiting",
 				Step:               fmt.Sprint(appStepMap[app.Name] + 1),
-				TargetRevisions:    app.Status.GetRevisions(),
 			}
 		} else {
 			// we have an existing AppStatus
 			currentAppStatus = applicationSet.Status.ApplicationStatus[idx]
-
-			// upgrade any existing AppStatus that might have been set by an older argo-cd version
-			// note: currentAppStatus.TargetRevisions may be set to empty list earlier during migrations,
-			// to prevent other usage of r.Client.Status().Update to fail before reaching here.
-			if len(currentAppStatus.TargetRevisions) == 0 {
-				currentAppStatus.TargetRevisions = app.Status.GetRevisions()
+			if !reflect.DeepEqual(currentAppStatus.TargetRevisions, app.Status.GetRevisions()) {
+				currentAppStatus.Message = "Application has pending changes, setting status to Waiting."
 			}
+
+		}
+
+		if !reflect.DeepEqual(currentAppStatus.TargetRevisions, app.Status.GetRevisions()) {
+			currentAppStatus.TargetRevisions = app.Status.GetRevisions()
+			currentAppStatus.Status = "Waiting"
+			currentAppStatus.LastTransitionTime = &now
+			currentAppStatus.Step = fmt.Sprint(appStepMap[currentAppStatus.Application] + 1)
 		}
 
 		appOutdated := false
@@ -1058,30 +1061,21 @@ func (r *ApplicationSetReconciler) updateApplicationSetApplicationStatus(ctx con
 		}
 
 		if appOutdated && currentAppStatus.Status != "Waiting" && currentAppStatus.Status != "Pending" {
+
 			logCtx.Infof("Application %v is outdated, updating its ApplicationSet status to Waiting", app.Name)
 			currentAppStatus.LastTransitionTime = &now
 			currentAppStatus.Status = "Waiting"
 			currentAppStatus.Message = "Application has pending changes, setting status to Waiting."
 			currentAppStatus.Step = fmt.Sprint(appStepMap[currentAppStatus.Application] + 1)
-			currentAppStatus.TargetRevisions = app.Status.GetRevisions()
 		}
 
 		if currentAppStatus.Status == "Pending" {
-			if operationPhaseString == "Succeeded" {
-				revisions := []string{}
-				if len(app.Status.OperationState.SyncResult.Revisions) > 0 {
-					revisions = app.Status.OperationState.SyncResult.Revisions
-				} else if app.Status.OperationState.SyncResult.Revision != "" {
-					revisions = append(revisions, app.Status.OperationState.SyncResult.Revision)
-				}
-
-				if reflect.DeepEqual(currentAppStatus.TargetRevisions, revisions) {
-					logCtx.Infof("Application %v has completed a sync successfully, updating its ApplicationSet status to Progressing", app.Name)
-					currentAppStatus.LastTransitionTime = &now
-					currentAppStatus.Status = "Progressing"
-					currentAppStatus.Message = "Application resource completed a sync successfully, updating status from Pending to Progressing."
-					currentAppStatus.Step = fmt.Sprint(appStepMap[currentAppStatus.Application] + 1)
-				}
+			if !appOutdated && operationPhaseString == "Succeeded" {
+				logCtx.Infof("Application %v has completed a sync successfully, updating its ApplicationSet status to Progressing", app.Name)
+				currentAppStatus.LastTransitionTime = &now
+				currentAppStatus.Status = "Progressing"
+				currentAppStatus.Message = "Application resource completed a sync successfully, updating status from Pending to Progressing."
+				currentAppStatus.Step = fmt.Sprint(appStepMap[currentAppStatus.Application] + 1)
 			} else if operationPhaseString == "Running" || healthStatusString == "Progressing" {
 				logCtx.Infof("Application %v has entered Progressing status, updating its ApplicationSet status to Progressing", app.Name)
 				currentAppStatus.LastTransitionTime = &now

applicationset/controllers/applicationset_controller_test.go

@@ -4585,6 +4585,9 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 						Health: v1alpha1.HealthStatus{
 							Status: health.HealthStatusProgressing,
 						},
+						Sync: v1alpha1.SyncStatus{
+							Revision: "Next",
+						},
 					},
 				},
 			},
@@ -4636,7 +4639,8 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 							Phase: common.OperationRunning,
 						},
 						Sync: v1alpha1.SyncStatus{
-							Status: v1alpha1.SyncStatusCodeSynced,
+							Status:   v1alpha1.SyncStatusCodeSynced,
+							Revision: "Current",
 						},
 					},
 				},
@@ -4689,7 +4693,8 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 							Phase: common.OperationSucceeded,
 						},
 						Sync: v1alpha1.SyncStatus{
-							Status: v1alpha1.SyncStatusCodeSynced,
+							Status:   v1alpha1.SyncStatusCodeSynced,
+							Revision: "Next",
 						},
 					},
 				},
@@ -4742,7 +4747,8 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 							Phase: common.OperationSucceeded,
 						},
 						Sync: v1alpha1.SyncStatus{
-							Status: v1alpha1.SyncStatusCodeSynced,
+							Revision: "Current",
+							Status:   v1alpha1.SyncStatusCodeSynced,
 						},
 					},
 				},
@@ -4946,7 +4952,7 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 			},
 		},
 		{
-			name: "does not progresses a pending application with a successful sync triggered by controller with invalid revision to progressing",
+			name: "removes the appStatus for applications that no longer exist",
 			appSet: v1alpha1.ApplicationSet{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "name",
@@ -4961,14 +4967,18 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 				Status: v1alpha1.ApplicationSetStatus{
 					ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
 						{
-							Application: "app1",
-							LastTransitionTime: &metav1.Time{
-								Time: time.Now().Add(time.Duration(-1) * time.Minute),
-							},
-							Message:         "",
-							Status:          "Pending",
+							Application:     "app1",
+							Message:         "Application has pending changes, setting status to Waiting.",
+							Status:          "Waiting",
 							Step:            "1",
-							TargetRevisions: []string{"Next"},
+							TargetRevisions: []string{"Current"},
+						},
+						{
+							Application:     "app2",
+							Message:         "Application has pending changes, setting status to Waiting.",
+							Status:          "Waiting",
+							Step:            "1",
+							TargetRevisions: []string{"Current"},
 						},
 					},
 				},
@@ -4980,41 +4990,30 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 					},
 					Status: v1alpha1.ApplicationStatus{
 						Health: v1alpha1.HealthStatus{
-							Status: health.HealthStatusDegraded,
+							Status: health.HealthStatusHealthy,
 						},
 						OperationState: &v1alpha1.OperationState{
 							Phase: common.OperationSucceeded,
-							StartedAt: metav1.Time{
-								Time: time.Now(),
-							},
-							Operation: v1alpha1.Operation{
-								InitiatedBy: v1alpha1.OperationInitiator{
-									Username:  "applicationset-controller",
-									Automated: true,
-								},
-							},
-							SyncResult: &v1alpha1.SyncOperationResult{
-								Revision: "Previous",
-							},
 						},
 						Sync: v1alpha1.SyncStatus{
-							Status: v1alpha1.SyncStatusCodeSynced,
+							Status:   v1alpha1.SyncStatusCodeSynced,
+							Revision: "Current",
 						},
 					},
 				},
 			},
 			expectedAppStatus: []v1alpha1.ApplicationSetApplicationStatus{
 				{
 					Application:     "app1",
-					Message:         "",
-					Status:          "Pending",
+					Message:         "Application resource is already Healthy, updating status from Waiting to Healthy.",
+					Status:          "Healthy",
 					Step:            "1",
-					TargetRevisions: []string{"Next"},
+					TargetRevisions: []string{"Current"},
 				},
 			},
 		},
 		{
-			name: "removes the appStatus for applications that no longer exist",
+			name: "progresses a pending synced application with an old revision to progressing with the Current one",
 			appSet: v1alpha1.ApplicationSet{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "name",
@@ -5030,17 +5029,10 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 					ApplicationStatus: []v1alpha1.ApplicationSetApplicationStatus{
 						{
 							Application:     "app1",
-							Message:         "Application has pending changes, setting status to Waiting.",
-							Status:          "Waiting",
-							Step:            "1",
-							TargetRevisions: []string{"Current"},
-						},
-						{
-							Application:     "app2",
-							Message:         "Application has pending changes, setting status to Waiting.",
-							Status:          "Waiting",
+							Message:         "",
+							Status:          "Pending",
 							Step:            "1",
-							TargetRevisions: []string{"Current"},
+							TargetRevisions: []string{"Old"},
 						},
 					},
 				},
@@ -5056,9 +5048,13 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
 						},
 						OperationState: &v1alpha1.OperationState{
 							Phase: common.OperationSucceeded,
+							SyncResult: &v1alpha1.SyncOperationResult{
+								Revision: "Current",
+							},
 						},
 						Sync: v1alpha1.SyncStatus{
-							Status: v1alpha1.SyncStatusCodeSynced,
+							Status:    v1alpha1.SyncStatusCodeSynced,
+							Revisions: []string{"Current"},
 						},
 					},
 				},