From 043d152d2d98a63bec45edfd4aecdf1aaf8596d8 Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Sun, 6 Sep 2026 05:18:04 +0200 Subject: [PATCH 1/5] fix(doctor): the node-capacity check compares the free memory it already computes, and tells a permanent shortage from a transient one (backend#2870) (#639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(doctor): the node-capacity check compares the free memory it already computes, and tells a permanent shortage from a transient one (backend#2870) Node capacity: batch-Job pods (the `job-name` label the CLI already uses to find a training pod) go into their own per-node sum instead of being dropped. The steady-state fit still decides the PERMANENT Fail (OverCommitted, now with the node's free figures); a node that fits beside the platform but not beside its running Jobs is the new TRANSIENT Warn (HeldByRunningJob) with the opposite remedy — wait or stop the job, do not resize. The rollup gets a matching arm so the top line says the next run waits instead of a green. Machine capacity: requestedMemory returns its read error instead of 0, so an unreadable pod list renders "unrequested: unknown" and StatusUnknown rather than the whole VM as free under a check-mark. The over-commit Warn, which needs no pod list, still fires. VERSION 0.10.24 (v0.10.23 is tagged). Co-Authored-By: Claude Opus 4.8 * fix(doctor): a Pending pod beside a running job names the job, not "not enough compute" (backend#2870) Bugbot High on #639: the transient shortage's own symptom is a second training pod Pending until the running job frees the room, and the stuck-Pending Fail arm matched it first -- "Not ready ... not enough free compute" with `resources set max` at exit 2, in exactly the case the HeldByRunningJob Warn was written for. A combined arm now sits above the stuck-Pending arm: when Pod health reports a pod Pending past grace AND Node capacity has measured that a running job holds the room, the top line says the next training is waiting for the running one, the remedy says wait or stop the job and that resizing will not help, and -- since checkPods cannot see WHY a pod is Pending -- what to do if the wait outlives the job. Warn, not Fail: the inference "Pending so cannot schedule" is refuted by the measured cause, and a Fail would exit 2 on healthy training (the Bugbot High on #628). A Pod-health FAIL and the OverCommitted Fail still outrank it; a Pending pod with no running job keeps the generic Fail and its sizing advice. Mutation: moving the combined arm back below the stuck-Pending arm reddens the new both-signals test with Bugbot's exact output. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- VERSION | 2 +- internal/cli/doctor.go | 55 ++++- internal/cli/doctor_test.go | 100 +++++++++ .../cli/testdata/golden/zz-all-strings.golden | 12 +- internal/doctor/doctor.go | 132 ++++++++++-- internal/doctor/doctor_test.go | 190 +++++++++++++++++- internal/doctor/machine.go | 46 ++++- internal/doctor/machine_test.go | 55 +++++ 8 files changed, 560 insertions(+), 32 deletions(-) diff --git a/VERSION b/VERSION index 04fdd6d..211f7aa 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.23 +0.10.24 diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 26c9707..d11acf7 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -444,6 +444,16 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // was actually detected (Bugbot on #561/#566: "Warn rollup shadowed by // 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). + stuckPending := by["Pod health"].Status == doctor.StatusWarn && + !strings.HasPrefix(by["Pod health"].Detail, "could not list pods") + heldByJob := by["Node capacity"].Status == doctor.StatusWarn && + strings.HasPrefix(by["Node capacity"].Detail, doctor.HeldByRunningJob) + switch { // ── Fail: a real, training-blocking problem (worst wins) ── case by["Pod health"].Status == doctor.StatusFail: @@ -482,7 +492,34 @@ 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())} - case by["Pod health"].Status == doctor.StatusWarn && !strings.HasPrefix(by["Pod health"].Detail, "could not list pods"): + 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 + // running, the next training pod is Pending until it frees the room -- the + // `waiting_for_capacity` state the HeldByRunningJob Warn exists to name. + // Left to the arm below, the stuck-Pending Fail matched first and the top + // line read "Not ready ... not enough free compute" with `computeRemedy` + // (`resources set max`) at exit 2 -- in exactly the case this change was + // made for, the wait-for-the-job advice never appeared. + // + // A WARN ABOVE A FAIL, deliberately, and the reason has to be stated + // because the tiers below are ordered by severity: the stuck-Pending arm + // is not a measured failure but an INFERENCE ("Pending past grace, so it + // cannot schedule, so compute or image"). checkNodeFit has measured the + // 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. + // + // 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 + // what to do if the wait outlives the job rather than pretending the + // attribution is certain. + ready = healthLine{doctor.StatusWarn, + "Ready to run training — a training is already running, and the next one is waiting for it to finish.", + fmt.Sprintf("A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`.", launcher())} + case stuckPending: // Pods stuck Pending past the grace window (unschedulable / image can't // pull) mean training can't actually schedule — so this is NOT ready, even // though the granular Pod-health check rates it a softer ⚠. Without this, @@ -532,6 +569,22 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusWarn, "Ready to run training — but your environment thinks this machine is bigger than it is.", fmt.Sprintf("It reports more memory than the machine really has, so two trainings that each look like they fit can together run it out of memory and take the environment down. Run one training at a time; to fix it for good, recreate the environment as a single-node one. `%s doctor --verbose` shows the numbers and the exact flags.", launcher())} + case heldByJob: + // backend#2870, the TRANSIENT shortage with no pod Pending yet (the + // co-occurring case is the Warn in the Fail tier above). checkNodeFit found the envelope + // fits this machine beside the platform, but a job that is running holds + // the room now, so the next run waits. This arm exists because the two + // things an operator could otherwise be told are both wrong: the capacity + // Fail's advice (ask for less / grow the machine) changes nothing about a + // job already running, and the plain green hides why a second run is + // waiting. Warn keeps exit 0 -- training IS running -- while doctorVerdict + // withholds "everything looks good". Below the over-commit Warn: a machine + // that lies about its size is the more consequential finding. + // + // Plain terms, no Kubernetes vocabulary, like its neighbours. + 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"): diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index e731443..acd12ce 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -508,6 +508,106 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#2870: the TRANSIENT shortage. The envelope fits the machine beside + // the platform, but a running job holds the room, so the next run waits. It + // must roll up as a Warn that says so -- not the green (which hides why a + // second run is waiting), not a Fail (training IS running: the Bugbot High on + // #628), and never the capacity Fail's "ask for less / grow the machine" + // advice, which changes nothing about a job already running. + t.Run("a running job holding the room → ready Warn that says to wait, not resize", func(t *testing.T) { + // DETAIL BUILT FROM THE PRODUCER'S CONSTANT, same discipline as the + // over-commit cases above: the arm classifies by prefix. + _, r := summarizeDoctor(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"), tokenOK) + if r.status != doctor.StatusWarn { + t.Fatalf("want ready Warn, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "waits for it") { + t.Errorf("the top line should say the next run waits, got %q", r.text) + } + if strings.Contains(r.remedy, "resources set max") || strings.Contains(r.remedy, "Ask for less") { + t.Errorf("the transient remedy must not send the operator to resize or shrink: %q", r.remedy) + } + if !strings.Contains(r.remedy, "let the running job finish") { + t.Errorf("the remedy should say to wait for or stop the running job: %q", r.remedy) + } + c, _ := summarizeDoctor(allOK, tokenOK) + if v := doctorVerdict(c.status, r.status); v != doctor.StatusWarn { + t.Errorf("verdict must own the Warn (exit 0, no 'everything looks good'), got %v", v) + } + }) + + // Bugbot High on #639. The transient shortage's own SYMPTOM is a second + // training pod sitting Pending until the running job frees the room -- the + // `waiting_for_capacity` case. With the stuck-Pending Fail arm first, that + // state rolled up to "Not ready ... not enough free compute" with + // `computeRemedy` (`resources set max`) at exit 2, and the wait-for-the-job + // line never appeared in exactly the case it was written for. + t.Run("a Pending pod AND a running job holding the room → the cause is named, not the generic stuck-Pending Fail", 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]") + c, r := summarizeDoctor(results, tokenOK) + if r.status == doctor.StatusFail { + t.Fatalf("the stuck-Pending arm shadowed the transient cause and failed the command on healthy training: %q / %q", r.text, r.remedy) + } + if r.status != doctor.StatusWarn { + t.Fatalf("want ready Warn, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "waiting for it") { + t.Errorf("the top line should say the next run waits for the running one, got %q", r.text) + } + if !strings.Contains(r.remedy, "running job holds") { + t.Errorf("the remedy should name the cause -- a running job -- got %q", r.remedy) + } + if strings.Contains(r.remedy, "resources set max") || strings.Contains(r.remedy, "Ask for less") { + t.Errorf("the remedy must not send the operator to resize or shrink: %q", r.remedy) + } + if !strings.Contains(r.remedy, "still waiting after") { + t.Errorf("checkPods cannot see WHY a pod is Pending, so the remedy must say what to do if the wait outlives the job: %q", r.remedy) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusWarn { + t.Errorf("verdict must own the 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, + // measured-nowhere case and keeps its Fail and its sizing advice. + _, r := summarizeDoctor(withDetail(allOK, "Pod health", doctor.StatusWarn, + "Pending > 5m0s: [trainer-x]"), tokenOK) + if r.status != doctor.StatusFail || !strings.Contains(r.remedy, "resources set max") { + t.Errorf("want the generic stuck-Pending Fail with the sizing remedy, got %v %q", r.status, 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. + results := withDetail(allOK, "Node capacity", doctor.StatusWarn, + doctor.HeldByRunningJob+": a Ready node fits a training job beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now") + results = withDetail(results, "Pod health", doctor.StatusFail, "crash-looping: [jobs-manager]") + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail || !strings.Contains(r.text, "isn't running") { + t.Errorf("a Pod-health Fail must still win, got %v (%q)", r.status, r.text) + } + }) + + t.Run("a machine that lies about its size outranks a running job", func(t *testing.T) { + // Both are Warns; the over-commit is the more consequential finding and + // sits first. Pin the order so a later reshuffle cannot demote it. + 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 beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now") + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusWarn || !strings.Contains(r.text, "bigger than it is") { + t.Errorf("want the over-commit Warn first, got %v (%q)", r.status, r.text) + } + }) + t.Run("node capacity GPU-soft warn → still ready", func(t *testing.T) { _, r := summarizeDoctor(withDetail(allOK, "Node capacity", doctor.StatusWarn, "no single Ready node satisfies cpu+memory AND nvidia.com/gpu — GPU jobs rely on the CPU fallback (needs cpu=2, memory=8Gi)"), tokenOK) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 3fbed7c..e238a3e 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -57,7 +57,7 @@ screen. %s/%d are runtime placeholders. "%s (unreadable: %v)" "%s Bound, mounted at %s" "%s contains a NUL byte — the file is corrupt or not really a CSV. The cluster rejects it after the upload; re-export the file and re-run." -"%s for a training job (%s) but not beside what is already running on it — the envelope over-asks the node's FREE %s, so the pod schedules Pending" +"%s for a training job (%s) but not beside what is already running on it — the envelope over-asks the node's FREE %s (%s has %s free beside the platform's own pods), so the pod schedules Pending" "%s has a header but no data rows (0 ingestable records). Add at least one data row and re-run." "%s has duplicate column name(s): %s. Each column must be unique — the cluster rejects duplicates, and the schema would map onto the wrong column. Rename them and re-run." "%s is empty — add a header and at least one data row, then re-run" @@ -83,6 +83,7 @@ screen. %s/%d are runtime placeholders. "%s/%s" "%s: %v" "%s: %w" +"%s: a Ready node fits a training job (%s) beside the platform's own pods, but running job(s) on %s hold %s right now, so the next run waits Pending until they finish" "%s=%s,%s=%s" "%v (policy: %v)" "%w in namespace %q, but tracebloc clients are running in: %s. Pass --namespace to pick one." @@ -119,10 +120,12 @@ screen. %s/%d are runtime placeholders. "--time-column is time_to_event_prediction only; it doesn't apply to task %q" "--timeout has no effect without --wait or --seal" "--wait and --seal are separate modes — run them one at a time" +". Also, no single Ready node satisfies cpu+memory AND %s, so GPU jobs would rely on the CPU fallback" "0:%d" "3 GiB" "A dataset named %q already exists — replace it?" "A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade" +"A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`." "A real run continues with step 2 (copy into your secure environment) and step 3 (validate and load)." "A tracebloc client is already running on this cluster — adopting it. Couldn't read the cluster identity, so its idempotency anchor was left unchanged; point --kubeconfig/--context at a cluster where kube-system is readable to stamp it." "A training run is allocated up to:" @@ -201,7 +204,7 @@ screen. %s/%d are runtime placeholders. "Diagnose auth / cluster problems with: tracebloc doctor" "Do you want to change the allocation? Run `%s resources set` (guided walkthrough on a terminal)." "Do you want to ingest training or test data?" -"Docker VM %s (%d cpu) → %d node%s claiming %s → %s unrequested" +"Docker VM %s (%d cpu) → %d node%s claiming %s → %s" "Docker and related tools — remove them yourself if you no longer need them" "Dry run — nothing was changed" "Dry-run complete — your data and secure environment check out; nothing was created." @@ -213,6 +216,7 @@ screen. %s/%d are runtime placeholders. "Email it to support@tracebloc.io." "Email support@tracebloc.io with the output of `%s doctor --diagnose`." "Ensure your kubeconfig user can list nodes." +"Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to measure what is unrequested." "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to verify free capacity." "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." "Enter" @@ -290,6 +294,8 @@ screen. %s/%d are runtime placeholders. "Not yet in the CLI:" "Note: %d file(s) in images/ have no labels.csv row and won't be part of the dataset: %s" "Note: %d sequence(s) grouped by %q — the platform counts this dataset in sequences, not rows" +"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." +"Nothing on the machine needs changing: let the running job finish, or stop it if it is not needed (kubectl get jobs -A). Lowering RESOURCE_REQUESTS or resizing does not free room a running job holds." "Offboarded %q. This machine is no longer connected to tracebloc." "Offboarded %q: the machine credential is revoked, so it can no longer connect to tracebloc — but some cleanup above didn't complete. Finish the flagged steps by hand." "Only tracebloc's small jobs-manager restarts — running training isn't interrupted." @@ -314,6 +320,8 @@ screen. %s/%d are runtime placeholders. "Reading your files locally first — nothing has touched your secure environment yet — so a layout or settings problem shows up right away." "Ready for `tracebloc data ingest`." "Ready to run training" +"Ready to run training — a training is already running, and the next one is waiting for it to finish." +"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 free compute (run with --verbose)" diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 18b3ffd..76a8939 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -585,9 +585,19 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, // checkNodeFit verifies at least one Ready node can satisfy the resource // requests the jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS -// / GPU_REQUESTS env) — the "Pending forever, no node big enough" class. GPU is -// soft: when a GPU is requested but no node exposes it, that's a ⚠ (jobs-manager -// has a GPU→CPU fallback), not a hard failure. +// / GPU_REQUESTS env) — the "Pending forever, no node big enough" class. The +// fit is against what is FREE on a node, not its allocatable, and it tells two +// shortages apart (backend#2870): +// +// - PERMANENT (Fail, prefix OverCommitted): the envelope does not fit beside +// the platform's own steady-state pods. No run can ever schedule here until +// the envelope shrinks or the machine grows. +// - TRANSIENT (Warn, prefix HeldByRunningJob): it fits beside the platform, +// but a running batch Job holds the room right now. The next run waits; +// nothing on the machine needs changing. +// +// GPU is soft: when a GPU is requested but no node exposes it, that's a ⚠ +// (jobs-manager has a GPU→CPU fallback), not a hard failure. func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]string) Result { const name = "Node capacity" cpuReq, memReq, ok := parseCPUMem(env["RESOURCE_REQUESTS"]) @@ -633,6 +643,15 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // say so and fall back to allocatable, never silently pass it off as free. reqCPU := map[string]int64{} // node -> already-requested millicores reqMem := map[string]int64{} // node -> already-requested bytes + // Requests held by RUNNING batch Jobs, per node, kept APART from the + // steady-state sums above. They answer a different question: "beside the + // platform" is about whether a run can ever schedule here, "beside the + // platform and the job that is running" is about whether one can schedule + // NOW. Folding them together produced a false Fail during healthy training + // (Bugbot High on #628); dropping them produced a green over a machine whose + // next run was going to wait. Two sums, two verdicts. + jobCPU := map[string]int64{} // node -> millicores held by running Jobs + jobMem := map[string]int64{} // node -> bytes held by running Jobs freeKnown := true if pods, perr := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}); perr == nil { for i := range pods.Items { @@ -644,24 +663,26 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s p.Status.Phase == corev1.PodSucceeded || p.Status.Phase == corev1.PodFailed { continue } - // SKIP batch-Job pods (they carry the `job-name` label the batch/v1 - // controller stamps -- see internal/submit/watch.go). A running - // training or ingestion job holds the envelope itself, so counting it - // would make doctor report "no room for a training job" on the exact - // healthy state it exists to bless -- a false negative that exits 2 - // during training (Bugbot High). The question is whether the envelope - // fits beside the STEADY-STATE control plane (Deployments/DaemonSets), - // not beside a transient workload; those pods have no `job-name`. + // Batch-Job pods carry the `job-name` label the batch/v1 controller + // stamps (see internal/submit/watch.go) -- that label, not a name + // pattern, is how the CLI already recognises a training or ingestion + // pod, so it is the one used here. They go into the JOB sums, never the + // steady-state ones: a running job holds the envelope itself, so + // counting it as platform would make doctor Fail "no room for a training + // job" on the exact healthy state it exists to bless (Bugbot High). The + // steady state is the control plane (Deployments/DaemonSets), which has + // no `job-name`. + cpu, mem := reqCPU, reqMem if _, isJob := p.Labels["job-name"]; isJob { - continue + cpu, mem = jobCPU, jobMem } for j := range p.Spec.Containers { r := p.Spec.Containers[j].Resources.Requests if q, ok := r[corev1.ResourceCPU]; ok { - reqCPU[p.Spec.NodeName] += q.MilliValue() + cpu[p.Spec.NodeName] += q.MilliValue() } if q, ok := r[corev1.ResourceMemory]; ok { - reqMem[p.Spec.NodeName] += q.Value() + mem[p.Spec.NodeName] += q.Value() } } } @@ -670,6 +691,20 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s } var cpuMemFits, fullFits, allocOnlyFit, overCPU, overMem bool + // The first node that is big enough by allocatable but not by FREE, with the + // free figures it had -- so the permanent Fail can print the numbers the + // rollup's remedy promises ("--verbose shows the exact numbers") instead of + // only the request. Bounded to the node the verdict is about; the CPU-major + // bestFree* below may belong to a different node. + var overNode string + var overFreeCPUm, overFreeMemB int64 + // Whether some node fits the envelope beside the platform AND the Jobs + // running on it -- i.e. can a run schedule NOW, not just ever. When it fits + // the steady state but not this, the first such node and what its Jobs hold + // are recorded for the transient Warn. + var nowFits bool + var heldNode string + var heldCPUm, heldMemB int64 // Largest Ready node for the drift nudge — CPU-major with memory as the // tie-break, EXACTLY like resources.nodeLarger, so the advertised ceiling // always matches what `resources set max` will actually apply (Bugbot). @@ -733,6 +768,9 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s if freeMemB < memReq.Value() { overMem = true } + if (freeCPUm < cpuReq.MilliValue() || freeMemB < memReq.Value()) && overNode == "" { + overNode, overFreeCPUm, overFreeMemB = n.Name, freeCPUm, freeMemB + } } // Disk joins cpu+memory as a WHOLE-NODE condition. A pod gets every // resource it requests from ONE node, so this must be AND-ed into the @@ -758,6 +796,23 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s if nodeCPUMem && nodeGPU { fullFits = true } + // The same whole-node fit with the running Jobs subtracted as well: can a + // run schedule NOW? Asked only of a node that passed the steady-state fit + // -- on one that did not, the permanent shortage is the finding and the + // Job on it is not what is in the way. With free unknown there is no Job + // sum either, so this collapses to the steady-state answer and the + // `!freeKnown` arms below carry the caveat. + nodeNow := nodeCPUMem + if nodeCPUMem && freeKnown { + nodeNow = freeCPUm-jobCPU[n.Name] >= cpuReq.MilliValue() && + freeMemB-jobMem[n.Name] >= memReq.Value() + if !nodeNow && heldNode == "" { + heldNode, heldCPUm, heldMemB = n.Name, jobCPU[n.Name], jobMem[n.Name] + } + } + if nodeNow { + nowFits = true + } } switch { @@ -775,7 +830,7 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s return Result{ Name: name, Status: StatusFail, - Detail: fmt.Sprintf("%s for a training job (%s) but not beside what is already running on it — the envelope over-asks the node's FREE %s, so the pod schedules Pending", OverCommitted, req, short), + Detail: fmt.Sprintf("%s for a training job (%s) but not beside what is already running on it — the envelope over-asks the node's FREE %s (%s has %s free beside the platform's own pods), so the pod schedules Pending", OverCommitted, req, short, overNode, cpuMemString(overFreeCPUm, overFreeMemB)), Remedy: "Lower RESOURCE_REQUESTS on jobs-manager to leave room for the platform's own pods, or move the control plane / add a node. The installer sizes the envelope from allocatable, not free, so a machine that is 'big enough' can still be over-committed (backend#2870).", } case !cpuMemFits: @@ -789,6 +844,36 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Detail: detail, Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } + case freeKnown && !nowFits: + // The TRANSIENT shortage (backend#2870): every dimension fits beside the + // platform's own pods, so this machine CAN run the envelope -- but a batch + // Job holds the room at this moment, and the next run sits Pending until + // it finishes. Before this arm the running Job was dropped from the sum and + // this state read as an unqualified green, which is the doctor-side half of + // the ticket's "waiting_for_capacity forever looks identical to a permanent + // shortage". + // + // Warn, not Fail: training is genuinely running here, and failing the + // command on the healthy state it exists to bless was the Bugbot High on + // #628. Warn, not OK: the operator asking why a second run is waiting + // needs the answer on this line, not a ✔. + // + // Its remedy is the OPPOSITE of the permanent arm's -- lowering the + // envelope or resizing changes nothing about a Job that is already + // running -- which is why the prefix is a distinct constant that the + // rollup (summarizeDoctor) classifies on. It sits above the soft GPU Warn + // because it is the stronger statement about scheduling; the GPU fact is + // folded in rather than lost, as #628 did for the can't-check. + detail := fmt.Sprintf("%s: a Ready node fits a training job (%s) beside the platform's own pods, but running job(s) on %s hold %s right now, so the next run waits Pending until they finish", HeldByRunningJob, req, heldNode, cpuMemString(heldCPUm, heldMemB)) + if gpuRequested && !fullFits { + detail += fmt.Sprintf(". Also, no single Ready node satisfies cpu+memory AND %s, so GPU jobs would rely on the CPU fallback", gpuName) + } + return Result{ + Name: name, + Status: StatusWarn, + Detail: detail, + Remedy: "Nothing on the machine needs changing: let the running job finish, or stop it if it is not needed (kubectl get jobs -A). Lowering RESOURCE_REQUESTS or resizing does not free room a running job holds.", + } case gpuRequested && !fullFits: // UNKNOWN FREE OUTRANKS THE SOFT GPU WARN (Bugbot High, #628). // @@ -914,6 +999,23 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // #628). The generic arm is right for a too-small machine and wrong here. const OverCommitted = "a Ready node is large enough" +// HeldByRunningJob is the prefix of the #2870 TRANSIENT Warn: the envelope fits +// this machine beside the platform, but a running batch Job holds the room now. +// ONE definition, for the same reason as OverCommitted -- the rollup classifies +// on it, and its remedy ("wait, or stop the job") is the opposite of both +// capacity Fails' ("shrink the envelope" / "grow the machine"). A retyped copy +// that drifted would send this state down the generic path again. +const HeldByRunningJob = "a running job holds the room" + +// 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. +func cpuMemString(cpuMilli, memBytes int64) string { + return fmt.Sprintf("cpu=%s, memory=%s", + resource.NewMilliQuantity(cpuMilli, resource.DecimalSI).String(), + resource.NewQuantity(memBytes, resource.BinarySI).String()) +} + // CantVerifyFreeCompute is the prefix every "we could not check free compute" // Node-capacity Warn must start with, and the ONE definition of it. // diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 3248e63..b44c10b 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -3,6 +3,7 @@ package doctor import ( "context" "errors" + "fmt" "strings" "testing" "time" @@ -809,13 +810,20 @@ func TestCheckNodeFitFreeMemory(t *testing.T) { }) // A RUNNING training job (batch/v1 job-name label) holds the envelope itself. - // It must NOT count against free, or doctor fails on the exact healthy state it - // blesses (Bugbot High). Same 12Gi neighbour, but labelled a Job -> still ok. - t.Run("a running training job is excluded, not counted -> ok", func(t *testing.T) { + // It must NOT be counted as platform, or doctor FAILS (exit 2) on the exact + // healthy state it blesses (Bugbot High on #628). Same 12Gi neighbour, but + // labelled a Job -> the TRANSIENT Warn, never the Fail: the machine can run + // the envelope, a job simply holds the room now. Before this PR the job was + // dropped from the sum entirely and this read as an unqualified OK. + t.Run("a running training job is the transient Warn, never the Fail", func(t *testing.T) { job := podOn("train-sim", "n1", "", "12Gi", map[string]string{"job-name": "exp-42"}) cs := fake.NewClientset(node("n1", "4", "16Gi"), job) - if r := checkNodeFit(bg(), cs, req); r.Status != StatusOK { - t.Fatalf("=> %v (%q), want ok (training job excluded)", r.Status, r.Detail) + r := checkNodeFit(bg(), cs, req) + if r.Status == StatusFail { + t.Fatalf("=> Fail (%q): a running job must never read as a capacity failure", r.Detail) + } + 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) } }) @@ -923,6 +931,178 @@ func TestCheckNodeFitFreeMemory(t *testing.T) { }) } +// backend#2870 DoD 3: the check compares the free figure and tells a PERMANENT +// shortage (the envelope cannot fit beside the platform's own pods -- ever) from +// a TRANSIENT one (it fits, but a running Job holds the room now). Fixtures use +// the ticket's measured 8 GiB reproduction: a k3d node claiming 8126672Ki +// (7.75 GiB), the installer's envelope of allocatable − 3 GiB, and a control +// plane requesting 3008Mi beside 140Mi of k3s system pods -- 3148Mi against a +// 3072Mi overhead constant, so the envelope over-asks by 76Mi. +func TestCheckNodeFitPermanentVsTransient(t *testing.T) { + const ( + nodeMem = "8126672Ki" + nodeMemBytes = int64(8126672) * 1024 + envelopeBytes = nodeMemBytes - 3*(1<<30) // what the installer writes + ) + envelope := map[string]string{"RESOURCE_REQUESTS": fmt.Sprintf("cpu=1,memory=%d", envelopeBytes)} + envelopeGPU := map[string]string{ + "RESOURCE_REQUESTS": envelope["RESOURCE_REQUESTS"], + "GPU_REQUESTS": "nvidia.com/gpu=1", + } + // Steady-state platform on the node: the chart's control plane + k3s system. + platform := func(n string) []runtime.Object { + return []runtime.Object{cpPod("control-plane", n, "3008Mi"), cpPod("k3s-system", n, "140Mi")} + } + // A smaller platform that leaves room for exactly one envelope. + smallPlatform := func(n string) []runtime.Object { + return []runtime.Object{cpPod("control-plane", n, "2000Mi")} + } + trainingJob := func(n, mem string) *corev1.Pod { + return podOn("train-sim", n, "1", mem, map[string]string{"job-name": "exp-42"}) + } + // A running pod that holds the envelope but is NOT a Job -- e.g. a Deployment + // someone added. It is platform, not transient, and must read as permanent. + forbidden := func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("pods is forbidden") + } + + cases := []struct { + name string + objects []runtime.Object + env map[string]string + podsBroken bool + want Status + prefix string // Detail must START with this (the rollup classifies on it) + contains []string + notContain []string + }{ + { + name: "fits beside the platform, nothing running -> ok", + objects: append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + env: envelope, + want: StatusOK, + }, + { + name: "the ticket: 3148Mi platform beside an allocatable-3GiB envelope -> permanent Fail", + objects: append([]runtime.Object{node("n1", "4", nodeMem)}, platform("n1")...), + env: envelope, + want: StatusFail, + prefix: OverCommitted, + contains: []string{"FREE memory", "n1 has", "free beside the platform"}, + // The permanent shape must not be blamed on a job that is not there. + notContain: []string{HeldByRunningJob}, + }, + { + name: "fits the machine, but a running job holds it -> transient Warn", + objects: append(append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + trainingJob("n1", fmt.Sprintf("%d", envelopeBytes))), + env: envelope, + want: StatusWarn, + prefix: HeldByRunningJob, + contains: []string{"running job(s) on n1 hold", "cpu=1", "waits Pending"}, + // And the remedy must not be the permanent one. + notContain: []string{OverCommitted, "over-asks"}, + }, + { + name: "transient on cpu alone is still the transient Warn", + // Platform leaves 4 cpu; a job holds 3 of them; envelope wants 1 -> 1 free... make it 4. + objects: append(append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + podOn("train-sim", "n1", "4", "", map[string]string{"job-name": "exp-42"})), + env: envelope, + want: StatusWarn, + prefix: HeldByRunningJob, + contains: []string{"cpu=4"}, + }, + { + name: "a job on ANOTHER node leaves this one free -> ok", + objects: append(append([]runtime.Object{node("n1", "4", nodeMem), node("n2", "4", nodeMem)}, + smallPlatform("n1")...), trainingJob("n2", fmt.Sprintf("%d", envelopeBytes))), + env: envelope, + want: StatusOK, + }, + { + name: "permanent and a running job together -> permanent wins, the job is not blamed", + objects: append(append([]runtime.Object{node("n1", "4", nodeMem)}, platform("n1")...), + trainingJob("n1", "500Mi")), + env: envelope, + want: StatusFail, + prefix: OverCommitted, + notContain: []string{HeldByRunningJob}, + }, + { + name: "a non-Job pod holding the envelope is platform, so permanent", + objects: append(append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + podOn("someones-deployment", "n1", "", fmt.Sprintf("%d", envelopeBytes), nil)), + env: envelope, + want: StatusFail, + prefix: OverCommitted, + }, + { + name: "transient with a GPU requested and absent -> transient wins, GPU fact kept", + objects: append(append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + trainingJob("n1", fmt.Sprintf("%d", envelopeBytes))), + env: envelopeGPU, + want: StatusWarn, + prefix: HeldByRunningJob, + contains: []string{"nvidia.com/gpu", "CPU fallback"}, + }, + { + name: "pod list unreadable -> not ok, and says free was not verified", + objects: []runtime.Object{node("n1", "4", nodeMem)}, + env: envelope, + podsBroken: true, + want: StatusWarn, + prefix: CantVerifyFreeCompute, + contains: []string{"allocatable only"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cs := fake.NewClientset(tc.objects...) + if tc.podsBroken { + cs.PrependReactor("list", "pods", forbidden) + } + r := checkNodeFit(bg(), cs, tc.env) + if r.Status != tc.want { + t.Fatalf("=> %v (%q), want %v", r.Status, r.Detail, tc.want) + } + if tc.prefix != "" && !strings.HasPrefix(r.Detail, tc.prefix) { + t.Fatalf("detail must START with %q (the rollup classifies on it), got %q", tc.prefix, r.Detail) + } + for _, s := range tc.contains { + if !strings.Contains(r.Detail, s) { + t.Errorf("detail should contain %q, got %q", s, r.Detail) + } + } + for _, s := range tc.notContain { + if strings.Contains(r.Detail, s) { + t.Errorf("detail must not contain %q, got %q", s, r.Detail) + } + } + if r.Status != StatusOK && r.Remedy == "" { + t.Errorf("a non-OK verdict must carry a remedy: %q", r.Detail) + } + }) + } + + // The permanent and transient remedies point in OPPOSITE directions, so pin + // that they are not the same text and that each says its own thing. + t.Run("the two remedies are opposites", func(t *testing.T) { + perm := checkNodeFit(bg(), fake.NewClientset(append([]runtime.Object{node("n1", "4", nodeMem)}, platform("n1")...)...), envelope) + trans := checkNodeFit(bg(), fake.NewClientset(append(append([]runtime.Object{node("n1", "4", nodeMem)}, smallPlatform("n1")...), + trainingJob("n1", fmt.Sprintf("%d", envelopeBytes)))...), envelope) + if perm.Remedy == trans.Remedy { + t.Fatalf("permanent and transient share a remedy: %q", perm.Remedy) + } + if !strings.Contains(perm.Remedy, "Lower RESOURCE_REQUESTS") { + t.Errorf("permanent remedy should say to shrink the envelope or grow the machine: %q", perm.Remedy) + } + if !strings.Contains(trans.Remedy, "let the running job finish") || !strings.Contains(trans.Remedy, "does not free room") { + t.Errorf("transient remedy should say wait/stop and that resizing does not help: %q", trans.Remedy) + } + }) +} + func dockerSecret(name string, data []byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, diff --git a/internal/doctor/machine.go b/internal/doctor/machine.go index 7a2584f..837f6f8 100644 --- a/internal/doctor/machine.go +++ b/internal/doctor/machine.go @@ -254,21 +254,37 @@ func checkMachineChain(ctx context.Context, cs kubernetes.Interface, serverURL s // VM would be this check repeating the very lie it is here to expose — the // fourth level inheriting the error from the third. min() is a no-op on an // honest cluster, where the sum never exceeds the VM. + // + // This figure is DISPLAYED here and COMPARED in checkNodeFit, deliberately + // (backend#2870). A pod takes every resource it requests from ONE node, so the + // schedulability question is per node, not per VM: on a capped two-node + // cluster the VM can have 4 GiB unrequested while no single node has 2 free, + // and a VM-level "fits" would be wrong in the direction that goes Pending. + // checkNodeFit asks it per node, tells a permanent shortage from a transient + // one, and is what the rollup reads. What this level owes the operator is + // honesty about the number: when the pod list cannot be read, "unrequested" + // is not measured, and printing the whole VM as free -- which is what a + // silent 0 did -- is the same fail-open the ticket is about. trueCeiling := sumMem if vmMem < trueCeiling { trueCeiling = vmMem } - freeMem := trueCeiling - requestedMemory(ctx, cs) + requested, reqErr := requestedMemory(ctx, cs) + freeMem := trueCeiling - requested if freeMem < 0 { freeMem = 0 } + unrequested := gib(freeMem) + " unrequested" + if reqErr != nil { + unrequested = "unrequested: unknown (pod list unreadable)" + } chain := "" if _, hostMem, herr := host(ctx); herr == nil && hostMem > 0 { chain = fmt.Sprintf("host %s → ", gib(hostMem)) } - chain += fmt.Sprintf("Docker VM %s (%d cpu) → %d node%s claiming %s → %s unrequested", - gib(vmMem), vmCores, len(ready), plural(len(ready)), gib(sumMem), gib(freeMem)) + chain += fmt.Sprintf("Docker VM %s (%d cpu) → %d node%s claiming %s → %s", + gib(vmMem), vmCores, len(ready), plural(len(ready)), gib(sumMem), unrequested) if float64(sumMem) > float64(vmMem)*overCommitTolerance { ratio := float64(sumMem) / float64(vmMem) @@ -280,6 +296,19 @@ func checkMachineChain(ctx context.Context, cs kubernetes.Interface, serverURL s } } + // The invariant held (nodes + VM needed no pod list), but the fourth level + // was not measured. A ✔ over a chain whose last figure is "unknown" would be + // a green this check cannot back -- the same rule checkNodeFit applies to + // the same unreadable list, one layer out. + if reqErr != nil { + return Result{ + Name: name, + Status: StatusUnknown, + Detail: chain + " — could not list pods: " + reqErr.Error(), + Remedy: "Ensure your kubeconfig user can list pods cluster-wide, then re-run doctor to measure what is unrequested.", + } + } + return Result{Name: name, Status: StatusOK, Detail: chain} } @@ -296,12 +325,13 @@ func allK3d(nodes []corev1.Node) bool { } // requestedMemory sums memory requests across pods that still hold resources. -// Best-effort: on a read failure it returns 0, so the "unrequested" level -// degrades to the full claim rather than reporting a negative remainder. -func requestedMemory(ctx context.Context, cs kubernetes.Interface) int64 { +// A read failure is returned, not swallowed: the old best-effort 0 made the +// "unrequested" level print the whole VM as free on exactly the clusters where +// nothing had been measured (backend#2870). +func requestedMemory(ctx context.Context, cs kubernetes.Interface) (int64, error) { pods, err := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}) if err != nil { - return 0 + return 0, err } var total int64 for i := range pods.Items { @@ -317,7 +347,7 @@ func requestedMemory(ctx context.Context, cs kubernetes.Interface) int64 { } } } - return total + return total, nil } func gib(b int64) string { diff --git a/internal/doctor/machine_test.go b/internal/doctor/machine_test.go index be96cef..d99cc2b 100644 --- a/internal/doctor/machine_test.go +++ b/internal/doctor/machine_test.go @@ -10,7 +10,9 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" ) // The numbers this ticket was measured on: k3d v5.9.0 / k3s v1.35.5 / Docker @@ -328,6 +330,59 @@ func TestMachineChain_OverRequestedNeverGoesNegative(t *testing.T) { } } +// ── backend#2870: the fourth level must not print a figure it did not measure ─ + +func podsForbidden(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("pods is forbidden") +} + +func TestMachineChain_UnreadablePodListIsNotAGreen(t *testing.T) { + // requestedMemory used to swallow a list failure into 0, so the chain + // printed the WHOLE VM as "unrequested" -- the most optimistic figure + // possible -- on exactly the clusters where nothing had been measured, and + // the check went ✔. The ticket is about a computed-and-not-compared free + // figure; a computed-from-nothing one is the same fail-open. + cs := fake.NewClientset(k3dNode("k3d-t-server-0", "10", measuredNodeMem)) + cs.PrependReactor("list", "pods", podsForbidden) + got := checkMachineChain(bg(), cs, localAPI, fixedVM(measuredVMCores, measuredVMMem), noHost()) + if got.Status == StatusOK { + t.Fatalf("unreadable pod list => OK (%q); the free level was not measured", got.Detail) + } + if got.Status != StatusUnknown { + t.Fatalf("unreadable pod list => %v (%q), want unknown", got.Status, got.Detail) + } + if !strings.Contains(got.Detail, "pods is forbidden") { + t.Errorf("detail should carry the read error, got %q", got.Detail) + } + if strings.Contains(got.Detail, "7.75 GiB unrequested") { + t.Errorf("the whole VM was printed as free without a measurement: %q", got.Detail) + } + if !strings.Contains(got.Detail, "unknown") { + t.Errorf("the fourth level should read as unknown, got %q", got.Detail) + } + if got.Remedy == "" { + t.Error("an unmeasured level should say how to make it measurable") + } +} + +func TestMachineChain_OverCommitStillWarnsWithoutAPodList(t *testing.T) { + // The invariant this check exists for (sum(node capacity) <= VM) needs no + // pod list. An unreadable one must not demote a found double-count to + // "unknown" -- the same no-shadowing rule the rollup follows. + cs := fake.NewClientset( + k3dNode("k3d-tracebloc-server-0", "10", measuredNodeMem), + k3dNode("k3d-tracebloc-agent-0", "10", measuredNodeMem), + ) + cs.PrependReactor("list", "pods", podsForbidden) + got := checkMachineChain(bg(), cs, localAPI, fixedVM(measuredVMCores, measuredVMMem), noHost()) + if got.Status != StatusWarn { + t.Fatalf("double-count with unreadable pods => %v (%q), want warn", got.Status, got.Detail) + } + if !strings.Contains(got.Detail, "2.00×") || !strings.Contains(got.Detail, "unknown") { + t.Errorf("want the ratio AND the unknown fourth level, got %q", got.Detail) + } +} + // ── Bugbot on #541: a remote k3d cluster is not this machine ───────────────── func TestMachineChain_RemoteK3dClusterGetsNoVerdict(t *testing.T) { From 20809220718173f28e99d598636e7e8c1d0b27de Mon Sep 17 00:00:00 2001 From: lukasWuttke <54042461+LukasWodka@users.noreply.github.com> Date: Sun, 6 Sep 2026 05:28:29 +0200 Subject: [PATCH 2/5] ci(version-bump-gate): realign publish-paths with repos.yml (backend#2953) (#640) Co-authored-by: Claude Opus 4.8 --- .github/workflows/version-bump-gate-caller.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/version-bump-gate-caller.yml b/.github/workflows/version-bump-gate-caller.yml index ae91e73..313939d 100644 --- a/.github/workflows/version-bump-gate-caller.yml +++ b/.github/workflows/version-bump-gate-caller.yml @@ -19,5 +19,5 @@ jobs: uses: tracebloc/.github/.github/workflows/version-bump-gate.yml@main with: version-file: "VERSION" - publish-paths: "cmd/* internal/* go.mod go.sum VERSION" + publish-paths: "cmd/* internal/* go.mod go.sum VERSION scripts/install.sh scripts/install.ps1" soft-fail: false From 66b73d0ab7ed855bc54daa91de95d103628f3221 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 11:12:17 +0200 Subject: [PATCH 3/5] fix(doctor): don't treat an assigned-but-Pending training pod as a running job (backend#3247) (#642) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(doctor): don't treat an assigned-but-Pending training pod as a running job (backend#3247) checkNodeFit counted every non-terminal `job-name` pod with a NodeName into the "a running job holds the room" sum. A training pod scheduled to a node but still Pending (ImagePullBackOff / ContainerCreating) was therefore reported as HeldByRunningJob, and combined with the same pod showing up stuck-Pending in Pod health, summarizeDoctor rolled it up to "a training is already running, wait for it to finish" at exit 0 — on a pod that is wedged and never will. The operator waits forever on a dead pod. A Pending pod (even with a NodeName) is not running: - checkNodeFit routes a `job-name` pod into the running-job sums only when it is genuinely Running; an assigned-but-Pending one feeds neither sum, so it can no longer emit HeldByRunningJob. - past the same grace window checkPods uses, such a pod is recorded and surfaced as an actionable Fail (new StuckJobPod prefix) that names its real state (ImagePullBackOff / ContainerCreating / Pending) and says to inspect the pod, not wait. A run legitimately waiting for a running job has no NodeName, so it is never mistaken for a wedged one. - summarizeDoctor gains a matching arm above the inferred stuck-Pending arm (measured beats inferred), so the wedged pod fails at exit 2 with the right remedy instead of the "wait for the running job" Warn at exit 0. The genuinely-running case is unchanged. Tests pin each pod state (Running vs assigned-Pending vs ImagePullBackOff/ContainerCreating vs within-grace) at both the checkNodeFit and summarizeDoctor levels. Co-Authored-By: Claude Opus 4.8 * fix(doctor): scope the stuck-pod Fail to wedged reasons; keep the rollup plain-terms (backend#3247) Addresses Saqlain's review on cli#642: - summarizeDoctor's stuck-pod ready line no longer names `kubectl` — it ends at "… names the pod", restoring the plain-terms invariant the function documents three times. The kubectl form stays in the granular checkNodeFit remedy, one --verbose away. - checkNodeFit escalates to the StuckJobPod Fail only for a genuinely-wedged waiting reason (ImagePullBackOff / ErrImagePull / ErrImageNeverPull / InvalidImageName / CreateContainerConfigError / CreateContainerError), via a new stuckReasonWedged allowlist. A pod still pulling/creating past the grace window (ContainerCreating / Pulling / bare Pending) is left to checkPods' age-based inference, so "waiting will not clear it" is only asserted where the kubelet has actually reported a failure — a large first pull on a cold node no longer trips the confident Fail. Tests updated to pin wedged reasons (→ stuck Fail) apart from still-progressing ones (→ deferred, node OK), and to guard the rolled-up line against Kubernetes vocabulary. Copy-catalog golden regenerated. go build/vet/test ./... green. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- internal/cli/doctor.go | 25 +++ internal/cli/doctor_test.go | 56 +++++++ .../cli/testdata/golden/zz-all-strings.golden | 5 + internal/doctor/doctor.go | 140 ++++++++++++++++- internal/doctor/doctor_test.go | 147 ++++++++++++++++++ 5 files changed, 367 insertions(+), 6 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index d11acf7..77f99a8 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -519,6 +519,31 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusWarn, "Ready to run training — a training is already running, and the next one is waiting for it to finish.", fmt.Sprintf("A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`.", launcher())} + case by["Node capacity"].Status == doctor.StatusFail && + strings.HasPrefix(by["Node capacity"].Detail, doctor.StuckJobPod): + // A training pod is scheduled to a node but stuck Pending -- an image pull + // backing off, or a container stuck creating -- NOT running (backend#3247). + // checkNodeFit used to count it as a running job holding the room, so with + // a pod also stuck Pending this rolled up through `stuckPending && heldByJob` + // to the transient "a training is already running, wait for it" Warn at + // exit 0 -- on a pod that is wedged and never will. + // + // IT SITS ABOVE THE STUCK-PENDING ARM by the same measured-beats-inferred + // rule that puts `stuckPending && heldByJob` there. checkNodeFit escalates + // to this Fail ONLY for a genuinely-wedged reason -- an image pull backing + // off or a create error -- so the cause is measured, not inferred: "waiting + // will not clear it" is exact. A pod merely still pulling/creating past the + // 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 + // `--verbose` away -- and PLAIN TERMS, no Kubernetes vocabulary, like its + // neighbours (the granular checkNodeFit remedy carries the `kubectl` form). + ready = healthLine{doctor.StatusFail, + "Not ready — a training pod is stuck starting and isn't running yet.", + fmt.Sprintf("A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod.", launcher())} case stuckPending: // Pods stuck Pending past the grace window (unschedulable / image can't // pull) mean training can't actually schedule — so this is NOT ready, even diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index acd12ce..cadd225 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -583,6 +583,62 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3247: the defect combination. A training pod is scheduled but + // wedged on an image pull (Node capacity Fail, StuckJobPod) and Pod health + // also sees it Pending past grace (stuckPending). This used to roll up through + // `stuckPending && heldByJob` to "a training is already running, wait for it + // to finish" at exit 0 -- because checkNodeFit mislabelled the wedged pod a + // running job. Now the Node-capacity Fail is a MEASURED cause and outranks the + // inferred stuck-Pending arm: a Fail that says the pod is stuck, whose remedy + // is to inspect the pod, not to wait or resize. + t.Run("a scheduled-but-stuck training pod is a Fail, not the wait-for-a-running-job Warn", func(t *testing.T) { + results := withDetail(allOK, "Node capacity", doctor.StatusFail, + doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ImagePullBackOff). The next run does not wait on a pod that is not running") + results = withDetail(results, "Pod health", doctor.StatusWarn, + "Pending > 5m0s: [train-stuck]") + c, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a wedged training pod must fail the rollup, got %v (%q)", r.status, r.text) + } + if strings.Contains(r.text, "already running") || strings.Contains(r.text, "waiting for it") { + t.Errorf("must not tell the operator to wait on a job that is not running: %q", r.text) + } + if !strings.Contains(r.text, "stuck starting") { + t.Errorf("the top line should say the pod is stuck starting, got %q", r.text) + } + if strings.Contains(r.remedy, "resources set max") || strings.Contains(r.remedy, "Ask for less") { + t.Errorf("resizing does not clear an image-pull stall; the remedy must not send them there: %q", r.remedy) + } + // PLAIN TERMS: this rolled-up line must carry no Kubernetes vocabulary -- + // the `kubectl` form lives in the granular checkNodeFit remedy, one + // `--verbose` away (the invariant summarizeDoctor documents three times). + if strings.Contains(r.remedy, "kubectl") { + t.Errorf("the rolled-up remedy must stay plain-terms, no `kubectl`: %q", r.remedy) + } + if !strings.Contains(r.remedy, "--verbose") { + t.Errorf("the remedy should point at --verbose to name the pod, got %q", r.remedy) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("verdict must be a Fail (exit 2), not a clean pass, got %v", v) + } + }) + + // The same measured Fail with Pod health NOT flagging it (checkPods could be + // scoped to a namespace that missed it, or unable to list). The dedicated arm + // must still fire on the Node-capacity signal alone -- never falling through + // to a green. + t.Run("a scheduled-but-stuck training pod fails even when Pod health is silent", func(t *testing.T) { + results := withDetail(allOK, "Node capacity", doctor.StatusFail, + doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ErrImagePull). The next run does not wait on a pod that is not running") + c, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail || !strings.Contains(r.text, "stuck starting") { + t.Fatalf("want the stuck-pod Fail on the Node-capacity signal alone, got %v (%q)", r.status, r.text) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("verdict must be a Fail (exit 2), got %v", v) + } + }) + 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 e238a3e..926ee30 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -81,6 +81,8 @@ screen. %s/%d are runtime placeholders. "%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" +"%s/%s on %s (%s)" +"%s: %s. The next run does not wait on a pod that is not running" "%s: %v" "%s: %w" "%s: a Ready node fits a training job (%s) beside the platform's own pods, but running job(s) on %s hold %s right now, so the next run waits Pending until they finish" @@ -127,6 +129,7 @@ screen. %s/%d are runtime placeholders. "A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade" "A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`." "A real run continues with step 2 (copy into your secure environment) and step 3 (validate and load)." +"A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod." "A tracebloc client is already running on this cluster — adopting it. Couldn't read the cluster identity, so its idempotency anchor was left unchanged; point --kubeconfig/--context at a cluster where kube-system is readable to stamp it." "A training run is allocated up to:" "Add --help to any command for the flags." @@ -251,6 +254,7 @@ screen. %s/%d are runtime placeholders. "Ingestion summary" "Ingestor SA token" "Ingests a local dataset into your secure environment's storage,\nsubmits the ingestion run, and follows it to completion (streaming\nprogress + the final summary). Your data never leaves your own\ninfrastructure. Supports %[1]d tasks across the image, text, and\ntabular / time-series families; pick one with --task.\n\n is the data itself. What it looks like depends on the task:\n\n tabular / time-series — the dataset is a single CSV. Pass the .csv\n file directly, or a folder holding exactly one .csv:\n\n churn.csv (the .csv file itself)\n or\n churn/\n data.csv (the one .csv in the folder)\n\n image classification / keypoint detection — a folder with\n labels.csv + an images/ subfolder:\n\n cats_dogs/\n labels.csv (required)\n images/ (required)\n 001.jpg\n ...\n\n object detection — a folder with images/ + annotations/ and NO\n labels.csv: records are enumerated from the Pascal-VOC XML, one per\n image, so there is no manifest and no label column to declare.\n\n visdrone/\n images/ (required)\n 001.jpg\n annotations/ (required — 001.xml pairs with 001.jpg)\n 001.xml\n\n text (classification, masked language modeling) — a folder with\n labels.csv + a %[2]s/ subfolder (masked language modeling uses %[3]s/):\n\n reviews/\n labels.csv (required)\n %[2]s/ (required — %[3]s/ for masked language modeling)\n 001.txt\n ...\n\nA bare .csv file is accepted only for the tabular / time-series family;\nimage and text datasets must be a folder.\n\nAccepted image extensions: .jpg, .jpeg, or .png (case-insensitive).\nAll images in one dataset must share a single type — the cluster\nvalidates the type it was told to expect.\n\nv0.1 caps the dataset at 1 GiB total + 500 MiB per file. Larger\ndatasets need the v0.2 cloud-source story (S3/GCS/HTTPS sources) —\nsee tracebloc/client#147 non-goals.\n\nExit codes:\n 0 files staged + ingested successfully (or --detach: just staged + submitted)\n 2 schema validation failed (synthesized spec rejected) or\n v0.1-unsupported task passed\n 3 local-layout or kubeconfig error\n 4 cluster reachable but no tracebloc client / shared storage missing\n 5 ingestor SA token couldn't be obtained, or jobs-manager\n rejected the token (401/403)\n 6 destination table already exists (re-run with --overwrite to\n replace it, or pick a different --name)\n 7 pre-flight succeeded but staging the files failed\n (Pod creation, image pull, exec stream, or remote tar error) —\n or, with --overwrite, removing the old table failed\n 8 jobs-manager rejected the submit (4xx/5xx other than auth)\n 9 ingestion Job exited non-zero, or completed with row-level\n failures the summary panel reports" +"Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container that can't be created. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it." "Interrupted before the change could be confirmed." "It may already have applied — re-run `%s resources set` to check the current per-run ceiling." "It reports more memory than the machine really has, so two trainings that each look like they fit can together run it out of memory and take the environment down. Run one training at a time; to fix it for good, recreate the environment as a single-node one. `%s doctor --verbose` shows the numbers and the exact flags." @@ -281,6 +285,7 @@ screen. %s/%d are runtime placeholders. "Not connected — couldn't read your secure environment." "Not connected — tracebloc didn't confirm your session (server error)." "Not connected — your secure environment isn't answering." +"Not ready — a training pod is stuck starting and isn't running yet." "Not ready — dataset storage isn't available." "Not ready — not enough free compute to start a training." "Not ready — part of your secure environment can't start yet." diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 76a8939..62d886d 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -405,6 +405,42 @@ func podCrashLooping(p corev1.Pod) bool { return false } +// podWaitingReason names why an assigned pod is still Pending -- the kubelet's +// own Waiting.Reason from the first container that reports one, e.g. +// "ImagePullBackOff", "ErrImagePull", "ContainerCreating". Init containers are +// read first: an init container stalls startup before the app containers begin. +// Falls back to the bare phase ("Pending") when no container has reported a +// reason yet, so the caller always has something concrete to print. +func podWaitingReason(p corev1.Pod) string { + for _, group := range [][]corev1.ContainerStatus{p.Status.InitContainerStatuses, p.Status.ContainerStatuses} { + for _, c := range group { + if c.State.Waiting != nil && c.State.Waiting.Reason != "" { + return c.State.Waiting.Reason + } + } + } + return string(p.Status.Phase) +} + +// stuckReasonWedged reports whether a Pending pod's kubelet Waiting.Reason is a +// genuinely-wedged pull/create FAILURE -- backing off or errored -- as opposed +// to work still legitimately in progress. An allowlist, deliberately: a reason +// we are not certain is a failure (ContainerCreating / Pulling / bare Pending, +// or any future reason) is treated as still-progressing and left to checkPods' +// age-based inference, so the measured "waiting will not clear it" Fail is only +// ever asserted for a cause the kubelet has actually reported as failing. A +// large first image pull on a cold node can exceed the grace window while still +// making progress, so age alone must not escalate it (Bugbot on backend#3247). +func stuckReasonWedged(reason string) bool { + switch reason { + case "ImagePullBackOff", "ErrImagePull", "ErrImageNeverPull", "InvalidImageName", + "CreateContainerConfigError", "CreateContainerError": + return true + default: + return false + } +} + // checkRestartHistory surfaces containers that have restarted repeatedly even // though they are not crash-looping right now — the restart-*history* signal // backend#1028 asked for. checkPods reads only the current waiting reason, so a @@ -583,6 +619,16 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, } } +// stuckJobPod is a training-Job pod the scheduler has placed on a node but that +// is not Running yet -- carried out of checkNodeFit's resource sums so a wedged +// pod is never mistaken for a running job holding the room (backend#3247). +type stuckJobPod struct { + name string + namespace string + node string + reason string // the kubelet's Waiting.Reason, e.g. ImagePullBackOff +} + // checkNodeFit verifies at least one Ready node can satisfy the resource // requests the jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS // / GPU_REQUESTS env) — the "Pending forever, no node big enough" class. The @@ -652,6 +698,19 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // next run was going to wait. Two sums, two verdicts. jobCPU := map[string]int64{} // node -> millicores held by running Jobs jobMem := map[string]int64{} // node -> bytes held by running Jobs + // Training-Job pods the scheduler has already PLACED on a node (they carry a + // NodeName) but that are Pending on a genuinely-wedged reason -- an image pull + // backing off, a create error (see stuckReasonWedged). Kept out of BOTH sums + // above: a pod that is not running holds no room in the "a running job will + // finish and free it" sense, and counting one as a running job is exactly what + // made doctor emit HeldByRunningJob and tell the operator to "wait for the job + // to finish" at exit 0 on a pod that is wedged and never will (backend#3247). + // Collected here (past the same grace window checkPods uses) so the verdict can + // name the real stuck state as an actionable finding instead. Recognised + // cluster-wide by the same `job-name` convention the running-job sum above uses + // -- a dedicated secure environment's batch Jobs are tracebloc's -- so "stuck" + // and "holds the room" agree on what a training/ingestion pod is. + var stuckJobs []stuckJobPod freeKnown := true if pods, perr := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}); perr == nil { for i := range pods.Items { @@ -666,14 +725,44 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // Batch-Job pods carry the `job-name` label the batch/v1 controller // stamps (see internal/submit/watch.go) -- that label, not a name // pattern, is how the CLI already recognises a training or ingestion - // pod, so it is the one used here. They go into the JOB sums, never the - // steady-state ones: a running job holds the envelope itself, so - // counting it as platform would make doctor Fail "no room for a training - // job" on the exact healthy state it exists to bless (Bugbot High). The - // steady state is the control plane (Deployments/DaemonSets), which has - // no `job-name`. + // pod, so it is the one used here. A running job goes into the JOB sums, + // never the steady-state ones: a running job holds the envelope itself, + // so counting it as platform would make doctor Fail "no room for a + // training job" on the exact healthy state it exists to bless (Bugbot + // High). The steady state is the control plane (Deployments/DaemonSets), + // which has no `job-name`. cpu, mem := reqCPU, reqMem if _, isJob := p.Labels["job-name"]; isJob { + // A Job pod holds the room ONLY when it is genuinely Running. An + // assigned-but-Pending one (it has a NodeName but its containers have + // not started) is NOT running: counting it as a running job made + // doctor emit HeldByRunningJob and tell the operator to wait for it to + // finish -- on a pod that may be wedged (backend#3247). So it feeds + // NEITHER sum. A pod merely WAITING for capacity has no NodeName yet + // and was skipped above, so this only ever sees pods the scheduler + // already placed. + // + // Whether it is WEDGED is a separate question from whether it holds + // the room. Only a genuinely-wedged reason -- an image pull backing + // off, or a create error -- is recorded here (past the same grace + // window checkPods uses) and escalated to the measured Fail below, + // where "waiting will not clear it" is exact. A pod still pulling or + // creating (ContainerCreating / Pulling / bare Pending) is left to + // checkPods' age-based inference instead: a large first pull on a cold + // node can legitimately exceed the grace, so asserting it is wedged + // would misfire (Bugbot on this PR). + if p.Status.Phase != corev1.PodRunning { + if reason := podWaitingReason(p); stuckReasonWedged(reason) && + time.Since(p.CreationTimestamp.Time) > pendingGrace { + stuckJobs = append(stuckJobs, stuckJobPod{ + name: p.Name, + namespace: p.Namespace, + node: p.Spec.NodeName, + reason: reason, + }) + } + continue + } cpu, mem = jobCPU, jobMem } for j := range p.Spec.Containers { @@ -844,6 +933,33 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Detail: detail, Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } + case len(stuckJobs) > 0: + // A training Job pod the scheduler already placed on a node is Pending on a + // genuinely-wedged reason -- an image pull backing off, a create error + // (stuckReasonWedged) -- NOT running (backend#3247). It used to be counted + // as a running job holding the room, so this state rolled up to the + // transient "a training is already running, wait for it to finish" Warn at + // exit 0 -- on a pod that is wedged and never will. It is a real, + // training-blocking problem whose remedy is the OPPOSITE of the transient + // Warn's (inspect the pod, do NOT wait), so it is a Fail with its own prefix + // the rollup (summarizeDoctor) classifies on. Ordering matters: it sits + // BELOW the two measured capacity Fails above -- when no node can fit the + // envelope that is the root cause -- and ABOVE the transient Warn, which a + // wedged pod must never be mistaken for. A run legitimately WAITING for a + // running job has no NodeName; one still pulling/creating is not wedged and + // was left to checkPods' inference -- neither is collected here. + sort.Slice(stuckJobs, func(a, b int) bool { return stuckJobs[a].name < stuckJobs[b].name }) + descs := make([]string, len(stuckJobs)) + for i, s := range stuckJobs { + descs[i] = fmt.Sprintf("%s/%s on %s (%s)", s.namespace, s.name, s.node, s.reason) + } + first := stuckJobs[0] + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("%s: %s. The next run does not wait on a pod that is not running", StuckJobPod, strings.Join(descs, ", ")), + Remedy: fmt.Sprintf("Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container that can't be created. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it.", first.namespace, first.name), + } case freeKnown && !nowFits: // The TRANSIENT shortage (backend#2870): every dimension fits beside the // platform's own pods, so this machine CAN run the envelope -- but a batch @@ -1007,6 +1123,18 @@ const OverCommitted = "a Ready node is large enough" // that drifted would send this state down the generic path again. const HeldByRunningJob = "a running job holds the room" +// StuckJobPod is the prefix of the backend#3247 Fail: a training Job pod is +// scheduled to a node (it has a NodeName) but is Pending on a genuinely-wedged +// reason -- an image pull backing off, or a create error (stuckReasonWedged). +// It used to be counted as a running job holding the room, so checkNodeFit emitted HeldByRunningJob +// and the rollup told the operator to "wait for the job to finish" at exit 0 -- +// on a pod that is wedged and never will. DISTINCT prefix, same discipline as +// HeldByRunningJob and OverCommitted: the rollup (summarizeDoctor) classifies on +// it, and its remedy ("inspect the stuck pod, do not wait") is the opposite of +// the transient Warn's ("wait for the running job to finish"). A retyped copy +// that drifted would send a wedged pod back down the "just wait" path. +const StuckJobPod = "a training pod is scheduled but not running" + // 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. diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index b44c10b..40e4f05 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -1103,6 +1103,153 @@ func TestCheckNodeFitPermanentVsTransient(t *testing.T) { }) } +// assignedPendingJobPod is a training-Job pod (the batch/v1 job-name label) the +// scheduler has already PLACED on a node -- it has a NodeName -- but that is NOT +// Running: still Pending, with the kubelet's Waiting.Reason on its container +// (e.g. "ImagePullBackOff", "ContainerCreating"; empty means bare Pending). age +// sets how long ago it was created so a test can sit inside or outside +// checkNodeFit's grace window. It requests memory so a test can also prove that +// request is never folded into the "held by a running job" sum. +func assignedPendingJobPod(name, nodeName, reason, mem string, age time.Duration) *corev1.Pod { + reqs := corev1.ResourceList{} + if mem != "" { + reqs[corev1.ResourceMemory] = resource.MustParse(mem) + } + var statuses []corev1.ContainerStatus + if reason != "" { + statuses = []corev1.ContainerStatus{{ + Name: "c", + State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: reason}}, + }} + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: map[string]string{"job-name": "exp-42"}, + CreationTimestamp: metav1.NewTime(time.Now().Add(age)), + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Containers: []corev1.Container{{Name: "c", Resources: corev1.ResourceRequirements{Requests: reqs}}}, + }, + Status: corev1.PodStatus{Phase: corev1.PodPending, ContainerStatuses: statuses}, + } +} + +// backend#3247: an assigned-but-Pending training pod is NOT a running job. +// checkNodeFit used to route every non-terminal job-name pod with a NodeName +// into the "a running job holds the room" sum, so a pod wedged on an image pull +// emitted HeldByRunningJob and the rollup told the operator to wait for a job +// that was not running. Each pod STATE must land on its own verdict. +func TestCheckNodeFitStuckJobPod(t *testing.T) { + req := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi"} + // A node big enough that, with NOTHING holding the room, the envelope fits -- + // so any non-OK verdict below is about the pod's STATE, not the machine. + fitNode := func() *corev1.Node { return node("n1", "4", "16Gi") } + const old = -10 * time.Minute // older than the 5m grace window + const fresh = -1 * time.Minute // inside it + + // A genuinely RUNNING job that fills the room is the transient Warn -- the + // pre-existing behaviour this fix must leave untouched. + t.Run("Running job filling the room -> HeldByRunningJob Warn (unchanged)", func(t *testing.T) { + job := podOn("train-run", "n1", "", "12Gi", map[string]string{"job-name": "exp-42"}) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), job), req) + if r.Status != StatusWarn || !strings.HasPrefix(r.Detail, HeldByRunningJob) { + t.Fatalf("=> %v (%q), want the transient Warn %q", r.Status, r.Detail, HeldByRunningJob) + } + }) + + // The defect, one WEDGED waiting reason per row: assigned, Pending, past grace, + // on a reason the kubelet has reported as failing. Each must be a stuck Fail + // that NAMES its reason -- never HeldByRunningJob. + for _, tc := range []struct{ name, reason string }{ + {"ImagePullBackOff", "ImagePullBackOff"}, + {"ErrImagePull", "ErrImagePull"}, + {"CreateContainerConfigError", "CreateContainerConfigError"}, + } { + t.Run("assigned+Pending past grace, wedged ("+tc.name+") -> stuck Fail", func(t *testing.T) { + pod := assignedPendingJobPod("train-stuck", "n1", tc.reason, "12Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if r.Status != StatusFail { + t.Fatalf("=> %v (%q), want Fail: a wedged pod is not a running job", r.Status, r.Detail) + } + if !strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("detail must START with %q so the rollup classifies it, got %q", StuckJobPod, r.Detail) + } + if strings.Contains(r.Detail, HeldByRunningJob) { + t.Fatalf("a wedged pod must never be reported as a running job: %q", r.Detail) + } + if !strings.Contains(r.Detail, tc.reason) { + t.Errorf("detail should name the real state %q, got %q", tc.reason, r.Detail) + } + if !strings.Contains(r.Remedy, "kubectl describe pod") { + t.Errorf("the remedy must be actionable (inspect the pod), got %q", r.Remedy) + } + }) + } + + // A still-PROGRESSING pod past grace (ContainerCreating / bare Pending, or a + // large first pull that has not backed off yet) is NOT wedged: asserting + // "waiting will not clear it" would misfire on a multi-GB image on a cold + // node. checkNodeFit leaves it to checkPods' age-based inference, so on its + // own it is neither the stuck Fail nor HeldByRunningJob -- the node reads OK + // (Bugbot on this PR, backend#3247). + for _, tc := range []struct{ name, reason string }{ + {"ContainerCreating", "ContainerCreating"}, + {"Pulling", "Pulling"}, + {"bare Pending, no reason yet", ""}, + } { + t.Run("assigned+Pending past grace, still progressing ("+tc.name+") -> not escalated", func(t *testing.T) { + pod := assignedPendingJobPod("train-pulling", "n1", tc.reason, "12Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("a still-progressing pod must not take the measured stuck Fail: %q", r.Detail) + } + if r.Status != StatusOK { + t.Fatalf("=> %v (%q), want OK: checkNodeFit defers a progressing pod to checkPods", r.Status, r.Detail) + } + }) + } + + // A pod the scheduler JUST placed and is normally starting (inside grace) is + // neither a running job nor stuck -- flagging it would false-positive on + // every training launch. Not counted as held either, so the node reads OK. + t.Run("assigned+Pending INSIDE grace -> not flagged, node still OK", func(t *testing.T) { + pod := assignedPendingJobPod("train-young", "n1", "ImagePullBackOff", "12Gi", fresh) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if r.Status != StatusOK { + t.Fatalf("=> %v (%q), want OK: a freshly-scheduled pod is normal startup", r.Status, r.Detail) + } + if strings.Contains(r.Detail, StuckJobPod) || strings.Contains(r.Detail, HeldByRunningJob) { + t.Fatalf("a within-grace pod must be neither stuck nor held: %q", r.Detail) + } + }) + + // Scope guard: the fix keys on the job-name label. A non-Job pod (platform) + // that is Pending-assigned is steady state and must not take the stuck-pod arm. + t.Run("a non-Job Pending pod is not a stuck training pod", func(t *testing.T) { + plat := assignedPendingJobPod("some-deploy", "n1", "ImagePullBackOff", "12Gi", old) + plat.Labels = nil // not a job-name pod, though its reason IS wedged + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), plat), req) + if strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("the stuck-training-pod arm must be scoped to job-name pods, got %q", r.Detail) + } + }) + + // A running job holds the room AND a second training pod is wedged on an + // image pull: the wedged pod is the actionable problem and its Fail must win + // over the transient Warn (the stuck arm sits above it in the switch). + t.Run("a stuck pod outranks a genuinely running job", func(t *testing.T) { + running := podOn("train-run", "n1", "", "12Gi", map[string]string{"job-name": "exp-42"}) + stuck := assignedPendingJobPod("train-stuck", "n1", "ImagePullBackOff", "1Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), running, stuck), req) + if r.Status != StatusFail || !strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("=> %v (%q), want the stuck Fail to win over the transient Warn", r.Status, r.Detail) + } + }) +} + func dockerSecret(name string, data []byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, From b6b263e8e5ce2591e3e5d7edeecc22c086fc9931 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 12:40:44 +0200 Subject: [PATCH 4/5] fix(doctor): measured image-pull/dataset Fail outranks the wait-for-capacity Warn (backend#3248) (#643) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(doctor): measured image-pull/dataset Fail outranks the wait-for-capacity Warn (backend#3248) 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 * fix(doctor): classify pull-secret/PVC read failures as can't-checks, not measured Fails (backend#3248) 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 * fix(doctor): close the jobs-manager-unreadable false green; tighten the Wait-Warn precondition (backend#3248) 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 * fix(doctor): plain-terms wording for the image-pull can't-check rollup line (backend#3248) 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 --------- Co-authored-by: Claude Opus 4.8 --- internal/cli/doctor.go | 69 +++++++-- internal/cli/doctor_test.go | 138 ++++++++++++++++++ .../cli/testdata/golden/zz-all-strings.golden | 6 +- internal/cluster/pvc.go | 13 +- internal/doctor/doctor.go | 46 +++++- internal/doctor/doctor_test.go | 43 ++++++ 6 files changed, 296 insertions(+), 19 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 77f99a8..87c837a 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, @@ -536,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, @@ -555,14 +582,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.", @@ -633,6 +652,28 @@ 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 (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)", ""} default: ready = healthLine{doctor.StatusOK, "Ready to run training", ""} } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index cadd225..695de9c 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -572,6 +572,61 @@ 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. + // 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 + // 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 +638,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) + } + }) + // backend#3247: the defect combination. A training pod is scheduled but // wedged on an image pull (Node capacity Fail, StuckJobPod) and Pod health // also sees it Pending past grace (stuckPending). This used to roll up through @@ -639,6 +716,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, "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") + 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..af95723 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 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" @@ -502,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" @@ -682,7 +685,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..694994f 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. @@ -1167,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 + ".", } } @@ -1181,6 +1210,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..6d3b2f5 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,37 @@ 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) + } + }) + // 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 df402057fb6e81ff0617bc11d87a5d8fce33cf2a Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 13:17:07 +0200 Subject: [PATCH 5/5] chore(doctor): one can't-check rollup rule via a CantCheck marker, not N prefix-matched arms (backend#3282) (#644) 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) } }) }