Skip to content

feat(#90): cluster doctor — node-fit + image-pull checks (WS3 follow-up) - #91

Merged
saadqbal merged 2 commits into
developfrom
feat/doctor-node-image-checks
Jun 19, 2026
Merged

feat(#90): cluster doctor — node-fit + image-pull checks (WS3 follow-up)#91
saadqbal merged 2 commits into
developfrom
feat/doctor-node-image-checks

Conversation

@saadqbal

@saadqbalsaadqbal commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

What

Adds two read-only checks to tracebloc cluster doctor (follow-up to #89). Closes part of #90.

CheckWhat it catches
Node capacityA training job can't fit any node → it sits Pending forever
Image pull secretA missing/malformed registry secret → private images ImagePullBackOff

How

  • Node capacity — parses the resource requests the jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS like cpu=2,memory=8Gi, GPU_REQUESTS like nvidia.com/gpu=1) from its env, and checks at least one Ready node's Allocatable can fit them. GPU is soft: a hard ✖ only when no node satisfies cpu/mem; a ⚠ when a GPU is requested but no node exposes it (jobs-manager has a GPU→CPU fallback, so that's not fatal). Missing/unreadable RESOURCE_REQUESTS or nodes → ⚠ (skip), never a false ✖.
  • Image pull secret — reads the jobs-manager's imagePullSecrets; if it references one, verifies the secret exists, is kubernetes.io/dockerconfigjson, and its .dockerconfigjson is non-empty valid JSON. No pull secret in use → ✔ (public/digest-pinned). (Bad-but-well-formed credentials can't be detected without an actual pull — noted; this catches the common missing/empty/malformed misconfig.)

Both fit doctor's existing read-only, best-effort, injectable-clientset design.

Out of scope (tracked on #90)

The in-cluster egress probe — the third deferred check — is intentionally a separate PR. Today's Backend egress check probes from the CLI host, not the cluster's egress path; a real probe needs a port-forward to the egress-proxy's probe endpoint (or an exec/probe pod), which is a different mechanism from these read-only checks and deserves its own design.

Testing

make ci green (vet, go test -race -cover, errcheck, ineffassign, misspell, gofmt -s, schema-check). New table tests with the fake clientset cover node-fit (fits / no-fit / GPU-missing-warn / not-ready-node / missing-requests-warn), image-pull (none / valid / missing / malformed), and the parseCPUMem/parseGPU helpers. TestRun_HealthyCluster updated to 8 checks.

🤖 Generated with Claude Code


Note

Low Risk
Read-only diagnostic checks using existing Kubernetes list/get APIs; no cluster mutations or auth changes.

Overview
Extends tracebloc cluster doctor with two read-only checks (8 total), aimed at jobs that stay Pending or hit ImagePullBackOff.

Node capacity reads jobs-manager RESOURCE_REQUESTS / GPU_REQUESTS, lists Ready nodes, and requires one node whose allocatable covers cpu+memory (and GPU when requested). GPU-only gaps are warn (CPU fallback); missing env or node list permission is warn, not fail. Logic explicitly avoids treating cpu/mem on one node plus GPU on another as schedulable (Bugbot #91).

Image pull secret inspects jobs-manager imagePullSecrets: absent refs → ok; otherwise each secret must exist, be kubernetes.io/dockerconfigjson, and have valid non-empty .dockerconfigjson JSON.

Adds parsing helpers (parseResourceSpec, parseCPUMem, parseGPU, nodeReady) and fake-clientset tests for both checks; TestRun_HealthyCluster now supplies RESOURCE_REQUESTS and a fitting node.

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

Two read-only checks added to `tracebloc cluster doctor` (follow-up to #89):
- Node capacity: parses the resource requests jobs-manager stamps on spawned
training jobs (RESOURCE_REQUESTS / GPU_REQUESTS env) and checks at least one
Ready node can fit them — the "Pending forever, no node big enough" class.
GPU is soft: a hard ✖ only on cpu/mem, and a ⚠ when a GPU is requested but no
node exposes it (jobs-manager has a GPU->CPU fallback).
- Image pull secret: when jobs-manager references a registry pull secret,
verifies it exists and is a well-formed dockerconfigjson so private-image
pulls don't ImagePullBackOff.
Both read-only/best-effort, tested with client-go's fake clientset. The
in-cluster egress probe (the third deferred check on #90) is intentionally a
separate PR — it needs a port-forward/exec mechanism, not this read-only pattern.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@saadqbalsaadqbal self-assigned this Jun 18, 2026

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a0bb000. Configure here.

Comment threadinternal/doctor/doctor.go
checkNodeFit set cpuMemFits and gpuFits independently, so they could come from
different nodes — reporting OK even when no single node had cpu+memory+GPU
together (a GPU job would then stay Pending). It now evaluates each node as a
whole: cpuMemFits (any node) drives the hard fail; fullFits (one node with
cpu+mem AND the GPU) drives the ok/warn split. Adds regression tests for the
cross-node and single-node-fits cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@saadqbal
saadqbal merged commit 393edc0 into developJun 19, 2026
15 checks passed
@aptracebloc

Copy link
Copy Markdown
Contributor

Non-blocking review notes — one actionable nit + one scope clarification. (Nice that it already carries the httpProbeClose() fix from #89.)

Map-iteration nondeterminism — parseGPU:

fork, v:=rangeparseResourceSpec(spec) {
ifq, err:=resource.ParseQuantity(v); err==nil&&!q.IsZero() {
returncorev1.ResourceName(k), q, true
}
}

Go randomizes map iteration order, so if GPU_REQUESTS ever carried more than one entry this returns a nondeterministic one (and risks a flaky test). Single-entry specs (nvidia.com/gpu=1) are fine today; cheap hardening is to iterate sorted keys, or read the expected key directly.

Scope clarification (not a bug) — capacity vs. free capacity:
checkNodeFit compares the request against each node's Allocatable (total schedulable capacity), not allocatable minus already-requested pods. So it answers "is any node fundamentally big enough?" — the right question for a doctor — but it won't catch "nodes are big enough but currently full." Worth a word in the detail/remedy so a ✔ isn't read as "schedulable right now."

The per-node fit logic (requiring cpu+mem+GPU on a single node rather than OR-ing across nodes) is the subtle-but-correct call here — good that it's pinned by tests.

— drafted with Claude (Opus 4.8), sent by @aptracebloc

@saadqbal
saadqbal deleted the feat/doctor-node-image-checks branch July 10, 2026 10:37
shujaatTracebloc added a commit that referenced this pull request Aug 24, 2026
…#2223) (#564)
* feat(doctor): measure disk, the dimension it never looked at (backend#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>
* chore(release): bump VERSION to 0.10.12 (backend#2223)
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>
---------
Co-authored-by: shujaat hasan <shujaathasan@shujaats-MacBook-Pro.local>
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@saadqbal@aptracebloc@LukasWodka