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) {