diff --git a/VERSION b/VERSION index e831019..d3dd9cb 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.21 +0.10.22 diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index f152b11..26c9707 100644 --- a/internal/cli/doctor.go +++ b/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 diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index d51b0c9..e731443 100644 --- a/internal/cli/doctor_test.go +++ b/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) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index ff24c69..f687ec5 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/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)" diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 949fe0f..18b3ffd 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -623,7 +623,53 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // A pod gets ALL its requested resources from ONE node, so evaluate each // node as a whole — never OR cpu/mem and GPU across different nodes, which // would pass even when no single node can run the job (Bugbot on PR #91). - var cpuMemFits, fullFits bool + // FREE, not allocatable (backend#2870). A node whose control plane already + // requests memory has less room than its allocatable advertises, and fitting + // the training envelope against allocatable is the blind spot that let every + // install go Pending unnoticed: the installer writes `allocatable − overhead`, + // and if the real control plane exceeds `overhead` the pod cannot schedule even + // though allocatable "fits". Sum the requests already on each node and evaluate + // the job against what is FREE. If the pod list cannot be read, free is UNKNOWN: + // 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 + freeKnown := true + if pods, perr := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}); perr == nil { + for i := range pods.Items { + p := pods.Items[i] + // A pod with no node holds no node's memory yet; a terminal pod holds + // none at all -- counting either would understate free (mirrors + // requestedMemory's Succeeded/Failed skip). + if p.Spec.NodeName == "" || + 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`. + if _, isJob := p.Labels["job-name"]; isJob { + continue + } + 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() + } + if q, ok := r[corev1.ResourceMemory]; ok { + reqMem[p.Spec.NodeName] += q.Value() + } + } + } + } else { + freeKnown = false + } + + var cpuMemFits, fullFits, allocOnlyFit, overCPU, overMem bool // 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). @@ -633,6 +679,13 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // to pick a node, so it must not perturb the drift nudge's tie-break. var bestDisk resource.Quantity var sawDisk bool + // The most cpu/memory actually FREE on any one Ready node. Tracked next to + // the allocatable-derived best above because the drift nudge must not + // advertise a ceiling the fit would refuse: the fit moved to free, the nudge + // did not, so on a large-but-claimed node it advised sizing UP to a figure + // that recreates the over-commit this very check now fails (Bugbot Medium, + // confirmed by @saqlainsyed007). + var bestFreeCPUm, bestFreeMemB int64 for i := range nodes.Items { n := nodes.Items[i] if !nodeReady(n) { @@ -650,7 +703,37 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s bestDisk = d } } - nodeCPUMem := alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 + // Allocatable minus what is already requested on THIS node (never below 0). + // When free is unknown, this is allocatable and the caveat is reported below. + freeCPUm := alloc.Cpu().MilliValue() + freeMemB := alloc.Memory().Value() + if freeKnown { + if freeCPUm -= reqCPU[n.Name]; freeCPUm < 0 { + freeCPUm = 0 + } + if freeMemB -= reqMem[n.Name]; freeMemB < 0 { + freeMemB = 0 + } + } + if freeCPUm > bestFreeCPUm || + (freeCPUm == bestFreeCPUm && freeMemB > bestFreeMemB) { + bestFreeCPUm, bestFreeMemB = freeCPUm, freeMemB + } + nodeCPUMem := freeCPUm >= cpuReq.MilliValue() && freeMemB >= memReq.Value() + // Whether the node is big enough IGNORING neighbours -- the old, blind + // verdict. Kept only to tell "no node is big enough at all" apart from + // "a node is big enough but not beside its control plane" in the message. + if alloc.Cpu().Cmp(cpuReq) >= 0 && alloc.Memory().Cmp(memReq) >= 0 { + allocOnlyFit = true + // Which dimension the FREE fit falls short on, so the over-commit + // message names cpu vs memory instead of always blaming memory (Bugbot). + if freeCPUm < cpuReq.MilliValue() { + overCPU = true + } + if freeMemB < memReq.Value() { + overMem = true + } + } // 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 // same per-node verdict and never OR-ed across nodes (Bugbot on PR #91 @@ -678,14 +761,59 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s } switch { + case !cpuMemFits && allocOnlyFit && freeKnown && (overCPU || overMem): + // The #2870 case: a node IS big enough, but not beside the requests its own + // control plane already holds. Allocatable said yes; free says no. This is + // the shape that goes Pending/Insufficient memory after a clean install. + short := "memory" + switch { + case overCPU && overMem: + short = "cpu and memory" + case overCPU: + short = "cpu" + } + 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), + 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: + detail := fmt.Sprintf("no Ready node can fit a training job (needs %s)", req) + if !freeKnown { + detail += " — checked against allocatable only; the pod list could not be read, so an over-committed control plane would be invisible here" + } return Result{ Name: name, Status: StatusFail, - Detail: fmt.Sprintf("no Ready node can fit a training job (needs %s)", req), + Detail: detail, Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } case gpuRequested && !fullFits: + // UNKNOWN FREE OUTRANKS THE SOFT GPU WARN (Bugbot High, #628). + // + // This case sits ABOVE the `!freeKnown` branch in the default arm, and its + // detail carries no `CantVerifyFreeCompute` prefix -- so when the pod list + // could not be read AND a GPU is requested, this Warn won, the can't-check + // was never emitted, and `summarizeDoctor` fell through to "Ready to run + // training" at exit 0. Doctor printed a clean bill over a cluster whose + // free compute it had not looked at. + // + // It is not an exotic path: the chart stamps `nvidia.com/gpu` on CPU-only + // installs too, so `gpuRequested` is commonly true where no node exposes a + // GPU -- which is exactly this case. + // + // The GPU fallback stays SOFT and stays reported; it just no longer + // suppresses the stronger statement. Both facts go in one Warn, with the + // 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.", + } + } return Result{ Name: name, Status: StatusWarn, @@ -705,17 +833,101 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // stale when a machine GROWS. When the configured budget uses no more // than half of what this machine could give one run (largest node − // platform overhead), say so. + // + // BOUNDED BY FREE, NOT ALLOCATABLE. The ceiling is derived from the + // largest node, but the verdict above is derived from what is FREE on + // it. On a node that is big enough and already partly claimed those two + // disagree, and the nudge won: an operator who ran `resources set max` + // recreated the exact over-commit this check fails on, and the next + // `doctor` told them not to size to their own machine. Advice that + // contradicts the verdict printed beside it is worse than no advice. + // + // So the machine handed to `MaxRunCores`/`MaxRunGiB` is the smaller of + // allocatable and free, and when free is UNKNOWN the nudge is suppressed + // entirely -- there is nothing to bound it with, and this arm is already + // a Warn that says free could not be verified. m := resources.Machine{CPU: bestCPU, Mem: bestMem} + if freeKnown { + freeCPU := *resource.NewMilliQuantity(bestFreeCPUm, resource.DecimalSI) + freeMem := *resource.NewQuantity(bestFreeMemB, resource.BinarySI) + if freeCPU.Cmp(m.CPU) < 0 { + m.CPU = freeCPU + } + if freeMem.Cmp(m.Mem) < 0 { + m.Mem = freeMem + } + } maxCores, maxGiB := resources.MaxRunCores(m), resources.MaxRunGiB(m) - if maxCores >= 1 && maxGiB >= 2 && + if freeKnown && maxCores >= 1 && maxGiB >= 2 && cpuReq.MilliValue()*2 <= int64(maxCores)*1000 && memReq.Value()*2 <= int64(maxGiB)<<30 { - detail += fmt.Sprintf(" — this machine could give a run up to cpu=%d,memory=%dGi ('tracebloc resources set max')", maxCores, maxGiB) + // NAME THE COMMAND THAT APPLIES THESE NUMBERS (Bugbot Medium). + // Bounding the ceiling by free fixed the figure and left the + // attribution behind: `resources set max` sizes from ALLOCATABLE via + // `LargestReadyNode`, so on a claimed node it applies more than the + // figure printed beside it -- and reapplies the over-commit this + // check just started refusing. The printed numbers and the suggested + // command have to be the same thing. + // + // So `set max` is named only when it would genuinely land on these + // numbers -- i.e. nothing meaningful is claimed and the free-bounded + // ceiling equals the allocatable one. Otherwise the explicit form is + // named, which applies exactly what is printed. + allocM := resources.Machine{CPU: bestCPU, Mem: bestMem} + how := fmt.Sprintf("'tracebloc resources set --cores %d --memory %dGi'", maxCores, maxGiB) + if maxCores == resources.MaxRunCores(allocM) && maxGiB == resources.MaxRunGiB(allocM) { + how = "'tracebloc resources set max'" + } + detail += fmt.Sprintf(" — this machine could give a run up to cpu=%d,memory=%dGi (%s)", maxCores, maxGiB, how) + } + // UNKNOWN free is not a clean pass (Bugbot High). When the pod list could + // not be read, this fit was against allocatable, not free -- so it cannot + // assert schedulability, and an over-committed control plane would be + // invisible. Warn and say so rather than greening node capacity. + 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.", + } } return Result{Name: name, Status: StatusOK, Detail: detail} } } +// OverCommitted is the prefix of the #2870 Fail: a node IS big enough, but not +// beside what its own control plane already holds. ONE definition, same reason +// as CantVerifyFreeCompute below. +// +// The rollup needs to tell this Fail apart from the generic "no node is big +// enough" one because the REMEDIES ARE OPPOSITE. `computeRemedy` ends every +// variant with "size runs to this machine with `tracebloc resources set max`", +// and `set max` sizes from ALLOCATABLE -- the exact figure this Fail rejected. +// Following it raises the envelope and the pod stays Pending (Bugbot Medium, +// #628). The generic arm is right for a too-small machine and wrong here. +const OverCommitted = "a Ready node is large enough" + +// CantVerifyFreeCompute is the prefix every "we could not check free compute" +// Node-capacity Warn must start with, and the ONE definition of it. +// +// `summarizeDoctor` in internal/cli/doctor.go classifies a Node-capacity Warn as +// a can't-check by PREFIX, so the producer's wording is load-bearing: a Warn that +// does not start with this string falls through to "Ready to run training" at +// exit 0. That string was written out three times -- here, in the classifier, and +// in the classifier's test -- which is a rule the checks held their own copy of. +// +// It is exported rather than duplicated because the two live in different +// packages and internal/cli already imports this one. A third caller that needs +// the same classification must reference this, not retype it. +const CantVerifyFreeCompute = "could not read the pod list" + // checkImagePull verifies that any registry pull secret the jobs-manager // references exists and is a well-formed dockerconfigjson — so private-image // pulls don't ImagePullBackOff. (Bad-but-well-formed credentials can't be diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 4ae6205..3248e63 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -678,6 +678,57 @@ func TestCheckNodeFit(t *testing.T) { t.Fatalf("=> %v (%q), want ok without nudge", r.Status, r.Detail) } }) + t.Run("large but CLAIMED node -> nudge must not advise past free", func(t *testing.T) { + // Bugbot Medium, confirmed by @saqlainsyed007. The fit moved to FREE; + // the nudge stayed on allocatable. On a node that is big enough and + // already partly claimed the two disagree, and the nudge won -- so an + // operator who ran `resources set max` recreated the exact over-commit + // this check now fails on, and the next `doctor` told them not to size + // to their own machine. + // + // 32/64Gi node, 24 cores + 40Gi already held by a non-job pod: the run + // (2/8Gi) still fits in the 8/24Gi that is free, so this stays OK -- but + // the advertised ceiling may not be the allocatable-derived 31/61Gi. + claim := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "control-plane", Namespace: ns}, + Spec: corev1.PodSpec{ + NodeName: "n1", + Containers: []corev1.Container{{ + Name: "c", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("24"), + corev1.ResourceMemory: resource.MustParse("40Gi"), + }, + }, + }}, + }, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, + } + cs := fake.NewClientset(node("n1", "32", "64Gi"), claim) + r := checkNodeFit(bg(), cs, cpuOnly) + if r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok (2/8Gi fits in the free 8/24Gi)", r.Status, r.Detail) + } + if strings.Contains(r.Detail, "cpu=31,memory=61Gi") { + t.Fatalf("nudge advertises the ALLOCATABLE ceiling 31/61Gi while only "+ + "8 cores / 24Gi are free -- following it recreates the over-commit "+ + "this check fails on: %q", r.Detail) + } + // AND THE COMMAND MUST APPLY THE PRINTED NUMBERS (Bugbot Medium). + // Bounding the figure by free is only half the fix: `set max` sizes from + // allocatable via LargestReadyNode, so attributing a free-bounded figure + // to it still sends the operator to a command that over-commits. On a + // claimed node the explicit form is the honest one. + if strings.Contains(r.Detail, "resources set max") { + t.Fatalf("nudge names 'set max', which sizes from ALLOCATABLE and so "+ + "applies more than the figure printed beside it: %q", r.Detail) + } + if r.Detail != "" && !strings.Contains(r.Detail, "resources set --cores") { + t.Fatalf("nudge does not name the explicit command that applies exactly "+ + "what it printed: %q", r.Detail) + } + }) t.Run("heterogeneous nodes: nudge quotes the CPU-major node, matching set max (Bugbot)", func(t *testing.T) { // resources.LargestReadyNode (what `set max` applies) is CPU-major: // it picks cpuBig (32/64Gi -> max 31/61Gi), not memBig (8/128Gi -> @@ -693,6 +744,185 @@ func TestCheckNodeFit(t *testing.T) { }) } +// cpPod is a control-plane pod scheduled on a node, requesting memory — the +// neighbour whose requests the fit must subtract (backend#2870). +func cpPod(name, nodeName, mem string) *corev1.Pod { + return podOn(name, nodeName, "", mem, nil) +} + +// podOn is a running pod on a node requesting cpu/mem (empty = unset), with +// optional labels (e.g. the batch/v1 job-name label that marks a training pod). +func podOn(name, nodeName, cpu, mem string, labels map[string]string) *corev1.Pod { + reqs := corev1.ResourceList{} + if cpu != "" { + reqs[corev1.ResourceCPU] = resource.MustParse(cpu) + } + if mem != "" { + reqs[corev1.ResourceMemory] = resource.MustParse(mem) + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "kube-system", Labels: labels}, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Containers: []corev1.Container{{Name: "c", Resources: corev1.ResourceRequirements{Requests: reqs}}}, + }, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, + } +} + +// backend#2870: node-fit must be against FREE memory (allocatable − what is +// already requested on the node), not allocatable. A node big enough by +// allocatable but over-committed by the control plane it hosts must FAIL. +func TestCheckNodeFitFreeMemory(t *testing.T) { + req := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi"} + + t.Run("allocatable fits but FREE does not -> fail", func(t *testing.T) { + // 16Gi node, control plane already claims 12Gi -> 4Gi free < 8Gi envelope. + // Allocatable (16Gi) "fits"; free (4Gi) does not. + cs := fake.NewClientset(node("n1", "4", "16Gi"), cpPod("cp", "n1", "12Gi")) + r := checkNodeFit(bg(), cs, req) + if r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail (over-committed)", r.Status, r.Detail) + } + if !strings.Contains(r.Detail, "FREE") || !strings.Contains(r.Detail, "over-asks") { + t.Fatalf("detail should name the free-memory over-commit: %q", r.Detail) + } + }) + + // The subtraction is load-bearing: the SAME node without the neighbour passes, + // so it is the pod request — not the node size — that flips the verdict. + t.Run("same node without the neighbour -> ok", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "16Gi")) + if r := checkNodeFit(bg(), cs, req); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok", r.Status, r.Detail) + } + }) + + // A terminal pod holds no memory; free stays 16Gi and the job fits. + t.Run("terminal neighbour does not consume free", func(t *testing.T) { + done := cpPod("old", "n1", "12Gi") + done.Status.Phase = corev1.PodSucceeded + cs := fake.NewClientset(node("n1", "4", "16Gi"), done) + if r := checkNodeFit(bg(), cs, req); r.Status != StatusOK { + t.Fatalf("=> %v (%q), want ok (terminal pod ignored)", r.Status, r.Detail) + } + }) + + // 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) { + 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) + } + }) + + // The over-commit message names the SHORT dimension, not always memory (Bugbot). + // Control plane claims cpu (3 of 4), leaving 1 free < the 2-cpu envelope; memory + // is fine. The message must say cpu, not memory. + t.Run("over-commit on cpu names cpu, not memory", func(t *testing.T) { + cp := podOn("cp", "n1", "3", "", nil) // 3 cpu, no memory + cs := fake.NewClientset(node("n1", "4", "16Gi"), cp) + r := checkNodeFit(bg(), cs, req) // needs cpu=2, memory=8Gi + if r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail (cpu over-commit)", r.Status, r.Detail) + } + if !strings.Contains(r.Detail, "FREE cpu") { + t.Fatalf("detail should name FREE cpu, not memory: %q", r.Detail) + } + }) + + // A DISK-only shortfall must not take the over-commit arm (Bugbot Medium): + // free cpu+memory are fine, only ephemeral-storage is short, so the message + // must name disk via the generic fail, not blame FREE memory. + t.Run("disk-only shortfall is not blamed on memory", func(t *testing.T) { + diskReq := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi,ephemeral-storage=50Gi"} + n := nodeWithDisk("n1", "4", "16Gi", "20Gi") // cpu+mem fit free; disk 20Gi < 50Gi + cs := fake.NewClientset(n) + r := checkNodeFit(bg(), cs, diskReq) + if r.Status != StatusFail { + t.Fatalf("=> %v (%q), want fail (disk)", r.Status, r.Detail) + } + if strings.Contains(r.Detail, "over-asks") || strings.Contains(r.Detail, "FREE memory") { + t.Fatalf("disk shortfall must not be reported as a memory over-commit: %q", r.Detail) + } + if !strings.Contains(r.Detail, "ephemeral-storage") { + t.Fatalf("detail should name ephemeral-storage: %q", r.Detail) + } + }) + + // UNKNOWN free is not a clean pass (Bugbot High): when the pod list can't be + // read, the fit falls back to allocatable and must WARN with a caveat, never + // green node capacity as if free were verified. + t.Run("unknown free (pod list fails) -> warn with caveat", func(t *testing.T) { + cs := fake.NewClientset(node("n1", "4", "16Gi")) + cs.PrependReactor("list", "pods", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("pods is forbidden") + }) + r := checkNodeFit(bg(), cs, req) + if r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want warn (free unknown)", r.Status, r.Detail) + } + if !strings.Contains(r.Detail, "allocatable only") { + t.Fatalf("detail should caveat allocatable-only: %q", r.Detail) + } + }) + + // Bugbot High on #628: the SOFT GPU warn used to outrank this one. + // + // `gpuRequested && !fullFits` sits above the `!freeKnown` branch and its + // detail carried no can't-check prefix, so with a GPU requested and the pod + // list unreadable the GPU Warn won, the caveat was never emitted, and + // `summarizeDoctor` -- which classifies by PREFIX -- fell through to "Ready + // to run training" at exit 0 over a cluster whose free compute doctor had + // not looked at. The chart stamps `nvidia.com/gpu` on CPU-only installs, so + // this is the common shape rather than an exotic one. + t.Run("unknown free AND gpu requested -> can't-check wins, GPU still reported", func(t *testing.T) { + gpu := map[string]string{ + "RESOURCE_REQUESTS": "cpu=2,memory=8Gi", + "GPU_REQUESTS": "nvidia.com/gpu=1", + } + cs := fake.NewClientset(node("n1", "4", "16Gi")) // cpu/mem fit, NO gpu + cs.PrependReactor("list", "pods", func(k8stesting.Action) (bool, runtime.Object, error) { + 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) + } + // PREFIX, not Contains: that is what the rollup matches on, so a detail + // merely mentioning the phrase somewhere would still green the run. + if !strings.HasPrefix(r.Detail, CantVerifyFreeCompute) { + t.Fatalf("detail must START with %q so summarizeDoctor classifies it as a can't-check, got %q", + CantVerifyFreeCompute, r.Detail) + } + // The soft GPU fact is not lost, it is just no longer the whole story. + if !strings.Contains(r.Detail, "nvidia.com/gpu") { + t.Fatalf("the GPU fallback should still be reported: %q", r.Detail) + } + }) + + // The other side, so the fix above cannot be read as "always warn about free": + // with the pod list READABLE, the GPU warn keeps its own wording and must not + // claim a can't-check. + t.Run("gpu requested, free KNOWN -> plain GPU warn, no can't-check prefix", func(t *testing.T) { + gpu := map[string]string{ + "RESOURCE_REQUESTS": "cpu=2,memory=8Gi", + "GPU_REQUESTS": "nvidia.com/gpu=1", + } + cs := fake.NewClientset(node("n1", "4", "16Gi")) + r := checkNodeFit(bg(), cs, gpu) + if r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want warn", r.Status, r.Detail) + } + if strings.HasPrefix(r.Detail, CantVerifyFreeCompute) { + t.Fatalf("free WAS readable; this must not report a can't-check: %q", r.Detail) + } + }) +} + func dockerSecret(name string, data []byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns},