fix: evaluate all possible refresh reasons for multi-source apps (arg…

…oproj#12379) (argoproj#12576) * fix: evaluate all possible refresh reasons for multi-source apps (argoproj#12379) Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * remove redundant parentheses Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * tests Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * don't auto-sync, it makes tests flaky Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * auto-sync because sync CLI doesn't work for multi-source apps Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * don't require out-of-sync - app may sync quickly Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * timeout 60 Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * fix timeout Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
yyzxw · Aug 9, 2023 · d7e664b · d7e664b
1 parent e5a9645
commit d7e664b
Show file tree

Hide file tree

Showing 7 changed files with 214 additions and 105 deletions.
diff --git a/controller/appcontroller.go b/controller/appcontroller.go
@@ -1475,6 +1475,13 @@ func resourceStatusKey(res appv1.ResourceStatus) string {
 	return strings.Join([]string{res.Group, res.Kind, res.Namespace, res.Name}, "/")
 }
 
+func currentSourceEqualsSyncedSource(app *appv1.Application) bool {
+	if app.Spec.HasMultipleSources() {
+		return app.Spec.Sources.Equals(app.Status.Sync.ComparedTo.Sources)
+	}
+	return app.Spec.Source.Equals(app.Status.Sync.ComparedTo.Source)
+}
+
 // needRefreshAppStatus answers if application status needs to be refreshed.
 // Returns true if application never been compared, has changed or comparison result has expired.
 // Additionally returns whether full refresh was requested or not.
@@ -1493,14 +1500,12 @@ func (ctrl *ApplicationController) needRefreshAppStatus(app *appv1.Application,
 		refreshType = requestedType
 		reason = fmt.Sprintf("%s refresh requested", refreshType)
 	} else {
-		if app.Spec.HasMultipleSources() {
-			if (len(app.Spec.Sources) != len(app.Status.Sync.ComparedTo.Sources)) || !reflect.DeepEqual(app.Spec.Sources, app.Status.Sync.ComparedTo.Sources) {
-				reason = "atleast one of the spec.sources differs"
-				compareWith = CompareWithLatestForceResolve
-			}
-		} else if !app.Spec.Source.Equals(app.Status.Sync.ComparedTo.Source) {
+		if !currentSourceEqualsSyncedSource(app) {
 			reason = "spec.source differs"
 			compareWith = CompareWithLatestForceResolve
+			if app.Spec.HasMultipleSources() {
+				reason = "at least one of the spec.sources differs"
+			}
 		} else if hardExpired || softExpired {
 			// The commented line below mysteriously crashes if app.Status.ReconciledAt is nil
 			// reason = fmt.Sprintf("comparison expired. reconciledAt: %v, expiry: %v", app.Status.ReconciledAt, statusRefreshTimeout)

diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go
@@ -209,6 +209,55 @@ status:
         repoURL: https://github.com/argoproj/argocd-example-apps.git
 `
 
+var fakeMultiSourceApp = `
+apiVersion: argoproj.io/v1alpha1
+kind: Application
+metadata:
+  uid: "123"
+  name: my-app
+  namespace: ` + test.FakeArgoCDNamespace + `
+spec:
+  destination:
+    namespace: ` + test.FakeDestNamespace + `
+    server: https://localhost:6443
+  project: default
+  sources:
+  - path: some/path
+    repoURL: https://github.com/argoproj/argocd-example-apps.git
+  - path: some/other/path
+    repoURL: https://github.com/argoproj/argocd-example-apps-fake.git
+  syncPolicy:
+    automated: {}
+status:
+  operationState:
+    finishedAt: 2018-09-21T23:50:29Z
+    message: successfully synced
+    operation:
+      sync:
+        revisions:
+        - HEAD
+        - 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
+      revisions: 
+      - aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+      - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+      sources:
+      - path: some/path
+        repoURL: https://github.com/argoproj/argocd-example-apps.git
+      - path: some/other/path
+        repoURL: https://github.com/argoproj/argocd-example-apps-fake.git
+`
+
 var fakeAppWithDestName = `
 apiVersion: argoproj.io/v1alpha1
 kind: Application
@@ -263,6 +312,10 @@ func newFakeApp() *argoappv1.Application {
 	return createFakeApp(fakeApp)
 }
 
+func newFakeMultiSourceApp() *argoappv1.Application {
+	return createFakeApp(fakeMultiSourceApp)
+}
+
 func newFakeAppWithDestMismatch() *argoappv1.Application {
 	return createFakeApp(fakeAppWithDestMismatch)
 }
@@ -857,101 +910,133 @@ func TestSetOperationStateOnDeletedApp(t *testing.T) {
 }
 
 func TestNeedRefreshAppStatus(t *testing.T) {
-	ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})
-
-	app := newFakeApp()
-	now := metav1.Now()
-	app.Status.ReconciledAt = &now
-	app.Status.Sync = argoappv1.SyncStatus{
-		Status: argoappv1.SyncStatusCodeSynced,
-		ComparedTo: argoappv1.ComparedTo{
-			Source:      app.Spec.GetSource(),
-			Destination: app.Spec.Destination,
+	testCases := []struct {
+		name string
+		app  *argoappv1.Application
+	}{
+		{
+			name: "single-source app",
+			app:  newFakeApp(),
+		},
+		{
+			name: "multi-source app",
+			app:  newFakeMultiSourceApp(),
 		},
 	}
 
-	// no need to refresh just reconciled application
-	needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
-	assert.False(t, needRefresh)
-
-	// refresh app using the 'deepest' requested comparison level
-	ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
-	ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
-
-	needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
-	assert.True(t, needRefresh)
-	assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
-	assert.Equal(t, CompareWithRecent, compareWith)
-
-	// refresh application which status is not reconciled using latest commit
-	app.Status.Sync = argoappv1.SyncStatus{Status: argoappv1.SyncStatusCodeUnknown}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctrl := newFakeController(&fakeData{apps: []runtime.Object{}})
+			app := tc.app
+			now := metav1.Now()
+			app.Status.ReconciledAt = &now
+
+			app.Status.Sync = argoappv1.SyncStatus{
+				Status: argoappv1.SyncStatusCodeSynced,
+				ComparedTo: argoappv1.ComparedTo{
+					Destination: app.Spec.Destination,
+				},
+			}
 
-	needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
-	assert.True(t, needRefresh)
-	assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
-	assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+			if app.Spec.HasMultipleSources() {
+				app.Status.Sync.ComparedTo.Sources = app.Spec.Sources
+			} else {
+				app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
+			}
 
-	{
-		// refresh app using the 'latest' level if comparison expired
-		app := app.DeepCopy()
-		ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
-		reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
-		app.Status.ReconciledAt = &reconciledAt
-		needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Minute, 2*time.Hour)
-		assert.True(t, needRefresh)
-		assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
-		assert.Equal(t, CompareWithLatestForceResolve, compareWith)
-	}
+			// no need to refresh just reconciled application
+			needRefresh, _, _ := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
+			assert.False(t, needRefresh)
+
+			// refresh app using the 'deepest' requested comparison level
+			ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
+			ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
+
+			needRefresh, refreshType, compareWith := ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
+			assert.True(t, needRefresh)
+			assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+			assert.Equal(t, CompareWithRecent, compareWith)
+
+			// refresh application which status is not reconciled using latest commit
+			app.Status.Sync = argoappv1.SyncStatus{Status: argoappv1.SyncStatusCodeUnknown}
+
+			needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
+			assert.True(t, needRefresh)
+			assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+			assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+
+			t.Run("refresh app using the 'latest' level if comparison expired", func(t *testing.T) {
+				app := app.DeepCopy()
+				ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
+				reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
+				app.Status.ReconciledAt = &reconciledAt
+				needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Minute, 2*time.Hour)
+				assert.True(t, needRefresh)
+				assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+				assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+			})
 
-	{
-		// refresh app using the 'latest' level if comparison expired for hard refresh
-		app := app.DeepCopy()
-		app.Status.Sync = argoappv1.SyncStatus{
-			Status: argoappv1.SyncStatusCodeSynced,
-			ComparedTo: argoappv1.ComparedTo{
-				Source:      app.Spec.GetSource(),
-				Destination: app.Spec.Destination,
-			},
-		}
-		ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
-		reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
-		app.Status.ReconciledAt = &reconciledAt
-		needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 2*time.Hour, 1*time.Minute)
-		assert.True(t, needRefresh)
-		assert.Equal(t, argoappv1.RefreshTypeHard, refreshType)
-		assert.Equal(t, CompareWithLatest, compareWith)
-	}
+			t.Run("refresh app using the 'latest' level if comparison expired for hard refresh", func(t *testing.T) {
+				app := app.DeepCopy()
+				app.Status.Sync = argoappv1.SyncStatus{
+					Status: argoappv1.SyncStatusCodeSynced,
+					ComparedTo: argoappv1.ComparedTo{
+						Destination: app.Spec.Destination,
+					},
+				}
+				if app.Spec.HasMultipleSources() {
+					app.Status.Sync.ComparedTo.Sources = app.Spec.Sources
+				} else {
+					app.Status.Sync.ComparedTo.Source = app.Spec.GetSource()
+				}
+				ctrl.requestAppRefresh(app.Name, CompareWithRecent.Pointer(), nil)
+				reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
+				app.Status.ReconciledAt = &reconciledAt
+				needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 2*time.Hour, 1*time.Minute)
+				assert.True(t, needRefresh)
+				assert.Equal(t, argoappv1.RefreshTypeHard, refreshType)
+				assert.Equal(t, CompareWithLatest, compareWith)
+			})
 
-	{
-		app := app.DeepCopy()
-		// execute hard refresh if app has refresh annotation
-		reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
-		app.Status.ReconciledAt = &reconciledAt
-		app.Annotations = map[string]string{
-			v1alpha1.AnnotationKeyRefresh: string(argoappv1.RefreshTypeHard),
-		}
-		needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
-		assert.True(t, needRefresh)
-		assert.Equal(t, argoappv1.RefreshTypeHard, refreshType)
-		assert.Equal(t, CompareWithLatestForceResolve, compareWith)
-	}
+			t.Run("execute hard refresh if app has refresh annotation", func(t *testing.T) {
+				app := app.DeepCopy()
+				reconciledAt := metav1.NewTime(time.Now().UTC().Add(-1 * time.Hour))
+				app.Status.ReconciledAt = &reconciledAt
+				app.Annotations = map[string]string{
+					v1alpha1.AnnotationKeyRefresh: string(argoappv1.RefreshTypeHard),
+				}
+				needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
+				assert.True(t, needRefresh)
+				assert.Equal(t, argoappv1.RefreshTypeHard, refreshType)
+				assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+			})
 
-	{
-		app := app.DeepCopy()
-		// ensure that CompareWithLatest level is used if application source has changed
-		ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
-		// sample app source change
-		app.Spec.Source.Helm = &argoappv1.ApplicationSourceHelm{
-			Parameters: []argoappv1.HelmParameter{{
-				Name:  "foo",
-				Value: "bar",
-			}},
-		}
+			t.Run("ensure that CompareWithLatest level is used if application source has changed", func(t *testing.T) {
+				app := app.DeepCopy()
+				ctrl.requestAppRefresh(app.Name, ComparisonWithNothing.Pointer(), nil)
+				// sample app source change
+				if app.Spec.HasMultipleSources() {
+					app.Spec.Sources[0].Helm = &argoappv1.ApplicationSourceHelm{
+						Parameters: []argoappv1.HelmParameter{{
+							Name:  "foo",
+							Value: "bar",
+						}},
+					}
+				} else {
+					app.Spec.Source.Helm = &argoappv1.ApplicationSourceHelm{
+						Parameters: []argoappv1.HelmParameter{{
+							Name:  "foo",
+							Value: "bar",
+						}},
+					}
+				}
 
-		needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
-		assert.True(t, needRefresh)
-		assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
-		assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+				needRefresh, refreshType, compareWith = ctrl.needRefreshAppStatus(app, 1*time.Hour, 2*time.Hour)
+				assert.True(t, needRefresh)
+				assert.Equal(t, argoappv1.RefreshTypeNormal, refreshType)
+				assert.Equal(t, CompareWithLatestForceResolve, compareWith)
+			})
+		})
 	}
 }
 

diff --git a/controller/state.go b/controller/state.go
@@ -471,7 +471,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap
 		failedToLoadObjs = true
 	}
 
-	logCtx.Debugf("Retrieved lived manifests")
+	logCtx.Debugf("Retrieved live manifests")
 
 	// filter out all resources which are not permitted in the application project
 	for k, v := range liveObjByKey {

diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
@@ -183,6 +183,18 @@ type ApplicationSource struct {
 // ApplicationSources contains list of required information about the sources of an application
 type ApplicationSources []ApplicationSource
 
+func (s ApplicationSources) Equals(other ApplicationSources) bool {
+	if len(s) != len(other) {
+		return false
+	}
+	for i := range s {
+		if !s[i].Equals(other[i]) {
+			return false
+		}
+	}
+	return true
+}
+
 func (a *ApplicationSpec) GetSource() ApplicationSource {
 	// if Application has multiple sources, return the first source in sources
 	if a.HasMultipleSources() {