-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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.
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
fix(repo-server): excess git request when using multi-source and ref sources (Issue #14725) #16309
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #16309 +/- ##
==========================================
+ Coverage 49.55% 49.86% +0.31%
==========================================
Files 269 269
Lines 47039 47215 +176
==========================================
+ Hits 23309 23545 +236
+ Misses 21444 21373 -71
- Partials 2286 2297 +11 ☔ View full report in Codecov by Sentry. |
f7d8914
to
973dd4d
Compare
Signed-off-by: nromriell <nateromriell@gmail.com>
Signed-off-by: nromriell <nateromriell@gmail.com>
Signed-off-by: nromriell <nateromriell@gmail.com>
Signed-off-by: nromriell <nateromriell@gmail.com>
8458366
to
e833653
Compare
Moving this back to draft as I work through some changes after the discussion on Thursday |
… consistent behavior Signed-off-by: nromriell <nateromriell@gmail.com>
Added taking advantage of the two level cache and removing the second method from the util/cache interface to avoid confusion so this should be ready to review now. The actual changes are fairly small but added quite a few lines in mocks and tests |
@nromriell is it possible to break fixes 1, 2, and 3 into separate PRs? I think it's a bit too big to reason about right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an initial comment to align on the strategy behind this PR. A proper review is still required.
type Item struct { | ||
Key string | ||
Object interface{} | ||
CacheActionOpts | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any strong reason to implement this struct as part of this PR? It changes the signature of the CacheClient
interface. It seems an interesting refactoring but I think it would be better to keep this PR as small as possible. How about provide code refactoring only in a separate PR? The reason is that it allows us to easily rollback changes if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary reason is for (2) in my description here. The need for DisableOverwrite
(redis NX or memcache add) otherwise depending on timing each caller could think they were the lock holder, of course in practice that likely wouldn't be the case but it would cause inconsistent behavior.
To add that as an option I'd have to modify the caller anyway to add another option.
This struct already exists as well it's in line 13 of the diff. I just add CacheActionOpts
to it.
Thanks @crenshaw-dev @leoluz for taking a look, I'll spend some time later today and split these pieces out into separate PRs |
I've split this PR into four parts. Each piece is on it's own a decent improvement to the overall git requests. We can just start with the first one that's open in this repo and then I'll retarget the others once the one's before it have been merge. The first two are fairly standalone, it's only the testing mocks that are shared. The last one has a hard dependency on the one before it.
|
I'll close out this PR in favor of those |
Fixes #14725
Overview
There are a couple issues with using multi-source and ref based sources in ArgoCD right now. At scale these start to create enormous amounts of requests to the git server making this pattern unsustainable.
This broke down to three issues:
resolveReferencedSources
andrunManifestGenAsync
were not using the cache.The related fixes in this PR:
resolveReferencedSources
andrunManifestGenAsync
to have access to the cacheAdditional changes:
reposerver/cache/cache
twolevelclient
to take advantage of the in-memory cache on top of the redis cache. Options are used to skip the in-memory cache for most items, this results in no change for most items in their cache.These items were changed to use both in-memory and redis from the two level cache:
- git refs
- git directories
- revision metadata
- helm indexes
- chart details
util/cache/cache
AddCacheFlagsToCmd
to return a two level cache instead of just the redis cacheSetItem
replacingdelete bool
with an options objectutil/cache/client
CacheClient
interface to just accept a singleItem
parameter forGet
calls, interface now matchesSet
. Options are passed as part of that item.util/cache/twolevelclient
Get
andSet
to allow options to be passed in by theItem
to skip either the local or remote cache. This allows us to swap the cache in place in parts of the application without forcing the move for everything to use both in-memory and redisComparison of 200 multi-source apps with the second source being just a values ref and with revision cache invalidation every 30s:
Before:
After:
Transitioning from v2.9.0 to the custom image
Checklist: