From e5c9047e05b463e8242eb0832c922e36c46cab96 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 20 Jul 2026 13:56:23 +0200 Subject: [PATCH 1/2] fix(doctor): stop the cluster-down cascade of misleading remedies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the cluster API is unreachable (Docker/k3d stopped, wrong server, network), doctor ran all 8 downstream checks anyway and each invented a domain-specific cause with the wrong fix: PVC "usable StorageClass", proxy "chart too old", requests-proxy "not found -> reinstall the chart", pods/RBAC "kubeconfig user can list pods". The one true cause (cluster stopped) was buried under ~6 false verdicts. Root mechanism: findDeployment/jobsManagerEnv collapse an API transport error and genuine absence into the same nil/empty. - Add StatusUnknown ("could not check") — Worst() ignores it, so it never affects the verdict/exit code. - isUnreachable(err): a transport/connectivity error (connection refused, timeout, DNS, TLS; net.Error/url.Error) that is NOT ErrNoParentRelease and NOT an RBAC/NotFound status (those mean the API answered). - Run() short-circuits on isUnreachable(relErr): emit the one honest "Cluster reachable" ✖ and mark the 7 cluster checks StatusUnknown. checkBackendEgress still runs (probes the backend from this machine, not the cluster API). Display order unchanged. When the API IS reachable, every check runs and reports exactly as before (isolated-failure messaging preserved). - checkReachable names the endpoint ("the cluster API server at isn't answering — is the cluster running?") and, for a loopback server, gives the local-cluster remedy ("start Docker Desktop, then `k3d cluster start`") instead of the kubeconfig/chart remedy. - doctor renders StatusUnknown as a neutral · line (no ✖/⚠, no remedy). Verified by TestRun_UnreachableCascade (fake clientset, connection-refused reactor): one ✖ naming 127.0.0.1:6550 + start-cluster remedy, 7 StatusUnknown with no remedy, backend egress still ok, Worst()=Fail (exit code unchanged). Fixes tracebloc/cli#350. Part of epic tracebloc/backend#1142 (Theme A). Co-Authored-By: Claude Opus 4.8 --- internal/cli/doctor.go | 9 ++- internal/doctor/doctor.go | 140 ++++++++++++++++++++++++++++++++- internal/doctor/doctor_test.go | 76 +++++++++++++++++- 3 files changed, 218 insertions(+), 7 deletions(-) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index 4504fa09..16c0482d 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -130,7 +130,10 @@ func runClusterDoctor( p.Field("server", resolved.ServerURL) p.Field("namespace", resolved.Namespace) - results := doctorRunFn(ctx, cs, doctor.Options{Namespace: resolved.Namespace}) + results := doctorRunFn(ctx, cs, doctor.Options{ + Namespace: resolved.Namespace, + ServerURL: resolved.ServerURL, + }) p.Section("Checks") for _, r := range results { @@ -147,6 +150,10 @@ func runClusterDoctor( if r.Remedy != "" { p.Hintf(" %s", r.Remedy) } + case doctor.StatusUnknown: + // No signal: a neutral · line, no ✖/⚠ and no remedy — the one honest + // "Cluster reachable" ✖ above already carries the cause and the fix. + p.Infof("%s — %s", r.Name, r.Detail) } } diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 8dec252a..6bdfe63c 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -13,14 +13,18 @@ package doctor import ( "context" "encoding/json" + "errors" "fmt" + "net" "net/http" + "net/url" "sort" "strings" "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -38,6 +42,13 @@ const ( StatusFail ) +// StatusUnknown marks a check that could not run because a prerequisite was +// unavailable — today, the cluster API being unreachable. It carries NO signal: +// Worst() ignores it, so it never affects the overall verdict or exit code. It +// exists so a single root cause (a stopped cluster) renders as one honest ✖ plus +// neutral "couldn't check" lines, instead of every check inventing a false cause. +const StatusUnknown Status = -1 + func (s Status) String() string { switch s { case StatusOK: @@ -46,6 +57,8 @@ func (s Status) String() string { return "warn" case StatusFail: return "fail" + case StatusUnknown: + return "unknown" default: return "unknown" } @@ -65,6 +78,9 @@ type Result struct { func Worst(results []Result) Status { worst := StatusOK for _, r := range results { + if r.Status == StatusUnknown { + continue // no signal — a "couldn't check" never sets the verdict + } if r.Status > worst { worst = r.Status } @@ -98,6 +114,12 @@ const ( type Options struct { Namespace string + // ServerURL is the cluster API endpoint from the resolved kubeconfig. Used + // only to name the endpoint (and detect a local/loopback cluster) in the + // "Cluster reachable" remedy when the API is unreachable. Empty is fine — the + // remedy just omits the address. + ServerURL string + // HTTPProbe reports whether a URL is reachable from where the CLI runs. // nil => httpProbe (proxy-aware, short timeout). Injected in tests. HTTPProbe func(ctx context.Context, url string) error @@ -113,13 +135,37 @@ func Run(ctx context.Context, cs kubernetes.Interface, opts Options) []Result { } ns := opts.Namespace - // Discovered once and shared: the parent release gates nothing (every - // check still runs and reports), but the later checks reuse it. + // Discovered once: the first API call. Its error is the reachability signal + // the rest of the sweep keys on. release, relErr := cluster.DiscoverParentRelease(ctx, cs, ns) + + // If the cluster API itself is unreachable (Docker/k3d stopped, wrong server, + // network down), every cluster-dependent check would fail its own API call + // and invent a domain-specific cause — "PVC unbound", "requests-proxy not + // found → reinstall the chart", "chart too old" — burying the one true cause + // under a wall of false ✖/⚠. Report checkReachable as the single honest ✖ and + // mark the cluster checks StatusUnknown ("couldn't check"). checkBackendEgress + // still runs: it probes the backend from THIS machine and never touches the + // cluster API, so its verdict stays truthful. Display order is preserved. + if isUnreachable(relErr) { + return []Result{ + checkReachable(release, relErr, ns, opts.ServerURL), + unknownCheck("Pod health"), + unknownCheck("Restart history"), + unknownCheck("Dataset volume (PVC)"), + unknownCheck("Node capacity"), + unknownCheck("Image pull secret"), + unknownCheck("Proxy configuration"), + checkBackendEgress(ctx, nil, opts.HTTPProbe), + unknownCheck("Service Bus egress (requests-proxy)"), + } + } + + // API reachable: every check runs and reports independently, as before. jmEnv := jobsManagerEnv(ctx, cs, ns, release) return []Result{ - checkReachable(release, relErr, ns), + checkReachable(release, relErr, ns, opts.ServerURL), checkPods(ctx, cs, ns), checkRestartHistory(ctx, cs, ns), checkPVC(ctx, cs, ns), @@ -133,9 +179,25 @@ func Run(ctx context.Context, cs kubernetes.Interface, opts Options) []Result { // checkReachable confirms the API answered and the parent client chart is // installed here. It's the gate the customer reads first; the rest still run. -func checkReachable(release *cluster.ParentRelease, err error, ns string) Result { +func checkReachable(release *cluster.ParentRelease, err error, ns, serverURL string) Result { const name = "Cluster reachable" if err != nil { + // Transport error: the API server never answered. Name the endpoint and, + // for a local (loopback) cluster, give the start-it remedy — the common + // "Docker/k3d stopped" case, not a kubeconfig or chart problem. This is the + // only branch reached when Run() takes its unreachable short-circuit. + if isUnreachable(err) { + detail := "the cluster API server isn't answering" + if serverURL != "" { + detail = fmt.Sprintf("the cluster API server at %s isn't answering — is the cluster running?", serverURL) + } + remedy := "Check the cluster is running and that your kubeconfig/context points at it." + if isLoopback(serverURL) { + remedy = "This is a local cluster — start Docker Desktop (or your container runtime), then `k3d cluster start` (or your cluster's start command)." + } + return Result{Name: name, Status: StatusFail, Detail: detail, Remedy: remedy} + } + // API answered but no chart here (ErrNoParentRelease) or another error. // The discovery error's remediation tail points at doctor — which is // what's running. Strip it so doctor never tells the user to run doctor. // (Must match the exact suffix cluster.discover appends.) @@ -155,6 +217,76 @@ func checkReachable(release *cluster.ParentRelease, err error, ns string) Result } } +// isUnreachable reports whether err is a transport/connectivity failure talking +// to the cluster API server (connection refused, timeout, DNS, TLS) — as opposed +// to the API answering with "no chart here" (ErrNoParentRelease) or an +// RBAC/NotFound status. Those latter cases mean the API IS reachable and the +// per-check verdicts are trustworthy; a transport error means they are not, so +// Run() short-circuits to a single honest "Cluster reachable" ✖. +func isUnreachable(err error) bool { + if err == nil { + return false + } + // The API answered — these are real verdicts, not connectivity failures. + if errors.Is(err, cluster.ErrNoParentRelease) || + apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err) || apierrors.IsNotFound(err) { + return false + } + var netErr net.Error + if errors.As(err, &netErr) { + return true + } + var urlErr *url.Error + if errors.As(err, &urlErr) { + return true + } + // client-go often wraps the transport error as a plain string by the time it + // reaches us; match the connectivity signatures as a fallback. + msg := err.Error() + for _, sig := range []string{ + "connection refused", "no such host", "i/o timeout", "dial tcp", + "TLS handshake", "network is unreachable", "connection reset", + "server misbehaving", "no route to host", + } { + if strings.Contains(msg, sig) { + return true + } + } + return false +} + +// isLoopback reports whether serverURL points at the local machine (127.0.0.1 / +// localhost / ::1) — a k3d/kind/Docker-Desktop cluster, whose "isn't answering" +// almost always means the container runtime or the cluster is simply stopped. +func isLoopback(serverURL string) bool { + if serverURL == "" { + return false + } + u, err := url.Parse(serverURL) + if err != nil { + return false + } + host := u.Hostname() + if host == "localhost" { + return true + } + if ip := net.ParseIP(host); ip != nil { + return ip.IsLoopback() + } + return false +} + +// unknownCheck is the placeholder for a cluster check that could not run because +// the API is unreachable — no signal, so Worst() ignores it and the verdict +// comes from "Cluster reachable" alone. +func unknownCheck(name string) Result { + return Result{ + Name: name, + Status: StatusUnknown, + Detail: "could not check — cluster API unreachable (see 'Cluster reachable' above)", + } +} + // checkPods flags crash-looping or long-Pending pods — the local complement // to the controller's crash-loop detection (client-runtime#117). Conservative // thresholds keep a transient restart or a briefly-Pending job from tripping it. diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index dc96a1f2..3cfc1814 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -11,7 +11,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" "github.com/tracebloc/cli/internal/cluster" ) @@ -166,14 +168,84 @@ func TestWorst(t *testing.T) { } func TestCheckReachable(t *testing.T) { - if r := checkReachable(nil, errors.New("boom"), ns); r.Status != StatusFail { + // A non-transport error (e.g. no chart / RBAC) keeps the kubeconfig+chart remedy. + if r := checkReachable(nil, errors.New("boom"), ns, ""); r.Status != StatusFail { t.Fatalf("error => %v, want fail", r.Status) } rel := &cluster.ParentRelease{ReleaseName: "tb", ChartVersion: "1.3.5", AppVersion: "1.3.5"} - r := checkReachable(rel, nil, ns) + r := checkReachable(rel, nil, ns, "") if r.Status != StatusOK || !strings.Contains(r.Detail, "tb") { t.Fatalf("release => %v / %q, want ok mentioning the release", r.Status, r.Detail) } + + // A transport error against a loopback server names the endpoint and gives + // the start-the-cluster remedy — not the kubeconfig/chart one. + tr := checkReachable(nil, errors.New(`Get "https://127.0.0.1:6550/api": dial tcp 127.0.0.1:6550: connect: connection refused`), ns, "https://127.0.0.1:6550") + if tr.Status != StatusFail { + t.Fatalf("transport => %v, want fail", tr.Status) + } + if !strings.Contains(tr.Detail, "127.0.0.1:6550") || !strings.Contains(tr.Detail, "isn't answering") { + t.Fatalf("transport detail = %q, want it to name the unreachable endpoint", tr.Detail) + } + if !strings.Contains(tr.Remedy, "start") || strings.Contains(tr.Remedy, "kubectl get deploy") { + t.Fatalf("transport remedy = %q, want a start-the-cluster hint, not the chart remedy", tr.Remedy) + } +} + +// TestRun_UnreachableCascade mimics the reported failure: the cluster API is +// down (connection refused on every call). Run() must emit ONE honest ✖ +// ("Cluster reachable", naming the endpoint) and mark the cluster checks +// StatusUnknown — never inventing "PVC unbound" / "requests-proxy not found → +// reinstall" / "chart too old". Backend egress still runs (it's independent of +// the cluster API), and the exit-code verdict stays Fail from the one real ✖. +func TestRun_UnreachableCascade(t *testing.T) { + cs := fake.NewClientset() + connRefused := errors.New(`Get "https://127.0.0.1:6550/apis": dial tcp 127.0.0.1:6550: connect: connection refused`) + cs.PrependReactor("*", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, connRefused + }) + + results := Run(bg(), cs, Options{ + Namespace: "lukas-test", + ServerURL: "https://127.0.0.1:6550", + HTTPProbe: func(context.Context, string) error { return nil }, // backend reachable from this machine + }) + + byName := map[string]Result{} + for _, r := range results { + byName[r.Name] = r + } + + reach := byName["Cluster reachable"] + if reach.Status != StatusFail || !strings.Contains(reach.Detail, "127.0.0.1:6550") { + t.Fatalf("Cluster reachable = %+v, want fail naming the endpoint", reach) + } + if !strings.Contains(reach.Remedy, "start") { + t.Errorf("Cluster reachable remedy = %q, want a start-the-cluster hint for a loopback server", reach.Remedy) + } + + for _, name := range []string{ + "Pod health", "Restart history", "Dataset volume (PVC)", "Node capacity", + "Image pull secret", "Proxy configuration", "Service Bus egress (requests-proxy)", + } { + r, ok := byName[name] + if !ok { + t.Fatalf("missing check %q in results", name) + } + if r.Status != StatusUnknown { + t.Errorf("%q = %v, want StatusUnknown (couldn't check) — must not invent a definitive verdict", name, r.Status) + } + if r.Remedy != "" { + t.Errorf("%q emitted remedy %q under an unreachable cluster — should stay silent", name, r.Remedy) + } + } + + if r := byName["Backend egress (from this machine)"]; r.Status != StatusOK { + t.Errorf("Backend egress = %v, want ok (probed from this machine, independent of the cluster API)", r.Status) + } + if w := Worst(results); w != StatusFail { + t.Fatalf("Worst = %v, want fail (verdict from the one real ✖, StatusUnknown ignored)", w) + } } func TestCheckPods(t *testing.T) { From c4494cbfc351dba1143e0a15391c941435e03d4b Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Mon, 20 Jul 2026 14:56:07 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(doctor):=20requests-proxy=20check=20?= =?UTF-8?q?=E2=80=94=20result-egress=20framing=20+=20honest=20=E2=9C=94=20?= =?UTF-8?q?(Part=201)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Service Bus egress (requests-proxy)" check had the direction backwards and overclaimed its ✔: - It said a down requests-proxy makes experiments "stay Pending". Wrong path: requests-proxy is the OUTBOUND relay (training pods POST epoch results/FLOPs → it forwards to Service Bus). Scheduling is a separate inbound path jobs-manager consumes directly. A down proxy stalls result/weights egress MID-RUN; it does not block scheduling. The old remedy misroutes triage. - The ✔ is inferred from Deployment ReadyReplicas, but requests-proxy has no readiness probe (backend#1143), so "Ready" only means the container started, not that Service Bus egress works. Reword the doc comment + all three lines: missing/not-ready remedies now say "result egress stalls mid-run (scheduling unaffected)"; the OK detail says "deployment-ready only; egress not directly probed". Status stays OK on a healthy install (no new ⚠, exit code unchanged). Name const kept for now. TestCheckRequestsProxy_Wording locks it: no line says "Pending", and the ✔ admits egress isn't probed. Part 2 (a real in-cluster Service Bus-egress probe) is deferred — it needs the requests-proxy /healthz + readiness probe (backend#1143 + a client-runtime health endpoint) that don't exist yet. Fixes tracebloc/cli#351 (Part 1). Part of epic tracebloc/backend#1142. Stacked on #354 (fix/doctor-reachability-gate) — merge that first. Co-Authored-By: Claude Opus 4.8 --- internal/doctor/doctor.go | 19 ++++++++++++------- internal/doctor/doctor_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 6bdfe63c..25e15677 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -493,10 +493,15 @@ func backendHost(clientEnv string) string { } } -// checkRequestsProxy verifies the requests-proxy deployment — the in-cluster -// broker for experiment egress (the Service Bus "experiments" queue) — is -// present and Ready. While it's down, experiments egress fails and the -// experiment silently stays Pending, the exact class this epic targets. +// checkRequestsProxy verifies the requests-proxy deployment is present and +// Ready. requests-proxy is the OUTBOUND relay: training pods POST epoch +// results/FLOPs to it and it forwards them to the Service Bus queues. It is NOT +// on the scheduling path — jobs-manager consumes the experiment subscription +// directly with its own credentials — so a down requests-proxy stalls result/ +// weights egress MID-RUN; it does not block scheduling (experiments do not +// "stay Pending" for this). Ready here is the Deployment's ReadyReplicas, which +// (absent a readiness probe — backend#1143) only means the container started, +// not that Service Bus egress actually works; the OK detail says so plainly. func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) Result { const name = "Service Bus egress (requests-proxy)" dep := findDeployment(ctx, cs, ns, release, "requests-proxy") @@ -505,7 +510,7 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, Name: name, Status: StatusFail, Detail: "requests-proxy deployment not found", - Remedy: "The requests-proxy brokers experiment (Service Bus) egress; without it experiments stay Pending. Reinstall/upgrade the client chart.", + Remedy: "requests-proxy relays training results/metrics to Service Bus; without it, running experiments can't send results back and training stalls mid-run (scheduling is unaffected). Reinstall/upgrade the client chart.", } } if dep.Status.ReadyReplicas < 1 { @@ -513,10 +518,10 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, Name: name, Status: StatusFail, Detail: fmt.Sprintf("requests-proxy not ready (%d/%d replicas)", dep.Status.ReadyReplicas, dep.Status.Replicas), - Remedy: "Experiments egress flows through requests-proxy; while it's down they stay Pending. kubectl describe deploy " + dep.Name + " -n " + ns, + Remedy: "While requests-proxy is down, in-flight training can't relay epoch results/FLOPs to Service Bus — result egress stalls (scheduling is unaffected). kubectl describe deploy " + dep.Name + " -n " + ns, } } - return Result{Name: name, Status: StatusOK, Detail: "requests-proxy ready (brokers the 'experiments' queue)"} + return Result{Name: name, Status: StatusOK, Detail: "requests-proxy Deployment Ready — relays result/FLOPs egress to Service Bus (deployment-ready only; egress not directly probed)"} } // checkNodeFit verifies at least one Ready node can satisfy the resource diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 3cfc1814..b1f6b34b 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -396,6 +396,33 @@ func TestCheckRequestsProxy(t *testing.T) { // When DiscoverParentRelease failed (release nil) but a release-prefixed // requests-proxy exists, the suffix fallback must still find it rather than // falsely report it missing (Bugbot on #89). +// TestCheckRequestsProxy_Wording locks the cli#351 reword: requests-proxy is +// the OUTBOUND result/FLOPs relay, so none of its lines may say experiments +// "stay Pending" (that's the scheduling path, which it doesn't touch), and the +// ✔ must be honest that egress is not actually probed. +func TestCheckRequestsProxy_Wording(t *testing.T) { + rel := &cluster.ParentRelease{ReleaseName: "tb"} + + ok := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 1)), ns, rel) + if strings.Contains(ok.Detail, "Pending") { + t.Errorf("OK detail says %q — must not mention 'Pending' (that's scheduling, not egress)", ok.Detail) + } + if !strings.Contains(ok.Detail, "not directly probed") { + t.Errorf("OK detail = %q, want it honest that egress is not directly probed", ok.Detail) + } + + notReady := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 0)), ns, rel) + missing := checkRequestsProxy(bg(), fake.NewClientset(), ns, rel) + for _, r := range []Result{notReady, missing} { + if strings.Contains(r.Remedy, "Pending") { + t.Errorf("remedy %q must not say experiments 'stay Pending' — a down proxy stalls result egress mid-run", r.Remedy) + } + if !strings.Contains(r.Remedy, "result") { + t.Errorf("remedy %q should describe the real failure (result/metrics egress stalls)", r.Remedy) + } + } +} + func TestCheckRequestsProxy_NilReleaseFindsPrefixed(t *testing.T) { cs := fake.NewClientset(requestsProxyDep("tb", 1)) // "tb-requests-proxy" if r := checkRequestsProxy(bg(), cs, ns, nil); r.Status != StatusOK {