diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 77f99a8..87c837a 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -492,6 +492,25 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — this machine is big enough, but the platform's own services have already claimed the room.", fmt.Sprintf("Ask for less per training run, or give the machine more memory/CPU. Do NOT size runs to the machine here — that measures the machine's total, not what is free, so it would ask for MORE and leave the training stuck. `%s doctor --verbose` shows the exact numbers and the knob to turn.", launcher())} + // MEASURED training-blockers that can co-occur with a Pending pod held by a + // running job, so they are read BEFORE the Wait-Warn below (backend#3248, + // Bugbot Medium on #641). checkImagePull and checkPVC are + // INDEPENDENT of Pod health and Node capacity, so either can be Fail while + // `stuckPending && heldByJob` is also true — a pod Pending on an image it + // cannot pull, or beside a dataset volume that never bound, is a measured + // failure, not a wait. Below the Wait-Warn these Fails were shadowed: a node + // held by a job with any pod Pending past grace made `doctor` exit 0 (Warn) + // over an exit-2 failure. They sit above the stuck-Pending arm too — the same + // measured-beats-inferred rule that lets the Wait-Warn refute the Pending + // inference puts a measured cause ahead of it. + case by["Image pull secret"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — the training images can't be pulled.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} + case by["Dataset volume (PVC)"].Status == doctor.StatusFail: + ready = healthLine{doctor.StatusFail, + "Not ready — dataset storage isn't available.", + fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} case stuckPending && heldByJob: // A PENDING POD WHOSE CAUSE HAS BEEN MEASURED (Bugbot High on #639, // backend#2870). This is the transient shortage's own symptom: a job is @@ -509,11 +528,17 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // cause -- the envelope fits this machine and a running job holds it -- // which refutes the inference, and a Fail here would exit 2 on healthy // training (the Bugbot High on #628) while recommending a resize that - // changes nothing. The two arms that still outrank this one are measured: - // a Pod-health FAIL (crash-loop) and the OverCommitted Fail. + // changes nothing. The arms that still outrank this one are all MEASURED + // failures — a Pod-health FAIL (crash-loop), the OverCommitted Fail, and + // the image-pull-secret and dataset-volume Fails just above — because a + // measured training-blocker must not be hidden behind a wait (backend#3248: + // those two Fails used to sit BELOW this arm, so a Pending pod beside a + // running job made `doctor` exit 0 over a real, exit-2 failure). // // checkPods does not know WHY a pod is Pending, so a pod stuck on an - // image pull beside a running job would land here too; the remedy names + // image pull beside a running job could still reach this arm when the + // image-pull-secret probe itself is healthy (the secret exists, but the + // pull is slow or the registry is briefly unreachable); the remedy names // what to do if the wait outlives the job rather than pretending the // attribution is certain. ready = healthLine{doctor.StatusWarn, @@ -536,9 +561,11 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // grace window is left to the stuck-Pending arm below, whose "usually not // enough free compute, or an image that can't be pulled" wording is an // honest age-based inference (a large first pull on a cold node CAN exceed - // the grace). Only the measured capacity Fails (OverCommitted) and a hard - // Pod-health crash-loop Fail outrank it. Its remedy is the OPPOSITE of the - // transient Warn's (inspect the pod, do NOT wait); the pod it names is one + // the grace). The arms that outrank it are all measured: a Pod-health + // crash-loop Fail, the OverCommitted Fail, and the image-pull-secret and + // dataset-volume Fails above (backend#3248 — those two sit above the + // wait-for-capacity Warn, hence above this arm too). Its remedy is the + // OPPOSITE of the transient Warn's (inspect the pod, do NOT wait); the pod it names is one // `--verbose` away -- and PLAIN TERMS, no Kubernetes vocabulary, like its // neighbours (the granular checkNodeFit remedy carries the `kubectl` form). ready = healthLine{doctor.StatusFail, @@ -555,14 +582,6 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready ready = healthLine{doctor.StatusFail, "Not ready — part of your secure environment can't start yet.", fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. %s Then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", computeRemedy(runtime.GOOS), launcher(), launcher())} - case by["Image pull secret"].Status == doctor.StatusFail: - ready = healthLine{doctor.StatusFail, - "Not ready — the training images can't be pulled.", - fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} - case by["Dataset volume (PVC)"].Status == doctor.StatusFail: - ready = healthLine{doctor.StatusFail, - "Not ready — dataset storage isn't available.", - fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())} case by["Node capacity"].Status == doctor.StatusFail: ready = healthLine{doctor.StatusFail, "Not ready — not enough free compute to start a training.", @@ -633,6 +652,28 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready // the OK default): training still runs via the jobs-manager's CPU fallback. ready = healthLine{doctor.StatusUnknown, "Ready to run training — couldn't check free compute (run with --verbose)", ""} + case by["Image pull secret"].Status == doctor.StatusWarn && + strings.HasPrefix(by["Image pull secret"].Detail, doctor.CantReadImagePullSecret): + // checkImagePull can't-check: the secret (or the jobs-manager that names it) + // could not be READ, not read-and-found-missing. It carries no signal about + // whether images can be pulled, so it lands here in the Unknown tier — never + // the measured "images can't be pulled" Fail above, which is now promoted + // over the wait-for-capacity Warn and would flip a healthy environment to + // exit 2 on an RBAC blip (backend#3248, LukasWodka on #643). + // + // PLAIN TERMS, no Kubernetes vocabulary — "image pull secret" is jargon that + // belongs one --verbose away in renderDoctorDetails, so this line mirrors the + // Fail arm's "training images can't be pulled" wording (Bugbot on #643). + ready = healthLine{doctor.StatusUnknown, + "Ready to run training — couldn't check whether training images can be pulled (run with --verbose)", ""} + case by["Dataset volume (PVC)"].Status == doctor.StatusWarn && + strings.HasPrefix(by["Dataset volume (PVC)"].Detail, cluster.PVCReadErrPrefix): + // checkPVC can't-check: the PVC could not be READ (Forbidden / network), + // not read-and-found-unbound. Same reasoning as the image-pull arm — a + // can't-read is no signal, so it stays in the Unknown tier rather than the + // measured "dataset storage isn't available" Fail above (backend#3248). + ready = healthLine{doctor.StatusUnknown, + "Ready to run training — couldn't check dataset storage (run with --verbose)", ""} default: ready = healthLine{doctor.StatusOK, "Ready to run training", ""} } diff --git a/internal/cli/doctor_test.go b/internal/cli/doctor_test.go index cadd225..695de9c 100644 --- a/internal/cli/doctor_test.go +++ b/internal/cli/doctor_test.go @@ -572,6 +572,61 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3248 (Bugbot Medium on #641). The Wait-Warn arm above is an + // INFERENCE that a Pending pod beside a running job is only waiting for the + // room. checkImagePull and checkPVC are MEASURED and independent of + // Pod health and Node capacity, so either can be Fail in that exact state — a + // missing pull secret, or a dataset volume that never bound, beside a running + // job. The Wait-Warn used to sit ABOVE those Fail arms, so the measured failure + // was shadowed and `doctor` exited 0 (Warn) instead of 2 (Fail). Pin that a + // measured Fail wins the precedence, while the Wait-Warn still applies when + // there is no measured Fail. + t.Run("a measured Fail beside a running job outranks the wait-for-capacity Warn", func(t *testing.T) { + // The full waiting_for_capacity state: a running job holds the room AND a + // pod is Pending past grace — together the arm that returns the Wait-Warn. + // Detail built from the producer's constant, same discipline as the arm. + waiting := withDetail(allOK, "Node capacity", doctor.StatusWarn, + doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish") + waiting = withDetail(waiting, "Pod health", doctor.StatusWarn, + "Pending > 5m0s: [train-second]") + // allOK omits "Image pull secret" (a fixture shortcut; Run() does emit it), + // and `with` only mutates an entry that already exists — so add it OK here, + // or the image-pull case below would silently stay unset and never flip. + waiting = append(waiting, res("Image pull secret", doctor.StatusOK)) + + // Precondition: with no measured Fail, that state is the exit-0 Wait-Warn. + // Assert the Wait-Warn's OWN top line, not merely "a Warn" (LukasWodka on + // #643): the bare heldByJob arm below is also a Warn, so a status-only check + // would still pass if the reorder were undone and the state fell through to + // it — leaving the exit-0 → exit-2 claim this test exists for unpinned. + if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn || + !strings.Contains(r.text, "the next one is waiting for it to finish") { + t.Fatalf("precondition: the wait-for-capacity state should be the Wait-Warn, got %v (%q)", r.status, r.text) + } + + // Each measured Fail, dropped into that same state, must win — top line and + // exit code both. Per-case subtests: map order is randomized, so a shared + // loop with t.Fatalf would report one nondeterministic case and hide the other. + measured := map[string]struct{ name, wantText string }{ + "image pull secret Fail": {"Image pull secret", "images can't be pulled"}, + "dataset volume Fail": {"Dataset volume (PVC)", "dataset storage isn't available"}, + } + for label, m := range measured { + t.Run(label, func(t *testing.T) { + c, r := summarizeDoctor(with(waiting, m.name, doctor.StatusFail), tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a measured Fail beside a running job must win — the Wait-Warn shadowed it and doctor exited 0 over a real failure, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, m.wantText) { + t.Errorf("want the measured Fail's own top line %q, got %q", m.wantText, r.text) + } + if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail { + t.Errorf("the verdict must own the Fail (exit 2), not the wait Warn (exit 0), got %v", v) + } + }) + } + }) + t.Run("a Pending pod with NO running job is still the stuck-Pending Fail", func(t *testing.T) { // The other side: the arm above is scoped to the co-occurrence. A pod // Pending on a machine where nothing holds the room is the generic, @@ -583,6 +638,28 @@ func TestSummarizeDoctor(t *testing.T) { } }) + t.Run("a measured Fail with a Pending pod but NO running job still outranks the stuck-Pending inference", func(t *testing.T) { + // backend#3248, the no-heldByJob half of the reorder. Moving the measured + // Fails above the Wait-Warn necessarily moves them above the plain + // stuck-Pending Fail too (the Wait-Warn sits above stuck-Pending, + // backend#2870). Both are exit-2, so the observable change is which top + // line and remedy the operator sees — the measured image-pull cause, not + // the generic "usually not enough free compute" guess. Pin it, matching + // this file's discipline of nailing every ordering a reshuffle could undo. + results := withDetail(allOK, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [trainer-x]") + results = append(results, res("Image pull secret", doctor.StatusFail)) + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusFail { + t.Fatalf("a measured image-pull Fail must stay a Fail, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "images can't be pulled") { + t.Errorf("the measured image-pull cause must win over the generic stuck-Pending guess, got %q", r.text) + } + if strings.Contains(r.remedy, "resources set max") { + t.Errorf("a measured image-pull Fail must not send the operator to resize compute: %q", r.remedy) + } + }) + // backend#3247: the defect combination. A training pod is scheduled but // wedged on an image pull (Node capacity Fail, StuckJobPod) and Pod health // also sees it Pending past grace (stuckPending). This used to roll up through @@ -639,6 +716,67 @@ func TestSummarizeDoctor(t *testing.T) { } }) + // backend#3248 (LukasWodka on #643): checkImagePull / checkPVC now return a + // can't-check Warn (with a distinct prefix) when the secret / PVC could not be + // READ, distinct from a measured missing / unbound Fail. A can't-read carries + // no signal, so it must roll up to the Unknown tier — never the promoted + // measured Fail, which would flip a healthy environment to exit 2 on an RBAC blip. + t.Run("a can't-READ image-pull or PVC is an honest can't-check, not the promoted Fail", func(t *testing.T) { + imgCantRead := append(append([]doctor.Result{}, allOK...), + doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`}) + if _, r := summarizeDoctor(imgCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "training images can be pulled") { + t.Errorf("a can't-read image-pull must roll up to a plain-terms can't-check, got %v (%q)", r.status, r.text) + } + pvcCantRead := withDetail(allOK, "Dataset volume (PVC)", doctor.StatusWarn, + cluster.PVCReadErrPrefix+"ns/client-pvc: is forbidden") + if _, r := summarizeDoctor(pvcCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "dataset storage") { + t.Errorf("a can't-read PVC must roll up to a can't-check, got %v (%q)", r.status, r.text) + } + }) + + // The exact regression from thread 1: a running job holds the room, the next + // pod is Pending (the wait-for-capacity state), AND the pull secret could not + // be read. Because that read failure is now a can't-check (not a Fail), it no + // longer promotes over the Wait-Warn — the operator is told to wait, not handed + // a false "images can't be pulled" exit 2 on a healthy environment. + t.Run("a can't-READ secret beside a running job stays the wait Warn, not a false exit-2", func(t *testing.T) { + results := withDetail(allOK, "Node capacity", doctor.StatusWarn, + doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish") + results = withDetail(results, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [train-second]") + results = append(results, doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`}) + c, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusWarn || !strings.Contains(r.text, "waiting for it") { + t.Fatalf("a can't-read secret must not flip the wait-for-capacity Warn to a Fail, got %v (%q)", r.status, r.text) + } + if v := doctorVerdict(c.status, r.status); v == doctor.StatusFail { + t.Errorf("a read blip must not make doctor exit 2 on a healthy environment, got verdict %v", v) + } + }) + + // backend#3248 thread 3 (LukasWodka on #643). When the machine over-commits AND + // a running job holds the room AND the next pod is Pending, both findings are + // Warns (exit 0). The plain-heldByJob case puts the over-commit Warn first + // ("a machine that lies about its size outranks a running job"), but the + // Pending variant cannot: the Wait-Warn sits above the stuck-Pending Fail + // (backend#2870) and the over-commit Warn must stay below that Fail (a Warn may + // not shadow a Fail), so by transitivity the Wait-Warn wins here. It is + // message-only (both exit 0) and pre-existing; lifting it would need a dedicated + // arm, not a reorder. Pin the current behavior so the gap is recorded, not implied. + t.Run("over-commit Warn is shadowed by the wait-for-capacity Warn in the Pending variant (known, message-only)", func(t *testing.T) { + results := withDetail(allOK, "Machine capacity", doctor.StatusWarn, + "Docker VM 7.75 GiB → 2 nodes claiming 15.50 GiB — Kubernetes believes 2.00× the memory this machine has") + results = withDetail(results, "Node capacity", doctor.StatusWarn, + doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish") + results = withDetail(results, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [train-second]") + _, r := summarizeDoctor(results, tokenOK) + if r.status != doctor.StatusWarn { + t.Fatalf("both findings are Warns → exit 0, got %v (%q)", r.status, r.text) + } + if !strings.Contains(r.text, "waiting for it") { + t.Errorf("known message-only gap: the Pending variant shows the wait-for-capacity Warn, not the over-commit Warn — if a dedicated arm is added, update this pin, got %q", r.text) + } + }) + t.Run("a crash-looping pod still outranks the running-job explanation", func(t *testing.T) { // The exception is scoped to the stuck-Pending WARN; a Pod-health FAIL is // a measured failure with a different fix, and must keep winning. diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 926ee30..af95723 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -49,6 +49,7 @@ screen. %s/%d are runtime placeholders. "%q won't work — no hyphens or spaces (use _); use letters, digits, and underscores, starting with a letter or underscore (e.g. churn_train)" "%s state=%s namespace=%s location=%s" "%s %q must be WxH (e.g. 512x512)" +"%s %q: %v" "%s %q: height is not an integer: %w" "%s %q: width and height must both be positive" "%s %q: width is not an integer: %w" @@ -78,6 +79,7 @@ screen. %s/%d are runtime placeholders. "%s ×%d" "%s — %s" "%s — Kubernetes believes %.2f× the memory this machine has, because the k3d node containers are uncapped and each reports the whole VM" +"%s%s/%s: %w" "%s, so free compute could not be verified — checked against allocatable only; an over-committed control plane would be invisible here. Also, no single Ready node satisfies cpu+memory AND %s, so GPU jobs would rely on the CPU fallback (needs %s)" "%s, … and %d more" "%s/%s" @@ -329,7 +331,9 @@ screen. %s/%d are runtime placeholders. "Ready to run training — but a job is already using this machine's free compute, so the next run waits for it." "Ready to run training — but your environment thinks this machine is bigger than it is." "Ready to run training — can't check yet" +"Ready to run training — couldn't check dataset storage (run with --verbose)" "Ready to run training — couldn't check free compute (run with --verbose)" +"Ready to run training — couldn't check whether training images can be pulled (run with --verbose)" "Ready to run training — couldn't check your workloads (run with --verbose)" "Reclaimed %d tracebloc image%s." "Reclaiming the temporary copy" @@ -502,7 +506,6 @@ screen. %s/%d are runtime placeholders. "couldn't reach the backend to finish signing in — %d attempts failed in a row (check your network / HTTPS_PROXY): %w" "couldn't read RESOURCE_REQUESTS from jobs-manager — skipping node-fit" "couldn't read capacity: %v" -"couldn't read jobs-manager to resolve image pull secrets — skipping" "couldn't read the account's client list to tell whether this cluster is new or already registered (%v) — retry when the backend is reachable (a re-run adopts an existing client), or pass --yes/--credential-file to provision now" "couldn't read the chart's conformance checks: %w" "couldn't read this machine's capacity: %w" @@ -682,7 +685,6 @@ screen. %s/%d are runtime placeholders. "reading %s: %w" "reading CSV header from %s: %w" "reading CSV row from %s: %w" -"reading PVC %s/%s: %w" "reading allocated port: %w" "reading dataset directory %q: %w" "reading dataset path %q: %w" diff --git a/internal/cluster/pvc.go b/internal/cluster/pvc.go index fd744d0..224f34b 100644 --- a/internal/cluster/pvc.go +++ b/internal/cluster/pvc.go @@ -41,6 +41,15 @@ const SharedPVCClaimName = "client-pvc" // mountPath: "/data/shared" const SharedPVCMountPath = "/data/shared" +// PVCReadErrPrefix marks the errors DiscoverSharedPVC returns when the PVC +// could not be READ at all (Forbidden / network / other), as opposed to a PVC +// that was read and found missing or unbound. Callers that classify a read +// failure differently from a measured verdict — e.g. doctor's checkPVC, which +// reports a can't-read as a can't-check rather than a training-blocking Fail +// (backend#3248) — match on this prefix, so it lives as one constant here +// rather than being retyped where the error is produced or consumed. +const PVCReadErrPrefix = "reading PVC " + // SharedPVC describes the chart's shared-data PVC after discovery. // Carries enough metadata for Phase 3 PR-b to construct a stage Pod // that can mount the same claim. @@ -101,8 +110,8 @@ func DiscoverSharedPVC(ctx context.Context, cs kubernetes.Interface, namespace s // Forbidden / network / other — surface as-is so the // customer can RBAC-debug. Wrapping rather than substituting // because the underlying %w already carries the useful info. - return nil, fmt.Errorf("reading PVC %s/%s: %w", - namespace, SharedPVCClaimName, err) + return nil, fmt.Errorf("%s%s/%s: %w", + PVCReadErrPrefix, namespace, SharedPVCClaimName, err) } if pvc.Status.Phase != corev1.ClaimBound { diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 62d886d..694994f 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -490,6 +490,21 @@ func checkPVC(ctx context.Context, cs kubernetes.Interface, ns string) Result { const name = "Dataset volume (PVC)" pvc, err := cluster.DiscoverSharedPVC(ctx, cs, ns) if err != nil { + if strings.HasPrefix(err.Error(), cluster.PVCReadErrPrefix) { + // DiscoverSharedPVC separates a Forbidden/network/other READ failure + // (this prefix) from a PVC it read and found missing or unbound. A + // can't-read is not a measured "storage isn't available": reporting it + // as a Fail (and promoting that Fail over the wait-for-capacity Warn -- + // backend#3248) would flip a healthy environment to exit 2 on an RBAC or + // timeout blip. Surface an honest can't-check the rollup drops to the + // Unknown tier instead. + return Result{ + Name: name, + Status: StatusWarn, + Detail: err.Error(), + Remedy: "Check the CLI can read PersistentVolumeClaims in " + ns + " (kubectl auth can-i get pvc -n " + ns + ").", + } + } return Result{ Name: name, Status: StatusFail, @@ -1135,6 +1150,14 @@ const HeldByRunningJob = "a running job holds the room" // that drifted would send a wedged pod back down the "just wait" path. const StuckJobPod = "a training pod is scheduled but not running" +// CantReadImagePullSecret is the prefix of checkImagePull's can't-check Warn: the +// image pull secret could not be READ (Forbidden / timeout / transient), as +// opposed to read and found missing or malformed. Same discipline as the +// prefixes above -- the rollup (summarizeDoctor) classifies on it to drop a +// can't-read to the Unknown tier rather than a measured Fail promoted over the +// wait-for-capacity Warn (backend#3248). +const CantReadImagePullSecret = "could not read image pull secret" + // cpuMemString renders a millicore/byte pair the way RESOURCE_REQUESTS reads // ("cpu=2, memory=8Gi"), so the free/held figures in a Node-capacity verdict // line up with the request printed beside them. @@ -1167,10 +1190,16 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel const name = "Image pull secret" dep := findDeployment(ctx, cs, ns, release, "jobs-manager") if dep == nil { + // The jobs-manager Deployment could not be read, so the pull secret can't + // be resolved — a can't-check, not a clean result. It carries the same + // CantReadImagePullSecret prefix as the unreadable-secret path below so the + // rollup drops BOTH to the Unknown tier; without the prefix this Warn fell + // through to the OK default and reported a false green ✔ (Saqlain + LukasWodka + // on #643 — fix the class, not just the secret-read instance). return Result{ Name: name, Status: StatusWarn, - Detail: "couldn't read jobs-manager to resolve image pull secrets — skipping", + Detail: CantReadImagePullSecret + ": couldn't read jobs-manager to resolve it — skipping", Remedy: "Check a tracebloc client is installed in " + ns + ".", } } @@ -1181,6 +1210,21 @@ func checkImagePull(ctx context.Context, cs kubernetes.Interface, ns string, rel for _, ref := range secrets { sec, err := cs.CoreV1().Secrets(ns).Get(ctx, ref.Name, metav1.GetOptions{}) if err != nil { + if !apierrors.IsNotFound(err) { + // Forbidden / timeout / transient API error — the secret was NOT + // READ, not proven absent. Reporting it as a measured Fail (and, in + // the rollup, promoting that Fail over the wait-for-capacity Warn -- + // backend#3248) would flip a healthy environment to exit 2 on an RBAC + // or timeout blip, with a detail that falsely says "not found". A + // can't-check is honest: StatusWarn with a distinct prefix the rollup + // drops to the Unknown tier, never a training-blocking verdict. + return Result{ + Name: name, + Status: StatusWarn, + Detail: fmt.Sprintf("%s %q: %v", CantReadImagePullSecret, ref.Name, err), + Remedy: "Check the CLI can read secrets in " + ns + " (kubectl auth can-i get secrets -n " + ns + ").", + } + } return Result{ Name: name, Status: StatusFail, diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 40e4f05..6d3b2f5 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -364,6 +364,18 @@ func TestCheckPVC(t *testing.T) { if r := checkPVC(bg(), fake.NewClientset(), ns); r.Status != StatusFail { t.Fatalf("missing PVC => %v, want fail", r.Status) } + // backend#3248 (LukasWodka on #643): a PVC that could not be READ + // (Forbidden/network) is a can't-check, NOT a measured "unavailable" Fail. + // DiscoverSharedPVC wraps it with PVCReadErrPrefix; checkPVC must surface a + // StatusWarn so the rollup drops it to the Unknown tier rather than promoting + // a false Fail over the wait-for-capacity Warn. + unreadable := fake.NewClientset() + unreadable.PrependReactor("get", "persistentvolumeclaims", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("persistentvolumeclaims \"client-pvc\" is forbidden: RBAC") + }) + if r := checkPVC(bg(), unreadable, ns); r.Status != StatusWarn || !strings.HasPrefix(r.Detail, cluster.PVCReadErrPrefix) { + t.Fatalf("unreadable PVC => %v (%q), want a can't-check Warn with the read-err prefix", r.Status, r.Detail) + } } func TestCheckProxy(t *testing.T) { @@ -1299,6 +1311,37 @@ func TestCheckImagePull(t *testing.T) { t.Fatalf("=> %v (%q), want fail", r.Status, r.Detail) } }) + // backend#3248 (LukasWodka on #643): a read failure (Forbidden/timeout) is a + // can't-check, NOT a measured "not found" Fail. Get returns the error but the + // secret's existence was never established — so this must be a StatusWarn with + // the can't-read prefix, or the rollup promotes a false Fail over the + // wait-for-capacity Warn and exits 2 on a healthy environment. + t.Run("secret unreadable (forbidden) -> can't-check Warn, not a false 'not found' Fail", func(t *testing.T) { + cs := fake.NewClientset(jmDepWithPullSecret("tb", "reg")) + cs.PrependReactor("get", "secrets", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("secrets \"reg\" is forbidden: RBAC") + }) + r := checkImagePull(bg(), cs, ns, rel) + if r.Status != StatusWarn { + t.Fatalf("=> %v (%q), want a can't-check Warn on a read failure", r.Status, r.Detail) + } + if !strings.HasPrefix(r.Detail, CantReadImagePullSecret) { + t.Errorf("detail must carry the can't-read prefix so summarizeDoctor can classify it, got %q", r.Detail) + } + if strings.Contains(r.Detail, "not found") { + t.Errorf("a read failure must not be reported as 'not found', got %q", r.Detail) + } + }) + // backend#3248 (Saqlain on #643): the OTHER can't-read path — the jobs-manager + // Deployment itself is unreadable — is also a can't-check, and must carry the + // same prefix so the rollup drops it to the Unknown tier instead of falling + // through to a false green ✔. + t.Run("jobs-manager unreadable -> can't-check Warn with the read prefix", func(t *testing.T) { + r := checkImagePull(bg(), fake.NewClientset(), ns, rel) // no jobs-manager Deployment + if r.Status != StatusWarn || !strings.HasPrefix(r.Detail, CantReadImagePullSecret) { + t.Fatalf("=> %v (%q), want a can't-check Warn carrying the read prefix", r.Status, r.Detail) + } + }) } // nodeWithDisk is `node` plus an ephemeral-storage allocatable. Separate helper