fix: Allow plugins to lock repository at top-level to prevent git races #6223
Conversation
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Codecov Report
@@ Coverage Diff @@
## master #6223 +/- ##
==========================================
- Coverage 41.37% 41.37% -0.01%
==========================================
Files 161 161
Lines 21595 21599 +4
==========================================
Hits 8936 8936
- Misses 11397 11399 +2
- Partials 1262 1264 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good! Since #4625 merged, reasoning about the concurrency implications of this particular area of the codebase is fairly time consuming 🤔 .
While you're at it, can you add this comment to the var manifestGenerateLock = sync.NewKeyLock() line?
// When multiple threads are accessing a Git repository (checked out in the local container,
// eg under '/tmp/(...)') at the the same time, the existing 'Service.repoLock' lock should
// prevent those threads from clobbering each other's files with different revisions (for non-Helm).
//
// However, some manifest generation utilities (helm, custom config management plugins) might still
// write to this repository path during execution of that utility. Thus we need a way to ALSO
// prevent multiple threads from running manifest generation at the same time against the
// same Git repository path. Note: for non-Helm, this will only happen when the two threads are
// both using the same Git revision at the same time (differing revisions is covered by repoLock).
//
// That is the purpose of manifestGenerateLock: this lock should be acquired if a manifest
// generation utility might cause files to be written to the checked out Git repository
// path, and thus multiple threads should not run this utility at the same time.
var manifestGenerateLock = sync.NewKeyLock()This is my understanding of the purpose of this lock, after reading through the code in the context of reviewing the PR.
| manifestGenerateLock.Lock(repoRoot) | ||
| defer manifestGenerateLock.Unlock(repoRoot) | ||
| } else { | ||
| concurrencyAllowed := isConcurrencyAllowed(appPath) |
There was a problem hiding this comment.
The isConcurrencyAllowed method already returns false for plugins. Unless I'm missing something plugins already lock repo. We probably need opposite setting allowConcurrency for plugins
There was a problem hiding this comment.
As far as I could tell, it locks on the appPath, but we need to lock at the repository root to prevent concurrent git operations (i.e. two applications using same plugin calling git in the same repo, but on different paths).
There was a problem hiding this comment.
That isConcurrencyAllowed method only checks for .argocd-allow-concurrency, which could exist for plugins, unless I am missing something? I had assumed that the general point of the logic here was about forcing this method to lock even if that file was specified (if the plugin is configured with lockrepo).
Signed-off-by: jannfis <jann@mistrust.net>
|
@jessesuen what is the status ? |
|
Hello, I've got the same issue. @jessesuen Is is possible to review this PR ? :) |
|
This should be good to go. @jannfis just needs to rebase and merge. |
|
@jannfis @jessesuen we have had a talk about this issue with our RHEL support (OpenShift pipeline). Does it help if I open an issue on that side of the road ? |
|
Hey sorry, somehow this slipped under the radar. Going to rebase and merge. |
…/plugin-global-lock Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Fixes #6174
This PR introduces a new plugin configuration option,
lockRepo: <bool>:Custom plugins may make use of
gitfor certain operations (e.g.git crypt), which requires an exclusive lock on the Git repository.The current locking mechanism for plugins lock on the application path within the repository, which may not be sufficient if two applications using a plugin that executes
giton the same repository, but on different paths.As a follow-up, we should modify our
KeyLockinterface at https://github.com/argoproj/pkg/blob/master/sync/key_lock.go to also consider locks on path-based matching, instead of exact matches. E.g. if a lock exist on/tmp/foo/bar, a request on a lock for/tmp/foo/bar/bazshould wait until the previous lock is released.Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: