fix(controllers): switch the PDB from maxUnavailable to minAvailable - #351
Open
K.J. Valencik (kjvalencik) wants to merge 1 commit into
Open
fix(controllers): switch the PDB from maxUnavailable to minAvailable#351K.J. Valencik (kjvalencik) wants to merge 1 commit into
K.J. Valencik (kjvalencik) wants to merge 1 commit into
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.github.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
| - **MaxUnavailable**: `(votingMembers - 1) / 2`, integer-divided so the result floors automatically. For 1 voter → 0 (any disruption is quorum loss). For 3 → 1, 4 → 1, 5 → 2, 7 → 3. | ||
| - **MinAvailable**: the quorum (`n/2 + 1`, integer-divided) of `max(votingMembers, status.observed.replicas)`. For 1 voter → 1, 3 → 2, 4 → 3, 5 → 3, 7 → 4. | ||
|
|
||
| Allowed disruptions = healthy voters − `minAvailable`; there is no `expectedCount` term for churn to re-base the budget against. Anchored to `max(live, target)`, the floor holds at the target's quorum while a node rotation shrinks live membership, steps down with the live count during an intentional scale-down (member removal goes through `MemberRemove`, not the eviction API, so the PDB never blocks it), and blocks voter evictions entirely while the cluster is below target (bootstrap, scale-up). Learners are outside the selector and evict freely throughout. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe a quorum floor, not a full-target availability requirement.
minAvailable = quorum(max(live voters, target)) does not block every eviction below target: with target 5 and 4 healthy voters, minAvailable is 3, so one eviction remains allowed. Document that it prevents voluntary disruption below the anchored quorum and that scale-down uses the larger live-voter anchor.
docs/concepts.md#L387-L387: replace “blocks voter evictions entirely while the cluster is below target” with quorum-floor semantics.README.md#L24-L24: mention themax(live voters, intended target)anchor or link to the detailed formula.docs/operations.md#L541-L541: remove the claim that all voter evictions block until membership is whole.
📍 Affects 3 files
docs/concepts.md#L387-L387(this comment)README.md#L24-L24docs/operations.md#L541-L541
🤖 Prompt for 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.
In `@docs/concepts.md` at 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.
Comment on lines
+399
to
402
| - **Scale-up (after promote).** Etcd's `MemberList` reports N+1 voters but `Status.IsVoter` for the freshly-promoted member hasn't been patched yet, so the PDB selects only the N old voter Pods. A drain in this window could evict the unlabelled new voter (no PDB protection) — etcd is left with N voters running of N+1 registered, which an M=N+1-voter cluster (write quorum `⌊M/2⌋+1`) tolerates for any N ≥ 2. That exposure is unchanged from the old budget. The N labelled voters are protected at least as strongly as before: the floor is the quorum of the scale-up *target* (≥ N+1), so `allowed = currentHealthy - minAvailable` in this window is never more permissive than the old `(N-1)/2` budget, and is often 0. | ||
| - **Scale-down (after `MemberRemove`).** Etcd has N-1 voters but the victim's Pod is briefly Terminating. The PDB's own selector still matches the Terminating Pod, but the k8s PDB controller's `currentHealthy` counts only Pods whose `Ready` condition is `True` — once kubelet flips the Terminating Pod's `Ready` to `False` (which happens at the start of graceful shutdown, before the Pod is gone), it stops counting toward the budget's healthy total. Under `minAvailable` that is the whole story: `allowed = currentHealthy - minAvailable`, and — unlike `maxUnavailable` — there is no `expectedCount` for the disappearing member's scale subresource to shrink, so an in-flight removal consumes budget instead of refilling it. | ||
|
|
||
| Both windows are one reconcile cycle wide. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Protect the unlabelled voter during a 1→2 scale-up.
At Line 399, N=1 is outside the stated safety condition: evicting the unlabelled promoted member leaves one running voter in a two-voter membership, below its quorum of two. Ensure the incoming member is PDB-selected before promotion (or prevent this topology), and add a regression case; the floor cannot protect a Pod outside its selector.
🤖 Prompt for 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.
In `@docs/concepts.md` around lines 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.
This was referenced Jul 31, 2026
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
1 participant
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
Bug Fixes
Documentation