fix: avoid deep-copying nodes, containers, and pods in planner inventory read

[kristina-solovyova]

No description provided.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[kristina-solovyova]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix: tlc drive core capping and address linting issues #2792
• fix: upgrade weka-sign-tool default image #2787
• fix: bump go version to 1.27.0 #2786
• fix: detect compute-blocked drive growth, split per-cause warnings, fix infeasibility binding #2785
• fix: tighten auto-full-drives compute sizing and per-role hugepages admission checks #2780
• fix: refactor the new wekacluster admission validators #2769
• fix: avoid deep-copying nodes, containers, and pods in planner inventory read #2770 👈 (View in Graphite)
• fix: aggregate auto-full-drives planner warnings into one event per condition instead of per node #2768
• docs: document act-as-daemonset mode (OP-345) #2748
• feat: weka-capacity dry-run for auto-full-drives (OP-345) #2747
• feat: admission validators for the capacity planners (OP-345) #2746
• feat: support full-drive wekacluster acting as a daemonset and using all drives (OP-345) #2742
• refactor: share node-eligibility, pod-status and drive-ordering helpers (OP-345) #2745
• main

---

How to use the Graphite Merge Queue

Add the label main-merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

Graphite Automations

"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/20/26)

2 reviewers were added to this PR based on Anton Bykov's automation.

[claude]

Claude finished @kristina-solovyova's task in 8m 7s —— View job

---

Review: UnsafeDisableDeepCopy in the planner inventory read

• Diff the new commit against the base
• Audit every consumer of the three unsafe Lists for mutation / retention
• Verify listAllWekaContainers matches the GetWekaContainersSimple it replaces
• Check the uncached CLI client tolerates the option
• Review the test changes
• Post review

---

Scope changed since my last pass

This is a different change from the one I reviewed at 09:49. The per-node client.MatchingFields fan-out is gone; chargeForeignPods is back to one cluster-wide List, and the PR is now about skipping deep copies on the three Lists the inventory pass issues. Both of my earlier comments were resolved by the rewrite — reviewing the new shape from scratch.

Heads up: the PR title and branch name now say the opposite of what the change does ("scope foreign-pod listing to candidate nodes" vs. a commit that deliberately keeps the listing cluster-wide). The commit message is correct; the title is what lands in the squash commit.

Verdict: the aliasing contract holds

UnsafeDisableDeepCopy hands back objects sharing memory with the informer store, so the whole change rests on "nothing in the read path writes." I traced every consumer rather than trusting the doc comments, and found no write.

One mechanical point worth stating, because it narrows the risk more than the comments suggest: apimeta.SetList copies each object into the Items slice by struct value, so list.Items[i] and any &list.Items[i] are already caller-owned. Only the transitive reference types — Labels, Annotations, Status.Allocatable, Spec.Containers, Spec.Resources, Status.Allocations — are shared. That is what "must not retain one past it" actually means here.

Against that, per List:

• Nodes (listNodesForSelector, 3 call sites) — resolveInventoryFDValue, nodeCPUTopology, ParseAllocatorNodeInfo, nodeHeadroom, nodeAllocatable{Hugepages,Memory}MiB, NodeIneligibleReason. All read-only. ParseAllocatorNodeInfo unmarshals annotation strings into fresh structs; discovery.ParseNodeInfo likewise. No *corev1.Node and no node-owned map or slice reaches a NodeCapacity or a NodeDetail — every field is a scalar or a freshly built slice.
• WekaContainers (listAllWekaContainers) — aggregateContainerResources, allocatedNodeDrives, consumerFrom, DriveContainerCapacities, spec2MiHugepages. All read-only; Consumer holds only strings and ints. allocatedNodeDrives iterates Status.Allocations.Drives (a shared slice behind a shared pointer) without touching it.
• Pods (chargeForeignPods) — effectivePodResourceRequests only. The resource.Quantity hazard is genuinely absent, and for the reason the comment gives: every quantity is reached through a ResourceList map, and ResourceList.Cpu()/.Memory() return &val on a local copy. Nothing calls String()/MarshalJSON() on a quantity reachable from the store, which is the one method that writes to its receiver.
• Uncached CLI path — client.New builds a typed client whose List lowers ListOptions through AsListOptions(); UnsafeDisableDeepCopy has no wire representation and is silently dropped, no error. The fake client ignores it too, so the tests are unaffected. Both claims in the doc comment check out.
• Collector's doc says every List it issues carries the option — grep '\.List(' in collect.go returns exactly three, all three carry it. True today, and cheap to keep true.

The one function in the unsafe read path I could not read is weka.GetTlcQlcCapacity (collect.go:1046), which receives c.Spec.DriveTypesRatio — a pointer shared with the store. It lives in the weka-k8s-api submodule, and pkg/weka-k8s-api/ is empty in this checkout. ratio.go describes it as a pure split and RatioFromCaps always allocates a fresh *DriveTypesRatio, so an in-place mutation would be surprising, but I'm flagging it as unverified rather than implying I checked it.

Findings

Two inline comments, both minor:

1. collect_test.go:1175 — TestChargeForeignPods_UnscheduledPodIgnored asserts on res.cores["n1"], but an unscheduled pod charges res.cores[""]. The test passes with the guard removed, and duplicates the pre-existing TestChargeForeignPods_SkipsUnscheduledAndTerminalPods, which has the same blind spot on its unscheduled half.
2. cmd/manager/main.go:239-240 — the new line contradicts the TODO above it, and the commented-out cache.Options block below is Pod-only, so the TODO is now dead rather than deferred.

On the residual risk

Worth naming plainly rather than burying: this invariant cannot fail a test. The fake client deep-copies unconditionally, so no unit test can observe an aliasing violation, and make test runs without -race (Makefile:161) — which wouldn't help anyway, since nothing else touches the store during a unit test. A future edit that sorts pod.Spec.Containers or writes a node label in one of these helpers would corrupt the shared cache for every controller in the process, and CI would be green.

That's an acceptable trade for the saving — nodes really do carry KB-scale discovery.json and drive annotations, and this runs per WekaCluster on every reconcile — and the mitigation already in place is the right one: all three Lists sit in a single file, and each carries a comment naming its consumers. Keeping it that way is the whole defence, so I'd resist any later move of these helpers into a shared package.

Other notes

• Folding listAllWekaContainers into listInventoryInputs is a real cleanup: FullDrivesInventory now derives allocatedDrives from the same container snapshot that fed consumedNodeResources, instead of a separately-fetched one. Same List count, but one consistent snapshot where there were two independent reads.
• listAllWekaContainers is behaviourally identical to the GetWekaContainersSimple(ctx, "", "", nil) it replaces — that call added only InNamespace(""), which is a no-op. Dropping the KubeService indirection also removes the last reason for inventory to import services/kubernetes.
• The two new inventory-level tests (TestInventory_SkipsFDSkippedNodes, TestInventory_ExcludesNodesOutsideSelectors) pin the useful negative: a foreign pod on a non-candidate node must not bleed into a candidate's headroom. Good regression value, and they survive the shape change from the previous revision.
• No security concerns.

I could not run go build or go test — the sandbox denies them, and pkg/weka-k8s-api/ is not checked out here, so the module wouldn't resolve regardless. CI is the only build signal on this.
· branch

[claude]

Claude finished @kristina-solovyova's task in 1m 53s —— View job

---

Follow-up review: force-pushed head (a9fd2ea)

• Diff the new head against my last pass
• Verify the test-assertion fix actually pins the guard
• Check the newInventoryTestClient dedup for compile/behaviour regressions
• Attempt a build/test run — still denied by the sandbox

---

What changed since my 15:14 pass

Only the test file. The UnsafeDisableDeepCopy change in collect.go and the main.go line are byte-identical to what I already reviewed, so the aliasing audit from that pass stands unchanged — I'm not repeating it.

Two deltas:

1. Finding #1 is fixed, and fixed the right way. TestChargeForeignPods_UnscheduledPodIgnored is gone and TestChargeForeignPods_SkipsUnscheduledAndTerminalPods (collect_test.go:619) now asserts on map length:

if len(res.cores) != 0 || len(res.hugepages) != 0 || len(res.memory) != 0 {

This is now sensitive to both guards, which the old res.cores["n1"] form was not:

• Delete the pod.Spec.NodeName == "" guard → pending-pod charges res.cores[""] += 4. Map assignment creates the key regardless of value, so len goes to 1 and the test fails. Previously res.cores["n1"] stayed 0 and it passed.
• Delete the terminal guard → job-done/job-failed charge res.cores["n1"], len goes to 1, fails. Covered before too.

One test covering both halves, no duplicate, and the failure message prints the actual maps — better diagnostics than the old three-scalar form. Preferring the consolidation over keeping both tests was the right call.

2. newFullDrivesTestClient now delegates to newInventoryTestClient (collect_test.go:313). Behaviourally identical — same two schemes, same WithObjects, just []*corev1.Node/[]*weka.WekaContainer flattened into []client.Object first. runtime and fake are each still referenced exactly once, inside newInventoryTestClient, so no unused-import breakage from removing the inline scheme block.

Remaining open item

cmd/manager/main.go:239-240 — the TODO still points the opposite way from the line added under it, and the commented-out cache.Options block below is Pod-only, so the TODO's precondition can never make it correct. Minor, and reasonable to leave; flagging only so it's a decision rather than an oversight.

The PR title now matches what the commit does — my earlier heads-up on that is resolved. The branch name still reads scope_foreign-pod_listing_to_candidate_nodes, which is cosmetic since the squash uses the title.

Verdict

No new findings. The one substantive issue from my last pass is properly closed, and the change is otherwise as I reviewed it: the unsafe-read contract holds across all three Lists, with the caveat I named before — it cannot fail a test (the fake client always deep-copies, make test has no -race), so the single-file locality of the three Lists plus their consumer-naming comments is the entire defence. Worth preserving as such.

I still could not run go vet/go test — the sandbox denies them, and pkg/weka-k8s-api/ isn't checked out here so the module wouldn't resolve anyway. CI remains the only build signal.
· branch