📢 แจ้งเตือน: skyhook io/radar/pull/

nadaverell · 2026-07-12T12:23:49Z

Ground-up fix for slow and failing Prometheus discovery on cold start. On a cluster with several Prometheus-like services, concurrent cost/traffic requests each ran their own discovery and fought over the process-global port-forward singleton — churning for 20–30s and often failing to connect. Verified against a live GKE cluster: ~24s-and-fail → ~6s-and-connect.

What changed

Coalesce discovery

Concurrent EnsureConnected callers share one discovery via singleflight; the shared run detaches from the leader's request context so it completes for every waiter, while each caller selects on its own context and returns promptly when cancelled.
A discovery generation (bumped by Reset/SetManualURL/SetHeaders) keys the flight and gates the connection write; markConnected reports whether it committed, so a run superseded by a config change surfaces a retryable error instead of a hollow success (address returned but baseURL empty).
A process-global gate serializes discovery across Client instances too — a client reinit on context setup can leave the startup pre-warm and the first request discovering at once, and only a global gate keeps them off the shared port-forward. It's context-aware, and Reset/Reinitialize cancel the in-flight run so a superseded discovery releases the gate promptly.

Prune candidates

Non-query services (node-exporter, kube-state-metrics, operator, alertmanager, pushgateway, grafana, blackbox, Thanos non-query, control-plane scrape targets) are excluded, and a minimum dynamic score drops bare name matches (kubelet, otel-collector). Scoring credits a recognized container targetPort (not just the service port), so a real Prometheus behind a port-80 service still qualifies. Duplicates across the well-known list and dynamic scan are de-duplicated.

Enumerate & probe faster

Well-known locations are fetched concurrently (bounded) instead of ~20 serial Gets — the dominant latency against a remote API server — assembled in priority order, still via targeted Gets (get-without-list callers keep working), logging serially to honor the Logger contract. Direct probing runs for every environment (connects in-cluster and over networks that route Service DNS, e.g. VPN), bounded by a budget larger than the per-probe timeout so a slow-but-reachable endpoint gets a full attempt while an unresolvable cluster-DNS name (which can wedge in the platform cgo resolver past its deadline) can't stall the pass. Port-forwarding stays the serial fallback.

Log trail

One line lists the candidates; the direct pass summarizes in one line (not one rejection per candidate); each phase (enumerate / direct probe / total) carries its own elapsed time; a superseded run is logged as such, not as a failure.

Tests

pkg/prom: de-dup, scoring disqualifiers, control-plane exclusion, unlabeled-custom-prometheus kept, concurrency-safe Logger. internal/prometheus: singleflight coalescing, prompt cancellation, stale-generation gate, Reset cancels in-flight discovery, batch respects its budget, probe ordering. Race-clean under -race.

Port-forward ownership (folded in)

The metrics port-forward was a process-global singleton shared by prometheus discovery and the traffic subsystem (Caretta/Hubble); when they wanted different services concurrently they clobbered each other. Now each subsystem owns its own metrics forward (OwnerPrometheus/OwnerTraffic) — Start/Stop only ever touch the caller's forward, cross-owner reuse stays read-only, and traffic connection status reads the live registry.

Deferred follow-up

OpenCost duplicate-series 422 — clusters with two kube-state-metrics instances break the storage-cost PV join (costs show 0); needs an RHS-dedup in the query. (Not discovery/port-forward related.)

Note

Medium Risk
Touches shared connection discovery, port-forward lifecycle, and context-switch cancellation paths; behavior is complex but heavily covered by new tests.

Overview
Fixes cold-start Prometheus discovery where parallel EnsureConnected callers and prometheus/traffic both fought over one metrics port-forward, often failing after long churn.

Discovery coalescing & lifecycle: Concurrent discovery is merged with singleflight and a process discovery gate, keyed by a discovery generation bumped on Reset / SetManualURL / SetHeaders. Detached runs can be cancelled on context switch; stale results are dropped via markConnected / connectionLive instead of hollow successes. Direct candidate probing is bounded and concurrent (priority-ordered, time-budgeted) before serial port-forward fallback.

Candidate quality & speed: pkg/prom.Discover excludes non-query metrics services, requires a Prometheus identity signal (not namespace-only neighbors), de-duplicates well-known vs dynamic hits, and fetches well-known Services in parallel. Quieter, phase-timed discovery logs.

Port-forward ownership: Replaces the global metrics forward with per-owner forwards (OwnerPrometheus / OwnerTraffic); Stop only tears down the caller’s tunnel, with epoch guards if Stop races in-flight Start. Prometheus Reset explicitly stops its forward; traffic/Caretta/Hubble pass OwnerTraffic.

^{Reviewed by Cursor Bugbot for commit 2ecb582. Bugbot is set up for automated code reviews on this repo. Configure here.}

On a cluster with several Prometheus-like services, concurrent cost/traffic requests each ran their own discovery and fought over the process-global port-forward singleton, churning for 20-30s and often failing to connect. Reworks discovery end to end; verified against a live GKE cluster (~24s-and-fail → ~6s-and-connect). Coalesce discovery. Concurrent EnsureConnected callers share one discovery via singleflight; the shared run detaches from the leader's request context so it completes for every waiter, while each caller selects on its own context and returns promptly when cancelled. A discovery generation (bumped by Reset / SetManualURL / SetHeaders) keys the flight and gates the connection write, so a run started under an old configuration can't publish a stale endpoint; markConnected reports whether it committed, so a superseded run surfaces a retryable error instead of a hollow success, and Set*/Reset cancel the in-flight run so it releases the gate promptly. A process-global gate serializes discovery across Client instances too — a client reinit on context setup can leave the startup pre-warm and the first request discovering at once, and only a global gate keeps them off the shared port-forward; Reinitialize both cancels and retires the outgoing client so even a flight that hadn't started yet aborts instead of driving the forward for a replaced context. Admit by identity, rank by score, gate by probe. Services we're certain can't answer PromQL — node-exporter, kube-state-metrics, the operator, alertmanager, pushgateway, grafana, blackbox, Thanos non-query components, control-plane scrape targets (kubelet, kube-etcd, …) — are excluded by identity. A candidate is admitted only on a Prometheus-family identity signal (a name/label/port match); the heuristic score merely ranks. Namespace membership alone does not admit — discovery probes candidates with the operator's configured Prometheus headers, so an unrelated neighbor must not be a candidate just for sharing a namespace — and a lightly-labeled real Prometheus is kept and validated by the probe rather than dropped by a score floor. Candidates found by both the well-known list and the dynamic scan are de-duplicated. Enumerate and probe faster. Well-known locations are fetched concurrently (bounded) instead of ~20 serial Gets — the dominant latency against a remote API server — assembled in declared priority order, still via targeted Gets so get-without-list callers keep working, and logging stays serial to honor the Logger callback contract. Direct in-cluster probing runs for every environment (it connects in-cluster and over networks that route Service DNS, e.g. VPN), launched in priority order under a budget larger than the per-probe timeout so a slow-but-reachable endpoint gets a full attempt while an unresolvable cluster-DNS name — which can wedge in the platform cgo resolver past its deadline — can't stall the pass; a candidate that already answered is never dropped on budget expiry, even if its completion was still buffered. Port-forwarding stays the serial fallback. Rework the log trail. One line lists the candidates; the direct pass summarizes in a single line instead of one rejection per candidate; each phase (enumerate / direct probe / total) carries its own elapsed time; a run that ends on a context error is logged as superseded or timed-out rather than as a failure.

…t clobber The metrics port-forward was a process-global singleton: one active tunnel, started/stopped by both prometheus discovery and the traffic subsystem (Caretta/Hubble). When they wanted different services concurrently, each Start tore down the other's forward. Hold one forward per owner (OwnerPrometheus / OwnerTraffic) instead. Start replaces only the calling owner's forward; Stop stops only theirs. Owners may still read each other's forward address (GetAddress) to reuse a compatible endpoint, but never stop it — read-only. Since traffic's Reset no longer stops the singleton (and thus incidentally prometheus's forward), prometheus's Reset now tears down its own forward on context switch / config change. Traffic connection status reads the live registry (its own forward, else any active forward for the context it may be reusing) rather than caching a snapshot that could report a stopped forward as connected. This is the per-subsystem fix for cross-owner clobbering; per-prometheus coalescing (singleflight + discovery gate) and the config/context-switch client-state guards are unchanged.

Map iteration made reuse selection nondeterministic when both owners hold a forward for the context: prometheus rediscovery could be handed Hubble's relay instead of its own live forward, fail the probe, and fall through to a full rediscovery. GetAddress now returns the caller's own forward first, then a peer's — deterministic, and never skips a live own forward.

A discovery superseded (Reset/Reinitialize) mid-fallback kept iterating the candidate list, holding the discovery gate, so a newer discovery waited. Check ctx before each port-forward attempt and return superseded immediately.

…resh read discoverShared keyed the singleflight on the generation but discover re-read the live discoveryGen as startGen. A flight keyed before a Reset could start after it, read the bumped generation, and markConnected under it — undoing the reset and holding the discovery gate. Thread the flight's generation into discover so markConnected checks against it.

The discovered bool was set on connect/reset but never read for any decision — connection status keys off baseURL. Remove the dead state.

GetConnectionInfo fell back to any owner's forward for the context, so traffic showed connected using Prometheus's forward even when traffic never adopted it (e.g. after a failed Hubble connect). Traffic's data path always uses its own forward, so a peer's forward means the peer is connected, not traffic. Drops the now-unused GetConnectionInfoForContext.

A config change or context switch mid-discovery cancels the run; the manual-URL probe and candidate enumeration reported that as "URL not reachable" / "no Prometheus found" in errorlog instead of recognizing supersession, unlike the later direct-probe and port-forward paths. Add the same ctx.Err() guard so the diagnostics stay honest.

…supersedes a run When Reset/SetManualURL/SetHeaders cancelled an in-flight discovery, a waiter whose own request context was still live received the supersession error and surfaced it (opencost reported Prometheus unavailable for an otherwise-healthy request). discoverShared now retries under the fresh generation while the caller's context is live; a retired client still returns promptly rather than retrying, since retirement is permanent.

The port-forward fallback loop recorded 'port-forward failed' / 'not responding' in errorlog for the in-flight candidate when a config change or context switch cancelled the run mid-attempt. Check ctx.Err() after Start and after the probe so supersession is surfaced, matching the guard the loop's other paths already have.

… ready wait Start held reg.mu for its whole body, including findPodForService and the up-to-10s wait for the forward to become ready. That blocked every other registry operation — the other owner's Start, Stop, GetAddress, and traffic's status reads — for the duration, and serialized the two owners' forwards that the per-owner split exists to keep independent. Hold reg.mu only for the short slot/field access and establish the forward without it; a per-owner establishMu serializes same-owner Starts so a race can't leak a forward goroutine.

+		info := f.info()
+		reg.mu.Unlock()
+		log.Printf("[portforward] Ready: localhost:%d -> %s/%s:%d (owner=%s, context: %s)",
+			localPort, namespace, serviceName, targetPort, owner, contextName)


…d-establish Narrowing Start's lock scope opened a window: while a forward was still coming up (not yet active), a concurrent Stop — e.g. a context-switch Reset — no-op'd, then Start committed the forward anyway, leaving a connection to the old context reported as live. Bump a per-owner epoch on every teardown (including when the forward is only establishing) and have Start refuse to publish if the epoch moved while it was establishing.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit c4dd177. Configure here.}

… it published its cancel A worker keyed at one generation could reach its cancel-publish point after a Reset/SetManualURL/SetHeaders had already bumped the generation and called cancel() — a no-op while the handle was still nil. It would then hold the discovery gate and a live detached context for up to discoveryTimeout, blocking rediscovery under the new configuration. Abort at publish when the generation no longer matches, alongside the existing retirement check.

nadaverell requested a review from hisco as a code owner July 12, 2026 12:23

nadaverell changed the title ~~prometheus: de-dupe discovery candidates and probe in-cluster in parallel~~ prometheus: redesign discovery (singleflight + candidate pruning + reachability probing) Jul 12, 2026