fix(doctor): node-fit checks FREE memory, not allocatable (backend#2870)

VERSION

@@ -1 +1 @@
-0.10.21
+0.10.22

internal/cli/doctor.go

@@ -450,6 +450,38 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		ready = healthLine{doctor.StatusFail,
 			"Not ready — part of your secure environment isn't running.",
 			fmt.Sprintf("Reinstall with `%s`, or email support@tracebloc.io with `%s doctor --diagnose`.", installer.Cmd, launcher())}
+	case by["Node capacity"].Status == doctor.StatusFail &&
+		strings.HasPrefix(by["Node capacity"].Detail, doctor.OverCommitted):
+		// THE OPPOSITE REMEDY FROM THE GENERIC CAPACITY ARM BELOW, which is why
+		// this case exists (Bugbot Medium, #628). `computeRemedy` ends every
+		// variant with "size runs to this machine with `resources set max`" -- and
+		// `set max` sizes from the machine's TOTAL, which is the figure this Fail
+		// just rejected. The machine is big enough; what is missing is room beside
+		// what is already on it. So the generic advice would raise the ask and
+		// leave the training stuck, which is worse than no advice: the user
+		// follows it and the symptom persists.
+		//
+		// IT SITS ABOVE THE STUCK-PENDING ARM, and that ordering is the whole
+		// point rather than a preference (Bugbot Medium, #628 second pass). The
+		// two states CO-OCCUR BY CONSTRUCTION: the producer's own Detail ends
+		// "so the pod schedules Pending" (`doctor.go:778`), so an over-committed
+		// node is *expected* to also have Pod health warning about pods stuck
+		// Pending. Below that arm this case was therefore almost unreachable in
+		// the field -- the stuck-Pending arm matched first and printed
+		// `computeRemedy`, putting `set max` back in front of the operator in the
+		// exact state this Fail exists to refuse. The first fix corrected the
+		// figure and left the ROLLUP still recommending the thing.
+		//
+		// Only a hard `Pod health` Fail outranks it: pods not running at all is a
+		// different problem with a different fix (reinstall), and it is not
+		// caused by this one.
+		//
+		// PLAIN TERMS, no Kubernetes vocabulary, like its two neighbours --
+		// `renderDoctorDetails` is documented as the only place that appears, and
+		// the granular Remedy one `--verbose` away already names the knob.
+		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"):
 		// Pods stuck Pending past the grace window (unschedulable / image can't
 		// pull) mean training can't actually schedule — so this is NOT ready, even
@@ -512,7 +544,8 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 			"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, "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

internal/cli/doctor_test.go

@@ -389,6 +389,19 @@ func TestSummarizeDoctor(t *testing.T) {
 		for _, detail := range []string{
 			"could not list nodes: nodes is forbidden",
 			"couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit",
+			// 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.
+			doctor.CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here",
+			// Bugbot High on #628: the same can't-check ARRIVING WITH the soft GPU
+			// warn. The GPU case fires first in checkNodeFit, so this combined
+			// detail is what a GPU-requesting install with an unreadable pod list
+			// actually produces -- and it must roll up the same way.
+			doctor.CantVerifyFreeCompute + ", so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here. Also, no single Ready node satisfies cpu+memory AND nvidia.com/gpu, so GPU jobs would rely on the CPU fallback (needs cpu=2, memory=8Gi)",
 		} {
 			_, r := summarizeDoctor(withDetail(allOK, "Node capacity", doctor.StatusWarn, detail), tokenOK)
 			if r.status != doctor.StatusUnknown {
@@ -400,6 +413,101 @@ func TestSummarizeDoctor(t *testing.T) {
 		}
 	})
 
+	t.Run("over-commit Fail must NOT advise sizing runs to the machine", func(t *testing.T) {
+		// Bugbot Medium on #628. The two Node-capacity Fails need OPPOSITE advice:
+		// "no node is big enough" is fixed by giving the machine more (or sizing
+		// runs to it); "big enough, but not beside what is already running" is
+		// made WORSE by that, because `resources set max` measures the machine's
+		// total, which is the figure this Fail rejected. A user who follows it asks
+		// for more and stays stuck.
+		//
+		// DETAIL BUILT FROM THE PRODUCER'S CONSTANT so the arm and the producer
+		// cannot drift apart -- the classification is by prefix, so a reworded
+		// producer would silently fall through to the generic arm again.
+		_, r := summarizeDoctor(withDetail(allOK, "Node capacity", doctor.StatusFail,
+			doctor.OverCommitted+" for a training job (cpu=2, memory=8Gi) but not beside what is already running on it — the envelope over-asks the node's FREE memory, so the pod schedules Pending"), tokenOK)
+		if r.status != doctor.StatusFail {
+			t.Fatalf("over-commit is still Not ready, got %v", r.status)
+		}
+		if strings.Contains(r.remedy, "resources set max") {
+			t.Errorf("the top-line remedy tells the user to size runs to the machine, which raises the ask this Fail rejected: %q", r.remedy)
+		}
+		if !strings.Contains(r.remedy, "Do NOT") {
+			t.Errorf("the remedy should warn against sizing to the machine, got %q", r.remedy)
+		}
+	})
+
+	t.Run("over-commit outranks stuck-Pending, which it CAUSES", func(t *testing.T) {
+		// Bugbot Medium on #628, second pass -- and the case the test above could
+		// not reach. That one starts from `allOK`, so Pod health is OK and the
+		// over-commit arm is the first Fail either way. The bug lived in the state
+		// where BOTH fire.
+		//
+		// THEY CO-OCCUR BY CONSTRUCTION, which is what makes this ordering a
+		// correctness question and not a preference: the producer's Detail ends
+		// "so the pod schedules Pending" (internal/doctor/doctor.go:778), so an
+		// over-committed node is EXPECTED to also have pods stuck Pending. With
+		// the stuck-Pending arm first, the rollup printed `computeRemedy` -- which
+		// ends in `resources set max` -- in the one state the Node-capacity Fail
+		// exists to refuse. The figure was fixed on the previous commit and the
+		// rollup went on recommending the thing.
+		//
+		// Both details are built from the producer's own constant/text rather than
+		// retyped, so a reworded producer reddens this instead of silently falling
+		// through to the generic arm.
+		results := withDetail(allOK, "Node capacity", doctor.StatusFail,
+			doctor.OverCommitted+" for a training job (cpu=2, memory=8Gi) but not beside what is already running on it — the envelope over-asks the node's FREE memory, so the pod schedules Pending")
+		results = withDetail(results, "Pod health", doctor.StatusWarn,
+			"1 pod stuck Pending past the grace window")
+
+		_, r := summarizeDoctor(results, tokenOK)
+		if r.status != doctor.StatusFail {
+			t.Fatalf("want Fail, got %v", r.status)
+		}
+		if strings.Contains(r.remedy, "resources set max") {
+			t.Errorf("the stuck-Pending arm shadowed the over-commit arm and put `set max` back in front of the operator, in the exact state the Fail refuses: %q", r.remedy)
+		}
+		if !strings.Contains(r.remedy, "Do NOT") {
+			t.Errorf("want the over-commit remedy, got the generic one: %q", r.remedy)
+		}
+		if !strings.Contains(r.text, "already claimed the room") {
+			t.Errorf("want the over-commit top line, got %q", r.text)
+		}
+	})
+
+	t.Run("a hard Pod-health Fail still outranks over-commit", func(t *testing.T) {
+		// The other side of the reorder: over-commit was moved above the
+		// stuck-Pending WARN, not above the Pod-health FAIL. Pods not running at
+		// all is a different problem with a different fix (reinstall), and it is
+		// not caused by over-commitment -- so it must still win. Without this,
+		// "move it up" could keep sliding until it shadowed a harder failure.
+		results := withDetail(allOK, "Node capacity", doctor.StatusFail,
+			doctor.OverCommitted+" for a training job (cpu=2, memory=8Gi) but not beside what is already running on it")
+		results = withDetail(results, "Pod health", doctor.StatusFail,
+			"2 pods CrashLoopBackOff")
+
+		_, r := summarizeDoctor(results, tokenOK)
+		if r.status != doctor.StatusFail {
+			t.Fatalf("want Fail, got %v", r.status)
+		}
+		if !strings.Contains(r.text, "isn't running") {
+			t.Errorf("a hard Pod-health Fail must still win the rollup, got %q", r.text)
+		}
+	})
+
+	t.Run("generic capacity Fail still gets the sizing advice", func(t *testing.T) {
+		// The other side: the fix must not strip the correct advice from the Fail
+		// it IS correct for -- a machine that is genuinely too small.
+		_, r := summarizeDoctor(withDetail(allOK, "Node capacity", doctor.StatusFail,
+			"no Ready node can fit a training job (needs cpu=2, memory=8Gi)"), tokenOK)
+		if r.status != doctor.StatusFail {
+			t.Fatalf("want Fail, got %v", r.status)
+		}
+		if !strings.Contains(r.remedy, "resources set max") {
+			t.Errorf("a too-small machine should still be offered the sizing fix: %q", r.remedy)
+		}
+	})
+
 	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)

internal/cli/testdata/golden/zz-all-strings.golden

@@ -57,6 +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 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"
@@ -77,6 +78,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, 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: %v"
@@ -87,6 +89,7 @@ screen. %s/%d are runtime placeholders.
 "%w in namespace %q. If your client runs in another namespace, pass --namespace; if this cluster has no tracebloc client yet, run the installer: %s. Diagnose with `tracebloc doctor`."
 "%w on the cluster your kubeconfig points at — if this machine should have one, run the installer to provision it; otherwise point at the right cluster with --context/--namespace"
 "%w. Run `tracebloc login` to start a new one"
+"'tracebloc resources set --cores %d --memory %dGi'"
 "(%d CPU · %d GiB"
 "(+%d more)"
 "(Pod phase: %s)"
@@ -126,6 +129,7 @@ screen. %s/%d are runtime placeholders.
 "Already signed out."
 "Applies to your next training run; a run already going keeps its size."
 "Applying the resource change…"
+"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."
 "Ask one of these admins (or ask them to grant you access)"
 "Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s"
 "Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s"
@@ -207,6 +211,8 @@ 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 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"
 "Everything looks good — you're ready to run training."
 "Fix the failing checks above, then re-run `tracebloc client status --seal` to confirm the seal."
@@ -252,6 +258,7 @@ screen. %s/%d are runtime placeholders.
 "Left alone"
 "Let each training run use up to %s?"
 "Local dataset"
+"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)."
 "Machine credential — needed by the installer to connect this client"
 "Memory"
 "Memory for one run in GiB (2–%d)"
@@ -273,6 +280,7 @@ screen. %s/%d are runtime placeholders.
 "Not ready — part of your secure environment can't start yet."
 "Not ready — part of your secure environment isn't running."
 "Not ready — the training images can't be pulled."
+"Not ready — this machine is big enough, but the platform's own services have already claimed the room."
 "Not ready — your active client points at namespace %q, which isn't on this cluster, so data commands will keep failing until you repoint."
 "Not signed in yet."
 "Not signed in — run `%s login`."
@@ -808,4 +816,4 @@ screen. %s/%d are runtime placeholders.
 "· %d classes"
 "— largest node offers ephemeral-storage=%s"
 "— sign-in codes are valid for %s"
-"— this machine could give a run up to cpu=%d,memory=%dGi ('tracebloc resources set max')"
+"— this machine could give a run up to cpu=%d,memory=%dGi (%s)"