From a67bc1938110971587f197021c3e318453551eea Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 13:04:06 +0200 Subject: [PATCH] chore(doctor): one can't-check rollup rule via a CantCheck marker, not N prefix-matched arms (backend#3282) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to Saqlain's review on cli#643. summarizeDoctor classified "a check couldn't READ its subject" into the Unknown tier with four near-identical prefix-matched arms, each needing three coordinated edits across three packages (an exported prefix constant, a checkX branch stamping it into Detail, and a matching CLI arm) coupled by fragile cross-package strings. Add a structural doctor.Result.CantCheck, set by the checkX producers on their can't-read paths, and replace the four arms with one rule (cantCheckReady) that surfaces the first flagged check's line. The per-check messages stay CLI copy in an ordered table (healthLine values, so the copy backstop still harvests them), so no user-facing output changes — both goldens are a zero diff — and stuckPending's can't-check exclusion moves from a Detail-prefix to the marker too. A new can't-read probe now needs only "producer sets CantCheck" plus a table row, with no shared prefix constant to keep in lockstep. Producer tests assert the marker on the can't-read paths (and its absence on the HeldByRunningJob soft finding); a rollup test pins the arm order and the unmapped-check fall-through. Closes tracebloc/backend#3282 Co-Authored-By: Claude Opus 4.8 --- internal/cli/copy_catalog_test.go | 7 +- internal/cli/doctor.go | 98 ++++++++++++------------- internal/cli/doctor_test.go | 61 ++++++++++++---- internal/doctor/doctor.go | 116 +++++++++++++++++------------- internal/doctor/doctor_test.go | 31 ++++---- 5 files changed, 187 insertions(+), 126 deletions(-) diff --git a/internal/cli/copy_catalog_test.go b/internal/cli/copy_catalog_test.go index fb5cf34..29b84ab 100644 --- a/internal/cli/copy_catalog_test.go +++ b/internal/cli/copy_catalog_test.go @@ -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.", diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 87c837a..5dc8109 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -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) @@ -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) { diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 695de9c..8ff3b9d 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -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) @@ -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 @@ -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, @@ -392,10 +406,10 @@ 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 @@ -403,7 +417,7 @@ func TestSummarizeDoctor(t *testing.T) { // 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) } @@ -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 @@ -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) @@ -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) } diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 694994f..e2670cb 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -88,6 +88,16 @@ type Result struct { // container runtime / network) needs a different fix than a reachable cluster // with no tracebloc installed. Zero (ReachOK) on every other check. Reach ReachState + + // CantCheck marks a StatusWarn that is a CAN'T-CHECK — the check could not + // READ its subject (RBAC/timeout/unlistable), so its Warn carries no signal + // about whether training can run. It is the structural signal the rollup + // (summarizeDoctor) keys on to drop such a result into the Unknown tier, in + // place of matching a per-probe prefix in Detail across package boundaries + // (backend#3282). A StatusWarn that is a real soft finding — an over-committed + // machine, a running job holding the room, the GPU CPU-fallback — leaves this + // false, so it keeps its own rollup arm rather than reading as a can't-check. + CantCheck bool } // ReachState classifies the "Cluster reachable" outcome so the cli summary can @@ -338,10 +348,11 @@ func checkPods(ctx context.Context, cs kubernetes.Interface, ns string) Result { pods, err := cs.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{}) if err != nil { return Result{ - Name: name, - Status: StatusWarn, - Detail: "could not list pods: " + err.Error(), - Remedy: "Ensure your kubeconfig user can list pods in " + ns + ".", + Name: name, + Status: StatusWarn, + Detail: "could not list pods: " + err.Error(), + Remedy: "Ensure your kubeconfig user can list pods in " + ns + ".", + CantCheck: true, } } @@ -454,10 +465,11 @@ func checkRestartHistory(ctx context.Context, cs kubernetes.Interface, ns string pods, err := cs.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{}) if err != nil { return Result{ - Name: name, - Status: StatusWarn, - Detail: "could not list pods: " + err.Error(), - Remedy: "Ensure your kubeconfig user can list pods in " + ns + ".", + Name: name, + Status: StatusWarn, + Detail: "could not list pods: " + err.Error(), + Remedy: "Ensure your kubeconfig user can list pods in " + ns + ".", + CantCheck: true, } } @@ -499,10 +511,11 @@ func checkPVC(ctx context.Context, cs kubernetes.Interface, ns string) Result { // 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 + ").", + Name: name, + Status: StatusWarn, + Detail: err.Error(), + Remedy: "Check the CLI can read PersistentVolumeClaims in " + ns + " (kubectl auth can-i get pvc -n " + ns + ").", + CantCheck: true, } } return Result{ @@ -664,10 +677,11 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s cpuReq, memReq, ok := parseCPUMem(env["RESOURCE_REQUESTS"]) if !ok { return Result{ - Name: name, - Status: StatusWarn, - Detail: "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit", - Remedy: "kubectl set env deploy/-jobs-manager --list | grep RESOURCE_REQUESTS", + Name: name, + Status: StatusWarn, + Detail: "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit", + Remedy: "kubectl set env deploy/-jobs-manager --list | grep RESOURCE_REQUESTS", + CantCheck: true, } } gpuName, gpuReq, gpuRequested := parseGPU(env["GPU_REQUESTS"]) @@ -676,10 +690,11 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s nodes, err := cs.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) if err != nil { return Result{ - Name: name, - Status: StatusWarn, - Detail: "could not list nodes: " + err.Error(), - Remedy: "Ensure your kubeconfig user can list nodes.", + Name: name, + Status: StatusWarn, + Detail: "could not list nodes: " + err.Error(), + Remedy: "Ensure your kubeconfig user can list nodes.", + CantCheck: true, } } @@ -1024,10 +1039,11 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // can't-check FIRST because the rollup matches on the prefix. if !freeKnown { return Result{ - Name: name, - Status: StatusWarn, - Detail: fmt.Sprintf("%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)", CantVerifyFreeCompute, gpuName, req), - Remedy: "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to verify free capacity. If GPU training is expected, also ensure one node has both the compute and the GPU capacity, with its device plugin.", + Name: name, + Status: StatusWarn, + Detail: fmt.Sprintf("%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)", CantVerifyFreeCompute, gpuName, req), + Remedy: "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to verify free capacity. If GPU training is expected, also ensure one node has both the compute and the GPU capacity, with its device plugin.", + CantCheck: true, } } return Result{ @@ -1103,15 +1119,17 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s if !freeKnown { return Result{ Name: name, - // DISTINCT can't-check PREFIX so the rollup (summarizeDoctor in - // cli/doctor.go) classifies this as "couldn't check free compute" - // rather than greening it: it matches Node-capacity can't-checks by - // prefix, and "a Ready node can schedule..." would fall through to - // "Ready to run training" at exit 0 (Bugbot High). Keep the - // "allocatable only" phrase the caveat and its test rely on. - Status: StatusWarn, - Detail: CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here (the node fits the envelope on allocatable: " + req + ")", - Remedy: "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to verify free capacity.", + // CAN'T-CHECK: the pod list was unreadable, so the fit above was + // against allocatable, not free. CantCheck routes this to the + // rollup's Unknown tier (summarizeDoctor, backend#3282); without it + // "a Ready node can schedule..." would green at exit 0 (Bugbot High). + // The CantVerifyFreeCompute prefix and the "allocatable only" phrase + // stay for the --verbose detail the caveat and its test rely on — they + // are no longer what the rollup classifies on. + Status: StatusWarn, + Detail: CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here (the node fits the envelope on allocatable: " + req + ")", + Remedy: "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to verify free capacity.", + CantCheck: true, } } return Result{Name: name, Status: StatusOK, Detail: detail} @@ -1191,16 +1209,17 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel 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). + // be resolved — a can't-check, not a clean result. Like the unreadable-secret + // path below it sets CantCheck, so the rollup drops BOTH to the Unknown tier; + // without it 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: CantReadImagePullSecret + ": couldn't read jobs-manager to resolve it — skipping", - Remedy: "Check a tracebloc client is installed in " + ns + ".", + Name: name, + Status: StatusWarn, + Detail: CantReadImagePullSecret + ": couldn't read jobs-manager to resolve it — skipping", + Remedy: "Check a tracebloc client is installed in " + ns + ".", + CantCheck: true, } } secrets := dep.Spec.Template.Spec.ImagePullSecrets @@ -1216,13 +1235,14 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel // 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. + // can't-check is honest: a StatusWarn that sets CantCheck so the rollup + // drops it 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 + ").", + 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 + ").", + CantCheck: true, } } return Result{ diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 6d3b2f5..ec9de8b 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -373,8 +373,8 @@ func TestCheckPVC(t *testing.T) { 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) + if r := checkPVC(bg(), unreadable, ns); r.Status != StatusWarn || !r.CantCheck || !strings.HasPrefix(r.Detail, cluster.PVCReadErrPrefix) { + t.Fatalf("unreadable PVC => %v CantCheck=%v (%q), want a can't-check Warn (marker set) with the read-err prefix", r.Status, r.CantCheck, r.Detail) } } @@ -837,6 +837,12 @@ func TestCheckNodeFitFreeMemory(t *testing.T) { if r.Status != StatusWarn || !strings.HasPrefix(r.Detail, HeldByRunningJob) { t.Fatalf("=> %v (%q), want the transient Warn with prefix %q", r.Status, r.Detail, HeldByRunningJob) } + // A running job holding the room is a real soft finding, NOT a can't-check — + // it must not set CantCheck, or the rollup would drop it to the Unknown tier + // instead of its own "waiting for it" Warn (backend#3282). + if r.CantCheck { + t.Errorf("a HeldByRunningJob Warn must not be marked CantCheck: %q", r.Detail) + } }) // The over-commit message names the SHORT dimension, not always memory (Bugbot). @@ -909,13 +915,14 @@ func TestCheckNodeFitFreeMemory(t *testing.T) { return true, nil, errors.New("pods is forbidden") }) r := checkNodeFit(bg(), cs, gpu) - if r.Status != StatusWarn { - t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) + if r.Status != StatusWarn || !r.CantCheck { + t.Fatalf("=> %v CantCheck=%v (%q), want a can't-check warn (marker set)", r.Status, r.CantCheck, r.Detail) } - // PREFIX, not Contains: that is what the rollup matches on, so a detail - // merely mentioning the phrase somewhere would still green the run. + // The prefix is now the --verbose detail wording (the rollup classifies on + // the CantCheck marker asserted above, not this string); keep asserting it so + // the "allocatable only" caveat text the producer emits does not drift. if !strings.HasPrefix(r.Detail, CantVerifyFreeCompute) { - t.Fatalf("detail must START with %q so summarizeDoctor classifies it as a can't-check, got %q", + t.Fatalf("detail must START with %q for the --verbose breakdown, got %q", CantVerifyFreeCompute, r.Detail) } // The soft GPU fact is not lost, it is just no longer the whole story. @@ -1322,11 +1329,11 @@ func TestCheckImagePull(t *testing.T) { 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 r.Status != StatusWarn || !r.CantCheck { + t.Fatalf("=> %v CantCheck=%v (%q), want a can't-check Warn (marker set) on a read failure", r.Status, r.CantCheck, 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) + t.Errorf("detail must carry the can't-read prefix for the --verbose breakdown, 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) @@ -1338,8 +1345,8 @@ func TestCheckImagePull(t *testing.T) { // 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) + if r.Status != StatusWarn || !r.CantCheck || !strings.HasPrefix(r.Detail, CantReadImagePullSecret) { + t.Fatalf("=> %v CantCheck=%v (%q), want a can't-check Warn (marker set) carrying the read prefix", r.Status, r.CantCheck, r.Detail) } }) }