operator: exempt stuck pods' claims from the PVCUnbinder pvc-rebinding gate#1658
Open
RafalKorepta wants to merge 1 commit into
Open
operator: exempt stuck pods' claims from the PVCUnbinder pvc-rebinding gate#1658RafalKorepta wants to merge 1 commit into
RafalKorepta wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
hidalgopl
reviewed
Jul 13, 2026
| // | ||
| // Names are unambiguous without a namespace: both this scan and | ||
| // [Controller.listClusterPVCsByName] are scoped to pod.Namespace. | ||
| func (r *Controller) stuckClaimNames(ctx context.Context, pod *corev1.Pod) (map[string]struct{}, error) { |
Contributor
There was a problem hiding this comment.
I'm having a hard time understanding what this function does:
- We get all PVCs for given
podargument (regardless of their status / being stuck) (fromStsPVCs(pod)) - Then we list all the pods for given Redpanda cluster and we add all stuck PVCs for these pods?
Is this correct?
…g gate
Gate 3 (pvc-rebinding) defers any unbind while a claim in the cluster
is unbound, assuming an in-progress rebind that will settle shortly.
That assumption is false for claims owned by Pods that are themselves
stuck Pending on a scheduling failure, and deadlocks the unbinder in a
three-way circular wait:
1. Gate 3 defers because a stuck Pod's datadir claim has no
volumeName,
2. that claim can never bind until its Pod schedules
(WaitForFirstConsumer), and
3. the Pod can never schedule until the unbinder frees its
mis-pinned sibling claim (e.g. a shadow-index-cache PV
provisioned onto an already-occupied node) — the very action
Gate 3 is deferring.
Hit in production by a fresh AWS cluster that never bootstrapped:
broker-1's shadow-index-cache PV landed on broker-0's node, the pod
could never fit there ("didn't have free ports") nor anywhere else
("didn't match PersistentVolume's node affinity"), and the unbinder
logged "a PVC in this cluster has no volumeName yet; deferring" every
30s for hours while the remaining brokers looped in cluster discovery
waiting for the missing seed.
Exempt claims owned by Pods stuck Pending with the unbinder's
scheduling-failure signature (schedulingFailureRE, the same weak form
ShouldRemediate accepts), the reconciled Pod first among them — the
same reasoning as Gate 0's own-claim exemption (inFlightClaimOwnedBy).
The exemption is symmetric across victims so two simultaneously
mis-pinned brokers don't mutually defer on each other's unbound
claims; destructive work stays serialized by Gate 0. The weak
signature deliberately matches any total scheduling failure (modern
schedulers don't reliably name volume affinity), so a mid-rebind Pod
can also be exempted; that is safe with rebinding off because
unbinder-initiated rebinds still defer siblings through Gate 0's
uncached settle check, the destructive pipeline only touches the
reconciled Pod's own claims, and no Available PV is ever produced.
Orphaned claims with no Pod keep deferring.
Under the deprecated --allow-pv-rebinding flag no exemption applies:
that mode floats freed PVs as live binding candidates (see
FreedPVAnnotation / INC-2818), where acting while any claim is unbound
could pair it with a disk it was never meant to hold, so the original
conservative deferral is kept there.
Also name the gating claim in the Gate 3 log line and Event (the
production incident took hours to attribute partly because the log
never said WHICH claim was unbound), update the stale
PVCUnbinderGateDeferred metric doc, and fix the withPVC test fixture
to produce unique volume names.
Known pre-existing limitations, out of scope here: an orphaned
never-bound claim (aborted scale-up, then scale-in) still defers all
unbinds in its cluster, and the same mis-provisioning class deadlocks
at Gate 2 instead when two victims' PVs land on two different occupied
nodes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
941b6e1 to
bf5d857
Compare
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 PVCUnbinder's Gate 3 (
pvc-rebinding) defers any unbind while any claim in the cluster has an emptyspec.volumeName, assuming an in-progress rebind that will settle shortly. That assumption is false for claims owned by Pods that are themselves stuck, producing a three-way circular wait that deadlocks the unbinder:volumeName,WaitForFirstConsumer), andProduction impact (cluster
d99mur9p5l53r2240m3g, 2026-07-12): a fresh AWS cluster never bootstrapped. broker-1'sshadow-index-cachePV was provisioned onto broker-0's node; the pod could never fit there (didn't have free ports) nor anywhere else (didn't match PersistentVolume's node affinity). The unbinder loggeda PVC in this cluster has no volumeName yet; deferringevery 30s for ~2h while brokers 0/2 looped incluster_discoverywaiting for the missing seed. Manual PVC deletion was required.Fix
Exempt claims owned by Pods stuck Pending with the unbinder's scheduling-failure signature (
schedulingFailureRE— the same weak formShouldRemediateaccepts), the reconciled Pod first among them. Same reasoning as Gate 0's own-claim exemption (inFlightClaimOwnedBy), which exists for exactly this deadlock class.0/N nodes are available:and don't reliably name volume affinity, so a mid-rebind Pod can also be exempted. Safe with rebinding off: unbinder-initiated rebinds still defer siblings through Gate 0's uncached settle check, the destructive pipeline only touches the reconciled Pod's own claims, and no Available PV is ever produced for a floating claim to mis-pair with.--allow-pv-rebindingkeeps the old conservative behavior (no exemption): that mode floats freed PVs as live binding candidates (seeFreedPVAnnotation/ INC-2818), where acting while any claim is unbound could pair it with a disk it was never meant to hold.Also:
PVCUnbinderGateDeferredmetric doc comment.withPVCtest fixture to produce unique volume names (previously two calls produced duplicatename: datavolumes — invalid against a real API server).Testing
TDD; each new subtest was watched failing against the old code first:
own unbound WFFC claim does not deadlock the unbind— the production scenario end-to-end throughReconcile(mis-pinned bound claim deleted, PV Retain + in-flight annotations, pod deleted, unbound claim untouched).two stuck pods with unbound WFFC claims do not mutually deadlock— the ≥2-victim variant.allow-pv-rebinding keeps the conservative deferral even for own claims.sibling stuck on a non-affinity total scheduling failure is also exempted (weak signature)— pins the deliberate breadth.sibling unbound claim still defers— pins serialization, and asserts viaevents.FakeRecorderthat thepvc-rebindinggate specifically fired and the event names the gating claim.go test ./internal/controller/pvcunbinder/ ./internal/observability/green;gofmt/go vetclean.The change was additionally put through a multi-agent adversarial review (10 finder angles → per-finding verification → sweep); all confirmed correctness findings are addressed here or listed below.
Known pre-existing limitations (out of scope, surfaced by the review)
🤖 Generated with Claude Code