fix(dataprotection): unblock ordered restore after PVC initial step - #10479
fix(dataprotection): unblock ordered restore after PVC initial step#10479weicao wants to merge 4 commits into
Conversation
|
Auto Cherry-pick Instructions CLA Recheck Instructions |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10479 +/- ##
==========================================
- Coverage 62.13% 62.09% -0.04%
==========================================
Files 533 533
Lines 63861 64055 +194
==========================================
+ Hits 39682 39777 +95
- Misses 20566 20641 +75
- Partials 3613 3637 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if err := r.updatePVCPopulatingCondition(reqCtx, pvc, reason, message); err != nil { | ||
| return err | ||
| } | ||
| if err := r.updatePVCRestoreCondition(reqCtx, pvc, reason, message); err != nil { |
There was a problem hiding this comment.
[P1] This marks the PVC Restore condition True as soon as the PV has been rebound/released, before ensurePostReadyRestoreCompleted creates or finishes the postReady Restore. The same PVC condition is aggregated by InstanceSet, then Component, then Cluster, and those layers treat True/False as terminal with early returns. In the provision-only/postReady case this can make the upper restore condition permanently True while postReady is still pending or later fails, so users can observe a false restore success. Keep the workload-unblock signal separate from the restore-complete contract, or only set Restore=True after postReady has completed.
Keep PVC Restore=True reserved for full restore completion after postReady actions finish. When a VolumePopulator restore PVC finishes its prepare/provision step but postReady still needs later component PVCs, publish the existing Populating=True initial-step signal instead. Teach ordered component and InstanceSet creation to consume that non-terminal Populating signal for restore PVCs, so later ordinals/components can be created without marking the restore terminally successful. Fixes #10478
e892d71 to
22ba22e
Compare
weicao
changed the title
|
Updated head to 22ba22e. The early terminal PVC restore condition is removed: PVC Restore=True is only written by the final UpdatePVCConditions path after postReady completes. The ordered-unblock path now uses the non-terminal PVC Populating=True signal with reason Succeed/Provisioned, and both InstanceSet and Cluster ordered creation consume that signal only while the predecessor component is still Restore=Unknown/RestoreRunning. Local tests run:
|
leon-ape
left a comment
leon-ape
left a comment
There was a problem hiding this comment.
No blocking findings on the current head.
Dismissed because kb controller PR reviews should report conclusions only and must not be approved directly from this workflow.
|
|
… restores The ordered-creation bypass introduced for restore PVCs had two gaps: - A PVC keeps Populating=True after the full restore completes (the condition is never removed), so the InstanceSet-level bypass stayed active after Restore=True landed, weakening OrderedReady semantics until the VCT restore annotations were cleaned up asynchronously by another controller. Treat any terminal Restore condition (True or False) as out of the initial-step window in both consumers. - The Cluster-level gate counted restore-annotated PVCs by component labels, so leftover PVCs (e.g. retained from a previous incarnation) could substitute for expected ones that do not exist yet. Resolve the exact expected PVC names from the predecessor's InstanceSet spec via the shared pod-name builder and require every named PVC to pass, cross-checked against the component-spec count so a stale InstanceSet spec falls back to the strict phase gate. Also make MockReader.Get type-aware since a Component and its InstanceSet share the same object key.
9c3294b to
366994e
Compare
The v2 instanceset alignment reconciler was missing the restore PVC initial-step bypass that v1 already had. During ordered-ready restore, the predecessor instance may not be available because its PVC is being populated, blocking creation of subsequent instances. The fix uses a two-layer approach: 1. Instance controller status reconciler checks PVC Populating conditions and sets an annotation when all restore PVCs complete their initial step. 2. InstanceSet v2 alignment reconciler reads this annotation to allow creating the next instance even when the predecessor is not yet available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
size/XXL
Update: v2 instanceset sync (commit 84f8782)What changedAdded Files:
Why this approachv2 InstanceSet tree only contains Instance objects (no PVCs), unlike v1 which has PVCs in the tree. Solution: Instance controller checks its PVCs and sets an annotation; InstanceSet v2 reads the annotation. How verified
|
|
Re: Loki review — the v1/v2 instanceset sync gap has been addressed in the current head ( TiDB exact-head runtime evidence is still pending — will follow up. |
|
PR body updated: added Scope section with v1/v2 layer enumeration table, v2 architecture note, bypass condition specification, test coverage table, and remaining gate. |
|
@wangyelei @ldming could one of you please take the maintainer review for the current head?\n\nCurrent state:\n- head: |
leon-ape
left a comment
leon-ape
left a comment
There was a problem hiding this comment.
[P1] Do not derive a cross-controller ordered-unblock contract from DataProtection implementation details
The public InstanceSet contract says OrderedReady waits for each predecessor Pod to become ready. This PR silently overrides that contract by teaching workload and cluster controllers to interpret Populating=True with the Succeed or Provisioned reason as a new restore lifecycle state. The same meaning is reconstructed in three places, and upper layers now need to know DataProtection reason strings, PVC naming, and InstanceTemplate storage details.
This is a module-boundary and API consistency issue. A non-terminal state that is authoritative for ordered unblock must be explicitly defined by the DP/workload API and aggregated through stable status, rather than inferred from internal condition reasons and annotations.
[P1] Compressed InstanceTemplates still fall back to the original deadlock
expectedRestorePVCNamesForComponent builds InstanceSetExt with an empty ObjectTree. Compressed templates are stored in a ConfigMap and cannot be resolved from that tree; the resulting name set is incomplete and the count check deliberately falls back to the strict component phase gate. For a legal component using compressed templates, the predecessor still waits for later PVCs while the later component waits for the predecessor to become Running, so the deadlock this PR targets remains.
[P1] The v2 annotation does not preserve the terminal-state invariant
v1 and Cluster read current PVC conditions and immediately reject the bypass after Restore becomes True or False. v2 reads only an asynchronously maintained boolean Instance annotation. After initial-step success, a later postReady failure can make the PVC terminal while the annotation remains true until another Instance reconciliation removes it; during that window, or during controller restart/failure, InstanceSet v2 can still create the successor. The PR claim that any terminal Restore condition disqualifies the bypass is therefore not true for v2, and the v1/v2 semantics are not equivalent.
4 participants
What changed
This keeps PVC
Restore=Truereserved for full restore completion after postReady finishes. When VolumePopulator has already finished the PVC prepare/provision step but postReady still needs later component PVCs, the PVC exposes the existing non-terminalPopulating=Truesignal with reasonSucceedorProvisioned.Ordered workload creation now consumes that initial-step signal for restore PVCs:
Restore=Unknown/RestoreRunning, as long as all expected restore PVCs for that predecessor have the initial-step signal.Scope
This PR touches three ordered-creation layers. Each layer has a v1 and v2 code path:
pkg/controller/instanceset/reconciler_instance_alignment.go—restorePVCInitialStepCompletedForPodreads PVC conditions directlypkg/controller/instanceset2/reconciler_alignment.go—restorePVCInitialStepCompletedForInstancereads Instance annotationPopulating=True(Succeed/Provisioned)pkg/controller/instance/reconciler_status.go—reconcileRestorePVCAnnotationchecks PVCs, setsRestorePVCInitialStepCompletedAnnotationKeyon InstancePopulating=True-> Instance annotationcontrollers/apps/cluster/transformer_cluster_component.go—restorePVCInitialStepCompletedreads PVC conditions via MockReaderPopulating=True(Succeed/Provisioned)v2 architecture note
v2 InstanceSet tree only contains Instance + Service objects (no PVCs). PVCs belong to the Instance subtree. Therefore the v2 bypass uses a two-layer annotation propagation: Instance controller checks its PVCs and sets annotation -> InstanceSet v2 alignment reads the annotation on predecessor Instance.
Bypass condition (all layers)
The bypass is disqualified when a terminal PVC
Restorecondition (True or False) exists — this scopes it to in-progress restores only.Why
A TiDB PITR restore can use a logical Backup whose target selector points at the
tidbcomponent while data PVCs belong totidb-pd/ other components. Those PVCs may be provision-only until postReady redirects to the TiDB component. If ordered creation waits for terminal component/pod readiness, DP waits for later component PVCs while those later pods/components are not created yet.The earlier version of this PR used early
Restore=True/Provisioned, but review correctly pointed out that this made Cluster/Component/InstanceSet restore state terminally successful before the postReady Restore finished. This version separates the contracts:Populating=Trueis the PVC initial-step/unblock signal;Restore=Trueremains the terminal restore-complete signal.Fixes #10478.
Related to #10393 and #10466.
Tests
go test ./pkg/controller/instanceset -count=1go test ./pkg/controller/instance/... -count=1go test ./pkg/controller/instanceset2/... -count=1KUBEBUILDER_ASSETS="..." go test ./controllers/dataprotection -count=1KUBEBUILDER_ASSETS="..." go test ./controllers/apps/cluster -count=1git diff --checkTest coverage
pkg/controller/instancerestorePVCInitialStepCompletedpkg/controller/instancereconcileRestorePVCAnnotationpkg/controller/instanceset2restorePVCInitialStepCompletedForInstanceFollow-up hardening (commit 9c3294b)
Restorecondition (True or False) now disqualifies the bypass in both the InstanceSet and Cluster consumers. PreviouslyPopulating=Truepersisted after postReady completion, keeping the bypass active until the VCT restore annotations were cleaned up asynchronously.ComposePVCName) instead of counting label-matched PVCs, so leftover PVCs from a previous incarnation cannot substitute for expected ones. The component-spec count cross-check makes a stale InstanceSet spec fall back to the strict phase gate.MockReader.Getnow matches the concrete object type, since a Component and its InstanceSet share the same key.Remaining gate