Skip to content

feat(doctor): measure disk, the dimension it never looked at (backend#2223) - #564

Merged
shujaatTracebloc merged 2 commits into
developfrom
feat/2223-doctor-disk
Aug 24, 2026
Merged

feat(doctor): measure disk, the dimension it never looked at (backend#2223)#564
shujaatTracebloc merged 2 commits into
developfrom
feat/2223-doctor-disk

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

backend#2223's CLI item: "The only disk check anywhere is an installer preflight. CLI doctor measures no disk at all."

That gap has already cost a misdiagnosis — backend#2053 was an ephemeral-storage eviction reported to the user as "CPU Overload", and three separate diagnoses went at the wrong system on the strength of it.

Two changes to checkNodeFit

1. ephemeral-storage joins cpu+memory as a whole-node fit condition when the envelope declares one. AND-ed into the same per-node verdict, never OR-ed across nodes — a pod gets every resource it requests from one node, which is the point Bugbot made about GPU on PR #91 and applies identically to a third dimension. There's a test for exactly that case (cpu/mem on one node, disk on another → fail, not ok).

2. The largest Ready node's ephemeral-storage is reported even when nothing requested it. This is the one that actually answers the ticket. Gating the report on a declaration would leave the common edge exactly as blind as before, since the installer writes no disk request — and "measures no disk at all" is the complaint.

Three deliberate choices

parseDisk is optional, shaped like parseGPU not parseCPUMem. The installer doesn't write a disk request and most edges won't carry one, so its absence must leave node-fit behaving exactly as before — not become a can't-read Warn the way a missing cpu/memory does. A test pins that the detail gains no dangling clause when there's no disk anywhere.

A node that doesn't report ephemeral-storage is not failed on it. Turning an unreadable field into "no node fits" would tell a user to resize a machine on the strength of a value we couldn't read. Same fail-open direction the rest of doctor takes.

bestDisk is tracked separately from the CPU-major "best node" used by the resize nudge, so reporting disk can't perturb that tie-break — it has to keep matching resources.nodeLarger, per an earlier Bugbot finding on this file.

A limit worth stating rather than hiding

Recorded at parseDisk, because it isn't obvious: this reads the jobs-manager Deployment env, i.e. the declared envelope. Since client-runtime#380, jobs-manager also applies a built-in ephemeral-storage default that never appears in that env — so an undeclared disk request is unknown here, not zero.

That's precisely why change (2) exists: since doctor can't see the effective default, the least it can do is show the machine's actual capacity. Closing that properly would mean jobs-manager publishing its effective envelope somewhere doctor can read, which is a bigger change than this ticket asks for.

Evidence

CheckResult
go test ./internal/doctor/ -run "Disk|ParseDisk|NodeFit"all pass — including the 7 pre-existing TestCheckNodeFit subtests unchanged
go test ./...pass
go build ./..., go vet ./internal/..., gofmt -lclean
zz-all-strings.goldenregenerated — exactly the 2 new copy strings, diff reviewed

Pairs with tracebloc/client#812 (chart grammar + the SECURITY.md tmpfs correction). The remaining #2223 item is the backend disk-eviction failure class, which the ticket says should be owned by epic:e2e-robustness rather than folded in here.

🤖 Generated with Claude Code


Note

Medium Risk
Changes scheduling diagnosis in checkNodeFit: a declared disk request can now fail node capacity even when CPU/memory fit. Diagnostic-only (no cluster mutation), with fail-open when disk is undeclared or unreadable.

Overview
tracebloc doctor now measures ephemeral-storage in node capacity, so disk-starved nodes are no longer reported as a healthy CPU/memory fit (the gap behind backend#2053 misdiagnoses).

When RESOURCE_REQUESTS includes ephemeral-storage, it is AND-ed into the same per-node verdict as CPU and memory (never OR-ed across nodes). Absence of a disk request stays optional like GPU, so existing installs without a declared envelope behave as before. Nodes that do not report the field are not failed on it.

OK details always surface the largest Ready node's allocatable disk when any node reports it, even if nothing requested disk — because jobs-manager's built-in default is not visible in the Deployment env. Disk tracking is kept separate from the CPU-major resize nudge so that ceiling still matches resources set max. Bumps to 0.10.12.

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

…#2223)
backend#2223: "The only disk check anywhere is an installer preflight. CLI
doctor measures no disk at all." That gap has already cost a misdiagnosis --
backend#2053 was an ephemeral-storage eviction reported to the user as "CPU
Overload", and three separate diagnoses went at the wrong system on it.
Two changes to checkNodeFit, and the second is the one that answers the ticket:
1. ephemeral-storage joins cpu+memory as a WHOLE-NODE fit condition when the
envelope declares one. AND-ed into the same per-node verdict, never OR-ed
across nodes -- a pod gets every resource it requests from ONE node, which is
the point Bugbot made about GPU on PR #91 and applies identically to a third
dimension.
2. The largest Ready node ephemeral-storage is reported EVEN WHEN NOTHING
REQUESTED IT. Visibility must not depend on someone having declared a
request; "doctor measures no disk at all" is the actual complaint, and gating
the report on a declaration would leave the common edge exactly as blind.
parseDisk is OPTIONAL, shaped like parseGPU rather than parseCPUMem. The
installer does not write a disk request and most edges will not carry one, so
its absence must leave node-fit behaving exactly as before -- not become a
can\x27t-read Warn the way a missing cpu/memory does.
THE LIMIT OF WHAT doctor CAN SEE, recorded at parseDisk because it is not
obvious: this reads the jobs-manager Deployment env, i.e. the DECLARED envelope.
Since client-runtime#380 jobs-manager also applies a built-in ephemeral-storage
default that never appears in that env, so an undeclared disk request is UNKNOWN
here, not zero. That is precisely why the node capacity is surfaced regardless.
A node that does not report ephemeral-storage is NOT failed on it. Turning an
unreadable field into "no node fits" would tell a user to resize a machine on
the strength of a value we could not read -- the fail-open direction the rest of
doctor already takes.
bestDisk is tracked separately from the CPU-major "best node" used by the
resize nudge, so reporting disk cannot perturb that tie-break (which must keep
matching resources.nodeLarger, per an earlier Bugbot finding).
Evidence:
go test ./internal/doctor/ -run "Disk|ParseDisk|NodeFit" all pass, incl. the
7 pre-existing TestCheckNodeFit subtests unchanged
go test ./... pass
go build ./... / go vet ./internal/... / gofmt -l clean
zz-all-strings.golden regenerated: exactly the 2 new copy strings
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The version-bump-gate failed this PR: VERSION still read 0.10.11, v0.10.11 is
ALREADY RELEASED, and this PR changes a published file
(internal/cli/testdata/golden/zz-all-strings.golden, matching internal/*).
The gate message explains why it is a hard failure rather than a nag, and it is
worth repeating: the release train READS this file to cut the tag and never
bumps for anyone, so a stale VERSION does not fail here -- it fails the next
prod hop, days later, on somebody else (backend#1561).
Patch bump, matching the repo own cadence: 0.10.8 -> 0.10.11 were all patch
steps within 0.10.x, including feature PRs, so a doctor capability lands as
0.10.12 rather than a minor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

Pushed 951b9a6 — the version-bump-gate failure was mine and is fixed.

VERSION still read 0.10.11, v0.10.11 is already released, and this PR changes a published file (internal/cli/testdata/golden/zz-all-strings.golden, matching internal/*). Bumped to 0.10.12 — patch, matching the repo's own cadence: 0.10.8 → 0.10.11 were all patch steps within 0.10.x including feature PRs, so a doctor capability lands as a patch rather than a minor.

Worth repeating the gate's own reasoning, because it explains why it is a hard failure rather than a nag: the release train readsVERSION to cut the tag and never bumps for anyone, so a stale value doesn't fail here — it fails the next prod hop, days later, on somebody else (backend#1561).

One note on this PR's state: it is still a draft, so Cursor Bugbot will skip it and it can't be reviewed or merged as-is. I've left it that way deliberately — the standing convention is that I only mark a PR ready when asked. It needs one word from @shujaatTracebloc to move.

@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 24, 2026 12:11

@saqlainsyed007saqlainsyed007 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.

Approving. Reviewed at high effort last pass; CI now green, no open threads.

The core is correct: ephemeral-storage is AND-ed into the per-node fit in checkNodeFit (one node must satisfy cpu+mem+disk — the same principle as the GPU per-node check), parseDisk is optional like parseGPU (its absence leaves node-fit exactly as before, not a can't-read Warn), a node that doesn't report ephemeral-storage is not failed on it (fail-open, matching the rest of doctor), and bestDisk is tracked separately so reporting disk can't perturb the CPU-major resize tie-break. Reporting the largest node's disk even when unrequested is the change that actually answers the ticket, since the installer writes no disk request. The stated limit (doctor reads the declared envelope, not jobs-manager's built-in default from client-runtime#380) is honestly recorded at the call site. The VERSION line is redundant post-#562 but harmless — it converges to the same 0.10.12 (mergeable, no conflict).

Green, mergeable, no threads.

@shujaatTracebloc
shujaatTracebloc merged commit 089b6e9 into developAug 24, 2026
39 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the feat/2223-doctor-disk branch August 24, 2026 12:22
LukasWodka added a commit that referenced this pull request Aug 25, 2026
Declares `0.10.13` as the next release version so the release train can cut an
rc for it.
`v0.10.12` is already tagged -- both `v0.10.12` and `v0.10.12-rc.1` exist -- and
the delta waiting on `develop` touches `internal/doctor/doctor.go`, under the
`internal/*` publish_path in release-train's `repos.yml`:
- #563 fix(release): retry the cosign call, bounded and transient-only
- #564 feat(doctor): measure disk, the dimension it never looked at
With `VERSION` still reading `0.10.12`, the staging hop's version preflight
refuses:
rc NOT tagged - v0.10.12 already exists and the delta DOES touch published
files (bump VERSION on develop before the next release)
Measured on the 2026-08-25 staging hop: the `cli` leg failed this preflight, so
`cli` sat the hop out. Same shape as the 0.10.12 bump (#562) one day earlier --
each hop that ships published files needs its own bump, and the previous one is
consumed by the hop that shipped it.
Patch-level, matching the whole 0.10.x series.
Co-authored-by: Claude Opus 5 <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@saqlainsyed007@LukasWodka