feat(installer): CPU is a share weight, so limits carry memory only (backend#2418)

[shujaatTracebloc]

What

Both installers now write RESOURCE_LIMITS with memory only — the two halves of the envelope are no longer the same string:

RESOURCE_LIMITS: "memory=29Gi"# was "cpu=7,memory=29Gi"RESOURCE_REQUESTS: "cpu=7,memory=29Gi"# unchanged

This is the producer half of backend#2418 (L0.2) — the half that reaches the fleet. #2418 delivered the same policy for the derive path (client-runtime#378), which is flagged off and runs nowhere; the installer writes RESOURCE_* unconditionally on every install, so this is the path that actually decides what a customer's training pod gets.

Why the two dimensions differ

They are not the same kind of resource:

• CPU is time-shared.requests with no limits becomes a cgroup cpu.weight — a share under contention, and the whole machine when nobody else wants it. With requests == limits it becomes a cpu.maxquota that throttles at its ceiling even on a completely idle box. On an 8-core machine, a run sized to 7 cores was capped at 7 while the 8th sat unused, benefiting nobody.
• Memory is not time-shared. There is no borrowing it back; over the limit is an OOM kill. So requests == limits stays, and it is the load-bearing safety property of the whole ladder.

Guaranteed QoS is given up deliberately — a pod is Guaranteed only when every container has limits for both dimensions. That is the trade: the memory guarantee is what mattered, and CPU burstability is what lets a second job exist at all.

⚠️ Ordering constraint — do not ship this ahead of the runtime

This requires a jobs-manager that treats RESOURCE_LIMITS as the complete limits envelope: client-runtime#388, merged to develop but not yet released.

An older image merges the parsed pairs onto its built-in cpu=2,memory=8Gi literal, so an omitted cpu comes back as a 2-core limit under a 7-core request — which Kubernetes rejects outright, and the pod never schedules. Measured, not inferred:

RESOURCE_REQUESTS = "cpu=7,memory=29Gi"
RESOURCE_LIMITS = "memory=29Gi"
→ requests = {'cpu': '7', 'memory': '29Gi'}
→ limits = {'cpu': '2', 'memory': '29Gi'} ← invalid

Same shape as the SafeTensors reader-before-writer ordering (avg#183 / DownloadView#1111): the runtime ships first, the fleet picks it up, then this. Holding this as a draft for that reason as much as for review.

A real twin divergence, found and fixed in development

The two installers are twins, and the bash side had a bug the PowerShell side did not:

case" cpu=7 "in cpu=*) # does NOT match

So an operator writing TRACEBLOC_TRAINING_RESOURCES="cpu=7, memory=29Gi" would have kept the CPU limit on Linux/macOS while .Trim() dropped it on Windows — silently, and in the dangerous direction. Fixed by trimming each pair before the match; both twins now agree on all nine inputs I checked, including whitespace, empty pairs and a cpu-only envelope.

That is the same "shared contract, two control flows" class backend#2220 found five of, which is why I did not stop at per-twin unit tests:

installer_parity.json gains a limits verdict field (schema_version 1 → 2), so both parity suites assert it per row against the same fixture. Leaving a new shared contract outside that fixture is exactly the pattern that produced those five bugs. The generator emits the new field and the bash table is regenerated, not hand-edited.

Decisions worth arguing with

Every non-cpu dimension survives, not "memory only". backend#2223 added ephemeral-storage; a hardcoded memory filter would silently drop a disk limit and let a pod fill the node's disk. cpu=7,memory=29Gi,ephemeral-storage=26Gi → memory=29Gi,ephemeral-storage=26Gi.

A cpu-only envelope returns the input unchanged, never empty. An empty RESOURCE_LIMITS reads to jobs-manager as unset, which since #388 mirrors the requests side back — resurrecting the very CPU limit this function exists to drop. $size is never empty on a reachable path (_training_resources' four-way fallback always yields one), and an empty input returns empty, which is the honest answer about an envelope that does not exist.

cpu= is matched as a prefix, so a future cpuset=0-3 survives — pinned by a test, because prefix matching is the kind of thing that quietly eats a neighbour.

Tests

+7 bats and +7 Pester on the helpers themselves — the same seven cases on both sides, including the whitespace case that was the divergence — plus the limits assertion inside both parity suites, plus two existing write-site assertions re-pointed (the installer no longer writes the same string twice, and one now also asserts nocpu appears in the limits value at all).

Mutations proven:

mutation	reddens
drop the trim from the bash twin	1 bats test (the divergence case)
keep cpu in the bash twin	4 bats + the bash parity suite

scripts/tests/install-client-helm.bats 203 ok, 0 not ok
scripts/tests/installer-parity.bats 4 ok
installer-parity.Tests.ps1 (Pester) 3 passed
install-k8s.Tests.ps1 Get-TrainingLimits 7 passed
bats-hygiene.bats 18 ok
shellcheck -S warning -x clean
PowerShell parse clean
chart-version-guard.sh "No packaged chart content changed — guard N/A"

No Chart.yaml bump: nothing under client/templates|values|schema changed, and the guard agrees. Bumping anyway would publish a chart version with no chart diff.

Not in this PR

client/templates/jobs-manager-deployment.yaml currently emits both env vars whenever either is set, defaulting the missing one to "cpu=2,memory=8Gi" — the chart-level twin of the bug #388 just fixed in the runtime, and it makes #388's mirroring unreachable for chart-direct installs. That is a separate change with the same ordering constraint, and it is a client/templates edit so it will need the Chart.yaml bump. Filed as the next PR rather than folded in here.

Refs: backend#2418, backend#664, client-runtime#388, backend#1236, backend#2220, backend#2223

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

🤖 Generated with Claude Code

---

Note

High Risk
Changes what every install writes for training pod resources and reinstall carry logic; shipping before client-runtime#388 can yield invalid Kubernetes requests/limits and pods that never schedule.

Overview
Implements Utilization Ladder L0.2 in both installers: RESOURCE_REQUESTS still carries the full cpu,memory envelope, while RESOURCE_LIMITS is derived by dropping every cpu=* pair (memory and other non-cpu dimensions stay). Bash _training_limits and PowerShell Get-TrainingLimits are the twins, with trim and case-insensitive cpu= matching so Linux/macOS and Windows agree.

Reinstall / carry-forward no longer reads memory-only limits as the carried size. Both install-client-helm.sh and install-k8s.ps1 prefer RESOURCE_REQUESTS, fall back to RESOURCE_LIMITS only when requests is missing, so reinstall keeps CPU requests and the historic cpu=2,memory=8Gi gate still works.

Parity contract bumps installer_parity.json to schema_version 2 with a per-row limits verdict; the generator and installer_parity.bash emit/assert it in both parity suites. Tests add helper coverage, carry-path cases, and install round-trip scenarios; manifest hashes update for the touched scripts.

^{Reviewed by Cursor Bugbot for commit 43c01e9. Bugbot is set up for automated code reviews on this repo. Configure here.}

[shujaatTracebloc]

bugbot run

[saadqbal]

The policy reasoning is right and the _training_limits tests are the good kind — the ephemeral-storage case in particular, since a hardcoded "memory only" filter would silently drop a disk limit. But Bugbot's High is correct, and I'd add that its second half is the worse of the two.

I traced it. _existing_training_values reads RESOURCE_LIMITS (:101) and that value becomes _TB_TRAINING_SIZE (:514), which line 1871 writes back as RESOURCE_REQUESTS. So a reinstall after this writer carries memory=29Gi into the requests half and the cpu request is gone — not just the limit. The pod drops out of any cpu share weight at all, silently.

The second half is nastier. The historic-default gate is "$prev" != "$_TRAINING_DEFAULT" against the literal cpu=2,memory=8Gi. Post-filter a default install writes memory=8Gi, which no longer equals the literal, so the gate reads it as a deliberate choice, keeps it, and machine sizing never runs again. That is precisely the "unschedulable 8Gi on exactly the machines this sizing exists to fix" the comment at :511 warns about, reintroduced by the filter that was added above it.

The single-lookup design at :504 is right — it's reading the wrong field now. Carry and the literal comparison both need the full envelope, so RESOURCE_REQUESTS is the field to read.

What I'd take from this: no test writes values, reads them back, and carries. _training_limits is well covered in isolation and the round-trip is what broke — worth a test that installs, re-reads, and asserts requests still has cpu, because that's the shape of the bug and it would have caught both halves.

Two smaller things while you're in here:

The ordering constraint on client-runtime#388 is documented in four places and enforced in none — every 388 in the diff is a comment. Merging to develop is fine, but nothing stops a promotion from shipping this ahead of the runtime, and the failure mode you describe (2-core limit under a 7-core request, pod never schedules) is fleet-wide and silent until someone trains. Worth deciding whether that's an installer preflight on the jobs-manager version or an explicit hold on the promotion — prose in a comment won't survive the release train.

Minor twin divergence: bash matches case "$pair" in cpu=*), which is case-sensitive, while PowerShell's -like 'cpu=*' is case-insensitive. CPU=7,memory=29Gi keeps the cpu limit on Linux/macOS and drops it on Windows. Same class as the trim divergence the fixture docstring records catching, and every fixture case is lowercase so nothing pins it.

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 14272c8. Configure here.}

[shujaatTracebloc]

All three points taken — one was already fixed when you reviewed, two were not, and both of those are now in 43c01e9.

1. The High — already fixed, in 14272c8

Your trace is exactly right, including that the second half is the worse one. It was pushed before your review landed (Bugbot re-reviewed 14272c8 clean), so nothing to redo — but recording the fix here so the thread isn't ambiguous:

Both readers now prefer RESOURCE_REQUESTS (still the whole envelope) and fall back to RESOURCE_LIMITS for a release installed before requests was written, or a chart-direct install that set only that key. install-client-helm.sh:101 and install-k8s.ps1's $vals.env.RESOURCE_LIMITS both changed — the divergence risk there is the same one installer_parity.json exists for.

2. The round-trip gap — you were right, and it caught a weak test of mine

no test writes values, reads them back, and carries

Correct, and that is precisely why both halves got through: every test wrote or read, never both, so a writer change that poisoned the reader passed everything. Two new tests do the whole loop — install, hand the generated values.yaml back as helm get values would, then carry:

• round trip: install, re-read, and the carried envelope still has cpu — asserts cpu=7,memory=29Gi survives. Reverting the reader reddens it with memory=29Gi.
• round trip: a default install re-reads as the literal, so it re-sizes — the second half. A default install writes RESOURCE_LIMITS: "memory=8Gi"; the test asserts the reader returns the fullcpu=2,memory=8Gi, so the gate at :546 still matches, and then that _TB_TRAINING_PROVENANCE is installer — i.e. machine sizing ran rather than the default being adopted as a human choice.

That second one also fixed a weak assertion I had written: my first version checked _existing_training_resources returned empty, which is the wrong function — the gate lives in _resolve_training_size, not the reader. It passed for the wrong reason until I traced :546 after reading your comment.

Reverting the reader now reddens 4 tests across the two round-trip and two unit cases.

3. The case-sensitivity divergence — real, fixed

Confirmed and fixed. bash used case "$pair" in cpu=*), case-sensitive; PowerShell's -like 'cpu=*' is not. So CPU=7,memory=29Gi kept the cpu limit on Linux/macOS and dropped it on Windows.

Fixed with character classes rather than ${pair,,}, because macOS ships bash 3.2 and has no case conversion:

case"$pair"in
[Cc][Pp][Uu]=*) continue ;;
esac

Cross-checked both twins on cpu=, CPU=, Cpu= and cpuset= — identical output on all four, and CPUSET=0-3 still survives so the prefix match doesn't eat a neighbour. +1 bats and +1 Pester; making bash case-sensitive again reddens the bats one.

You're right that no fixture row would have caught it — every installer_parity.json case is lowercase. I have not added an upper-case row: the fixture's rows are cluster states, and a mis-cased operator string is an input-shape question rather than a cluster one, so it belongs in the per-twin unit tests where it now is. Say the word if you'd rather it were pinned in the fixture too.

4. The ordering constraint — you're right that prose won't survive the release train, and I looked into what would

I checked rather than guessing, and the answer changes the shape of the ask. images.jobsManager.digest defaults to "", so jobs-manager resolves to repository:tag with imagePullPolicy=Always on the env-floating tag, and the image-refresh CronJob rolls that pod. The runtime is therefore always the latest published :<env> image, not whatever an edge installed with.

So there is no per-edge staleness for a preflight to detect, and an installer check would have nothing to read on a fresh install anyway. The real exposure is narrower and entirely release-ordering: a chart published before a runtime image containing #388 is published. Between those two publishes, every edge that rolls its jobs-manager pod gets the new template against an old runtime.

Two mechanisms actually enforce that, and both are yours or the release train's rather than mine to land unilaterally:

• Pin images.jobsManager.digest in the chart release that carries this — real coupling, and the field already exists. Costs a chart release per runtime release.
• A promotion-order hold in the release train: this chart version cannot promote to staging/main before the client-runtime release carrying Installer (Windows): still the legacy manual Client-ID/password connect — port RFC-0001 provisioning (login + client create) from the bash installer #388.

I'd take the second — it's the accurate model of the constraint and doesn't couple every chart release to a digest. I can file it against the release train with these specifics if you want it tracked rather than agreed in a thread; tell me which and I'll do it.

Local verification on 43c01e9: install-client-helm.bats 212 ok / 0 not ok, installer-parity.bats 4 ok, Get-TrainingLimits 8 Pester passed, carry-path 3 Pester passed, bats-hygiene clean, shellcheck -S warning -x clean, manifest regenerated.

[shujaatTracebloc]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 43c01e9. Configure here.}

[saadqbal]

All three addressed, and the carry fix is better than what I asked for.

Reading RESOURCE_REQUESTS with LIMITS as fallback is the right shape — a bare switch would have dropped the carry for a chart-direct install that set only the one key, and you kept that path working and said why. It also fixes the second half for free: prev is the full envelope again, so the historic-literal gate matches cpu=2,memory=8Gi and the machine gets re-sized instead of inheriting a post-filter memory=8Gi as a deliberate choice. The three new _existing_training_values tests cover exactly that, and naming the mutation in the first one ("read RESOURCE_LIMITS here and this reddens with 'memory=29Gi'") is what makes it worth having.

[Cc][Pp][Uu]=* over ${pair,,} for bash 3.2 is the detail I'd have missed — and pinning it from the PowerShell side with "the bash twin now agrees" is the right place for it, since that's the assertion that fails if either half drifts.

I read the cpu-only fallback again and I'm not going to push on it. Carrying the operator's stated value beats discarding it, both directions lose something, and you documented which one you picked.

Not blocking on this, but the client-runtime#388 ordering constraint is still prose-only here and in #821 — two PRs now, both correct, both safe to merge to develop and neither safe to promote until the runtime ships. Wherever that gets enforced, I don't think it should be a comment.

Holding my change request on one thing only: Prereqs — almalinux:8 is still running. Nothing left on my side — I'll clear it as soon as that lands.

[shujaatTracebloc]

On the ordering constraint you raised: filed as backend#2473 rather than left in a thread, since you were right that four prose comments are invisible to the release train.

It records the two mechanisms that would actually enforce it — pin images.jobsManager.digest in the chart release, or a promotion-order hold in the release train — with the finding that no installer preflight can help: digest defaults empty, so jobs-manager floats on the env tag with Always pull and is rolled by image-refresh, meaning an edge always runs the latest published :<env> image. There is no per-edge staleness to detect, and the exposure is exactly one window — a chart published before a runtime carrying #388.

I recommended the promotion hold and said why, but the decision isn't mine to land unilaterally, so the ticket holds it as pending rather than losing it when these merge.

It also notes this is a general shape, not a one-off: any future client change depending on client-runtime behaviour has the same exposure and there's currently no way to express it.

[saadqbal]

Prereqs green across all six distros — clearing my change request. Approving.

Everything I raised is in and verified: the carry reads RESOURCE_REQUESTS with a documented LIMITS fallback, which fixes the cpu-drop and restores the historic-literal match in one move; the round-trip is tested with the mutation named; and [Cc][Pp][Uu]=* closes the twin divergence without reaching for bash 4 syntax.

One thing that isn't mine to hold and I want on the record rather than in a comment thread: this and #821 are both correct and neither is safe to promote until client-runtime#388 ships. Two PRs now depend on a constraint that exists only in prose. Merging to develop is fine — I'd just want the promotion side to have something mechanical before this reaches an edge, because the failure mode is fleet-wide and silent until someone tries to train.