diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index ed134196..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 { @@ -519,7 +527,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 +549,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) } }