Skip to content

fix(pvc): converge external populator dependency handoffs - #9

Open
weicao wants to merge 5 commits into
fix/external-populator-topology-gatefrom
derek/mariadb-r2-external-populator-wakeups
Open

fix(pvc): converge external populator dependency handoffs#9
weicao wants to merge 5 commits into
fix/external-populator-topology-gatefrom
derek/mariadb-r2-external-populator-wakeups

Conversation

@weicao

@weicao weicao commented Jul 24, 2026

Copy link
Copy Markdown

Problem

An external-populator target PVC can reconcile before its guest PV or populate-helper dependency reaches an authoritative state. Later PV/helper cache convergence does not enqueue the target PVC, so the handoff can remain incomplete with timer requeues at zero.

The same reconcile path performs direct optimistic-lock host materialization writes after constructing SyncerPatcher. A later deferred host patch could therefore use the stale pre-write baseline and overwrite another writer or replay stale state.

Change

  • index guest PVCs by exact UID and watch guest PV and PVC dependency events through the real PVC controller builder;
  • map only authoritative namespace/name/UID matches back to the exact external-populator target PVC;
  • distinguish create/update, delete tombstones, terminating objects, absent dependencies, and recreate events without adding a polling timer;
  • propagate mapper lookup failures and fail closed instead of falling back to stale identity;
  • rebase only the host half of SyncerPatcher to the server-returned object/resourceVersion after direct materialization or a fresher host target read;
  • detect Conflict inside Kubernetes aggregate errors and request a bounded fresh reconcile instead of silently overwriting concurrent metadata/resource changes.

NotFound while mapping means skip the current event; the watched authoritative create/recreate event supplies the later wake. A missing host PV in materialization retains the existing scoped 2s convergence result. An invalid rebase object or empty committed resourceVersion is an error.

Commit stack

This PR is cumulative on fix/external-populator-topology-gate and preserves the already reviewed exact prerequisite heads:

  • 22d640701c1468285571edee50f8ea6775944e53 terminating-dependency guard
  • 82b48a2ee57b2edc19b4f77769e3a6ac96e2e679 effective host StorageClass projection
  • 42a462faadada201fec1bd539d83f419e4822f81 canonical tests-only dependency-wakeup RED
  • f2206ed0319f9bf519dc4eeeebe85724f5968a78 implementation and negative/wiring coverage

Final tree: f36d7e9d3e0da9406df38e67c95a9655af84abd0.

Design contract

  1. API/schema: the wakeup implementation adds no public API, CRD, or new configuration surface. This cumulative PR also carries the already accepted 82b48a2e chart/config/RBAC projection prerequisite; f2206ed0 adds no chart or RBAC delta.
  2. Runtime wiring: real ModifyController/builder/cache/informer coverage proves both dependency event types reach the exact target.
  3. Concurrency: direct-write rebase preserves the guest half; a concurrent host writer causes Conflict, remains unmodified, and is recomputed from a fresh second reconcile.
  4. Convergence: canonical PV NotFound→create, helper absent→create, and PV Pending→Bound cases converge with exact target wake and timer0.
  5. Lifecycle: delete/recreate, ordinary Terminating, absent, and NotFound paths are separate and tested.
  6. Failure behavior: lookup and rebase failures fail closed; no stale-baseline fallback.
  7. Compatibility: full resource tree plus PVC/PV/StorageClass and chart suites remain green.
  8. Operations/evidence: source-only change; no image build/push, live mutation, cleanup, or frozen-scene action. Runtime/product N=0 pending the independent Test lane.

Verification

  • focused canonical RED→GREEN, negative mapper, real wiring, class-3 concurrent-writer, and rebase fail-closed tests
  • go test ./pkg/controllers/resources/persistentvolumeclaims -count=1
  • go test -race ./pkg/controllers/resources/persistentvolumeclaims -count=1
  • go test ./pkg/controllers/resources/... -count=1
  • go test ./pkg/patcher ./pkg/controllers/resources/persistentvolumes ./pkg/controllers/resources/storageclasses -count=1
  • go vet ./pkg/controllers/resources/persistentvolumeclaims ./pkg/patcher
  • helm unittest chart (20 suites, 219 tests)
  • git diff --check

wei and others added 5 commits July 22, 2026 16:49
Treat terminating populated PVs, helper PVCs, and selected Nodes as topology-not-ready before external-populator handoff commits. Cover each dependency with focused no-write regressions.
Capture guest PV creation, populate-helper creation, and PV Bound visibility transitions after the target PVC pre-gate returns (nil, false, nil). Require an authoritative mapped target request while preserving zero timer retries.

@weicao weicao left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKER at current head 35909724128e2c57c9f3d3aadd70cafa02649722: the dependency event can still be lost permanently after the finite mapper retry budget.

runExternalPopulatorDependencyRetryWorker calls queue.Forget and returns after five failed mapper attempts (syncer.go:221-239). No exact target request is added to the controller queue. This PR's motivating failure is specifically that later dependency/cache convergence does not enqueue the target and the target has timer requeues at zero, so dropping the only watch event recreates the same indefinite-stall state whenever the lookup outage lasts beyond the finite budget.

The current test at syncer_test.go:1386-1450 locks the drop as expected behavior (targetQueue.Len() == 0) rather than proving recovery. I added a temporary adversarial test with exactly five injected mapper failures and a healthy mapper on the next possible call, without sending a second dependency event. It failed deterministically after 1s: mapper recovered after 5 failures, but the one-shot dependency event was dropped before the exact target was enqueued. The temporary test was removed and the worktree is clean.

Please preserve eventual delivery until manager shutdown, or introduce another bounded mechanism that cannot discard the only event before the exact target is enqueued. Add a regression where mapper recovery occurs just after the current budget and the target is still enqueued exactly once without another PV/PVC event.

Current-head review walk:

  • Pre-flight A/C: current cumulative purpose/head are explicit; evidence below is direct and source/unit scoped only.
  • Class 1: no stale-identity fallback found.
  • Class 2: exact mapper identity checks are enforced, but the eventual-enqueue convergence contract is not enforced at retry exhaustion (blocker above).
  • Class 3: direct host materialization is rebased before deferred patch; concurrent host conflict is retried from fresh state.
  • Classes 4/4a: no new ambiguous sentinel/cross-field token found.
  • Class 5: no cleanup pre-state issue found in this delta.
  • Class 6: retry exhaustion short-circuits the required enqueue side effect (blocker above); ordinary NotFound event handling is otherwise explicit.
  • Class 7: deleting dependency events are separately gated with allowDeletingDependency.
  • Class 8: no unsafe precedence chain found.

Direct verification at exact head:

  • focused dependency/rebase/conflict tests: PASS
  • full persistentvolumeclaims + patcher packages: PASS
  • persistentvolumeclaims race suite: PASS
  • git diff --check: PASS
  • adversarial post-budget recovery: genuine RED as described above

No runtime/product or release-readiness conclusion is implied.

@weicao weicao left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional pre-flight BLOCKER at current head 35909724128e2c57c9f3d3aadd70cafa02649722: the PR has no complete landing path to the maintained branch.

Fresh topology readback:

  • configured base is fix/external-populator-topology-gate@eade1076f84fbd27fd4fb47e1dd3b2e11d42fba9;
  • GitHub has no PR whose head is that base branch;
  • maintained apecloud-v0.35.1@bf374bcb8268ac2bc44b4b712a71479b89c54321 and the configured base diverge 6 left / 1 right;
  • maintained is not an ancestor of this PR head; maintained vs head is 6 left / 6 right;
  • therefore the current CLEAN status proves only that the PR can merge into the unlanded base branch. It does not prove that the cumulative change can reach the maintained line.

Directly retargeting is not review-equivalent: the maintained-base delta expands to 15 files and +4060/-55, including the vendored node-affinity helper, while the current-base delta is already 13 files and +2732/-70. That scope must be made deliberate and reviewed; it cannot be hidden behind the dependency-wakeup title.

Please establish one exact landing chain: either (a) open and land the prerequisite base against apecloud-v0.35.1, then keep this PR stacked on that reviewed prerequisite, or (b) rebuild/retarget this PR as an explicit maintained-base cumulative stack and update purpose/body/tests to the full delta. Do not simply retarget without a fresh scope and contract review.

Pre-flight A/C is blocked on source integration topology. The previously reported finite-retry event-loss blocker at this same head remains independently open. This is source-review evidence only; runtime/product N=0 and no release conclusion is implied.

@weicao

weicao commented Jul 29, 2026

Copy link
Copy Markdown
Author

The finite-budget event-loss blocker is reproduced and fixed in a focused follow-up commit:

  • candidate: bf83e69e34f23e5d8e554f9e1c2856d0d83e3662 (tree d5343bd683c0602afee10f037cca73275f7432a8), parent exact PR head 35909724128e2c57c9f3d3aadd70cafa02649722
  • branch: julie/pr9-finite-retry
  • behavior: when one exponential retry burst is exhausted, reset the rate-limit history and requeue the same dependency item instead of permanently dropping the only event
  • permanent regression: mapper fails through the full budget, then recovers without a second dependency event; the exact target must still be enqueued

Fresh verification:

  • focused recovery tests: PASS
  • go test ./pkg/controllers/resources/persistentvolumeclaims -count=1: PASS
  • go test -race ./pkg/controllers/resources/persistentvolumeclaims -count=1: PASS

This closes only the finite-budget control-plane/source blocker on the follow-up commit. The PR's configured base is still not landed in apecloud-v0.35.1, so the independent integration/base blocker remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant