Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(repo-server): excess git requests, resolveReferencedSources and runManifestGenAsync not using cache (Issue #14725) #16410

Merged
merged 2 commits into from
Nov 30, 2023
Merged

fix(repo-server): excess git requests, resolveReferencedSources and runManifestGenAsync not using cache (Issue #14725) #16410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c118ff4
fix(repo-server): excess git requests part 1, resolveReferencedSource…
nromriell Nov 21, 2023
131ce0c
fix: remove unnecessary settings instantiation
nromriell Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions reposerver/cache/mocks/reposervercache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
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
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
}

// Checks that the cache was called the expected number of times
func (mockCache *MockRepoCache) AssertCacheCalledTimes(t *testing.T, calls *CacheCallCounts) {
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.RedisClient.On("Get", mock.Anything, 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)}
newMockCache := &MockRepoCache{RedisClient: redisCacheClient, StopRedisCallback: stopRedis}
newMockCache.ConfigureDefaultCallbacks()
return newMockCache
}
11 changes: 6 additions & 5 deletions reposerver/repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the repository.Service.cache is just Redis based and doesn't leverage the twolevel cache client. Can you please confirm my understanding of this part of the code? If so, it means that we are moving the chatty communication from Git to Redis which can bring new problems.

I don't want to push for a premature optimization as long as we are aware of this possible behaviour and have a plan B if we want to go with this implementation which I am not opposed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, overall redis requests are down after the change due to not having to try and get and set the values for each application each time.

I do however have the optimization for moving this to a two level cache as part of the original PR and in the final part of the PR split nromriell#3

revision = textutils.FirstNonEmpty(revision, source.TargetRevision)
unresolvedRevision := revision
if source.IsHelm() {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -728,7 +729,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, !q.NoRevisionCache && !q.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)
Expand Down
Loading
Loading