Skip to content

feat(installer): both installers can read the VM ceiling (backend#2221) - #804

Merged
shujaatTracebloc merged 2 commits into
developfrom
feature/2221-installers-read-vm-ceiling
Aug 24, 2026
Merged

feat(installer): both installers can read the VM ceiling (backend#2221)#804
shujaatTracebloc merged 2 commits into
developfrom
feature/2221-installers-read-vm-ceiling

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

⚠️ Blocked — do not merge yet

scripts/.client-runtime-ref pins the feature-branch commit of tracebloc/client-runtime#363. That PR squash-merges, which mints a new SHA and orphans this one, so the drift gate would stop resolving the ref. The file carries a DO-NOT-MERGE block with the re-pin recipe; it comes out once #363 lands.

Everything else is finished and green.

What

_honest_topology (bash) and Get-HonestTopology (PowerShell): the VM's real size in, an honest node count and per-node memory cap out. Plus contract v2 and its new embedded constants.

Called by nobody. The arithmetic lands first, dark and tested, so the flip that changes the default cluster shape stays a separate revertable PR — the dark → wire → flip shape that worked on backend#2220. That flip is also the part awaiting @LukasWodka's product call, so keeping it out of here means this can be reviewed on its merits now.

Why

Uncapped k3d node containers (NanoCpus=0 CpuQuota=0 Memory=0) each report the whole Docker VM. The default topology already hits this — common.sh:821-822 ships SERVERS=1 AGENTS=1. Measured on k3d v5.9.0 / k3s v1.35.5 / Docker 29.5.2:

LayerCPUMemory
Docker VM107.75 GiB (MemTotal 8321712128)
each node container108126672Ki — ×1024 = 8321712128, byte-identical
Kubernetes believes2015.50 GiB (2.000×)

And it is reachable through this file: on that cluster _machine_training_resources returned cpu=9,memory=4Gi, and two pods at that envelope both went Running18 CPU / 8 GiB admitted on a 10 CPU / 7.75 GiB machine.

The two asymmetries, both measured

Memory is cappable — at create time only. k3d caps a node by bind-mounting a synthetic /proc/meminfo into the container (a fakeowner mount), not via the cgroup; kubelet never reads the cgroup for capacity. docker update --memory=4g on a running node set memory.max=4294967296 and left /proc/meminfo at the VM's 8126672 kB, so capacity did not budge even across a restart. An existing cluster cannot be capped in place.

CPU is not cappable at all. k3d 5.9.0 has no CPU flag, and neither mechanism reaches kubelet:

mechanisminside the containercapacity.cpu
--cpus=2 (CFS quota)cpu.max 200000 10000010
--cpuset-cpus=0-4nproc → 510

cadvisor counts /sys/devices/system/cpu/present and /proc/cpuinfo; no cgroup namespaces either. Verified against a forced node de-register/re-register, so not a stale node object.

So memory is made honest by capping and CPU only by fewer node containers — which is why the return value carries cpu_honest rather than implying the cap closed it. Same conclusion the GPU path already reached for --gpus=all.

Contract v2 and the derivation guard

New embedded constants: VM_RESERVE_MEM_BYTES (1 GiB — measured: serverlb 39.27 MiB + tools 14.69 MiB on a live cluster, plus dockerd/containerd and guest page cache outside any container) and NODE_MIN_{CPU_MILLI,MEM_BYTES}.

The node minimum is overhead + floor = 5 GiB / 2 cores — derived, but recorded in the contract because neither installer can do arithmetic on JSON. A recorded derivation nobody checks is just a fourth constant waiting to rot, so gen-envelope-embed.sh now re-checks the derivation and refuses before writing anything. That matters specifically here: an embed generated from a half-vendored contract would be wrong in two installers at once, and a generator that rewrites one file then dies is exactly Bugbot#766.

The guard immediately caught its own blast radius. The existing "adopting a changed contract" test bumps overhead and left per_node_minimum behind — the generator refused and the test went red. That refusal is correct: upstream's generator recomputes the derived value, so a vendored contract always arrives consistent. The test now simulates a properly regenerated upstream contract, and the half-vendored case became a test of its own, which also asserts neither installer was touched on the refusal.

Also: a comment I got wrong in #2220, in both twins

"Installer-provisioned clusters are single-node k3d, where the orders cannot differ, so this is a no-op in the field."

They are not — the default is two nodes. The anchor tie-break is a field no-op only because both k3d node containers report identical figures, and they do that because each reports the whole VM, which is this bug. Corrected in install-client-helm.sh and install-k8s.ps1, with the reason.

Tests

11 topology vectors generated from the contract and replayed through both twins — 5 bats + 7 Pester cases. Both return the same string, so the twins are compared byte-for-byte off the same rows rather than through two shapes. Covered:

  • every vector, including the stock-macOS case that cannot host the shipped 2-node default
  • sum(nodes × cap) ≤ usable VM as a property over 12 VM sizes × 5 requested counts — the invariant the ticket exists to establish
  • unreadable VM → nothing / $null, distinguishable from requested < 1 → non-zero exit / $null
  • per_node_minimum is overhead + floor, asserted in-suite as well as in the generator

I verified out of band that both languages produce byte-identical output on all 11 rows before wiring the suites up.

installer_parity.json is deliberately unchanged. This function is pure arithmetic — no docker, no kubectl, no branching on cluster state — so the contract vectors are its parity mechanism. That fixture exists for control flow: every backend#2220 divergence lived in a state that was not a clean measurement. The eventual caller belongs there, because it will shell out to docker and branch. Reasoning is recorded in both suites so a reviewer can disagree with it in the right place.

Local gates

  • 461 bats and 887 Pester pass. assess.bats:40 fails on clean develop too — verified by stashing, not assumed.
  • shellcheck at severity=error, plus -S warning -x on both files I touched: clean.
  • PSScriptAnalyzer -Severity Error: identical to develop (8 pre-existing ConvertTo-SecureString findings in the Tests file; install-k8s.ps1 itself is 0).
  • gen-envelope-embed.sh --check, gen-installer-parity.sh --check, gen-manifest.sh --check: all clean. Manifest regenerated, never merge-resolved.
  • helm lint fails identically on clean develop (bare values.yaml has no clientId/clientPassword). No chart files touched, so no Chart.yaml version bump.

One incidental fix worth knowing about

Adding a comment containing an apostrophe to the heredoc inside gen-envelope-embed.sh's < <(python3 … <<'PY') broke the whole script with bad substitution: no closing ')' — bash tracks quotes while scanning for a process substitution's closing paren, so one ' in a comment ate it. The heredoc now lives in a plain function body, which is parsed normally. Noted in the script so the next person doesn't rediscover it.

Still to come on backend#2221

The flip (_create_new_cluster's K3D_ARGS and $k3dArgs), the macOS minimum-VM guidance mirroring .wslconfig, and bounding the ladder by the VM ceiling. Audit of the ticket's own claims — two corrections that changed the fix — is on backend#2221.

🤖 Generated with Claude Code


Note

Medium Risk
Touches installer resource/topology arithmetic and the shared envelope contract, but the new functions are uncalled so install behavior does not change yet.

Overview
Adds dark (uncalled) topology arithmetic so both installers can answer “how many k3d nodes may this Docker VM honestly host, and at what memory cap?” Uncapped k3d nodes each report the whole VM, so the default two-node layout double-counts CPU and memory.

_honest_topology (bash) and Get-HonestTopology (PowerShell) take VM CPU/memory plus requested node count and return nodes=N,cap=BYTES,cpu_honest=0|1,viable=0|1 (or empty/$null if unreadable). Memory is reserved then floored across nodes; CPU honesty is only true for a single node. Envelope contract v2 embeds vm_reserve (1 GiB) and derived per_node_minimum (overhead + floor). gen-envelope-embed.sh refuses a stale derivation before writing either installer.

Golden topology vectors replay through bats and Pester, including a property that nodes × cap never exceeds usable VM. Cluster create is not wired yet. Also moves the Python heredoc out of a process substitution so apostrophes in comments cannot break the generator.

Reviewed by Cursor Bugbot for commit 1f38dca. Bugbot is set up for automated code reviews on this repo. Configure here.

Called by nobody. The topology arithmetic lands first, dark and tested, so the
flip that changes the default cluster shape stays a separate, revertable PR --
the shape that worked on backend#2220.
WHY. Uncapped k3d node containers (`NanoCpus=0 CpuQuota=0 Memory=0`) each report
the WHOLE Docker VM, so the DEFAULT topology -- common.sh:821-822 ships
SERVERS=1 AGENTS=1 -- tells Kubernetes the machine is twice its size. Measured
on k3d v5.9.0 / k3s v1.35.5 / Docker 29.5.2: a 7.75 GiB VM presented as
15.50 GiB, byte-exactly 2.000x. And it is reachable through THIS file: on that
cluster _machine_training_resources returned cpu=9,memory=4Gi, and two pods at
that envelope both went Running -- 18 cpu / 8 GiB admitted on 10 cpu / 7.75 GiB.
WHAT. `_honest_topology` (bash) and `Get-HonestTopology` (ps1): the VM's real
size in, an honest node count and per-node memory cap out. Both return the SAME
STRING so the twins are compared byte-for-byte by the same fixture rows, and
both treat an unreadable VM as NOTHING/$null -- "I cannot answer", which a
caller must not read as one node, because collapsing a cluster on a failed probe
is worse than leaving the topology alone.
Two asymmetries decide the shape, both measured, neither assumed:
MEMORY IS CAPPABLE, CREATE-TIME ONLY. k3d caps a node by bind-mounting a
SYNTHETIC /proc/meminfo into the container (a "fakeowner" mount), not via the
cgroup -- kubelet never reads the cgroup for capacity. `docker update
--memory=4g` on a running node set memory.max and left /proc/meminfo at the
VM's 8126672 kB, so capacity did not budge even across a restart. An existing
cluster cannot be capped in place.
CPU IS NOT CAPPABLE AT ALL. k3d 5.9.0 has no CPU flag, and neither `--cpus`
(CFS quota, cpu.max 200000 100000) nor `--cpuset-cpus` (nproc 5) moves
capacity.cpu off 10, because cadvisor counts /sys/devices/system/cpu/present
and /proc/cpuinfo and no cgroup namespaces either. Verified against a forced
node de-register/re-register.
So memory is made honest by capping and cpu only by FEWER NODE CONTAINERS, which
is why the return value carries cpu_honest rather than implying the cap closed
it. Same conclusion the GPU path already reached for --gpus=all.
CONTRACT v2. New embedded constants: VM_RESERVE_MEM_BYTES (1 GiB, measured --
serverlb 39.27 MiB + tools 14.69 MiB on a live cluster, plus dockerd and guest
page cache outside any container) and NODE_MIN_{CPU_MILLI,MEM_BYTES}.
The node minimum is overhead + floor = 5 GiB / 2 cores -- DERIVED, but recorded
in the contract because neither installer can do arithmetic on JSON. A recorded
derivation nobody checks is just a fourth constant waiting to rot, so
gen-envelope-embed.sh now re-checks the derivation and REFUSES a contract where
it has drifted, before writing anything. That matters here specifically: an
embed generated from a half-vendored contract would be wrong in two installers
at once, and a generator that rewrites one file then dies is Bugbot#766.
That guard also caught its own blast radius: "adopting a changed contract"
bumped overhead and left per_node_minimum behind, so the generator refused and
the test failed. Correct refusal -- upstream's generator recomputes the derived
value, so a vendored contract always arrives consistent. The test now simulates
that, and the half-vendored case became a test of its own.
Also fixes a comment I got wrong in backend#2220, in BOTH twins
(install-client-helm.sh, install-k8s.ps1): "installer-provisioned clusters are
single-node k3d, so this is a no-op in the field". They are not -- the default
is two nodes. The anchor tie-break is a field no-op only because both k3d nodes
report IDENTICAL figures, and they do that because each reports the whole VM,
which is this bug.
TESTS. 11 topology vectors generated from the contract, replayed through BOTH
twins (5 bats + 7 Pester cases), including the sum(nodes x cap) <= usable VM
invariant as a property over 12 VM sizes x 5 requested counts, and the unreadable
and requested<1 boundaries. Verified out of band that both languages produce
byte-identical output on all 11 rows.
installer_parity.json deliberately unchanged: this function is pure arithmetic
with no external commands and no branching on cluster state, so the contract
vectors are its parity mechanism. That fixture exists for CONTROL FLOW -- every
backend#2220 divergence lived in a state that was not a clean measurement -- and
the eventual CALLER belongs there, because it will shell out to docker and
branch. Reasoning recorded in both suites.
Local: 461 bats + 887 Pester pass (assess.bats:40 fails on clean develop too,
verified by stashing); shellcheck severity=error and -S warning -x clean;
PSScriptAnalyzer errors identical to develop; gen-envelope-embed/gen-manifest/
gen-installer-parity --check all clean. No chart files touched, so no version
bump.
NOT READY: scripts/.client-runtime-ref still pins the client-runtime FEATURE
BRANCH commit, which the squash-merge of #363 will orphan. The file carries a
DO-NOT-MERGE block with the re-pin recipe.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@shujaatTracebloc
shujaatTracebloc requested review from aptracebloc and removed request for saadqbalAugust 24, 2026 07:05
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 24, 2026 07:05
@shujaatTracebloc
shujaatTracebloc removed the request for review from aptraceblocAugust 24, 2026 07:10
@shujaatTracebloc
shujaatTracebloc marked this pull request as draft August 24, 2026 07:10
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

Putting this back to draft — it was flipped to ready-for-review without a comment, and it must not be mergeable yet.

scripts/.client-runtime-ref still pins the feature-branch commit of tracebloc/client-runtime#363. That PR squash-merges, which mints a new SHA and orphans this one, so merging as-is would land a ref on develop that the envelope-contract drift gate cannot resolve — breaking that gate for everyone, not just this PR.

Draft is the durable guard for that; the DO-NOT-MERGE block in the ref file is only a comment. Nothing else is outstanding — all 46 checks are green, including the full E2E cluster matrix.

Sequence to unblock: #363 merges → re-pin to the post-merge develop SHA → re-vendor the fixture → gen-envelope-embed.sh && gen-manifest.sh → drop the DO-NOT-MERGE block → mark ready. The recipe is in the ref file itself.

Reviews are very welcome in the meantime — the code is final.

@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 24, 2026 07:14
@shujaatTraceblocshujaatTracebloc changed the title feat(installer): both installers can read the VM ceiling (backend#2221)[BLOCKED on client-runtime#363] feat(installer): both installers can read the VM ceiling (backend#2221)Aug 24, 2026
…backend#2221)
client-runtime#363 merged as 48ccbac, so the temporary pin this PR shipped with
is retired: scripts/.client-runtime-ref now points at the post-merge develop sha
and the DO-NOT-MERGE block is gone. The fixture is re-vendored from that ref
(read out of git, not a working tree), and the embed + manifest regenerated from
it rather than hand-resolved.
Three conflicts, none of them mechanical:
1. install-client-helm.sh -- develop's #798 (backend#2237) RESTRUCTURED the
function my comment fix lived in, extracting the node ranking into a shared
_anchor_largest_schedulable so the cordon skip could not be added to one copy
and not the other. That structure is strictly better than what I branched
from, so it is taken wholesale and only the #2221 correction is re-applied on
top.
Worth noting: the claim that correction exists to remove -- "installer-
provisioned clusters are single-node k3d" -- was RE-INTRODUCED verbatim in
#798's new comment. It is wrong for the same reason as before (common.sh
defaults SERVERS=1 AGENTS=1, so the default topology is two nodes), and the
re-appearance is itself the argument for fixing the underlying bug rather
than the sentence: the belief keeps regenerating because the cluster keeps
looking single-node from the inside, which is exactly what #2221 is about.
The resolved comment now says why the tie-break is a field no-op (both k3d
nodes report identical figures because each reports the whole VM) instead of
claiming there is only one node.
2. install-client-helm.bats -- purely additive, both sides appended @tests at
the same seam. Kept BOTH: develop's four cordoned-node regressions and this
PR's five topology-contract tests, 10 in total with the derivation guard.
Git had left the trailing `}` outside the conflict markers, so splicing the
two blocks dropped the brace off develop's last test; caught by bats
reporting a setup_file syntax error, restored, and both blocks verified
green.
3. manifest.sha256 -- regenerated, never resolved by hand, then --check'd
against the resulting tree.
Also picked up: develop's gen-envelope-embed.sh change (the generator now emits
whole clusters instead of pre-filtering cordoned nodes) merged cleanly with the
topology-table emitter added here, and all three vector tables are still
produced.
Local: 766 bats pass with 2 failures that also fail on clean develop
(assess.bats:40, install-bootstrap.bats:365 -- verified in a develop worktree,
not assumed); 891 Pester pass; bats-hygiene clean; shellcheck severity=error and
-S warning -x clean; gen-envelope-embed / gen-installer-parity / gen-manifest
--check all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTraceblocshujaatTracebloc changed the title [BLOCKED on client-runtime#363] feat(installer): both installers can read the VM ceiling (backend#2221)feat(installer): both installers can read the VM ceiling (backend#2221)Aug 24, 2026
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

Unblocked — the temporary pin is retired

client-runtime#363 merged as 48ccbac, so 1f38dca does the re-pin this PR was waiting on:

  • scripts/.client-runtime-ref48ccbac839cdcdf85a1fa55b8c0321287fcb567e (develop, not a feature branch)
  • the DO-NOT-MERGE block is gone, and the [BLOCKED …] title prefix with it
  • the fixture is re-vendored from that git ref rather than a working tree, and the embed + manifest regenerated from it — gen-manifest.sh never hand-resolved

It also merges develop, which had three conflicts, none mechanical:

1. install-client-helm.sh — develop restructured the function my comment fix lived in.#798 (backend#2237) extracted the node ranking into a shared _anchor_largest_schedulable so the cordon skip could not be added to one copy and not the other. That is strictly better than what I branched from, so I took it wholesale and re-applied only the #2221 correction on top.

Worth flagging: the claim that correction exists to remove — "installer-provisioned clusters are single-node k3d" — was re-introduced verbatim in #798's new comment. It is wrong for the same reason (common.sh defaults SERVERS=1 AGENTS=1, so the default topology is two nodes), and the re-appearance is itself the argument for fixing the underlying bug rather than the sentence: the belief keeps regenerating because the cluster keeps looking single-node from the inside, which is what this ticket is about. The resolved comment now explains why the tie-break is a field no-op — both k3d nodes report identical figures because each reports the whole VM — instead of claiming there is only one node.

2. install-client-helm.bats — additive, kept both sides. develop's four cordoned-node regressions plus this PR's five topology-contract tests (10 with the derivation guard). Git had left the trailing }outside the conflict markers, so splicing the two blocks dropped the brace off develop's last test — bats caught it as a setup_file syntax error rather than a silent skip, restored, both blocks green.

3. manifest.sha256 — regenerated, then --checked against the resulting tree.

develop's gen-envelope-embed.sh change (the generator now emits whole clusters instead of pre-filtering cordoned nodes) merged cleanly with the topology-table emitter added here; all three vector tables still produced.

Local gates: 766 bats pass; 891 Pester pass; bats-hygiene clean; shellcheck at severity=error and -S warning -x clean; all three generator --checks clean. Two bats failures — assess.bats:40 and install-bootstrap.bats:365 — also fail on clean develop, verified in a develop worktree rather than assumed.

Ready for review.

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the arithmetic on its merits (as invited) — both ports are faithful to client-runtime/node_sizing.py::honest_topology:

  • bash _honest_topology: clamp max(1, min(requested, fits)), floored cap = usable/nodes, cpu_honest=(nodes==1), viable=(fits>=1 && vm_cpu>=min_cpu) — all match. 64-bit bash arithmetic handles the ~8e9 byte values. Embedded constants (1GiB reserve, 2000m/5GiB minimum, contract v2) are consistent with the overhead+floor derivation.
  • PowerShell Get-HonestTopology: [long] throughout with [math]::Max([long]0, …) correctly avoids the Int32-overload trap from #2220; [math]::Floor over the byte magnitudes is exact (all ≪ 2^53). Same clamp/cap/viable logic; unit spellings (Ki/Mi/Gi binary, bare bytes, Nm) match the bash parsers.
  • The requested<1 handling differs by idiom (bash non-zero exit, PS $null, Python raises) but all mean "no topology," and it's a caller-bug path installers never hit — no field impact.

Holding approval on two counts, neither about the code: (1) CI is still pending; (2) the PR is self-blocked — scripts/.client-runtime-ref pins #363's feature-branch SHA, which the DO-NOT-MERGE block says to re-pin once #363 squash-merges. I approved #363 this pass, so the unblock is close. I'll re-check and approve once CI is green and the ref is re-pinned post-merge.

— drafted with Claude Code

@shujaatTracebloc
shujaatTracebloc requested review from aptracebloc and removed request for saadqbalAugust 24, 2026 07:51

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both holds cleared:

  • #363 merged (SHA 48ccbac) and scripts/.client-runtime-ref is re-pinned to exactly that squash-merge commit, with the DO-NOT-MERGE block removed — the drift gate resolves it and CI is fully green (44 checks, no pending).
  • Re-verified the arithmetic survived the rebase onto develop (which pulled in #798's cordoned-node refactor of the same files): _honest_topology is byte-for-byte what I reviewed — faithful to node_sizing.py::honest_topology, constants intact.

No conflict, no open threads. LGTM — approving.

— drafted with Claude Code

@shujaatTracebloc
shujaatTracebloc merged commit e645499 into developAug 24, 2026
48 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the feature/2221-installers-read-vm-ceiling branch August 24, 2026 08:04
aptracebloc added a commit that referenced this pull request Aug 26, 2026
…ent#836) (#850)
When TRACEBLOC_TRAINING_RESOURCES is unset, the VM-ceiling sizing
(backend#2221 / #804) derives e.g. cpu=9,memory=12Gi, and the L0.2 limits
half (backend#2418 / #820) drops cpu so RESOURCE_LIMITS ships memory-only
(memory=12Gi). A pre-backend#2223 chart schema pinned RESOURCE_LIMITS to
`^(cpu=\S+,memory=\S+)?$`, which rejects that value, aborting `helm install`.
backend#2223 (#812) already loosened the schema to admit any subset, so the
current chart accepts memory-only limits — the two changes are a coordinated
pair. Rather than revert the memory-only design, this adds regression tests
that keep the derivation and the schema pinned together:
- install-client-helm.bats: drives the real derivation
(_resolve_training_size then _training_limits) for the VM-ceiling repro and
asserts the derived RESOURCE_LIMITS matches the pattern READ FROM
client/values.schema.json — so a re-tightening back to the strict pattern
reddens here, not at a customer's helm step.
- chart-env-vocabulary.sh: renders the same memory=12Gi through the REAL
chart schema via `helm template`, the authoritative validator.
No production code changes.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@shujaatTracebloc@aptracebloc@LukasWodka