From e6e3a5a35c39069a19911349dc217f0cdd05eda8 Mon Sep 17 00:00:00 2001 From: nromriell Date: Sun, 12 Nov 2023 10:47:52 -0800 Subject: [PATCH 1/5] fix: ref and multi-source git cache logic Signed-off-by: nromriell --- .../argocd-repo-server-deployment.yaml | 18 +++ manifests/core-install.yaml | 18 +++ manifests/ha/install.yaml | 18 +++ manifests/ha/namespace-install.yaml | 18 +++ manifests/install.yaml | 18 +++ manifests/namespace-install.yaml | 18 +++ reposerver/cache/cache.go | 123 +++++++++++++++--- reposerver/repository/repository.go | 25 +++- server/server.go | 4 +- util/cache/cache.go | 33 +++-- util/cache/client.go | 5 + util/cache/inmemory.go | 6 +- util/cache/redis.go | 1 + util/git/client.go | 22 +++- 14 files changed, 294 insertions(+), 33 deletions(-) diff --git a/manifests/base/repo-server/argocd-repo-server-deployment.yaml b/manifests/base/repo-server/argocd-repo-server-deployment.yaml index 728c80c14bc2a..b7dfa9b9455af 100644 --- a/manifests/base/repo-server/argocd-repo-server-deployment.yaml +++ b/manifests/base/repo-server/argocd-repo-server-deployment.yaml @@ -90,6 +90,24 @@ spec: name: argocd-cmd-params-cm key: reposerver.repo.cache.expiration optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index 2469f171351fe..1eedd89131839 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -21232,6 +21232,24 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index 0f637cd786dc2..88e8810ac9b41 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -22719,6 +22719,24 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/namespace-install.yaml b/manifests/ha/namespace-install.yaml index 33ada40e37004..31213ad78aff8 100644 --- a/manifests/ha/namespace-install.yaml +++ b/manifests/ha/namespace-install.yaml @@ -2106,6 +2106,24 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/install.yaml b/manifests/install.yaml index cb4cda559caf0..f3a54f82d5198 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -21765,6 +21765,24 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 9e6f98030d89b..b5ab71345cdb8 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -1152,6 +1152,24 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.enabled + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.timeout + name: argocd-cmd-params-cm + optional: true + - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL + valueFrom: + configMapKeyRef: + key: reposerver.repo.cache.lock.interval + name: argocd-cmd-params-cm + optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 79d3a02b62750..0b22a461674b3 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -12,6 +12,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/utils/text" "github.com/go-git/go-git/v5/plumbing" + "github.com/google/uuid" "github.com/redis/go-redis/v9" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" @@ -25,11 +26,27 @@ import ( ) var ErrCacheMiss = cacheutil.ErrCacheMiss +var ErrCacheKeyLocked = cacheutil.ErrCacheKeyLocked + +var CacheOptsDefaults = &CacheOpts{ + repoCacheExpiration: env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), + revisionCacheExpiration: env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), + revisionCacheLockWaitEnabled: env.ParseBoolFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED", false), + revisionCacheLockTimeout: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_TIMEOUT", 10*time.Second, 0, math.MaxInt64), + revisionCacheLockWaitInterval: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL", 1*time.Second, 0, math.MaxInt64), +} + +type CacheOpts struct { + repoCacheExpiration time.Duration + revisionCacheExpiration time.Duration + revisionCacheLockWaitEnabled bool + revisionCacheLockTimeout time.Duration + revisionCacheLockWaitInterval time.Duration +} type Cache struct { - cache *cacheutil.Cache - repoCacheExpiration time.Duration - revisionCacheExpiration time.Duration + cache *cacheutil.Cache + CacheOpts } // ClusterRuntimeInfo holds cluster runtime information @@ -40,16 +57,21 @@ type ClusterRuntimeInfo interface { GetKubeVersion() string } -func NewCache(cache *cacheutil.Cache, repoCacheExpiration time.Duration, revisionCacheExpiration time.Duration) *Cache { - return &Cache{cache, repoCacheExpiration, revisionCacheExpiration} +func NewCache(cache *cacheutil.Cache, cacheOpts *CacheOpts) *Cache { + if cacheOpts == nil { + cacheOpts = CacheOptsDefaults + } + return &Cache{cache, *cacheOpts} } func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { - var repoCacheExpiration time.Duration - var revisionCacheExpiration time.Duration + var cacheOpts *CacheOpts = &CacheOpts{} - cmd.Flags().DurationVar(&repoCacheExpiration, "repo-cache-expiration", env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") - cmd.Flags().DurationVar(&revisionCacheExpiration, "revision-cache-expiration", env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), "Cache expiration for cached revision") + cmd.Flags().DurationVar(&cacheOpts.repoCacheExpiration, "repo-cache-expiration", CacheOptsDefaults.repoCacheExpiration, "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") + cmd.Flags().DurationVar(&cacheOpts.revisionCacheExpiration, "revision-cache-expiration", CacheOptsDefaults.revisionCacheExpiration, "Cache expiration for cached revision") + cmd.Flags().BoolVar(&cacheOpts.revisionCacheLockWaitEnabled, "enable-revision-cache-lock", CacheOptsDefaults.revisionCacheLockWaitEnabled, "Enables a lock to prevent duplicate git requests during revision cache updates") + cmd.Flags().DurationVar(&cacheOpts.revisionCacheLockTimeout, "revision-cache-lock-timeout", CacheOptsDefaults.revisionCacheLockTimeout, "Specifies the max amount of time each routine making identical git requests will wait for the git reference cache to update before making a new git requests") + cmd.Flags().DurationVar(&cacheOpts.revisionCacheLockWaitInterval, "revision-cache-lock-wait-interval", CacheOptsDefaults.revisionCacheLockWaitInterval, "Specifies the amount of time between each check to see if the git cache refs lock has been released") repoFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) @@ -58,7 +80,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) if err != nil { return nil, fmt.Errorf("error adding cache flags to cmd: %w", err) } - return NewCache(cache, repoCacheExpiration, revisionCacheExpiration), nil + return NewCache(cache, cacheOpts), nil } } @@ -176,20 +198,91 @@ func (c *Cache) SetGitReferences(repo string, references []*plumbing.Reference) return c.cache.SetItem(gitRefsKey(repo), input, c.revisionCacheExpiration, false) } +func GitRefCacheItemToReferences(cacheItem [][2]string) []*plumbing.Reference { + var res []*plumbing.Reference + for i := range cacheItem { + res = append(res, plumbing.NewReferenceFromStrings(cacheItem[i][0], cacheItem[i][1])) + } + return res +} + // GetGitReferences retrieves resolved Git repository references from cache func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { var input [][2]string if err := c.cache.GetItem(gitRefsKey(repo), &input); err != nil { return err } - var res []*plumbing.Reference - for i := range input { - res = append(res, plumbing.NewReferenceFromStrings(input[i][0], input[i][1])) - } - *references = res + *references = GitRefCacheItemToReferences(input) return nil } +// TryLockGitRefCache attempts to lock the key for the Git repository references if the key doesn't exist +func (c *Cache) TryLockGitRefCache(repo string, lockId string) { + err := c.cache.SetCacheItem(&cacheutil.Item{ + Key: gitRefsKey(repo), + Object: [][2]string{{cacheutil.CacheLockedValue, lockId}}, + Expiration: c.revisionCacheLockTimeout, + DisableOverwrite: true, + }, false) + if err != nil { + // The key already existing does not produce an error for go-redis so we'll use the get to validate + // In memory would produce an error, but ignoring provides a consistent flow + log.Debug("Error setting git references cache lock: ", err) + } +} + +// GetOrLockGitReferences retrieves the git references if they exist, otherwise creates a lock and returns so the caller can populate the cache +func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (lockCreated bool, lockId string, err error) { + // feature flagged for now + if !c.revisionCacheLockWaitEnabled { + log.Debug("Git references cache lock is disabled") + return false, "", c.GetGitReferences(repo, references) + } + var input [][2]string + myLockUUID, err := uuid.NewRandom() + if err != nil { + log.Debug("Error generating git references cache lock id: ", err) + return false, "", err + } + // We need to be able to identify that our lock was the successful one, otherwise we'll still have duplicate requests + myLockId := myLockUUID.String() + waitUntil := time.Now().Add(c.revisionCacheLockTimeout) + // Wait only the maximum amount of time configured for the lock + for time.Now().Before(waitUntil) { + // Attempt to get the lock + c.TryLockGitRefCache(repo, myLockId) + err = c.cache.GetItem(gitRefsKey(repo), &input) + if err == nil && len(input) > 0 && len(input[0]) > 0 { + if input[0][0] != cacheutil.CacheLockedValue { + // Valid value in cache, convert to plumbing.Reference and return + *references = GitRefCacheItemToReferences(input) + return false, myLockId, err + } else if input[0][1] == myLockId { + // Our lock was successful + return true, myLockId, nil + } + } + // Wait for lock, valid value, or timeout + time.Sleep(c.revisionCacheLockWaitInterval) + } + return false, myLockId, cacheutil.ErrLockWaitExpired +} + +// UnlockGitReferences unlocks the key for the Git repository references if needed +func (c *Cache) UnlockGitReferences(repo string, lockId string) error { + var input [][2]string + var err error + if err = c.cache.GetItem(gitRefsKey(repo), &input); err == nil && + len(input) > 0 && + len(input[0]) > 1 && + input[0][0] == cacheutil.CacheLockedValue && + input[0][1] == lockId { + // We have the lock, so remove it + return c.cache.SetItem(gitRefsKey(repo), input, 0, true) + } + return err +} + // 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 { diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index f66bc71ac4621..df9b1a36f4805 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -300,6 +300,7 @@ func (s *Service) runRepoOperation( var gitClient git.Client var helmClient helm.Client var err error + gitClientOpts := git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache) revision = textutils.FirstNonEmpty(revision, source.TargetRevision) unresolvedRevision := revision if source.IsHelm() { @@ -308,13 +309,13 @@ func (s *Service) runRepoOperation( return err } } else { - gitClient, revision, err = s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) + gitClient, revision, err = s.newClientResolveRevision(repo, revision, gitClientOpts) if err != nil { return err } } - repoRefs, err := resolveReferencedSources(hasMultipleSources, source.Helm, refSources, s.newClientResolveRevision) + repoRefs, err := resolveReferencedSources(hasMultipleSources, source.Helm, refSources, s.newClientResolveRevision, gitClientOpts) if err != nil { return err } @@ -463,7 +464,7 @@ type gitClientGetter func(repo *v1alpha1.Repository, revision string, opts ...gi // // Much of this logic is duplicated in runManifestGenAsync. If making changes here, check whether runManifestGenAsync // should be updated. -func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.ApplicationSourceHelm, refSources map[string]*v1alpha1.RefTarget, newClientResolveRevision gitClientGetter) (map[string]string, error) { +func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.ApplicationSourceHelm, refSources map[string]*v1alpha1.RefTarget, newClientResolveRevision gitClientGetter, gitClientOpts git.ClientOpts) (map[string]string, error) { repoRefs := make(map[string]string) if !hasMultipleSources || source == nil { return repoRefs, nil @@ -490,7 +491,7 @@ func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.Applicat normalizedRepoURL := git.NormalizeGitURL(refSourceMapping.Repo.Repo) _, ok = repoRefs[normalizedRepoURL] if !ok { - _, referencedCommitSHA, err := newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) + _, referencedCommitSHA, err := newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision, gitClientOpts) if err != nil { log.Errorf("Failed to get git client for repo %s: %v", refSourceMapping.Repo.Repo, err) return nil, fmt.Errorf("failed to get git client for repo %s", refSourceMapping.Repo.Repo) @@ -506,6 +507,17 @@ func resolveReferencedSources(hasMultipleSources bool, source *v1alpha1.Applicat func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestRequest) (*apiclient.ManifestResponse, error) { var res *apiclient.ManifestResponse var err error + + settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} + // Skip this path for ref only sources + if q.HasMultipleSources && q.ApplicationSource.Path == "" && q.ApplicationSource.Chart == "" && q.ApplicationSource.Ref != "" { + _, revision, err := s.newClientResolveRevision(q.Repo, q.Revision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) + res = &apiclient.ManifestResponse{ + Revision: revision, + } + return res, err + } + cacheFn := func(cacheKey string, refSourceCommitSHAs cache.ResolvedRevisions, firstInvocation bool) (bool, error) { ok, resp, err := s.getManifestCacheEntry(cacheKey, q, refSourceCommitSHAs, firstInvocation) res = resp @@ -544,7 +556,6 @@ func (s *Service) GenerateManifest(ctx context.Context, q *apiclient.ManifestReq return nil } - settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} err = s.runRepoOperation(ctx, q.Revision, q.Repo, q.ApplicationSource, q.VerifySignature, cacheFn, operation, settings, q.HasMultipleSources, q.RefSources) // if the tarDoneCh message is sent it means that the manifest @@ -686,6 +697,8 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, close(ch.responseCh) }() + settings := operationSettings{sem: s.parallelismLimitSemaphore, noCache: q.NoCache, noRevisionCache: q.NoRevisionCache, allowConcurrent: q.ApplicationSource.AllowsConcurrentProcessing()} + // GenerateManifests mutates the source (applies overrides). Those overrides shouldn't be reflected in the cache // key. Overrides will break the cache anyway, because changes to overrides will change the revision. appSourceCopy := q.ApplicationSource.DeepCopy() @@ -728,7 +741,7 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, return } } else { - gitClient, referencedCommitSHA, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision) + gitClient, referencedCommitSHA, err := s.newClientResolveRevision(&refSourceMapping.Repo, refSourceMapping.TargetRevision, git.WithCache(s.cache, !settings.noRevisionCache && !settings.noCache)) if err != nil { log.Errorf("Failed to get git client for repo %s: %v", refSourceMapping.Repo.Repo, err) ch.errCh <- fmt.Errorf("failed to get git client for repo %s", refSourceMapping.Repo.Repo) diff --git a/server/server.go b/server/server.go index e52416927143b..2d007dd984c91 100644 --- a/server/server.go +++ b/server/server.go @@ -1026,9 +1026,9 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl // Dex reverse proxy and client app and OAuth2 login/callback a.registerDexHandlers(mux) - // Webhook handler for git events (Note: cache timeouts are hardcoded because API server does not write to cache and not really using them) + // Webhook handler for git events (Note: repo cache timeouts can be configured with the same environment variables as repo-server) argoDB := db.NewDB(a.Namespace, a.settingsMgr, a.KubeClientset) - acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, repocache.NewCache(a.Cache.GetCache(), 24*time.Hour, 3*time.Minute), a.Cache, argoDB) + acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, repocache.NewCache(a.Cache.GetCache(), repocache.CacheOptsDefaults), a.Cache, argoDB) mux.HandleFunc("/api/webhook", acdWebhookHandler.Handler) diff --git a/util/cache/cache.go b/util/cache/cache.go index fdea46cdea0d2..0b77c200952d8 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -16,6 +16,7 @@ import ( "github.com/argoproj/argo-cd/v2/common" certutil "github.com/argoproj/argo-cd/v2/util/cert" "github.com/argoproj/argo-cd/v2/util/env" + log "github.com/sirupsen/logrus" ) const ( @@ -163,30 +164,46 @@ func (c *Cache) SetClient(client CacheClient) { c.client = client } -func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { - key = fmt.Sprintf("%s|%s", key, common.CacheVersion) +func (c *Cache) generateFullKey(key string) string { + if key == "" { + log.Debug("Cache key is empty, this will result in key collisions if there is more than one empty key") + } + return fmt.Sprintf("%s|%s", key, common.CacheVersion) +} + +func (c *Cache) SetCacheItem(item *Item, delete bool) error { + item.Key = c.generateFullKey(item.Key) if delete { - return c.client.Delete(key) + return c.client.Delete(item.Key) } else { if item == nil { - return fmt.Errorf("cannot set item to nil for key %s", key) + return fmt.Errorf("cannot set item to nil for key %s", item.Key) } - return c.client.Set(&Item{Object: item, Key: key, Expiration: expiration}) + return c.client.Set(item) + } +} + +func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { + cacheItem := &Item{ + Key: key, + Object: item, + Expiration: expiration, } + return c.SetCacheItem(cacheItem, delete) } func (c *Cache) GetItem(key string, item interface{}) error { + key = c.generateFullKey(key) if item == nil { return fmt.Errorf("cannot get item into a nil for key %s", key) } - key = fmt.Sprintf("%s|%s", key, common.CacheVersion) return c.client.Get(key, item) } func (c *Cache) OnUpdated(ctx context.Context, key string, callback func() error) error { - return c.client.OnUpdated(ctx, fmt.Sprintf("%s|%s", key, common.CacheVersion), callback) + return c.client.OnUpdated(ctx, c.generateFullKey(key), callback) } func (c *Cache) NotifyUpdated(key string) error { - return c.client.NotifyUpdated(fmt.Sprintf("%s|%s", key, common.CacheVersion)) + return c.client.NotifyUpdated(c.generateFullKey(key)) } diff --git a/util/cache/client.go b/util/cache/client.go index 434c2a8da187a..080f37bf2d49c 100644 --- a/util/cache/client.go +++ b/util/cache/client.go @@ -7,10 +7,15 @@ import ( ) var ErrCacheMiss = errors.New("cache: key is missing") +var ErrCacheKeyLocked = errors.New("cache: key is locked") +var ErrLockWaitExpired = errors.New("cache: wait for lock exceeded max wait time") +var CacheLockedValue = "locked" type Item struct { Key string Object interface{} + // Disable writing if key already exists (NX) + DisableOverwrite bool // Expiration is the cache expiration time. Expiration time.Duration } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index 53e690925d940..13828c6f5e4f9 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -29,7 +29,11 @@ func (i *InMemoryCache) Set(item *Item) error { if err != nil { return err } - i.memCache.Set(item.Key, buf, item.Expiration) + if item.DisableOverwrite { + i.memCache.Add(item.Key, buf, item.Expiration) + } else { + i.memCache.Set(item.Key, buf, item.Expiration) + } return nil } diff --git a/util/cache/redis.go b/util/cache/redis.go index c5365c4984e21..b6a681398540a 100644 --- a/util/cache/redis.go +++ b/util/cache/redis.go @@ -110,6 +110,7 @@ func (r *redisCache) Set(item *Item) error { Key: r.getKey(item.Key), Value: val, TTL: expiration, + SetNX: item.DisableOverwrite, }) } diff --git a/util/git/client.go b/util/git/client.go index 6a8828d13f432..0f652ed487268 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -56,6 +56,8 @@ type Refs struct { type gitRefCache interface { SetGitReferences(repo string, references []*plumbing.Reference) error GetGitReferences(repo string, references *[]*plumbing.Reference) error + GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (lockCreated bool, lockId string, err error) + UnlockGitReferences(repo string, lockId string) error } // Client is a generic git client interface @@ -477,11 +479,28 @@ func (m *nativeGitClient) Checkout(revision string, submoduleEnabled bool) error } func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { + // Prevent an additional get call to cache if we know our state isn't stale + needsUnlock := true if m.gitRefCache != nil && m.loadRefFromCache { var res []*plumbing.Reference - if m.gitRefCache.GetGitReferences(m.repoURL, &res) == nil { + lockCreated, lockId, err := m.gitRefCache.GetOrLockGitReferences(m.repoURL, &res) + if !lockCreated && err == nil { + // Valid value already in cache return res, nil + } else if err != nil { + // Error getting value from cache + log.Debugf("Error getting git references from cache: %v", err) + return nil, err + } else { + // We don't own the lock so there's no path where we need to unlock + needsUnlock = false } + // Defer a soft reset of the cache lock, if the value is set this call will be ignored + defer func() { + if needsUnlock { + m.gitRefCache.UnlockGitReferences(m.repoURL, lockId) + } + }() } if m.OnLsRemote != nil { @@ -509,6 +528,7 @@ func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { if err := m.gitRefCache.SetGitReferences(m.repoURL, res); err != nil { log.Warnf("Failed to store git references to cache: %v", err) } + needsUnlock = false return res, nil } return res, err From 39745d065fcc861351a31ed218a2201809fca82e Mon Sep 17 00:00:00 2001 From: nromriell Date: Sun, 12 Nov 2023 18:51:13 -0800 Subject: [PATCH 2/5] chore: add unit testing for repo server getRefs, cleanup, fixes Signed-off-by: nromriell --- reposerver/cache/cache.go | 59 ++-- reposerver/cache/cache_test.go | 18 +- .../repository/mocks/RepositoryCache.go | 144 ++++++++++ reposerver/repository/repository_test.go | 260 +++++++++++++++++- util/cache/client.go | 1 - util/git/client.go | 15 +- util/git/client_test.go | 23 +- util/webhook/webhook_test.go | 9 +- 8 files changed, 475 insertions(+), 54 deletions(-) create mode 100644 reposerver/repository/mocks/RepositoryCache.go diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 0b22a461674b3..96b015c239dff 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -29,14 +29,23 @@ var ErrCacheMiss = cacheutil.ErrCacheMiss var ErrCacheKeyLocked = cacheutil.ErrCacheKeyLocked var CacheOptsDefaults = &CacheOpts{ - repoCacheExpiration: env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), - revisionCacheExpiration: env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), - revisionCacheLockWaitEnabled: env.ParseBoolFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED", false), - revisionCacheLockTimeout: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_TIMEOUT", 10*time.Second, 0, math.MaxInt64), - revisionCacheLockWaitInterval: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL", 1*time.Second, 0, math.MaxInt64), + RepoCacheExpiration: env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), + RevisionCacheExpiration: env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), + RevisionCacheLockWaitEnabled: env.ParseBoolFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED", false), + RevisionCacheLockTimeout: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_TIMEOUT", 10*time.Second, 0, math.MaxInt64), + RevisionCacheLockWaitInterval: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL", 1*time.Second, 0, math.MaxInt64), } type CacheOpts struct { + RepoCacheExpiration time.Duration + RevisionCacheExpiration time.Duration + RevisionCacheLockWaitEnabled bool + RevisionCacheLockTimeout time.Duration + RevisionCacheLockWaitInterval time.Duration +} + +type Cache struct { + cache *cacheutil.Cache repoCacheExpiration time.Duration revisionCacheExpiration time.Duration revisionCacheLockWaitEnabled bool @@ -44,11 +53,6 @@ type CacheOpts struct { revisionCacheLockWaitInterval time.Duration } -type Cache struct { - cache *cacheutil.Cache - CacheOpts -} - // ClusterRuntimeInfo holds cluster runtime information type ClusterRuntimeInfo interface { // GetApiVersions returns supported api versions @@ -61,17 +65,26 @@ func NewCache(cache *cacheutil.Cache, cacheOpts *CacheOpts) *Cache { if cacheOpts == nil { cacheOpts = CacheOptsDefaults } - return &Cache{cache, *cacheOpts} + if cacheOpts.RevisionCacheLockWaitEnabled && cacheOpts.RevisionCacheLockWaitInterval > cacheOpts.RevisionCacheLockTimeout { + log.Warnf("revision-cache-lock-wait-interval (%s) is greater than revision-cache-lock-timeout (%s) reducing interval to revision-cache-lock-timeout", cacheOpts.RevisionCacheLockWaitInterval, cacheOpts.RevisionCacheLockTimeout) + cacheOpts.RevisionCacheLockWaitInterval = cacheOpts.RevisionCacheLockTimeout + } + return &Cache{cache, + cacheOpts.RepoCacheExpiration, + cacheOpts.RevisionCacheExpiration, + cacheOpts.RevisionCacheLockWaitEnabled, + cacheOpts.RevisionCacheLockTimeout, + cacheOpts.RevisionCacheLockWaitInterval} } func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { var cacheOpts *CacheOpts = &CacheOpts{} - cmd.Flags().DurationVar(&cacheOpts.repoCacheExpiration, "repo-cache-expiration", CacheOptsDefaults.repoCacheExpiration, "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") - cmd.Flags().DurationVar(&cacheOpts.revisionCacheExpiration, "revision-cache-expiration", CacheOptsDefaults.revisionCacheExpiration, "Cache expiration for cached revision") - cmd.Flags().BoolVar(&cacheOpts.revisionCacheLockWaitEnabled, "enable-revision-cache-lock", CacheOptsDefaults.revisionCacheLockWaitEnabled, "Enables a lock to prevent duplicate git requests during revision cache updates") - cmd.Flags().DurationVar(&cacheOpts.revisionCacheLockTimeout, "revision-cache-lock-timeout", CacheOptsDefaults.revisionCacheLockTimeout, "Specifies the max amount of time each routine making identical git requests will wait for the git reference cache to update before making a new git requests") - cmd.Flags().DurationVar(&cacheOpts.revisionCacheLockWaitInterval, "revision-cache-lock-wait-interval", CacheOptsDefaults.revisionCacheLockWaitInterval, "Specifies the amount of time between each check to see if the git cache refs lock has been released") + cmd.Flags().DurationVar(&cacheOpts.RepoCacheExpiration, "repo-cache-expiration", CacheOptsDefaults.RepoCacheExpiration, "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") + cmd.Flags().DurationVar(&cacheOpts.RevisionCacheExpiration, "revision-cache-expiration", CacheOptsDefaults.RevisionCacheExpiration, "Cache expiration for cached revision") + cmd.Flags().BoolVar(&cacheOpts.RevisionCacheLockWaitEnabled, "enable-revision-cache-lock", CacheOptsDefaults.RevisionCacheLockWaitEnabled, "Enables a lock to prevent duplicate git requests during revision cache updates") + cmd.Flags().DurationVar(&cacheOpts.RevisionCacheLockTimeout, "revision-cache-lock-timeout", CacheOptsDefaults.RevisionCacheLockTimeout, "Specifies the max amount of time each routine making identical git requests will wait for the git reference cache to update before making a new git requests") + cmd.Flags().DurationVar(&cacheOpts.RevisionCacheLockWaitInterval, "revision-cache-lock-wait-interval", CacheOptsDefaults.RevisionCacheLockWaitInterval, "Specifies the amount of time between each check to see if the git cache refs lock has been released") repoFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) @@ -232,11 +245,17 @@ func (c *Cache) TryLockGitRefCache(repo string, lockId string) { } // GetOrLockGitReferences retrieves the git references if they exist, otherwise creates a lock and returns so the caller can populate the cache -func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (lockCreated bool, lockId string, err error) { +func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { // feature flagged for now if !c.revisionCacheLockWaitEnabled { log.Debug("Git references cache lock is disabled") - return false, "", c.GetGitReferences(repo, references) + err = c.GetGitReferences(repo, references) + if err == ErrCacheMiss { + // In the previous iteration this would be fine, but since the caller now expects a valid value unless it obtained the lock + // we need to absorb the error so that the caller continues to refresh the cache + err = nil + } + return true, "", err } var input [][2]string myLockUUID, err := uuid.NewRandom() @@ -265,7 +284,9 @@ func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Refe // Wait for lock, valid value, or timeout time.Sleep(c.revisionCacheLockWaitInterval) } - return false, myLockId, cacheutil.ErrLockWaitExpired + // Timeout waiting for lock + log.Debug("Repository cache was unable to acquire lock or valid data within timeout") + return true, myLockId, nil } // UnlockGitReferences unlocks the key for the Git repository references if needed diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index 190ddfc78fe09..4cb0f740a498f 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -22,8 +22,13 @@ type fixtures struct { func newFixtures() *fixtures { return &fixtures{NewCache( cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Hour)), - 1*time.Minute, - 1*time.Minute, + &CacheOpts{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 10 * time.Second, + RevisionCacheLockWaitInterval: 1 * time.Second, + }, )} } @@ -143,8 +148,13 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) { repoCache := NewCache( cacheutil.NewCache(inMemCache), - 1*time.Minute, - 1*time.Minute, + &CacheOpts{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 10 * time.Second, + RevisionCacheLockWaitInterval: 1 * time.Second, + }, ) response := apiclient.ManifestResponse{ diff --git a/reposerver/repository/mocks/RepositoryCache.go b/reposerver/repository/mocks/RepositoryCache.go new file mode 100644 index 0000000000000..84b742208d3fb --- /dev/null +++ b/reposerver/repository/mocks/RepositoryCache.go @@ -0,0 +1,144 @@ +package mocks + +import ( + "context" + "time" + + "github.com/alicebob/miniredis/v2" + reposerverCache "github.com/argoproj/argo-cd/v2/reposerver/cache" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/go-git/go-git/v5/plumbing" + "github.com/redis/go-redis/v9" +) + +type MockCacheType int + +const ( + MockCacheTypeRedis MockCacheType = iota + MockCacheTypeInMem +) + +type MockRepoCache struct { + cache *cacheutil.Cache + baseCache *reposerverCache.Cache + reposerverCache.CacheOpts + CallCounts map[string]int + ErrorResponses map[string]error +} + +type MockCacheOptions struct { + reposerverCache.CacheOpts + ReadDelay time.Duration + WriteDelay time.Duration + // Map of function name keys to error values that should be returned by the function + ErrorResponses map[string]error +} + +func NewInMemoryRedis() (*redis.Client, func()) { + cacheutil.NewInMemoryCache(5 * time.Second) + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close +} + +func NewMockRepoCache(cacheType MockCacheType, cacheOpts *MockCacheOptions) *MockRepoCache { + redisClient, _ := NewInMemoryRedis() + var cacheClient cacheutil.CacheClient + switch cacheType { + case MockCacheTypeRedis: + cacheClient = cacheutil.NewRedisCache(redisClient, 5*time.Second, cacheutil.RedisCompressionNone) + default: + cacheClient = cacheutil.NewInMemoryCache(5 * time.Second) + } + utilCache := cacheutil.NewCache(&FakeCache{baseCache: cacheClient, ReadDelay: cacheOpts.ReadDelay, WriteDelay: cacheOpts.WriteDelay}) + if cacheOpts.ErrorResponses == nil { + cacheOpts.ErrorResponses = make(map[string]error) + } + baseCacheOptions := reposerverCache.CacheOpts{ + RepoCacheExpiration: cacheOpts.RepoCacheExpiration, + RevisionCacheExpiration: cacheOpts.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: cacheOpts.RevisionCacheLockWaitEnabled, + RevisionCacheLockTimeout: cacheOpts.RevisionCacheLockTimeout, + RevisionCacheLockWaitInterval: cacheOpts.RevisionCacheLockWaitInterval, + } + return &MockRepoCache{ + cache: utilCache, + baseCache: reposerverCache.NewCache(utilCache, &baseCacheOptions), + CacheOpts: baseCacheOptions, + CallCounts: make(map[string]int), + ErrorResponses: cacheOpts.ErrorResponses, + } +} + +func (c *MockRepoCache) GetGitCache() *cacheutil.Cache { + return c.cache +} + +func (c *MockRepoCache) SetGitReferences(repo string, references []*plumbing.Reference) error { + if err := c.ErrorResponses["SetGitReferences"]; err != nil { + return err + } + c.CallCounts["SetGitReferences"]++ + return c.baseCache.SetGitReferences(repo, references) +} + +func (c *MockRepoCache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { + if err := c.ErrorResponses["GetGitReferences"]; err != nil { + return err + } + c.CallCounts["GetGitReferences"]++ + return c.baseCache.GetGitReferences(repo, references) +} + +func (c *MockRepoCache) UnlockGitReferences(repo string, lockId string) error { + if err := c.ErrorResponses["UnlockGitReferences"]; err != nil { + return err + } + c.CallCounts["UnlockGitReferences"]++ + return c.baseCache.UnlockGitReferences(repo, lockId) +} + +func (c *MockRepoCache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { + if err := c.ErrorResponses["GetOrLockGitReferences"]; err != nil { + return false, "", err + } + c.CallCounts["GetOrLockGitReferences"]++ + return c.baseCache.GetOrLockGitReferences(repo, references) +} + +type FakeCache struct { + baseCache cacheutil.CacheClient + ReadDelay time.Duration + WriteDelay time.Duration +} + +func (c *FakeCache) Set(item *cacheutil.Item) error { + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.baseCache.Set(item) +} + +func (c *FakeCache) Get(key string, obj interface{}) error { + if c.ReadDelay > 0 { + time.Sleep(c.ReadDelay) + } + return c.baseCache.Get(key, obj) +} + +func (c *FakeCache) Delete(key string) error { + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.baseCache.Delete(key) +} + +func (c *FakeCache) OnUpdated(ctx context.Context, key string, callback func() error) error { + return c.baseCache.OnUpdated(ctx, key, callback) +} + +func (c *FakeCache) NotifyUpdated(key string) error { + return c.baseCache.NotifyUpdated(key) +} diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a24eb5008c47b..8267bb2f912ab 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -14,6 +14,7 @@ import ( "regexp" "sort" "strings" + "sync" "testing" "time" @@ -28,10 +29,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/yaml" + "github.com/argoproj/argo-cd/v2/common" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/reposerver/apiclient" "github.com/argoproj/argo-cd/v2/reposerver/cache" "github.com/argoproj/argo-cd/v2/reposerver/metrics" + repositorymocks "github.com/argoproj/argo-cd/v2/reposerver/repository/mocks" fileutil "github.com/argoproj/argo-cd/v2/test/fixture/path" "github.com/argoproj/argo-cd/v2/util/argo" cacheutil "github.com/argoproj/argo-cd/v2/util/cache" @@ -96,8 +99,13 @@ func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client) cf(gitClient, helmClient, paths) service := NewService(metrics.NewMetricsServer(), cache.NewCache( cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Minute)), - 1*time.Minute, - 1*time.Minute, + &cache.CacheOpts{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 10 * time.Second, + RevisionCacheLockWaitInterval: 1 * time.Second, + }, ), RepoServerInitConstants{ParallelismLimit: 1}, argo.NewResourceTracking(), &git.NoopCredsStore{}, root) service.newGitClient = func(rawRepoURL string, root string, creds git.Creds, insecure bool, enableLfs bool, prosy string, opts ...git.ClientOpts) (client git.Client, e error) { @@ -2579,9 +2587,11 @@ func TestDirectoryPermissionInitializer(t *testing.T) { require.Error(t, err) } -func initGitRepo(repoPath string, remote string) error { - if err := os.Mkdir(repoPath, 0755); err != nil { - return err +func initGitRepo(repoPath string, remote string, createPath bool, makeEmptyCommit bool) error { + if createPath { + if err := os.Mkdir(repoPath, 0755); err != nil { + return err + } } cmd := exec.Command("git", "init", repoPath) @@ -2589,9 +2599,21 @@ func initGitRepo(repoPath string, remote string) error { if err := cmd.Run(); err != nil { return err } - cmd = exec.Command("git", "remote", "add", "origin", remote) - cmd.Dir = repoPath - return cmd.Run() + if remote != "" { + cmd = exec.Command("git", "remote", "add", "origin", remote) + cmd.Dir = repoPath + if err := cmd.Run(); err != nil { + return err + } + } + if makeEmptyCommit { + cmd = exec.Command("git", "commit", "-m", "Initial commit", "--allow-empty") + cmd.Dir = repoPath + if err := cmd.Run(); err != nil { + return err + } + } + return nil } func TestInit(t *testing.T) { @@ -2604,7 +2626,7 @@ func TestInit(t *testing.T) { }) repoPath := path.Join(dir, "repo1") - require.NoError(t, initGitRepo(repoPath, "https://github.com/argo-cd/test-repo1")) + require.NoError(t, initGitRepo(repoPath, "https://github.com/argo-cd/test-repo1", true, false)) service := newService(".") service.rootDir = dir @@ -2613,7 +2635,7 @@ func TestInit(t *testing.T) { _, err := os.ReadDir(dir) require.Error(t, err) - require.NoError(t, initGitRepo(path.Join(dir, "repo2"), "https://github.com/argo-cd/test-repo2")) + require.NoError(t, initGitRepo(path.Join(dir, "repo2"), "https://github.com/argo-cd/test-repo2", true, false)) } // TestCheckoutRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In @@ -3090,3 +3112,221 @@ func Test_getRepoSanitizerRegex(t *testing.T) { msg = r.ReplaceAllString("error message containing /tmp/_argocd-repo/SENSITIVE/with/trailing/path and other stuff", "") assert.Equal(t, "error message containing /with/trailing/path and other stuff", msg) } + +func TestGetRefs_CacheDisabled(t *testing.T) { + // Test that default get refs with cache disabled does not call GetOrLockGitReferences + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + // Test in-memory and redis + cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} + for _, cacheType := range cacheTypes { + repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ + CacheOpts: cache.CacheOpts{ + RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, + RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 30 * time.Second, + RevisionCacheLockWaitInterval: 100 * time.Millisecond, + }, + ReadDelay: 0, + WriteDelay: 0, + }) + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, false)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + // Unlock should not have been called + assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") + assert.Zero(t, + repoCache.CallCounts["GetOrLockGitReferences"], + "GetOrLockGitReferences should not be called when cache is disabled but was called %d times", + repoCache.CallCounts["GetOrLockGitReferences"]) + } +} + +func TestGetRefs_CacheWithLockDisabled(t *testing.T) { + // Test that when the lock is disabled the default behavior still works correctly + // Also shows the current issue with the git requests due to cache misses + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + // Test in-memory and redis + cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} + for _, cacheType := range cacheTypes { + repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ + CacheOpts: cache.CacheOpts{ + RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, + RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: false, + RevisionCacheLockTimeout: 1 * time.Second, + RevisionCacheLockWaitInterval: 100 * time.Millisecond, + }, + ReadDelay: 0, + WriteDelay: 0, + }) + var wg sync.WaitGroup + numberOfCallers := 10 + for i := 0; i < numberOfCallers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + }() + } + wg.Wait() + // Unlock should not have been called + assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") + // This would normally be all 10 due to them being so close together, + // but since it's timing related, to prevent flakiness we only check that it is greater than half the number of callers + assert.Greater(t, + repoCache.CallCounts["SetGitReferences"], + numberOfCallers/2, + "SetGitReferences have been called more than %d times but was called %d times", + numberOfCallers/2, + repoCache.CallCounts["SetGitReferences"]) + } +} + +func TestGetRefs_CacheWithLockEnabled(t *testing.T) { + // Test that when the lock is enabled there is only one call to SetGitReferences for the same repo which is done after the ls-remote + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + // Test in-memory and redis + cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} + for _, cacheType := range cacheTypes { + repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ + CacheOpts: cache.CacheOpts{ + RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, + RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 1 * time.Second, + RevisionCacheLockWaitInterval: 100 * time.Millisecond, + }, + ReadDelay: 0, + WriteDelay: 0, + }) + var wg sync.WaitGroup + numberOfCallers := 10 + for i := 0; i < numberOfCallers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + }() + } + wg.Wait() + // Unlock should not have been called + assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") + assert.Equal(t, + repoCache.CallCounts["SetGitReferences"], + 1, + "SetGitReferences should have been called only once but was called %d times", + repoCache.CallCounts["SetGitReferences"]) + } +} + +func TestGetRefs_CacheLockTimeout(t *testing.T) { + // Test that the lock timeout is respected + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + // Test in-memory and redis + cacheTypes := []repositorymocks.MockCacheType{ + repositorymocks.MockCacheTypeInMem, + repositorymocks.MockCacheTypeRedis} + for _, cacheType := range cacheTypes { + repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ + CacheOpts: cache.CacheOpts{ + RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, + RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 100 * time.Millisecond, + RevisionCacheLockWaitInterval: 10 * time.Millisecond, + }, + ReadDelay: 0, + // Add a relatively high write delay so that all routines will reach their timeout without obtaining the lock + WriteDelay: 1 * time.Second, + }) + var wg sync.WaitGroup + numberOfCallers := 3 + for i := 0; i < numberOfCallers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + }() + } + wg.Wait() + // Unlock should not have been called + assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") + assert.Equal(t, + repoCache.CallCounts["SetGitReferences"], + numberOfCallers, + "SetGitReferences should have been called %d times but was called %d times", + numberOfCallers, + repoCache.CallCounts["SetGitReferences"]) + } +} + +func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { + // Worst case the ttl on the lock expires and the lock is removed + // however if the holder of the lock fails to update the cache the caller should remove the lock + // to allow other callers to attempt to update the cache as quickly as possible + dir := t.TempDir() + err := initGitRepo(dir, "", false, true) + assert.NoError(t, err, "Could not init git repo") + // Test in-memory and redis + cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} + for _, cacheType := range cacheTypes { + repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ + CacheOpts: cache.CacheOpts{ + RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, + RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 30 * time.Second, + RevisionCacheLockWaitInterval: 100 * time.Millisecond, + }, + ReadDelay: 0, + WriteDelay: 0, + ErrorResponses: map[string]error{ + "SetGitReferences": fmt.Errorf("test error updating cache"), + }, + }) + repoUrl := fmt.Sprintf("file://%s", dir) + client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + gitCache := repoCache.GetGitCache() + assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 1, "UnlockGitReferences should have been called") + var output [][2]string + gitCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output) + assert.Equal(t, 0, len(output), "Expected cache to be empty for key") + } +} diff --git a/util/cache/client.go b/util/cache/client.go index 080f37bf2d49c..a19cc9afb7031 100644 --- a/util/cache/client.go +++ b/util/cache/client.go @@ -8,7 +8,6 @@ import ( var ErrCacheMiss = errors.New("cache: key is missing") var ErrCacheKeyLocked = errors.New("cache: key is locked") -var ErrLockWaitExpired = errors.New("cache: wait for lock exceeded max wait time") var CacheLockedValue = "locked" type Item struct { diff --git a/util/git/client.go b/util/git/client.go index 0f652ed487268..898960a9b3f44 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -56,7 +56,7 @@ type Refs struct { type gitRefCache interface { SetGitReferences(repo string, references []*plumbing.Reference) error GetGitReferences(repo string, references *[]*plumbing.Reference) error - GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (lockCreated bool, lockId string, err error) + GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) UnlockGitReferences(repo string, lockId string) error } @@ -483,17 +483,14 @@ func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { needsUnlock := true if m.gitRefCache != nil && m.loadRefFromCache { var res []*plumbing.Reference - lockCreated, lockId, err := m.gitRefCache.GetOrLockGitReferences(m.repoURL, &res) - if !lockCreated && err == nil { + updateCache, lockId, err := m.gitRefCache.GetOrLockGitReferences(m.repoURL, &res) + if !updateCache && err == nil { // Valid value already in cache return res, nil - } else if err != nil { + } else if !updateCache && err != nil { // Error getting value from cache log.Debugf("Error getting git references from cache: %v", err) return nil, err - } else { - // We don't own the lock so there's no path where we need to unlock - needsUnlock = false } // Defer a soft reset of the cache lock, if the value is set this call will be ignored defer func() { @@ -527,8 +524,10 @@ func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { if err == nil && m.gitRefCache != nil { if err := m.gitRefCache.SetGitReferences(m.repoURL, res); err != nil { log.Warnf("Failed to store git references to cache: %v", err) + } else { + // Since we successfully overwrote the lock with valid data, we don't need to unlock + needsUnlock = false } - needsUnlock = false return res, nil } return res, err diff --git a/util/git/client_test.go b/util/git/client_test.go index d5509edc2b55c..6e91868549f3e 100644 --- a/util/git/client_test.go +++ b/util/git/client_test.go @@ -20,14 +20,23 @@ func runCmd(workingDir string, name string, args ...string) error { return cmd.Run() } -func Test_nativeGitClient_Fetch(t *testing.T) { +func _createEmptyGitRepo() (string, error) { tempDir, err := os.MkdirTemp("", "") - require.NoError(t, err) + if err != nil { + return tempDir, err + } err = runCmd(tempDir, "git", "init") - require.NoError(t, err) + if err != nil { + return tempDir, err + } err = runCmd(tempDir, "git", "commit", "-m", "Initial commit", "--allow-empty") + return tempDir, err +} + +func Test_nativeGitClient_Fetch(t *testing.T) { + tempDir, err := _createEmptyGitRepo() require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") @@ -41,13 +50,7 @@ func Test_nativeGitClient_Fetch(t *testing.T) { } func Test_nativeGitClient_Fetch_Prune(t *testing.T) { - tempDir, err := os.MkdirTemp("", "") - require.NoError(t, err) - - err = runCmd(tempDir, "git", "init") - require.NoError(t, err) - - err = runCmd(tempDir, "git", "commit", "-m", "Initial commit", "--allow-empty") + tempDir, err := _createEmptyGitRepo() require.NoError(t, err) client, err := NewClient(fmt.Sprintf("file://%s", tempDir), NopCreds{}, true, false, "") diff --git a/util/webhook/webhook_test.go b/util/webhook/webhook_test.go index b241d7c671841..04c1683dfd9f6 100644 --- a/util/webhook/webhook_test.go +++ b/util/webhook/webhook_test.go @@ -69,8 +69,13 @@ func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects return NewHandler("argocd", applicationNamespaces, appClientset, &settings.ArgoCDSettings{}, &fakeSettingsSrc{}, cache.NewCache( cacheClient, - 1*time.Minute, - 1*time.Minute, + &cache.CacheOpts{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + RevisionCacheLockWaitEnabled: true, + RevisionCacheLockTimeout: 10 * time.Second, + RevisionCacheLockWaitInterval: 1 * time.Second, + }, ), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{}) } From 617ae72023d3acc5dbeb89da70037dd7b4dbb772 Mon Sep 17 00:00:00 2001 From: nromriell Date: Sun, 12 Nov 2023 21:10:54 -0800 Subject: [PATCH 3/5] chore: finish up tests, fixes, cleanup Signed-off-by: nromriell --- reposerver/cache/cache.go | 12 ++- reposerver/cache/cache_test.go | 48 ++++++++- .../repository/mocks/RepositoryCache.go | 29 +++-- reposerver/repository/repository_test.go | 5 + util/cache/cache.go | 5 +- util/cache/cache_test.go | 102 +++++++++++++----- 6 files changed, 162 insertions(+), 39 deletions(-) diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 96b015c239dff..db1de48a61443 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -214,7 +214,10 @@ func (c *Cache) SetGitReferences(repo string, references []*plumbing.Reference) func GitRefCacheItemToReferences(cacheItem [][2]string) []*plumbing.Reference { var res []*plumbing.Reference for i := range cacheItem { - res = append(res, plumbing.NewReferenceFromStrings(cacheItem[i][0], cacheItem[i][1])) + // Skip empty data + if cacheItem[i][0] != "" || cacheItem[i][1] != "" { + res = append(res, plumbing.NewReferenceFromStrings(cacheItem[i][0], cacheItem[i][1])) + } } return res } @@ -231,6 +234,9 @@ func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference) // TryLockGitRefCache attempts to lock the key for the Git repository references if the key doesn't exist func (c *Cache) TryLockGitRefCache(repo string, lockId string) { + // This try set with DisableOverwrite is important for making sure that only one process is able to claim ownership + // A normal get + set, or just set would cause ownership to go to whoever the last writer was, and during race conditions + // leads to duplicate requests err := c.cache.SetCacheItem(&cacheutil.Item{ Key: gitRefsKey(repo), Object: [][2]string{{cacheutil.CacheLockedValue, lockId}}, @@ -238,8 +244,8 @@ func (c *Cache) TryLockGitRefCache(repo string, lockId string) { DisableOverwrite: true, }, false) if err != nil { - // The key already existing does not produce an error for go-redis so we'll use the get to validate - // In memory would produce an error, but ignoring provides a consistent flow + // The key already existing does not produce an error for go-redis so throwing it here for in-memory + // would cause inconsistent behavior, use a get on the key to determine if the lock was successful log.Debug("Error setting git references cache lock: ", err) } } diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index 4cb0f740a498f..1062b3670ac32 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -3,6 +3,7 @@ package cache import ( "encoding/json" "errors" + "fmt" "strings" "testing" "time" @@ -26,7 +27,7 @@ func newFixtures() *fixtures { RepoCacheExpiration: 1 * time.Minute, RevisionCacheExpiration: 1 * time.Minute, RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 10 * time.Second, + RevisionCacheLockTimeout: 30 * time.Second, RevisionCacheLockWaitInterval: 1 * time.Second, }, )} @@ -319,3 +320,48 @@ func TestCachedManifestResponse_ShallowCopyExpectedFields(t *testing.T) { } } + +func TestGitRefCacheItemToReferences_DataChecks(t *testing.T) { + references := GitRefCacheItemToReferences(nil) + assert.Equal(t, 0, len(references), "No data should be handled gracefully by returning an empty slice") + references = GitRefCacheItemToReferences([][2]string{{"", ""}}) + assert.Equal(t, 0, len(references), "Empty data should be discarded") + references = GitRefCacheItemToReferences([][2]string{{"test", ""}}) + assert.Equal(t, 1, len(references), "Just the key being set should not be discarded") + assert.Equal(t, "test", references[0].Name().String(), "Name should be set and equal test") + references = GitRefCacheItemToReferences([][2]string{{"", "ref: test1"}}) + assert.Equal(t, 1, len(references), "Just the value being set should not be discarded") + assert.Equal(t, "test1", references[0].Target().String(), "Target should be set and equal test1") + references = GitRefCacheItemToReferences([][2]string{{"test2", "ref: test2"}}) + assert.Equal(t, 1, len(references), "Valid data is should be preserved") + assert.Equal(t, "test2", references[0].Name().String(), "Name should be set and equal test2") + assert.Equal(t, "test2", references[0].Target().String(), "Target should be set and equal test2") + references = GitRefCacheItemToReferences([][2]string{{"test3", "ref: test3"}, {"test4", "ref: test4"}}) + assert.Equal(t, 2, len(references), "Valid data is should be preserved") + assert.Equal(t, "test3", references[0].Name().String(), "Name should be set and equal test3") + assert.Equal(t, "test3", references[0].Target().String(), "Target should be set and equal test3") + assert.Equal(t, "test4", references[1].Name().String(), "Name should be set and equal test4") + assert.Equal(t, "test4", references[1].Target().String(), "Target should be set and equal test4") +} + +func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) { + cache := newFixtures().Cache + // Test setting the lock + cache.TryLockGitRefCache("my-repo-url", "my-lock-id") + var output [][2]string + key := fmt.Sprintf("git-refs|%s", "my-repo-url") + cache.cache.GetItem(key, &output) + assert.Equal(t, "locked", output[0][0], "The lock should be set") + assert.Equal(t, "my-lock-id", output[0][1], "The lock should be set to the provided lock id") + // Test not being able to overwrite the lock + cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + cache.cache.GetItem(key, &output) + assert.Equal(t, "locked", output[0][0], "The lock should not have changed") + assert.Equal(t, "my-lock-id", output[0][1], "The lock should not have changed") + // Test can overwrite once there is nothing set + cache.cache.SetItem(key, [][2]string{}, 0, true) + cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + cache.cache.GetItem(key, &output) + assert.Equal(t, "locked", output[0][0], "The lock should be set") + assert.Equal(t, "other-lock-id", output[0][1], "The lock id should have changed to other-lock-id") +} diff --git a/reposerver/repository/mocks/RepositoryCache.go b/reposerver/repository/mocks/RepositoryCache.go index 84b742208d3fb..37511765715d5 100644 --- a/reposerver/repository/mocks/RepositoryCache.go +++ b/reposerver/repository/mocks/RepositoryCache.go @@ -2,6 +2,7 @@ package mocks import ( "context" + "sync" "time" "github.com/alicebob/miniredis/v2" @@ -22,8 +23,10 @@ type MockRepoCache struct { cache *cacheutil.Cache baseCache *reposerverCache.Cache reposerverCache.CacheOpts - CallCounts map[string]int - ErrorResponses map[string]error + CallCounts map[string]int + ErrorResponses map[string]error + StopRedisCallback func() + CallCountsMutex *sync.Mutex } type MockCacheOptions struct { @@ -44,7 +47,7 @@ func NewInMemoryRedis() (*redis.Client, func()) { } func NewMockRepoCache(cacheType MockCacheType, cacheOpts *MockCacheOptions) *MockRepoCache { - redisClient, _ := NewInMemoryRedis() + redisClient, stopRedis := NewInMemoryRedis() var cacheClient cacheutil.CacheClient switch cacheType { case MockCacheTypeRedis: @@ -64,11 +67,13 @@ func NewMockRepoCache(cacheType MockCacheType, cacheOpts *MockCacheOptions) *Moc RevisionCacheLockWaitInterval: cacheOpts.RevisionCacheLockWaitInterval, } return &MockRepoCache{ - cache: utilCache, - baseCache: reposerverCache.NewCache(utilCache, &baseCacheOptions), - CacheOpts: baseCacheOptions, - CallCounts: make(map[string]int), - ErrorResponses: cacheOpts.ErrorResponses, + cache: utilCache, + baseCache: reposerverCache.NewCache(utilCache, &baseCacheOptions), + CacheOpts: baseCacheOptions, + CallCounts: make(map[string]int), + CallCountsMutex: &sync.Mutex{}, + ErrorResponses: cacheOpts.ErrorResponses, + StopRedisCallback: stopRedis, } } @@ -80,7 +85,9 @@ func (c *MockRepoCache) SetGitReferences(repo string, references []*plumbing.Ref if err := c.ErrorResponses["SetGitReferences"]; err != nil { return err } + c.CallCountsMutex.Lock() c.CallCounts["SetGitReferences"]++ + c.CallCountsMutex.Unlock() return c.baseCache.SetGitReferences(repo, references) } @@ -88,7 +95,9 @@ func (c *MockRepoCache) GetGitReferences(repo string, references *[]*plumbing.Re if err := c.ErrorResponses["GetGitReferences"]; err != nil { return err } + c.CallCountsMutex.Lock() c.CallCounts["GetGitReferences"]++ + c.CallCountsMutex.Unlock() return c.baseCache.GetGitReferences(repo, references) } @@ -96,7 +105,9 @@ func (c *MockRepoCache) UnlockGitReferences(repo string, lockId string) error { if err := c.ErrorResponses["UnlockGitReferences"]; err != nil { return err } + c.CallCountsMutex.Lock() c.CallCounts["UnlockGitReferences"]++ + c.CallCountsMutex.Unlock() return c.baseCache.UnlockGitReferences(repo, lockId) } @@ -104,7 +115,9 @@ func (c *MockRepoCache) GetOrLockGitReferences(repo string, references *[]*plumb if err := c.ErrorResponses["GetOrLockGitReferences"]; err != nil { return false, "", err } + c.CallCountsMutex.Lock() c.CallCounts["GetOrLockGitReferences"]++ + c.CallCountsMutex.Unlock() return c.baseCache.GetOrLockGitReferences(repo, references) } diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 8267bb2f912ab..f27b96db2e0eb 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -3132,6 +3132,7 @@ func TestGetRefs_CacheDisabled(t *testing.T) { ReadDelay: 0, WriteDelay: 0, }) + t.Cleanup(repoCache.StopRedisCallback) client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, false)) require.NoError(t, err) refs, err := client.LsRefs() @@ -3168,6 +3169,7 @@ func TestGetRefs_CacheWithLockDisabled(t *testing.T) { ReadDelay: 0, WriteDelay: 0, }) + t.Cleanup(repoCache.StopRedisCallback) var wg sync.WaitGroup numberOfCallers := 10 for i := 0; i < numberOfCallers; i++ { @@ -3216,6 +3218,7 @@ func TestGetRefs_CacheWithLockEnabled(t *testing.T) { ReadDelay: 0, WriteDelay: 0, }) + t.Cleanup(repoCache.StopRedisCallback) var wg sync.WaitGroup numberOfCallers := 10 for i := 0; i < numberOfCallers; i++ { @@ -3264,6 +3267,7 @@ func TestGetRefs_CacheLockTimeout(t *testing.T) { // Add a relatively high write delay so that all routines will reach their timeout without obtaining the lock WriteDelay: 1 * time.Second, }) + t.Cleanup(repoCache.StopRedisCallback) var wg sync.WaitGroup numberOfCallers := 3 for i := 0; i < numberOfCallers; i++ { @@ -3315,6 +3319,7 @@ func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { "SetGitReferences": fmt.Errorf("test error updating cache"), }, }) + t.Cleanup(repoCache.StopRedisCallback) repoUrl := fmt.Sprintf("file://%s", dir) client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) require.NoError(t, err) diff --git a/util/cache/cache.go b/util/cache/cache.go index 0b77c200952d8..5e4905818170d 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -172,11 +172,14 @@ func (c *Cache) generateFullKey(key string) string { } func (c *Cache) SetCacheItem(item *Item, delete bool) error { + if item == nil { + return fmt.Errorf("cannot set nil item in cache") + } item.Key = c.generateFullKey(item.Key) if delete { return c.client.Delete(item.Key) } else { - if item == nil { + if item.Object == nil { return fmt.Errorf("cannot set item to nil for key %s", item.Key) } return c.client.Set(item) diff --git a/util/cache/cache_test.go b/util/cache/cache_test.go index 6a1b519e7eb57..c9e6174eb9201 100644 --- a/util/cache/cache_test.go +++ b/util/cache/cache_test.go @@ -1,9 +1,13 @@ package cache import ( + "fmt" "testing" "time" + "github.com/alicebob/miniredis/v2" + "github.com/argoproj/argo-cd/v2/common" + "github.com/redis/go-redis/v9" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -14,33 +18,79 @@ func TestAddCacheFlagsToCmd(t *testing.T) { assert.Equal(t, 24*time.Hour, cache.client.(*redisCache).expiration) } +func NewInMemoryRedis() (*redis.Client, func()) { + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close +} + func TestCacheClient(t *testing.T) { + clientRedis, stopRedis := NewInMemoryRedis() + defer stopRedis() + redisCache := NewRedisCache(clientRedis, 5*time.Second, RedisCompressionNone) + clientMemCache := NewInMemoryCache(60 * time.Second) + // Run tests for both Redis and InMemoryCache + for _, client := range []CacheClient{clientMemCache, redisCache} { + cache := NewCache(client) + t.Run("SetItem", func(t *testing.T) { + err := cache.SetItem("foo", "bar", 60*time.Second, false) + assert.NoError(t, err) + }) + t.Run("SetCacheItem", func(t *testing.T) { + err := cache.SetCacheItem(&Item{Key: "foo", Object: "bar", Expiration: 60 * time.Second}, false) + assert.NoError(t, err) + var output string + err = cache.GetItem("foo", &output) + assert.NoError(t, err) + assert.Equal(t, "bar", output) + }) + t.Run("SetCacheItem W/Disable Overwrite", func(t *testing.T) { + err := cache.SetCacheItem(&Item{Key: "foo", Object: "bar", Expiration: 60 * time.Second}, false) + assert.NoError(t, err) + var output string + err = cache.GetItem("foo", &output) + assert.NoError(t, err) + assert.Equal(t, "bar", output) + err = cache.SetCacheItem(&Item{Key: "foo", Object: "baz", Expiration: 60 * time.Second, DisableOverwrite: true}, false) + assert.NoError(t, err) + err = cache.GetItem("foo", &output) + assert.NoError(t, err) + assert.Equal(t, "bar", output, "output should not have changed with DisableOverwrite set to true") + }) + t.Run("GetItem", func(t *testing.T) { + var val string + err := cache.GetItem("foo", &val) + assert.NoError(t, err) + assert.Equal(t, "bar", val) + }) + t.Run("DeleteItem", func(t *testing.T) { + err := cache.SetItem("foo", "bar", 0, true) + assert.NoError(t, err) + var val string + err = cache.GetItem("foo", &val) + assert.Error(t, err) + assert.Empty(t, val) + }) + t.Run("Check for nil items", func(t *testing.T) { + err := cache.SetItem("foo", nil, 0, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot set item") + err = cache.SetCacheItem(nil, false) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot set nil item") + err = cache.GetItem("foo", nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot get item") + }) + } +} + +// Smoke test to ensure key changes aren't done accidentally +func TestGenerateCacheKey(t *testing.T) { client := NewInMemoryCache(60 * time.Second) cache := NewCache(client) - t.Run("SetItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 60*time.Second, false) - assert.NoError(t, err) - }) - t.Run("GetItem", func(t *testing.T) { - var val string - err := cache.GetItem("foo", &val) - assert.NoError(t, err) - assert.Equal(t, "bar", val) - }) - t.Run("DeleteItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 0, true) - assert.NoError(t, err) - var val string - err = cache.GetItem("foo", &val) - assert.Error(t, err) - assert.Empty(t, val) - }) - t.Run("Check for nil items", func(t *testing.T) { - err := cache.SetItem("foo", nil, 0, false) - assert.Error(t, err) - assert.Contains(t, err.Error(), "cannot set item") - err = cache.GetItem("foo", nil) - assert.Error(t, err) - assert.Contains(t, err.Error(), "cannot get item") - }) + testKey := cache.generateFullKey("testkey") + assert.Equal(t, fmt.Sprintf("testkey|%s", common.CacheVersion), testKey) } From aafad63be6d43b5a6ebbeba177daa1fa29a5fe88 Mon Sep 17 00:00:00 2001 From: nromriell Date: Sun, 12 Nov 2023 22:07:39 -0800 Subject: [PATCH 4/5] chore: lint fixes Signed-off-by: nromriell --- reposerver/cache/cache_test.go | 12 ++++++++---- reposerver/repository/repository_test.go | 3 ++- util/cache/inmemory.go | 3 ++- util/git/client.go | 3 ++- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index 1062b3670ac32..d6b9cfb0f5d09 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -350,18 +350,22 @@ func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) { cache.TryLockGitRefCache("my-repo-url", "my-lock-id") var output [][2]string key := fmt.Sprintf("git-refs|%s", "my-repo-url") - cache.cache.GetItem(key, &output) + err := cache.cache.GetItem(key, &output) + assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should be set") assert.Equal(t, "my-lock-id", output[0][1], "The lock should be set to the provided lock id") // Test not being able to overwrite the lock cache.TryLockGitRefCache("my-repo-url", "other-lock-id") - cache.cache.GetItem(key, &output) + err = cache.cache.GetItem(key, &output) + assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should not have changed") assert.Equal(t, "my-lock-id", output[0][1], "The lock should not have changed") // Test can overwrite once there is nothing set - cache.cache.SetItem(key, [][2]string{}, 0, true) + err = cache.cache.SetItem(key, [][2]string{}, 0, true) + assert.NoError(t, err) cache.TryLockGitRefCache("my-repo-url", "other-lock-id") - cache.cache.GetItem(key, &output) + err = cache.cache.GetItem(key, &output) + assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should be set") assert.Equal(t, "other-lock-id", output[0][1], "The lock id should have changed to other-lock-id") } diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index f27b96db2e0eb..a5e1b155cf86d 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -3331,7 +3331,8 @@ func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { gitCache := repoCache.GetGitCache() assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 1, "UnlockGitReferences should have been called") var output [][2]string - gitCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output) + err = gitCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output) + assert.Error(t, err, "Should be a cache miss") assert.Equal(t, 0, len(output), "Expected cache to be empty for key") } } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index 13828c6f5e4f9..a07dfd47528a3 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -30,7 +30,8 @@ func (i *InMemoryCache) Set(item *Item) error { return err } if item.DisableOverwrite { - i.memCache.Add(item.Key, buf, item.Expiration) + // go-redis doesn't throw an error on Set with NX, so absorbing here to keep the interface consistent + _ = i.memCache.Add(item.Key, buf, item.Expiration) } else { i.memCache.Set(item.Key, buf, item.Expiration) } diff --git a/util/git/client.go b/util/git/client.go index 898960a9b3f44..38bff40b27d29 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -495,7 +495,8 @@ func (m *nativeGitClient) getRefs() ([]*plumbing.Reference, error) { // Defer a soft reset of the cache lock, if the value is set this call will be ignored defer func() { if needsUnlock { - m.gitRefCache.UnlockGitReferences(m.repoURL, lockId) + err := m.gitRefCache.UnlockGitReferences(m.repoURL, lockId) + log.Debugf("Error unlocking git references from cache: %v", err) } }() } From 33808ec225b18387bd665b333b766c6737f4d4d2 Mon Sep 17 00:00:00 2001 From: nromriell Date: Sat, 18 Nov 2023 20:42:32 -0800 Subject: [PATCH 5/5] feat: move to two level cache, reconfigure cache interface to provide consistent behavior Signed-off-by: nromriell --- cmd/argocd/commands/headless/headless.go | 4 +- .../argocd-repo-server-deployment.yaml | 18 - manifests/core-install.yaml | 18 - manifests/ha/install.yaml | 18 - manifests/ha/namespace-install.yaml | 18 - manifests/install.yaml | 18 - manifests/namespace-install.yaml | 18 - reposerver/cache/cache.go | 230 +++++---- reposerver/cache/cache_test.go | 469 ++++++++++++++++-- reposerver/cache/mocks/reposervercache.go | 88 ++++ .../repository/mocks/RepositoryCache.go | 157 ------ reposerver/repository/repository_test.go | 394 +++++++-------- server/server.go | 4 +- util/cache/appstate/cache.go | 6 +- util/cache/cache.go | 63 ++- util/cache/cache_test.go | 40 +- util/cache/client.go | 25 +- util/cache/client_test.go | 6 +- util/cache/inmemory.go | 6 +- util/cache/inmemory_test.go | 4 +- util/cache/mocks/cachclient.go | 65 +++ util/cache/redis.go | 6 +- util/cache/redis_test.go | 6 +- util/cache/twolevelclient.go | 33 +- util/git/client.go | 1 - util/webhook/webhook_test.go | 9 +- 26 files changed, 1002 insertions(+), 722 deletions(-) create mode 100644 reposerver/cache/mocks/reposervercache.go delete mode 100644 reposerver/repository/mocks/RepositoryCache.go create mode 100644 util/cache/mocks/cachclient.go diff --git a/cmd/argocd/commands/headless/headless.go b/cmd/argocd/commands/headless/headless.go index 070d9c9c83bcb..8c715b8d24508 100644 --- a/cmd/argocd/commands/headless/headless.go +++ b/cmd/argocd/commands/headless/headless.go @@ -78,9 +78,9 @@ func (c *forwardCacheClient) Set(item *cache.Item) error { }) } -func (c *forwardCacheClient) Get(key string, obj interface{}) error { +func (c *forwardCacheClient) Get(item *cache.Item) error { return c.doLazy(func(client cache.CacheClient) error { - return client.Get(key, obj) + return client.Get(item) }) } diff --git a/manifests/base/repo-server/argocd-repo-server-deployment.yaml b/manifests/base/repo-server/argocd-repo-server-deployment.yaml index b7dfa9b9455af..728c80c14bc2a 100644 --- a/manifests/base/repo-server/argocd-repo-server-deployment.yaml +++ b/manifests/base/repo-server/argocd-repo-server-deployment.yaml @@ -90,24 +90,6 @@ spec: name: argocd-cmd-params-cm key: reposerver.repo.cache.expiration optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index 1eedd89131839..2469f171351fe 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -21232,24 +21232,6 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index 88e8810ac9b41..0f637cd786dc2 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -22719,24 +22719,6 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/ha/namespace-install.yaml b/manifests/ha/namespace-install.yaml index 31213ad78aff8..33ada40e37004 100644 --- a/manifests/ha/namespace-install.yaml +++ b/manifests/ha/namespace-install.yaml @@ -2106,24 +2106,6 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/install.yaml b/manifests/install.yaml index f3a54f82d5198..cb4cda559caf0 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -21765,24 +21765,6 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index b5ab71345cdb8..9e6f98030d89b 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -1152,24 +1152,6 @@ spec: key: reposerver.repo.cache.expiration name: argocd-cmd-params-cm optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.enabled - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_TIMEOUT - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.timeout - name: argocd-cmd-params-cm - optional: true - - name: ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL - valueFrom: - configMapKeyRef: - key: reposerver.repo.cache.lock.interval - name: argocd-cmd-params-cm - optional: true - name: REDIS_SERVER valueFrom: configMapKeyRef: diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index db1de48a61443..241a5a2d5934c 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -25,32 +25,16 @@ import ( "github.com/argoproj/argo-cd/v2/util/hash" ) +//go:generate go run github.com/vektra/mockery/v2@v2.37.0 --name=Cache + var ErrCacheMiss = cacheutil.ErrCacheMiss var ErrCacheKeyLocked = cacheutil.ErrCacheKeyLocked -var CacheOptsDefaults = &CacheOpts{ - RepoCacheExpiration: env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), - RevisionCacheExpiration: env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), - RevisionCacheLockWaitEnabled: env.ParseBoolFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_ENABLED", false), - RevisionCacheLockTimeout: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_TIMEOUT", 10*time.Second, 0, math.MaxInt64), - RevisionCacheLockWaitInterval: env.ParseDurationFromEnv("ARGOCD_REVISION_CACHE_LOCK_WAIT_INTERVAL", 1*time.Second, 0, math.MaxInt64), -} - -type CacheOpts struct { - RepoCacheExpiration time.Duration - RevisionCacheExpiration time.Duration - RevisionCacheLockWaitEnabled bool - RevisionCacheLockTimeout time.Duration - RevisionCacheLockWaitInterval time.Duration -} - type Cache struct { - cache *cacheutil.Cache - repoCacheExpiration time.Duration - revisionCacheExpiration time.Duration - revisionCacheLockWaitEnabled bool - revisionCacheLockTimeout time.Duration - revisionCacheLockWaitInterval time.Duration + cache *cacheutil.Cache + repoCacheExpiration time.Duration + revisionCacheExpiration time.Duration + revisionCacheLockTimeout time.Duration } // ClusterRuntimeInfo holds cluster runtime information @@ -61,39 +45,25 @@ type ClusterRuntimeInfo interface { GetKubeVersion() string } -func NewCache(cache *cacheutil.Cache, cacheOpts *CacheOpts) *Cache { - if cacheOpts == nil { - cacheOpts = CacheOptsDefaults - } - if cacheOpts.RevisionCacheLockWaitEnabled && cacheOpts.RevisionCacheLockWaitInterval > cacheOpts.RevisionCacheLockTimeout { - log.Warnf("revision-cache-lock-wait-interval (%s) is greater than revision-cache-lock-timeout (%s) reducing interval to revision-cache-lock-timeout", cacheOpts.RevisionCacheLockWaitInterval, cacheOpts.RevisionCacheLockTimeout) - cacheOpts.RevisionCacheLockWaitInterval = cacheOpts.RevisionCacheLockTimeout - } - return &Cache{cache, - cacheOpts.RepoCacheExpiration, - cacheOpts.RevisionCacheExpiration, - cacheOpts.RevisionCacheLockWaitEnabled, - cacheOpts.RevisionCacheLockTimeout, - cacheOpts.RevisionCacheLockWaitInterval} +func NewCache(cache *cacheutil.Cache, repoCacheExpiration time.Duration, revisionCacheExpiration time.Duration) *Cache { + return &Cache{cache, repoCacheExpiration, revisionCacheExpiration, 10 * time.Second} } func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { - var cacheOpts *CacheOpts = &CacheOpts{} + var repoCacheExpiration time.Duration + var revisionCacheExpiration time.Duration - cmd.Flags().DurationVar(&cacheOpts.RepoCacheExpiration, "repo-cache-expiration", CacheOptsDefaults.RepoCacheExpiration, "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") - cmd.Flags().DurationVar(&cacheOpts.RevisionCacheExpiration, "revision-cache-expiration", CacheOptsDefaults.RevisionCacheExpiration, "Cache expiration for cached revision") - cmd.Flags().BoolVar(&cacheOpts.RevisionCacheLockWaitEnabled, "enable-revision-cache-lock", CacheOptsDefaults.RevisionCacheLockWaitEnabled, "Enables a lock to prevent duplicate git requests during revision cache updates") - cmd.Flags().DurationVar(&cacheOpts.RevisionCacheLockTimeout, "revision-cache-lock-timeout", CacheOptsDefaults.RevisionCacheLockTimeout, "Specifies the max amount of time each routine making identical git requests will wait for the git reference cache to update before making a new git requests") - cmd.Flags().DurationVar(&cacheOpts.RevisionCacheLockWaitInterval, "revision-cache-lock-wait-interval", CacheOptsDefaults.RevisionCacheLockWaitInterval, "Specifies the amount of time between each check to see if the git cache refs lock has been released") + cmd.Flags().DurationVar(&repoCacheExpiration, "repo-cache-expiration", env.ParseDurationFromEnv("ARGOCD_REPO_CACHE_EXPIRATION", 24*time.Hour, 0, math.MaxInt64), "Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data") + cmd.Flags().DurationVar(&revisionCacheExpiration, "revision-cache-expiration", env.ParseDurationFromEnv("ARGOCD_RECONCILIATION_TIMEOUT", 3*time.Minute, 0, math.MaxInt64), "Cache expiration for cached revision") repoFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) return func() (*Cache, error) { - cache, err := repoFactory() + cache, err := repoFactory(true) if err != nil { return nil, fmt.Errorf("error adding cache flags to cmd: %w", err) } - return NewCache(cache, cacheOpts), nil + return NewCache(cache, repoCacheExpiration, revisionCacheExpiration), nil } } @@ -176,12 +146,18 @@ func listApps(repoURL, revision string) string { func (c *Cache) ListApps(repoUrl, revision string) (map[string]string, error) { res := make(map[string]string) - err := c.cache.GetItem(listApps(repoUrl, revision), &res) + err := c.cache.GetItem(listApps(repoUrl, revision), &res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) return res, err } func (c *Cache) SetApps(repoUrl, revision string, apps map[string]string) error { - return c.cache.SetItem(listApps(repoUrl, revision), apps, c.repoCacheExpiration, apps == nil) + return c.cache.SetItem( + listApps(repoUrl, revision), + apps, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: apps == nil, + CacheType: cacheutil.CacheTypeExternal}) } func helmIndexRefsKey(repo string) string { @@ -190,12 +166,19 @@ func helmIndexRefsKey(repo string) string { // SetHelmIndex stores helm repository index.yaml content to cache func (c *Cache) SetHelmIndex(repo string, indexData []byte) error { - return c.cache.SetItem(helmIndexRefsKey(repo), indexData, c.revisionCacheExpiration, false) + if indexData == nil { + // Logged as warning upstream + return fmt.Errorf("helm index data is nil, skipping cache") + } + return c.cache.SetItem( + helmIndexRefsKey(repo), + indexData, + &cacheutil.CacheActionOpts{Expiration: c.revisionCacheExpiration}) } // GetHelmIndex retrieves helm repository index.yaml content from cache func (c *Cache) GetHelmIndex(repo string, indexData *[]byte) error { - return c.cache.GetItem(helmIndexRefsKey(repo), indexData) + return c.cache.GetItem(helmIndexRefsKey(repo), indexData, nil) } func gitRefsKey(repo string) string { @@ -208,10 +191,10 @@ func (c *Cache) SetGitReferences(repo string, references []*plumbing.Reference) for i := range references { input = append(input, references[i].Strings()) } - return c.cache.SetItem(gitRefsKey(repo), input, c.revisionCacheExpiration, false) + return c.cache.SetItem(gitRefsKey(repo), input, &cacheutil.CacheActionOpts{Expiration: c.revisionCacheExpiration}) } -func GitRefCacheItemToReferences(cacheItem [][2]string) []*plumbing.Reference { +func GitRefCacheItemToReferences(cacheItem [][2]string) *[]*plumbing.Reference { var res []*plumbing.Reference for i := range cacheItem { // Skip empty data @@ -219,51 +202,41 @@ func GitRefCacheItemToReferences(cacheItem [][2]string) []*plumbing.Reference { res = append(res, plumbing.NewReferenceFromStrings(cacheItem[i][0], cacheItem[i][1])) } } - return res -} - -// GetGitReferences retrieves resolved Git repository references from cache -func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { - var input [][2]string - if err := c.cache.GetItem(gitRefsKey(repo), &input); err != nil { - return err - } - *references = GitRefCacheItemToReferences(input) - return nil + return &res } // TryLockGitRefCache attempts to lock the key for the Git repository references if the key doesn't exist -func (c *Cache) TryLockGitRefCache(repo string, lockId string) { +func (c *Cache) TryLockGitRefCache(repo string, lockId string) error { // This try set with DisableOverwrite is important for making sure that only one process is able to claim ownership // A normal get + set, or just set would cause ownership to go to whoever the last writer was, and during race conditions // leads to duplicate requests - err := c.cache.SetCacheItem(&cacheutil.Item{ - Key: gitRefsKey(repo), - Object: [][2]string{{cacheutil.CacheLockedValue, lockId}}, + err := c.cache.SetItem(gitRefsKey(repo), [][2]string{{cacheutil.CacheLockedValue, lockId}}, &cacheutil.CacheActionOpts{ Expiration: c.revisionCacheLockTimeout, DisableOverwrite: true, - }, false) - if err != nil { - // The key already existing does not produce an error for go-redis so throwing it here for in-memory - // would cause inconsistent behavior, use a get on the key to determine if the lock was successful - log.Debug("Error setting git references cache lock: ", err) + CacheType: cacheutil.CacheTypeExternal}) + return err +} + +func (c *Cache) GetGitReferences(repo string, lockId string, cacheType cacheutil.CacheType) (lockOwner string, references *[]*plumbing.Reference, err error) { + var input [][2]string + err = c.cache.GetItem(gitRefsKey(repo), &input, &cacheutil.CacheActionOpts{CacheType: cacheType, Expiration: c.revisionCacheExpiration}) + if err == ErrCacheMiss { + // Expected + return "", nil, nil + } else if err == nil && len(input) > 0 && len(input[0]) > 0 { + if input[0][0] != cacheutil.CacheLockedValue { + // Valid value in cache, convert to plumbing.Reference and return + return "", GitRefCacheItemToReferences(input), nil + } else { + // The key lock is being held + return input[0][1], nil, nil + } } + return "", nil, err } // GetOrLockGitReferences retrieves the git references if they exist, otherwise creates a lock and returns so the caller can populate the cache func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { - // feature flagged for now - if !c.revisionCacheLockWaitEnabled { - log.Debug("Git references cache lock is disabled") - err = c.GetGitReferences(repo, references) - if err == ErrCacheMiss { - // In the previous iteration this would be fine, but since the caller now expects a valid value unless it obtained the lock - // we need to absorb the error so that the caller continues to refresh the cache - err = nil - } - return true, "", err - } - var input [][2]string myLockUUID, err := uuid.NewRandom() if err != nil { log.Debug("Error generating git references cache lock id: ", err) @@ -271,24 +244,36 @@ func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Refe } // We need to be able to identify that our lock was the successful one, otherwise we'll still have duplicate requests myLockId := myLockUUID.String() + // Value matches the ttl on the lock in TryLockGitRefCache waitUntil := time.Now().Add(c.revisionCacheLockTimeout) // Wait only the maximum amount of time configured for the lock for time.Now().Before(waitUntil) { - // Attempt to get the lock - c.TryLockGitRefCache(repo, myLockId) - err = c.cache.GetItem(gitRefsKey(repo), &input) - if err == nil && len(input) > 0 && len(input[0]) > 0 { - if input[0][0] != cacheutil.CacheLockedValue { - // Valid value in cache, convert to plumbing.Reference and return - *references = GitRefCacheItemToReferences(input) - return false, myLockId, err - } else if input[0][1] == myLockId { - // Our lock was successful - return true, myLockId, nil + // Attempt to retrieve the key from local cache only + if _, cacheReferences, err := c.GetGitReferences(repo, myLockId, cacheutil.CacheTypeInMemory); err != nil || cacheReferences != nil { + if cacheReferences != nil { + *references = *cacheReferences + } + return false, myLockId, err + } + // Could not get key locally attempt to get the lock + err = c.TryLockGitRefCache(repo, myLockId) + if err != nil { + // Log but ignore this error since we'll want to retry, failing to obtain the lock should not throw an error + log.Errorf("Error attempting to acquire git references cache lock: %v", err) + } + // Attempt to retrieve the key again to see if we have the lock, or the key was populated + if lockOwner, cacheReferences, err := c.GetGitReferences(repo, myLockId, cacheutil.CacheTypeTwoLevel); err != nil || cacheReferences != nil { + if cacheReferences != nil { + // Someone else populated the key + *references = *cacheReferences } + return false, myLockId, err + } else if lockOwner == myLockId { + // We have the lock, populate the key + return true, myLockId, nil } // Wait for lock, valid value, or timeout - time.Sleep(c.revisionCacheLockWaitInterval) + time.Sleep(1 * time.Second) } // Timeout waiting for lock log.Debug("Repository cache was unable to acquire lock or valid data within timeout") @@ -299,13 +284,13 @@ func (c *Cache) GetOrLockGitReferences(repo string, references *[]*plumbing.Refe func (c *Cache) UnlockGitReferences(repo string, lockId string) error { var input [][2]string var err error - if err = c.cache.GetItem(gitRefsKey(repo), &input); err == nil && + if err = c.cache.GetItem(gitRefsKey(repo), &input, nil); err == nil && len(input) > 0 && len(input[0]) > 1 && input[0][0] == cacheutil.CacheLockedValue && input[0][1] == lockId { // We have the lock, so remove it - return c.cache.SetItem(gitRefsKey(repo), input, 0, true) + return c.cache.SetItem(gitRefsKey(repo), input, &cacheutil.CacheActionOpts{Delete: true, CacheType: cacheutil.CacheTypeExternal}) } return err } @@ -346,7 +331,7 @@ 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, refSourceCommitSHAs ResolvedRevisions) error { - err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res) + err := c.cache.GetItem(manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) if err != nil { return err @@ -389,11 +374,20 @@ 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, refSourceCommitSHAs), res, c.repoCacheExpiration, res == nil) + return c.cache.SetItem( + manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), + res, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: res == nil, + CacheType: cacheutil.CacheTypeExternal}) } 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) + return c.cache.SetItem( + manifestCacheKey(revision, appSrc, srcRefs, namespace, trackingMethod, appLabelKey, appName, clusterInfo, refSourceCommitSHAs), + "", + &cacheutil.CacheActionOpts{Delete: true, CacheType: cacheutil.CacheTypeExternal}) } func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRefs appv1.RefTargetRevisionMapping, trackingMethod appv1.TrackingMethod, refSourceCommitSHAs ResolvedRevisions) string { @@ -404,11 +398,17 @@ func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, srcRef } 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) + return c.cache.GetItem(appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), res, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) } 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) + return c.cache.SetItem( + appDetailsCacheKey(revision, appSrc, srcRefs, trackingMethod, refSourceCommitSHAs), + res, + &cacheutil.CacheActionOpts{ + Expiration: c.repoCacheExpiration, + Delete: res == nil, + CacheType: cacheutil.CacheTypeExternal}) } func revisionMetadataKey(repoURL, revision string) string { @@ -417,11 +417,14 @@ func revisionMetadataKey(repoURL, revision string) string { func (c *Cache) GetRevisionMetadata(repoURL, revision string) (*appv1.RevisionMetadata, error) { item := &appv1.RevisionMetadata{} - return item, c.cache.GetItem(revisionMetadataKey(repoURL, revision), item) + return item, c.cache.GetItem(revisionMetadataKey(repoURL, revision), item, nil) } func (c *Cache) SetRevisionMetadata(repoURL, revision string, item *appv1.RevisionMetadata) error { - return c.cache.SetItem(revisionMetadataKey(repoURL, revision), item, c.repoCacheExpiration, false) + return c.cache.SetItem( + revisionMetadataKey(repoURL, revision), + item, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func revisionChartDetailsKey(repoURL, chart, revision string) string { @@ -430,11 +433,14 @@ func revisionChartDetailsKey(repoURL, chart, revision string) string { func (c *Cache) GetRevisionChartDetails(repoURL, chart, revision string) (*appv1.ChartDetails, error) { item := &appv1.ChartDetails{} - return item, c.cache.GetItem(revisionChartDetailsKey(repoURL, chart, revision), item) + return item, c.cache.GetItem(revisionChartDetailsKey(repoURL, chart, revision), item, nil) } func (c *Cache) SetRevisionChartDetails(repoURL, chart, revision string, item *appv1.ChartDetails) error { - return c.cache.SetItem(revisionChartDetailsKey(repoURL, chart, revision), item, c.repoCacheExpiration, false) + return c.cache.SetItem( + revisionChartDetailsKey(repoURL, chart, revision), + item, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func gitFilesKey(repoURL, revision, pattern string) string { @@ -442,12 +448,15 @@ func gitFilesKey(repoURL, revision, pattern string) string { } func (c *Cache) SetGitFiles(repoURL, revision, pattern string, files map[string][]byte) error { - return c.cache.SetItem(gitFilesKey(repoURL, revision, pattern), &files, c.repoCacheExpiration, false) + return c.cache.SetItem( + gitFilesKey(repoURL, revision, pattern), + &files, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration, CacheType: cacheutil.CacheTypeExternal}) } func (c *Cache) GetGitFiles(repoURL, revision, pattern string) (map[string][]byte, error) { var item map[string][]byte - return item, c.cache.GetItem(gitFilesKey(repoURL, revision, pattern), &item) + return item, c.cache.GetItem(gitFilesKey(repoURL, revision, pattern), &item, &cacheutil.CacheActionOpts{CacheType: cacheutil.CacheTypeExternal}) } func gitDirectoriesKey(repoURL, revision string) string { @@ -455,12 +464,15 @@ func gitDirectoriesKey(repoURL, revision string) string { } func (c *Cache) SetGitDirectories(repoURL, revision string, directories []string) error { - return c.cache.SetItem(gitDirectoriesKey(repoURL, revision), &directories, c.repoCacheExpiration, false) + return c.cache.SetItem( + gitDirectoriesKey(repoURL, revision), + &directories, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) } func (c *Cache) GetGitDirectories(repoURL, revision string) ([]string, error) { var item []string - return item, c.cache.GetItem(gitDirectoriesKey(repoURL, revision), &item) + return item, c.cache.GetItem(gitDirectoriesKey(repoURL, revision), &item, nil) } func (cmr *CachedManifestResponse) shallowCopy() *CachedManifestResponse { diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index d6b9cfb0f5d09..c4101e8f68445 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -8,36 +8,43 @@ import ( "testing" "time" - "github.com/spf13/cobra" - "github.com/stretchr/testify/assert" - . "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/reposerver/apiclient" + "github.com/argoproj/argo-cd/v2/reposerver/cache/mocks" cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/go-git/go-git/v5/plumbing" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) -type fixtures struct { +type MockedCache struct { + mock.Mock *Cache } +type fixtures struct { + mockCache *mocks.MockRepoCache + cache *MockedCache +} + func newFixtures() *fixtures { - return &fixtures{NewCache( - cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Hour)), - &CacheOpts{ - RepoCacheExpiration: 1 * time.Minute, - RevisionCacheExpiration: 1 * time.Minute, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 30 * time.Second, - RevisionCacheLockWaitInterval: 1 * time.Second, - }, - )} + mockCache := mocks.NewMockRepoCache(&mocks.MockCacheOptions{RevisionCacheExpiration: 1 * time.Minute, RepoCacheExpiration: 1 * time.Minute}) + newBaseCache := cacheutil.NewCache(mockCache.TwoLevelClient) + baseCache := NewCache(newBaseCache, 1*time.Minute, 1*time.Minute) + return &fixtures{mockCache: mockCache, cache: &MockedCache{Cache: baseCache}} } func TestCache_GetRevisionMetadata(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss _, err := cache.GetRevisionMetadata("my-repo-url", "my-revision") assert.Equal(t, ErrCacheMiss, err) + mockCache.TwoLevelClient.AssertCalled(t, "Get", mock.Anything) // populate cache err = cache.SetRevisionMetadata("my-repo-url", "my-revision", &RevisionMetadata{Message: "my-message"}) assert.NoError(t, err) @@ -51,10 +58,14 @@ func TestCache_GetRevisionMetadata(t *testing.T) { value, err := cache.GetRevisionMetadata("my-repo-url", "my-revision") assert.NoError(t, err) assert.Equal(t, &RevisionMetadata{Message: "my-message"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 3, InMemoryGets: 1}) } func TestCache_ListApps(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss _, err := cache.ListApps("my-repo-url", "my-revision") assert.Equal(t, ErrCacheMiss, err) @@ -71,10 +82,14 @@ func TestCache_ListApps(t *testing.T) { value, err := cache.ListApps("my-repo-url", "my-revision") assert.NoError(t, err) assert.Equal(t, map[string]string{"foo": "bar"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 4}) } func TestCache_GetManifests(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss q := &apiclient.ManifestRequest{} value := &CachedManifestResponse{} @@ -113,10 +128,14 @@ func TestCache_GetManifests(t *testing.T) { assert.NoError(t, err) assert.Equal(t, &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}, value) }) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 8}) } func TestCache_GetAppDetails(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + mockCache := fixtures.mockCache // cache miss value := &apiclient.RepoAppDetailsResponse{} emptyRefSources := map[string]*RefTarget{} @@ -135,6 +154,7 @@ func TestCache_GetAppDetails(t *testing.T) { err = cache.GetAppDetails("my-revision", &ApplicationSource{}, emptyRefSources, value, "", nil) assert.NoError(t, err) assert.Equal(t, &apiclient.RepoAppDetailsResponse{Type: "my-type"}, value) + mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 4}) } func TestAddCacheFlagsToCmd(t *testing.T) { @@ -149,13 +169,8 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) { repoCache := NewCache( cacheutil.NewCache(inMemCache), - &CacheOpts{ - RepoCacheExpiration: 1 * time.Minute, - RevisionCacheExpiration: 1 * time.Minute, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 10 * time.Second, - RevisionCacheLockWaitInterval: 1 * time.Second, - }, + 1*time.Minute, + 1*time.Minute, ) response := apiclient.ManifestResponse{ @@ -321,22 +336,75 @@ func TestCachedManifestResponse_ShallowCopyExpectedFields(t *testing.T) { } +func TestGetGitReferences(t *testing.T) { + t.Run("Valid args, nothing in cache, in-memory only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeInMemory) + assert.NoError(t, err, "Error is cache miss handled inside function") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1}) + }) + + t.Run("Valid args, nothing in cache, external only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeExternal) + assert.NoError(t, err, "Error is cache miss handled inside function") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + + t.Run("Valid args, value in cache, in-memory only", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeInMemory) + assert.NoError(t, err) + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Equal(t, 1, len(*references)) + assert.Equal(t, "test", (*references)[0].Target().String()) + assert.Equal(t, "test-repo", (*references)[0].Name().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("cache error", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Return(errors.New("test cache error")) + lockOwner, references, err := cache.GetGitReferences("test-repo", "test-lock-id", cacheutil.CacheTypeExternal) + assert.ErrorContains(t, err, "test cache error", "Error should be propagated") + assert.Equal(t, "", lockOwner, "Lock owner should be empty") + assert.Nil(t, references) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1}) + }) + +} + func TestGitRefCacheItemToReferences_DataChecks(t *testing.T) { - references := GitRefCacheItemToReferences(nil) + references := *GitRefCacheItemToReferences(nil) assert.Equal(t, 0, len(references), "No data should be handled gracefully by returning an empty slice") - references = GitRefCacheItemToReferences([][2]string{{"", ""}}) + references = *GitRefCacheItemToReferences([][2]string{{"", ""}}) assert.Equal(t, 0, len(references), "Empty data should be discarded") - references = GitRefCacheItemToReferences([][2]string{{"test", ""}}) + references = *GitRefCacheItemToReferences([][2]string{{"test", ""}}) assert.Equal(t, 1, len(references), "Just the key being set should not be discarded") assert.Equal(t, "test", references[0].Name().String(), "Name should be set and equal test") - references = GitRefCacheItemToReferences([][2]string{{"", "ref: test1"}}) + references = *GitRefCacheItemToReferences([][2]string{{"", "ref: test1"}}) assert.Equal(t, 1, len(references), "Just the value being set should not be discarded") assert.Equal(t, "test1", references[0].Target().String(), "Target should be set and equal test1") - references = GitRefCacheItemToReferences([][2]string{{"test2", "ref: test2"}}) + references = *GitRefCacheItemToReferences([][2]string{{"test2", "ref: test2"}}) assert.Equal(t, 1, len(references), "Valid data is should be preserved") assert.Equal(t, "test2", references[0].Name().String(), "Name should be set and equal test2") assert.Equal(t, "test2", references[0].Target().String(), "Target should be set and equal test2") - references = GitRefCacheItemToReferences([][2]string{{"test3", "ref: test3"}, {"test4", "ref: test4"}}) + references = *GitRefCacheItemToReferences([][2]string{{"test3", "ref: test3"}, {"test4", "ref: test4"}}) assert.Equal(t, 2, len(references), "Valid data is should be preserved") assert.Equal(t, "test3", references[0].Name().String(), "Name should be set and equal test3") assert.Equal(t, "test3", references[0].Target().String(), "Target should be set and equal test3") @@ -345,27 +413,350 @@ func TestGitRefCacheItemToReferences_DataChecks(t *testing.T) { } func TestTryLockGitRefCache_OwnershipFlows(t *testing.T) { - cache := newFixtures().Cache + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + utilCache := cache.cache // Test setting the lock - cache.TryLockGitRefCache("my-repo-url", "my-lock-id") + err := cache.TryLockGitRefCache("my-repo-url", "my-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + assert.NoError(t, err) var output [][2]string key := fmt.Sprintf("git-refs|%s", "my-repo-url") - err := cache.cache.GetItem(key, &output) + err = utilCache.GetItem(key, &output, nil) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 1}) assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should be set") assert.Equal(t, "my-lock-id", output[0][1], "The lock should be set to the provided lock id") // Test not being able to overwrite the lock - cache.TryLockGitRefCache("my-repo-url", "other-lock-id") - err = cache.cache.GetItem(key, &output) + err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1}) + assert.NoError(t, err) + err = utilCache.GetItem(key, &output, nil) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1, InMemoryGets: 1}) assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should not have changed") assert.Equal(t, "my-lock-id", output[0][1], "The lock should not have changed") // Test can overwrite once there is nothing set - err = cache.cache.SetItem(key, [][2]string{}, 0, true) + err = utilCache.SetItem(key, [][2]string{}, &cacheutil.CacheActionOpts{Expiration: 0, Delete: true}) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, ExternalGets: 1, ExternalDeletes: 1, InMemoryGets: 1}) + assert.NoError(t, err) + err = cache.TryLockGitRefCache("my-repo-url", "other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 3, ExternalGets: 1, ExternalDeletes: 1, InMemoryGets: 1}) assert.NoError(t, err) - cache.TryLockGitRefCache("my-repo-url", "other-lock-id") - err = cache.cache.GetItem(key, &output) + err = utilCache.GetItem(key, &output, nil) assert.NoError(t, err) assert.Equal(t, "locked", output[0][0], "The lock should be set") assert.Equal(t, "other-lock-id", output[0][1], "The lock id should have changed to other-lock-id") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 3, ExternalGets: 2, ExternalDeletes: 1, InMemoryGets: 1}) +} + +func TestGetOrLockGitReferences(t *testing.T) { + t.Run("Test cache lock get lock", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, ExternalGets: 1, InMemoryGets: 1}) + }) + + t.Run("Test cache lock, cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + assert.Equal(t, "test-repo", references[0].Name().String()) + assert.Equal(t, "test", references[0].Target().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("Test cache lock, cache hit remote", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + err := fixtures.cache.cache.SetItem( + "git-refs|test-repo", + [][2]string{{"test-repo", "ref: test"}}, + &cacheutil.CacheActionOpts{ + Expiration: 30 * time.Second}) + assert.NoError(t, err) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + assert.Equal(t, "test-repo", references[0].Name().String()) + assert.Equal(t, "test", references[0].Target().String()) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1, InMemoryGets: 1}) + }) + + t.Run("Test miss, populated by external", func(t *testing.T) { + // Tests the case where another process populates the external cache when trying + // to obtain the lock + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Get", mock.Anything).Return(cacheutil.ErrCacheMiss).Once().Run(func(args mock.Arguments) { + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + }).On("Get", mock.Anything).Return(nil) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.False(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 2, InMemoryGets: 2}) + }) + + t.Run("Test cache lock timeout", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + // Create conditions for cache hit, which would result in false on updateCache if we weren't reaching the timeout + err := cache.SetGitReferences("test-repo", *GitRefCacheItemToReferences([][2]string{{"test-repo", "ref: test"}})) + assert.NoError(t, err) + cache.revisionCacheLockTimeout = -1 * time.Second + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + cache.revisionCacheLockTimeout = 10 * time.Second + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + }) + + t.Run("Test cache lock error", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + fixtures.cache.revisionCacheLockTimeout = 10 * time.Second + fixtures.mockCache.TwoLevelClient.On("Set", mock.Anything).Unset() + fixtures.mockCache.TwoLevelClient.On("Set", mock.Anything).Return(errors.New("test cache error")).Once(). + On("Set", mock.Anything).Return(nil) + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + fixtures.mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Set", 2) + fixtures.mockCache.RedisClient.AssertNumberOfCalls(t, "Set", 1) + fixtures.mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Get", 4) + fixtures.mockCache.RedisClient.AssertNumberOfCalls(t, "Get", 2) + }) +} + +func TestUnlockGitReferences(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + + t.Run("Test not locked", func(t *testing.T) { + err := cache.UnlockGitReferences("test-repo", "") + assert.Error(t, err) + assert.Contains(t, err.Error(), "key is missing") + }) + + t.Run("Test unlock", func(t *testing.T) { + // Get lock + var references []*plumbing.Reference + updateCache, lockId, err := cache.GetOrLockGitReferences("test-repo", &references) + assert.NoError(t, err) + assert.True(t, updateCache) + assert.NotEqual(t, "", lockId, "Lock id should be set") + // Release lock + err = cache.UnlockGitReferences("test-repo", lockId) + assert.NoError(t, err) + }) +} + +func TestSetHelmIndex(t *testing.T) { + t.Run("SetHelmIndex with valid data", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + err := fixtures.cache.SetHelmIndex("test-repo", []byte("test-data")) + assert.NoError(t, err) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalSets: 1}) + }) + t.Run("SetHelmIndex with nil", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + err := fixtures.cache.SetHelmIndex("test-repo", nil) + assert.Error(t, err, "nil data should not be cached") + var indexData []byte + err = fixtures.cache.GetHelmIndex("test-repo", &indexData) + assert.Error(t, err) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) +} + +func TestRevisionChartDetails(t *testing.T) { + t.Run("GetRevisionChartDetails cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, &appv1.ChartDetails{}, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetRevisionChartDetails cache miss local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := cache.cache.SetItem( + revisionChartDetailsKey("test-repo", "test-revision", "v1.0.0"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("GetRevisionChartDetails cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := cache.cache.SetItem( + revisionChartDetailsKey("test-repo", "test-revision", "v1.0.0"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeInMemory}) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, InMemorySets: 1}) + }) + + t.Run("SetRevisionChartDetails", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := &appv1.ChartDetails{ + Description: "test-chart", + Home: "v1.0.0", + Maintainers: []string{"test-maintainer"}, + } + err := fixtures.cache.SetRevisionChartDetails("test-repo", "test-revision", "v1.0.0", expectedItem) + assert.NoError(t, err) + details, err := fixtures.cache.GetRevisionChartDetails("test-repo", "test-revision", "v1.0.0") + assert.NoError(t, err) + assert.Equal(t, expectedItem, details) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, ExternalSets: 1}) + }) + +} + +func TestGetGitDirectories(t *testing.T) { + t.Run("GetGitDirectories cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, 0, len(directories)) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetGitDirectories cache miss local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := []string{"test/dir", "test/dir2"} + err := cache.cache.SetItem( + gitDirectoriesKey("test-repo", "test-revision"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("GetGitDirectories cache hit local", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := []string{"test/dir", "test/dir2"} + err := cache.cache.SetItem( + gitDirectoriesKey("test-repo", "test-revision"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeInMemory}) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, InMemorySets: 1}) + }) + + t.Run("SetGitDirectories", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := []string{"test/dir", "test/dir2"} + err := fixtures.cache.SetGitDirectories("test-repo", "test-revision", expectedItem) + assert.NoError(t, err) + directories, err := fixtures.cache.GetGitDirectories("test-repo", "test-revision") + assert.NoError(t, err) + assert.Equal(t, expectedItem, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{InMemoryGets: 1, ExternalSets: 1}) + }) + +} + +func TestGetGitFiles(t *testing.T) { + t.Run("GetGitFiles cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + directories, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.ErrorAs(t, err, &ErrCacheMiss) + assert.Equal(t, 0, len(directories)) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetGitFiles cache hit", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := map[string][]byte{"test/file.json": []byte("\"test\":\"contents\""), "test/file1.json": []byte("\"test1\":\"contents1\"")} + err := cache.cache.SetItem( + gitFilesKey("test-repo", "test-revision", "*.json"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second, CacheType: cacheutil.CacheTypeExternal}) + assert.NoError(t, err) + files, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("SetGitFiles", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := map[string][]byte{"test/file.json": []byte("\"test\":\"contents\""), "test/file1.json": []byte("\"test1\":\"contents1\"")} + err := fixtures.cache.SetGitFiles("test-repo", "test-revision", "*.json", expectedItem) + assert.NoError(t, err) + files, err := fixtures.cache.GetGitFiles("test-repo", "test-revision", "*.json") + assert.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + } diff --git a/reposerver/cache/mocks/reposervercache.go b/reposerver/cache/mocks/reposervercache.go new file mode 100644 index 0000000000000..4cd237dfa91e7 --- /dev/null +++ b/reposerver/cache/mocks/reposervercache.go @@ -0,0 +1,88 @@ +package mocks + +import ( + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" + cacheutilmocks "github.com/argoproj/argo-cd/v2/util/cache/mocks" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/mock" +) + +type MockCacheType int + +const ( + MockCacheTypeRedis MockCacheType = iota + MockCacheTypeInMem +) + +type MockRepoCache struct { + mock.Mock + RedisClient *cacheutilmocks.MockCacheClient + TwoLevelClient *cacheutilmocks.MockCacheClient + StopRedisCallback func() +} + +type MockCacheOptions struct { + RepoCacheExpiration time.Duration + RevisionCacheExpiration time.Duration + ReadDelay time.Duration + WriteDelay time.Duration +} + +type CacheCallCounts struct { + ExternalSets int + ExternalGets int + ExternalDeletes int + InMemorySets int + InMemoryGets int + InMemoryDeletes int +} + +// Checks that the cache was called the expected number of times +func (mockCache *MockRepoCache) AssertCacheCalledTimes(t *testing.T, calls *CacheCallCounts) { + totalSets := calls.ExternalSets + calls.InMemorySets + totalGets := calls.ExternalGets + calls.InMemoryGets + totalDeletes := calls.ExternalDeletes + calls.InMemoryDeletes + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Get", totalGets) + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Set", totalSets) + mockCache.TwoLevelClient.AssertNumberOfCalls(t, "Delete", totalDeletes) + mockCache.RedisClient.AssertNumberOfCalls(t, "Get", calls.ExternalGets) + mockCache.RedisClient.AssertNumberOfCalls(t, "Set", calls.ExternalSets) + mockCache.RedisClient.AssertNumberOfCalls(t, "Delete", calls.ExternalDeletes) +} + +func (mockCache *MockRepoCache) ConfigureDefaultCallbacks() { + mockCache.TwoLevelClient.On("Get", mock.Anything).Return(nil) + mockCache.TwoLevelClient.On("Set", mock.Anything).Return(nil) + mockCache.TwoLevelClient.On("Delete", mock.Anything).Return(nil) + mockCache.RedisClient.On("Get", mock.Anything).Return(nil) + mockCache.RedisClient.On("Set", mock.Anything).Return(nil) + mockCache.RedisClient.On("Delete", mock.Anything).Return(nil) +} + +func NewInMemoryRedis() (*redis.Client, func()) { + cacheutil.NewInMemoryCache(5 * time.Second) + mr, err := miniredis.Run() + if err != nil { + panic(err) + } + return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close +} + +func NewMockRepoCache(cacheOpts *MockCacheOptions) *MockRepoCache { + redisClient, stopRedis := NewInMemoryRedis() + redisCacheClient := &cacheutilmocks.MockCacheClient{ + ReadDelay: cacheOpts.ReadDelay, + WriteDelay: cacheOpts.WriteDelay, + BaseCache: cacheutil.NewRedisCache(redisClient, cacheOpts.RepoCacheExpiration, cacheutil.RedisCompressionNone)} + twoLevelClient := &cacheutilmocks.MockCacheClient{ + ReadDelay: cacheOpts.ReadDelay, + WriteDelay: cacheOpts.WriteDelay, + BaseCache: cacheutil.NewTwoLevelClient(redisCacheClient, cacheOpts.RepoCacheExpiration)} + newMockCache := &MockRepoCache{TwoLevelClient: twoLevelClient, RedisClient: redisCacheClient, StopRedisCallback: stopRedis} + newMockCache.ConfigureDefaultCallbacks() + return newMockCache +} diff --git a/reposerver/repository/mocks/RepositoryCache.go b/reposerver/repository/mocks/RepositoryCache.go deleted file mode 100644 index 37511765715d5..0000000000000 --- a/reposerver/repository/mocks/RepositoryCache.go +++ /dev/null @@ -1,157 +0,0 @@ -package mocks - -import ( - "context" - "sync" - "time" - - "github.com/alicebob/miniredis/v2" - reposerverCache "github.com/argoproj/argo-cd/v2/reposerver/cache" - cacheutil "github.com/argoproj/argo-cd/v2/util/cache" - "github.com/go-git/go-git/v5/plumbing" - "github.com/redis/go-redis/v9" -) - -type MockCacheType int - -const ( - MockCacheTypeRedis MockCacheType = iota - MockCacheTypeInMem -) - -type MockRepoCache struct { - cache *cacheutil.Cache - baseCache *reposerverCache.Cache - reposerverCache.CacheOpts - CallCounts map[string]int - ErrorResponses map[string]error - StopRedisCallback func() - CallCountsMutex *sync.Mutex -} - -type MockCacheOptions struct { - reposerverCache.CacheOpts - ReadDelay time.Duration - WriteDelay time.Duration - // Map of function name keys to error values that should be returned by the function - ErrorResponses map[string]error -} - -func NewInMemoryRedis() (*redis.Client, func()) { - cacheutil.NewInMemoryCache(5 * time.Second) - mr, err := miniredis.Run() - if err != nil { - panic(err) - } - return redis.NewClient(&redis.Options{Addr: mr.Addr()}), mr.Close -} - -func NewMockRepoCache(cacheType MockCacheType, cacheOpts *MockCacheOptions) *MockRepoCache { - redisClient, stopRedis := NewInMemoryRedis() - var cacheClient cacheutil.CacheClient - switch cacheType { - case MockCacheTypeRedis: - cacheClient = cacheutil.NewRedisCache(redisClient, 5*time.Second, cacheutil.RedisCompressionNone) - default: - cacheClient = cacheutil.NewInMemoryCache(5 * time.Second) - } - utilCache := cacheutil.NewCache(&FakeCache{baseCache: cacheClient, ReadDelay: cacheOpts.ReadDelay, WriteDelay: cacheOpts.WriteDelay}) - if cacheOpts.ErrorResponses == nil { - cacheOpts.ErrorResponses = make(map[string]error) - } - baseCacheOptions := reposerverCache.CacheOpts{ - RepoCacheExpiration: cacheOpts.RepoCacheExpiration, - RevisionCacheExpiration: cacheOpts.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: cacheOpts.RevisionCacheLockWaitEnabled, - RevisionCacheLockTimeout: cacheOpts.RevisionCacheLockTimeout, - RevisionCacheLockWaitInterval: cacheOpts.RevisionCacheLockWaitInterval, - } - return &MockRepoCache{ - cache: utilCache, - baseCache: reposerverCache.NewCache(utilCache, &baseCacheOptions), - CacheOpts: baseCacheOptions, - CallCounts: make(map[string]int), - CallCountsMutex: &sync.Mutex{}, - ErrorResponses: cacheOpts.ErrorResponses, - StopRedisCallback: stopRedis, - } -} - -func (c *MockRepoCache) GetGitCache() *cacheutil.Cache { - return c.cache -} - -func (c *MockRepoCache) SetGitReferences(repo string, references []*plumbing.Reference) error { - if err := c.ErrorResponses["SetGitReferences"]; err != nil { - return err - } - c.CallCountsMutex.Lock() - c.CallCounts["SetGitReferences"]++ - c.CallCountsMutex.Unlock() - return c.baseCache.SetGitReferences(repo, references) -} - -func (c *MockRepoCache) GetGitReferences(repo string, references *[]*plumbing.Reference) error { - if err := c.ErrorResponses["GetGitReferences"]; err != nil { - return err - } - c.CallCountsMutex.Lock() - c.CallCounts["GetGitReferences"]++ - c.CallCountsMutex.Unlock() - return c.baseCache.GetGitReferences(repo, references) -} - -func (c *MockRepoCache) UnlockGitReferences(repo string, lockId string) error { - if err := c.ErrorResponses["UnlockGitReferences"]; err != nil { - return err - } - c.CallCountsMutex.Lock() - c.CallCounts["UnlockGitReferences"]++ - c.CallCountsMutex.Unlock() - return c.baseCache.UnlockGitReferences(repo, lockId) -} - -func (c *MockRepoCache) GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) { - if err := c.ErrorResponses["GetOrLockGitReferences"]; err != nil { - return false, "", err - } - c.CallCountsMutex.Lock() - c.CallCounts["GetOrLockGitReferences"]++ - c.CallCountsMutex.Unlock() - return c.baseCache.GetOrLockGitReferences(repo, references) -} - -type FakeCache struct { - baseCache cacheutil.CacheClient - ReadDelay time.Duration - WriteDelay time.Duration -} - -func (c *FakeCache) Set(item *cacheutil.Item) error { - if c.WriteDelay > 0 { - time.Sleep(c.WriteDelay) - } - return c.baseCache.Set(item) -} - -func (c *FakeCache) Get(key string, obj interface{}) error { - if c.ReadDelay > 0 { - time.Sleep(c.ReadDelay) - } - return c.baseCache.Get(key, obj) -} - -func (c *FakeCache) Delete(key string) error { - if c.WriteDelay > 0 { - time.Sleep(c.WriteDelay) - } - return c.baseCache.Delete(key) -} - -func (c *FakeCache) OnUpdated(ctx context.Context, key string, callback func() error) error { - return c.baseCache.OnUpdated(ctx, key, callback) -} - -func (c *FakeCache) NotifyUpdated(key string) error { - return c.baseCache.NotifyUpdated(key) -} diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a5e1b155cf86d..b2af471b30e6f 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -18,6 +18,7 @@ import ( "testing" "time" + cacheutil "github.com/argoproj/argo-cd/v2/util/cache" log "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/api/resource" @@ -30,14 +31,14 @@ import ( "sigs.k8s.io/yaml" "github.com/argoproj/argo-cd/v2/common" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/reposerver/apiclient" "github.com/argoproj/argo-cd/v2/reposerver/cache" + repositorymocks "github.com/argoproj/argo-cd/v2/reposerver/cache/mocks" "github.com/argoproj/argo-cd/v2/reposerver/metrics" - repositorymocks "github.com/argoproj/argo-cd/v2/reposerver/repository/mocks" fileutil "github.com/argoproj/argo-cd/v2/test/fixture/path" "github.com/argoproj/argo-cd/v2/util/argo" - cacheutil "github.com/argoproj/argo-cd/v2/util/cache" dbmocks "github.com/argoproj/argo-cd/v2/util/db/mocks" "github.com/argoproj/argo-cd/v2/util/git" gitmocks "github.com/argoproj/argo-cd/v2/util/git/mocks" @@ -54,7 +55,29 @@ gpg: Good signature from "GitHub (web-flow commit signing) " type clientFunc func(*gitmocks.Client, *helmmocks.Client, *iomocks.TempPaths) -func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client) { +type repoCacheMocks struct { + mock.Mock + cacheutilCache *cacheutil.Cache + cache *cache.Cache + mockCache *repositorymocks.MockRepoCache +} + +func newCacheMocks() *repoCacheMocks { + mockRepoCache := repositorymocks.NewMockRepoCache(&repositorymocks.MockCacheOptions{ + RepoCacheExpiration: 1 * time.Minute, + RevisionCacheExpiration: 1 * time.Minute, + ReadDelay: 0, + WriteDelay: 0, + }) + cacheutilCache := cacheutil.NewCache(mockRepoCache.TwoLevelClient) + return &repoCacheMocks{ + cacheutilCache: cacheutilCache, + cache: cache.NewCache(cacheutilCache, 1*time.Minute, 1*time.Minute), + mockCache: mockRepoCache, + } +} + +func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client, *repoCacheMocks) { root, err := filepath.Abs(root) if err != nil { panic(err) @@ -92,21 +115,13 @@ func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client) }, root) } -func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client) { +func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client, *repoCacheMocks) { helmClient := &helmmocks.Client{} gitClient := &gitmocks.Client{} paths := &iomocks.TempPaths{} cf(gitClient, helmClient, paths) - service := NewService(metrics.NewMetricsServer(), cache.NewCache( - cacheutil.NewCache(cacheutil.NewInMemoryCache(1*time.Minute)), - &cache.CacheOpts{ - RepoCacheExpiration: 1 * time.Minute, - RevisionCacheExpiration: 1 * time.Minute, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 10 * time.Second, - RevisionCacheLockWaitInterval: 1 * time.Second, - }, - ), RepoServerInitConstants{ParallelismLimit: 1}, argo.NewResourceTracking(), &git.NoopCredsStore{}, root) + cacheMocks := newCacheMocks() + service := NewService(metrics.NewMetricsServer(), cacheMocks.cache, RepoServerInitConstants{ParallelismLimit: 1}, argo.NewResourceTracking(), &git.NoopCredsStore{}, root) service.newGitClient = func(rawRepoURL string, root string, creds git.Creds, insecure bool, enableLfs bool, prosy string, opts ...git.ClientOpts) (client git.Client, e error) { return gitClient, nil @@ -118,16 +133,16 @@ func newServiceWithOpt(cf clientFunc, root string) (*Service, *gitmocks.Client) return io.NopCloser } service.gitRepoPaths = paths - return service, gitClient + return service, gitClient, cacheMocks } func newService(root string) *Service { - service, _ := newServiceWithMocks(root, false) + service, _, _ := newServiceWithMocks(root, false) return service } func newServiceWithSignature(root string) *Service { - service, _ := newServiceWithMocks(root, true) + service, _, _ := newServiceWithMocks(root, true) return service } @@ -139,7 +154,7 @@ func newServiceWithCommitSHA(root, revision string) *Service { revisionErr = errors.New("not a commit SHA") } - service, gitClient := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + service, gitClient, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) @@ -384,7 +399,7 @@ func TestHelmChartReferencingExternalValues_OutOfBounds_Symlink(t *testing.T) { } func TestGenerateManifestsUseExactRevision(t *testing.T) { - service, gitClient := newServiceWithMocks(".", false) + service, gitClient, _ := newServiceWithMocks(".", false) src := argoappv1.ApplicationSource{Path: "./testdata/recurse", Directory: &argoappv1.ApplicationSourceDirectory{Recurse: true}} @@ -1409,7 +1424,7 @@ func TestGetHelmCharts(t *testing.T) { } func TestGetRevisionMetadata(t *testing.T) { - service, gitClient := newServiceWithMocks("../..", false) + service, gitClient, _ := newServiceWithMocks("../..", false) now := time.Now() gitClient.On("RevisionMetadata", mock.Anything).Return(&git.RevisionMetadata{ @@ -1768,6 +1783,27 @@ func TestGenerateManifestsWithAppParameterFile(t *testing.T) { }) }) + t.Run("Multi-source with source as ref only does not generate manifests", func(t *testing.T) { + service := newService(".") + runWithTempTestdata(t, "single-app-only", func(t *testing.T, path string) { + manifests, err := service.GenerateManifest(context.Background(), &apiclient.ManifestRequest{ + Repo: &argoappv1.Repository{}, + ApplicationSource: &argoappv1.ApplicationSource{ + Path: "", + Chart: "", + Ref: "test", + }, + AppName: "testapp-multi-ref-only", + ProjectName: "something", + ProjectSourceRepos: []string{"*"}, + HasMultipleSources: true, + }) + assert.NoError(t, err) + assert.Empty(t, manifests.Manifests) + assert.NotEmpty(t, manifests.Revision) + }) + }) + t.Run("Application specific override for other app", func(t *testing.T) { service := newService(".") runWithTempTestdata(t, "single-app-only", func(t *testing.T, path string) { @@ -2957,7 +2993,7 @@ func TestErrorGetGitDirectories(t *testing.T) { }, }, want: nil, wantErr: assert.Error}, {name: "InvalidResolveRevision", fields: fields{service: func() *Service { - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", mock.Anything).Return("", fmt.Errorf("ah error")) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -2988,7 +3024,7 @@ func TestErrorGetGitDirectories(t *testing.T) { func TestGetGitDirectories(t *testing.T) { // test not using the cache root := "./testdata/git-files-dirs" - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, cacheMocks := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Once().Return(nil) @@ -3011,6 +3047,11 @@ func TestGetGitDirectories(t *testing.T) { directories, err = s.GetGitDirectories(context.TODO(), dirRequest) assert.Nil(t, err) assert.ElementsMatch(t, []string{"app", "app/bar", "app/foo/bar", "somedir", "app/foo"}, directories.GetPaths()) + cacheMocks.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ + ExternalSets: 1, + ExternalGets: 1, + InMemoryGets: 1, + }) } func TestErrorGetGitFiles(t *testing.T) { @@ -3037,7 +3078,7 @@ func TestErrorGetGitFiles(t *testing.T) { }, }, want: nil, wantErr: assert.Error}, {name: "InvalidResolveRevision", fields: fields{service: func() *Service { - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", mock.Anything).Return("", fmt.Errorf("ah error")) paths.On("GetPath", mock.Anything).Return(".", nil) @@ -3070,7 +3111,7 @@ func TestGetGitFiles(t *testing.T) { files := []string{"./testdata/git-files-dirs/somedir/config.yaml", "./testdata/git-files-dirs/config.yaml", "./testdata/git-files-dirs/config.yaml", "./testdata/git-files-dirs/app/foo/bar/config.yaml"} root := "" - s, _ := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { + s, _, cacheMocks := newServiceWithOpt(func(gitClient *gitmocks.Client, helmClient *helmmocks.Client, paths *iomocks.TempPaths) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) gitClient.On("Checkout", mock.Anything, mock.Anything).Once().Return(nil) @@ -3103,6 +3144,10 @@ func TestGetGitFiles(t *testing.T) { fileResponse, err = s.GetGitFiles(context.TODO(), filesRequest) assert.Nil(t, err) assert.Equal(t, expected, fileResponse.GetMap()) + cacheMocks.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ + ExternalSets: 1, + ExternalGets: 2, + }) } func Test_getRepoSanitizerRegex(t *testing.T) { @@ -3118,221 +3163,130 @@ func TestGetRefs_CacheDisabled(t *testing.T) { dir := t.TempDir() err := initGitRepo(dir, "", false, true) assert.NoError(t, err, "Could not init git repo") - // Test in-memory and redis - cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} - for _, cacheType := range cacheTypes { - repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ - CacheOpts: cache.CacheOpts{ - RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, - RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 30 * time.Second, - RevisionCacheLockWaitInterval: 100 * time.Millisecond, - }, - ReadDelay: 0, - WriteDelay: 0, - }) - t.Cleanup(repoCache.StopRedisCallback) - client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, false)) - require.NoError(t, err) - refs, err := client.LsRefs() - assert.NoError(t, err) - assert.NotNil(t, refs) - assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") - assert.NotEmpty(t, refs.Branches[0]) - // Unlock should not have been called - assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") - assert.Zero(t, - repoCache.CallCounts["GetOrLockGitReferences"], - "GetOrLockGitReferences should not be called when cache is disabled but was called %d times", - repoCache.CallCounts["GetOrLockGitReferences"]) - } + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, false)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + // Unlock should not have been called + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) } -func TestGetRefs_CacheWithLockDisabled(t *testing.T) { - // Test that when the lock is disabled the default behavior still works correctly - // Also shows the current issue with the git requests due to cache misses - dir := t.TempDir() - err := initGitRepo(dir, "", false, true) - assert.NoError(t, err, "Could not init git repo") - // Test in-memory and redis - cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} - for _, cacheType := range cacheTypes { - repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ - CacheOpts: cache.CacheOpts{ - RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, - RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: false, - RevisionCacheLockTimeout: 1 * time.Second, - RevisionCacheLockWaitInterval: 100 * time.Millisecond, - }, - ReadDelay: 0, - WriteDelay: 0, - }) - t.Cleanup(repoCache.StopRedisCallback) - var wg sync.WaitGroup - numberOfCallers := 10 - for i := 0; i < numberOfCallers; i++ { - wg.Add(1) - go func() { - defer wg.Done() - client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) - require.NoError(t, err) - refs, err := client.LsRefs() - assert.NoError(t, err) - assert.NotNil(t, refs) - assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") - assert.NotEmpty(t, refs.Branches[0]) - }() - } - wg.Wait() - // Unlock should not have been called - assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") - // This would normally be all 10 due to them being so close together, - // but since it's timing related, to prevent flakiness we only check that it is greater than half the number of callers - assert.Greater(t, - repoCache.CallCounts["SetGitReferences"], - numberOfCallers/2, - "SetGitReferences have been called more than %d times but was called %d times", - numberOfCallers/2, - repoCache.CallCounts["SetGitReferences"]) - } -} - -func TestGetRefs_CacheWithLockEnabled(t *testing.T) { - // Test that when the lock is enabled there is only one call to SetGitReferences for the same repo which is done after the ls-remote +func TestGetRefs_CacheWithLock(t *testing.T) { + // Test that there is only one call to SetGitReferences for the same repo which is done after the ls-remote dir := t.TempDir() err := initGitRepo(dir, "", false, true) assert.NoError(t, err, "Could not init git repo") - // Test in-memory and redis - cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} - for _, cacheType := range cacheTypes { - repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ - CacheOpts: cache.CacheOpts{ - RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, - RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 1 * time.Second, - RevisionCacheLockWaitInterval: 100 * time.Millisecond, - }, - ReadDelay: 0, - WriteDelay: 0, - }) - t.Cleanup(repoCache.StopRedisCallback) - var wg sync.WaitGroup - numberOfCallers := 10 - for i := 0; i < numberOfCallers; i++ { - wg.Add(1) - go func() { - defer wg.Done() - client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) - require.NoError(t, err) - refs, err := client.LsRefs() - assert.NoError(t, err) - assert.NotNil(t, refs) - assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") - assert.NotEmpty(t, refs.Branches[0]) - }() - } - wg.Wait() - // Unlock should not have been called - assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") - assert.Equal(t, - repoCache.CallCounts["SetGitReferences"], - 1, - "SetGitReferences should have been called only once but was called %d times", - repoCache.CallCounts["SetGitReferences"]) + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + var wg sync.WaitGroup + numberOfCallers := 10 + for i := 0; i < numberOfCallers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + }() } + wg.Wait() + // Unlock should not have been called + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) } -func TestGetRefs_CacheLockTimeout(t *testing.T) { - // Test that the lock timeout is respected +func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { + // Worst case the ttl on the lock expires and the lock is removed + // however if the holder of the lock fails to update the cache the caller should remove the lock + // to allow other callers to attempt to update the cache as quickly as possible dir := t.TempDir() err := initGitRepo(dir, "", false, true) assert.NoError(t, err, "Could not init git repo") - // Test in-memory and redis - cacheTypes := []repositorymocks.MockCacheType{ - repositorymocks.MockCacheTypeInMem, - repositorymocks.MockCacheTypeRedis} - for _, cacheType := range cacheTypes { - repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ - CacheOpts: cache.CacheOpts{ - RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, - RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 100 * time.Millisecond, - RevisionCacheLockWaitInterval: 10 * time.Millisecond, - }, - ReadDelay: 0, - // Add a relatively high write delay so that all routines will reach their timeout without obtaining the lock - WriteDelay: 1 * time.Second, - }) - t.Cleanup(repoCache.StopRedisCallback) - var wg sync.WaitGroup - numberOfCallers := 3 - for i := 0; i < numberOfCallers; i++ { - wg.Add(1) - go func() { - defer wg.Done() - client, err := git.NewClient(fmt.Sprintf("file://%s", dir), git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) - require.NoError(t, err) - refs, err := client.LsRefs() - assert.NoError(t, err) - assert.NotNil(t, refs) - assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") - assert.NotEmpty(t, refs.Branches[0]) - }() - } - wg.Wait() - // Unlock should not have been called - assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 0, "UnlockGitReferences should not have been called") - assert.Equal(t, - repoCache.CallCounts["SetGitReferences"], - numberOfCallers, - "SetGitReferences should have been called %d times but was called %d times", - numberOfCallers, - repoCache.CallCounts["SetGitReferences"]) - } -} - -func TestGetRefs_CacheUnlockedOnUpdateFailed(t *testing.T) { + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + repoUrl := fmt.Sprintf("file://%s", dir) + client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) + assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") + assert.NotEmpty(t, refs.Branches[0]) + var output [][2]string + err = cacheMocks.cacheutilCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output, nil) + assert.Error(t, err, "Should be a cache miss") + assert.Equal(t, 0, len(output), "Expected cache to be empty for key") + cacheMocks.mockCache.AssertNumberOfCalls(t, "UnlockGitReferences", 0) + cacheMocks.mockCache.AssertNumberOfCalls(t, "GetOrLockGitReferences", 0) +} + +func TestGetRefs_CacheLockTryLockGitRefCacheError(t *testing.T) { // Worst case the ttl on the lock expires and the lock is removed // however if the holder of the lock fails to update the cache the caller should remove the lock // to allow other callers to attempt to update the cache as quickly as possible dir := t.TempDir() err := initGitRepo(dir, "", false, true) assert.NoError(t, err, "Could not init git repo") - // Test in-memory and redis - cacheTypes := []repositorymocks.MockCacheType{repositorymocks.MockCacheTypeInMem, repositorymocks.MockCacheTypeRedis} - for _, cacheType := range cacheTypes { - repoCache := repositorymocks.NewMockRepoCache(cacheType, &repositorymocks.MockCacheOptions{ - CacheOpts: cache.CacheOpts{ - RepoCacheExpiration: cache.CacheOptsDefaults.RepoCacheExpiration, - RevisionCacheExpiration: cache.CacheOptsDefaults.RevisionCacheExpiration, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 30 * time.Second, - RevisionCacheLockWaitInterval: 100 * time.Millisecond, + cacheMocks := newCacheMocks() + t.Cleanup(cacheMocks.mockCache.StopRedisCallback) + repoUrl := fmt.Sprintf("file://%s", dir) + // buf := bytes.Buffer{} + // log.SetOutput(&buf) + client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(cacheMocks.cache, true)) + require.NoError(t, err) + refs, err := client.LsRefs() + assert.NoError(t, err) + assert.NotNil(t, refs) +} + +func TestGetRevisionChartDetails(t *testing.T) { + t.Run("Test revision semvar", func(t *testing.T) { + root := t.TempDir() + service := newService(root) + _, err := service.GetRevisionChartDetails(context.Background(), &apiclient.RepoServerRevisionChartDetailsRequest{ + Repo: &v1alpha1.Repository{ + Repo: fmt.Sprintf("file://%s", root), + Name: "test-repo-name", + Type: "helm", }, - ReadDelay: 0, - WriteDelay: 0, - ErrorResponses: map[string]error{ - "SetGitReferences": fmt.Errorf("test error updating cache"), + Name: "test-name", + Revision: "test-revision", + }) + assert.ErrorContains(t, err, "invalid revision") + }) + + t.Run("Test GetRevisionChartDetails", func(t *testing.T) { + root := t.TempDir() + service := newService(root) + repoUrl := fmt.Sprintf("file://%s", root) + err := service.cache.SetRevisionChartDetails(repoUrl, "my-chart", "1.1.0", &argoappv1.ChartDetails{ + Description: "test-description", + Home: "test-home", + Maintainers: []string{"test-maintainer"}, + }) + assert.NoError(t, err) + chartDetails, err := service.GetRevisionChartDetails(context.Background(), &apiclient.RepoServerRevisionChartDetailsRequest{ + Repo: &v1alpha1.Repository{ + Repo: fmt.Sprintf("file://%s", root), + Name: "test-repo-name", + Type: "helm", }, + Name: "my-chart", + Revision: "1.1.0", }) - t.Cleanup(repoCache.StopRedisCallback) - repoUrl := fmt.Sprintf("file://%s", dir) - client, err := git.NewClient(repoUrl, git.NopCreds{}, true, false, "", git.WithCache(repoCache, true)) - require.NoError(t, err) - refs, err := client.LsRefs() assert.NoError(t, err) - assert.NotNil(t, refs) - assert.NotEqual(t, 0, len(refs.Branches), "Expected branches to be populated") - assert.NotEmpty(t, refs.Branches[0]) - gitCache := repoCache.GetGitCache() - assert.Equal(t, repoCache.CallCounts["UnlockGitReferences"], 1, "UnlockGitReferences should have been called") - var output [][2]string - err = gitCache.GetItem(fmt.Sprintf("git-refs|%s|%s", repoUrl, common.CacheVersion), &output) - assert.Error(t, err, "Should be a cache miss") - assert.Equal(t, 0, len(output), "Expected cache to be empty for key") - } + assert.Equal(t, "test-description", chartDetails.Description) + assert.Equal(t, "test-home", chartDetails.Home) + assert.Equal(t, []string{"test-maintainer"}, chartDetails.Maintainers) + }) } diff --git a/server/server.go b/server/server.go index 2d007dd984c91..e52416927143b 100644 --- a/server/server.go +++ b/server/server.go @@ -1026,9 +1026,9 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl // Dex reverse proxy and client app and OAuth2 login/callback a.registerDexHandlers(mux) - // Webhook handler for git events (Note: repo cache timeouts can be configured with the same environment variables as repo-server) + // Webhook handler for git events (Note: cache timeouts are hardcoded because API server does not write to cache and not really using them) argoDB := db.NewDB(a.Namespace, a.settingsMgr, a.KubeClientset) - acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, repocache.NewCache(a.Cache.GetCache(), repocache.CacheOptsDefaults), a.Cache, argoDB) + acdWebhookHandler := webhook.NewHandler(a.Namespace, a.ArgoCDServerOpts.ApplicationNamespaces, a.AppClientset, a.settings, a.settingsMgr, repocache.NewCache(a.Cache.GetCache(), 24*time.Hour, 3*time.Minute), a.Cache, argoDB) mux.HandleFunc("/api/webhook", acdWebhookHandler.Handler) diff --git a/util/cache/appstate/cache.go b/util/cache/appstate/cache.go index d59d31befb12e..3e9b81a0ff9fd 100644 --- a/util/cache/appstate/cache.go +++ b/util/cache/appstate/cache.go @@ -37,7 +37,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) cacheFactory := cacheutil.AddCacheFlagsToCmd(cmd, opts...) return func() (*Cache, error) { - cache, err := cacheFactory() + cache, err := cacheFactory(false) if err != nil { return nil, err } @@ -46,11 +46,11 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) } func (c *Cache) GetItem(key string, item interface{}) error { - return c.Cache.GetItem(key, item) + return c.Cache.GetItem(key, item, nil) } func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { - return c.Cache.SetItem(key, item, expiration, delete) + return c.Cache.SetItem(key, item, &cacheutil.CacheActionOpts{Expiration: expiration, Delete: delete}) } func appManagedResourcesKey(appName string) string { diff --git a/util/cache/cache.go b/util/cache/cache.go index 5e4905818170d..049a1c998a5db 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -31,7 +31,7 @@ const ( ) func NewCache(client CacheClient) *Cache { - return &Cache{client} + return &Cache{client: client} } func buildRedisClient(redisAddress, password, username string, redisDB, maxRetries int, tlsConfig *tls.Config) *redis.Client { @@ -74,7 +74,7 @@ func buildFailoverRedisClient(sentinelMaster, password, username string, redisDB } // AddCacheFlagsToCmd adds flags which control caching to the specified command -func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func() (*Cache, error) { +func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) func(useTwoLevelCache bool) (*Cache, error) { redisAddress := "" sentinelAddresses := make([]string, 0) sentinelMaster := "" @@ -98,7 +98,7 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) cmd.Flags().BoolVar(&insecureRedis, "redis-insecure-skip-tls-verify", false, "Skip Redis server certificate validation.") cmd.Flags().StringVar(&redisCACertificate, "redis-ca-certificate", "", "Path to Redis server CA certificate (e.g. /etc/certs/redis/ca.crt). If not specified, system trusted CAs will be used for server certificate validation.") cmd.Flags().StringVar(&compressionStr, "redis-compress", env.StringFromEnv("REDIS_COMPRESSION", string(RedisCompressionGZip)), "Enable compression for data sent to Redis with the required compression algorithm. (possible values: gzip, none)") - return func() (*Cache, error) { + return func(useTwoLevelCache bool) (*Cache, error) { var tlsConfig *tls.Config = nil if redisUseTLS { tlsConfig = &tls.Config{} @@ -132,22 +132,23 @@ func AddCacheFlagsToCmd(cmd *cobra.Command, opts ...func(client *redis.Client)) if err != nil { return nil, err } + var redisClient *redis.Client if len(sentinelAddresses) > 0 { - client := buildFailoverRedisClient(sentinelMaster, password, username, redisDB, maxRetries, tlsConfig, sentinelAddresses) - for i := range opts { - opts[i](client) + redisClient = buildFailoverRedisClient(sentinelMaster, password, username, redisDB, maxRetries, tlsConfig, sentinelAddresses) + } else { + if redisAddress == "" { + redisAddress = common.DefaultRedisAddr } - return NewCache(NewRedisCache(client, defaultCacheExpiration, compression)), nil + redisClient = buildRedisClient(redisAddress, password, username, redisDB, maxRetries, tlsConfig) } - if redisAddress == "" { - redisAddress = common.DefaultRedisAddr - } - - client := buildRedisClient(redisAddress, password, username, redisDB, maxRetries, tlsConfig) + redisCache := NewRedisCache(redisClient, defaultCacheExpiration, compression) for i := range opts { - opts[i](client) + opts[i](redisClient) } - return NewCache(NewRedisCache(client, defaultCacheExpiration, compression)), nil + if useTwoLevelCache { + return NewCache(NewTwoLevelClient(redisCache, defaultCacheExpiration)), nil + } + return NewCache(redisCache), nil } } @@ -156,6 +157,7 @@ type Cache struct { client CacheClient } +// Returns the cache client with the given type func (c *Cache) GetClient() CacheClient { return c.client } @@ -171,36 +173,33 @@ func (c *Cache) generateFullKey(key string) string { return fmt.Sprintf("%s|%s", key, common.CacheVersion) } -func (c *Cache) SetCacheItem(item *Item, delete bool) error { +// Sets or deletes an item in cache +func (c *Cache) SetItem(key string, item interface{}, opts *CacheActionOpts) error { + if opts == nil { + opts = &CacheActionOpts{} + } if item == nil { return fmt.Errorf("cannot set nil item in cache") } - item.Key = c.generateFullKey(item.Key) - if delete { - return c.client.Delete(item.Key) + fullKey := c.generateFullKey(key) + client := c.GetClient() + if opts.Delete { + return client.Delete(fullKey) } else { - if item.Object == nil { - return fmt.Errorf("cannot set item to nil for key %s", item.Key) - } - return c.client.Set(item) + return client.Set(&Item{Key: fullKey, Object: item, CacheActionOpts: *opts}) } } -func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { - cacheItem := &Item{ - Key: key, - Object: item, - Expiration: expiration, +func (c *Cache) GetItem(key string, item interface{}, opts *CacheActionOpts) error { + if opts == nil { + opts = &CacheActionOpts{} } - return c.SetCacheItem(cacheItem, delete) -} - -func (c *Cache) GetItem(key string, item interface{}) error { key = c.generateFullKey(key) if item == nil { return fmt.Errorf("cannot get item into a nil for key %s", key) } - return c.client.Get(key, item) + client := c.GetClient() + return client.Get(&Item{Key: key, Object: item, CacheActionOpts: *opts}) } func (c *Cache) OnUpdated(ctx context.Context, key string, callback func() error) error { diff --git a/util/cache/cache_test.go b/util/cache/cache_test.go index c9e6174eb9201..aef831d83ad6a 100644 --- a/util/cache/cache_test.go +++ b/util/cache/cache_test.go @@ -13,11 +13,17 @@ import ( ) func TestAddCacheFlagsToCmd(t *testing.T) { - cache, err := AddCacheFlagsToCmd(&cobra.Command{})() + cache, err := AddCacheFlagsToCmd(&cobra.Command{})(false) assert.NoError(t, err) assert.Equal(t, 24*time.Hour, cache.client.(*redisCache).expiration) } +func TestAddCacheFlagsToCmdTwoLevelCache(t *testing.T) { + cache, err := AddCacheFlagsToCmd(&cobra.Command{})(true) + assert.NoError(t, err) + assert.Equal(t, 24*time.Hour, cache.client.(*twoLevelClient).externalCache.(*redisCache).expiration) +} + func NewInMemoryRedis() (*redis.Client, func()) { mr, err := miniredis.Run() if err != nil { @@ -31,56 +37,50 @@ func TestCacheClient(t *testing.T) { defer stopRedis() redisCache := NewRedisCache(clientRedis, 5*time.Second, RedisCompressionNone) clientMemCache := NewInMemoryCache(60 * time.Second) + twoLevelClient := NewTwoLevelClient(redisCache, 5*time.Second) // Run tests for both Redis and InMemoryCache - for _, client := range []CacheClient{clientMemCache, redisCache} { + for _, client := range []CacheClient{clientMemCache, redisCache, twoLevelClient} { cache := NewCache(client) t.Run("SetItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 60*time.Second, false) - assert.NoError(t, err) - }) - t.Run("SetCacheItem", func(t *testing.T) { - err := cache.SetCacheItem(&Item{Key: "foo", Object: "bar", Expiration: 60 * time.Second}, false) + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) assert.NoError(t, err) var output string - err = cache.GetItem("foo", &output) + err = cache.GetItem("foo", &output, nil) assert.NoError(t, err) assert.Equal(t, "bar", output) }) t.Run("SetCacheItem W/Disable Overwrite", func(t *testing.T) { - err := cache.SetCacheItem(&Item{Key: "foo", Object: "bar", Expiration: 60 * time.Second}, false) + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) assert.NoError(t, err) var output string - err = cache.GetItem("foo", &output) + err = cache.GetItem("foo", &output, nil) assert.NoError(t, err) assert.Equal(t, "bar", output) - err = cache.SetCacheItem(&Item{Key: "foo", Object: "baz", Expiration: 60 * time.Second, DisableOverwrite: true}, false) + err = cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 60 * time.Second, DisableOverwrite: true, Delete: false}) assert.NoError(t, err) - err = cache.GetItem("foo", &output) + err = cache.GetItem("foo", &output, nil) assert.NoError(t, err) assert.Equal(t, "bar", output, "output should not have changed with DisableOverwrite set to true") }) t.Run("GetItem", func(t *testing.T) { var val string - err := cache.GetItem("foo", &val) + err := cache.GetItem("foo", &val, nil) assert.NoError(t, err) assert.Equal(t, "bar", val) }) t.Run("DeleteItem", func(t *testing.T) { - err := cache.SetItem("foo", "bar", 0, true) + err := cache.SetItem("foo", "bar", &CacheActionOpts{Expiration: 0, Delete: true}) assert.NoError(t, err) var val string - err = cache.GetItem("foo", &val) + err = cache.GetItem("foo", &val, nil) assert.Error(t, err) assert.Empty(t, val) }) t.Run("Check for nil items", func(t *testing.T) { - err := cache.SetItem("foo", nil, 0, false) - assert.Error(t, err) - assert.Contains(t, err.Error(), "cannot set item") - err = cache.SetCacheItem(nil, false) + err := cache.SetItem("foo", nil, &CacheActionOpts{Expiration: 0, Delete: true}) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot set nil item") - err = cache.GetItem("foo", nil) + err = cache.GetItem("foo", nil, nil) assert.Error(t, err) assert.Contains(t, err.Error(), "cannot get item") }) diff --git a/util/cache/client.go b/util/cache/client.go index a19cc9afb7031..33e188057c1c1 100644 --- a/util/cache/client.go +++ b/util/cache/client.go @@ -10,18 +10,35 @@ var ErrCacheMiss = errors.New("cache: key is missing") var ErrCacheKeyLocked = errors.New("cache: key is locked") var CacheLockedValue = "locked" -type Item struct { - Key string - Object interface{} +type CacheType int + +const ( + CacheTypeDefault CacheType = iota + CacheTypeInMemory + CacheTypeTwoLevel + CacheTypeExternal +) + +type CacheActionOpts struct { + // Delete item from cache + Delete bool // Disable writing if key already exists (NX) DisableOverwrite bool // Expiration is the cache expiration time. Expiration time.Duration + // Determines the cache type to use in case of two level cache, Default == Two Level(in-memory + external) + CacheType CacheType +} + +type Item struct { + Key string + Object interface{} + CacheActionOpts } type CacheClient interface { Set(item *Item) error - Get(key string, obj interface{}) error + Get(item *Item) error Delete(key string) error OnUpdated(ctx context.Context, key string, callback func() error) error NotifyUpdated(key string) error diff --git a/util/cache/client_test.go b/util/cache/client_test.go index 058b30e21ccc0..bba7876089eb2 100644 --- a/util/cache/client_test.go +++ b/util/cache/client_test.go @@ -15,7 +15,7 @@ type testStruct struct { func TestCache(t *testing.T) { c := NewInMemoryCache(time.Hour) var obj testStruct - err := c.Get("key", &obj) + err := c.Get(&Item{Key: "key", Object: &obj}) assert.Equal(t, err, ErrCacheMiss) cacheObj := testStruct{ Foo: "foo", @@ -26,13 +26,13 @@ func TestCache(t *testing.T) { Object: &cacheObj, }) cacheObj.Foo = "baz" - err = c.Get("key", &obj) + err = c.Get(&Item{Key: "key", Object: &obj}) assert.Nil(t, err) assert.EqualValues(t, obj.Foo, "foo") assert.EqualValues(t, string(obj.Bar), "bar") err = c.Delete("key") assert.Nil(t, err) - err = c.Get("key", &obj) + err = c.Get(&Item{Key: "key", Object: &obj}) assert.Equal(t, err, ErrCacheMiss) } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index a07dfd47528a3..77ae9355e5190 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -57,13 +57,13 @@ func (i *InMemoryCache) HasSame(key string, obj interface{}) (bool, error) { return bytes.Equal(buf.Bytes(), existingBuf.Bytes()), nil } -func (i *InMemoryCache) Get(key string, obj interface{}) error { - bufIf, found := i.memCache.Get(key) +func (i *InMemoryCache) Get(item *Item) error { + bufIf, found := i.memCache.Get(item.Key) if !found { return ErrCacheMiss } buf := bufIf.(bytes.Buffer) - return gob.NewDecoder(&buf).Decode(obj) + return gob.NewDecoder(&buf).Decode(item.Object) } func (i *InMemoryCache) Delete(key string) error { diff --git a/util/cache/inmemory_test.go b/util/cache/inmemory_test.go index 4e0d1e48a5a89..3c4ae3d25193d 100644 --- a/util/cache/inmemory_test.go +++ b/util/cache/inmemory_test.go @@ -16,12 +16,12 @@ func TestInMemoryCache(t *testing.T) { // https://stackoverflow.com/questions/46671636/gob-decode-giving-decodevalue-of-unassignable-value-error obj := &foo{} // cache miss - err := cache.Get("my-key", obj) + err := cache.Get(&Item{Key: "my-key", Object: obj}) assert.Equal(t, ErrCacheMiss, err) // cache hit err = cache.Set(&Item{Key: "my-key", Object: &foo{Bar: "bar"}}) assert.NoError(t, err) - err = cache.Get("my-key", obj) + err = cache.Get(&Item{Key: "my-key", Object: obj}) assert.NoError(t, err) assert.Equal(t, &foo{Bar: "bar"}, obj) } diff --git a/util/cache/mocks/cachclient.go b/util/cache/mocks/cachclient.go new file mode 100644 index 0000000000000..0637735572541 --- /dev/null +++ b/util/cache/mocks/cachclient.go @@ -0,0 +1,65 @@ +package mocks + +import ( + "context" + "time" + + cache "github.com/argoproj/argo-cd/v2/util/cache" + "github.com/stretchr/testify/mock" +) + +type MockCacheClient struct { + mock.Mock + BaseCache cache.CacheClient + ReadDelay time.Duration + WriteDelay time.Duration +} + +func (c *MockCacheClient) Set(item *cache.Item) error { + args := c.Called(item) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.BaseCache.Set(item) +} + +func (c *MockCacheClient) Get(item *cache.Item) error { + args := c.Called(item) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.ReadDelay > 0 { + time.Sleep(c.ReadDelay) + } + return c.BaseCache.Get(item) +} + +func (c *MockCacheClient) Delete(key string) error { + args := c.Called(key) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + if c.WriteDelay > 0 { + time.Sleep(c.WriteDelay) + } + return c.BaseCache.Delete(key) +} + +func (c *MockCacheClient) OnUpdated(ctx context.Context, key string, callback func() error) error { + args := c.Called(ctx, key, callback) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + return c.BaseCache.OnUpdated(ctx, key, callback) +} + +func (c *MockCacheClient) NotifyUpdated(key string) error { + args := c.Called(key) + if len(args) > 0 && args.Get(0) != nil { + return args.Get(0).(error) + } + return c.BaseCache.NotifyUpdated(key) +} diff --git a/util/cache/redis.go b/util/cache/redis.go index b6a681398540a..832094203d348 100644 --- a/util/cache/redis.go +++ b/util/cache/redis.go @@ -114,16 +114,16 @@ func (r *redisCache) Set(item *Item) error { }) } -func (r *redisCache) Get(key string, obj interface{}) error { +func (r *redisCache) Get(item *Item) error { var data []byte - err := r.cache.Get(context.TODO(), r.getKey(key), &data) + err := r.cache.Get(context.TODO(), r.getKey(item.Key), &data) if err == rediscache.ErrCacheMiss { err = ErrCacheMiss } if err != nil { return err } - return r.unmarshal(data, obj) + return r.unmarshal(data, item.Object) } func (r *redisCache) Delete(key string) error { diff --git a/util/cache/redis_test.go b/util/cache/redis_test.go index 3800753cee3ec..b0fc602498c0a 100644 --- a/util/cache/redis_test.go +++ b/util/cache/redis_test.go @@ -27,7 +27,7 @@ func TestRedisSetCache(t *testing.T) { t.Run("Successful get", func(t *testing.T) { var res string client := NewRedisCache(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 10*time.Second, RedisCompressionNone) - err = client.Get("foo", &res) + err = client.Get(&Item{Key: "foo", Object: &res}) assert.NoError(t, err) assert.Equal(t, res, "bar") }) @@ -41,7 +41,7 @@ func TestRedisSetCache(t *testing.T) { t.Run("Cache miss", func(t *testing.T) { var res string client := NewRedisCache(redis.NewClient(&redis.Options{Addr: mr.Addr()}), 10*time.Second, RedisCompressionNone) - err = client.Get("foo", &res) + err = client.Get(&Item{Key: "foo", Object: &res}) assert.Error(t, err) assert.Contains(t, err.Error(), "cache: key is missing") }) @@ -66,7 +66,7 @@ func TestRedisSetCacheCompressed(t *testing.T) { assert.True(t, len(compressedData) > len([]byte(testValue)), "compressed data is bigger than uncompressed") var result string - assert.NoError(t, client.Get("my-key", &result)) + assert.NoError(t, client.Get(&Item{Key: "my-key", Object: &result})) assert.Equal(t, testValue, result) } diff --git a/util/cache/twolevelclient.go b/util/cache/twolevelclient.go index 14a4279e87c89..fc972634b09ca 100644 --- a/util/cache/twolevelclient.go +++ b/util/cache/twolevelclient.go @@ -13,6 +13,9 @@ func NewTwoLevelClient(client CacheClient, inMemoryExpiration time.Duration) *tw return &twoLevelClient{inMemoryCache: NewInMemoryCache(inMemoryExpiration), externalCache: client} } +// compile-time validation of adherance of the CacheClient contract +var _ CacheClient = &twoLevelClient{} + type twoLevelClient struct { inMemoryCache *InMemoryCache externalCache CacheClient @@ -21,6 +24,17 @@ type twoLevelClient struct { // Set stores the given value in both in-memory and external cache. // Skip storing the value in external cache if the same value already exists in memory to avoid requesting external cache. func (c *twoLevelClient) Set(item *Item) error { + switch item.CacheType { + case CacheTypeInMemory: + return c.inMemoryCache.Set(item) + case CacheTypeExternal: + return c.externalCache.Set(item) + default: + return c.SetTwoLevel(item) + } +} + +func (c *twoLevelClient) SetTwoLevel(item *Item) error { has, err := c.inMemoryCache.HasSame(item.Key, item.Object) if has { return nil @@ -35,17 +49,28 @@ func (c *twoLevelClient) Set(item *Item) error { return c.externalCache.Set(item) } +func (c *twoLevelClient) Get(item *Item) error { + switch item.CacheType { + case CacheTypeInMemory: + return c.inMemoryCache.Get(item) + case CacheTypeExternal: + return c.externalCache.Get(item) + default: + return c.GetTwoLevel(item) + } +} + // Get returns cache value from in-memory cache if it present. Otherwise loads it from external cache and persists // in memory to avoid future requests to external cache. -func (c *twoLevelClient) Get(key string, obj interface{}) error { - err := c.inMemoryCache.Get(key, obj) +func (c *twoLevelClient) GetTwoLevel(item *Item) error { + err := c.inMemoryCache.Get(item) if err == nil { return nil } - err = c.externalCache.Get(key, obj) + err = c.externalCache.Get(item) if err == nil { - _ = c.inMemoryCache.Set(&Item{Key: key, Object: obj}) + _ = c.inMemoryCache.Set(item) } return err } diff --git a/util/git/client.go b/util/git/client.go index 38bff40b27d29..bba124d79f4f1 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -55,7 +55,6 @@ type Refs struct { type gitRefCache interface { SetGitReferences(repo string, references []*plumbing.Reference) error - GetGitReferences(repo string, references *[]*plumbing.Reference) error GetOrLockGitReferences(repo string, references *[]*plumbing.Reference) (updateCache bool, lockId string, err error) UnlockGitReferences(repo string, lockId string) error } diff --git a/util/webhook/webhook_test.go b/util/webhook/webhook_test.go index 04c1683dfd9f6..b241d7c671841 100644 --- a/util/webhook/webhook_test.go +++ b/util/webhook/webhook_test.go @@ -69,13 +69,8 @@ func NewMockHandler(reactor *reactorDef, applicationNamespaces []string, objects return NewHandler("argocd", applicationNamespaces, appClientset, &settings.ArgoCDSettings{}, &fakeSettingsSrc{}, cache.NewCache( cacheClient, - &cache.CacheOpts{ - RepoCacheExpiration: 1 * time.Minute, - RevisionCacheExpiration: 1 * time.Minute, - RevisionCacheLockWaitEnabled: true, - RevisionCacheLockTimeout: 10 * time.Second, - RevisionCacheLockWaitInterval: 1 * time.Second, - }, + 1*time.Minute, + 1*time.Minute, ), servercache.NewCache(appstate.NewCache(cacheClient, time.Minute), time.Minute, time.Minute, time.Minute), &mocks.ArgoDB{}) }