argoproj · jessesuen · Sep 22, 2018
@@ -711,32 +711,26 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, comparisonRe
 		return nil
 	}
 	desiredCommitSHA := comparisonResult.Revision
+
 	// It is possible for manifests to remain OutOfSync even after a sync/kubectl apply (e.g.
 	// auto-sync with pruning disabled). We need to ensure that we do not keep Syncing an
 	// application in an infinite loop. To detect this, we only attempt the Sync if the revision
-	// and parameter overrides do *not* appear in the application's most recent history.
-	historyLen := len(app.Status.History)
-	if historyLen > 0 {
-		mostRecent := app.Status.History[historyLen-1]
-		if mostRecent.Revision == desiredCommitSHA && reflect.DeepEqual(app.Spec.Source.ComponentParameterOverrides, mostRecent.ComponentParameterOverrides) {
-			logCtx.Infof("Skipping auto-sync: most recent sync already to %s", desiredCommitSHA)
-			return nil
-		}
-	}
-	// If a sync failed, the revision will not make it's way into application history. We also need
-	// to check the operationState to see if the last operation was the one we just attempted.
-	if app.Status.OperationState != nil && app.Status.OperationState.SyncResult != nil {
-		if app.Status.OperationState.SyncResult.Revision == desiredCommitSHA {
+	// and parameter overrides are different from our most recent sync operation.
+	if alreadyAttemptedSync(app, desiredCommitSHA) {
+		if app.Status.OperationState.Phase != appv1.OperationSucceeded {
 			logCtx.Warnf("Skipping auto-sync: failed previous sync attempt to %s", desiredCommitSHA)
 			message := fmt.Sprintf("Failed sync attempt to %s: %s", desiredCommitSHA, app.Status.OperationState.Message)
 			return &appv1.ApplicationCondition{Type: appv1.ApplicationConditionSyncError, Message: message}
 		}
+		logCtx.Infof("Skipping auto-sync: most recent sync already to %s", desiredCommitSHA)
+		return nil
 	}
 
 	op := appv1.Operation{
 		Sync: &appv1.SyncOperation{
-			Revision: desiredCommitSHA,
-			Prune:    app.Spec.SyncPolicy.Automated.Prune,
+			Revision:           desiredCommitSHA,
+			Prune:              app.Spec.SyncPolicy.Automated.Prune,
+			ParameterOverrides: app.Spec.Source.ComponentParameterOverrides,
 		},
 	}
 	appIf := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(app.Namespace)
@@ -749,6 +743,21 @@ func (ctrl *ApplicationController) autoSync(app *appv1.Application, comparisonRe
 	return nil
 }
 
+// alreadyAttemptedSync returns whether or not the most recent sync was performed against the
+// commitSHA and with the same parameter overrides which are currently set in the app
+func alreadyAttemptedSync(app *appv1.Application, commitSHA string) bool {
+	if app.Status.OperationState == nil || app.Status.OperationState.Operation.Sync == nil || app.Status.OperationState.SyncResult == nil {
+		return false
+	}
+	if app.Status.OperationState.SyncResult.Revision != commitSHA {
+		return false
+	}
+	if !reflect.DeepEqual(appv1.ParameterOverrides(app.Spec.Source.ComponentParameterOverrides), app.Status.OperationState.Operation.Sync.ParameterOverrides) {
+		return false
+	}
+	return true
+}
+
 func (ctrl *ApplicationController) newApplicationInformer() cache.SharedIndexInformer {
 	appInformerFactory := appinformers.NewFilteredSharedInformerFactory(
 		ctrl.applicationClientset,

@@ -45,11 +45,24 @@ spec:
   syncPolicy:
     automated: {}
 status:
-  history:
-  - deployedAt: 2018-09-08T09:16:50Z
-    id: 0
-    params: []
-    revision: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+  operationState:
+    finishedAt: 2018-09-21T23:50:29Z
+    message: successfully synced
+    operation:
+      sync:
+        revision: HEAD
+    phase: Succeeded
+    startedAt: 2018-09-21T23:50:25Z
+    syncResult:
+      resources:
+      - kind: RoleBinding
+        message: |-
+          rolebinding.rbac.authorization.k8s.io/always-outofsync reconciled
+          rolebinding.rbac.authorization.k8s.io/always-outofsync configured
+        name: always-outofsync
+        namespace: default
+        status: Synced
+      revision: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
 `
 
 func newFakeApp() *argoappv1.Application {
@@ -123,6 +136,9 @@ func TestSkipAutoSync(t *testing.T) {
 	// Set current to 'aaaaa', desired to 'bbbbb' and add 'bbbbb' to failure history
 	app = newFakeApp()
 	app.Status.OperationState = &argoappv1.OperationState{
+		Operation: argoappv1.Operation{
+			Sync: &argoappv1.SyncOperation{},
+		},
 		Phase: argoappv1.OperationFailed,
 		SyncResult: &argoappv1.SyncOperationResult{
 			Revision: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
@@ -139,3 +155,77 @@ func TestSkipAutoSync(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Nil(t, app.Operation)
 }
+
+// TestAutoSyncIndicateError verifies we skip auto-sync and return error condition if previous sync failed
+func TestAutoSyncIndicateError(t *testing.T) {
+	app := newFakeApp()
+	app.Spec.Source.ComponentParameterOverrides = []argoappv1.ComponentParameter{
+		{
+			Name:  "a",
+			Value: "1",
+		},
+	}
+	ctrl := newFakeController(app)
+	compRes := argoappv1.ComparisonResult{
+		Status:   argoappv1.ComparisonStatusOutOfSync,
+		Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+	}
+	app.Status.OperationState = &argoappv1.OperationState{
+		Operation: argoappv1.Operation{
+			Sync: &argoappv1.SyncOperation{
+				ParameterOverrides: argoappv1.ParameterOverrides{
+					{
+						Name:  "a",
+						Value: "1",
+					},
+				},
+			},
+		},
+		Phase: argoappv1.OperationFailed,
+		SyncResult: &argoappv1.SyncOperationResult{
+			Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+		},
+	}
+	cond := ctrl.autoSync(app, &compRes)
+	assert.NotNil(t, cond)
+	app, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("argocd").Get("my-app", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Nil(t, app.Operation)
+}
+
+// TestAutoSyncParameterOverrides verifies we auto-sync if revision is same but parameter overrides are different
+func TestAutoSyncParameterOverrides(t *testing.T) {
+	app := newFakeApp()
+	app.Spec.Source.ComponentParameterOverrides = []argoappv1.ComponentParameter{
+		{
+			Name:  "a",
+			Value: "1",
+		},
+	}
+	ctrl := newFakeController(app)
+	compRes := argoappv1.ComparisonResult{
+		Status:   argoappv1.ComparisonStatusOutOfSync,
+		Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+	}
+	app.Status.OperationState = &argoappv1.OperationState{
+		Operation: argoappv1.Operation{
+			Sync: &argoappv1.SyncOperation{
+				ParameterOverrides: argoappv1.ParameterOverrides{
+					{
+						Name:  "a",
+						Value: "2", // this value changed
+					},
+				},
+			},
+		},
+		Phase: argoappv1.OperationFailed,
+		SyncResult: &argoappv1.SyncOperationResult{
+			Revision: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+		},
+	}
+	cond := ctrl.autoSync(app, &compRes)
+	assert.Nil(t, cond)
+	app, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications("argocd").Get("my-app", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.NotNil(t, app.Operation)
+}
@@ -47,7 +47,7 @@ type syncContext struct {
 }
 
 func (s *appStateManager) SyncAppState(app *appv1.Application, state *appv1.OperationState) {
-	// Sync requests are usually requested with ambiguous revisions (e.g. master, HEAD, v1.2.3).
+	// Sync requests might be requested with ambiguous revisions (e.g. master, HEAD, v1.2.3).
 	// This can change meaning when resuming operations (e.g a hook sync). After calculating a
 	// concrete git commit SHA, the SHA is remembered in the status.operationState.syncResult and
 	// rollbackResult fields. This ensures that when resuming an operation, we sync to the same
@@ -59,6 +59,7 @@ func (s *appStateManager) SyncAppState(app *appv1.Application, state *appv1.Oper
 
 	if state.Operation.Sync != nil {
 		syncOp = *state.Operation.Sync
+		overrides = []appv1.ComponentParameter(state.Operation.Sync.ParameterOverrides)
 		if state.SyncResult != nil {
 			syncRes = state.SyncResult
 			revision = state.SyncResult.Revision