fix: tlc drive core capping and address linting issues

[kristina-solovyova]

No description provided.

[github-actions]

Dependency Review

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

Scanned Files

None

[claude]

Claude Code Review

• Fetch and read PR diff
• Verify UsesAutoFullDrives() nil-receiver behavior (removed nil guard)
• Verify TLC/QLC core split invariant in cores.go
• Check test helpers compile / run package tests
• Post review

View job run

[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: replace auto-full-drives projection validators with planner-backed feasibility check #2801
• fix: tlc drive core capping and address linting issues #2792 👈 (View in Graphite)
• 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
• 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 • (09/03/26)

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

[claude]

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

---

Review: tlc drive core capping + lint fixes

• Fetch PR diff vs origin/09-02-fix_upgrade_weka-sign-tool_default_image
• Read affected code (cores.go, cluster_min_drives_feasibility.go, cluster_drive_compute_core_ratio.go, inventory/planner loops)
• Verify behaviour claims (nil-receiver UsesAutoFullDrives, TLC/QLC split invariants, shipped ratio defaults)
• Post inline comments
• Run go build / tests — blocked, see note below

---

Verdict

The core fix is correct, and the reasoning in the new comments checks out against the code. No blocking issues. Two inline notes posted, both non-blocking.

The TLC cap (internal/capacityplanner/cores.go:117-123)

I worked the split algebra by hand for every branch:

case	tlc side	qlc side	sum
tlcDerived <= assigned	tlcDerived	assigned - tlcDerived	assigned ✓
tlcDerived > assigned	assigned (capped)	max(assigned-tlcDerived,0) = 0	assigned ✓
qlcGiB <= 0, assigned > 0	assigned	0	assigned ✓
tlcGiB <= 0, assigned > 0	0	assigned	assigned ✓
assigned == 0	TlcDriveCores	QlcDriveCores	capacity-derived (intended fallback) ✓

tlc + qlc == assignedCores now holds in every assigned branch. Without the cap the mixed case could exceed it, and since totalTlcDriveCores feeds both the total hard floor and the ratioed term of RequiredComputeCores (cores.go:60-72), the over-count propagated straight into over-stated compute. Real bug, real fix.

I also confirmed the cap can only bind for existing un-grown containers: both the grow path (planner.go:600) and the new-container path (planner.go:617) run pinCores, whose derived = RequiredDriveCores(tlc, qlc, cons) is ceil(tlc/tlcPer) + ceil(qlc/qlcPer) — always ≥ the TLC share alone. So the premise in the new comment holds, modulo the wording nit in my inline note.

Test coverage

cores_test.go is well-targeted and the arithmetic in every case is right (testCons() is TlcCapacityPerCoreGiB = 5*tib = 5120, QlcCapacityPerCoreGiB = 50*tib = 51200, so 10240 → 2 and 51200 → 1 as the comments claim). The tlc+gotQlc == assigned assertion is the right invariant to pin.

Suggestion (optional): the test's own header comment explains that an over-count "reaches totalTlcDriveCores and from there both terms of RequiredComputeCores" — but nothing tests at that level. A single case feeding a mixed ExistingContainer with stale NumCores through totalTlcDriveCores → RequiredComputeCores would lock in the behaviour the fix actually exists to protect, and would catch a future regression in finalCores/finalPoolCap that the leaf-level tests can't see. Fix this →

Validation changes

• cluster_min_drives_feasibility.go — dead-branch removal is correct; derivedSizingMode (cluster_sizing_mode_flip.go:94-99) documents the same nil-receiver contract. See inline note on the resulting inconsistency with cluster_signed_drives.go:32-36.
• cluster_drive_compute_core_ratio.go — the new "advisory is inert for drive-sharing" comment is verified accurate. With ComputeToTlcDriveCoreRatio = 1.0 (internal/config/env.go:537), required == driveSide, and line 45 has already returned for computeSide < driveSide, so the advisory can never fire in drive-sharing mode. Documenting this rather than silently leaving a validator that never triggers is the right call, and the paired test-comment update explains why the test pins 2.0.

Lint fixes

All mechanically sound, no behaviour change:

• plan.go, collect.go, planner.go, mergeRoleNodes — index-based ranging over []NodeCapacity to dodge rangeValCopy. Checked for aliasing bugs: mergeRoleNodes builds inventory from append([]NodeCapacity(nil), driveInv...) (fresh backing array) and appends *nc by value, so the pointer into computeInv never escapes. Same for planner.go:521 — nc: *nc copies into the nodeState value field.
• drainingComputeClause — in.remaining is map[string]NodeCapacity (value type), so the rewrite genuinely avoids the struct copy. It does cost a second map hash per iteration; irrelevant at this scale.
• pkg/util/hashes.go — reflect.Ptr → reflect.Pointer. Confirmed no reflect.Ptr occurrences remain anywhere under pkg/, internal/, cmd/.

Not verified

I could not run go build ./... or go test ./internal/capacityplanner/... ./internal/validation/... — both were denied by the tool permission policy in this environment. Everything above is static analysis of the diff and the surrounding code. Adding Bash(go build:*) and Bash(go test:*) to --allowedTools would let me confirm the new tests actually pass.
• 09-03-fix_tlc_drive_core_capping_and_address_linting_issues

[claude]

The cap itself is right — it restores tlc + qlc == assignedCores for mixed containers, which is what totalTlcDriveCores/totalQlcDriveCores feed into both terms of RequiredComputeCores. Two notes:

1. The comment is slightly narrower than reality. Grown containers also go through pinCores (planner.go:600), so the precise statement is "only an existing un-grown container can need the cap" — a grown one is re-pinned at or above RequiredDriveCores(newTlc, newQlc), which already includes the TLC share. Suggest tightening the wording so a reader doesn't go looking for a growth-path hole that isn't there.

2. When the cap binds, QLC gets attributed zero cores. qlcDriveCoresForContainer computes max(assignedCores-tlc, 0) against the uncapped tlc, so for assigned < tlcDerived the split is (assigned, 0) — the QLC pool contributes nothing to compute sizing despite holding capacity. That's harmless at shipped defaults (ComputeToQlcDriveCoreRatio = 0.0, internal/config/env.go:538), and it's the only split that preserves the sum invariant, so I think it's the right call. But if an operator ever sets CAPACITY_COMPUTE_TO_QLC_DRIVE_CORE_RATIO above the TLC ratio, this under-states required compute rather than over-stating it. Worth one line in the comment recording that the whole shortfall is charged to TLC deliberately.

[claude]

Correct — derivedSizingMode (cluster_sizing_mode_flip.go:94-99) documents the same nil-receiver invariant, so line 46's cluster.Spec.Dynamic.DriveContainers is only reached with a non-nil Dynamic, and the removed branch was genuinely dead.

Two things worth being aware of rather than changing:

• The safety now rests entirely on UsesAutoFullDrives()'s nil-receiver contract, which lives in the external weka-k8s-api module. cluster_signed_drives.go:32-36 keeps an explicit Dynamic == nil guard with a comment saying the branch is redundant but "keeps the field reads below obviously safe" — so the codebase is now inconsistent on this. Either is defensible; picking one and applying it to both would be nicer than having each file argue the opposite case in a comment.
• validateMinDrivesAutoFullDrives still needs its own Dynamic != nil guard at line 86, and correctly keeps it. Good.

[claude]

Claude finished @kristina-solovyova's task in 52s —— View job

---

Review: no changes since last review — skipping

• Check whether the PR content changed since the previous review
• Compare post-image blob SHAs for all 11 changed files
• Exit early (no re-review needed)

---

This synchronize event was a rebase with no content change — the commit date moved to 2026-09-07 but the tree is byte-identical to what I reviewed on 2026-09-03.

All 11 post-image blob SHAs match the previously reviewed set exactly:

file	blob
cmd/weka-capacity/plan.go	6ecfe68b
internal/capacityplanner/autofulldrives_compute.go	980bf675
internal/capacityplanner/cores.go	24cfbfc2
internal/capacityplanner/cores_test.go	9ba94f71
internal/capacityplanner/inventory/collect.go	4e89c42d
internal/capacityplanner/planner.go	eda6ec96
internal/controllers/wekacluster/steps_cluster_creation.go	166f0202
internal/validation/cluster_drive_compute_core_ratio.go	8a5ae1b0
internal/validation/cluster_drive_compute_core_ratio_test.go	061b39c9
internal/validation/cluster_min_drives_feasibility.go	4b881fd7
pkg/util/hashes.go	79fe9864

Diffstat is likewise unchanged at +117/−38.

My earlier review and its two inline comments (on cores.go:123 and cluster_min_drives_feasibility.go:44) still stand as written — verdict was no blocking issues, both notes non-blocking.

_{Not run: go build ./... / go test ./... — still blocked by the tool permission policy in this environment, same as last time. Adding Bash(go build:*) and Bash(go test:*) to --allowedTools would let me verify the new tests pass.}

• 09-03-fix_tlc_drive_core_capping_and_address_linting_issues •