Repository navigation
Conversation
github-actions
Bot
added
the
component/operator
20e7dae to
0b54238
Compare
On a large cluster the leader runs a GC every few seconds and holds about twice its live heap, while the follower, which only holds caches, is flat. Profiles show the allocation is on the reconcile path, not cache upkeep: - globalDelayHandler.relevant listed every NodeWright from the cache on every Node event, and a cached List deep-copies each object. NodeWright status carries per-node state, so each heartbeat copied megabytes to run a label selector, and initial sync replayed that for every Node. The list is now read without the copy inside skyhookSelectors, which lets only the parsed selectors out, so no cache pointer outlives that call. An unparseable selector now matches every node instead of dereferencing a nil selector. - BuildState deep-copied a Node once per NodeWright that selected it, although ObjectTracker.Track keeps the first copy and drops the rest. It also parsed the selector and built a wrapper for every node before checking the match. Parse once per NodeWright, check first, copy once. - Cached objects kept metadata.managedFields, which nothing reads and which is a large share of Pods and Nodes. Strip it on the way into the cache; a nil managedFields on update leaves the server's intact. Two behaviour changes in BuildState, both covered by new specs: - A NodeWright whose package graph cannot be built (the only error NewSkyhookNode can return) and which selects no node no longer fails the pass for every NodeWright. It still fails when it selects a node. - An invalid nodeSelector now fails the pass even with no nodes listed, where before the parse only ran inside the node loop. Admission already rejects both cases. Signed-off-by: Taylor Daugherty <tdaugherty@nvidia.com>
0b54238 to
9104b77
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #796 requires lower reconcile-path allocation and unchanged behavior. The current code implements the allocation work: skyhookSelectors uses client.UnsafeDisableDeepCopy, BuildState parses… |
Preserve the existing behavior for invalid selectors and for unselected NodeWrights with an unbuildable package graph while retaining the allocation optimizations. Update the tests to assert the required unchanged behavior. |
| Out of Scope Changes check | |
The cache transform, selector reuse, relevance filtering, tracker check, and related tests directly support issue #796. The current invalid-selector fallback in event_handler.go and the changed `Bui… |
Remove the behavior changes from the allocation-focused patch. Keep the optimizations and tests that directly verify issue #796, and preserve the prior error and relevance semantics for rejected inputs. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: reducing reconcile-path allocations in the operator for large clusters. Description check The description directly explains the allocation problem, the implemented optimizations, behavior differences, testing, profiling, and verification status.
Full details: Linked Issues check
-
Explanation
Issue
#796requires lower reconcile-path allocation and unchanged behavior. The current code implements the allocation work:skyhookSelectorsusesclient.UnsafeDisableDeepCopy,BuildStateparses each selector once and avoids duplicate Node copies, and the manager cache stripsmanagedFields. However,BuildStatenow skips package-graph construction for a NodeWright that selects no Nodes, andskyhookSelectorstreats an invalid selector aslabels.Everything(). These are observable behavior changes. The PR identifies them as admission-rejected cases, but issue#796still requires unchanged behavior.
Full details: Out of Scope Changes check
-
Explanation
The cache transform, selector reuse, relevance filtering, tracker check, and related tests directly support issue
#796. The current invalid-selector fallback inevent_handler.goand the changedBuildStateerror path for unselected NodeWrights change behavior rather than reduce allocations. Issue#796does not include these behavior changes, even if admission normally rejects the inputs.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
ayuskauskas
left a comment
ayuskauskas
left a comment
There was a problem hiding this comment.
Compared BuildState and the relevance check against main with a randomized base-vs-head harness: the only differences are the two admission-rejected cases in the description, and the tracker still holds one unaliased original per node. A synthetic 2700-node, 4-NodeWright benchmark puts the relevance check at ~14 MB per Node event on main and ~6 KB here, consistent with your startup number.
One behavior change worth adding to the description: on main an unparseable nodeSelector makes matchSelectors call Matches on the nil Selector LabelSelectorAsSelector returns, which panics in the Node event handler; now it matches every node.
Your 2.7k-node numbers also show the sizing guidance falling short at this scale: docs/operations/resources-at-scale.md is validated only up to 1k nodes, and the chart would compute a 2160Mi limit for that cluster. I opened #839 to redo that investigation on top of this PR. It does not block this one.
Description
Why: on a cluster of roughly 2,700 nodes and 80,000 pods, the operator could not stay up at its 4 Gi memory limit. Both replicas were OOMKilled in a loop, each dying about 40 seconds after its informers started, so no packages were applied at all. Raising the limit to 12 Gi kept it alive, with the leader then settling at about 7 GiB of working set against roughly 3 GiB of live heap, allocating around 800 MiB/s and running a GC every 3 seconds, while the follower, which only holds caches, sat flat at 3 GiB (#796). Profiling put the difference on the reconcile path, not cache upkeep. This removes the three allocation sources without changing what the operator decides.
skyhookSelectors. This alone accounted for about 40 GB of allocation during startup, which is the crash-loop.BuildState: parse each selector once, check it before building a wrapper, copy a Node into the tracker once.managedFieldsas objects enter the cache.Behaviour changes, both in cases admission already rejects: a NodeWright whose package graph cannot be built and selects no node no longer fails the pass for every NodeWright; an invalid
nodeSelectorfails the pass even with no nodes listed.closes #796
How this was verified
make unit-testsfromoperator/: all suites pass. New specs cover both behaviour changes (they fail onmain), the tracker invariant the copy-once change relies on, andObjectTracker.Tracked.mainvs this branch, same base: allocation during initial sync 44.8 GB to 4.0 GB, per heavy pass 1.5 GB to 0.45 GB, retained heap down about 20%,Node.DeepCopyCPU share 23% to 8%. Live heap after the change is about 2.5 GiB at that scale, dominated by the Pod cache, and with default GOGC the heap still peaks near twice live between collections; a limit around 6 Gi, or 4 Gi with GOMEMLIMIT set, is the realistic floor there.make lint(toolchain mismatch); relying on CI.Checklist
git commit -s -S.