fix: Set operation state to nil if new operation has been initiated b…

…y a direct object update (argoproj#20875) Fixes argoproj#20875 Some systems like Kargo update the application operation directly to initiate a new sync, bypassing the server. However, they can't update the status operation state to become nil, resulting in discrepancy, see akuity/kargo#2985 and the discussion. Handle this case on the app controller side. Let's cherry-pick to v2.13. Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada · Nov 24, 2024 · 1189c95 · 1189c95
1 parent 6d8d32f
commit 1189c95
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 0 deletions.
diff --git a/controller/appcontroller.go b/controller/appcontroller.go
@@ -1927,6 +1927,27 @@ func (ctrl *ApplicationController) persistAppStatus(orig *appv1.Application, new
 	return patchMs
 }
 
+// resetAppStatusOperationState updates the app status operation state to be nil. If no changes were made, it is a no-op
+func (ctrl *ApplicationController) resetAppStatusOperationState(orig *appv1.Application) {
+	logCtx := getAppLog(orig).WithField("method", "resetAppStatusOperationState")
+	if orig.Status.OperationState == nil {
+		return
+	}
+	newStatus := orig.Status.DeepCopy()
+	newStatus.OperationState = nil
+	patch, _, err := createMergePatch(
+		&appv1.Application{Status: orig.Status},
+		&appv1.Application{Status: *newStatus})
+	if err != nil {
+		logCtx.Errorf("Error constructing app status patch: %v", err)
+		return
+	}
+	_, err = ctrl.PatchAppWithWriteBack(context.Background(), orig.Name, orig.Namespace, types.MergePatchType, patch, metav1.PatchOptions{})
+	if err != nil {
+		logCtx.Warnf("Error updating application: %v", err)
+	}
+}
+
 // autoSync will initiate a sync operation for an application configured with automated sync
 func (ctrl *ApplicationController) autoSync(app *appv1.Application, syncStatus *appv1.SyncStatus, resources []appv1.ResourceStatus, revisionUpdated bool) (*appv1.ApplicationCondition, time.Duration) {
 	logCtx := getAppLog(app)
@@ -2318,6 +2339,11 @@ func (ctrl *ApplicationController) newApplicationInformerAndLister() (cache.Shar
 						jitter := time.Duration(float64(ctrl.statusRefreshJitter) * rand.Float64())
 						delay = &jitter
 					}
+					// Something updated application operation state bypassing the controller and
+					// operation state in status still reflects the old sync. Reset it.
+					if oldApp.Operation == nil && newApp.Operation != nil && newApp.Status.OperationState != nil {
+						ctrl.resetAppStatusOperationState(newApp)
+					}
 				}
 
 				ctrl.requestAppRefresh(newApp.QualifiedName(), compareWith, delay)

diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go
@@ -3001,3 +3001,35 @@ func TestDeletionConfirmation(t *testing.T) {
 		When().ConfirmDeletion().
 		Then().Expect(DoesNotExist())
 }
+
+func TestNewOperationTriggeredByUpdatingAppObject(t *testing.T) {
+	Given(t).
+		Path(guestbookPath).
+		When().
+		CreateApp().
+		Sync().
+		Then().
+		Expect(OperationPhaseIs(OperationSucceeded)).
+		Expect(SyncStatusIs(SyncStatusCodeSynced)).
+		Expect(HealthIs(health.HealthStatusHealthy)).
+		When().
+		PatchAppHttp(`{"operation": {"sync": { "revisions": ["test"] }, "initiatedBy": {"username": "test"}}}`).
+		// PatchApp(`[
+		// 	{
+		// 		"op": "add",
+		// 		"path": "/operation",
+		// 		"value": {
+		// 			"sync":	{
+		// 				"revisions": ["test"]
+		// 			},
+		// 			"initiatedBy": {
+		// 				"username": "test"
+		// 			}
+		// 		}
+		// 	}
+		// ]`).
+		Then().
+		And(func(app *Application) {
+			assert.Nil(t, app.Status.OperationState)
+		})
+}