feat: make manifest hydration queue concurrency configurable (#27926) - #27948
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
fa9e8f8 to
6a829c5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #27948 +/- ##
==========================================
+ Coverage 64.67% 64.70% +0.02%
==========================================
Files 423 423
Lines 58428 58437 +9
==========================================
+ Hits 37790 37811 +21
+ Misses 17116 17111 -5
+ Partials 3522 3515 -7 ☔ View full report in Codecov by Harness. |
6a829c5 to
464b619
Compare
|
@GuruduGanesh picking this up this week. :-) |
…worker Per @crenshaw-dev's review on argoproj#27948, the per-app Hydrating status update that used to live on the single-threaded app hydrate queue now happens inside the hydration queue worker, where the workqueue's per-key dedup guarantees exclusive ownership of the whole app group. This removes the race condition described in argoproj#27926 by construction (rather than guarding each status write against a nil/stale CurrentOperation), and the ProcessAppHydrateQueueItem worker becomes a pure enqueuer for the hydration key. The new helper Hydrator.markAppsHydrating runs at the start of ProcessHydrationQueueItem and stamps every app in the group as Hydrating before validation or hydration runs, so the success and failure paths can drop their CurrentOperation nil guards. The complete-path-set commit behavior is preserved. Tests updated: - Replace the race-window tests (TestProcessHydrationQueueItem_RaceConditionAppNotHydrating, _MixedPhases_OnlyHydratingAppsPersisted, _SkippedAppFinalizesOnLaterPass, TestSetAppHydratorError_NilCurrentOperation) with tests that lock the new contract: - _MarksAllAppsHydratingThenHydrated (mixed initial phases) - _MarksHydratingBeforeValidation (ordering guarantee) - _LargeGroupAllAppsPersisted (20-app scale check) - _CommitsCompletePathSet kept as the partial-hydration regression guard. - Updated PAHQI tests (_HydrationNeeded_NoCurrentOperation, _RevisionChanges) to reflect that PAHQI no longer mutates CurrentOperation. Refs: argoproj#27926 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Ganesh Gurudu <ganesh.gurudu@gmail.com>
Bundle ReportBundle size has no change ✅ |
…j#27926) Signed-off-by: Ganesh Gurudu <ganesh.gurudu@gmail.com>
…rebase Signed-off-by: GuruduGanesh <ganesh.gurudu@gmail.com>
510787c to
9a01966
Compare
…on/DeleteWithInvalidClusterName This is an extra fix outside the scope of argoproj#27926 (hydration queue concurrency). It addresses a pre-existing data race in TestFinalizeAppDeletion that the "Run unit tests with -race" CI stage caught on this PR. I am including it here because the race is currently the only thing keeping the PR from a deterministic green CI run. Race details (Go race detector report on CI and reproduced locally): - Write: fakeAppCs.ReactionChain = nil controller/appcontroller_test.go:1165 (TestFinalizeAppDeletion.func4.1) - Read: k8s.io/client-go/testing.(*Fake).Invokes via setAppCondition → Patch, fired from the application informer's namespace indexer at controller/appcontroller.go:2562 The DeleteWithInvalidClusterName/app3 case sets both Destination.Name and Destination.Server, which makes argo.GetDestinationCluster return "destination can't have both name and server defined". The namespace indexer in newApplicationInformerAndLister handles that error by calling ctrl.setAppCondition, which patches the app via the fake clientset on the informer goroutine. Meanwhile the test goroutine wipes the reactor chain in place. The fake client's RWMutex protects against this, but the direct field write ReactionChain = nil bypasses the mutex. Confirmed pre-existing in upstream: reproduced 2/20 on plain upstream/master without any of this PR's changes applied, in a disposable git worktree. The same risky pattern (ReactionChain = nil) appears at 21 sites in this file, but only this one trips because only DeleteWithInvalidClusterName constructs a destination that drives the informer into setAppCondition. The other 20 sites are left untouched here to keep the fix minimal and tightly scoped. Fix: wrap the chain read+swap in fakeAppCs.Fake.Lock/Unlock, which serializes with the RLock that Fake.Invokes already takes. AddReactor below acquires the lock internally. Local verification on this branch: - go test -race -count=40 -run '^TestFinalizeAppDeletion$' ./controller: 0 races, PASS - go test -race -count=1 ./controller (full suite): 0 races, PASS Refs: argoproj#27926 Signed-off-by: Ganesh Gurudu <ganesh.gurudu@gmail.com>
Signed-off-by: GuruduGanesh <ganesh.gurudu@gmail.com>
|
Thanks @crenshaw-dev, addressed. I moved the per-app This is safe under parallel hydration workers because the workqueue dedups by key: I also fixed a pre-existing data race that the latest CI run surfaced. That failure was in Local validation:
|
crenshaw-dev
left a comment
There was a problem hiding this comment.
Thank you, @GuruduGanesh!
What / why
Closes #27926.
The manifest hydration queue used by the Source Hydrator is currently drained by a single goroutine, so source repos are hydrated one at a time. In hub-spoke topologies where a single Argo CD instance hydrates many repos, this serial processing becomes the bottleneck for how quickly changes become deployable.
This PR makes the hydration queue concurrency tunable, mirroring the existing
--status-processors/--operation-processorspattern:--hydration-processors(envARGOCD_APPLICATION_CONTROLLER_HYDRATION_PROCESSORS), default5, minimum1.hydrationQueueis drained by N workers;appHydrateQueuestays single-worker. The queue is keyed by{SourceRepoURL, SourceTargetRevision, DestinationBranch}and is rate-limiting, so the same key is never processed by two workers at once — additional workers only parallelize across distinct keys.Run(...), the application-controller manifests,argocd-cmd-params-cm, and the HA/operator docs.Additional correctness fix (required to enable concurrency safely): running multiple workers makes a pre-existing race likely — a hydration item can be processed before all apps for a key are marked
Hydrating(nilCurrentOperation), which previously panicked and could overwrite status. To handle this safely the hydrator now always commits the complete app set for a key in a single pass (the commit server records a git note per dry SHA and short-circuits later commits, so a partial commit would leave some apps marked hydrated without their manifests), and only the per-app status writes are guarded (apps not yetHydratingare skipped and finalize on a later refresh/resync pass). The worker count is clamped to a minimum of 1 so a0/negative flag value cannot silently stall hydration.Checklist: