From 476d7b65e42cd0b8a70ff73a593fb6a10f4ef4e9 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 09:45:27 +0200 Subject: [PATCH 1/2] fix(doctor): don't treat an assigned-but-Pending training pod as a running job (backend#3247) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkNodeFit counted every non-terminal `job-name` pod with a NodeName into the "a running job holds the room" sum. A training pod scheduled to a node but still Pending (ImagePullBackOff / ContainerCreating) was therefore reported as HeldByRunningJob, and combined with the same pod showing up stuck-Pending in Pod health, summarizeDoctor rolled it up to "a training is already running, wait for it to finish" at exit 0 — on a pod that is wedged and never will. The operator waits forever on a dead pod. A Pending pod (even with a NodeName) is not running: - checkNodeFit routes a `job-name` pod into the running-job sums only when it is genuinely Running; an assigned-but-Pending one feeds neither sum, so it can no longer emit HeldByRunningJob. - past the same grace window checkPods uses, such a pod is recorded and surfaced as an actionable Fail (new StuckJobPod prefix) that names its real state (ImagePullBackOff / ContainerCreating / Pending) and says to inspect the pod, not wait. A run legitimately waiting for a running job has no NodeName, so it is never mistaken for a wedged one. - summarizeDoctor gains a matching arm above the inferred stuck-Pending arm (measured beats inferred), so the wedged pod fails at exit 2 with the right remedy instead of the "wait for the running job" Warn at exit 0. The genuinely-running case is unchanged. Tests pin each pod state (Running vs assigned-Pending vs ImagePullBackOff/ContainerCreating vs within-grace) at both the checkNodeFit and summarizeDoctor levels. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 24 ++++ internal/cli/doctor_test.go | 50 +++++++ .../cli/testdata/golden/zz-all-strings.golden | 5 + internal/doctor/doctor.go | 110 +++++++++++++++- internal/doctor/doctor_test.go | 123 ++++++++++++++++++ 5 files changed, 306 insertions(+), 6 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index d11acf7..d2187e6 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -519,6 +519,30 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusWarn, "Ready to run training — a training is already running, and the next one is waiting for it to finish.", fmt.Sprintf("A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`.", launcher())} + case by["Node capacity"].Status == doctor.StatusFail && + strings.HasPrefix(by["Node capacity"].Detail, doctor.StuckJobPod): + // A training pod is scheduled to a node but stuck Pending -- an image pull + // backing off, or a container stuck creating -- NOT running (backend#3247). + // checkNodeFit used to count it as a running job holding the room, so with + // a pod also stuck Pending this rolled up through `stuckPending && heldByJob` + // to the transient "a training is already running, wait for it" Warn at + // exit 0 -- on a pod that is wedged and never will. + // + // IT SITS ABOVE THE STUCK-PENDING ARM by the same measured-beats-inferred + // rule that puts `stuckPending && heldByJob` there: the stuck-Pending arm is + // an INFERENCE ("Pending past grace, so compute or image"), while + // checkNodeFit has MEASURED the cause -- the pod is scheduled but its + // containers are not running. The two co-occur (a wedged pod is also Pending + // past grace), and below that arm this precise, correct remedy would be + // shadowed by the generic "usually not enough free compute" one, putting + // `resources set max` in front of an operator whose image simply will not + // pull. Only the measured capacity Fails (OverCommitted) and a hard + // Pod-health crash-loop Fail outrank it, exactly as they outrank the generic + // stuck-Pending arm. Its remedy is the OPPOSITE of the transient Warn's + // (inspect the pod, do NOT wait); the pod it names is one `--verbose` away. + ready = healthLine{doctor.StatusFail, + "Not ready — a training pod is stuck starting and isn't running yet.", + fmt.Sprintf("A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod, then `kubectl describe pod` it. Asking for less per run or resizing will not help.", launcher())} case stuckPending: // Pods stuck Pending past the grace window (unschedulable / image can't // pull) mean training can't actually schedule — so this is NOT ready, even diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index acd12ce..075fcd6 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -583,6 +583,56 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3247: the defect combination. A training pod is scheduled but + // wedged on an image pull (Node capacity Fail, StuckJobPod) and Pod health + // also sees it Pending past grace (stuckPending). This used to roll up through + // `stuckPending && heldByJob` to "a training is already running, wait for it + // to finish" at exit 0 -- because checkNodeFit mislabelled the wedged pod a + // running job. Now the Node-capacity Fail is a MEASURED cause and outranks the + // inferred stuck-Pending arm: a Fail that says the pod is stuck, whose remedy + // is to inspect the pod, not to wait or resize. + t.Run("a scheduled-but-stuck training pod is a Fail, not the wait-for-a-running-job Warn", func(t *testing.T) { + results := withDetail(allOK, "Node capacity", doctor.StatusFail, + doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ImagePullBackOff). The next run does not wait on a pod that is not running") + results = withDetail(results, "Pod health", doctor.StatusWarn, + "Pending > 5m0s: [train-stuck]") + c, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a wedged training pod must fail the rollup, got %v (%q)", r.status, r.text) + } + if strings.Contains(r.text, "already running") || strings.Contains(r.text, "waiting for it") { + t.Errorf("must not tell the operator to wait on a job that is not running: %q", r.text) + } + if !strings.Contains(r.text, "stuck starting") { + t.Errorf("the top line should say the pod is stuck starting, got %q", r.text) + } + if strings.Contains(r.remedy, "resources set max") || strings.Contains(r.remedy, "Ask for less") { + t.Errorf("resizing does not clear an image-pull stall; the remedy must not send them there: %q", r.remedy) + } + if !strings.Contains(r.remedy, "kubectl describe pod") { + t.Errorf("the remedy should point at inspecting the stuck pod, got %q", r.remedy) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("verdict must be a Fail (exit 2), not a clean pass, got %v", v) + } + }) + + // The same measured Fail with Pod health NOT flagging it (checkPods could be + // scoped to a namespace that missed it, or unable to list). The dedicated arm + // must still fire on the Node-capacity signal alone -- never falling through + // to a green. + t.Run("a scheduled-but-stuck training pod fails even when Pod health is silent", func(t *testing.T) { + results := withDetail(allOK, "Node capacity", doctor.StatusFail, + doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ContainerCreating). The next run does not wait on a pod that is not running") + c, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail || !strings.Contains(r.text, "stuck starting") { + t.Fatalf("want the stuck-pod Fail on the Node-capacity signal alone, got %v (%q)", r.status, r.text) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("verdict must be a Fail (exit 2), got %v", v) + } + }) + t.Run("a crash-looping pod still outranks the running-job explanation", func(t *testing.T) { // The exception is scoped to the stuck-Pending WARN; a Pod-health FAIL is // a measured failure with a different fix, and must keep winning. diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index e238a3e..c1b18f2 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -81,6 +81,8 @@ screen. %s/%d are runtime placeholders. "%s, so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here. Also, no single Ready node satisfies cpu+memory AND %s, so GPU jobs would rely on the CPU fallback (needs %s)" "%s, … and %d more" "%s/%s" +"%s/%s on %s (%s)" +"%s: %s. The next run does not wait on a pod that is not running" "%s: %v" "%s: %w" "%s: a Ready node fits a training job (%s) beside the platform's own pods, but running job(s) on %s hold %s right now, so the next run waits Pending until they finish" @@ -127,6 +129,7 @@ screen. %s/%d are runtime placeholders. "A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade" "A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`." "A real run continues with step 2 (copy into your secure environment) and step 3 (validate and load)." +"A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod, then `kubectl describe pod` it. Asking for less per run or resizing will not help." "A tracebloc client is already running on this cluster — adopting it. Couldn't read the cluster identity, so its idempotency anchor was left unchanged; point --kubeconfig/--context at a cluster where kube-system is readable to stamp it." "A training run is allocated up to:" "Add --help to any command for the flags." @@ -251,6 +254,7 @@ screen. %s/%d are runtime placeholders. "Ingestion summary" "Ingestor SA token" "Ingests a local dataset into your secure environment's storage,\nsubmits the ingestion run, and follows it to completion (streaming\nprogress + the final summary). Your data never leaves your own\ninfrastructure. Supports %[1]d tasks across the image, text, and\ntabular / time-series families; pick one with --task.\n\n is the data itself. What it looks like depends on the task:\n\n tabular / time-series — the dataset is a single CSV. Pass the .csv\n file directly, or a folder holding exactly one .csv:\n\n churn.csv (the .csv file itself)\n or\n churn/\n data.csv (the one .csv in the folder)\n\n image classification / keypoint detection — a folder with\n labels.csv + an images/ subfolder:\n\n cats_dogs/\n labels.csv (required)\n images/ (required)\n 001.jpg\n ...\n\n object detection — a folder with images/ + annotations/ and NO\n labels.csv: records are enumerated from the Pascal-VOC XML, one per\n image, so there is no manifest and no label column to declare.\n\n visdrone/\n images/ (required)\n 001.jpg\n annotations/ (required — 001.xml pairs with 001.jpg)\n 001.xml\n\n text (classification, masked language modeling) — a folder with\n labels.csv + a %[2]s/ subfolder (masked language modeling uses %[3]s/):\n\n reviews/\n labels.csv (required)\n %[2]s/ (required — %[3]s/ for masked language modeling)\n 001.txt\n ...\n\nA bare .csv file is accepted only for the tabular / time-series family;\nimage and text datasets must be a folder.\n\nAccepted image extensions: .jpg, .jpeg, or .png (case-insensitive).\nAll images in one dataset must share a single type — the cluster\nvalidates the type it was told to expect.\n\nv0.1 caps the dataset at 1 GiB total + 500 MiB per file. Larger\ndatasets need the v0.2 cloud-source story (S3/GCS/HTTPS sources) —\nsee tracebloc/client#147 non-goals.\n\nExit codes:\n 0 files staged + ingested successfully (or --detach: just staged + submitted)\n 2 schema validation failed (synthesized spec rejected) or\n v0.1-unsupported task passed\n 3 local-layout or kubeconfig error\n 4 cluster reachable but no tracebloc client / shared storage missing\n 5 ingestor SA token couldn't be obtained, or jobs-manager\n rejected the token (401/403)\n 6 destination table already exists (re-run with --overwrite to\n replace it, or pick a different --name)\n 7 pre-flight succeeded but staging the files failed\n (Pod creation, image pull, exec stream, or remote tar error) —\n or, with --overwrite, removing the old table failed\n 8 jobs-manager rejected the submit (4xx/5xx other than auth)\n 9 ingestion Job exited non-zero, or completed with row-level\n failures the summary panel reports" +"Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container stuck creating. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it." "Interrupted before the change could be confirmed." "It may already have applied — re-run `%s resources set` to check the current per-run ceiling." "It reports more memory than the machine really has, so two trainings that each look like they fit can together run it out of memory and take the environment down. Run one training at a time; to fix it for good, recreate the environment as a single-node one. `%s doctor --verbose` shows the numbers and the exact flags." @@ -281,6 +285,7 @@ screen. %s/%d are runtime placeholders. "Not connected — couldn't read your secure environment." "Not connected — tracebloc didn't confirm your session (server error)." "Not connected — your secure environment isn't answering." +"Not ready — a training pod is stuck starting and isn't running yet." "Not ready — dataset storage isn't available." "Not ready — not enough free compute to start a training." "Not ready — part of your secure environment can't start yet." diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 76a8939..74d4f02 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -405,6 +405,23 @@ func podCrashLooping(p corev1.Pod) bool { return false } +// podWaitingReason names why an assigned pod is still Pending -- the kubelet's +// own Waiting.Reason from the first container that reports one, e.g. +// "ImagePullBackOff", "ErrImagePull", "ContainerCreating". Init containers are +// read first: an init container stalls startup before the app containers begin. +// Falls back to the bare phase ("Pending") when no container has reported a +// reason yet, so the caller always has something concrete to print. +func podWaitingReason(p corev1.Pod) string { + for _, group := range [][]corev1.ContainerStatus{p.Status.InitContainerStatuses, p.Status.ContainerStatuses} { + for _, c := range group { + if c.State.Waiting != nil && c.State.Waiting.Reason != "" { + return c.State.Waiting.Reason + } + } + } + return string(p.Status.Phase) +} + // checkRestartHistory surfaces containers that have restarted repeatedly even // though they are not crash-looping right now — the restart-*history* signal // backend#1028 asked for. checkPods reads only the current waiting reason, so a @@ -583,6 +600,16 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, } } +// stuckJobPod is a training-Job pod the scheduler has placed on a node but that +// is not Running yet -- carried out of checkNodeFit's resource sums so a wedged +// pod is never mistaken for a running job holding the room (backend#3247). +type stuckJobPod struct { + name string + namespace string + node string + reason string // the kubelet's Waiting.Reason, e.g. ImagePullBackOff +} + // checkNodeFit verifies at least one Ready node can satisfy the resource // requests the jobs-manager stamps on spawned training jobs (RESOURCE_REQUESTS // / GPU_REQUESTS env) — the "Pending forever, no node big enough" class. The @@ -652,6 +679,19 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // next run was going to wait. Two sums, two verdicts. jobCPU := map[string]int64{} // node -> millicores held by running Jobs jobMem := map[string]int64{} // node -> bytes held by running Jobs + // Training-Job pods the scheduler has already PLACED on a node (they carry a + // NodeName) but that are NOT yet Running -- an image pull backing off, a + // container stuck creating. Kept out of BOTH sums above: a pod that is not + // running holds no room in the "a running job will finish and free it" sense, + // and counting one as a running job is exactly what made doctor emit + // HeldByRunningJob and tell the operator to "wait for the job to finish" at + // exit 0 on a pod that is wedged and never will (backend#3247). Collected here + // (past the same grace window checkPods uses) so the verdict can name the real + // stuck state as an actionable finding instead. Recognised cluster-wide by the + // same `job-name` convention the running-job sum above uses -- a dedicated + // secure environment's batch Jobs are tracebloc's -- so "stuck" and + // "holds the room" agree on what a training/ingestion pod is. + var stuckJobs []stuckJobPod freeKnown := true if pods, perr := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}); perr == nil { for i := range pods.Items { @@ -666,14 +706,34 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s // Batch-Job pods carry the `job-name` label the batch/v1 controller // stamps (see internal/submit/watch.go) -- that label, not a name // pattern, is how the CLI already recognises a training or ingestion - // pod, so it is the one used here. They go into the JOB sums, never the - // steady-state ones: a running job holds the envelope itself, so - // counting it as platform would make doctor Fail "no room for a training - // job" on the exact healthy state it exists to bless (Bugbot High). The - // steady state is the control plane (Deployments/DaemonSets), which has - // no `job-name`. + // pod, so it is the one used here. A running job goes into the JOB sums, + // never the steady-state ones: a running job holds the envelope itself, + // so counting it as platform would make doctor Fail "no room for a + // training job" on the exact healthy state it exists to bless (Bugbot + // High). The steady state is the control plane (Deployments/DaemonSets), + // which has no `job-name`. cpu, mem := reqCPU, reqMem if _, isJob := p.Labels["job-name"]; isJob { + // A Job pod holds the room ONLY when it is genuinely Running. An + // assigned-but-Pending one (it has a NodeName but its containers have + // not started -- image pull backing off, container stuck creating) is + // NOT running: counting it as a running job made doctor emit + // HeldByRunningJob and tell the operator to wait for it to finish -- + // on a pod that is wedged (backend#3247). It feeds NEITHER sum. Past + // the grace window it is recorded as stuck and surfaced below; a pod + // merely WAITING for capacity has no NodeName yet and was skipped + // above, so this only ever catches pods the scheduler already placed. + if p.Status.Phase != corev1.PodRunning { + if time.Since(p.CreationTimestamp.Time) > pendingGrace { + stuckJobs = append(stuckJobs, stuckJobPod{ + name: p.Name, + namespace: p.Namespace, + node: p.Spec.NodeName, + reason: podWaitingReason(p), + }) + } + continue + } cpu, mem = jobCPU, jobMem } for j := range p.Spec.Containers { @@ -844,6 +904,32 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Detail: detail, Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } + case len(stuckJobs) > 0: + // A training Job pod the scheduler already placed on a node is stuck + // Pending -- an image pull backing off, or a container that will not finish + // creating -- NOT running (backend#3247). It used to be counted as a + // running job holding the room, so this state rolled up to the transient + // "a training is already running, wait for it to finish" Warn at exit 0 -- + // on a pod that is wedged and never will. It is a real, training-blocking + // problem whose remedy is the OPPOSITE of the transient Warn's (inspect the + // pod, do NOT wait), so it is a Fail with its own prefix the rollup + // (summarizeDoctor) classifies on. Ordering matters: it sits BELOW the two + // measured capacity Fails above -- when no node can fit the envelope that + // is the root cause -- and ABOVE the transient Warn, which a wedged pod + // must never be mistaken for. A run legitimately WAITING for a running job + // has no NodeName, so it is never collected here. + sort.Slice(stuckJobs, func(a, b int) bool { return stuckJobs[a].name < stuckJobs[b].name }) + descs := make([]string, len(stuckJobs)) + for i, s := range stuckJobs { + descs[i] = fmt.Sprintf("%s/%s on %s (%s)", s.namespace, s.name, s.node, s.reason) + } + first := stuckJobs[0] + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("%s: %s. The next run does not wait on a pod that is not running", StuckJobPod, strings.Join(descs, ", ")), + Remedy: fmt.Sprintf("Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container stuck creating. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it.", first.namespace, first.name), + } case freeKnown && !nowFits: // The TRANSIENT shortage (backend#2870): every dimension fits beside the // platform's own pods, so this machine CAN run the envelope -- but a batch @@ -1007,6 +1093,18 @@ const OverCommitted = "a Ready node is large enough" // that drifted would send this state down the generic path again. const HeldByRunningJob = "a running job holds the room" +// StuckJobPod is the prefix of the backend#3247 Fail: a training Job pod is +// scheduled to a node (it has a NodeName) but is NOT Running -- Pending on an +// image pull backing off or a container stuck creating. It used to be counted +// as a running job holding the room, so checkNodeFit emitted HeldByRunningJob +// and the rollup told the operator to "wait for the job to finish" at exit 0 -- +// on a pod that is wedged and never will. DISTINCT prefix, same discipline as +// HeldByRunningJob and OverCommitted: the rollup (summarizeDoctor) classifies on +// it, and its remedy ("inspect the stuck pod, do not wait") is the opposite of +// the transient Warn's ("wait for the running job to finish"). A retyped copy +// that drifted would send a wedged pod back down the "just wait" path. +const StuckJobPod = "a training pod is scheduled but not running" + // cpuMemString renders a millicore/byte pair the way RESOURCE_REQUESTS reads // ("cpu=2, memory=8Gi"), so the free/held figures in a Node-capacity verdict // line up with the request printed beside them. diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index b44c10b..9d1b101 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -1103,6 +1103,129 @@ func TestCheckNodeFitPermanentVsTransient(t *testing.T) { }) } +// assignedPendingJobPod is a training-Job pod (the batch/v1 job-name label) the +// scheduler has already PLACED on a node -- it has a NodeName -- but that is NOT +// Running: still Pending, with the kubelet's Waiting.Reason on its container +// (e.g. "ImagePullBackOff", "ContainerCreating"; empty means bare Pending). age +// sets how long ago it was created so a test can sit inside or outside +// checkNodeFit's grace window. It requests memory so a test can also prove that +// request is never folded into the "held by a running job" sum. +func assignedPendingJobPod(name, nodeName, reason, mem string, age time.Duration) *corev1.Pod { + reqs := corev1.ResourceList{} + if mem != "" { + reqs[corev1.ResourceMemory] = resource.MustParse(mem) + } + var statuses []corev1.ContainerStatus + if reason != "" { + statuses = []corev1.ContainerStatus{{ + Name: "c", + State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: reason}}, + }} + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: map[string]string{"job-name": "exp-42"}, + CreationTimestamp: metav1.NewTime(time.Now().Add(age)), + }, + Spec: corev1.PodSpec{ + NodeName: nodeName, + Containers: []corev1.Container{{Name: "c", Resources: corev1.ResourceRequirements{Requests: reqs}}}, + }, + Status: corev1.PodStatus{Phase: corev1.PodPending, ContainerStatuses: statuses}, + } +} + +// backend#3247: an assigned-but-Pending training pod is NOT a running job. +// checkNodeFit used to route every non-terminal job-name pod with a NodeName +// into the "a running job holds the room" sum, so a pod wedged on an image pull +// emitted HeldByRunningJob and the rollup told the operator to wait for a job +// that was not running. Each pod STATE must land on its own verdict. +func TestCheckNodeFitStuckJobPod(t *testing.T) { + req := map[string]string{"RESOURCE_REQUESTS": "cpu=2,memory=8Gi"} + // A node big enough that, with NOTHING holding the room, the envelope fits -- + // so any non-OK verdict below is about the pod's STATE, not the machine. + fitNode := func() *corev1.Node { return node("n1", "4", "16Gi") } + const old = -10 * time.Minute // older than the 5m grace window + const fresh = -1 * time.Minute // inside it + + // A genuinely RUNNING job that fills the room is the transient Warn -- the + // pre-existing behaviour this fix must leave untouched. + t.Run("Running job filling the room -> HeldByRunningJob Warn (unchanged)", func(t *testing.T) { + job := podOn("train-run", "n1", "", "12Gi", map[string]string{"job-name": "exp-42"}) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), job), req) + if r.Status != StatusWarn || !strings.HasPrefix(r.Detail, HeldByRunningJob) { + t.Fatalf("=> %v (%q), want the transient Warn %q", r.Status, r.Detail, HeldByRunningJob) + } + }) + + // The defect, one waiting reason per row: assigned, Pending, past grace. Each + // must be a stuck Fail that NAMES its reason -- never HeldByRunningJob. + for _, tc := range []struct{ name, reason, wantInDetail string }{ + {"ImagePullBackOff", "ImagePullBackOff", "ImagePullBackOff"}, + {"ContainerCreating", "ContainerCreating", "ContainerCreating"}, + {"bare Pending, no reason yet", "", "Pending"}, + } { + t.Run("assigned+Pending past grace ("+tc.name+") -> stuck Fail", func(t *testing.T) { + pod := assignedPendingJobPod("train-stuck", "n1", tc.reason, "12Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if r.Status != StatusFail { + t.Fatalf("=> %v (%q), want Fail: a wedged pod is not a running job", r.Status, r.Detail) + } + if !strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("detail must START with %q so the rollup classifies it, got %q", StuckJobPod, r.Detail) + } + if strings.Contains(r.Detail, HeldByRunningJob) { + t.Fatalf("a wedged pod must never be reported as a running job: %q", r.Detail) + } + if !strings.Contains(r.Detail, tc.wantInDetail) { + t.Errorf("detail should name the real state %q, got %q", tc.wantInDetail, r.Detail) + } + if !strings.Contains(r.Remedy, "kubectl describe pod") { + t.Errorf("the remedy must be actionable (inspect the pod), got %q", r.Remedy) + } + }) + } + + // A pod the scheduler JUST placed and is normally starting (inside grace) is + // neither a running job nor stuck -- flagging it would false-positive on + // every training launch. Not counted as held either, so the node reads OK. + t.Run("assigned+Pending INSIDE grace -> not flagged, node still OK", func(t *testing.T) { + pod := assignedPendingJobPod("train-young", "n1", "ContainerCreating", "12Gi", fresh) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if r.Status != StatusOK { + t.Fatalf("=> %v (%q), want OK: a freshly-scheduled pod is normal startup", r.Status, r.Detail) + } + if strings.Contains(r.Detail, StuckJobPod) || strings.Contains(r.Detail, HeldByRunningJob) { + t.Fatalf("a within-grace pod must be neither stuck nor held: %q", r.Detail) + } + }) + + // Scope guard: the fix keys on the job-name label. A non-Job pod (platform) + // that is Pending-assigned is steady state and must not take the stuck-pod arm. + t.Run("a non-Job Pending pod is not a stuck training pod", func(t *testing.T) { + plat := assignedPendingJobPod("some-deploy", "n1", "ContainerCreating", "12Gi", old) + plat.Labels = nil // not a job-name pod + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), plat), req) + if strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("the stuck-training-pod arm must be scoped to job-name pods, got %q", r.Detail) + } + }) + + // A running job holds the room AND a second training pod is wedged on an + // image pull: the wedged pod is the actionable problem and its Fail must win + // over the transient Warn (the stuck arm sits above it in the switch). + t.Run("a stuck pod outranks a genuinely running job", func(t *testing.T) { + running := podOn("train-run", "n1", "", "12Gi", map[string]string{"job-name": "exp-42"}) + stuck := assignedPendingJobPod("train-stuck", "n1", "ImagePullBackOff", "1Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), running, stuck), req) + if r.Status != StatusFail || !strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("=> %v (%q), want the stuck Fail to win over the transient Warn", r.Status, r.Detail) + } + }) +} + func dockerSecret(name string, data []byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, From 0e28595a3446d2ec41280e6a6c11e348b884d18a Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Mon, 7 Sep 2026 10:39:39 +0200 Subject: [PATCH 2/2] fix(doctor): scope the stuck-pod Fail to wedged reasons; keep the rollup plain-terms (backend#3247) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Saqlain's review on cli#642: - summarizeDoctor's stuck-pod ready line no longer names `kubectl` — it ends at "… names the pod", restoring the plain-terms invariant the function documents three times. The kubectl form stays in the granular checkNodeFit remedy, one --verbose away. - checkNodeFit escalates to the StuckJobPod Fail only for a genuinely-wedged waiting reason (ImagePullBackOff / ErrImagePull / ErrImageNeverPull / InvalidImageName / CreateContainerConfigError / CreateContainerError), via a new stuckReasonWedged allowlist. A pod still pulling/creating past the grace window (ContainerCreating / Pulling / bare Pending) is left to checkPods' age-based inference, so "waiting will not clear it" is only asserted where the kubelet has actually reported a failure — a large first pull on a cold node no longer trips the confident Fail. Tests updated to pin wedged reasons (→ stuck Fail) apart from still-progressing ones (→ deferred, node OK), and to guard the rolled-up line against Kubernetes vocabulary. Copy-catalog golden regenerated. go build/vet/test ./... green. Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 25 +++-- internal/cli/doctor_test.go | 12 +- .../cli/testdata/golden/zz-all-strings.golden | 4 +- internal/doctor/doctor.go | 104 +++++++++++------- internal/doctor/doctor_test.go | 48 ++++++-- 5 files changed, 127 insertions(+), 66 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index d2187e6..77f99a8 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -529,20 +529,21 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // exit 0 -- on a pod that is wedged and never will. // // IT SITS ABOVE THE STUCK-PENDING ARM by the same measured-beats-inferred - // rule that puts `stuckPending && heldByJob` there: the stuck-Pending arm is - // an INFERENCE ("Pending past grace, so compute or image"), while - // checkNodeFit has MEASURED the cause -- the pod is scheduled but its - // containers are not running. The two co-occur (a wedged pod is also Pending - // past grace), and below that arm this precise, correct remedy would be - // shadowed by the generic "usually not enough free compute" one, putting - // `resources set max` in front of an operator whose image simply will not - // pull. Only the measured capacity Fails (OverCommitted) and a hard - // Pod-health crash-loop Fail outrank it, exactly as they outrank the generic - // stuck-Pending arm. Its remedy is the OPPOSITE of the transient Warn's - // (inspect the pod, do NOT wait); the pod it names is one `--verbose` away. + // rule that puts `stuckPending && heldByJob` there. checkNodeFit escalates + // to this Fail ONLY for a genuinely-wedged reason -- an image pull backing + // off or a create error -- so the cause is measured, not inferred: "waiting + // will not clear it" is exact. A pod merely still pulling/creating past the + // grace window is left to the stuck-Pending arm below, whose "usually not + // enough free compute, or an image that can't be pulled" wording is an + // honest age-based inference (a large first pull on a cold node CAN exceed + // the grace). Only the measured capacity Fails (OverCommitted) and a hard + // Pod-health crash-loop Fail outrank it. Its remedy is the OPPOSITE of the + // transient Warn's (inspect the pod, do NOT wait); the pod it names is one + // `--verbose` away -- and PLAIN TERMS, no Kubernetes vocabulary, like its + // neighbours (the granular checkNodeFit remedy carries the `kubectl` form). ready = healthLine{doctor.StatusFail, "Not ready — a training pod is stuck starting and isn't running yet.", - fmt.Sprintf("A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod, then `kubectl describe pod` it. Asking for less per run or resizing will not help.", launcher())} + fmt.Sprintf("A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod.", launcher())} case stuckPending: // Pods stuck Pending past the grace window (unschedulable / image can't // pull) mean training can't actually schedule — so this is NOT ready, even diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index 075fcd6..cadd225 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -609,8 +609,14 @@ func TestSummarizeDoctor(t *testing.T) { if strings.Contains(r.remedy, "resources set max") || strings.Contains(r.remedy, "Ask for less") { t.Errorf("resizing does not clear an image-pull stall; the remedy must not send them there: %q", r.remedy) } - if !strings.Contains(r.remedy, "kubectl describe pod") { - t.Errorf("the remedy should point at inspecting the stuck pod, got %q", r.remedy) + // PLAIN TERMS: this rolled-up line must carry no Kubernetes vocabulary -- + // the `kubectl` form lives in the granular checkNodeFit remedy, one + // `--verbose` away (the invariant summarizeDoctor documents three times). + if strings.Contains(r.remedy, "kubectl") { + t.Errorf("the rolled-up remedy must stay plain-terms, no `kubectl`: %q", r.remedy) + } + if !strings.Contains(r.remedy, "--verbose") { + t.Errorf("the remedy should point at --verbose to name the pod, got %q", r.remedy) } if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { t.Errorf("verdict must be a Fail (exit 2), not a clean pass, got %v", v) @@ -623,7 +629,7 @@ func TestSummarizeDoctor(t *testing.T) { // to a green. t.Run("a scheduled-but-stuck training pod fails even when Pod health is silent", func(t *testing.T) { results := withDetail(allOK, "Node capacity", doctor.StatusFail, - doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ContainerCreating). The next run does not wait on a pod that is not running") + doctor.StuckJobPod+": tracebloc/train-stuck on n1 (ErrImagePull). The next run does not wait on a pod that is not running") c, r := summarizeDoctor(results, tokenOK) if r.status != doctor.StatusFail || !strings.Contains(r.text, "stuck starting") { t.Fatalf("want the stuck-pod Fail on the Node-capacity signal alone, got %v (%q)", r.status, r.text) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index c1b18f2..926ee30 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -129,7 +129,7 @@ screen. %s/%d are runtime placeholders. "A newer tracebloc is available: %s (you have %s). Update: tracebloc upgrade" "A pod is waiting to start because a running job holds this machine's free compute. Let the job finish, or stop it if it is not needed; asking for less per run or resizing will not help — the room comes back when the job ends. If the pod is still waiting after that, something else is holding it: `%s doctor --verbose`." "A real run continues with step 2 (copy into your secure environment) and step 3 (validate and load)." -"A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod, then `kubectl describe pod` it. Asking for less per run or resizing will not help." +"A scheduled training pod is stuck (usually a training image that can't be pulled). Waiting will not clear it — `%s doctor --verbose` names the pod." "A tracebloc client is already running on this cluster — adopting it. Couldn't read the cluster identity, so its idempotency anchor was left unchanged; point --kubeconfig/--context at a cluster where kube-system is readable to stamp it." "A training run is allocated up to:" "Add --help to any command for the flags." @@ -254,7 +254,7 @@ screen. %s/%d are runtime placeholders. "Ingestion summary" "Ingestor SA token" "Ingests a local dataset into your secure environment's storage,\nsubmits the ingestion run, and follows it to completion (streaming\nprogress + the final summary). Your data never leaves your own\ninfrastructure. Supports %[1]d tasks across the image, text, and\ntabular / time-series families; pick one with --task.\n\n is the data itself. What it looks like depends on the task:\n\n tabular / time-series — the dataset is a single CSV. Pass the .csv\n file directly, or a folder holding exactly one .csv:\n\n churn.csv (the .csv file itself)\n or\n churn/\n data.csv (the one .csv in the folder)\n\n image classification / keypoint detection — a folder with\n labels.csv + an images/ subfolder:\n\n cats_dogs/\n labels.csv (required)\n images/ (required)\n 001.jpg\n ...\n\n object detection — a folder with images/ + annotations/ and NO\n labels.csv: records are enumerated from the Pascal-VOC XML, one per\n image, so there is no manifest and no label column to declare.\n\n visdrone/\n images/ (required)\n 001.jpg\n annotations/ (required — 001.xml pairs with 001.jpg)\n 001.xml\n\n text (classification, masked language modeling) — a folder with\n labels.csv + a %[2]s/ subfolder (masked language modeling uses %[3]s/):\n\n reviews/\n labels.csv (required)\n %[2]s/ (required — %[3]s/ for masked language modeling)\n 001.txt\n ...\n\nA bare .csv file is accepted only for the tabular / time-series family;\nimage and text datasets must be a folder.\n\nAccepted image extensions: .jpg, .jpeg, or .png (case-insensitive).\nAll images in one dataset must share a single type — the cluster\nvalidates the type it was told to expect.\n\nv0.1 caps the dataset at 1 GiB total + 500 MiB per file. Larger\ndatasets need the v0.2 cloud-source story (S3/GCS/HTTPS sources) —\nsee tracebloc/client#147 non-goals.\n\nExit codes:\n 0 files staged + ingested successfully (or --detach: just staged + submitted)\n 2 schema validation failed (synthesized spec rejected) or\n v0.1-unsupported task passed\n 3 local-layout or kubeconfig error\n 4 cluster reachable but no tracebloc client / shared storage missing\n 5 ingestor SA token couldn't be obtained, or jobs-manager\n rejected the token (401/403)\n 6 destination table already exists (re-run with --overwrite to\n replace it, or pick a different --name)\n 7 pre-flight succeeded but staging the files failed\n (Pod creation, image pull, exec stream, or remote tar error) —\n or, with --overwrite, removing the old table failed\n 8 jobs-manager rejected the submit (4xx/5xx other than auth)\n 9 ingestion Job exited non-zero, or completed with row-level\n failures the summary panel reports" -"Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container stuck creating. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it." +"Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container that can't be created. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it." "Interrupted before the change could be confirmed." "It may already have applied — re-run `%s resources set` to check the current per-run ceiling." "It reports more memory than the machine really has, so two trainings that each look like they fit can together run it out of memory and take the environment down. Run one training at a time; to fix it for good, recreate the environment as a single-node one. `%s doctor --verbose` shows the numbers and the exact flags." diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 74d4f02..62d886d 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -422,6 +422,25 @@ func podWaitingReason(p corev1.Pod) string { return string(p.Status.Phase) } +// stuckReasonWedged reports whether a Pending pod's kubelet Waiting.Reason is a +// genuinely-wedged pull/create FAILURE -- backing off or errored -- as opposed +// to work still legitimately in progress. An allowlist, deliberately: a reason +// we are not certain is a failure (ContainerCreating / Pulling / bare Pending, +// or any future reason) is treated as still-progressing and left to checkPods' +// age-based inference, so the measured "waiting will not clear it" Fail is only +// ever asserted for a cause the kubelet has actually reported as failing. A +// large first image pull on a cold node can exceed the grace window while still +// making progress, so age alone must not escalate it (Bugbot on backend#3247). +func stuckReasonWedged(reason string) bool { + switch reason { + case "ImagePullBackOff", "ErrImagePull", "ErrImageNeverPull", "InvalidImageName", + "CreateContainerConfigError", "CreateContainerError": + return true + default: + return false + } +} + // checkRestartHistory surfaces containers that have restarted repeatedly even // though they are not crash-looping right now — the restart-*history* signal // backend#1028 asked for. checkPods reads only the current waiting reason, so a @@ -680,17 +699,17 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s jobCPU := map[string]int64{} // node -> millicores held by running Jobs jobMem := map[string]int64{} // node -> bytes held by running Jobs // Training-Job pods the scheduler has already PLACED on a node (they carry a - // NodeName) but that are NOT yet Running -- an image pull backing off, a - // container stuck creating. Kept out of BOTH sums above: a pod that is not - // running holds no room in the "a running job will finish and free it" sense, - // and counting one as a running job is exactly what made doctor emit - // HeldByRunningJob and tell the operator to "wait for the job to finish" at - // exit 0 on a pod that is wedged and never will (backend#3247). Collected here - // (past the same grace window checkPods uses) so the verdict can name the real - // stuck state as an actionable finding instead. Recognised cluster-wide by the - // same `job-name` convention the running-job sum above uses -- a dedicated - // secure environment's batch Jobs are tracebloc's -- so "stuck" and - // "holds the room" agree on what a training/ingestion pod is. + // NodeName) but that are Pending on a genuinely-wedged reason -- an image pull + // backing off, a create error (see stuckReasonWedged). Kept out of BOTH sums + // above: a pod that is not running holds no room in the "a running job will + // finish and free it" sense, and counting one as a running job is exactly what + // made doctor emit HeldByRunningJob and tell the operator to "wait for the job + // to finish" at exit 0 on a pod that is wedged and never will (backend#3247). + // Collected here (past the same grace window checkPods uses) so the verdict can + // name the real stuck state as an actionable finding instead. Recognised + // cluster-wide by the same `job-name` convention the running-job sum above uses + // -- a dedicated secure environment's batch Jobs are tracebloc's -- so "stuck" + // and "holds the room" agree on what a training/ingestion pod is. var stuckJobs []stuckJobPod freeKnown := true if pods, perr := cs.CoreV1().Pods("").List(ctx, metav1.ListOptions{}); perr == nil { @@ -716,20 +735,30 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s if _, isJob := p.Labels["job-name"]; isJob { // A Job pod holds the room ONLY when it is genuinely Running. An // assigned-but-Pending one (it has a NodeName but its containers have - // not started -- image pull backing off, container stuck creating) is - // NOT running: counting it as a running job made doctor emit - // HeldByRunningJob and tell the operator to wait for it to finish -- - // on a pod that is wedged (backend#3247). It feeds NEITHER sum. Past - // the grace window it is recorded as stuck and surfaced below; a pod - // merely WAITING for capacity has no NodeName yet and was skipped - // above, so this only ever catches pods the scheduler already placed. + // not started) is NOT running: counting it as a running job made + // doctor emit HeldByRunningJob and tell the operator to wait for it to + // finish -- on a pod that may be wedged (backend#3247). So it feeds + // NEITHER sum. A pod merely WAITING for capacity has no NodeName yet + // and was skipped above, so this only ever sees pods the scheduler + // already placed. + // + // Whether it is WEDGED is a separate question from whether it holds + // the room. Only a genuinely-wedged reason -- an image pull backing + // off, or a create error -- is recorded here (past the same grace + // window checkPods uses) and escalated to the measured Fail below, + // where "waiting will not clear it" is exact. A pod still pulling or + // creating (ContainerCreating / Pulling / bare Pending) is left to + // checkPods' age-based inference instead: a large first pull on a cold + // node can legitimately exceed the grace, so asserting it is wedged + // would misfire (Bugbot on this PR). if p.Status.Phase != corev1.PodRunning { - if time.Since(p.CreationTimestamp.Time) > pendingGrace { + if reason := podWaitingReason(p); stuckReasonWedged(reason) && + time.Since(p.CreationTimestamp.Time) > pendingGrace { stuckJobs = append(stuckJobs, stuckJobPod{ name: p.Name, namespace: p.Namespace, node: p.Spec.NodeName, - reason: podWaitingReason(p), + reason: reason, }) } continue @@ -905,19 +934,20 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Remedy: "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager.", } case len(stuckJobs) > 0: - // A training Job pod the scheduler already placed on a node is stuck - // Pending -- an image pull backing off, or a container that will not finish - // creating -- NOT running (backend#3247). It used to be counted as a - // running job holding the room, so this state rolled up to the transient - // "a training is already running, wait for it to finish" Warn at exit 0 -- - // on a pod that is wedged and never will. It is a real, training-blocking - // problem whose remedy is the OPPOSITE of the transient Warn's (inspect the - // pod, do NOT wait), so it is a Fail with its own prefix the rollup - // (summarizeDoctor) classifies on. Ordering matters: it sits BELOW the two - // measured capacity Fails above -- when no node can fit the envelope that - // is the root cause -- and ABOVE the transient Warn, which a wedged pod - // must never be mistaken for. A run legitimately WAITING for a running job - // has no NodeName, so it is never collected here. + // A training Job pod the scheduler already placed on a node is Pending on a + // genuinely-wedged reason -- an image pull backing off, a create error + // (stuckReasonWedged) -- NOT running (backend#3247). It used to be counted + // as a running job holding the room, so this state rolled up to the + // transient "a training is already running, wait for it to finish" Warn at + // exit 0 -- on a pod that is wedged and never will. It is a real, + // training-blocking problem whose remedy is the OPPOSITE of the transient + // Warn's (inspect the pod, do NOT wait), so it is a Fail with its own prefix + // the rollup (summarizeDoctor) classifies on. Ordering matters: it sits + // BELOW the two measured capacity Fails above -- when no node can fit the + // envelope that is the root cause -- and ABOVE the transient Warn, which a + // wedged pod must never be mistaken for. A run legitimately WAITING for a + // running job has no NodeName; one still pulling/creating is not wedged and + // was left to checkPods' inference -- neither is collected here. sort.Slice(stuckJobs, func(a, b int) bool { return stuckJobs[a].name < stuckJobs[b].name }) descs := make([]string, len(stuckJobs)) for i, s := range stuckJobs { @@ -928,7 +958,7 @@ func checkNodeFit(ctx context.Context, cs kubernetes.Interface, env map[string]s Name: name, Status: StatusFail, Detail: fmt.Sprintf("%s: %s. The next run does not wait on a pod that is not running", StuckJobPod, strings.Join(descs, ", ")), - Remedy: fmt.Sprintf("Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container stuck creating. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it.", first.namespace, first.name), + Remedy: fmt.Sprintf("Inspect the stuck pod: kubectl describe pod -n %s %s — usually a training image that can't be pulled, or a container that can't be created. This is not a capacity shortage; lowering RESOURCE_REQUESTS or resizing will not clear it.", first.namespace, first.name), } case freeKnown && !nowFits: // The TRANSIENT shortage (backend#2870): every dimension fits beside the @@ -1094,9 +1124,9 @@ const OverCommitted = "a Ready node is large enough" const HeldByRunningJob = "a running job holds the room" // StuckJobPod is the prefix of the backend#3247 Fail: a training Job pod is -// scheduled to a node (it has a NodeName) but is NOT Running -- Pending on an -// image pull backing off or a container stuck creating. It used to be counted -// as a running job holding the room, so checkNodeFit emitted HeldByRunningJob +// scheduled to a node (it has a NodeName) but is Pending on a genuinely-wedged +// reason -- an image pull backing off, or a create error (stuckReasonWedged). +// It used to be counted as a running job holding the room, so checkNodeFit emitted HeldByRunningJob // and the rollup told the operator to "wait for the job to finish" at exit 0 -- // on a pod that is wedged and never will. DISTINCT prefix, same discipline as // HeldByRunningJob and OverCommitted: the rollup (summarizeDoctor) classifies on diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 9d1b101..40e4f05 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -1160,14 +1160,15 @@ func TestCheckNodeFitStuckJobPod(t *testing.T) { } }) - // The defect, one waiting reason per row: assigned, Pending, past grace. Each - // must be a stuck Fail that NAMES its reason -- never HeldByRunningJob. - for _, tc := range []struct{ name, reason, wantInDetail string }{ - {"ImagePullBackOff", "ImagePullBackOff", "ImagePullBackOff"}, - {"ContainerCreating", "ContainerCreating", "ContainerCreating"}, - {"bare Pending, no reason yet", "", "Pending"}, + // The defect, one WEDGED waiting reason per row: assigned, Pending, past grace, + // on a reason the kubelet has reported as failing. Each must be a stuck Fail + // that NAMES its reason -- never HeldByRunningJob. + for _, tc := range []struct{ name, reason string }{ + {"ImagePullBackOff", "ImagePullBackOff"}, + {"ErrImagePull", "ErrImagePull"}, + {"CreateContainerConfigError", "CreateContainerConfigError"}, } { - t.Run("assigned+Pending past grace ("+tc.name+") -> stuck Fail", func(t *testing.T) { + t.Run("assigned+Pending past grace, wedged ("+tc.name+") -> stuck Fail", func(t *testing.T) { pod := assignedPendingJobPod("train-stuck", "n1", tc.reason, "12Gi", old) r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) if r.Status != StatusFail { @@ -1179,8 +1180,8 @@ func TestCheckNodeFitStuckJobPod(t *testing.T) { if strings.Contains(r.Detail, HeldByRunningJob) { t.Fatalf("a wedged pod must never be reported as a running job: %q", r.Detail) } - if !strings.Contains(r.Detail, tc.wantInDetail) { - t.Errorf("detail should name the real state %q, got %q", tc.wantInDetail, r.Detail) + if !strings.Contains(r.Detail, tc.reason) { + t.Errorf("detail should name the real state %q, got %q", tc.reason, r.Detail) } if !strings.Contains(r.Remedy, "kubectl describe pod") { t.Errorf("the remedy must be actionable (inspect the pod), got %q", r.Remedy) @@ -1188,11 +1189,34 @@ func TestCheckNodeFitStuckJobPod(t *testing.T) { }) } + // A still-PROGRESSING pod past grace (ContainerCreating / bare Pending, or a + // large first pull that has not backed off yet) is NOT wedged: asserting + // "waiting will not clear it" would misfire on a multi-GB image on a cold + // node. checkNodeFit leaves it to checkPods' age-based inference, so on its + // own it is neither the stuck Fail nor HeldByRunningJob -- the node reads OK + // (Bugbot on this PR, backend#3247). + for _, tc := range []struct{ name, reason string }{ + {"ContainerCreating", "ContainerCreating"}, + {"Pulling", "Pulling"}, + {"bare Pending, no reason yet", ""}, + } { + t.Run("assigned+Pending past grace, still progressing ("+tc.name+") -> not escalated", func(t *testing.T) { + pod := assignedPendingJobPod("train-pulling", "n1", tc.reason, "12Gi", old) + r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) + if strings.HasPrefix(r.Detail, StuckJobPod) { + t.Fatalf("a still-progressing pod must not take the measured stuck Fail: %q", r.Detail) + } + if r.Status != StatusOK { + t.Fatalf("=> %v (%q), want OK: checkNodeFit defers a progressing pod to checkPods", r.Status, r.Detail) + } + }) + } + // A pod the scheduler JUST placed and is normally starting (inside grace) is // neither a running job nor stuck -- flagging it would false-positive on // every training launch. Not counted as held either, so the node reads OK. t.Run("assigned+Pending INSIDE grace -> not flagged, node still OK", func(t *testing.T) { - pod := assignedPendingJobPod("train-young", "n1", "ContainerCreating", "12Gi", fresh) + pod := assignedPendingJobPod("train-young", "n1", "ImagePullBackOff", "12Gi", fresh) r := checkNodeFit(bg(), fake.NewClientset(fitNode(), pod), req) if r.Status != StatusOK { t.Fatalf("=> %v (%q), want OK: a freshly-scheduled pod is normal startup", r.Status, r.Detail) @@ -1205,8 +1229,8 @@ func TestCheckNodeFitStuckJobPod(t *testing.T) { // Scope guard: the fix keys on the job-name label. A non-Job pod (platform) // that is Pending-assigned is steady state and must not take the stuck-pod arm. t.Run("a non-Job Pending pod is not a stuck training pod", func(t *testing.T) { - plat := assignedPendingJobPod("some-deploy", "n1", "ContainerCreating", "12Gi", old) - plat.Labels = nil // not a job-name pod + plat := assignedPendingJobPod("some-deploy", "n1", "ImagePullBackOff", "12Gi", old) + plat.Labels = nil // not a job-name pod, though its reason IS wedged r := checkNodeFit(bg(), fake.NewClientset(fitNode(), plat), req) if strings.HasPrefix(r.Detail, StuckJobPod) { t.Fatalf("the stuck-training-pod arm must be scoped to job-name pods, got %q", r.Detail)