From 89b9880ab3d33c661d3e168871e08fa28bbeaaec Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 09:55:49 +0200 Subject: [PATCH 1/4] fix(doctor): measured image-pull/dataset Fail outranks the wait-for-capacity Warn (backend#3248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `stuckPending && heldByJob` arm in summarizeDoctor returned StatusWarn above the Image-pull-secret and Dataset-volume StatusFail arms. checkImagePull and checkPVC are independent of Pod health and Node capacity, so either can be Fail while a pod is Pending past grace beside a running job — the exact co-occurrence the Wait-Warn matches. Sitting below the Warn, those measured failures were shadowed: `doctor` exited 0 (Warn) instead of 2 (Fail), hiding a real training-blocker behind a "wait for the job" warning. Reorder so the two measured Fail arms are read before the Wait-Warn. The Wait-Warn must stay above the plain stuck-Pending Fail (backend#2870), so the measured Fails necessarily move above stuck-Pending too — consistent with the switch's measured-beats-inferred rule. A test pins that an image-pull-secret or dataset-volume Fail beside a running job now wins (exit 2), while the Wait-Warn still applies when there is no measured Fail. Closes tracebloc/backend#3248 Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 39 ++++++++++++++------ internal/cli/doctor_test.go | 72 +++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index d11acf7..b513262 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -492,6 +492,25 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — this machine is big enough, but the platform's own services have already claimed the room.", fmt.Sprintf("Ask for less per training run, or give the machine more memory/CPU. Do NOT size runs to the machine here — that measures the machine's total, not what is free, so it would ask for MORE and leave the training stuck. `%s doctor --verbose` shows the exact numbers and the knob to turn.", launcher())} + // MEASURED training-blockers that can co-occur with a Pending pod held by a + // running job, so they are read BEFORE the Wait-Warn below (backend#3248, + // Bugbot Medium on #641). checkImagePull and checkPVC are + // INDEPENDENT of Pod health and Node capacity, so either can be Fail while + // `stuckPending && heldByJob` is also true — a pod Pending on an image it + // cannot pull, or beside a dataset volume that never bound, is a measured + // failure, not a wait. Below the Wait-Warn these Fails were shadowed: a node + // held by a job with any pod Pending past grace made `doctor` exit 0 (Warn) + // over an exit-2 failure. They sit above the stuck-Pending arm too — the same + // measured-beats-inferred rule that lets the Wait-Warn refute the Pending + // inference puts a measured cause ahead of it. + case by["Image pull secret"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — the training images can't be pulled.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} + case by["Dataset volume (PVC)"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — dataset storage isn't available.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} case stuckPending && heldByJob: // A PENDING POD WHOSE CAUSE HAS BEEN MEASURED (Bugbot High on #639, // backend#2870). This is the transient shortage's own symptom: a job is @@ -509,11 +528,17 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // cause -- the envelope fits this machine and a running job holds it -- // which refutes the inference, and a Fail here would exit 2 on healthy // training (the Bugbot High on #628) while recommending a resize that - // changes nothing. The two arms that still outrank this one are measured: - // a Pod-health FAIL (crash-loop) and the OverCommitted Fail. + // changes nothing. The arms that still outrank this one are all MEASURED + // failures — a Pod-health FAIL (crash-loop), the OverCommitted Fail, and + // the image-pull-secret and dataset-volume Fails just above — because a + // measured training-blocker must not be hidden behind a wait (backend#3248: + // those two Fails used to sit BELOW this arm, so a Pending pod beside a + // running job made `doctor` exit 0 over a real, exit-2 failure). // // checkPods does not know WHY a pod is Pending, so a pod stuck on an - // image pull beside a running job would land here too; the remedy names + // image pull beside a running job could still reach this arm when the + // image-pull-secret probe itself is healthy (the secret exists, but the + // pull is slow or the registry is briefly unreachable); the remedy names // what to do if the wait outlives the job rather than pretending the // attribution is certain. ready = healthLine{doctor.StatusWarn, @@ -530,14 +555,6 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment can't start yet.", fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. %s Then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", computeRemedy(runtime.GOOS), launcher(), launcher())} - case by["Image pull secret"].Status == doctor.StatusFail: - ready = healthLine{doctor.StatusFail, - "Not ready — the training images can't be pulled.", - fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} - case by["Dataset volume (PVC)"].Status == doctor.StatusFail: - ready = healthLine{doctor.StatusFail, - "Not ready — dataset storage isn't available.", - fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} case by["Node capacity"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — not enough free compute to start a training.", diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index acd12ce..7f07683 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -572,6 +572,56 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3248 (Bugbot Medium on #641). The Wait-Warn arm above is an + // INFERENCE that a Pending pod beside a running job is only waiting for the + // room. checkImagePull and checkPVC are MEASURED and independent of + // Pod health and Node capacity, so either can be Fail in that exact state — a + // missing pull secret, or a dataset volume that never bound, beside a running + // job. The Wait-Warn used to sit ABOVE those Fail arms, so the measured failure + // was shadowed and `doctor` exited 0 (Warn) instead of 2 (Fail). Pin that a + // measured Fail wins the precedence, while the Wait-Warn still applies when + // there is no measured Fail. + t.Run("a measured Fail beside a running job outranks the wait-for-capacity Warn", func(t *testing.T) { + // The full waiting_for_capacity state: a running job holds the room AND a + // pod is Pending past grace — together the arm that returns the Wait-Warn. + // Detail built from the producer's constant, same discipline as the arm. + waiting := 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") + waiting = withDetail(waiting, "Pod health", doctor.StatusWarn, + "Pending > 5m0s: [train-second]") + // allOK omits "Image pull secret" (a fixture shortcut; Run() does emit it), + // and `with` only mutates an entry that already exists — so add it OK here, + // or the image-pull case below would silently stay unset and never flip. + waiting = append(waiting, res("Image pull secret", doctor.StatusOK)) + + // Precondition: with no measured Fail, that state is the exit-0 Wait-Warn. + if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn { + t.Fatalf("precondition: the wait-for-capacity state should be a Warn, got %v (%q)", r.status, r.text) + } + + // Each measured Fail, dropped into that same state, must win — top line and + // exit code both. Per-case subtests: map order is randomized, so a shared + // loop with t.Fatalf would report one nondeterministic case and hide the other. + measured := map[string]struct{ name, wantText string }{ + "image pull secret Fail": {"Image pull secret", "images can't be pulled"}, + "dataset volume Fail": {"Dataset volume (PVC)", "dataset storage isn't available"}, + } + for label, m := range measured { + t.Run(label, func(t *testing.T) { + c, r := summarizeDoctor(with(waiting, m.name, doctor.StatusFail), tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a measured Fail beside a running job must win — the Wait-Warn shadowed it and doctor exited 0 over a real failure, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, m.wantText) { + t.Errorf("want the measured Fail's own top line %q, got %q", m.wantText, r.text) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("the verdict must own the Fail (exit 2), not the wait Warn (exit 0), got %v", v) + } + }) + } + }) + t.Run("a Pending pod with NO running job is still the stuck-Pending Fail", func(t *testing.T) { // The other side: the arm above is scoped to the co-occurrence. A pod // Pending on a machine where nothing holds the room is the generic, @@ -583,6 +633,28 @@ func TestSummarizeDoctor(t *testing.T) { } }) + t.Run("a measured Fail with a Pending pod but NO running job still outranks the stuck-Pending inference", func(t *testing.T) { + // backend#3248, the no-heldByJob half of the reorder. Moving the measured + // Fails above the Wait-Warn necessarily moves them above the plain + // stuck-Pending Fail too (the Wait-Warn sits above stuck-Pending, + // backend#2870). Both are exit-2, so the observable change is which top + // line and remedy the operator sees — the measured image-pull cause, not + // the generic "usually not enough free compute" guess. Pin it, matching + // this file's discipline of nailing every ordering a reshuffle could undo. + results := withDetail(allOK, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [trainer-x]") + results = append(results, res("Image pull secret", doctor.StatusFail)) + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a measured image-pull Fail must stay a Fail, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "images can't be pulled") { + t.Errorf("the measured image-pull cause must win over the generic stuck-Pending guess, got %q", r.text) + } + if strings.Contains(r.remedy, "resources set max") { + t.Errorf("a measured image-pull Fail must not send the operator to resize compute: %q", r.remedy) + } + }) + t.Run("a crash-looping pod still outranks the running-job explanation", func(t *testing.T) { // The exception is scoped to the stuck-Pending WARN; a Pod-health FAIL is // a measured failure with a different fix, and must keep winning. From f687edf58d36e65d8e9dba3afdde4ab0ca2c2b02 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 11:31:01 +0200 Subject: [PATCH 2/4] fix(doctor): classify pull-secret/PVC read failures as can't-checks, not measured Fails (backend#3248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses LukasWodka's review on cli#643. checkImagePull and checkPVC returned StatusFail for ANY error from the secret/PVC read — Forbidden, timeout, transient — conflating a can't-check with a measured absence. Once the reorder promoted those Fail arms above the wait-for-capacity Warn, a read blip beside a running job flipped a healthy environment from exit 0 to exit 2 with a detail that falsely said "not found". - checkImagePull: a non-NotFound Get error is now a StatusWarn can't-check (CantReadImagePullSecret prefix); StatusFail only for a genuine not-found / wrong-type / malformed secret. - checkPVC: a DiscoverSharedPVC read failure (PVCReadErrPrefix, now a shared constant) is a StatusWarn can't-check; StatusFail only for a read-and-found missing / unbound PVC. - summarizeDoctor: two new can't-check Unknown arms drop those reads to the honest "couldn't check ..." tier instead of the promoted measured Fail. - Update the StuckJobPod arm comment (backend#3247), now stale: the image-pull and dataset Fails sit above it too. - Thread 3: the over-commit Warn can't be lifted above the Wait-Warn in the Pending variant (the stuck-Pending Fail sits between them), so pin the current message-only behavior rather than add a fragile special-case arm. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 26 +++++++- internal/cli/doctor_test.go | 61 +++++++++++++++++++ .../cli/testdata/golden/zz-all-strings.golden | 5 +- internal/cluster/pvc.go | 13 +++- internal/doctor/doctor.go | 38 ++++++++++++ internal/doctor/doctor_test.go | 33 ++++++++++ 6 files changed, 170 insertions(+), 6 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 4477f1f..bf9c0f7 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -561,9 +561,11 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // grace window is left to the stuck-Pending arm below, whose "usually not // enough free compute, or an image that can't be pulled" wording is an // honest age-based inference (a large first pull on a cold node CAN exceed - // the grace). Only the measured capacity Fails (OverCommitted) and a hard - // Pod-health crash-loop Fail outrank it. Its remedy is the OPPOSITE of the - // transient Warn's (inspect the pod, do NOT wait); the pod it names is one + // the grace). The arms that outrank it are all measured: a Pod-health + // crash-loop Fail, the OverCommitted Fail, and the image-pull-secret and + // dataset-volume Fails above (backend#3248 — those two sit above the + // wait-for-capacity Warn, hence above this arm too). Its remedy is the + // OPPOSITE of the transient Warn's (inspect the pod, do NOT wait); the pod it names is one // `--verbose` away -- and PLAIN TERMS, no Kubernetes vocabulary, like its // neighbours (the granular checkNodeFit remedy carries the `kubectl` form). ready = healthLine{doctor.StatusFail, @@ -650,6 +652,24 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // 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 could not be READ (Forbidden / + // timeout), 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). + ready = healthLine{doctor.StatusUnknown, + "Ready to run training — couldn't check the image pull secret (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)", ""} default: ready = healthLine{doctor.StatusOK, "Ready to run training", ""} } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 3272af7..d7f66bc 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -711,6 +711,67 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3248 (LukasWodka on #643): checkImagePull / checkPVC now return a + // can't-check Warn (with a distinct prefix) when the secret / PVC could not be + // READ, distinct from a measured missing / unbound Fail. A can't-read carries + // no signal, so it must roll up to the Unknown tier — never the promoted + // 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`}) + if _, r := summarizeDoctor(imgCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "image pull secret") { + t.Errorf("a can't-read image-pull must roll up to a can't-check, got %v (%q)", r.status, r.text) + } + pvcCantRead := withDetail(allOK, "Dataset volume (PVC)", doctor.StatusWarn, + 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) + } + }) + + // 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 + // longer promotes over the Wait-Warn — the operator is told to wait, not handed + // a false "images can't be pulled" exit 2 on a healthy environment. + t.Run("a can't-READ secret beside a running job stays the wait Warn, not a false exit-2", func(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`}) + 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) + } + if v := doctorVerdict(c.status, r.status); v == doctor.StatusFail { + t.Errorf("a read blip must not make doctor exit 2 on a healthy environment, got verdict %v", v) + } + }) + + // backend#3248 thread 3 (LukasWodka on #643). When the machine over-commits AND + // a running job holds the room AND the next pod is Pending, both findings are + // Warns (exit 0). The plain-heldByJob case puts the over-commit Warn first + // ("a machine that lies about its size outranks a running job"), but the + // Pending variant cannot: the Wait-Warn sits above the stuck-Pending Fail + // (backend#2870) and the over-commit Warn must stay below that Fail (a Warn may + // not shadow a Fail), so by transitivity the Wait-Warn wins here. It is + // message-only (both exit 0) and pre-existing; lifting it would need a dedicated + // arm, not a reorder. Pin the current behavior so the gap is recorded, not implied. + t.Run("over-commit Warn is shadowed by the wait-for-capacity Warn in the Pending variant (known, message-only)", func(t *testing.T) { + results := 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") + results = withDetail(results, "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]") + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusWarn { + t.Fatalf("both findings are Warns → exit 0, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "waiting for it") { + t.Errorf("known message-only gap: the Pending variant shows the wait-for-capacity Warn, not the over-commit Warn — if a dedicated arm is added, update this pin, got %q", r.text) + } + }) + t.Run("a crash-looping pod still outranks the running-job explanation", func(t *testing.T) { // The exception is scoped to the stuck-Pending WARN; a Pod-health FAIL is // a measured failure with a different fix, and must keep winning. diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 926ee30..fb0ee16 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -49,6 +49,7 @@ screen. %s/%d are runtime placeholders. "%q won't work — no hyphens or spaces (use _); use letters, digits, and underscores, starting with a letter or underscore (e.g. churn_train)" "%s state=%s namespace=%s location=%s" "%s %q must be WxH (e.g. 512x512)" +"%s %q: %v" "%s %q: height is not an integer: %w" "%s %q: width and height must both be positive" "%s %q: width is not an integer: %w" @@ -78,6 +79,7 @@ screen. %s/%d are runtime placeholders. "%s ×%d" "%s — %s" "%s — Kubernetes believes %.2f× the memory this machine has, because the k3d node containers are uncapped and each reports the whole VM" +"%s%s/%s: %w" "%s, 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 %s, so GPU jobs would rely on the CPU fallback (needs %s)" "%s, … and %d more" "%s/%s" @@ -329,7 +331,9 @@ screen. %s/%d are runtime placeholders. "Ready to run training — but a job is already using this machine's free compute, so the next run waits for it." "Ready to run training — but your environment thinks this machine is bigger than it is." "Ready to run training — can't check yet" +"Ready to run training — couldn't check dataset storage (run with --verbose)" "Ready to run training — couldn't check free compute (run with --verbose)" +"Ready to run training — couldn't check the image pull secret (run with --verbose)" "Ready to run training — couldn't check your workloads (run with --verbose)" "Reclaimed %d tracebloc image%s." "Reclaiming the temporary copy" @@ -682,7 +686,6 @@ screen. %s/%d are runtime placeholders. "reading %s: %w" "reading CSV header from %s: %w" "reading CSV row from %s: %w" -"reading PVC %s/%s: %w" "reading allocated port: %w" "reading dataset directory %q: %w" "reading dataset path %q: %w" diff --git a/internal/cluster/pvc.go b/internal/cluster/pvc.go index fd744d0..224f34b 100644 --- a/internal/cluster/pvc.go +++ b/internal/cluster/pvc.go @@ -41,6 +41,15 @@ const SharedPVCClaimName = "client-pvc" // mountPath: "/data/shared" const SharedPVCMountPath = "/data/shared" +// PVCReadErrPrefix marks the errors DiscoverSharedPVC returns when the PVC +// could not be READ at all (Forbidden / network / other), as opposed to a PVC +// that was read and found missing or unbound. Callers that classify a read +// failure differently from a measured verdict — e.g. doctor's checkPVC, which +// reports a can't-read as a can't-check rather than a training-blocking Fail +// (backend#3248) — match on this prefix, so it lives as one constant here +// rather than being retyped where the error is produced or consumed. +const PVCReadErrPrefix = "reading PVC " + // SharedPVC describes the chart's shared-data PVC after discovery. // Carries enough metadata for Phase 3 PR-b to construct a stage Pod // that can mount the same claim. @@ -101,8 +110,8 @@ func DiscoverSharedPVC(ctx context.Context, cs kubernetes.Interface, namespace s // Forbidden / network / other — surface as-is so the // customer can RBAC-debug. Wrapping rather than substituting // because the underlying %w already carries the useful info. - return nil, fmt.Errorf("reading PVC %s/%s: %w", - namespace, SharedPVCClaimName, err) + return nil, fmt.Errorf("%s%s/%s: %w", + PVCReadErrPrefix, namespace, SharedPVCClaimName, err) } if pvc.Status.Phase != corev1.ClaimBound { diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 62d886d..dcaae98 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -490,6 +490,21 @@ func checkPVC(ctx context.Context, cs kubernetes.Interface, ns string) Result { const name = "Dataset volume (PVC)" pvc, err := cluster.DiscoverSharedPVC(ctx, cs, ns) if err != nil { + if strings.HasPrefix(err.Error(), cluster.PVCReadErrPrefix) { + // DiscoverSharedPVC separates a Forbidden/network/other READ failure + // (this prefix) from a PVC it read and found missing or unbound. A + // can't-read is not a measured "storage isn't available": reporting it + // as a Fail (and promoting that Fail over the wait-for-capacity Warn -- + // backend#3248) would flip a healthy environment to exit 2 on an RBAC or + // timeout blip. Surface an honest can't-check the rollup drops to the + // Unknown tier instead. + return Result{ + Name: name, + Status: StatusWarn, + Detail: err.Error(), + Remedy: "Check the CLI can read PersistentVolumeClaims in " + ns + " (kubectl auth can-i get pvc -n " + ns + ").", + } + } return Result{ Name: name, Status: StatusFail, @@ -1135,6 +1150,14 @@ const HeldByRunningJob = "a running job holds the room" // that drifted would send a wedged pod back down the "just wait" path. const StuckJobPod = "a training pod is scheduled but not running" +// CantReadImagePullSecret is the prefix of checkImagePull's can't-check Warn: the +// image pull secret could not be READ (Forbidden / timeout / transient), as +// opposed to read and found missing or malformed. Same discipline as the +// prefixes above -- the rollup (summarizeDoctor) classifies on it to drop a +// can't-read to the Unknown tier rather than a measured Fail promoted over the +// wait-for-capacity Warn (backend#3248). +const CantReadImagePullSecret = "could not read image pull secret" + // cpuMemString renders a millicore/byte pair the way RESOURCE_REQUESTS reads // ("cpu=2, memory=8Gi"), so the free/held figures in a Node-capacity verdict // line up with the request printed beside them. @@ -1181,6 +1204,21 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel for _, ref := range secrets { sec, err := cs.CoreV1().Secrets(ns).Get(ctx, ref.Name, metav1.GetOptions{}) if err != nil { + if !apierrors.IsNotFound(err) { + // Forbidden / timeout / transient API error — the secret was NOT + // READ, not proven absent. Reporting it as a measured Fail (and, in + // the rollup, promoting that Fail over the wait-for-capacity Warn -- + // backend#3248) would flip a healthy environment to exit 2 on an RBAC + // or timeout blip, with a detail that falsely says "not found". A + // can't-check is honest: StatusWarn with a distinct prefix the rollup + // drops to the Unknown tier, never a training-blocking verdict. + return Result{ + Name: name, + Status: StatusWarn, + Detail: fmt.Sprintf("%s %q: %v", CantReadImagePullSecret, ref.Name, err), + Remedy: "Check the CLI can read secrets in " + ns + " (kubectl auth can-i get secrets -n " + ns + ").", + } + } return Result{ Name: name, Status: StatusFail, diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 40e4f05..b2ce30c 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -364,6 +364,18 @@ func TestCheckPVC(t *testing.T) { if r := checkPVC(bg(), fake.NewClientset(), ns); r.Status != StatusFail { t.Fatalf("missing PVC => %v, want fail", r.Status) } + // backend#3248 (LukasWodka on #643): a PVC that could not be READ + // (Forbidden/network) is a can't-check, NOT a measured "unavailable" Fail. + // DiscoverSharedPVC wraps it with PVCReadErrPrefix; checkPVC must surface a + // StatusWarn so the rollup drops it to the Unknown tier rather than promoting + // a false Fail over the wait-for-capacity Warn. + unreadable := fake.NewClientset() + unreadable.PrependReactor("get", "persistentvolumeclaims", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("persistentvolumeclaims \"client-pvc\" is forbidden: RBAC") + }) + if r := checkPVC(bg(), unreadable, ns); r.Status != StatusWarn || !strings.HasPrefix(r.Detail, cluster.PVCReadErrPrefix) { + t.Fatalf("unreadable PVC => %v (%q), want a can't-check Warn with the read-err prefix", r.Status, r.Detail) + } } func TestCheckProxy(t *testing.T) { @@ -1299,6 +1311,27 @@ func TestCheckImagePull(t *testing.T) { t.Fatalf("=> %v (%q), want fail", r.Status, r.Detail) } }) + // backend#3248 (LukasWodka on #643): a read failure (Forbidden/timeout) is a + // can't-check, NOT a measured "not found" Fail. Get returns the error but the + // secret's existence was never established — so this must be a StatusWarn with + // the can't-read prefix, or the rollup promotes a false Fail over the + // wait-for-capacity Warn and exits 2 on a healthy environment. + t.Run("secret unreadable (forbidden) -> can't-check Warn, not a false 'not found' Fail", func(t *testing.T) { + cs := fake.NewClientset(jmDepWithPullSecret("tb", "reg")) + cs.PrependReactor("get", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("secrets \"reg\" is forbidden: RBAC") + }) + r := checkImagePull(bg(), cs, ns, rel) + if r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want a can't-check Warn on a read failure", r.Status, r.Detail) + } + if !strings.HasPrefix(r.Detail, CantReadImagePullSecret) { + t.Errorf("detail must carry the can't-read prefix so summarizeDoctor can classify it, got %q", r.Detail) + } + if strings.Contains(r.Detail, "not found") { + t.Errorf("a read failure must not be reported as 'not found', got %q", r.Detail) + } + }) } // nodeWithDisk is `node` plus an ephemeral-storage allocatable. Separate helper From f7f12c7d577c7063b36b9b7d455802a2b5c6fccb Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 11:55:11 +0200 Subject: [PATCH 3/4] fix(doctor): close the jobs-manager-unreadable false green; tighten the Wait-Warn precondition (backend#3248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review points from cli#643: - Saqlain: checkImagePull's OTHER can't-read path — the jobs-manager Deployment itself unreadable (dep == nil) — returned a StatusWarn with no CantReadImagePullSecret prefix, so summarizeDoctor matched no arm and it fell through to the OK default: a false green at exit 0, the same class this PR closes for the secret read. Route it to the Unknown tier by carrying the prefix. Producer test added. - LukasWodka: the co-occurrence test's precondition asserted only StatusWarn, so a fall-through to the bare heldByJob Warn (also exit 0) would keep it green if the reorder were undone — leaving the exit-0 → exit-2 claim unpinned. Assert the Wait-Warn's own top line instead. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor_test.go | 9 +++++++-- internal/cli/testdata/golden/zz-all-strings.golden | 1 - internal/doctor/doctor.go | 8 +++++++- internal/doctor/doctor_test.go | 10 ++++++++++ 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index d7f66bc..074dd12 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -595,8 +595,13 @@ func TestSummarizeDoctor(t *testing.T) { waiting = append(waiting, res("Image pull secret", doctor.StatusOK)) // Precondition: with no measured Fail, that state is the exit-0 Wait-Warn. - if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn { - t.Fatalf("precondition: the wait-for-capacity state should be a Warn, got %v (%q)", r.status, r.text) + // Assert the Wait-Warn's OWN top line, not merely "a Warn" (LukasWodka on + // #643): the bare heldByJob arm below is also a Warn, so a status-only check + // would still pass if the reorder were undone and the state fell through to + // it — leaving the exit-0 → exit-2 claim this test exists for unpinned. + if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn || + !strings.Contains(r.text, "the next one is waiting for it to finish") { + t.Fatalf("precondition: the wait-for-capacity state should be the Wait-Warn, got %v (%q)", r.status, r.text) } // Each measured Fail, dropped into that same state, must win — top line and diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index fb0ee16..b578fdf 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -506,7 +506,6 @@ screen. %s/%d are runtime placeholders. "couldn't reach the backend to finish signing in — %d attempts failed in a row (check your network / HTTPS_PROXY): %w" "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit" "couldn't read capacity: %v" -"couldn't read jobs-manager to resolve image pull secrets — skipping" "couldn't read the account's client list to tell whether this cluster is new or already registered (%v) — retry when the backend is reachable (a re-run adopts an existing client), or pass --yes/--credential-file to provision now" "couldn't read the chart's conformance checks: %w" "couldn't read this machine's capacity: %w" diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index dcaae98..694994f 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -1190,10 +1190,16 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel const name = "Image pull secret" dep := findDeployment(ctx, cs, ns, release, "jobs-manager") if dep == nil { + // The jobs-manager Deployment could not be read, so the pull secret can't + // be resolved — a can't-check, not a clean result. It carries the same + // CantReadImagePullSecret prefix as the unreadable-secret path below so the + // rollup drops BOTH to the Unknown tier; without the prefix this Warn fell + // through to the OK default and reported a false green ✔ (Saqlain + LukasWodka + // on #643 — fix the class, not just the secret-read instance). return Result{ Name: name, Status: StatusWarn, - Detail: "couldn't read jobs-manager to resolve image pull secrets — skipping", + Detail: CantReadImagePullSecret + ": couldn't read jobs-manager to resolve it — skipping", Remedy: "Check a tracebloc client is installed in " + ns + ".", } } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index b2ce30c..6d3b2f5 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -1332,6 +1332,16 @@ func TestCheckImagePull(t *testing.T) { t.Errorf("a read failure must not be reported as 'not found', got %q", r.Detail) } }) + // backend#3248 (Saqlain on #643): the OTHER can't-read path — the jobs-manager + // Deployment itself is unreadable — is also a can't-check, and must carry the + // same prefix so the rollup drops it to the Unknown tier instead of falling + // through to a false green ✔. + t.Run("jobs-manager unreadable -> can't-check Warn with the read prefix", func(t *testing.T) { + r := checkImagePull(bg(), fake.NewClientset(), ns, rel) // no jobs-manager Deployment + if r.Status != StatusWarn || !strings.HasPrefix(r.Detail, CantReadImagePullSecret) { + t.Fatalf("=> %v (%q), want a can't-check Warn carrying the read prefix", r.Status, r.Detail) + } + }) } // nodeWithDisk is `node` plus an ephemeral-storage allocatable. Separate helper From e4a446db9e0b4f9da54075b23f3eeaf1f6743b01 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 12:33:05 +0200 Subject: [PATCH 4/4] fix(doctor): plain-terms wording for the image-pull can't-check rollup line (backend#3248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot on cli#643: the new image-pull can't-check ready line said "image pull secret" — Kubernetes jargon. The rolled-up summarizeDoctor lines stay in plain terms (renderDoctorDetails is the only place k8s vocabulary appears), and the twin PVC line already says "dataset storage". Reword to mirror the Fail arm's "training images can't be pulled". Test assertion + golden updated. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 18 +++++++++++------- internal/cli/doctor_test.go | 4 ++-- .../cli/testdata/golden/zz-all-strings.golden | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index bf9c0f7..87c837a 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -654,14 +654,18 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready "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 could not be READ (Forbidden / - // timeout), 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). + // 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 the image pull secret (run with --verbose)", ""} + "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), diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 074dd12..695de9c 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -724,8 +724,8 @@ func TestSummarizeDoctor(t *testing.T) { 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`}) - if _, r := summarizeDoctor(imgCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "image pull secret") { - t.Errorf("a can't-read image-pull must roll up to a can't-check, got %v (%q)", r.status, r.text) + 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, cluster.PVCReadErrPrefix+"ns/client-pvc: is forbidden") diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index b578fdf..af95723 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -333,7 +333,7 @@ screen. %s/%d are runtime placeholders. "Ready to run training — can't check yet" "Ready to run training — couldn't check dataset storage (run with --verbose)" "Ready to run training — couldn't check free compute (run with --verbose)" -"Ready to run training — couldn't check the image pull secret (run with --verbose)" +"Ready to run training — couldn't check whether training images can be pulled (run with --verbose)" "Ready to run training — couldn't check your workloads (run with --verbose)" "Reclaimed %d tracebloc image%s." "Reclaiming the temporary copy"