Skip to content

Commit

Permalink
fix: multi-source app refresh (#11772) (#12217)
Browse files Browse the repository at this point in the history 
* fix multi-source refresh

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* serialize nil and empty resolvedRevisions the same to avoid cache misses

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* more consistent naming

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* document duplication

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* add todo

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
  • Loading branch information
@crenshaw-dev
crenshaw-dev authored Feb 6, 2023
1 parent 4a06cb2 commit 212029d
Show file tree
Hide file tree
Showing 5 changed files with 160 additions and 80 deletions.
63 changes: 38 additions & 25 deletions reposerver/cache/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,31 @@ func getRefTargetRevisionMappingForCacheKey(refTargetRevisionMapping appv1.RefTa
return res
}

func appSourceKey(appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping) uint32 {
return hash.FNVa(appSourceKeyJSON(appSrc, srcRefs))
func appSourceKey(appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, refSourceCommitSHAs ResolvedRevisions) uint32 {
return hash.FNVa(appSourceKeyJSON(appSrc, srcRefs, refSourceCommitSHAs))
}

// ResolvedRevisions is a map of "normalized git URL" -> "git commit SHA". When one source references another source,
// the referenced source revision may change, for example, when someone pushes a commit to the referenced branch. This
// map lets us keep track of the current revision for each referenced source.
type ResolvedRevisions map[string]string

type appSourceKeyStruct struct {
AppSrc *appv1.ApplicationSource `json:"appSrc"`
SrcRefs refTargetRevisionMappingForCacheKey `json:"srcRefs"`
AppSrc *appv1.ApplicationSource `json:"appSrc"`
SrcRefs refTargetRevisionMappingForCacheKey `json:"srcRefs"`
ResolvedRevisions ResolvedRevisions `json:"resolvedRevisions,omitempty"`
}

func appSourceKeyJSON(appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping) string {
func appSourceKeyJSON(appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, refSourceCommitSHAs ResolvedRevisions) string {
appSrc = appSrc.DeepCopy()
if !appSrc.IsHelm() {
appSrc.RepoURL = "" // superseded by commitSHA
appSrc.TargetRevision = "" // superseded by commitSHA
}
appSrcStr, _ := json.Marshal(appSourceKeyStruct{
AppSrc: appSrc,
SrcRefs: getRefTargetRevisionMappingForCacheKey(srcRefs),
AppSrc: appSrc,
SrcRefs: getRefTargetRevisionMappingForCacheKey(srcRefs),
ResolvedRevisions: refSourceCommitSHAs,
})
return string(appSrcStr)
}
Expand Down Expand Up @@ -183,9 +190,15 @@ func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference)
return nil
}

func manifestCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, namespace string, trackingMethod string, appLabelKey string, appName string, info ClusterRuntimeInfo) string {
// refSourceCommitSHAs is a list of resolved revisions for each ref source. This allows us to invalidate the cache
// when someone pushes a commit to a source which is referenced from the main source (the one referred to by `revision`).
func manifestCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, namespace string, trackingMethod string, appLabelKey string, appName string, info ClusterRuntimeInfo, refSourceCommitSHAs ResolvedRevisions) string {
// TODO: this function is getting unwieldy. We should probably consolidate some of this stuff into a struct. For
// example, revision could be part of ResolvedRevisions. And srcRefs is probably redundant now that
// refSourceCommitSHAs has been added. We don't need to know the _target_ revisions of the referenced sources
// when the _resolved_ revisions are already part of the key.
trackingKey := trackingKey(appLabelKey, trackingMethod)
return fmt.Sprintf("mfst|%s|%s|%s|%s|%d", trackingKey, appName, revision, namespace, appSourceKey(appSrc, srcRefs)+clusterRuntimeInfoKey(info))
return fmt.Sprintf("mfst|%s|%s|%s|%s|%d", trackingKey, appName, revision, namespace, appSourceKey(appSrc, srcRefs, refSourceCommitSHAs)+clusterRuntimeInfoKey(info))
}

func trackingKey(appLabelKey string, trackingMethod string) string {
Expand All @@ -198,11 +211,11 @@ func trackingKey(appLabelKey string, trackingMethod string) string {

// LogDebugManifestCacheKeyFields logs all the information included in a manifest cache key. It's intended to be run
// before every manifest cache operation to help debug cache misses.
func LogDebugManifestCacheKeyFields(message string, reason string, revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string) {
func LogDebugManifestCacheKeyFields(message string, reason string, revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, refSourceCommitSHAs ResolvedRevisions) {
if log.IsLevelEnabled(log.DebugLevel) {
log.WithFields(log.Fields{
"revision": revision,
"appSrc": appSourceKeyJSON(appSrc, srcRefs),
"appSrc": appSourceKeyJSON(appSrc, srcRefs, refSourceCommitSHAs),
"namespace": namespace,
"trackingKey": trackingKey(appLabelKey, trackingMethod),
"appName": appName,
Expand All @@ -212,8 +225,8 @@ func LogDebugManifestCacheKeyFields(message string, reason string, revision stri
}
}

func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse) error {
err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo), res)
func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse, refSourceCommitSHAs ResolvedRevisions) error {
err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res)

if err != nil {
return err
Expand All @@ -228,9 +241,9 @@ func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, s
if hash != res.CacheEntryHash || res.ManifestResponse == nil && res.MostRecentError == "" {
log.Warnf("Manifest hash did not match expected value or cached manifests response is empty, treating as a cache miss: %s", appName)

LogDebugManifestCacheKeyFields("deleting manifests cache", "manifest hash did not match or cached response is empty", revision, appSrc, srcRefs, clusterInfo, namespace, trackingMethod, appLabelKey, appName)
LogDebugManifestCacheKeyFields("deleting manifests cache", "manifest hash did not match or cached response is empty", revision, appSrc, srcRefs, clusterInfo, namespace, trackingMethod, appLabelKey, appName, refSourceCommitSHAs)

err = c.DeleteManifests(revision, appSrc, srcRefs, clusterInfo, namespace, trackingMethod, appLabelKey, appName)
err = c.DeleteManifests(revision, appSrc, srcRefs, clusterInfo, namespace, trackingMethod, appLabelKey, appName, refSourceCommitSHAs)
if err != nil {
return fmt.Errorf("Unable to delete manifest after hash mismatch, %v", err)
}
Expand All @@ -245,7 +258,7 @@ func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, s
return nil
}

func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse) error {
func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse, refSourceCommitSHAs ResolvedRevisions) error {
// Generate and apply the cache entry hash, before writing
if res != nil {
res = res.shallowCopy()
Expand All @@ -256,26 +269,26 @@ func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, s
res.CacheEntryHash = hash
}

return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo), res, c.repoCacheExpiration, res == nil)
return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res, c.repoCacheExpiration, res == nil)
}

func (c *Cache) DeleteManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace, trackingMethod, appLabelKey, appName string) error {
return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo), "", c.repoCacheExpiration, true)
func (c *Cache) DeleteManifests(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, clusterInfo ClusterRuntimeInfo, namespace, trackingMethod, appLabelKey, appName string, refSourceCommitSHAs ResolvedRevisions) error {
return c.cache.SetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), "", c.repoCacheExpiration, true)
}

func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, trackingMethod appv1.TrackingMethod) string {
func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) string {
if trackingMethod == "" {
trackingMethod = argo.TrackingMethodLabel
}
return fmt.Sprintf("appdetails|%s|%d|%s", revision, appSourceKey(appSrc, srcRefs), trackingMethod)
return fmt.Sprintf("appdetails|%s|%d|%s", revision, appSourceKey(appSrc, srcRefs, refSourceCommitSHAs), trackingMethod)
}

func (c *Cache) GetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod) error {
return c.cache.GetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod), res)
func (c *Cache) GetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) error {
return c.cache.GetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res)
}

func (c *Cache) SetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod) error {
return c.cache.SetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod), res, c.repoCacheExpiration, res == nil)
func (c *Cache) SetAppDetails(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, res *apiclient.RepoAppDetailsResponse, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) error {
return c.cache.SetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res, c.repoCacheExpiration, res == nil)
}

func revisionMetadataKey(repoURL, revision string) string {
Expand Down
36 changes: 20 additions & 16 deletions reposerver/cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,34 +73,38 @@ func TestCache_GetManifests(t *testing.T) {
// cache miss
q := &apiclient.ManifestRequest{}
value := &CachedManifestResponse{}
err := cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
err := cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
// populate cache
res := &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}
err = cache.SetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", res)
err = cache.SetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", res, nil)
assert.NoError(t, err)
t.Run("expect cache miss because of changed revision", func(t *testing.T) {
err = cache.GetManifests("other-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("other-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache miss because of changed path", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{Path: "other-path"}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{Path: "other-path"}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache miss because of changed namespace", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "other-namespace", "", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "other-namespace", "", "my-app-label-key", "my-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache miss because of changed app label key", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "other-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "other-app-label-key", "my-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache miss because of changed app label value", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "other-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "other-app-label-value", value, nil)
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache miss because of changed referenced source", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "other-app-label-value", value, map[string]string{"my-referenced-source": "my-referenced-revision"})
assert.Equal(t, ErrCacheMiss, err)
})
t.Run("expect cache hit", func(t *testing.T) {
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q.RefSources, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value, nil)
assert.NoError(t, err)
assert.Equal(t, &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}, value)
})
Expand All @@ -111,19 +115,19 @@ func TestCache_GetAppDetails(t *testing.T) {
// cache miss
value := &apiclient.RepoAppDetailsResponse{}
emptyRefSources := map[string]*appv1.RefTarget{}
err := cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "")
err := cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "", nil)
assert.Equal(t, ErrCacheMiss, err)
res := &apiclient.RepoAppDetailsResponse{Type: "my-type"}
err = cache.SetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, res, "")
err = cache.SetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, res, "", nil)
assert.NoError(t, err)
//cache miss
err = cache.GetAppDetails("other-revision", &ApplicationSource{}, emptyRefSources, value, "")
err = cache.GetAppDetails("other-revision", &ApplicationSource{}, emptyRefSources, value, "", nil)
assert.Equal(t, ErrCacheMiss, err)
//cache miss
err = cache.GetAppDetails("my-revision", &ApplicationSource{Path: "other-path"}, emptyRefSources, value, "")
err = cache.GetAppDetails("my-revision", &ApplicationSource{Path: "other-path"}, emptyRefSources, value, "", nil)
assert.Equal(t, ErrCacheMiss, err)
// cache hit
err = cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "")
err = cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "", nil)
assert.NoError(t, err)
assert.Equal(t, &apiclient.RepoAppDetailsResponse{Type: "my-type"}, value)
}
Expand Down Expand Up @@ -162,7 +166,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {
NumberOfConsecutiveFailures: 0,
}
q := &apiclient.ManifestRequest{}
err := repoCache.SetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, store)
err := repoCache.SetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, store, nil)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -193,7 +197,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {

// Retrieve the value using 'GetManifests' and confirm it works
retrievedVal := &CachedManifestResponse{}
err = repoCache.GetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, retrievedVal)
err = repoCache.GetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, retrievedVal, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -216,7 +220,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {

// Retrieve the value using GetManifests and confirm it returns a cache miss
retrievedVal = &CachedManifestResponse{}
err = repoCache.GetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, retrievedVal)
err = repoCache.GetManifests(response.Revision, appSrc, q.RefSources, q, response.Namespace, "", appKey, appValue, retrievedVal, nil)

assert.True(t, err == cacheutil.ErrCacheMiss)

Expand Down
Loading

0 comments on commit 212029d

Please sign in to comment.