Skip to content

chore(doctor): one can't-check rollup rule via a CantCheck marker, not N prefix-matched arms (backend#3282) #644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aptracebloc merged 1 commit into from
Sep 7, 2026
Merged

chore(doctor): one can't-check rollup rule via a CantCheck marker, not N prefix-matched arms (backend#3282) #644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions internal/cli/copy_catalog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,11 @@ func TestCopyCatalog(t *testing.T) {
}
}
// Connected + readiness unknown: Pod health warns with a list failure, which
// summarizeDoctor maps to an honest "couldn't check your workloads".
cantCheck := []doctor.Result{{Name: "Pod health", Status: doctor.StatusWarn, Detail: "could not list pods: forbidden"}}
// summarizeDoctor maps to an honest "couldn't check your workloads". CantCheck
// is the marker the producer (checkPods) now sets on a can't-read Warn, and the
// rollup classifies on it (backend#3282) — without it this fixture reads as a
// stuck-Pending Fail.
cantCheck := []doctor.Result{{Name: "Pod health", Status: doctor.StatusWarn, CantCheck: true, Detail: "could not list pods: forbidden"}}
doctorFile := doc(
"tb doctor — is my secure environment healthy?",
"What you see when you run `tb doctor`. The two rollup lines (Connected, Ready)\nplus a verdict are shown below for the healthy and the can't-fully-check cases.\nThe failure variants (Not connected — …, Not ready — …) and their remedies vary\nwith the reachability classification and embed the launcher name, so the full set\nis indexed in zz-all-strings.golden. --verbose adds a Kubernetes breakdown\n(context/server/namespace + each granular check); those strings are in the\nbackstop too.",
Expand Down
98 changes: 47 additions & 51 deletions internal/cli/doctor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,12 +445,13 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
// Unknown", backend#2438). StatusUnknown carries no signal, so it must be the
// last thing consulted — it may only win when nothing above it fired.
// Two signals that are read in more than one arm, named once so the arms
// cannot drift apart on the prefix (the same discipline as the producer
// constants they match). stuckPending is checkPods' Pending-past-grace Warn,
// with its "could not list pods" can't-check excluded; heldByJob is
// checkNodeFit's transient verdict (backend#2870).
// cannot drift apart. stuckPending is checkPods' Pending-past-grace Warn, with
// its can't-check (pod-list-unreadable) excluded via the structural CantCheck
// marker rather than a Detail prefix (backend#3282); heldByJob is checkNodeFit's
// transient verdict, matched by the HeldByRunningJob prefix it is classified on
// (a real soft finding, not a can't-check — backend#2870).
stuckPending := by["Pod health"].Status == doctor.StatusWarn &&
!strings.HasPrefix(by["Pod health"].Detail, "could not list pods")
!by["Pod health"].CantCheck
heldByJob := by["Node capacity"].Status == doctor.StatusWarn &&
strings.HasPrefix(by["Node capacity"].Detail, doctor.HeldByRunningJob)

Expand Down Expand Up @@ -629,57 +630,52 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
ready = healthLine{doctor.StatusWarn,
"Ready to run training — but a job is already using this machine's free compute, so the next run waits for it.",
fmt.Sprintf("Nothing is wrong with the machine: let the running job finish, or stop it if it is not needed. Asking for less per run or resizing will not help here — the room comes back when the job ends. `%s doctor --verbose` shows the numbers.", launcher())}
// ── Unknown: a check couldn't complete — no signal, so it never shadows a
// Fail or Warn above and only surfaces when nothing real was found. ──
case by["Pod health"].Status == doctor.StatusWarn && strings.HasPrefix(by["Pod health"].Detail, "could not list pods"):
// checkPods returns StatusWarn for TWO different situations: pods stuck
// Pending (the Fail arm above) AND a failure to list pods at all (e.g. RBAC,
// doctor.go checkPods). For the latter we simply can't tell whether training
// can run, so report an honest can't-check — never the stuck-pending/compute
// remedy, which would misdiagnose a permissions problem (Bugbot).
ready = healthLine{doctor.StatusUnknown,
"Ready to run training — couldn't check your workloads (run with --verbose)", ""}
case by["Node capacity"].Status == doctor.StatusWarn &&
(strings.HasPrefix(by["Node capacity"].Detail, "couldn't read RESOURCE_REQUESTS") ||
strings.HasPrefix(by["Node capacity"].Detail, "could not list nodes") ||
strings.HasPrefix(by["Node capacity"].Detail, doctor.CantVerifyFreeCompute)):
// checkNodeFit's Warn covers two different situations: a can't-check
// (RESOURCE_REQUESTS unreadable, nodes unlistable) and the soft GPU
// fallback. For a can't-check we simply don't know whether a node can fit
// a training job, so report an honest can't-check — mirroring the Pod-health
// list-failure case above — never a ✔ that skipped the capacity probe
// (Bugbot). The GPU-soft Warn intentionally stays Ready (falls through to
// the OK default): training still runs via the jobs-manager's CPU fallback.
ready = healthLine{doctor.StatusUnknown,
"Ready to run training — couldn't check free compute (run with --verbose)", ""}
case by["Image pull secret"].Status == doctor.StatusWarn &&
strings.HasPrefix(by["Image pull secret"].Detail, doctor.CantReadImagePullSecret):
// checkImagePull can't-check: the secret (or the jobs-manager that names it)
// could not be READ, not read-and-found-missing. It carries no signal about
// whether images can be pulled, so it lands here in the Unknown tier — never
// the measured "images can't be pulled" Fail above, which is now promoted
// over the wait-for-capacity Warn and would flip a healthy environment to
// exit 2 on an RBAC blip (backend#3248, LukasWodka on #643).
//
// PLAIN TERMS, no Kubernetes vocabulary — "image pull secret" is jargon that
// belongs one --verbose away in renderDoctorDetails, so this line mirrors the
// Fail arm's "training images can't be pulled" wording (Bugbot on #643).
ready = healthLine{doctor.StatusUnknown,
"Ready to run training — couldn't check whether training images can be pulled (run with --verbose)", ""}
case by["Dataset volume (PVC)"].Status == doctor.StatusWarn &&
strings.HasPrefix(by["Dataset volume (PVC)"].Detail, cluster.PVCReadErrPrefix):
// checkPVC can't-check: the PVC could not be READ (Forbidden / network),
// not read-and-found-unbound. Same reasoning as the image-pull arm — a
// can't-read is no signal, so it stays in the Unknown tier rather than the
// measured "dataset storage isn't available" Fail above (backend#3248).
ready = healthLine{doctor.StatusUnknown,
"Ready to run training — couldn't check dataset storage (run with --verbose)", ""}
// ── Unknown: a check couldn't COMPLETE — no signal, so it never shadows a
// Fail or Warn above and only surfaces when nothing real was found. ONE rule
// for every can't-read probe (backend#3282): a check that set Result.CantCheck
// rolls up to an honest "couldn't check …", picked by cantCheckReady, in place
// of a per-probe arm that matched the producer's Detail prefix across package
// boundaries. A soft finding (over-commit, held-by-job, GPU fallback) does NOT
// set CantCheck, so it kept its own arm above and never reaches here. ──
default:
ready = healthLine{doctor.StatusOK, "Ready to run training", ""}
ready = cantCheckReady(by)
}
return connected, ready
}

// cantCheckReady is the rollup's single can't-check rule (backend#3282), the one
// arm that replaced four near-identical prefix-matched ones. When no real finding
// fired, a check that could not READ its subject (Result.CantCheck, set by the
// producer) carries no signal, so it surfaces as an honest Unknown "couldn't
// check …" rather than a false green ✔ — with the classification a structural
// marker rather than a Detail-prefix the CLI matches across package boundaries.
//
// The per-check line is CLI copy, in PLAIN TERMS (never the producer's Detail),
// and the order is the severity of what could not be verified — the same order as
// the arms this replaced, so a co-occurrence resolves identically. A check that
// sets CantCheck but is absent from this table is not surfaced (falls through to
// OK); add a row when a new probe should roll up — the only edit a new can't-read
// probe needs here, no shared prefix constant.
func cantCheckReady(by map[string]doctor.Result) healthLine {
// healthLine values, not bare strings, so the copy backstop (copy_catalog_test)
// harvests these lines the same way it did the arms they replaced — it scans
// healthLine{} literals for user-facing text.
for _, cc := range []struct {
check string
line healthLine
}{
{"Pod health", healthLine{doctor.StatusUnknown, "Ready to run training — couldn't check your workloads (run with --verbose)", ""}},
{"Node capacity", healthLine{doctor.StatusUnknown, "Ready to run training — couldn't check free compute (run with --verbose)", ""}},
{"Image pull secret", healthLine{doctor.StatusUnknown, "Ready to run training — couldn't check whether training images can be pulled (run with --verbose)", ""}},
{"Dataset volume (PVC)", healthLine{doctor.StatusUnknown, "Ready to run training — couldn't check dataset storage (run with --verbose)", ""}},
} {
if by[cc.check].CantCheck {
return cc.line
}
}
return healthLine{doctor.StatusOK, "Ready to run training", ""}
}

// renderHealth prints one rolled-up line: ✔ for OK, ✖ + remedy for a problem,
// and a neutral · "can't check" for StatusUnknown (no false green, no alarm).
func renderHealth(p *ui.Printer, h healthLine) {
Expand Down
61 changes: 48 additions & 13 deletions internal/cli/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,20 @@ func TestSummarizeDoctor(t *testing.T) {
}
return out
}
// cantCheck marks a named check as a can't-check the way its producer now does:
// StatusWarn + Result.CantCheck (backend#3282). The rollup classifies on the
// marker, not the Detail prefix, so withDetail alone no longer reads as a
// can't-check — these fixtures must set the marker, exactly as the checkX
// producers do.
cantCheck := func(base []doctor.Result, name, detail string) []doctor.Result {
out := withDetail(base, name, doctor.StatusWarn, detail)
for i := range out {
if out[i].Name == name {
out[i].CantCheck = true
}
}
return out
}

t.Run("all healthy → both OK", func(t *testing.T) {
c, r := summarizeDoctor(allOK, tokenOK)
Expand Down Expand Up @@ -311,11 +325,11 @@ func TestSummarizeDoctor(t *testing.T) {
base := withDetail(allOK, "Machine capacity", doctor.StatusWarn,
"Docker VM 7.75 GiB → 2 nodes claiming 15.50 GiB — Kubernetes believes 2.00× the memory this machine has")
cases := map[string][]doctor.Result{
"pod-list RBAC failure": withDetail(base, "Pod health", doctor.StatusWarn,
"pod-list RBAC failure": cantCheck(base, "Pod health",
"could not list pods: pods is forbidden"),
"RESOURCE_REQUESTS unreadable": withDetail(base, "Node capacity", doctor.StatusWarn,
"RESOURCE_REQUESTS unreadable": cantCheck(base, "Node capacity",
"couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit"),
"nodes unlistable": withDetail(base, "Node capacity", doctor.StatusWarn,
"nodes unlistable": cantCheck(base, "Node capacity",
"could not list nodes: nodes is forbidden"),
}
// Per-case subtests: map iteration is randomized, so a shared loop with
Expand All @@ -339,7 +353,7 @@ func TestSummarizeDoctor(t *testing.T) {
// Node-capacity / dataset Fail arms and swallowed them (backend#2438) — the
// Warn variant above got the fix + a test; this pins the Fail variant too.
t.Run("Fail is not shadowed by a co-occurring Unknown", func(t *testing.T) {
base := withDetail(allOK, "Pod health", doctor.StatusWarn,
base := cantCheck(allOK, "Pod health",
"could not list pods: pods is forbidden")
cases := map[string][]doctor.Result{
"node capacity Fail": withDetail(base, "Node capacity", doctor.StatusFail,
Expand Down Expand Up @@ -392,18 +406,18 @@ func TestSummarizeDoctor(t *testing.T) {
// backend#2870: unreadable pod list -> free unverifiable -> can't-check,
// not a green pass (Bugbot High: this used to roll up to "Ready").
//
// BUILT FROM THE PRODUCER'S CONSTANT, not retyped. This string was a
// third copy of the prefix the classifier matches on -- so a producer
// that reworded it would leave this test passing against a phrase
// nothing emits any more.
// BUILT FROM THE PRODUCER'S CONSTANT, not retyped — it is the --verbose
// detail wording (the rollup now classifies on the CantCheck marker, not
// this prefix), so keeping it from the constant still guards the caveat
// text the producer emits.
doctor.CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here",
// Bugbot High on #628: the same can't-check ARRIVING WITH the soft GPU
// warn. The GPU case fires first in checkNodeFit, so this combined
// detail is what a GPU-requesting install with an unreadable pod list
// actually produces -- and it must roll up the same way.
doctor.CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here. Also, no single Ready node satisfies cpu+memory AND nvidia.com/gpu, so GPU jobs would rely on the CPU fallback (needs cpu=2, memory=8Gi)",
} {
_, r := summarizeDoctor(withDetail(allOK, "Node capacity", doctor.StatusWarn, detail), tokenOK)
_, r := summarizeDoctor(cantCheck(allOK, "Node capacity", detail), tokenOK)
if r.status != doctor.StatusUnknown {
t.Errorf("%q: ready should be Unknown, got %v", detail, r.status)
}
Expand Down Expand Up @@ -723,17 +737,38 @@ func TestSummarizeDoctor(t *testing.T) {
// measured Fail, which would flip a healthy environment to exit 2 on an RBAC blip.
t.Run("a can't-READ image-pull or PVC is an honest can't-check, not the promoted Fail", func(t *testing.T) {
imgCantRead := append(append([]doctor.Result{}, allOK...),
doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, CantCheck: true, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
if _, r := summarizeDoctor(imgCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "training images can be pulled") {
t.Errorf("a can't-read image-pull must roll up to a plain-terms can't-check, got %v (%q)", r.status, r.text)
}
pvcCantRead := withDetail(allOK, "Dataset volume (PVC)", doctor.StatusWarn,
pvcCantRead := cantCheck(allOK, "Dataset volume (PVC)",
cluster.PVCReadErrPrefix+"ns/client-pvc: is forbidden")
if _, r := summarizeDoctor(pvcCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "dataset storage") {
t.Errorf("a can't-read PVC must roll up to a can't-check, got %v (%q)", r.status, r.text)
}
})

// backend#3282: the single can't-check rule (cantCheckReady). Two properties
// the four arms it replaced had implicitly, now pinned explicitly.
t.Run("the can't-check rollup rule keeps arm order and ignores unmapped checks", func(t *testing.T) {
// PRIORITY: multiple can't-checks co-occur → the first in the table (Pod
// health) wins, the same order as the arms this replaced.
both := cantCheck(cantCheck(allOK, "Pod health", "could not list pods: forbidden"),
"Dataset volume (PVC)", cluster.PVCReadErrPrefix+"ns/client-pvc: forbidden")
if _, r := summarizeDoctor(both, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "your workloads") {
t.Errorf("Pod-health can't-check should win the co-occurrence, got %v (%q)", r.status, r.text)
}
// UNMAPPED: a CantCheck on a check the rollup table does not list (here
// "Restart history", which checkRestartHistory marks) is not surfaced — it
// falls through to OK. A new probe must add a table row to roll up; until
// then it stays inert rather than greening or crashing.
unmapped := append(append([]doctor.Result{}, allOK...),
doctor.Result{Name: "Restart history", Status: doctor.StatusWarn, CantCheck: true, Detail: "could not list pods: forbidden"})
if _, r := summarizeDoctor(unmapped, tokenOK); r.status != doctor.StatusOK {
t.Errorf("a CantCheck on an unmapped check must not surface (falls through to OK), got %v (%q)", r.status, r.text)
}
})

// The exact regression from thread 1: a running job holds the room, the next
// pod is Pending (the wait-for-capacity state), AND the pull secret could not
// be read. Because that read failure is now a can't-check (not a Fail), it no
Expand All @@ -743,7 +778,7 @@ func TestSummarizeDoctor(t *testing.T) {
results := withDetail(allOK, "Node capacity", doctor.StatusWarn,
doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish")
results = withDetail(results, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [train-second]")
results = append(results, doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
results = append(results, doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, CantCheck: true, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
c, r := summarizeDoctor(results, tokenOK)
if r.status != doctor.StatusWarn || !strings.Contains(r.text, "waiting for it") {
t.Fatalf("a can't-read secret must not flip the wait-for-capacity Warn to a Fail, got %v (%q)", r.status, r.text)
Expand Down Expand Up @@ -892,7 +927,7 @@ func TestSummarizeDoctor(t *testing.T) {
// can't-check — it must NOT get the stuck-pending/compute (Docker Desktop)
// remedy (Bugbot follow-up).
t.Run("pod-health warn = could not list pods (RBAC) → can't-check, not stuck-pending", func(t *testing.T) {
_, r := summarizeDoctor(warnPods("could not list pods: pods is forbidden"), tokenOK)
_, r := summarizeDoctor(cantCheck(allOK, "Pod health", "could not list pods: pods is forbidden"), tokenOK)
if r.status == doctor.StatusFail {
t.Errorf("a can't-list-pods warn must not be a hard not-ready, got %v %q", r.status, r.text)
}
Expand Down
Loading
Loading