refactor(shard): remove dormant DRAINED stand-in machinery - #570
Merged
mkindahl merged 3 commits intoSep 25, 2026
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
mkindahl
force-pushed
the
refactor/remove-drained-standin
branch
from
August 14, 2026 12:33
75e787c to
656025b
Compare
This comment has been minimized.
This comment has been minimized.
mkindahl
marked this pull request as draft
August 17, 2026 06:11
mkindahl
force-pushed
the
refactor/remove-drained-standin
branch
from
September 11, 2026 08:44
656025b to
19e4cb4
Compare
This comment has been minimized.
This comment has been minimized.
Quarantine remediation (delete pod + wipe data PVC + re-bootstrap from backup) supersedes the older "stand-in replica" model, in which a pod whose topology role was DRAINED was kept alive for investigation while a replacement replica was provisioned at a higher index. GetPoolerStatus only ever emits PRIMARY, REPLICA, or QUARANTINED, so nothing sets the DRAINED role anymore and that machinery is dead code. Remove it: - countDrainedPods (topology-role counter) and every effectiveReplicas = replicas + drainedCount offset that keyed on it. createMissingResources and handleScaleDown retain an effectiveReplicas parameter, but it now reflects only maintenance surges (replicas + maintenanceSurges); the DRAINED term is gone. - the drainedCount offsets that maintenance-surge indexing (maintenance_surge.go) and disruption-budget accounting (disruption.go) had picked up; both now use the user-desired replica count directly. These were no-ops because countDrainedPods was always zero. - syncDrainedLabels and the multigres.com/role (LabelPodRole) label it maintained, plus the DRAINED branch in cleanupDrainedPod that orphaned a pod's PVC unconditionally. - the DRAINED short-circuits in isDrainStale, isPoolHealthy, and the maintenance-surge base-stability check (isPoolHealthy still excludes QUARANTINED pods). No behavior change for live paths: rolling updates, scale-down, maintenance surges, and quarantine remediation are unaffected. Tests that exercised the removed stand-in behavior are deleted or refolded onto the index-vs-replicas semantics that remain. Part of MUL-1009. Signed-off-by: Mats Kindahl <mats.kindahl@supabase.io> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mkindahl
force-pushed
the
refactor/remove-drained-standin
branch
from
September 24, 2026 07:23
19e4cb4 to
86ef292
Compare
This comment has been minimized.
This comment has been minimized.
mkindahl
marked this pull request as ready for review
September 24, 2026 07:31
Contributor
|
/e2e |
|
✅ E2E tests success — View run |
Verolop
approved these changes
Sep 24, 2026
Verolop
left a comment
Verolop
left a comment
Contributor
There was a problem hiding this comment.
lgtm!
Remaining references to DRAINED() need to be addressed; can be included here or as a follow up
Follow-up to the DRAINED stand-in removal, addressing review feedback. - Update the Shard.Status.PodRoles godoc to list QUARANTINED instead of DRAINED and regenerate the CRD description. - Refresh the operator capability, pod-management-architecture, and pvc-lifecycle docs: replace the DRAINED stand-in / DRAINED-specific PVC handling with the quarantine-remediation and orphan-on-scale-down behavior that actually runs today. - Add a dated API-design changelog entry recording the removal. The multiorch-facing "DRAINED Pooler Replacement" narrative in pod-management-design.md is left in place but marked obsolete / needs rewrite, since rewriting the multiorch internals is out of scope here. Signed-off-by: Mats Kindahl <mats.kindahl@supabase.io> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The observer's replication checks special-cased the DRAINED role and lumped everything else into the replica bucket. Since the operator now surfaces unrecoverable poolers as QUARANTINED (never DRAINED), quarantined pods fell through as replicas: they inflated the expected replica count (producing false "primary has fewer standbys than expected" findings), got probed as replicas (spurious "no WAL receiver" errors), and were never surfaced as a distinct condition. Replace the dead DRAINED handling with QUARANTINED: count and report quarantined pods separately, exclude them from the replica set so expectedReplicas is correct, and update the finding wording to describe quarantine remediation. Add unit coverage for the classification and counting paths. Signed-off-by: Mats Kindahl <mats.kindahl@supabase.io> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
Contributor
Author
Thanks for the review @Verolop! I did the following additions, as agreed on separately:
|
Contributor
Author
|
/e2e |
|
✅ E2E tests success — View run |
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.
Summary
Removes the now-dormant DRAINED stand-in machinery. Quarantine remediation (#568) supersedes it: a pooler whose PostgreSQL is unrecoverable is now marked
QUARANTINEDand the operator wipes + re-bootstraps the pod in place, rather than keeping the bad pod alive and provisioning a stand-in replica at a higher index. Nothing ever sets theDRAINEDtopology role anymore (multigres only emitsPRIMARY/REPLICA/QUARANTINED), so all the code that reacted to it is dead.What's removed (core)
countDrainedPodsand everyeffectiveReplicas = replicas + drainedCountoffset that keyed on it — inreconcile_pool_pods.goand, after rebasing onto the maintenance-surge/disruption features, inmaintenance_surge.goanddisruption.gotoo.effectiveReplicasnow reflects only maintenance surges.syncDrainedLabelsand themultigres.com/role(LabelPodRole) label it maintained, plus theDRAINEDbranch incleanupDrainedPod.DRAINEDshort-circuits inisDrainStale,isPoolHealthy, and the maintenance-surge base-stability check (isPoolHealthystill excludesQUARANTINEDpods).Follow-up cleanup (addressing review)
Shard.Status.PodRolesgodoc + regenerated CRD now listQUARANTINEDinstead ofDRAINED; operator-capability, pod-management-architecture, and pvc-lifecycle docs updated to quarantine remediation / orphan-on-scale-down; API-design changelog entry added. The multiorch "DRAINED Pooler Replacement" narrative inpod-management-design.mdis left in place but marked obsolete / needs-rewrite (out of scope here).DRAINEDand lumped everything else into replicas, soQUARANTINEDpods were counted as replicas — inflating the expected replica count and getting probed as replicas. Now classified separately, excluded from the replica set, and reported with quarantine wording. Adds unit coverage.Behavior
No change to live data-plane paths (rolling updates, scale-down, maintenance surge, quarantine remediation) —
DRAINEDwas never produced once quarantine landed. The one intentional behavioral change is the observer diagnostic fix above. Tests exercising the removed stand-in behavior are deleted or refolded onto the index-vs-replicas semantics that remain.Part of MUL-1009.