target: cache compiled LabelSelector per ClusterGroup to reduce CPU - #5445
target: cache compiled LabelSelector per ClusterGroup to reduce CPU#5445himabindugit wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes target selection by caching compiled Kubernetes LabelSelector objects per ClusterGroup (keyed by namespace/name@resourceVersion) to reduce repeated metav1.LabelSelectorAsSelector compilations during reconcile loops.
Changes:
- Implement per-
Managerselector compilation caching viasync.MapinclusterGroupsForCluster. - Add unit tests for selector matching behavior and caching behavior.
- Extend the
target.Managerstruct to hold the selector cache.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cmd/controller/target/query.go | Adds selector caching in clusterGroupsForCluster to reduce CPU/allocations during target queries. |
| internal/cmd/controller/target/query_test.go | Adds unit tests for selector matching/caching behavior. |
| internal/cmd/controller/target/builder.go | Adds a sync.Map field to Manager to store the selector cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @aruiz14 — following up on this PR. Note:
Could you let us know if there's any additional information needed to move this forward, or an expected timeline for review? |
|
Load test + production incident evidence Environment: Load test (2026-07-28): 30-second CPU profile (before fix): Duration: 30.20s, Total samples = 401.12s (1328.14%) LabelSelectorAsSelector accounts for 55.54% of all CPU (222.79s out of 401.12s sampled) — consistent with the original production profile where it was 61.49%. Production incident (2026-07-27): A post-incident investigation on our production cluster (3,932 clusters / 4,153 bundles / 25,865 bundledeployments) found fleet-controller listing all bundles every ~1.5–1.9 seconds with no resourceVersion and no limit — ~40 full-namespace etcd scans per minute. This drove the etcd node's EBS volume to its 3,000 IOPS ceiling (9.6 GB read from disk in one minute from a 1 GB database), freezing etcd and crashing fleet-controller. The excessive LIST frequency is a direct consequence of the O(N³) reconciliation loop. The caching fix in this PR reduces reconciliation frequency dramatically, which in turn reduces LIST pressure on etcd. |
d3flex
left a comment
There was a problem hiding this comment.
Can you please squash some commits in the history into a couple of logical commits? Right now the history has a lot of fixup/WIP ones. It would make easier the review and keep the history tidy. And while fixing that, two other things missing: commit body and signatures. add a few words on the commits . Mostly are empty (especially the caching change)
Cache the compiled metav1.LabelSelector → labels.Selector per ClusterGroup, keyed by namespace/name@resourceVersion. Avoids repeated LabelSelectorAsSelector calls during reconcile — previously O(clusters × bundles × ClusterGroups), now O(ClusterGroups) on cache miss. Cache entry is evicted when ResourceVersion changes, ensuring correctness after any ClusterGroup update. Also fixes range-variable pointer bug (cgCopy := cg) so appended pointers refer to independent copies. Signed-off-by: Himabindu Sanagavarapu <Himabindu.Sanagavarapu@viasat.com>
Previously called once per Bundle inside BundlesForCluster, causing O(bundles × clusterGroups) work per cluster reconcile. Moving the call above the loop reduces this to O(clusterGroups) per reconcile. Adds integration tests verifying correct bundle refresh/cleanup behavior and that all bundles are evaluated against the same pre-computed ClusterGroups. Signed-off-by: Himabindu Sanagavarapu <Himabindu.Sanagavarapu@viasat.com>
24a9ea5 to
ac7ff89
Compare
Thanks for the feedback! Addressed all three points:
|
|
#5444
Refers to #5444
Problem
clusterGroupsForClustercallsmetav1.LabelSelectorAsSelectoron every reconcile loop for every ClusterGroup, with no caching. At scale this causes an allocation storm.With ~3900 clusters × 10 ClusterGroups: 39,130
LabelSelectorAsSelectorcompilations perOnBundleChangeevent. pprof showedLabelSelectorAsSelectorconsuming 61% CPU on the fleet-controller, leading to OOM and lease-lock failures.Reported in: #5444
Fix
Cache the compiled
labels.Selectorin async.Mapon theManager, keyed bynamespace/name@resourceVersion. TheResourceVersionensures the selector is recompiled when a ClusterGroup is updated.ClusterGroups change infrequently and their count is small, so stale entries from old ResourceVersions are acceptable and not evicted.
Added unit tests covering:
Additional Information
AI-Assisted Code Review
This change was analyzed using Claude Code (Anthropic, claude-sonnet-4-6) as part of a broader investigation into fleet-controller CPU instability (production incident IM-890045).
What the analysis found:
LabelSelectorAsSelectoris called inside nested per-cluster × per-bundle loops intarget.go, recompiling selectors on every reconcile cyclereduces this to O(ClusterGroups) ≈ 50 unique compilations
Evidence:
LabelSelectorAsSelectoras the dominant hot pathCorrectness conclusion:
The AI analysis concluded that correctness is maintained — cache entries are invalidated on resourceVersion change, ensuring stale selectors are never used.
This was independently verified by Stephen O'Neal (sonealv), and GitHub Copilot.
Checklist