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

nromriell · 2023-11-21T08:45:47Z

Partially Fixes #14725

First part of a split of the initial PR. Starts the fixes required to remove the excess git requests when using multi-source and ref sources.

Issue:
When resolveReferencedSources and runManifestGenAsync call newClientResolveRevision they currently do not pass the git options to use the cache when enabled, because of this when newClientResolveRevision creates a new git client and calls LsRemote the cache is always skipped

Changes:
Updated the ls-remote calls from resolveReferencedSources and runManifestGenAsync to have access to the cache

Impact:
On its own this does not fix the issue. This will reduce the number of ls-remote currently being made by preventing multiple calls inside the same goroutine or slightly time offset calls from missing cache, however due to the fact that a large number of calls are made by nearly simultaneous processes when the cache expires this is only a partial fix.

This also does not fix the large number of fetch calls being made to ref-only repos.

These two remaining items will be addressed in follow PRs

Baseline v2.9.0 with 200 multi-source applications with a ref only source:

With the changes in this PR under the same conditions:

Checklist:

codecov · 2023-11-21T08:54:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (819f0b3) 49.53% compared to head (131ce0c) 49.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16410      +/-   ##
==========================================
+ Coverage   49.53%   49.56%   +0.02%     
==========================================
  Files         269      269              
  Lines       47222    47223       +1     
==========================================
+ Hits        23392    23405      +13     
+ Misses      21530    21517      -13     
- Partials     2300     2301       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crenshaw-dev · 2023-11-21T16:35:43Z

Nice, looks like this massively mitigates the issue with a relatively lightweight change. Very safe and easy to cherry-pick back. Thanks for splitting up the PRs!

…s and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com>

nromriell · 2023-11-21T17:40:55Z

Nice, looks like this massively mitigates the issue with a relatively lightweight change. Very safe and easy to cherry-pick back. Thanks for splitting up the PRs!

Of course! Definitely was a great idea to split them, much easier to follow the changes

reposerver/repository/repository.go

Signed-off-by: nromriell <nateromriell@gmail.com>

ishitasequeira

Thanks @nromriell for breaking the huge PR into smaller chunks. The change is really effective.

The code looks good to me overall.

nromriell · 2023-11-23T00:13:13Z

Thanks @nromriell for breaking the huge PR into smaller chunks. The change is really effective.

The code looks good to me overall.

Of course, thanks for taking the time to review!

leoluz

Tks a lot for breaking the original PR.
Please check my comment.

leoluz · 2023-11-29T14:38:46Z

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)


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.

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

leoluz

LGTM
Awesome job @nromriell !

crenshaw-dev · 2023-11-30T15:19:35Z

/cherry-pick release-2.9

…unManifestGenAsync not using cache (Issue #14725) (#16410) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com> * fix: remove unnecessary settings instantiation Signed-off-by: nromriell <nateromriell@gmail.com> --------- Signed-off-by: nromriell <nateromriell@gmail.com>

…unManifestGenAsync not using cache (Issue #14725) (#16410) (#16494) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync * fix: remove unnecessary settings instantiation --------- Signed-off-by: nromriell <nateromriell@gmail.com> Co-authored-by: Nathan Romriell <nateromriell@gmail.com>

…unManifestGenAsync not using cache (Issue argoproj#14725) (argoproj#16410) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com> * fix: remove unnecessary settings instantiation Signed-off-by: nromriell <nateromriell@gmail.com> --------- Signed-off-by: nromriell <nateromriell@gmail.com> Signed-off-by: irinam0992 <irinam0992@gmail.com>

nromriell · 2023-12-01T03:03:13Z

Thanks @crenshaw-dev @ishitasequeira @leoluz for taking the time to review!

…unManifestGenAsync not using cache (Issue argoproj#14725) (argoproj#16410) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com> * fix: remove unnecessary settings instantiation Signed-off-by: nromriell <nateromriell@gmail.com> --------- Signed-off-by: nromriell <nateromriell@gmail.com>

…unManifestGenAsync not using cache (Issue argoproj#14725) (argoproj#16410) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com> * fix: remove unnecessary settings instantiation Signed-off-by: nromriell <nateromriell@gmail.com> --------- Signed-off-by: nromriell <nateromriell@gmail.com> Signed-off-by: Kevin Lyda <kevin@lyda.ie>

…unManifestGenAsync not using cache (Issue argoproj#14725) (argoproj#16410) * fix(repo-server): excess git requests part 1, resolveReferencedSources and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com> * fix: remove unnecessary settings instantiation Signed-off-by: nromriell <nateromriell@gmail.com> --------- Signed-off-by: nromriell <nateromriell@gmail.com>

nromriell requested a review from a team as a code owner November 21, 2023 08:45

fix(repo-server): excess git requests part 1, resolveReferencedSource…

c118ff4

…s and runManifestGenAsync Signed-off-by: nromriell <nateromriell@gmail.com>

nromriell force-pushed the lsremote-part1 branch from d47a64c to c118ff4 Compare November 21, 2023 17:33

crenshaw-dev reviewed Nov 21, 2023

View reviewed changes

reposerver/repository/repository.go Outdated Show resolved Hide resolved

fix: remove unnecessary settings instantiation

131ce0c

Signed-off-by: nromriell <nateromriell@gmail.com>

nromriell force-pushed the lsremote-part1 branch from 1e6588c to 131ce0c Compare November 21, 2023 18:09

ishitasequeira approved these changes Nov 22, 2023

View reviewed changes

leoluz reviewed Nov 29, 2023

View reviewed changes

leoluz approved these changes Nov 30, 2023

View reviewed changes

leoluz merged commit 5c187a1 into argoproj:master Nov 30, 2023
25 checks passed

gcp-cherry-pick-bot bot mentioned this pull request Nov 30, 2023

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

Merged

nromriell mentioned this pull request Dec 1, 2023

fix(repo-server): excess git requests, short-circuit GenerateManifests ref only #16501

Merged

13 tasks

nromriell mentioned this pull request Feb 7, 2024

fix(repo-server): excess git requests, add shared cache lock on revisions (Issue #14725) #17109

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nromriell commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

crenshaw-dev commented Nov 21, 2023

nromriell commented Nov 21, 2023

ishitasequeira left a comment

nromriell commented Nov 23, 2023

leoluz left a comment •

edited

Loading

leoluz Nov 29, 2023

nromriell Nov 29, 2023

leoluz left a comment

crenshaw-dev commented Nov 30, 2023

nromriell commented Dec 1, 2023

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

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

Conversation

nromriell commented Nov 21, 2023 • edited Loading

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

crenshaw-dev commented Nov 21, 2023

nromriell commented Nov 21, 2023

ishitasequeira left a comment

Choose a reason for hiding this comment

nromriell commented Nov 23, 2023

leoluz left a comment • edited Loading

Choose a reason for hiding this comment

leoluz Nov 29, 2023

Choose a reason for hiding this comment

nromriell Nov 29, 2023

Choose a reason for hiding this comment

leoluz left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Nov 30, 2023

nromriell commented Dec 1, 2023

nromriell commented Nov 21, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

leoluz left a comment •

edited

Loading