fix(controllers): switch the PDB from maxUnavailable to minAvailable - #351
Merged
Timofei Larkin (lllamnyp) merged 2 commits intoAug 3, 2026
Merged
Conversation
The generated PodDisruptionBudget set maxUnavailable = (voters-1)/2 over
the role=voter pods. That form re-bases under churn: the disruption
controller computes allowed = currentHealthy - (expectedCount -
maxUnavailable), and expectedCount is derived from the EtcdMember /scale
subresources of the currently-matching pods. During a node rotation each
removed member shrank expectedCount, refilling the budget mid-drain — a
3-of-5 production cluster was legally evicted down to 2-of-2 quorum for
~36 minutes (2026-07-30 incident).
An integer minAvailable has no expectedCount term (allowed =
currentHealthy - minAvailable), so the floor cannot move as membership
shrinks. The floor is the quorum (n/2+1) of max(live voters,
status.observed.replicas):
- Steady state: identical disruptions to the old budget, since
n - (n-1)/2 = n/2 + 1.
- Unplanned churn: the latched target holds the floor at steady-state
quorum while the operator refills membership; this would have
clamped the incident.
- Intentional scale-down: the live count dominates and steps the
floor down as members are removed via MemberRemove (not the
eviction API), so a 5->3 shrink never wedges drains.
- Bootstrap/scale-up: the target dominates and voter evictions block
until the cluster reaches size; learners are outside the selector
and unaffected.
The update path now also migrates PDBs left by previous versions:
maxUnavailable is cleared in the same patch that sets minAvailable
(a PDB with both fields is invalid). Delete-at-zero-voters, the
role=voter selector, and additionalMetadata merging are unchanged;
no API surface is added.
The EtcdMember /scale contract also stays, but its comments are
corrected: they claimed the PDB controller requires /scale and goes
SyncFailed without it, which is only true of maxUnavailable and
percentage minAvailable budgets — an integer minAvailable takes
expectedCount = len(selected pods) and never resolves scale. The
subresource is kept for any user-created budget over member Pods that
does resolve scale.
Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
K.J. Valencik (kjvalencik)
requested review from
Andrey Kolkov (androndo) and
Timofei Larkin (lllamnyp)
as code owners
July 30, 2026 21:32
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
api-change
controllers
documentation
Contributor
📝 WalkthroughWalkthroughThe operator now manages PodDisruptionBudgets with quorum-based ChangesPodDisruptionBudget quorum behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EtcdClusterController
participant ClusterStatus
participant PodDisruptionBudget
EtcdClusterController->>ClusterStatus: read observed replicas
EtcdClusterController->>EtcdClusterController: calculate quorum MinAvailable
EtcdClusterController->>PodDisruptionBudget: create or patch MinAvailable
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts.md`:
- Line 387: Update the quorum-floor documentation: in docs/concepts.md:387-387,
replace the claim that voter evictions are entirely blocked below target with
the behavior that voluntary disruptions are prevented only below quorum(max(live
voters, target)), while scale-down follows the larger live-voter anchor; in
README.md:24-24, mention the max(live voters, intended target) anchor or link to
the detailed formula; in docs/operations.md:541-541, remove the claim that all
voter evictions are blocked until membership is whole.
- Around line 399-402: The scale-up safety discussion around “Scale-up (after
promote)” incorrectly permits N=1, where evicting the unlabelled promoted voter
violates quorum. Update the implementation or documented topology to ensure the
incoming voter is PDB-selected before promotion, or explicitly prevent 1→2
scale-ups, and add a regression case covering this scenario; do not rely on the
quorum floor to protect Pods outside the selector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ebde05c-b355-4ae6-8e5f-48e83b229e54
📒 Files selected for processing (8)
README.mdapi/v1alpha2/etcdmember_types.gocharts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yamlcontrollers/etcdcluster_controller.gocontrollers/etcdcluster_controller_test.gocontrollers/etcdmember_controller.godocs/concepts.mddocs/operations.md
This was referenced Jul 31, 2026
Two descriptions of the new minAvailable budget were inaccurate. The migration tool still told users their dropped spec.podDisruptionBudgetTemplate would be replaced by a PDB with maxUnavailable=(voters-1)/2, a shape the operator no longer emits. Describe the emitted budget instead. The kitchen-sink warning assertion matched only the "spec.podDisruptionBudgetTemplate" prefix, so it passed against the stale text; it now pins the full sentence. The docs claimed voter evictions "block entirely" while the cluster is below target. Allowed disruptions are healthy - (target/2 + 1), so being below target is not sufficient: at target 5 with 4 healthy voters one eviction is still allowed, at target 7 with 6 healthy two are. The claim holds only for a 3-member target, where any shortfall does reach the floor. Reword concepts.md and operations.md to state the actual condition — evictions block once healthy voters fall to floor(target/2)+1 — and add table rows to TestPDBMinAvailable pinning the anchors behind those numbers. README dropped the max(live, target) half of the formula, which is the branch that keeps a 5->3 scale-down from wedging drains; restore it. Also rename the local min to minAvail in reconcilePDB so it stops shadowing the builtin nine lines below a use of the builtin max. No behaviour change: pdbMinAvailable and reconcilePDB are untouched apart from the rename. Assisted-By: Claude Opus 5 Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/etcdcluster_controller_test.go`:
- Around line 3188-3190: Update the comment near the healthy-voter disruption
test to narrow the target range from “targets > 3” to “targets >= 5,” or
explicitly state that only some larger targets permit a one-voter shortfall;
keep the quorum and disruption behavior description accurate.
In `@internal/migrate/translate.go`:
- Line 106: Update the migration warning in the translation logic to describe
the operator’s target-based PDB floor: quorum is calculated from max(live
voters, status.observed.replicas), and the PDB is deleted when no voters remain.
Update the matching expected warning in the relevant translate test to reflect
this wording and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f612fa5-c539-48f2-bc28-825b90ac1b30
📒 Files selected for processing (7)
README.mdcontrollers/etcdcluster_controller.gocontrollers/etcdcluster_controller_test.godocs/concepts.mddocs/operations.mdinternal/migrate/translate.gointernal/migrate/translate_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/operations.md
- docs/concepts.md
- README.md
- controllers/etcdcluster_controller.go
Timofei Larkin (lllamnyp)
approved these changes
Aug 3, 2026
Member
|
K.J. Valencik (@kjvalencik) thanks for your work on this, merged. |
K.J. Valencik (kjvalencik)
added a commit
to kjvalencik/etcd-operator
that referenced
this pull request
Aug 3, 2026
The crash-loop self-heal (cozystack#336) excluded memory-medium members on the theory that the pod-loss self-heal already covers them. That check only fires when the Pod is gone or replaced (Status.PodUID mismatch); a memory member whose Pod is alive but whose etcd can never start keeps the same Pod UID forever and is invisible to it. The live failure mode: a replacement learner is created with --initial-cluster baked into its immutable Pod spec; if membership changes again before the learner's first successful boot, etcd fatals with "error validating peerURLs ...: member count is unequal" on every restart. With max-learners=1 the wedged learner also blocks all further member replacement, so the cluster stays degraded until a human (or a node rotation that happens to kill the Pod) intervenes. Drop the medium exclusion. Every other guard stays: non-bootstrap, etcdContainerStuck (not ready, restart threshold, not OOMKilled, Pod not terminating) and the quorum gate. Replacing a stuck memory member is strictly milder than the already-covered PVC case — its data dies with the Pod anyway. Related: cozystack#351 Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
K.J. Valencik (kjvalencik)
added a commit
to kjvalencik/etcd-operator
that referenced
this pull request
Aug 3, 2026
The crash-loop self-heal (cozystack#336) excluded memory-medium members on the theory that the pod-loss self-heal already covers them. That check only fires when the Pod is gone or replaced (Status.PodUID mismatch); a memory member whose Pod is alive but whose etcd can never start keeps the same Pod UID forever and is invisible to it. The live failure mode: a replacement learner is created with --initial-cluster baked into its immutable Pod spec; if membership changes again before the learner's first successful boot, etcd fatals with "error validating peerURLs ...: member count is unequal" on every restart. With max-learners=1 the wedged learner also blocks all further member replacement, so the cluster stays degraded until a human (or a node rotation that happens to kill the Pod) intervenes. Drop the medium exclusion. Every other guard stays: non-bootstrap, etcdContainerStuck (not ready, restart threshold, not OOMKilled, Pod not terminating) and the quorum gate. Replacing a stuck memory member is strictly milder than the already-covered PVC case — its data dies with the Pod anyway. Related: cozystack#351 Signed-off-by: K.J. Valencik <kjvalencik@gmail.com>
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.
Production impact
During a node rotation (Karpenter), a healthy 5-voter cluster was drained to 2 voters — below its steady-state quorum of 3 — for ~36 minutes. The budget was
maxUnavailable: 2, which should have prevented this, but it re-bases under churn:allowed = currentHealthy - (expectedCount - maxUnavailable), whereexpectedCountderives from the /scale subresources of the currently matching pods. Each removed member shrankexpectedCountand refilled the budget mid-drain (evicted-but-terminating voters also briefly kept counting as healthy). No single eviction violated the budget, yet the cluster ended up at 2-of-2.The window stayed open that long because recovery didn't converge on its own: a replacement pod crashlooped, having been created with a static config that expected a number of voters that had since shrunk.
Why minAvailable, and why the target anchors it
An integer
minAvailablehas noexpectedCountterm —allowed = currentHealthy - minAvailable— so the floor cannot re-base as membership shrinks. The floor is the quorum (n/2 + 1) ofmax(live voters, status.observed.replicas):n - (n-1)/2 = n/2 + 1).MemberRemove, not the eviction API) — a 5→3 shrink never wedges drains.Existing PDBs are migrated in place: the reconciler clears
maxUnavailablein the same patch that setsminAvailable(a PDB with both is invalid). No API change; selector, delete-at-zero-voters, additionalMetadata, and the /scale contract are untouched.Future direction
This is deliberately the least impactful fix: no new API, steady-state semantics unchanged. What I'd actually like is to allow only a single voter to be replaced at a time regardless of cluster size (
minAvailable = n - 1) — rotations replace nodes serially anyway, and quorum is the emergency floor, not a comfortable operating point. That needs configuration and more complex code, so it's left to a future improvement. Feedback on that design is welcome.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Documentation