fix: repository.Init() to Add gitRepoPaths using the proper new cache key format #20962
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
fcb7feb to
2ed5acb
Compare
…key format Signed-off-by: Jean-Philippe Bélanger <30598824+jpbelangerupgrade@users.noreply.github.com>
2ed5acb to
c6534c0
Compare
|
We use pre-cloned repository so if a repository is already cloned, before it would cache that repo location. Now because of the project key, it’s ignoring the existing cloned repository and cloning a new one. So before this change. we’d end up with twice the repo cloned. |
| return err | ||
| } | ||
|
|
||
| s.gitRepoPaths.Add(string(keyData), fullPath) |
There was a problem hiding this comment.
Shall we update getters as well? How does it work?
There was a problem hiding this comment.
All other "Get" (GetPath and GetPathIfExists) have been updated already to use the Project with the composite key. Add was left behind apparently in the original PR
|
Hey @andrii-korotkov-verkada . Do you have think we can have an ETA on that ? We are waiting for that to be able to update. |
|
Unfortunately no. I'm not an approver myself and it's a holiday season. I suggest posting in #argo-cd-contributors in CNCF Slack, but still it can take time. |
|
@blakepettersson just as an FYI |
|
We should have a unit test that reproduces the bug. |
|
@crenshaw-dev As much as I'd like to, this whole method uses os package and binds itself to the filesystem. This would be extremely hard to mock and actually do a valid test case. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20962 +/- ##
==========================================
+ Coverage 55.21% 55.27% +0.05%
==========================================
Files 337 337
Lines 56840 56843 +3
==========================================
+ Hits 31387 31419 +32
+ Misses 22757 22736 -21
+ Partials 2696 2688 -8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hey JP! Long time no see :)
@jpbelangerupgrade Can you please clarify in the PR description the real impact of this supposed regression?
It seems that you are trying to pre-clone the monorepo and have the repo-server using it. Can you pls confirm? Why is this a hard requirement for monorepos? I'd like to understand how impactful this regression really is.
| @@ -172,7 +172,12 @@ func (s *Service) Init() error { | |||
| closer := s.gitRepoInitializer(fullPath) | |||
| if repo, err := gogit.PlainOpen(fullPath); err == nil { | |||
| if remotes, err := repo.Remotes(); err == nil && len(remotes) > 0 && len(remotes[0].Config().URLs) > 0 { | |||
| s.gitRepoPaths.Add(git.NormalizeGitURL(remotes[0].Config().URLs[0]), fullPath) | |||
| keyData, err := json.Marshal(map[string]string{"url": git.NormalizeGitURL(remotes[0].Config().URLs[0]), "project": ""}) | |||
There was a problem hiding this comment.
@blakepettersson Do you see an issue about setting the project with an empty string at this stage? User is claiming that the introduction of the project in the key broke this flow. Can you please confirm if this is indeed a regression of #18388?
There was a problem hiding this comment.
Also updated PR description with more details and link to another issue we were discussing for alternative
There was a problem hiding this comment.
@jpbelangerupgrade Can you pls confirm how you use AppProject at your company? Do you have a single AppProject for all your apps, 1:1 or everything goes in the default project??
There was a problem hiding this comment.
To follow up on @leoluz's question, I presume you have a single (non-project scoped) credential for the repo (I presume yes since you have not yet upgraded to 2.12)?
My concern with this approach is that while this may solve your immediate predicament, this would break once/if a project scoped repo credential is added for the same repo url, since that would involve another git clone. Thinking out loud, it might be worthwhile having a way to configure these cache mappings in the repo-server which it would then use to initialize the cache mappings? Something like Repo URL -> n projects + paths? IMO this needs a bit more thought before saying yay nor nay.
Ultimately, since your repo is ginormous (20gb+) I think alternative ways should be considered (e.g #11198 and/or something more immediate, e.g this comment).
There was a problem hiding this comment.
Shallow and sparse checkouts aren't a viable short-term option. I'm really not sure they're even a viable long-term option. We really should be able to support 20GB repos without too much pain.
I'm trying to make sure I understand why we don't have the project name available in Service.Init. It looks to me as if we're reconstructing the git paths cache from the repos found on disk. If we need additional info from storage (i.e. the project(s) associated with each repo dir), why don't we just write that info to disk so it's available when we need it?
There was a problem hiding this comment.
Personally, I feel reasonably okay w/ multiple projects sharing the same cache dir. Even though different credentials could technically expose different contents, I think that would be super atypical. The permissions granularity in every SCM afaik is "clone the repo" or "not clone the repo."
There was a problem hiding this comment.
Personally, I feel reasonably okay w/ multiple projects sharing the same cache dir. Even though different credentials could technically expose different contents, I think that would be super atypical. The permissions granularity in every SCM afaik is "clone the repo" or "not clone the repo."
@crenshaw-dev Do you mean, removing the project from the cache key?
There was a problem hiding this comment.
My first comment is proposing keeping the cache key, but storing it on disk so it's available for cache restoration.
My second comment is, yep, might make sense to just remove the project from the cache key.
There was a problem hiding this comment.
Which entity is the owner of the cached revision? Would it be the project or the credentials used to access the repo?
There was a problem hiding this comment.
Strictly speaking, I think it makes sense to say that the credential owns the cached revision. But practically, I think project serves as a reasonable proxy for credentials which may or may not be different. At worst, we treat two projects as different when the credentials are actually the same.
But I'm not sure it makes sense to differentiate cache ownership by credential or project. I feel like it's safe to assume that each credential has the same read access to the repo (full clone access) and that any write operations performed by applications in different projects are protected by the concurrent access lock (i.e. one project can't lift a secret that was written to disk by a different project).
But I think it would be fair to review any write operations that occur during manifest generation (for example, modifications to files in the .git config repository) to ensure that 1) they're protected by the concurrent access lock and 2) sensitive changes are cleaned up between operations.
Signed-off-by: Jean-Philippe Bélanger <30598824+jpbelangerupgrade@users.noreply.github.com>
Long time no see indeed. Thank for taking a look. Updated the description with more details and impact on our side. |
|
I did a validation of before and after this change. One thing that @leoluz was mentioning was that my change would regress the current behavior of the project based auth. Based on my test it doesn't:
@crenshaw-dev and @blakepettersson this might be of interest to figure out the next step here. |
Regression from #18388
The gitRepoPaths cache key was updated to now include not just the repository url but the repository Project. Since during init we have no way of knowing the repository project. At least add that repository into cache as the "default" / undefined project.
Note: Cherrypicking this in all active release would be great given 2.12+ are all impacted.
Context:
Our manifest repository is ~20Gi or so. It takes about 5mins minimum to get it cloned properly. What happens on repo-server startup is that repo-server become available BEFORE the manifest repository is cloned. Creating a high cpu usage and also a massive pile-up of request to the restarting repo-server which then cause app to become "unknown" and paging our dev team. This makes argo-cd completely freeze up for 5-15mins everytime we do a node update, recycle a node or any update to repo-server itself.
More information and alternative discussed can be read on #7927 (comment)
To alleviate that situation, we run an init-container that pre-clones the repo before starting the repo-server process. Which makes repo-server startup much more stable and doesn't come with a freeze on argo-cd reconciliation
Checklist: