From 75b26d8af9b302d37eae14cd66c6aa96dc8ff6ce Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 15:42:37 +0500 Subject: [PATCH 1/6] =?UTF-8?q?feat(#88):=20tracebloc=20cluster=20doctor?= =?UTF-8?q?=20=E2=80=94=20live-cluster=20health=20checks=20(WS3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `tracebloc cluster doctor`, a read-only health sweep of a running tracebloc client cluster that prints ✔/⚠/✖ per check with a remedy — so a customer can diagnose "why isn't my experiment running?" without tracebloc shelling into their cluster (epic client-runtime#116, WS3). Sibling of `cluster info` (which the code's own comment anticipated); reuses its kubeconfig/context/namespace flags + cluster.Load / NewClientset / DiscoverParentRelease, the ui.Printer status vocabulary, and exitError. Lean MVP — 6 checks: - cluster reachable (parent client release discovered) - pod health (crash-loops / long-Pending — local complement to #117) - dataset volume (shared PVC Bound, via cluster.DiscoverSharedPVC) - proxy configuration (in-cluster requests/egress proxy wiring) - backend egress (host-side, proxy-aware probe; in-cluster probe = follow-up) - Service Bus egress (requests-proxy readiness — the experiments-queue broker) internal/doctor is a standalone package with injectable network probes, 82% covered via client-go's fake clientset. Every check is independent and best-effort (one failure never hides the others); the worst status sets the exit code (0 ok/warn, 2 failures, 3 kubeconfig). Out of scope (already shipped / follow-up): support-bundle ships as the installer's `--diagnose`; node-resources-vs-job-request and image-pullability are the broader cut. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/cluster.go | 13 +- internal/cli/doctor.go | 133 ++++++++++++ internal/doctor/doctor.go | 370 +++++++++++++++++++++++++++++++++ internal/doctor/doctor_test.go | 245 ++++++++++++++++++++++ 4 files changed, 756 insertions(+), 5 deletions(-) create mode 100644 internal/cli/doctor.go create mode 100644 internal/doctor/doctor.go create mode 100644 internal/doctor/doctor_test.go diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index cea1b271..db2cd7ad 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -13,11 +13,13 @@ import ( "github.com/tracebloc/cli/internal/ui" ) -// newClusterCmd wires the `tracebloc cluster` subtree. Today it has -// a single verb — `info` — which is the customer's "is the CLI -// pointing at the right cluster?" pre-flight before running -// `dataset push`. Future verbs (e.g. `cluster doctor` for -// diagnostics, `cluster contexts` for switching) hang off this +// newClusterCmd wires the `tracebloc cluster` subtree: +// - `info` — the customer's "is the CLI pointing at the right +// cluster?" pre-flight before running `dataset push`. +// - `doctor` — a read-only health sweep of the running release with +// ✔/⚠/✖ checks + remedies (epic client-runtime#116, WS3). +// +// Future verbs (e.g. `cluster contexts` for switching) hang off this // parent in later phases. func newClusterCmd() *cobra.Command { cmd := &cobra.Command{ @@ -33,6 +35,7 @@ the wrong cluster).`, } cmd.AddCommand(newClusterInfoCmd()) + cmd.AddCommand(newClusterDoctorCmd()) return cmd } diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go new file mode 100644 index 00000000..71a484fb --- /dev/null +++ b/internal/cli/doctor.go @@ -0,0 +1,133 @@ +package cli + +import ( + "context" + "fmt" + + "github.com/spf13/cobra" + + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/doctor" + "github.com/tracebloc/cli/internal/ui" +) + +// newClusterDoctorCmd implements `tracebloc cluster doctor` — the sibling of +// `cluster info` that cluster.go's doc comment anticipated. Where `info` +// answers "is the CLI pointing at the right cluster?", `doctor` answers "is +// this running cluster healthy enough to run an experiment, and if not, what +// do I fix?" — a read-only, post-install health sweep with remedies +// (epic client-runtime#116, WS3). +// +// The three kubeconfig flags match `cluster info` exactly so muscle memory +// carries over; all are zero-value-safe. +func newClusterDoctorCmd() *cobra.Command { + var ( + kubeconfigPath string + contextOverride string + nsOverride string + ) + + cmd := &cobra.Command{ + Use: "doctor", + Short: "Diagnose a running tracebloc client cluster (✔/⚠/✖ health checks + remedies)", + Long: `Runs a read-only health sweep over the tracebloc client release in the +configured cluster + namespace and prints a ✔/⚠/✖ line per check with a +remedy for anything that isn't green: + + • Cluster reachable — the API answers and the client chart is installed + • Pod health — nothing crash-looping or stuck Pending + • Dataset volume — the shared PVC exists and is Bound + • Proxy configuration — the in-cluster requests/egress proxy wiring + • Backend egress — the tracebloc backend is reachable (from this machine) + • Service Bus egress — the requests-proxy that brokers experiment egress is Ready + +For a full redacted support bundle to send to tracebloc, use the installer's +` + "`./install-k8s.sh --diagnose`" + ` instead. + +Exit codes: + 0 all checks passed (or warnings only) + 2 one or more checks failed + 3 kubeconfig could not be loaded`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + return runClusterDoctor( + cmd.Context(), + printerFor(cmd), + kubeconfigPath, contextOverride, nsOverride, + ) + }, + } + + cmd.Flags().StringVar(&kubeconfigPath, "kubeconfig", "", + "path to the kubeconfig file (default: $KUBECONFIG, then ~/.kube/config)") + cmd.Flags().StringVar(&contextOverride, "context", "", + "name of the kubeconfig context to use (default: kubeconfig's current-context)") + cmd.Flags().StringVarP(&nsOverride, "namespace", "n", "", + "namespace where the parent tracebloc/client release is installed (default: the context's namespace, or 'default')") + + return cmd +} + +func runClusterDoctor( + ctx context.Context, + p *ui.Printer, + kubeconfigPath, contextOverride, nsOverride string, +) error { + p.Banner("tracebloc", "cluster doctor") + + resolved, err := cluster.Load(cluster.KubeconfigOptions{ + Path: kubeconfigPath, + Context: contextOverride, + Namespace: nsOverride, + }) + if err != nil { + // 3 = kubeconfig file/parse problem (same class as cluster info). + return &exitError{code: 3, err: fmt.Errorf("loading kubeconfig: %w", err)} + } + + cs, err := cluster.NewClientset(resolved) + if err != nil { + return &exitError{code: 3, err: err} + } + + p.Section("Kubeconfig") + p.Field("context", resolved.Context) + p.Field("server", resolved.ServerURL) + p.Field("namespace", resolved.Namespace) + + results := doctor.Run(ctx, cs, doctor.Options{Namespace: resolved.Namespace}) + + p.Section("Checks") + for _, r := range results { + switch r.Status { + case doctor.StatusOK: + p.Successf("%s — %s", r.Name, r.Detail) + case doctor.StatusWarn: + p.Warnf("%s — %s", r.Name, r.Detail) + if r.Remedy != "" { + p.Hintf(" %s", r.Remedy) + } + case doctor.StatusFail: + p.Errorf("%s — %s", r.Name, r.Detail) + if r.Remedy != "" { + p.Hintf(" %s", r.Remedy) + } + } + } + + p.Newline() + switch doctor.Worst(results) { + case doctor.StatusFail: + p.Errorf("Problems found — fix the ✖ items above.") + p.Hintf("For deeper triage, send tracebloc a support bundle: ./install-k8s.sh --diagnose") + // Silent (err == nil): the per-check lines above already explained it, + // so main() shouldn't print a redundant "Error:" line. + return &exitError{code: 2, err: nil} + case doctor.StatusWarn: + p.Warnf("Completed with warnings — review the ⚠ items above.") + return nil + default: + p.Successf("All checks passed — the cluster looks healthy.") + return nil + } +} diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go new file mode 100644 index 00000000..5e7a6aec --- /dev/null +++ b/internal/doctor/doctor.go @@ -0,0 +1,370 @@ +// Package doctor implements the checks behind `tracebloc cluster doctor`: +// a read-only, best-effort health sweep of a running tracebloc client +// cluster. Each check reports ✔/⚠/✖ plus a one-line remedy, so a customer +// can answer "why isn't my experiment running?" without tracebloc shelling +// into their cluster (epic tracebloc/client-runtime#116, WS3). +// +// Design mirrors the installer's preflight.sh: every check is independent +// and returns a Result instead of aborting, so one failure never hides the +// others. Network probes are injectable (Options) so the package is fully +// exercisable with client-go's fake clientset — no real cluster or egress. +package doctor + +import ( + "context" + "fmt" + "net/http" + "sort" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + + "github.com/tracebloc/cli/internal/cluster" +) + +// Status is a single check's severity. Ordered so the numerically-greatest +// status is the worst — see Worst. +type Status int + +const ( + StatusOK Status = iota + StatusWarn + StatusFail +) + +func (s Status) String() string { + switch s { + case StatusOK: + return "ok" + case StatusWarn: + return "warn" + case StatusFail: + return "fail" + default: + return "unknown" + } +} + +// Result is one check's outcome. Remedy is shown to the customer only when +// Status is not OK. +type Result struct { + Name string + Status Status + Detail string + Remedy string +} + +// Worst returns the most severe status across results (StatusOK for none). +// The doctor command maps a non-OK worst to a non-zero exit code. +func Worst(results []Result) Status { + worst := StatusOK + for _, r := range results { + if r.Status > worst { + worst = r.Status + } + } + return worst +} + +// Tunables. Exported-as-vars (not consts) so a future flag could override +// them; kept conservative to avoid false positives on a busy cluster. +const ( + crashRestartThreshold = 3 // restarts before we call it crash-looping + pendingGrace = 5 * time.Minute // a pod Pending longer than this is flagged + httpProbeTimeout = 8 * time.Second +) + +// Options configures a diagnosis run. The zero value is usable: Namespace +// defaults are the caller's concern (it passes the resolved namespace), and +// HTTPProbe falls back to the real proxy-aware prober. +type Options struct { + Namespace 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 +} + +// Run executes every check in display order and returns their results. It +// never returns an error: an unreachable cluster or a failing probe is a +// Result, not a Go error — the command renders all of them and derives the +// exit code from Worst. +func Run(ctx context.Context, cs kubernetes.Interface, opts Options) []Result { + if opts.HTTPProbe == nil { + opts.HTTPProbe = httpProbe + } + ns := opts.Namespace + + // Discovered once and shared: the parent release gates nothing (every + // check still runs and reports), but the later checks reuse it. + release, relErr := cluster.DiscoverParentRelease(ctx, cs, ns) + jmEnv := jobsManagerEnv(ctx, cs, ns, release) + + return []Result{ + checkReachable(release, relErr, ns), + checkPods(ctx, cs, ns), + checkPVC(ctx, cs, ns), + checkProxy(jmEnv), + checkBackendEgress(ctx, jmEnv, opts.HTTPProbe), + checkRequestsProxy(ctx, cs, ns, release), + } +} + +// 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 { + const name = "Cluster reachable" + if err != nil { + return Result{ + Name: name, + Status: StatusFail, + Detail: err.Error(), + Remedy: "Check your kubeconfig/context and that the tracebloc client chart is installed here: kubectl get deploy -n " + ns, + } + } + return Result{ + Name: name, + Status: StatusOK, + Detail: fmt.Sprintf("release %q, chart %s, appVersion %s (namespace %s)", + release.ReleaseName, release.ChartVersion, release.AppVersion, ns), + } +} + +// 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. +func checkPods(ctx context.Context, cs kubernetes.Interface, ns string) Result { + const name = "Pod health" + pods, err := cs.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{}) + if err != nil { + return Result{ + Name: name, + Status: StatusWarn, + Detail: "could not list pods: " + err.Error(), + Remedy: "Ensure your kubeconfig user can list pods in " + ns + ".", + } + } + + var crashing, pending []string + for _, p := range pods.Items { + if podCrashLooping(p) { + crashing = append(crashing, p.Name) + continue + } + if p.Status.Phase == corev1.PodPending && + time.Since(p.CreationTimestamp.Time) > pendingGrace { + pending = append(pending, p.Name) + } + } + sort.Strings(crashing) + sort.Strings(pending) + + switch { + case len(crashing) > 0: + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("crash-looping: %v", crashing), + Remedy: "Inspect the container's own logs: kubectl logs -n " + ns + " --previous", + } + case len(pending) > 0: + return Result{ + Name: name, + Status: StatusWarn, + Detail: fmt.Sprintf("Pending > %s: %v", pendingGrace, pending), + Remedy: "kubectl describe pod -n " + ns + " — usually unschedulable (resources) or an image pull issue.", + } + default: + return Result{Name: name, Status: StatusOK, Detail: fmt.Sprintf("%d pod(s), none crash-looping or stuck Pending", len(pods.Items))} + } +} + +// podCrashLooping reports whether any container is in CrashLoopBackOff or has +// restarted past the threshold. +func podCrashLooping(p corev1.Pod) bool { + for _, c := range p.Status.ContainerStatuses { + if c.State.Waiting != nil && c.State.Waiting.Reason == "CrashLoopBackOff" { + return true + } + if c.RestartCount >= crashRestartThreshold { + return true + } + } + return false +} + +// checkPVC reuses cluster.DiscoverSharedPVC — which already verifies the +// shared-data PVC exists and is Bound, with actionable errors. +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 { + return Result{ + Name: name, + Status: StatusFail, + Detail: err.Error(), + Remedy: "Check the cluster has a usable StorageClass (kubectl get sc) and the PVC is Bound (kubectl get pvc -n " + ns + ").", + } + } + return Result{ + Name: name, + Status: StatusOK, + Detail: fmt.Sprintf("%s Bound, mounted at %s", pvc.ClaimName, pvc.MountPath), + } +} + +// checkProxy surfaces the in-cluster proxy wiring read from jobs-manager's +// env — the corporate-proxy propagation that, when missing, silently strands +// egress. Informational: present config is ✔; a missing requests-proxy URL is +// the only genuine anomaly. +func checkProxy(env map[string]string) Result { + const name = "Proxy configuration" + rp := env["REQUESTS_PROXY_URL"] + if rp == "" { + return Result{ + Name: name, + Status: StatusWarn, + Detail: "jobs-manager has no REQUESTS_PROXY_URL (could not read jobs-manager env, or chart too old)", + Remedy: "Verify the requests-proxy is wired: kubectl set env deploy/-jobs-manager --list | grep PROXY", + } + } + detail := "requests-proxy=" + rp + if eg := env["EGRESS_PROXY_URL"]; eg != "" { + detail += ", egress-proxy=" + eg + } + if env["HTTPS_PROXY"] != "" || env["HTTP_PROXY"] != "" { + detail += ", corporate HTTP(S)_PROXY set" + } else { + detail += ", no corporate HTTP(S)_PROXY" + } + return Result{Name: name, Status: StatusOK, Detail: detail} +} + +// checkBackendEgress probes the tracebloc backend API. Honest scope: this +// runs from the machine the CLI is on, NOT from inside the cluster — the +// cluster egresses via its egress-proxy. An in-cluster probe is the WS3 +// follow-up; this still catches a customer network/proxy that can't reach +// the backend at all. +func checkBackendEgress(ctx context.Context, env map[string]string, probe func(context.Context, string) error) Result { + const name = "Backend egress (from this machine)" + host := backendHost(env["CLIENT_ENV"]) + url := "https://" + host + "/" + if err := probe(ctx, url); err != nil { + return Result{ + Name: name, + Status: StatusFail, + Detail: fmt.Sprintf("%s unreachable: %v", host, err), + Remedy: "Check this machine's network/proxy to " + host + ". The cluster egresses via its egress-proxy, so this is indicative, not definitive.", + } + } + return Result{Name: name, Status: StatusOK, Detail: host + " reachable"} +} + +// backendHost maps CLIENT_ENV to the backend API host, mirroring the edge +// runtime's own mapping (controller.py). Unset/unknown defaults to prod, the +// chart's CLIENT_ENV default. +func backendHost(clientEnv string) string { + switch clientEnv { + case "dev": + return "dev-api.tracebloc.io" + case "stg": + return "stg-api.tracebloc.io" + default: + return "api.tracebloc.io" + } +} + +// 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. +func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) Result { + const name = "Service Bus egress (requests-proxy)" + dep := getDeployment(ctx, cs, ns, requestsProxyNames(release)) + if dep == nil { + return Result{ + 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.", + } + } + if dep.Status.ReadyReplicas < 1 { + return Result{ + 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, + } + } + return Result{Name: name, Status: StatusOK, Detail: "requests-proxy ready (brokers the 'experiments' queue)"} +} + +// jobsManagerEnv reads jobs-manager's first-container plain env into a map +// (valueFrom entries have no literal value and are skipped). Best-effort: +// returns an empty map when the deployment can't be fetched. +func jobsManagerEnv(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) map[string]string { + env := map[string]string{} + dep := getDeployment(ctx, cs, ns, jobsManagerNames(release)) + if dep == nil || len(dep.Spec.Template.Spec.Containers) == 0 { + return env + } + for _, e := range dep.Spec.Template.Spec.Containers[0].Env { + if e.Value != "" { + env[e.Name] = e.Value + } + } + return env +} + +// getDeployment returns the first of candidates that exists, or nil. +func getDeployment(ctx context.Context, cs kubernetes.Interface, ns string, candidates []string) *appsv1.Deployment { + for _, n := range candidates { + if n == "" { + continue + } + d, err := cs.AppsV1().Deployments(ns).Get(ctx, n, metav1.GetOptions{}) + if err == nil { + return d + } + } + return nil +} + +// jobsManagerNames / requestsProxyNames mirror the chart's naming: the +// release-prefixed form first, then the bare form for older/unprefixed charts. +func jobsManagerNames(release *cluster.ParentRelease) []string { + names := []string{"jobs-manager"} + if release != nil && release.ReleaseName != "" { + names = append([]string{release.ReleaseName + "-jobs-manager"}, names...) + } + return names +} + +func requestsProxyNames(release *cluster.ParentRelease) []string { + names := []string{"requests-proxy"} + if release != nil && release.ReleaseName != "" { + names = append([]string{release.ReleaseName + "-requests-proxy"}, names...) + } + return names +} + +// httpProbe is the default backend prober: a GET with a short timeout over +// Go's default transport, which honors HTTP(S)_PROXY/NO_PROXY via +// http.ProxyFromEnvironment. Any HTTP response (even 4xx/5xx) means the host +// is reachable — we're testing connectivity, not the endpoint's health. +func httpProbe(ctx context.Context, url string) error { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return err + } + client := &http.Client{Timeout: httpProbeTimeout} + resp, err := client.Do(req) + if err != nil { + return err + } + return resp.Body.Close() +} diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go new file mode 100644 index 00000000..15123bf2 --- /dev/null +++ b/internal/doctor/doctor_test.go @@ -0,0 +1,245 @@ +package doctor + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + + "github.com/tracebloc/cli/internal/cluster" +) + +const ns = "tracebloc" + +func bg() context.Context { return context.Background() } + +// jobsManagerDep mirrors the chart labels DiscoverParentRelease keys off +// (see internal/cluster/discover_test.go) so the fake clientset discovers it. +func jobsManagerDep(release string, env ...corev1.EnvVar) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: release + "-jobs-manager", + Namespace: ns, + Labels: map[string]string{ + "app.kubernetes.io/name": "client", + "app.kubernetes.io/instance": release, + "app.kubernetes.io/managed-by": "Helm", + "app.kubernetes.io/version": "1.3.5", + "helm.sh/chart": "client-1.3.5", + }, + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "api", Env: env}}, + }, + }, + }, + } +} + +func requestsProxyDep(release string, ready int32) *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: release + "-requests-proxy", Namespace: ns}, + Status: appsv1.DeploymentStatus{Replicas: 1, ReadyReplicas: ready}, + } +} + +func boundPVC() *corev1.PersistentVolumeClaim { + return &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: cluster.SharedPVCClaimName, Namespace: ns}, + Spec: corev1.PersistentVolumeClaimSpec{AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}}, + Status: corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound}, + } +} + +func runningPod(name string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{{Name: "c", RestartCount: 0}}, + }, + } +} + +func crashPod(name string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "c", + State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"}}, + }}, + }, + } +} + +func pendingPod(name string, age time.Duration) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + CreationTimestamp: metav1.NewTime(time.Now().Add(age)), + }, + Status: corev1.PodStatus{Phase: corev1.PodPending}, + } +} + +func TestWorst(t *testing.T) { + if got := Worst(nil); got != StatusOK { + t.Fatalf("Worst(nil) = %v, want ok", got) + } + rs := []Result{{Status: StatusOK}, {Status: StatusFail}, {Status: StatusWarn}} + if got := Worst(rs); got != StatusFail { + t.Fatalf("Worst = %v, want fail", got) + } +} + +func TestCheckReachable(t *testing.T) { + 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) + if r.Status != StatusOK || !strings.Contains(r.Detail, "tb") { + t.Fatalf("release => %v / %q, want ok mentioning the release", r.Status, r.Detail) + } +} + +func TestCheckPods(t *testing.T) { + tests := []struct { + name string + pod *corev1.Pod + want Status + }{ + {"healthy", runningPod("ok"), StatusOK}, + {"crash-loop", crashPod("bad"), StatusFail}, + {"pending-old", pendingPod("stuck", -10*time.Minute), StatusWarn}, + {"pending-fresh", pendingPod("fresh", -time.Minute), StatusOK}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cs := fake.NewClientset(tc.pod) + if r := checkPods(bg(), cs, ns); r.Status != tc.want { + t.Fatalf("checkPods = %v (%q), want %v", r.Status, r.Detail, tc.want) + } + }) + } +} + +func TestCheckPVC(t *testing.T) { + if r := checkPVC(bg(), fake.NewClientset(boundPVC()), ns); r.Status != StatusOK { + t.Fatalf("bound PVC => %v, want ok", r.Status) + } + if r := checkPVC(bg(), fake.NewClientset(), ns); r.Status != StatusFail { + t.Fatalf("missing PVC => %v, want fail", r.Status) + } +} + +func TestCheckProxy(t *testing.T) { + tests := []struct { + name string + env map[string]string + want Status + substr string + }{ + {"requests-proxy set", map[string]string{"REQUESTS_PROXY_URL": "http://requests-proxy-service:8888"}, StatusOK, "requests-proxy="}, + {"corporate proxy", map[string]string{"REQUESTS_PROXY_URL": "http://x", "HTTPS_PROXY": "http://corp:3128"}, StatusOK, "corporate HTTP(S)_PROXY set"}, + {"empty", map[string]string{}, StatusWarn, "REQUESTS_PROXY_URL"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := checkProxy(tc.env) + if r.Status != tc.want || !strings.Contains(r.Detail, tc.substr) { + t.Fatalf("checkProxy = %v / %q, want %v containing %q", r.Status, r.Detail, tc.want, tc.substr) + } + }) + } +} + +func TestCheckBackendEgress(t *testing.T) { + okProbe := func(context.Context, string) error { return nil } + failProbe := func(context.Context, string) error { return errors.New("dns failure") } + + if r := checkBackendEgress(bg(), map[string]string{"CLIENT_ENV": "dev"}, okProbe); r.Status != StatusOK || !strings.Contains(r.Detail, "dev-api.tracebloc.io") { + t.Fatalf("reachable dev => %v / %q", r.Status, r.Detail) + } + if r := checkBackendEgress(bg(), map[string]string{}, failProbe); r.Status != StatusFail || !strings.Contains(r.Detail, "api.tracebloc.io") { + t.Fatalf("unreachable default => %v / %q", r.Status, r.Detail) + } +} + +func TestBackendHost(t *testing.T) { + tests := map[string]string{ + "dev": "dev-api.tracebloc.io", + "stg": "stg-api.tracebloc.io", + "prod": "api.tracebloc.io", + "": "api.tracebloc.io", + "weird": "api.tracebloc.io", + } + for in, want := range tests { + if got := backendHost(in); got != want { + t.Errorf("backendHost(%q) = %q, want %q", in, got, want) + } + } +} + +func TestCheckRequestsProxy(t *testing.T) { + rel := &cluster.ParentRelease{ReleaseName: "tb"} + tests := []struct { + name string + dep *appsv1.Deployment // nil => deployment absent + want Status + }{ + {"ready", requestsProxyDep("tb", 1), StatusOK}, + {"not-ready", requestsProxyDep("tb", 0), StatusFail}, + {"missing", nil, StatusFail}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cs := fake.NewClientset() + if tc.dep != nil { + cs = fake.NewClientset(tc.dep) + } + if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status != tc.want { + t.Fatalf("checkRequestsProxy = %v (%q), want %v", r.Status, r.Detail, tc.want) + } + }) + } +} + +func TestRun_HealthyCluster(t *testing.T) { + const rel = "tb" + cs := fake.NewClientset( + jobsManagerDep(rel, + corev1.EnvVar{Name: "REQUESTS_PROXY_URL", Value: "http://requests-proxy-service:8888"}, + corev1.EnvVar{Name: "CLIENT_ENV", Value: "dev"}, + ), + requestsProxyDep(rel, 1), + boundPVC(), + runningPod("tb-jobs-manager-abc"), + ) + + results := Run(bg(), cs, Options{ + Namespace: ns, + HTTPProbe: func(context.Context, string) error { return nil }, + }) + + if len(results) != 6 { + t.Fatalf("want 6 checks, got %d", len(results)) + } + if w := Worst(results); w != StatusOK { + for _, r := range results { + t.Logf("%-32s %-4s %s", r.Name, r.Status, r.Detail) + } + t.Fatalf("healthy cluster worst = %v, want ok", w) + } +} From 2c514b9c3fba3af48426527769a94d1ec78ae31e Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 15:47:10 +0500 Subject: [PATCH 2/6] fix(#88): don't flag Succeeded/recovered pods as crash-looping (Bugbot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit podCrashLooping flagged any pod with RestartCount>=3 — including Succeeded job pods that retried before completing, and Running pods that recovered after past restarts — producing a false ✖ when nothing is actually unhealthy. Guard terminal phases (Succeeded/Failed) and require the container to not be currently running, mirroring the controller's recovered-container fix (client-runtime#117). Adds regression tests for both false-positive cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 15 ++++++++++++--- internal/doctor/doctor_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 5e7a6aec..9b12f751 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -182,14 +182,23 @@ func checkPods(ctx context.Context, cs kubernetes.Interface, ns string) Result { } } -// podCrashLooping reports whether any container is in CrashLoopBackOff or has -// restarted past the threshold. +// podCrashLooping reports whether a pod is ACTIVELY crash-looping. +// +// Two false positives are deliberately excluded (Bugbot on PR #89; mirrors the +// controller's recovered-container fix in client-runtime#117): +// - Terminal pods (Succeeded/Failed) carry a historical RestartCount — a +// batch/ingestion pod that retried before completing is done, not unhealthy. +// - A high RestartCount on a container that is CURRENTLY running means the pod +// recovered on retry; only the active-backoff or not-running case counts. func podCrashLooping(p corev1.Pod) bool { + if p.Status.Phase == corev1.PodSucceeded || p.Status.Phase == corev1.PodFailed { + return false + } for _, c := range p.Status.ContainerStatuses { if c.State.Waiting != nil && c.State.Waiting.Reason == "CrashLoopBackOff" { return true } - if c.RestartCount >= crashRestartThreshold { + if c.RestartCount >= crashRestartThreshold && c.State.Running == nil { return true } } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 15123bf2..0f7c7e08 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -82,6 +82,34 @@ func crashPod(name string) *corev1.Pod { } } +// succeededPod is a finished job pod that retried before completing — a high +// RestartCount here is historical, not a current crash-loop (Bugbot on #89). +func succeededPod(name string, restarts int32) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{Name: "c", RestartCount: restarts}}, + }, + } +} + +// recoveredPod restarted several times but its container is running again now — +// recovered, not crash-looping (cf. controller recovered-container fix, #117). +func recoveredPod(name string, restarts int32) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "c", + RestartCount: restarts, + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }}, + }, + } +} + func pendingPod(name string, age time.Duration) *corev1.Pod { return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -124,6 +152,8 @@ func TestCheckPods(t *testing.T) { {"crash-loop", crashPod("bad"), StatusFail}, {"pending-old", pendingPod("stuck", -10*time.Minute), StatusWarn}, {"pending-fresh", pendingPod("fresh", -time.Minute), StatusOK}, + {"succeeded-high-restarts", succeededPod("done", 5), StatusOK}, + {"recovered-running", recoveredPod("recovered", 5), StatusOK}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { From d418f58aef1b89804aad89480349c3560cf4a9eb Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 15:54:40 +0500 Subject: [PATCH 3/6] fix(#88): detect init-container crash-loops + nil-release prefixed deploys (Bugbot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Medium Bugbot findings on the previous commit: - podCrashLooping ignored InitContainerStatuses, so an init container stuck in CrashLoopBackOff read as a Pending warning instead of a failure even though the pod cannot start. It now checks init + app containers, and detects only active CrashLoopBackOff — dropping the RestartCount heuristic entirely, since that was the source of the earlier Succeeded/recovered-pod false positives. - requestsProxyNames/jobsManagerNames only probed unprefixed names when the release was nil (e.g. DiscoverParentRelease errored on multiple releases), falsely reporting missing wiring even though -requests-proxy exists. Added findDeployment: exact-name Get, then a namespace List + name-suffix fallback that resolves the prefixed name without knowing the release. Adds regression tests: init-crash-loop, nil-release-finds-prefixed. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 61 ++++++++++++++++++++++++---------- internal/doctor/doctor_test.go | 27 +++++++++++++++ 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 9b12f751..dbb920cc 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -15,6 +15,7 @@ import ( "fmt" "net/http" "sort" + "strings" "time" appsv1 "k8s.io/api/apps/v1" @@ -72,9 +73,8 @@ func Worst(results []Result) Status { // Tunables. Exported-as-vars (not consts) so a future flag could override // them; kept conservative to avoid false positives on a busy cluster. const ( - crashRestartThreshold = 3 // restarts before we call it crash-looping - pendingGrace = 5 * time.Minute // a pod Pending longer than this is flagged - httpProbeTimeout = 8 * time.Second + pendingGrace = 5 * time.Minute // a pod Pending longer than this is flagged + httpProbeTimeout = 8 * time.Second ) // Options configures a diagnosis run. The zero value is usable: Namespace @@ -182,24 +182,27 @@ func checkPods(ctx context.Context, cs kubernetes.Interface, ns string) Result { } } -// podCrashLooping reports whether a pod is ACTIVELY crash-looping. +// podCrashLooping reports whether a pod has a container actively stuck in +// CrashLoopBackOff — the state Kubernetes sets for a container that keeps +// crashing. Both init AND app containers are checked: an init container in +// CrashLoopBackOff keeps the pod Pending and blocks startup silently (Bugbot +// on PR #89). // -// Two false positives are deliberately excluded (Bugbot on PR #89; mirrors the -// controller's recovered-container fix in client-runtime#117): -// - Terminal pods (Succeeded/Failed) carry a historical RestartCount — a -// batch/ingestion pod that retried before completing is done, not unhealthy. -// - A high RestartCount on a container that is CURRENTLY running means the pod -// recovered on retry; only the active-backoff or not-running case counts. +// We deliberately do NOT infer crash-looping from RestartCount: a high count +// is equally produced by a pod that recovered on retry, a job that retried +// before Succeeding, or a completed init container — all healthy (Bugbot on +// PR #89; cf. the controller's recovered-container fix, client-runtime#117). +// The terminal-phase guard is belt-and-suspenders: a Succeeded/Failed pod has +// no waiting containers anyway. func podCrashLooping(p corev1.Pod) bool { if p.Status.Phase == corev1.PodSucceeded || p.Status.Phase == corev1.PodFailed { return false } - for _, c := range p.Status.ContainerStatuses { - if c.State.Waiting != nil && c.State.Waiting.Reason == "CrashLoopBackOff" { - return true - } - if c.RestartCount >= crashRestartThreshold && c.State.Running == nil { - return true + for _, group := range [][]corev1.ContainerStatus{p.Status.InitContainerStatuses, p.Status.ContainerStatuses} { + for _, c := range group { + if c.State.Waiting != nil && c.State.Waiting.Reason == "CrashLoopBackOff" { + return true + } } } return false @@ -292,7 +295,7 @@ func backendHost(clientEnv string) string { // experiment silently stays Pending, the exact class this epic targets. func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) Result { const name = "Service Bus egress (requests-proxy)" - dep := getDeployment(ctx, cs, ns, requestsProxyNames(release)) + dep := findDeployment(ctx, cs, ns, requestsProxyNames(release), "requests-proxy") if dep == nil { return Result{ Name: name, @@ -317,7 +320,7 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, // returns an empty map when the deployment can't be fetched. func jobsManagerEnv(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) map[string]string { env := map[string]string{} - dep := getDeployment(ctx, cs, ns, jobsManagerNames(release)) + dep := findDeployment(ctx, cs, ns, jobsManagerNames(release), "jobs-manager") if dep == nil || len(dep.Spec.Template.Spec.Containers) == 0 { return env } @@ -343,6 +346,28 @@ func getDeployment(ctx context.Context, cs kubernetes.Interface, ns string, cand return nil } +// findDeployment returns the first existing deployment among candidates (exact +// Get), falling back to a namespace-wide list matched by name suffix. The +// fallback matters when the parent release couldn't be discovered (release nil +// => only the unprefixed candidate name), yet the chart installed a +// release-prefixed deployment like "-requests-proxy" — e.g. when +// multiple parent releases were detected (Bugbot on PR #89). +func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, candidates []string, suffix string) *appsv1.Deployment { + if d := getDeployment(ctx, cs, ns, candidates); d != nil { + return d + } + deps, err := cs.AppsV1().Deployments(ns).List(ctx, metav1.ListOptions{}) + if err != nil { + return nil + } + for i := range deps.Items { + if n := deps.Items[i].Name; n == suffix || strings.HasSuffix(n, "-"+suffix) { + return &deps.Items[i] + } + } + return nil +} + // jobsManagerNames / requestsProxyNames mirror the chart's naming: the // release-prefixed form first, then the bare form for older/unprefixed charts. func jobsManagerNames(release *cluster.ParentRelease) []string { diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 0f7c7e08..e253581a 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -121,6 +121,22 @@ func pendingPod(name string, age time.Duration) *corev1.Pod { } } +// initCrashPod has an init container stuck in CrashLoopBackOff — the pod stays +// Pending and cannot start, so it must read as a failure, not a Pending warning +// (Bugbot on #89). +func initCrashPod(name string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{{ + Name: "init", + State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"}}, + }}, + }, + } +} + func TestWorst(t *testing.T) { if got := Worst(nil); got != StatusOK { t.Fatalf("Worst(nil) = %v, want ok", got) @@ -154,6 +170,7 @@ func TestCheckPods(t *testing.T) { {"pending-fresh", pendingPod("fresh", -time.Minute), StatusOK}, {"succeeded-high-restarts", succeededPod("done", 5), StatusOK}, {"recovered-running", recoveredPod("recovered", 5), StatusOK}, + {"init-crash-loop", initCrashPod("initbad"), StatusFail}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -246,6 +263,16 @@ 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). +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) + } +} + func TestRun_HealthyCluster(t *testing.T) { const rel = "tb" cs := fake.NewClientset( From 289f237ea1e5de8375d001419a2fec3812b055ad Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 19:13:22 +0500 Subject: [PATCH 4/6] docs/polish(#88): address Arturo's post-approval review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Tunables comment: they're conservative package consts, not vars; point at Options (like HTTPProbe) for any future runtime tuning. - checkProxy WARN: note that a REQUESTS_PROXY_URL set via a configMap/secret ref reads as empty here (jobsManagerEnv reads only literal env). - httpProbe: a successful connection means reachable — discard the body-close error rather than reporting it as "unreachable". Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index dbb920cc..899a43a0 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -70,8 +70,9 @@ func Worst(results []Result) Status { return worst } -// Tunables. Exported-as-vars (not consts) so a future flag could override -// them; kept conservative to avoid false positives on a busy cluster. +// Conservative tunables, kept as package consts to avoid false positives on a +// busy cluster. If a check ever needs runtime tuning, thread it through Options +// (like HTTPProbe) rather than making these package vars. const ( pendingGrace = 5 * time.Minute // a pod Pending longer than this is flagged httpProbeTimeout = 8 * time.Second @@ -239,7 +240,11 @@ func checkProxy(env map[string]string) Result { return Result{ Name: name, Status: StatusWarn, - Detail: "jobs-manager has no REQUESTS_PROXY_URL (could not read jobs-manager env, or chart too old)", + // jobsManagerEnv reads only literal env values, so a chart that sets + // REQUESTS_PROXY_URL via a configMap/secret ref reads as empty here — + // called out in the remedy so a ref-based install isn't mistaken for + // missing wiring. + Detail: "jobs-manager has no literal REQUESTS_PROXY_URL (chart too old, or it's set via a configMap/secret ref)", Remedy: "Verify the requests-proxy is wired: kubectl set env deploy/-jobs-manager --list | grep PROXY", } } @@ -400,5 +405,8 @@ func httpProbe(ctx context.Context, url string) error { if err != nil { return err } - return resp.Body.Close() + // Connected — the host is reachable regardless of status code. Discard the + // close error so a rare post-connect close failure isn't reported as "down". + _ = resp.Body.Close() + return nil } From 86787e2a24332d4cb1d755f7017f281a0ce1da88 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 19:18:57 +0500 Subject: [PATCH 5/6] fix(#88): suffix fallback must not pick across multiple releases (Bugbot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findDeployment's suffix fallback (added for the nil-release case) picked the first suffix-matching deployment, so in a namespace running multiple parent releases, jobsManagerEnv and checkRequestsProxy could resolve to different releases in a single run — presenting mixed data as fact. Resolve the fallback only when exactly one deployment carries the suffix; with more than one (the multi-release case DiscoverParentRelease already refuses to disambiguate) return nil and let the check report can't-determine. The single-release nil-discovery case still resolves. Adds TestCheckRequestsProxy_NilReleaseAmbiguous. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 26 ++++++++++++++++++-------- internal/doctor/doctor_test.go | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 899a43a0..09ad280e 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -351,12 +351,18 @@ func getDeployment(ctx context.Context, cs kubernetes.Interface, ns string, cand return nil } -// findDeployment returns the first existing deployment among candidates (exact -// Get), falling back to a namespace-wide list matched by name suffix. The -// fallback matters when the parent release couldn't be discovered (release nil -// => only the unprefixed candidate name), yet the chart installed a -// release-prefixed deployment like "-requests-proxy" — e.g. when -// multiple parent releases were detected (Bugbot on PR #89). +// findDeployment returns the deployment for a chart component: first by exact +// name (the release-prefixed candidate, a fast Get), else by a namespace-wide +// list matched on name suffix. The suffix fallback covers a nil release (parent +// discovery failed) whose deployments are release-prefixed. +// +// The fallback resolves ONLY when exactly one deployment carries the suffix. +// With more than one — a namespace running multiple parent releases, which +// DiscoverParentRelease already refuses to disambiguate — there's no way to +// attribute them to a single release, and picking arbitrarily would let +// different checks (jobsManagerEnv vs checkRequestsProxy) describe different +// releases in one run. So we return nil and let the check report +// can't-determine rather than present mixed data as fact (Bugbot on PR #89). func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, candidates []string, suffix string) *appsv1.Deployment { if d := getDeployment(ctx, cs, ns, candidates); d != nil { return d @@ -365,12 +371,16 @@ func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, can if err != nil { return nil } + var match *appsv1.Deployment for i := range deps.Items { if n := deps.Items[i].Name; n == suffix || strings.HasSuffix(n, "-"+suffix) { - return &deps.Items[i] + if match != nil { + return nil // ambiguous across releases — don't guess + } + match = &deps.Items[i] } } - return nil + return match } // jobsManagerNames / requestsProxyNames mirror the chart's naming: the diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index e253581a..406b511d 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -273,6 +273,20 @@ func TestCheckRequestsProxy_NilReleaseFindsPrefixed(t *testing.T) { } } +// With multiple parent releases in one namespace (the case DiscoverParentRelease +// refuses) and no discovered release, the suffix fallback must NOT pick one +// arbitrarily — guessing could let different checks describe different releases +// in a single run (Bugbot on #89). It should report can't-determine, not OK. +func TestCheckRequestsProxy_NilReleaseAmbiguous(t *testing.T) { + cs := fake.NewClientset( + requestsProxyDep("relA", 1), // "relA-requests-proxy" + requestsProxyDep("relB", 1), // "relB-requests-proxy" + ) + if r := checkRequestsProxy(bg(), cs, ns, nil); r.Status == StatusOK { + t.Fatalf("ambiguous multi-release => %v (%q), want not-OK (no guessing)", r.Status, r.Detail) + } +} + func TestRun_HealthyCluster(t *testing.T) { const rel = "tb" cs := fake.NewClientset( From c9fa17b51d0aeae2d1dd4c01f2b0c804dcdcb135 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 18 Jun 2026 19:24:17 +0500 Subject: [PATCH 6/6] fix(#88): tie deployment lookup to the discovered release (Bugbot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a release was discovered, findDeployment's fallbacks could still match a DIFFERENT release's component (or a stray bare one), so the Service Bus check went green on the wrong requests-proxy while the discovered release's was missing. findDeployment now takes the release directly. When it's known, it accepts only "-" or a bare "" whose app.kubernetes.io/instance label ties it to that release — never another release's, never an unattributable bare one. The release-unknown path keeps the exactly-one-suffix-match rule (returns nil on >1, so checks report can't-determine rather than guess). Folds the jobsManagerNames/requestsProxyNames candidate builders into findDeployment. Adds tests: other-release-ignored, bare-name-tied-by-label. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/doctor/doctor.go | 62 +++++++++++++++------------------- internal/doctor/doctor_test.go | 25 ++++++++++++++ 2 files changed, 53 insertions(+), 34 deletions(-) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 09ad280e..6e6ee7ee 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -300,7 +300,7 @@ func backendHost(clientEnv string) string { // experiment silently stays Pending, the exact class this epic targets. 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, requestsProxyNames(release), "requests-proxy") + dep := findDeployment(ctx, cs, ns, release, "requests-proxy") if dep == nil { return Result{ Name: name, @@ -325,7 +325,7 @@ func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, // returns an empty map when the deployment can't be fetched. func jobsManagerEnv(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) map[string]string { env := map[string]string{} - dep := findDeployment(ctx, cs, ns, jobsManagerNames(release), "jobs-manager") + dep := findDeployment(ctx, cs, ns, release, "jobs-manager") if dep == nil || len(dep.Spec.Template.Spec.Containers) == 0 { return env } @@ -351,22 +351,34 @@ func getDeployment(ctx context.Context, cs kubernetes.Interface, ns string, cand return nil } -// findDeployment returns the deployment for a chart component: first by exact -// name (the release-prefixed candidate, a fast Get), else by a namespace-wide -// list matched on name suffix. The suffix fallback covers a nil release (parent -// discovery failed) whose deployments are release-prefixed. +// findDeployment locates a chart component's Deployment ("", e.g. +// "requests-proxy"), tied to the discovered release so a check can never be +// satisfied by a DIFFERENT release's component or a stray bare one (Bugbot on +// PR #89). // -// The fallback resolves ONLY when exactly one deployment carries the suffix. -// With more than one — a namespace running multiple parent releases, which -// DiscoverParentRelease already refuses to disambiguate — there's no way to -// attribute them to a single release, and picking arbitrarily would let -// different checks (jobsManagerEnv vs checkRequestsProxy) describe different -// releases in one run. So we return nil and let the check report -// can't-determine rather than present mixed data as fact (Bugbot on PR #89). -func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, candidates []string, suffix string) *appsv1.Deployment { - if d := getDeployment(ctx, cs, ns, candidates); d != nil { - return d +// Release known: take the chart's standard "-" name, or a bare +// "" ONLY when its app.kubernetes.io/instance label ties it to this +// release (older unprefixed charts). A deployment belonging to another release +// is never accepted — if this release's component is missing, return nil and let +// the check report it. +// +// Release unknown (discovery failed): match by name suffix, but only when +// EXACTLY ONE deployment carries it. With several (multiple releases, which +// DiscoverParentRelease refuses to disambiguate) there's no safe attribution, so +// return nil rather than guess — which would let different checks describe +// different releases in one run. +func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease, suffix string) *appsv1.Deployment { + if release != nil && release.ReleaseName != "" { + if d := getDeployment(ctx, cs, ns, []string{release.ReleaseName + "-" + suffix}); d != nil { + return d + } + if d := getDeployment(ctx, cs, ns, []string{suffix}); d != nil && + d.Labels["app.kubernetes.io/instance"] == release.ReleaseName { + return d + } + return nil } + deps, err := cs.AppsV1().Deployments(ns).List(ctx, metav1.ListOptions{}) if err != nil { return nil @@ -383,24 +395,6 @@ func findDeployment(ctx context.Context, cs kubernetes.Interface, ns string, can return match } -// jobsManagerNames / requestsProxyNames mirror the chart's naming: the -// release-prefixed form first, then the bare form for older/unprefixed charts. -func jobsManagerNames(release *cluster.ParentRelease) []string { - names := []string{"jobs-manager"} - if release != nil && release.ReleaseName != "" { - names = append([]string{release.ReleaseName + "-jobs-manager"}, names...) - } - return names -} - -func requestsProxyNames(release *cluster.ParentRelease) []string { - names := []string{"requests-proxy"} - if release != nil && release.ReleaseName != "" { - names = append([]string{release.ReleaseName + "-requests-proxy"}, names...) - } - return names -} - // httpProbe is the default backend prober: a GET with a short timeout over // Go's default transport, which honors HTTP(S)_PROXY/NO_PROXY via // http.ProxyFromEnvironment. Any HTTP response (even 4xx/5xx) means the host diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 406b511d..a47fd186 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -287,6 +287,31 @@ func TestCheckRequestsProxy_NilReleaseAmbiguous(t *testing.T) { } } +// With a release discovered, the check must be tied to THAT release: another +// release's requests-proxy must not be accepted as the discovered release's, +// or relA goes green on relB's proxy while relA's is actually missing +// (Bugbot on #89). +func TestCheckRequestsProxy_DiscoveredReleaseIgnoresOtherReleases(t *testing.T) { + rel := &cluster.ParentRelease{ReleaseName: "relA"} // relA has no requests-proxy + cs := fake.NewClientset(requestsProxyDep("relB", 1)) + if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status == StatusOK { + t.Fatalf("relA proxy missing, relB present => %v (%q), want not-OK", r.Status, r.Detail) + } +} + +// A bare (unprefixed) requests-proxy is accepted only when its instance label +// ties it to the discovered release — covering older unprefixed charts. +func TestCheckRequestsProxy_BareNameAcceptedWhenLabelledForRelease(t *testing.T) { + rel := &cluster.ParentRelease{ReleaseName: "relA"} + bare := requestsProxyDep("relA", 1) + 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) + } +} + func TestRun_HealthyCluster(t *testing.T) { const rel = "tb" cs := fake.NewClientset(