fix(diff): avoid cache miss in server-side diff (#20423) (#20424)

* fix(diff): avoid cache miss in server-side diff (#20423) Signed-off-by: Michael Crenshaw <[email protected]> * fix silly mistakes Signed-off-by: Michael Crenshaw <[email protected]> --------- Signed-off-by: Michael Crenshaw <[email protected]>
argoproj · Oct 18, 2024 · ec499bb · ec499bb
1 parent 76104da
commit ec499bb
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 9 deletions.
diff --git a/controller/state.go b/controller/state.go
@@ -936,9 +936,7 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou
  return false
  }
 
- currentSpec := app.BuildComparedToStatus()
- specChanged := !reflect.DeepEqual(app.Status.Sync.ComparedTo, currentSpec)
- if specChanged {
+ if !specEqualsCompareTo(app.Spec, app.Status.Sync.ComparedTo) {
  log.WithField("useDiffCache", "false").Debug("specChanged")
  return false
  }
@@ -947,6 +945,25 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou
  return true
 }
 
+// specEqualsCompareTo compares the application spec to the comparedTo status. It normalizes the destination to match
+// the comparedTo destination before comparing. It does not mutate the original spec or comparedTo.
+func specEqualsCompareTo(spec v1alpha1.ApplicationSpec, comparedTo v1alpha1.ComparedTo) bool {
+ // Make a copy to be sure we don't mutate the original.
+ specCopy := spec.DeepCopy()
+ currentSpec := specCopy.BuildComparedToStatus()
+
+ // The spec might have been augmented to include both server and name, so change it to match the comparedTo before
+ // comparing.
+ if comparedTo.Destination.Server == "" {
+ currentSpec.Destination.Server = ""
+ }
+ if comparedTo.Destination.Name == "" {
+ currentSpec.Destination.Name = ""
+ }
+
+ return reflect.DeepEqual(comparedTo, currentSpec)
+}
+
 func (m *appStateManager) persistRevisionHistory(
  app *v1alpha1.Application,
  revision string,

diff --git a/controller/state_test.go b/controller/state_test.go
@@ -1431,6 +1431,8 @@ func TestIsLiveResourceManaged(t *testing.T) {
 }
 
 func TestUseDiffCache(t *testing.T) {
+ t.Parallel()
+
  type fixture struct {
  testName string
  noCache bool
@@ -1691,6 +1693,44 @@ func TestUseDiffCache(t *testing.T) {
  expectedUseCache: false,
  serverSideDiff: false,
  },
+ {
+ // There are code paths that modify the ApplicationSpec and augment the destination field with both the
+ // destination server and name. Since both fields are populated in the app spec but not in the comparedTo,
+ // we need to make sure we correctly compare the fields and don't miss the cache.
+ testName: "will return true if the app spec destination contains both server and name, but otherwise matches comparedTo",
+ noCache: false,
+ manifestInfos: manifestInfos("rev1"),
+ sources: sources(),
+ app: app("httpbin", "rev1", false, &argoappv1.Application{
+ Spec: argoappv1.ApplicationSpec{
+ Destination: argoappv1.ApplicationDestination{
+ Server: "https://kubernetes.default.svc",
+ Name: "httpbin",
+ Namespace: "httpbin",
+ },
+ },
+ Status: argoappv1.ApplicationStatus{
+ Resources: []argoappv1.ResourceStatus{},
+ Sync: argoappv1.SyncStatus{
+ Status: argoappv1.SyncStatusCodeSynced,
+ ComparedTo: argoappv1.ComparedTo{
+ Destination: argoappv1.ApplicationDestination{
+ Server: "https://kubernetes.default.svc",
+ Namespace: "httpbin",
+ },
+ },
+ Revision: "rev1",
+ },
+ ReconciledAt: &metav1.Time{
+ Time: time.Now().Add(-time.Hour),
+ },
+ },
+ }),
+ manifestRevisions: []string{"rev1"},
+ statusRefreshTimeout: time.Hour * 24,
+ expectedUseCache: true,
+ serverSideDiff: true,
+ },
  }
 
  for _, tc := range cases {

diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
@@ -1056,15 +1056,15 @@ func (a *ApplicationStatus) GetRevisions() []string {
 
 // BuildComparedToStatus will build a ComparedTo object based on the current
 // Application state.
-func (app *Application) BuildComparedToStatus() ComparedTo {
+func (spec *ApplicationSpec) BuildComparedToStatus() ComparedTo {
  ct := ComparedTo{
- Destination: app.Spec.Destination,
- IgnoreDifferences: app.Spec.IgnoreDifferences,
+ Destination: spec.Destination,
+ IgnoreDifferences: spec.IgnoreDifferences,
  }
- if app.Spec.HasMultipleSources() {
- ct.Sources = app.Spec.Sources
+ if spec.HasMultipleSources() {
+ ct.Sources = spec.Sources
  } else {
- ct.Source = app.Spec.GetSource()
+ ct.Source = spec.GetSource()
  }
  return ct
 }