Skip to content

Requeue VolumePopulator reconciles while waiting on external state - #2183

Merged
openshift-merge-bot[bot] merged 3 commits into
backube:mainfrom
JohnStrunk:opencode/issue-2178-volumepopulator-rebind-requeue
Oct 27, 2026
Merged

openshift-merge-bot[bot] merged 3 commits into
backube:mainfrom
JohnStrunk:opencode/issue-2178-volumepopulator-rebind-requeue

Conversation

@JohnStrunk

@JohnStrunk JohnStrunk commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes #2178.

A PVC using a ReplicationDestination as its dataSourceRef could stay
Pending forever. The controller creates the temporary vs-prime-<uid> PVC
from the latest snapshot, waits for the prime to bind, and then waits for the
bind controller to release the volume to the target PVC. Every "not ready" path
returned an empty ctrl.Result, so progress depended entirely on watch events
(prime PVC bind, PV bind controller, RD status). A missed event had no recovery:
the prime ended up Bound, the target stayed Pending, and pods mounting it
could not schedule. This was the largest single source of e2e flakes (7 of the
10 job failures between 2026-10-08 and 2026-10-23, across unrelated playbooks).

Changes

  • Bounded self-healing requeue (internal/controller/volumepopulator_controller.go):
    every path that waits on external state now returns
    ctrl.Result{RequeueAfter: 10s} — missing StorageClass,
    WaitForFirstConsumer without the selected-node annotation, missing
    ReplicationDestination, RD without latestImage, prime PVC without a volume,
    PV not found, after patching the PV claimRef, and waiting for the bind
    acknowledgement. Non-retryable paths (in-tree StorageClass, Direct/None copy
    method) and the completed/cleanup path still return an empty result. The
    interval is deliberately fixed rather than exponential backoff (recorded in
    the code comment): these waits target healthy external actors, not failing
    operations; RequeueAfter bypasses controller-runtime's rate limiter; and a
    flat interval keeps the worst-case recovery bounded and predictable.
  • pvcPrime shadowing fix: reconcilePVC() no longer shadows the outer
    pvcPrime when assigning the result of reconcilePVCPrime(). A freshly
    created prime always has no volume, so rebindPVClaim() returns early today
    and the bug is latent; the fix keeps the outer value in sync as
    defense-in-depth so the wait-for-rebind check cannot be bypassed if that early
    return changes.
  • compare_pvc_data diagnostics
    (test-e2e/roles/compare_pvc_data/tasks/main.yml): the compare Job is created
    first — it is the first consumer of the PVCs, which WaitForFirstConsumer
    storage requires before any volume can bind — then the role waits for both
    PVCs to be Bound with a wall-clock deadline of pvc_bound_timeout (default
    300s), then waits for the Job. If a PVC is still unbound at the deadline, the
    role fails with both PVC names and phases, the namespace, and guidance. The
    Job is deleted on success and on every failure path, and log retrieval
    tolerates a Job whose pod never scheduled.
    An earlier revision checked Bound before creating the Job. That deadlocked
    the OpenShift e2e jobs, where the default StorageClass (gp3-csi) is
    WaitForFirstConsumer: 42 playbooks per job failed because the only consumer
    was the compare Job that the check was blocking. Creating the Job first
    restores the previous binding behavior while keeping the bounded diagnostic.
    retries is only a safety cap — now() against a deadline keeps the bound
    honest even when a k8s_info call is slow.
  • Regression tests (internal/controller/volumepopulator_controller_test.go):
    fake-client specs drive the exact stuck sequence (prime created out-of-band →
    prime binds → claimRef patched → bind-controller acknowledgement → cleanup)
    and assert RequeueAfter while waiting. With the requeues removed, 7 of the 8
    new specs fail.

Walkthrough

Why

The rebind depends on several external signals. A dropped event had no recovery:
no requeue was scheduled after creating the prime, after patching claimRef, or
while waiting for the claim release. No SyncPeriod is configured, so informer
resync defaults to 10 hours, and an idle RD emits no events at all.

See it work (disposable Kind cluster, real operator and API server)

A populator PVC whose ReplicationDestination had no latestImage. The operator
re-reconciled every 10 seconds with no object changes:

2026-10-23T21:19:52.542Z  INFO  ReplicationDestination has no latestImage, cannot populate volume yet
2026-10-23T21:19:52.550Z  INFO  ReplicationDestination has no latestImage, cannot populate volume yet
2026-10-23T21:20:02.550Z  INFO  ReplicationDestination has no latestImage, cannot populate volume yet
2026-10-23T21:20:12.552Z  INFO  ReplicationDestination has no latestImage, cannot populate volume yet

A file was written to the source PVC before snapshotting. After
status.latestImage was set once, the populator created the prime PVC, patched
the PV claim, waited for the bind acknowledgement, and cleaned up:

21:20:28.014Z  Creating temp populator pvc from snapshot
21:20:28.064Z  Patching PV claim
21:20:28.070Z  Waiting for pv rebind
21:20:28.073Z  Waiting for pv rebind
21:20:28.081Z  Waiting for pv rebind
21:20:36.007Z  Cleanup - deleting temp volume populator PVC

Data written before the snapshot was readable from the populated PVC:

$ kubectl -n demo-2178 logs job/demo-reader
volpop-demo-1792790382
NAME           PHASE
demo-bound     Bound
demo-dest      Bound     <- populated from the ReplicationDestination
demo-source    Bound
demo-unbound   Pending

Fail-fast guard

The Job is created first, so the guard works for both binding modes. With one
Bound PVC and one deliberately unbound PVC (storageClassName that does not
exist) and pvc_bound_timeout: 5, the play fails after the deadline with both
phases and deletes the compare Job:

Timed out after up to 5s waiting for PVCs to become Bound in namespace wffc-demo:
data-source=Bound, pending-pvc=Pending. The compare Job compare-pvcs-x8rrx cannot
run while these PVCs are unbound. For a VolumePopulator destination PVC, inspect
the VolSync VolumePopulator state; for any PVC, inspect `kubectl describe pvc
<name> -n wffc-demo` (events often show WaitForFirstConsumer or provisioning
errors). Raise pvc_bound_timeout if provisioning is simply slow.

WaitForFirstConsumer reproduction (the OpenShift failure)

Local Kind cluster, destination PVC on a csi-hostpath-wffc StorageClass
(volumeBindingMode: WaitForFirstConsumer) populated by the VolumePopulator,
with the compare role invoked while the destination is still Pending. Because
the Job is created first, its pod is the first consumer and binding proceeds:

17:54:55  VolumeBindingMode is WaitForFirstConsumer, need to wait for nodeName annotation
17:55:05  VolumeBindingMode is WaitForFirstConsumer, need to wait for nodeName annotation
17:55:09  Creating temp populator pvc from snapshot
17:55:09  Patching PV claim
17:55:15  Cleanup - deleting temp volume populator PVC
...
NAME        SC                   PHASE   VOLUME
data-dest   csi-hostpath-wffc    Bound   pvc-96ec34da-...
data-source csi-hostpath-sc      Bound   pvc-3fa0dfb8-...
...
localhost                  : ok=21   changed=2   unreachable=0   failed=0

Evidence and verification

  • go build ./..., go vet ./internal/controller/...
  • golangci-lint run ./internal/controller/... → 0 issues
  • Envtest (go test ./internal/controller -args -ginkgo.focus="VolumePopulator")
    → 35 passed, 0 failed
  • Requeue specs with the requeues removed → 7 failed, 1 passed (the pass is the
    completion path, which needs no requeue)
  • pre-commit run on the changed files (yamllint, markdownlint, whitespace,
    end-of-file) → passed
  • Real e2e on a disposable Kind cluster (hack/setup-kind-cluster.sh, VolSync
    from this tree, hack/run-rustfs.sh):
    • test-e2e/test_simple_rsync.yml → 56 ok, 0 failed
    • test-e2e/test_volumepopulator.yml → 142 ok, 0 failed
    • WFFC populator reproduction (above) → compare passed, 21 ok, 0 failed
    • Fail-fast/cleanup reproduction → precise failure, compare Job deleted
    • No playbook hit the new Bound timeout.
  • The walkthrough was produced and re-verified with Showboat (showboat verify); the only verification diff is the envtest duration line.
  • OpenShift RCA: the first revision of the compare-role change failed all
    three OpenShift e2e jobs (42 playbooks each, data-dest*=Pending). Root cause
    was the pre-Job Bound check deadlocking WaitForFirstConsumer storage
    (gp3-csi): the compare Job was the only consumer, and the check blocked it.
    The controller change was unaffected (its 10s requeue is visible in those logs
    waiting for the selected-node annotation). This revision creates the Job first
    and is validated locally on a WFFC StorageClass; OpenShift CI re-runs on push.

Reproduce and clean up

./hack/setup-kind-cluster.sh && ./hack/run-in-kind.sh && ./hack/run-rustfs.sh
cd test-e2e && pipenv run ansible-playbook test_volumepopulator.yml

The cluster was disposable and was deleted after capture. The live run cannot
force the operator to drop a watch event, so missed-event recovery is proven by
the fake-client regression specs (asserting RequeueAfter and driving the full
sequence); the live cluster shows the requeue cadence and end-to-end population.
To reproduce the OpenShift binding mode locally, use a StorageClass with
volumeBindingMode: WaitForFirstConsumer on the hostpath CSI driver for the
populator destination PVC.

Scope notes

  • test-e2e/roles/compare_pvc_data_block/tasks/main.yml has the same
    Job-mounts-PVCs shape but was intentionally left unchanged; the issue and its
    acceptance criteria target compare_pvc_data.
  • OpenShift's default gp3-csi StorageClass is WaitForFirstConsumer. The
    compare role therefore creates its Job before waiting for binding; this also
    works for Immediate classes, where the wait normally succeeds on the first
    attempt.
  • The 10s requeue also applies to RD/StorageClass waits. A permanently broken
    configuration now emits a warning event per 10s interval (aggregated by the
    events API) instead of only on object changes. This was chosen deliberately
    for a single, predictable recovery bound.

(AI generated)

@openshift-ci

openshift-ci Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnStrunk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@JohnStrunk

Copy link
Copy Markdown
Member Author

/retest

2 similar comments
@JohnStrunk

Copy link
Copy Markdown
Member Author

/retest

@JohnStrunk

Copy link
Copy Markdown
Member Author

/retest

JohnStrunk and others added 3 commits October 27, 2026 14:11
The volume populator creates a prime PVC from the ReplicationDestination's
latest snapshot and then relies on watch events (prime PVC bind, PV bind
controller, RD status) to drive the PV claim rebind. Every "not ready" path
returned an empty result with no requeue, so a missed event left the target PVC
Pending forever. This was the largest single source of e2e flakes.

Return a bounded 10s RequeueAfter from every path that waits on external state:
missing StorageClass, WaitForFirstConsumer without a selected node, missing
ReplicationDestination, RD without latestImage, prime without a volume, missing
PV, after patching the PV claimRef, and waiting for the bind acknowledgement.
Non-retryable and completed paths are unchanged.

Also fix the pvcPrime shadowing in reconcilePVC(). A freshly created prime has
no volume, so rebindPVClaim() currently returns early and the bug is latent;
the fix keeps the outer value in sync as defense-in-depth.

Make compare_pvc_data wait for both PVCs to be Bound with a wall-clock deadline
(pvc_bound_timeout, default 120s), then fail with the PVC names, phases, and
namespace. This keeps normal asynchronous population working while turning a
stuck PVC into a precise failure instead of an opaque ~900s compare-Job wait.

Add fake-client regression specs that drive the out-of-band prime through bind,
claimRef patch, bind acknowledgement, and cleanup, and that pin the requeue for
a missed bind event.

Signed-off-by: John Strunk <jstrunk@redhat.com>
Co-authored-by: OpenCode - DeepSeek V4.1 Flash <noreply@opencode.ai>
RequeueAfter bypasses controller-runtime's rate limiter, and these waits are
for healthy external actors rather than failing operations. Record the
rationale so a future reader weighs the recovery-latency trade-off before
switching to exponential backoff.

Signed-off-by: John Strunk <jstrunk@redhat.com>
Co-authored-by: OpenCode - DeepSeek V4.1 Flash <noreply@opencode.ai>
The previous revision required both PVCs to be Bound before creating the
compare Job. On OpenShift the default StorageClass (gp3-csi) uses
volumeBindingMode: WaitForFirstConsumer, so a PVC cannot bind until a pod
consumes it -- and that pod is the compare Job. The check deadlocked and failed
all three OpenShift e2e jobs (42 playbooks each, data-dest*=Pending).

Create the Job first, then wait for both PVCs to bind with the wall-clock
deadline (pvc_bound_timeout, default 300s, since WFFC plus EBS provisioning is
slower than the old 120s), then wait for the Job. The Job is deleted on success
and on every failure path, and log retrieval tolerates a pod that never
scheduled.

Signed-off-by: John Strunk <jstrunk@redhat.com>
Co-authored-by: OpenCode - DeepSeek V4.1 Flash <noreply@opencode.ai>
@JohnStrunk
JohnStrunk force-pushed the opencode/issue-2178-volumepopulator-rebind-requeue branch from acc950b to a16ca3b Compare October 27, 2026 18:12
@openshift-ci openshift-ci Bot added size/XL and removed size/L labels Oct 27, 2026
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.4%. Comparing base (dd33ebe) to head (a16ca3b).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/controller/volumepopulator_controller.go 90.0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2183   +/-   ##
=====================================
  Coverage   68.4%   68.4%           
=====================================
  Files         61      61           
  Lines       6220    6221    +1     
=====================================
+ Hits        4255    4261    +6     
+ Misses      1645    1641    -4     
+ Partials     320     319    -1     
Files with missing lines Coverage Δ
internal/controller/volumepopulator_controller.go 74.9% <90.0%> (+1.1%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tesshuflower

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Oct 27, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 03d47c1 into backube:main Oct 27, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VolumePopulator can leave a PVC Pending forever when the PV rebind reconcile is missed

2 participants