From e60d1602a80f7cb7a2209f3f55695279058168e6 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 12:45:36 +0200 Subject: [PATCH 1/2] fix(doctor): stop requests-proxy readiness false-green (cli#351) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto develop after the doctor redesign (#365), which kept the readiness-based StatusOK. checkRequestsProxy no longer greens off ReadyReplicas alone — a present + Ready relay returns a neutral StatusUnknown ("running, egress not actively verified"), never a checkmark; down / missing still fail. worstStatus ignores StatusUnknown, so a healthy environment's overall verdict is unchanged. The real green arrives once the requests-proxy health probe (backend#1143) ships and doctor consumes it. Refs #351 Co-Authored-By: Claude Opus 4.8 --- internal/doctor/doctor.go | 13 ++++++++++-- internal/doctor/doctor_test.go | 38 +++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index ed134196..0867643b 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -519,7 +519,9 @@ func backendHost(clientEnv string) string { // 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. +// not that Service Bus egress actually works. So a Ready relay returns a neutral +// StatusUnknown ("running, egress not actively verified"), never a ✔ (cli#351); +// the real ✔ arrives once the chart ships that probe and doctor consumes it. 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") @@ -539,7 +541,14 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, 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 Deployment Ready — relays result/FLOPs egress to Service Bus (deployment-ready only; egress not directly probed)"} + // Present and Ready — but readiness ≠ egress works (see the doc comment). Do + // not green this off readiness; report a neutral, honest "unknown" so doctor + // never claims Service Bus egress is verified when it hasn't been (cli#351). + return Result{ + Name: name, + Status: StatusUnknown, + Detail: "requests-proxy is running, but egress to Service Bus is not actively verified — readiness only confirms the relay started, not that it can reach Service Bus", + } } // 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 be6ee50e..fff8b971 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -425,7 +425,10 @@ func TestCheckRequestsProxy(t *testing.T) { dep *appsv1.Deployment // nil => deployment absent want Status }{ - {"ready", requestsProxyDep("tb", 1), StatusOK}, + // Ready is NOT a green ✔: readiness ≠ Service Bus egress works, so a running + // relay is an honest StatusUnknown (egress not actively verified), never OK + // (cli#351). A down / missing relay is still a real ✖. + {"ready", requestsProxyDep("tb", 1), StatusUnknown}, {"not-ready", requestsProxyDep("tb", 0), StatusFail}, {"missing", nil, StatusFail}, } @@ -445,19 +448,23 @@ 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. +// TestCheckRequestsProxy_Wording locks the cli#351 reword + Part-2 downgrade: +// 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 a Ready relay must read as a neutral StatusUnknown (egress not +// actively verified), never a green ✔. 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) + ready := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 1)), ns, rel) + if ready.Status != StatusUnknown { + t.Errorf("Ready relay = %v, want StatusUnknown — readiness must not green a ✔ egress claim (cli#351)", ready.Status) } - if !strings.Contains(ok.Detail, "not directly probed") { - t.Errorf("OK detail = %q, want it honest that egress is not directly probed", ok.Detail) + if strings.Contains(ready.Detail, "Pending") { + t.Errorf("detail says %q — must not mention 'Pending' (that's scheduling, not egress)", ready.Detail) + } + if !strings.Contains(ready.Detail, "not actively verified") { + t.Errorf("detail = %q, want it honest that egress is not actively verified", ready.Detail) } notReady := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 0)), ns, rel) @@ -474,8 +481,10 @@ func TestCheckRequestsProxy_Wording(t *testing.T) { 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 { - t.Fatalf("nil release with prefixed deploy => %v (%q), want ok", r.Status, r.Detail) + // Found (not falsely reported missing) now reads as StatusUnknown — running, + // egress not actively verified — rather than a green ✔ (cli#351). + if r := checkRequestsProxy(bg(), cs, ns, nil); r.Status != StatusUnknown { + t.Fatalf("nil release with prefixed deploy => %v (%q), want unknown (found, egress unverified)", r.Status, r.Detail) } } @@ -513,8 +522,9 @@ func TestCheckRequestsProxy_BareNameAcceptedWhenLabelledForRelease(t *testing.T) bare.Name = "requests-proxy" bare.Labels = map[string]string{"app.kubernetes.io/instance": "relA"} cs := fake.NewClientset(bare) - if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status != StatusOK { - t.Fatalf("bare requests-proxy labelled for relA => %v (%q), want ok", r.Status, r.Detail) + // Accepted (found, not missing) → StatusUnknown, not a green ✔ (cli#351). + if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status != StatusUnknown { + t.Fatalf("bare requests-proxy labelled for relA => %v (%q), want unknown (accepted/found, egress unverified)", r.Status, r.Detail) } } From 002841a6d73952ae822f47daa91de98ee56a814a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 13:20:42 +0200 Subject: [PATCH 2/2] docs(doctor): document StatusUnknown's second meaning (ran-but-declines-to-assert) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asad's review on #369: the StatusUnknown doc only covered the 'prerequisite unavailable' case, but the requests-proxy egress check now uses it for a different reason — the check ran fine and just declines to assert an egress green it can't back. Documents both meanings on the sentinel. Co-Authored-By: Claude Opus 4.8 --- internal/doctor/doctor.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 0867643b..ddc9b455 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -42,12 +42,20 @@ 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: -// the verdict rollup ignores it, so it never affects the overall result 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. +// StatusUnknown marks a check with no trustworthy signal either way. Two cases +// produce it: +// +// 1. The check could not run because a prerequisite was unavailable — today, +// the cluster API being unreachable. A single root cause (a stopped cluster) +// then renders as one honest ✖ plus neutral "couldn't check" lines, instead +// of every downstream check inventing a false cause. +// 2. The check ran fine, but deliberately declines to assert a green it cannot +// back — e.g. requests-proxy is present and Ready, yet readiness does not +// prove Service Bus egress actually works and there is no probe for it yet +// (backend#1143). A neutral line is more honest than a false ✔. +// +// Either way it carries NO signal: the verdict rollup ignores it, so it never +// affects the overall result or exit code. const StatusUnknown Status = -1 func (s Status) String() string {