fix(pvc): converge external populator dependency handoffs - #9
Conversation
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
left a comment
There was a problem hiding this comment.
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+patcherpackages: PASS persistentvolumeclaimsrace suite: PASSgit diff --check: PASS- adversarial post-budget recovery: genuine RED as described above
No runtime/product or release-readiness conclusion is implied.
weicao
left a comment
There was a problem hiding this comment.
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@bf374bcb8268ac2bc44b4b712a71479b89c54321and the configured base diverge6 left / 1 right; - maintained is not an ancestor of this PR head; maintained vs head is
6 left / 6 right; - therefore the current
CLEANstatus 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.
|
The finite-budget event-loss blocker is reproduced and fixed in a focused follow-up commit:
Fresh verification:
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 |
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
SyncerPatcherto the server-returned object/resourceVersion after direct materialization or a fresher host target read;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-gateand preserves the already reviewed exact prerequisite heads:22d640701c1468285571edee50f8ea6775944e53terminating-dependency guard82b48a2ee57b2edc19b4f77769e3a6ac96e2e679effective host StorageClass projection42a462faadada201fec1bd539d83f419e4822f81canonical tests-only dependency-wakeup REDf2206ed0319f9bf519dc4eeeebe85724f5968a78implementation and negative/wiring coverageFinal tree:
f36d7e9d3e0da9406df38e67c95a9655af84abd0.Design contract
82b48a2echart/config/RBAC projection prerequisite;f2206ed0adds no chart or RBAC delta.ModifyController/builder/cache/informer coverage proves both dependency event types reach the exact target.Verification
go test ./pkg/controllers/resources/persistentvolumeclaims -count=1go test -race ./pkg/controllers/resources/persistentvolumeclaims -count=1go test ./pkg/controllers/resources/... -count=1go test ./pkg/patcher ./pkg/controllers/resources/persistentvolumes ./pkg/controllers/resources/storageclasses -count=1go vet ./pkg/controllers/resources/persistentvolumeclaims ./pkg/patcherhelm unittest chart(20 suites, 219 tests)git diff --check