feat(shard): remediate quarantined poolers by wiping and recreating - #568
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
mkindahl
force-pushed
the
feat/quarantine-remediation
branch
from
August 11, 2026 11:33
d3cca4f to
0a02f8c
Compare
This comment has been minimized.
This comment has been minimized.
DRAFT — Layer 2 of the unrecoverable-postgres work. When a multipooler self-quarantines (LIFECYCLE_QUARANTINED: postgres is unrecoverably failing to start, e.g. a genuinely diverged standby), the shard controller now replaces it: delete the backing pod AND hard-delete its data PVC, so the recreated pod (via createMissingResources) re-bootstraps clean from backup. This is the only remediation that heals genuine on-disk divergence — a same-PVC restart FATAL-loops identically. - topo: GetQuarantinedPods surfaces the precise per-pod quarantine signal (pod name + the reason recorded on the lifecycle entry). GetPoolerStatus now maps quarantined poolers to a distinct QUARANTINED role — visible in Shard.Status.PodRoles, excluded from the scale-down health gate, and no longer driving the stand-in-replica path (which keyed on DRAINED). - shard: reconcileQuarantineRemediation runs in the topo-aware data-plane phase, before the drain state machine. Gating: at most one pod per reconcile, never the primary, only when the pool is otherwise healthy, only for pods old enough to rule out a stale topology record, and — the key safety gate — only when a healthy backup exists to restore from (the hard-delete is irreversible; without a good backup it would destroy the last copy). Requeues after acting; the reason is echoed on the QuarantineRemediation event. Part of MUL-1009. Signed-off-by: Mats Kindahl <mats.kindahl@supabase.io>
mkindahl
force-pushed
the
feat/quarantine-remediation
branch
from
August 11, 2026 11:44
0a02f8c to
df0157c
Compare
mkindahl
marked this pull request as ready for review
August 11, 2026 11:44
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
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
2 participants
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.
What & why
This is Layer 2 of the unrecoverable-postgres work (Layer 1 is the pooler-side self-quarantine in multigres/multigres). When a multipooler's PostgreSQL is unrecoverably failing to start — the classic case being a genuinely diverged standby — the pooler self-quarantines: it writes
LIFECYCLE_QUARANTINEDto its own topology record and marks itself cohort-INELIGIBLE.The problem this PR solves — and why a rebuild is necessary: a quarantined pod is invisible to Kubernetes. postgres is a child of pgctld, so both containers stay
Runningand the pod staysReady(readiness gates on the gRPC control plane, not postgres).Crucially, the cluster does not automatically add a replacement pooler.
createMissingResourcesreconciles each replica index0..replicas-1and only creates a pod/PVC for an index that has no pod. The quarantined pod is stillRunningand still occupies its index, so it is counted as a present replica — the index isn't missing, socreateMissingResourcescreates nothing. The shard simply sits one healthy replica short indefinitely, with a zombie holding the slot. Nothing else evicts it either: pooler pruning only fires when a pod is absent, and Kubernetes seesReady. So no fresh standby appears on its own.That is exactly why the quarantined pooler must be rebuilt. And a plain pod restart on the same PVC just FATAL-loops again — the on-disk WAL is the problem — so the rebuild has to discard the bad data and re-bootstrap a fresh standby from backup. That is what this PR does.
What it does
The shard controller now remediates a quarantined pooler by wiping it in place: delete the backing pod and hard-delete its data PVC, so the existing
createMissingResourcesrecreates both at the same identity/index and the fresh, empty volume makes pgctld restore from backup and rejoin as a standby.topo.GetQuarantinedPodssurfaces the precise per-pod quarantine signal — pod name plus the human-readablereasonrecorded on the topology lifecycle entry (e.g. "postgres failed to recover for 5m0s across 60 attempts …").GetPoolerStatusnow maps a quarantined pooler to a distinctQUARANTINEDrole instead ofDRAINED. It's visible inShard.Status.PodRoles, excluded from the scale-down health gate, and — deliberately — no longer drives the stand-in-replica path (which keyed onDRAINED). So a quarantined pooler is replaced in place rather than having a stand-in provisioned at a new index while the bad pod lingers.reconcileQuarantineRemediationruns in the topo-aware data-plane phase, before the drain state machine.Why rebuild in place, instead of creating a new server and setting the quarantined one aside?
That "provision a fresh replica now, keep the bad one for later inspection" model is essentially what the code did before this PR: quarantined mapped to the
DRAINEDrole, which bumped the effective replica count (effectiveReplicas = replicas + drainedCount) so a stand-in was created at a new, higher index while the quarantined pod kept running. We deliberately moved away from it:…-0,…-1,data-…-1). You can't bring up a second pod at the same identity, so "create a new one immediately" means a different index — leaving the replica set non-contiguous, and (since nothing removed the quarantined pod) permanently at N+1 pods, one of them a dead zombie pinning a full-size data PVC indefinitely.INELIGIBLE/dropped from consensus. So keeping it "to the side" buys zero availability; it's pure cost (a wasted pod slot + volume). Rebuilding the same index converges to exactly the desired replica set with no leftover state to later reconcile away.reason. If the diverged bytes themselves are needed, aVolumeSnapshottaken before the wipe preserves them without pinning a live pod/PVC — a follow-up could add that. A permanently-retained zombie pod is a poor forensics mechanism.Gating (all required before any destructive wipe)
backuphealth.ConditionHealthy) — the data-safety gate. The hard-delete is irreversible, so with no restore source we must not destroy the last copy. Absent/false blocks remediation and emitsQuarantineRemediationBlocked.QUARANTINEDrecord.The reason is echoed on the
QuarantineRemediationevent sokubectl describe shardshows the postgres-level cause.Deliberately out of scope
UNRECOVERABLElifecycle state / no quarantine attempt-cap. A reconstructed pod is just a new standby, so if reconstruction repeatedly fails (e.g. a corrupt backup) that is a general standby bring-up / restore failure — flagged by backup-health and standby-provisioning monitoring — independent of whether the pod was ever quarantined. Encoding it into the quarantine lifecycle would conflate two orthogonal concerns.Testing
reconcile_quarantine_internal_test.go— wipes-when-healthy (asserts pod + data PVC deleted and the reason on the event), defer-when-too-young, defer-when-pool-unhealthy, defer-when-no-healthy-backup (never wipe the last copy), and no-op-when-nothing-quarantined. Plus the topo role test updated to expect theQUARANTINEDrole.golangci-lintclean.Follow-ups before merge (need envtest, not the fake client)
pvc-protectionfinalizer clears → PV reclaimed). The fake-client tests don't model finalizers; this needs the//go:build integrationenvtest path.Delete(dynamic-provisioning default); on aRetainstorage class the volume is only released and the data wouldn't actually be reclaimed. Worth asserting/handling explicitly.DRAINEDstand-in machinery (unused once quarantine no longer maps toDRAINED) can be removed in a follow-up.Related PRs
LIFECYCLE_QUARANTINEDsignal this PR consumes.DRAINEDstand-in machinery noted above (stacked on this PR).Part of MUL-1009.