Placement shim: detect a lost remote apiserver and rebuild the manager - #1190
Merged
Conversation
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
September 4, 2026 09:11
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are small but concrete fixups needed (e.g., duplicated doc comment and preserving cancel cause in the manager exit path to keep restart diagnostics accurate).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an external liveness signal to the placement shim so it can detect when a remote apiserver becomes truly unreachable (transport failure) and force a controller-manager rebuild, avoiding the “silent idle” case where controller-runtime retries a dead watch forever and mgr.Start() never returns.
Changes:
- Introduces per-remote
/readyzreachability probing with sustained-failure detection, and uses it to cancel a per-cycle manager context to trigger supervisor rebuilds. - Adds a process-lifetime Prometheus gauge for per-remote reachability plus a corresponding alert.
- Updates tests and the multicluster guide RBAC/docs to support the new behavior and reduce noise.
File summaries
| File | Description |
|---|---|
| pkg/shim/supervisor/supervisor_test.go | Adds coverage ensuring the supervisor rebuilds when a manager cycle cancels its own child context while the parent context remains live. |
| pkg/multicluster/monitor.go | Adds a per-remote reachability gauge to the multicluster monitor and exposes it via Describe/Collect. |
| pkg/multicluster/monitor_test.go | Tests reachability gauge behavior (1/0 and overwrite semantics). |
| pkg/multicluster/client.go | Demotes “scheme gvk registered” logging to V(1) to reduce noise. |
| pkg/multicluster/client_test.go | Extends the fake cluster to provide a non-nil GetConfig() (host) for probing/logging paths used in production. |
| pkg/multicluster/client_probe.go | Implements remote deduplication and sustained transport-failure probing against /readyz, with per-remote goroutines and threshold-based onLost. |
| pkg/multicluster/client_probe_test.go | Adds tests for classification (HTTP response vs transport failure) and sustained-failure trigger/reset behavior + gauge updates. |
| helm/bundles/cortex-placement-shim/templates/alerts.yaml | Adds an alert that fires per host when the reachability gauge is persistently 0. |
| docs/guides/multicluster/run.sh | Stops pinning the guide to ACTIVE_DEPLOYMENTS="nova" when starting Tilt. |
| docs/guides/multicluster/cortex-remote-crb.yaml | Adds the placement shim service account user to the remote cluster-admin binding to fix guide RBAC. |
| cmd/shim/main.go | Derives a per-cycle cancelable context, starts reachability probing, and runs mgr.Start() on the cycle context so probe-triggered cancellation forces a rebuild. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Philipp Matthes <p.matthes@sap.com>
PhilippMatthes
force-pushed
the
placement-shim-detect-lost-remote
branch
from
September 4, 2026 09:34
40ff114 to
8125b28
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the shim’s manager lifecycle semantics and introduces automated restart triggers based on remote apiserver reachability, which warrants final human validation of operational behavior and failure modes.
Review details
Suppressed comments (1)
pkg/multicluster/client_probe.go:201
- The probeReachable doc comment is duplicated, which makes the godoc noisy and harder to read. Remove the redundant first block so only one comment remains.
// probeReachable issues a lightweight GET /readyz against the remote apiserver
// and reports whether the apiserver is reachable. Any HTTP response — including
// non-2xx statuses surfaced as a Kubernetes StatusError — counts as reachable;
// only a transport-level error (connection refused, no route, TLS failure,
// timeout) counts as unreachable, which is the signature of a deleted cluster.
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
KonstantinGasser
approved these changes
Sep 4, 2026
PhilippMatthes
requested review from
Copilot
and removed request for
Copilot
September 4, 2026 10:47
Contributor
Test Coverage ReportTest Coverage 📊: 73.1% |
juliusclausnitzer
added a commit
that referenced
this pull request
Sep 7, 2026
## Release cortex v0.4.1, cortex-shim v0.1.17 ### cortex-shim v0.1.17 (sha-cd38777a) New features: - Remote apiserver liveness probe — adds a per-remote reachability probe (`GET /readyz`) that detects when a remote apiserver becomes unreachable after its informer cache has synced; after a configurable failure threshold (default 3 × 10s), the probe cancels the manager cycle context so the existing self-healing supervisor rebuilds the manager with current config (#1190) - `cortex_multicluster_remote_apiserver_reachable{host}` gauge — exposes per-remote apiserver reachability on the process-lifetime monitor, surviving manager rebuilds (#1190) - `CortexPlacementShimRemoteApiserverUnreachable` alert — fires when a remote apiserver is unreachable (#1190) Non-breaking changes: - Fix multicluster recorder for controller-runtime v0.25.0 `EventRecorder` interface change — adds `AnnotatedEventf` method to `MultiClusterRecorder` to satisfy the wider `recorder.EventRecorder` interface (#1191) - Update `sigs.k8s.io/controller-runtime` to v0.25.0 (#1188) - Update `sigs.k8s.io/controller-tools` to v0.22.0 (CRD annotation bump) (#1186) - Update `github.com/sapcc/go-bits` (#1187) - Update `kube-prometheus-stack` to v89 (#1189) ### cortex v0.4.1 (sha-cd38777a) Non-breaking changes: - Fix multicluster recorder for controller-runtime v0.25.0 `EventRecorder` interface change (#1191) - CRDs regenerated with controller-tools v0.22.0 (#1186) - Update `sigs.k8s.io/controller-runtime` to v0.25.0 (#1188) - Update `github.com/sapcc/go-bits` (#1187) ### cortex-placement-shim v0.1.17 Includes updated chart cortex-shim v0.1.17. ### cortex-nova v0.0.90 Includes updated chart cortex v0.4.1. ### cortex-cinder v0.0.90 Includes updated chart cortex v0.4.1. ### cortex-manila v0.0.90 Includes updated chart cortex v0.4.1. ### cortex-crds v0.0.90 Includes updated chart cortex v0.4.1. ### cortex-ironcore v0.0.90 Includes updated chart cortex v0.4.1. ### cortex-pods v0.0.90 Includes updated chart cortex v0.4.1. --- ### Chart bumps | Chart | Previous | New | |---|---|---| | `cortex` (library) | `0.4.0` / `sha-91df27a0` | `0.4.1` / `sha-cd38777a` | | `cortex-shim` (library) | `0.1.16` / `sha-b577e306` | `0.1.17` / `sha-cd38777a` | | `cortex-placement-shim` (bundle) | `0.1.16` | `0.1.17` | | `cortex-nova` (bundle) | `0.0.89` | `0.0.90` | | `cortex-cinder` (bundle) | `0.0.89` | `0.0.90` | | `cortex-manila` (bundle) | `0.0.89` | `0.0.90` | | `cortex-crds` (bundle) | `0.0.89` | `0.0.90` | | `cortex-ironcore` (bundle) | `0.0.89` | `0.0.90` | | `cortex-pods` (bundle) | `0.0.89` | `0.0.90` | No bumps for: cortex-postgres.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The placement shim runs a self-healing supervisor that rebuilds the controller-manager only when
mgr.Startreturns. Remote apiservers are added as manager Runnables, and when a remote cluster is deleted after its informer cache already synced (e.g.kind delete cluster --name kind-cortex-remote-az-a), controller-runtime's informer retries the dead watch forever and never errors. Somgr.Startnever returns, the supervisor never rebuilds,cortex_placement_shim_manager_upstays 1, and the pod silently idles.Fix
Add an external liveness signal: a per-remote reachability probe (
GET /readyz) that, on sustained failure, cancels a per-cycle context bound tomgr.Start, causing it to return so the existing supervisor backoff rebuilds the whole manager (which re-reads config and reconnects to currently-configured remotes).client_probe.go:UniqueRemotes()dedupes remotes by cluster identity;ProbeRemotes()runs one probe goroutine per remote viawait.PollUntilContextCancel, tracks consecutive failures, and callsonLost(host)afterFailureThreshold(default 3 × 10s ≈ 30s, deliberately above the supervisor backoff floor to keep the rebuild loop period bounded). Classification keys on the transport outcome: any HTTP response (incl. 401/403/5xxStatusError) = reachable; only a transport failure (connection refused, TLS/handshake failure, timeout) = unreachable — so an authz/server error never triggers a rebuild storm.main.go:buildAndStartderives a per-cyclecontext.WithCancelCause, launchesProbeRemotes, and bindsmgr.Start(cycleCtx). Nosupervisor.gochange needed — the supervisor already rebuilds on everyBuildAndStartreturn.monitor.go: newcortex_multicluster_remote_apiserver_reachable{host}gauge on the process-lifetime Monitor (survives manager rebuilds).alerts.yaml: newCortexPlacementShimRemoteApiserverUnreachablealert.reachability probe:log lines at every decision point (start, per-remote watch, per-tick outcome at V(1) with the underlying error, unreachable/threshold/lost/recovered transitions).Guide RBAC fix
Running the shim against the multicluster kind guide produced
hypervisors.kvm.cloud.sap is forbiddenfor user...#system:serviceaccount:default:cortex-placement-shim: the shim's SA user was missing from the remote clusters' cluster-admin ClusterRoleBinding. Added it as a subject tocortex-remote-crb.yaml(applied to both remotes by the existingrun.shsteps).Auxiliary changes
Also included (unrelated to the core fix, bundled for convenience):
run.sh: no longer pins the guide toACTIVE_DEPLOYMENTS="nova".client.go: demotes a noisy per-GVK log line toV(1).Testing
makepasses (generate produced no diffs, lint + format clean, full test suite green). New/updated tests cover probe classification, sustained-failure trigger + gauge, no-trigger-when-reachable, no-remotes no-op, transient-blip counter reset, the monitor gauge setter, and a supervisor test locking down the exact internal-cancel → rebuild mechanismmain.gorelies on.pkg/multiclusterandpkg/shim/supervisoralso pass under-race.Assisted-by: Claude Code:claude-opus-4-8 [Bash] [Read]