docs: kubevirt-datamover design doc — 2026-08-06 implementation status - #2360
Draft
kaovilai wants to merge 18 commits into
Draft
docs: kubevirt-datamover design doc — 2026-08-06 implementation status#2360kaovilai wants to merge 18 commits into
kaovilai wants to merge 18 commits into
Conversation
…tion status Add implementation status snapshot sourced from kdm-controller, kdm-plugin, and oadp-e2e peer sessions covering reconciler state machines, checkpoint chain rebase/restore, VMB/VMBT lifecycle, PVC sizing, plugin registration, and current e2e coverage/gaps. Mark PVC sizing, VMBackup deletion timing, and force-full-backup open questions as resolved with pointers to the new status section. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe design document updates implementation status, VM run-state restoration, PVC sizing, VMB/VMBT lifecycle behavior, disk conversion, checkpoint handling, and E2E coverage. ChangesKubeVirt datamover design
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/kubevirt-datamover.md`:
- Line 273: Update the open-question text in the VirtualMachineBackup cleanup
section to remove the stale claim that the controller always deletes VMBs.
Describe the split ownership: the uploader pod deletes the VMB on successful
completion, while the controller deletes it when cancellation occurs; retain the
statement that VMBT is not deleted.
- Around line 257-259: Update the E2E coverage section to distinguish results
verified in the current session from those validated in a prior session.
Separate the generic CSI-datamover coverage from the CBT-specific coverage,
citing the corresponding test entries at lines 399-409 and 510-519, and ensure
the heading does not imply prior-session results were verified today.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e3f5f8e-e91e-4b6c-9788-c53795a862bc
📒 Files selected for processing (1)
docs/design/kubevirt-datamover.md
…sk resume gating Both findings verified against actual code by kdm-controller/kdm-plugin peer agents rather than guessed: - VMB is orphaned on genuine Failed (non-canceled) DataUpload; issue #12 closed but only delivered the success-path half. - VM RIA resume gating counts only currently-discovered DataDownloads, not the VM's full expected volume count; accepted single-disk-only scope boundary for #124/#44, dormant since multi-disk restore itself is blocked on controller#73 phase4. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…tnote Backup/restore design sections (VirtualMachine RIA plugin, DataDownload reconciler) previously only described pre-#124/#44 behavior. Add the halt-at-restore/resume-on-siblings-complete mechanism to the actual design prose, including the multi-disk scope boundary, and trim the now-duplicated description out of the Implementation status section. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/design/kubevirt-datamover.md (2)
261-261: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the blocker for each E2E gap separately.
Assign multi-PVC restore to phase 4. Assign incremental CBT restore to phase 5.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/kubevirt-datamover.md` at line 261, Update the “Known gaps” entry to list multi-PVC restore from a CBT backup and restore from an incremental CBT backup as separate E2E gaps, assigning the former to kubevirt-datamover-controller#73 phase 4 and the latter to phase 5; keep the maxIncrementalBackups=0 checkpoint-delete blocker tied to CNV-85377.
251-251: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the actual
AnnotationVMNamekey.AnnotationVMNameiskubevirt-datamover.io/vm-name, notkubevirt-datamover-vm. PVC BIA and PVC RIA use the same key. Update the design references at Lines 65 and 71.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/kubevirt-datamover.md` at line 251, Update the design references at lines 65 and 71 to document the actual AnnotationVMName key, kubevirt-datamover.io/vm-name, consistently for both PVC BIA and PVC RIA.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/design/kubevirt-datamover.md`:
- Line 261: Update the “Known gaps” entry to list multi-PVC restore from a CBT
backup and restore from an incremental CBT backup as separate E2E gaps,
assigning the former to kubevirt-datamover-controller#73 phase 4 and the latter
to phase 5; keep the maxIncrementalBackups=0 checkpoint-delete blocker tied to
CNV-85377.
- Line 251: Update the design references at lines 65 and 71 to document the
actual AnnotationVMName key, kubevirt-datamover.io/vm-name, consistently for
both PVC BIA and PVC RIA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ddabf45-440b-485c-8378-3426b10a5e74
📒 Files selected for processing (1)
docs/design/kubevirt-datamover.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/kubevirt-datamover.md (1)
243-244: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the
Canceling -> Canceledtransition.The DataDownload CRD defines both phases, but this lifecycle ends at
Canceling. Add the terminalCanceledstate and its transition so the design matches the status contract.Proposed wording
- - DataUpload/DataDownload reconcilers implement `New -> Accepted -> Prepared -> InProgress -> Completed/Failed/Canceling`, with `Spec.Cancel` handled at any non-terminal phase. + - DataUpload/DataDownload reconcilers implement `New -> Accepted -> Prepared -> InProgress -> Completed/Failed`, with `Spec.Cancel` handled at any non-terminal phase and transitioning through `Canceling -> Canceled`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/kubevirt-datamover.md` around lines 243 - 244, Update the DataUpload/DataDownload lifecycle description to include the terminal Canceled phase and explicitly document the Canceling -> Canceled transition, matching the DataDownload CRD status contract.
🧹 Nitpick comments (1)
docs/design/kubevirt-datamover.md (1)
100-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDefine behavior for failed or canceled sibling restores.
The trigger is defined only for
Completed, butDataDownloadalso hasFailedandCanceledterminal phases. State whether the VM remains halted when any sibling fails or is canceled, and how recovery proceeds. This prevents an undocumented partial-restore state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/kubevirt-datamover.md` at line 100, Update the DataDownload completion trigger description to define behavior when a correlated sibling reaches the Failed or Canceled terminal phase. Specify whether the VM remains halted and how recovery proceeds, including the final state or operator action for partial restores.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/design/kubevirt-datamover.md`:
- Around line 243-244: Update the DataUpload/DataDownload lifecycle description
to include the terminal Canceled phase and explicitly document the Canceling ->
Canceled transition, matching the DataDownload CRD status contract.
---
Nitpick comments:
In `@docs/design/kubevirt-datamover.md`:
- Line 100: Update the DataDownload completion trigger description to define
behavior when a correlated sibling reaches the Failed or Canceled terminal
phase. Specify whether the VM remains halted and how recovery proceeds,
including the final state or operator action for partial restores.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d22926e7-7bf6-466f-8d31-8221066ddf43
📒 Files selected for processing (1)
docs/design/kubevirt-datamover.md
Per feedback, restructure so implementation facts live inline in the sections they describe (BIA/RIA plugins, controller reconcilers) rather than in a separate 'Implementation status' dump that duplicated them. Move E2E coverage to its own subsection since it has no other home. Also address two more coderabbit findings, verified against real behavior by kdm-controller/kdm-plugin rather than guessed: - Multi-disk resume-gating: state as an explicit phase4 design requirement (must reject/hold, not resume opportunistically) rather than just a noted race. - Terminal-failure handling for the VM run-state-restore path: a Failed/Canceled sibling DataDownload blocks resume permanently (not a timing gap), and a manual retry hangs unless the superseded DataDownload object is deleted first. Documented as a design requirement for operator-visible signaling and retry cleanup. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Round-4 coderabbit findings pushed for these to read as actual fix requirements rather than narrated known-limitations, since the design doc should say what needs to change, not just that it's broken. The actual code lives in migtools/kubevirt-datamover-controller (a separate repo), so this doc states the requirement rather than implementing it here: - VMB cleanup must run on every Failed transition, not just Canceling. - DataDownload sibling-completeness check must scope to the current restore attempt (e.g. via Restore UID/name correlation) instead of requiring operators to manually delete superseded objects. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/design/kubevirt-datamover.md (1)
264-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the status section or remove resolved questions.
This section remains
Open questions, but Lines 264 and 269 mark PVC sizing and force-full backup as resolved. Rename the section toStatus and open questions, or move resolved items out of it. Keep the genuinely unresolved failed-VMB cleanup item visible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/kubevirt-datamover.md` around lines 264 - 269, Update the documentation section heading containing these resolved PVC sizing and force-full backup entries to “Status and open questions,” or move the resolved entries into a separate status section. Keep the unresolved failed-VMB cleanup issue explicitly visible as an open question.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/kubevirt-datamover.md`:
- Line 269: Update the force-full backup resolution note to avoid claiming E2E
coverage unless a test specifically exercises AnnotationForceFullBackup or
VMB.Spec.ForceFullBackup. Add a citation to such an annotation-specific test if
one exists; otherwise qualify the note as implementation-only and remove the E2E
coverage claim.
---
Nitpick comments:
In `@docs/design/kubevirt-datamover.md`:
- Around line 264-269: Update the documentation section heading containing these
resolved PVC sizing and force-full backup entries to “Status and open
questions,” or move the resolved entries into a separate status section. Keep
the unresolved failed-VMB cleanup issue explicitly visible as an open question.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b3d2cea-2c52-425c-9ccd-92690b30d2f1
📒 Files selected for processing (1)
docs/design/kubevirt-datamover.md
…ecision Mechanical: replace tab-indented nested list items with spaces for consistent rendering (coderabbit minor finding). Substantive, all verified by peer agents rather than assumed: - Force-full-backup e2e coverage claim was wrong: e2e only tests the automatic max-incremental-backups threshold path, zero coverage for the manual force-full-backup annotation. Corrected both the E2E coverage bullet and the Open Questions resolution note. - PVC RIA spec.selector omission: documented why it's very likely safe (kubevirt PVCs are always dynamically provisioned, never carry a selector) plus the empirical e2e signal, while flagging the one remaining unverified step (inspecting an actual backed-up PVC's YAML). - PVC-sizing bound-PV-capacity fix is NOT on oadp-dev yet - it's part of the same unmerged PR124 as everything else in this doc, not separately shipped. Currently-shipping manifests record requested size, and the restore-side floor doesn't protect against the exact backend-bump scenario the fix targets, so pre-#124 backups need a migration/compat note before the fix merges. Added a warning next to the pvcSizes manifest schema example so readers don't assume a fixed meaning without checking which uploader version wrote it. - Linked the real tracking issue for VMB-orphan-on-Failed (kubevirt-datamover-controller#168, filed by kdm-controller, who had full context) instead of a vague 'needs an issue' placeholder. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…rseded The design originally proposed a force-full-virt-backup annotation on the Velero Backup object. What actually shipped is a DataUpload-level annotation (kubevirt-datamover.io/force-full-backup) honored as VMB.Spec.ForceFullBackup - a different object entirely. Mark the original proposal superseded so operators aren't misled into annotating the wrong resource. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…xplicit open question - DataDownload restore-attempt-correlation fix now links kubevirt-datamover-controller#169 (filed by kdm-controller, proposes correlating by restore-attempt id coordinated with kdm-plugin) instead of a placeholder 'needs an issue'. - Add pvcSizes schema-versioning as a new Open Question, per kdm-controller's explicitly-unreviewed sketch (schemaVersion/ sizeSemantics field, fail-open-to-conservative restore fallback) - flagged as needing real design review, not written up as decided. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Fixed an error I introduced last round: claimed delete-then-retry 'does not reliably unblock' the VMB run-state gate, adding an unverified 'race with controller cache' caveat that was never confirmed by kdm-controller. What they actually verified is that delete-then-retry DOES mechanically unblock it - it's just an undocumented manual workaround with no product-level support, not a broken mechanism. Corrected the wording to match what was verified. Also added force-full-backup annotation provenance: introduced in kubevirt-datamover-controller PR #13 (mpryc) as part of a squashed Phase 4 commit; no PR discussion, commit message, or linked issue documents why DataUpload-level was chosen over the originally-proposed Backup-level annotation - attributed as undocumented rather than inferring a rationale. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kdm-plugin confirmed via grep of pvc/restore_test.go: no fixture sets spec.selector, no assertion on clearPVCBinding's handling of it either way. Live-cluster/e2e YAML inspection remains the only way to close this verification gap. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kdm-plugin added TestClearPVCBinding_LeavesSelectorUntouched (pvc/restore_test.go, commit 65147c4) pinning that clearPVCBinding leaves spec.selector untouched regardless of its input value. Only remaining gap is a live-cluster/e2e check that real kubevirt-backed PVCs never carry a selector in the first place, requested from oadp-e2e. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
oadp-e2e PR openshift#2350 (commit 30a3352) confirms on a real cluster that the source PVC has spec.selector == nil before backup, closing the last open piece of this deviation - combined with kdm-plugin's unit test, this is now RESOLVED rather than an open verification gap. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
… a fixed bug Per kaovilai's framing correction (relayed via kdm-plugin): the first-DataDownload grace period is anchored to when the operation first observed an empty DataDownload list, not the restore's start time (plugin commit 8b05d38) - this was always the intended design for this PR, not a pre-existing bug later discovered and fixed. Document it as such rather than as a discovery/fix narrative. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…d-vs-not Per kaovilai: oadp-dev/main hasn't shipped to any customer yet, so 'merged into oadp-dev' should never be described as 'shipping' or 'shipped' - that implies customer availability that doesn't exist. Replace 'currently shipping'/'ships'/'shipped' language throughout with 'merged into oadp-dev' (with explicit notes that this is not customer-available), keeping the technical merged-vs-unmerged distinction without the misleading shipped connotation. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai placed a hold on kubevirt-datamover-controller#124 and kubevirt-datamover-plugin#44: e2e only asserts VM Running, DataDownload Completed, block volumeMode, and spec.selector state - none of it reads back actual restored disk contents, and there's no checksum or known-file check anywhere in oadp-operator's e2e suite for kubevirt-datamover. A restore reporting 'success' today could still have wrong/corrupted data on disk undetected. oadp-e2e is adding real data verification now; caveat every PASS claim in this section until that lands and the hold lifts. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…lanned bug Per kaovilai: leaving the VMB in place when a DataUpload genuinely Fails is the actual current intent, to aid debugging - not simply an unfixed oversight. Reframe from 'Required fix' to 'currently intentional, tracked as openshift#168 (unplanned)', noting a future configurable opt-in cleanup is a possibility, not committed work. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…tribution
Per /firstsubmit: remove round-tracking artifacts that accumulated
across the coderabbit-iterate loop - date stamps (2026-08-06),
RESOLVED/PARTIALLY RESOLVED/SUPERSEDED/NEW status badges, 'this
session' framing, and 'confirmed/verified by <agent>' attribution.
Restructure Open Questions as direct answers rather than review-status
labels, consistent with the existing [alitke]-attribution style
already used in this doc for named design opinions.
Also drop 'was' phrasing that narrated implementation history ('was
never implemented', 'was designed for', 'not what was implemented')
in favor of present-tense statements of current design and behavior,
since a design doc should read as if it predates the implementation
it describes, not as a changelog of how review found it.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
The data-integrity gap that put a hold on controller#124/plugin#44 is closed: a checksum test writes a known payload directly to the source Block-mode PVC (dd oflag=direct conv=fsync), bracket-verifies via iflag=direct reads before/after the backup window to confirm the region was quiescent, then checksums the same region on the restored PVC. A mismatch hard-fails the test - deterministic proof, not a soft/logged comparison. Both PRs' holds are lifted. Known limitation carried forward: this method only covers full backups (the host-side dd write bypasses qemu's CBT dirty-bitmap), so incremental-chain data integrity is still unverified - tracked via TODO markers in the e2e code pending a guest-agent-equipped fixture. Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
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
1 participant
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.
Summary
Adds an implementation-status layer to
docs/design/kubevirt-datamover.md, folded directly into the relevant design sections (BIA/RIA plugins, controller reconcilers, manifest schema) rather than as a separate status dump, plus a new E2E coverage section.Design notes
oadp-dev.kubevirt-datamover.io/force-full-backupannotation on the DataUpload object, not the Velero Backup as originally proposed — the doc marks the original Backup-level proposal superseded and documents the actual mechanism, with its rationale noted as undocumented in git/GitHub history.pvcSizesmanifest semantics differ between the uploader currently merged intooadp-dev(records requested PVC size) and the in-flight PR Add different cloud provider support between BSL and VSL #124 (records bound-PV actual capacity) — a live compat gap needing a migration/versioning plan before Add different cloud provider support between BSL and VSL #124 merges.Known limitations
Failed(non-canceled) DataUpload. This is intentional, to aid debugging failed backups; tracked as kubevirt-datamover-controller#168 (currently unplanned).pvcSizesmanifest versioning has no plan yet — see the Open Questions section for a sketch.Testing
Content in this doc reflects direct code inspection and test runs against the controller, plugin, and e2e repos.
Test plan
Note
Responses generated with Claude