perf: improve reconcile throughput under registry latency and large namespaces#16
Merged
Conversation
Both controllers ran at controller-runtime's default of one worker while reconciles block on registry probes, so one slow request delayed every unrelated CR. Concurrency is now per-controller configurable (flag or env, default 4) and rejected below 1.
Both ensure passes eagerly listed every CocoonHibernation in the namespace before checking whether any pod was missing, so a steady reconcile paid two O(Hibernations) Lists to build a map it never read. A per-reconcile memo defers the List to the first pod that has to be built and shares it across the main, sub-agent and toolbox paths.
- extract createSubAgents so the fan-out stops nesting inside ensureSubAgents
and its goroutines no longer shadow a live outer err (nestif + govet shadow)
- replace the hand-rolled sync.Once memo with stdlib sync.OnceValues
- drop the dead empty-string branch in envInt; strconv.Atoi("") already errors
- fold the benchmark and blocking-probe registry fakes into fakeRegistry
Drop the ones restating a return value or repeating an adjacent godoc, and fold the rest to the one non-obvious constraint each carries.
…urrency Nothing rejects two CocoonHibernation CRs naming the same podRef, and the pod watcher fans an event out to every CR that targets it, so opposing Hibernate/Wake desires can both be live. Above one worker they interleave their patch of the shared hibernate annotation and their HasManifest / DeleteManifest of the one :hibernate tag, so a CR can settle Hibernated after the tag was already deleted. A per-podRef lock restores the ordering the single worker gave for free; distinct pods stay concurrent. reconcileDelete drops its always-zero ctrl.Result while here. Also document the two concurrency knobs and make the wedged-probe test wait for the probe to actually block, so it cannot pass under serialization.
…able reconcileDelete drops the same :hibernate tag a live CR on that pod probes, but the lock sat below the deletion fast path, so a deleting CR skipped it. Stripe podLocks into a fixed table: pod names come and go with CocoonSets, so a per-name map grows for the process lifetime; a stripe collision only costs two unrelated pods an occasional serialization.
spec.podRef.name carries no immutability constraint (the CRD has only minLength), and markPending never rewrites Status.VMName, so a retargeted CR keeps naming its old VM in status. reconcileDelete drops the tag of that stale VM, so keying the lock on spec.podRef locked the new pod while touching the old VM's tag, leaving a live CR on the old pod free to probe the tag being deleted. Lock the VM whose tag and pod annotation are actually being touched: the resolved vmName on the live paths, Status.VMName on the delete path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #15.
Problem
Two independent effects limited controller throughput at scale.
1. Reconciles were single-worker while performing network I/O. Neither controller set
MaxConcurrentReconciles, so controller-runtime ran one worker each. Reconciles synchronously wait on registry operations (HasManifest,DeleteManifest), so one slow registry request delayed every unrelated CR.2. CocoonSet listed all namespace Hibernation CRs twice on the steady path.
ensureSubAgentsandensureToolboxesboth loaded restore intent before checking whether any Pod was missing, so a steady reconcile paid two O(Hibernations-in-namespace) cache Lists to build a map it never read.Fix
Concurrencyis now an independently configurable field on both reconcilers (--cocoonset-concurrency/--hibernation-concurrency, envCOCOONSET_CONCURRENCY/HIBERNATION_CONCURRENCY, default 4), passed throughcontroller.Options{MaxConcurrentReconciles}and rejected below 1 inSetupWithManager.sync.OnceValuesmemo built once per reconcile and shared across the main, sub-agent and toolbox create paths. Nothing missing → no List; anything missing → exactly one.Raising CocoonSet concurrency needed no extra locking:
cocoonset.Reconcilerholds no mutable state, andhibernation.Reconciler.observedis already async.Mapkeyed per-CR.The pod lock (why hibernation needed more than a flag)
Nothing rejects two
CocoonHibernationCRs naming onepodRef— cocoon-webhook has no validator for the kind, and the controller anticipates the shape (indexPodRefName's comment says "the CRs that target it";hibernationsTargetingPodreturns a list). Opposing Hibernate/Wake desires contend for one hibernate annotation and one:hibernatetag.The bad end state predates this PR — it is reachable sequentially at one worker (A probes present →
Hibernated→ no requeue; B then deletes the tag). Raising the worker count widens and nondeterminizes that window rather than creating it. A striped per-podRef lock restores the ordering the single worker gave for free; distinct pods stay fully concurrent. The lock sits ahead of the deletion fast path, sincereconcileDeletedrops the same tag a live CR may be probing.The remaining semantic gap — two live CRs with opposing desires on one pod is nonsense regardless of timing — is out of scope here and filed as #17 (reject the second CR at admission).
Tests
TestReconcileSteadyStateSkipsHibernationList— steady reconcile performs zero Hibernation Lists (asserts the pods survive first, so 0 can't pass vacuously via an early return).TestReconcileMissingPodsListsHibernationsOnce— missing sub-agent and toolbox → exactly one List.TestReconcileUnrelatedKeyProgressesWhileProbeBlocks— one CocoonSet's registry probe wedged, an unrelated one still completes. Waits for the probe to actually block first, so it cannot pass under serialization.TestReconcileSerializesCRsTargetingOnePod/TestReconcileSerializesDeletingCRAgainstLiveCR— two CRs on one pod (live/live and deleting/live) never overlap a registry call.TestSetupWithManagerRejectsInvalidConcurrency(both packages) +TestEnvInt— invalid concurrency values.Both serialization tests were mutation-checked in both directions: removing the lock (or moving it below the deletion fast path) makes them fail; restoring it makes them pass under
-race.Benchmark
BenchmarkReconcileThroughput: 24 independent CocoonSets, each reconcile blocking on one 5ms fixed-latency registry probe, drained through a worker pool (concurrency 1 = pre-change behavior). Apple M2 Max,-benchtime 5x:Wall time drops ~2.6x at 4 and ~3.9x at 8. Per-reconcile p95 rises with contention on the shared fake client — throughput is what this buys, not individual latency.
Note the eliminated Lists are served from the controller-runtime cache, not the apiserver, so that half saves O(N) scan and allocation per reconcile rather than network round trips; the win shows up in large namespaces.
Gates
go build/go vet/go test ./... -racegreen;make lint0 issues on both GOOS=linux and GOOS=darwin.