diff --git a/reposerver/cache/cache.go b/reposerver/cache/cache.go index 193070850c..b9f240ec84 100644 --- a/reposerver/cache/cache.go +++ b/reposerver/cache/cache.go @@ -520,6 +520,23 @@ func (c *Cache) GetGitDirectories(repoURL, revision string) ([]string, error) { return item, err } +func getGitFilesChangesKey(repoURL, revision, targetRevision string) string { + return fmt.Sprintf("gitFilesChanges|%s|%s|%s", repoURL, revision, targetRevision) +} + +func (c *Cache) SetGitFilesChanges(repoURL, revision, targetRevision string, files []string) error { + return c.cache.SetItem( + getGitFilesChangesKey(repoURL, revision, targetRevision), + &files, + &cacheutil.CacheActionOpts{Expiration: c.repoCacheExpiration}) +} + +func (c *Cache) GetGitFilesChanges(repoURL, revision, targetRevision string) ([]string, error) { + var files []string + err := c.cache.GetItem(getGitFilesChangesKey(repoURL, revision, targetRevision), &files) + return files, err +} + func (cmr *CachedManifestResponse) shallowCopy() *CachedManifestResponse { if cmr == nil { return nil diff --git a/reposerver/cache/cache_test.go b/reposerver/cache/cache_test.go index a96ba55652..689e9cf863 100644 --- a/reposerver/cache/cache_test.go +++ b/reposerver/cache/cache_test.go @@ -750,3 +750,41 @@ func TestGetGitFiles(t *testing.T) { fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) }) } + +func TestGetGitFilesChanges(t *testing.T) { + t.Run("GetGitFilesChanges cache miss", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + directories, err := fixtures.cache.GetGitFilesChanges("test-repo", "test-revision", "syncedRevision") + require.ErrorIs(t, err, ErrCacheMiss) + assert.Empty(t, directories) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1}) + }) + t.Run("GetGitFilesChanges cache hit", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + cache := fixtures.cache + expectedItem := []string{"test/path/file-1.yaml", "test/path1/file-2.yaml"} + err := cache.cache.SetItem( + getGitFilesChangesKey("test-repo", "test-revision", "syncedRevision"), + expectedItem, + &cacheutil.CacheActionOpts{Expiration: 30 * time.Second}) + require.NoError(t, err) + files, err := fixtures.cache.GetGitFilesChanges("test-repo", "test-revision", "syncedRevision") + require.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) + + t.Run("SetGitFilesChanges", func(t *testing.T) { + fixtures := newFixtures() + t.Cleanup(fixtures.mockCache.StopRedisCallback) + expectedItem := []string{"test/path/file-1.yaml", "test/path1/file-2.yaml"} + err := fixtures.cache.SetGitFilesChanges("test-repo", "test-revision", "syncedRevision", expectedItem) + require.NoError(t, err) + files, err := fixtures.cache.GetGitFilesChanges("test-repo", "test-revision", "syncedRevision") + require.NoError(t, err) + assert.Equal(t, expectedItem, files) + fixtures.mockCache.AssertCacheCalledTimes(t, &mocks.CacheCallCounts{ExternalGets: 1, ExternalSets: 1}) + }) +} diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index c87af7fe01..67e87e4c4e 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -3061,24 +3061,41 @@ func (s *Service) gitSourceHasChanges(repo *v1alpha1.Repository, revision, synce return revision, syncedRevision, false, nil } - s.metricsServer.IncPendingRepoRequest(repo.Repo) - defer s.metricsServer.DecPendingRepoRequest(repo.Repo) + getGitFilesChanges := func() ([]string, error) { + var files []string - closer, err := s.repoLock.Lock(gitClient.Root(), revision, true, func() (goio.Closer, error) { - return s.checkoutRevision(gitClient, revision, false, 0) - }) - if err != nil { - return revision, syncedRevision, true, status.Errorf(codes.Internal, "unable to checkout git repo %s with revision %s: %v", repo.Repo, revision, err) - } - defer utilio.Close(closer) + s.metricsServer.IncPendingRepoRequest(repo.Repo) + defer s.metricsServer.DecPendingRepoRequest(repo.Repo) - if err := s.fetch(gitClient, []string{syncedRevision}); err != nil { - return revision, syncedRevision, true, status.Errorf(codes.Internal, "unable to fetch git repo %s with syncedRevisions %s: %v", repo.Repo, syncedRevision, err) + closer, err := s.repoLock.Lock(gitClient.Root(), revision, true, func() (goio.Closer, error) { + return s.checkoutRevision(gitClient, revision, false, 0) + }) + if err != nil { + return files, status.Errorf(codes.Internal, "unable to checkout git repo %s with revision %s: %v", repo.Repo, revision, err) + } + defer utilio.Close(closer) + + if err := s.fetch(gitClient, []string{syncedRevision}); err != nil { + return files, status.Errorf(codes.Internal, "unable to fetch git repo %s with syncedRevisions %s: %v", repo.Repo, syncedRevision, err) + } + + files, err = gitClient.ChangedFiles(syncedRevision, revision) + if err != nil { + return files, status.Errorf(codes.Internal, "unable to get changed files for repo %s with revision %s: %v", repo.Repo, revision, err) + } + return files, nil } - files, err := gitClient.ChangedFiles(syncedRevision, revision) + files, err := s.cache.GetGitFilesChanges(repo.Repo, revision, syncedRevision) if err != nil { - return revision, syncedRevision, true, status.Errorf(codes.Internal, "unable to get changed files for repo %s with revision %s: %v", repo.Repo, revision, err) + files, err = getGitFilesChanges() + if err != nil { + return revision, syncedRevision, true, err + } + } + + if err := s.cache.SetGitFilesChanges(repo.Repo, revision, syncedRevision, files); err != nil { + log.Warnf("Failed to store git files changes for `%s` repo in `%s...%s` with: %v", repo.Repo, revision, syncedRevision, err) } changed := false diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index dd53ca383f..8fddf71d18 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -3915,12 +3915,13 @@ func TestUpdateRevisionForPaths(t *testing.T) { previousRevision string } tests := []struct { - name string - fields fields - args args - want *apiclient.UpdateRevisionForPathsResponse - wantErr assert.ErrorAssertionFunc - cacheHit *cacheHit + name string + fields fields + args args + want *apiclient.UpdateRevisionForPathsResponse + wantErr assert.ErrorAssertionFunc + cacheHit *cacheHit + cacheCallCount *repositorymocks.CacheCallCounts }{ {name: "NoPathAbort", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, _ *iomocks.TempPaths) { @@ -3961,7 +3962,11 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, }, want: &apiclient.UpdateRevisionForPathsResponse{ Revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", - }, wantErr: assert.NoError}, + }, wantErr: assert.NoError, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 0, + ExternalGets: 0, + ExternalSets: 0, + }}, {name: "ChangedFilesDoNothing", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { gitClient.EXPECT().Init().Return(nil) @@ -3994,7 +3999,11 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, want: &apiclient.UpdateRevisionForPathsResponse{ Revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", Changes: true, - }, wantErr: assert.NoError}, + }, wantErr: assert.NoError, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 0, + ExternalGets: 1, + ExternalSets: 1, + }}, {name: "NoChangesUpdateCache", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { gitClient.EXPECT().Init().Return(nil) @@ -4036,6 +4045,10 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, wantErr: assert.NoError, cacheHit: &cacheHit{ previousRevision: "1e67a504d03def3a6a1125d934cb511680f72555", revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 1, + ExternalSets: 1, }}, {name: "NoChangesHelmMultiSourceUpdateCache", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { @@ -4078,6 +4091,10 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, wantErr: assert.NoError, cacheHit: &cacheHit{ previousRevision: "1e67a504d03def3a6a1125d934cb511680f72555", revision: "632039659e542ed7de0c170a4fcc1c571b288fc0", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 1, + ExternalSets: 1, }}, {name: "NoChangesHelmWithRefMultiSourceUpdateCache", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { @@ -4131,6 +4148,10 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, wantErr: assert.NoError, cacheHit: &cacheHit{ previousRevision: "0.0.1", revision: "0.0.1", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 2, + ExternalSets: 2, }}, {name: "NoChangesHelmWithRefMultiSource_IgnoreUnusedRef", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { @@ -4182,6 +4203,10 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, wantErr: assert.NoError, cacheHit: &cacheHit{ previousRevision: "0.0.1", revision: "0.0.1", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 1, + ExternalSets: 1, }}, {name: "NoChangesHelmWithRefMultiSource_UndefinedRef", fields: func() fields { s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, _ *ocimocks.Client, paths *iomocks.TempPaths) { @@ -4284,6 +4309,10 @@ func TestUpdateRevisionForPaths(t *testing.T) { }, wantErr: assert.NoError, cacheHit: &cacheHit{ previousRevision: "632039659e542ed7de0c170a4fcc1c571b288fc0", revision: "1e67a504d03def3a6a1125d934cb511680f72555", + }, cacheCallCount: &repositorymocks.CacheCallCounts{ + ExternalRenames: 1, + ExternalGets: 1, + ExternalSets: 1, }}, } for _, tt := range tests { @@ -4301,14 +4330,8 @@ func TestUpdateRevisionForPaths(t *testing.T) { } assert.Equalf(t, tt.want, got, "UpdateRevisionForPaths(%v, %v)", tt.args.ctx, tt.args.request) - if tt.cacheHit != nil { - cache.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ - ExternalRenames: 1, - }) - } else { - cache.mockCache.AssertCacheCalledTimes(t, &repositorymocks.CacheCallCounts{ - ExternalRenames: 0, - }) + if tt.cacheCallCount != nil { + cache.mockCache.AssertCacheCalledTimes(t, tt.cacheCallCount) } }) }