From 857c6cdd99e51665bf6bfa167c682691f712aa98 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 08:45:02 -0400 Subject: [PATCH 01/18] docs: update kubevirt-datamover design doc with 2026-08-06 implementation 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 --- docs/design/kubevirt-datamover.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 1f02b64a27c..f1680ebf576 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -233,6 +233,34 @@ Per-Backup-oer-vm Manifest (manifests//.json): - We could use kopia on top of the object storage API, but it is not clear that this will provide any real benefits, since we're already working with files that represent just the data diff we need. We can just manage them as individual objects. - This will also require additional overhead around kopia maintenance, and we still may need to manage qcow2 file deletion manually. +### Implementation status (as of 2026-08-06) + +Snapshot of `oadp-dev` HEAD (`5b6f6370`) plus in-flight PRs kubevirt-datamover-controller#124 and kubevirt-datamover-plugin#44 (both open, not yet merged as of this date). + +**Kubevirt Datamover Controller** +- DataUpload/DataDownload reconcilers implement `New -> Accepted -> Prepared -> InProgress -> Completed/Failed/Canceling`, with `Spec.Cancel` handled at any non-terminal phase. +- Concurrency: `MaxConcurrentReconciles` per controller (default 3 if unset). DataUpload additionally serializes per-VM (`hasOlderActiveDUForVM` requeues a new DU if an older active one targets the same VM) so incremental checkpoint chains stay ordered even under concurrency. +- Checkpoint chain restore (`pkg/downloader/chain.go`): `rebaseChain` repoints each qcow2's backing-file to the local predecessor path (the backup-time path is meaningless on the restore pod), then `flattenToRaw` runs `qemu-img convert -O raw` onto the target — now passing `-S 0` for block-mode targets (landed today; sparse-write skip is unsafe on a reused block device). Chain resolution (`pkg/uploader` index → `resolveTargetDiskName`) prefers the newest checkpoint's disk-name mapping, falling back through older entries if the newest is malformed (also landed today). +- VMB/VMBT lifecycle (resolves open question below): VMB is deleted by the uploader pod itself on success (after S3 upload completes), or by the controller (`cleanupVMBackupResources`) on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). +- PVC sizing (resolves open question below): scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). +- **Deviation / in-flight**: PR #124 adds VM run-state restore (VM halted at restore by the plugin, flipped back to running by the controller once all its DataDownloads complete) — see plugin section below. Not present on `oadp-dev` HEAD; lands only after both #124 and plugin#44 merge. + +**BackupItemAction/RestoreItemAction plugins** +All 6 registered in `main.go` (kubevirt-datamover-plugin repo): +- VM BIA (prio 01) — implemented: CBT check, per-PVC volume-policy conflict detection (`hasKubevirtPolicy`/`hasConflictingPolicy`), creates DataUpload, stamps `DataUploadNameAnnotation` on the VM. +- PVC BIA (prio 02) — implemented: stamps `AnnotationVMName` on the PVC (raw unstructured, to preserve unknown fields). +- VM DeleteItemAction (prio 01, separate action type) — implemented; not originally scoped in this doc. +- PVC RIA (prio 03) — implemented: clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as originally specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. +- VMBackup/VMBT discard RIA (prio 04) — implemented as designed: discards both CRs via `WithoutRestore()` so they don't re-trigger backup logic on restore. +- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged): halts VM to `RunStrategyHalted` if it was auto-starting at backup time (handles both `spec.runStrategy` and the deprecated `spec.running` bool), resumes by aggregating sibling-DataDownload progress/cancel. Known gaps documented in the PR: (1) progress reports "done" once all *discovered* DataDownloads complete rather than once the VM's full expected volume count is known — could report done early for multi-disk VMs (tracked as controller#73 phase4, explicitly out of scope for #44); (2) the 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. + +**E2E coverage** (`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt today) +- PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). +- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → `maxIncrementalBackups` forces a full backup (validated prior session, referenced as green in the current PR description; not independently rerun today). +- PASS: restore from a full kubevirt-datamover CBT backup (verified twice today, including after the RBAC fix). +- Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on kubevirt-datamover-controller#73 phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). +- No flakes observed across 4 runs today (small sample — not a long-term flake-free claim). + ### Open questions - How to determine PVC size? - user-configurable? configmap or annotation? @@ -240,9 +268,12 @@ Per-Backup-oer-vm Manifest (manifests//.json): - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space. - Since controller is responsible for PVC creation rather than plugin, the controller may be able to respond to PVC too small errors by retrying with a larger PVC. - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. + - **RESOLVED (2026-08-06)**: derived from recorded bound-PV actual capacity, not requested size — see Implementation status above. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. + - **RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events) — see Implementation status above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. + - **RESOLVED (2026-08-06)**: implemented via `AnnotationForceFullBackup` + `VMB.Spec.ForceFullBackup`, with e2e coverage — see Implementation status above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From ed91dcfd398dbd3fb2cb1e575d67e96d8635e1d1 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 08:54:14 -0400 Subject: [PATCH 02/18] docs: address coderabbit findings on VMB failure-cleanup and multi-disk 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 --- docs/design/kubevirt-datamover.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index f1680ebf576..b73c74b3055 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -241,7 +241,7 @@ Snapshot of `oadp-dev` HEAD (`5b6f6370`) plus in-flight PRs kubevirt-datamover-c - DataUpload/DataDownload reconcilers implement `New -> Accepted -> Prepared -> InProgress -> Completed/Failed/Canceling`, with `Spec.Cancel` handled at any non-terminal phase. - Concurrency: `MaxConcurrentReconciles` per controller (default 3 if unset). DataUpload additionally serializes per-VM (`hasOlderActiveDUForVM` requeues a new DU if an older active one targets the same VM) so incremental checkpoint chains stay ordered even under concurrency. - Checkpoint chain restore (`pkg/downloader/chain.go`): `rebaseChain` repoints each qcow2's backing-file to the local predecessor path (the backup-time path is meaningless on the restore pod), then `flattenToRaw` runs `qemu-img convert -O raw` onto the target — now passing `-S 0` for block-mode targets (landed today; sparse-write skip is unsafe on a reused block device). Chain resolution (`pkg/uploader` index → `resolveTargetDiskName`) prefers the newest checkpoint's disk-name mapping, falling back through older entries if the newest is malformed (also landed today). -- VMB/VMBT lifecycle (resolves open question below): VMB is deleted by the uploader pod itself on success (after S3 upload completes), or by the controller (`cleanupVMBackupResources`) on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). +- VMB/VMBT lifecycle (partially resolves open question below): VMB is deleted by the uploader pod itself on success (after S3 upload completes), or by the controller (`cleanupVMBackupResources`) on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it. It is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. - PVC sizing (resolves open question below): scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). - **Deviation / in-flight**: PR #124 adds VM run-state restore (VM halted at restore by the plugin, flipped back to running by the controller once all its DataDownloads complete) — see plugin section below. Not present on `oadp-dev` HEAD; lands only after both #124 and plugin#44 merge. @@ -252,7 +252,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo): - VM DeleteItemAction (prio 01, separate action type) — implemented; not originally scoped in this doc. - PVC RIA (prio 03) — implemented: clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as originally specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. - VMBackup/VMBT discard RIA (prio 04) — implemented as designed: discards both CRs via `WithoutRestore()` so they don't re-trigger backup logic on restore. -- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged): halts VM to `RunStrategyHalted` if it was auto-starting at backup time (handles both `spec.runStrategy` and the deprecated `spec.running` bool), resumes by aggregating sibling-DataDownload progress/cancel. Known gaps documented in the PR: (1) progress reports "done" once all *discovered* DataDownloads complete rather than once the VM's full expected volume count is known — could report done early for multi-disk VMs (tracked as controller#73 phase4, explicitly out of scope for #44); (2) the 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. +- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged): halts VM to `RunStrategyHalted` if it was auto-starting at backup time (handles both `spec.runStrategy` and the deprecated `spec.running` bool), resumes via `restoreVMRunStateIfAllSiblingsCompleted`/`allSiblingDataDownloadsCompleted`, which gates on "every DataDownload *currently matching* this VM's correlation annotations is Completed" — it does not independently check that count against the VM's actual `spec.volumes` count. **Known-accepted scope boundary, not a merge blocker**: #124/#44 validated only the single-disk case (exactly one sibling, so "all discovered = Completed" is race-free by construction); multi-disk VMs are explicitly out of scope for this phase per #124's own Known Limitations, tracked under [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73)'s later phases — and multi-disk CBT restore itself doesn't work yet regardless (see E2E coverage above), so the race is dormant today. If the plugin ever creates a multi-disk VM's DataDownload CRs in a staggered/non-atomic way, this gate could see "1 of eventually-N discovered, and it's Completed" and resume the VM prematurely — phase4 needs an explicit expected-volume-count signal (from the VM spec or the plugin) rather than "whatever DataDownloads exist right now"; (2) the 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. **E2E coverage** (`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt today) - PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). @@ -270,7 +270,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo): - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - **RESOLVED (2026-08-06)**: derived from recorded bound-PV actual capacity, not requested size — see Implementation status above. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - - **RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events) — see Implementation status above. + - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See Implementation status above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - **RESOLVED (2026-08-06)**: implemented via `AnnotationForceFullBackup` + `VMB.Spec.ForceFullBackup`, with e2e coverage — see Implementation status above. From 6bef71326948678b717b8b6103c036c2e11ce174 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 08:57:59 -0400 Subject: [PATCH 03/18] docs: fold VM run-state restore into design body, not just status footnote 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 --- docs/design/kubevirt-datamover.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index b73c74b3055..3e0bea80a65 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -67,6 +67,7 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - Similar in functionality to csi PVC restore action - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin + - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" @@ -96,6 +97,7 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup + - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. Making this correct for multi-disk VMs is deferred to the multi-disk restore hardening phase (see [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73)), which will need an explicit expected-volume-count signal (from the VM spec or the plugin) rather than "whatever DataDownloads currently exist." - Create the temporary PVC to download the qcow2 files onto. - PV here is also temporary - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes @@ -243,7 +245,7 @@ Snapshot of `oadp-dev` HEAD (`5b6f6370`) plus in-flight PRs kubevirt-datamover-c - Checkpoint chain restore (`pkg/downloader/chain.go`): `rebaseChain` repoints each qcow2's backing-file to the local predecessor path (the backup-time path is meaningless on the restore pod), then `flattenToRaw` runs `qemu-img convert -O raw` onto the target — now passing `-S 0` for block-mode targets (landed today; sparse-write skip is unsafe on a reused block device). Chain resolution (`pkg/uploader` index → `resolveTargetDiskName`) prefers the newest checkpoint's disk-name mapping, falling back through older entries if the newest is malformed (also landed today). - VMB/VMBT lifecycle (partially resolves open question below): VMB is deleted by the uploader pod itself on success (after S3 upload completes), or by the controller (`cleanupVMBackupResources`) on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it. It is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. - PVC sizing (resolves open question below): scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). -- **Deviation / in-flight**: PR #124 adds VM run-state restore (VM halted at restore by the plugin, flipped back to running by the controller once all its DataDownloads complete) — see plugin section below. Not present on `oadp-dev` HEAD; lands only after both #124 and plugin#44 merge. +- **Deviation / in-flight**: PR #124 adds VM run-state restore (mechanism now documented in the DataDownload reconciler section above). Not present on `oadp-dev` HEAD; lands only after both #124 and plugin#44 merge. **BackupItemAction/RestoreItemAction plugins** All 6 registered in `main.go` (kubevirt-datamover-plugin repo): @@ -252,7 +254,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo): - VM DeleteItemAction (prio 01, separate action type) — implemented; not originally scoped in this doc. - PVC RIA (prio 03) — implemented: clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as originally specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. - VMBackup/VMBT discard RIA (prio 04) — implemented as designed: discards both CRs via `WithoutRestore()` so they don't re-trigger backup logic on restore. -- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged): halts VM to `RunStrategyHalted` if it was auto-starting at backup time (handles both `spec.runStrategy` and the deprecated `spec.running` bool), resumes via `restoreVMRunStateIfAllSiblingsCompleted`/`allSiblingDataDownloadsCompleted`, which gates on "every DataDownload *currently matching* this VM's correlation annotations is Completed" — it does not independently check that count against the VM's actual `spec.volumes` count. **Known-accepted scope boundary, not a merge blocker**: #124/#44 validated only the single-disk case (exactly one sibling, so "all discovered = Completed" is race-free by construction); multi-disk VMs are explicitly out of scope for this phase per #124's own Known Limitations, tracked under [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73)'s later phases — and multi-disk CBT restore itself doesn't work yet regardless (see E2E coverage above), so the race is dormant today. If the plugin ever creates a multi-disk VM's DataDownload CRs in a staggered/non-atomic way, this gate could see "1 of eventually-N discovered, and it's Completed" and resume the VM prematurely — phase4 needs an explicit expected-volume-count signal (from the VM spec or the plugin) rather than "whatever DataDownloads exist right now"; (2) the 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. +- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged), mechanism now documented in the VirtualMachine RIA plugin and DataDownload reconciler sections above. The 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. **E2E coverage** (`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt today) - PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). From 6ec71784f941344ad594f3964de64df972c26232 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:05:23 -0400 Subject: [PATCH 04/18] docs: fold implementation status into design body, drop status section 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 --- docs/design/kubevirt-datamover.md | 55 +++++++++++++------------------ 1 file changed, 23 insertions(+), 32 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 3e0bea80a65..d6780379888 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -47,6 +47,7 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - In `pkg/restore/actions/dataupload_retrieve_action.go` and in `DataDownload` we need to add SnapshotType. ### BackupItemAction/RestoreItemAction plugins +All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - VirtualMachine BIA plugin - The plugin will check whether the VirtualMachine's `status.ChangedBlockTracking` is `Enabled` - The plugin must also determine whether the VM is running, since offline backup is not supported in the initial release. @@ -61,25 +62,32 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - Add `velerov1api.DataUploadNameAnnotation` to VirtualMachine - Add `velerov1api.PVCNamespaceNameLabel` annotation to VirtualMachine (doesn't need to be a label, since we're just using it to figure out what label selector to use for the ConfigMap on restore). - OperationID will be created and returned similar to what's done with the CSI PVC plugin, and the async operation Progress method will report on progress based on the DU status (similar to CSI PVC plugin) + - **As implemented** (prio 01): CBT check via `controllercommon.ValidateCBTEnabled`, then `checkVolumePolicies` detects custom-kubevirt vs. conflicting volume policies per-PVC (`hasKubevirtPolicy`/`hasConflictingPolicy`), matching the design above. - PVC BIA plugin - Add `kubevirt-datamover-vm` annotation to PVC with the `VirtualMachine` name to signal to RIA that we need to remove `VolumeName` and set `Selector.MatchLabels` on PVC. + - **As implemented** (prio 02): the actual annotation is `controllercommon.AnnotationVMName`, stamped on the raw unstructured PVC (not a typed round-trip) to preserve unknown fields. +- VM DeleteItemAction (prio 01, separate action type) — implemented; not originally scoped in this doc. - VirtualMachine RIA plugin - Similar in functionality to csi PVC restore action - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. + - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Design requirement**: document (and, ideally, surface via a condition/event) that operators must delete superseded Failed/Canceled DataDownload objects before retrying a restore for the same VM, and that `Progress()`'s grace-period-expired error message doubles as the only operator-facing signal today. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - set selector with MatchLabels to match PV that will be created by restore controller + - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. + - **As implemented** (prio 04): discards both `virtualmachinebackups.backup.kubevirt.io` and `virtualmachinebackuptrackers.backup.kubevirt.io` via `WithoutRestore()`, matching the design above. ### Kubevirt Datamover Controller - Responsible for reconciling DataUploads/DataDownloads where `Spec.DataMover` is "kubevirt" -- Configurable concurrency limits: concurrent-vm-backups and concurrent-vm-datauploads +- Configurable concurrency limits: concurrent-vm-backups and concurrent-vm-datauploads. **As implemented**: `MaxConcurrentReconciles` per controller (default 3 if unset); DataUpload additionally serializes per-VM (`hasOlderActiveDUForVM` requeues a new DU if an older active one targets the same VM) so incremental checkpoint chains stay ordered even under concurrency. - We need the `qemu-img` binary built into the controller image. +- Both reconcilers implement the same phase state machine: `New -> Accepted -> Prepared -> InProgress -> Completed/Failed/Canceling`, with `Spec.Cancel` handled at any non-terminal phase. - DataUpload reconciler (backup): - create the (temporary) PVC. - identify the VirtualMachine from the PVC metadata. @@ -94,14 +102,15 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - Save any required metadata to identify the stored data (collection of qcow2 pathnames/checkpoints, etc.), along with identifying the backup and VirtualMachine they're associated with. Save this metadata file as well (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - We need to properly handle cases where we attempt an incremental backup but a full backup is taken instead (checkpoint lost, CSI snapshot restore since last checkpoint, VM restart, etc.) - Aborted backups also need to be handled (resulting in a failed PVC backup on the Velero side) + - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup - - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. Making this correct for multi-disk VMs is deferred to the multi-disk restore hardening phase (see [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73)), which will need an explicit expected-volume-count signal (from the VM spec or the plugin) rather than "whatever DataDownloads currently exist." + - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore ships, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. - Create the temporary PVC to download the qcow2 files onto. - PV here is also temporary - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes - - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. + - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **As implemented**: scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). - Create temporary PVCs for each PVC in the VM (identified from BSL metadata). - These need to be mounted as block mode volumes. - PV will be bound to workload PVCs after restore, similar to velero datamover. @@ -128,8 +137,9 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - (continue for each incremental in chain order) - Convert the top-of-chain directly to the target block device (no intermediate raw file or `dd` needed): - - `qemu-img convert -f qcow2 -O raw incN.qcow2 /dev/target_pvc_block_device` + - `qemu-img convert -f qcow2 -O raw incN.qcow2 /dev/target_pvc_block_device`, passing `-S 0` for block-mode targets (sparse-write skip is unsafe on a reused block device). - Delete all qcow2 files from scratch space. + - Chain resolution (`pkg/uploader` index → `resolveTargetDiskName`) prefers the newest checkpoint's disk-name mapping, falling back through older entries if the newest is malformed. - References: - Chained rebase approach: [KubeVirt VEP — Restore from Backup](https://github.com/kubevirt/enhancements/blob/main/veps/sig-storage/incremental-backup.md?plain=1#L443-L466) - `-F` backing format flag required since [QEMU 6.1](https://wiki.qemu.org/ChangeLog/6.1#Block_layer); see [qemu-img rebase docs](https://www.qemu.org/docs/master/tools/qemu-img.html#cmdoption-qemu-img-commands-arg-F) @@ -235,33 +245,14 @@ Per-Backup-oer-vm Manifest (manifests//.json): - We could use kopia on top of the object storage API, but it is not clear that this will provide any real benefits, since we're already working with files that represent just the data diff we need. We can just manage them as individual objects. - This will also require additional overhead around kopia maintenance, and we still may need to manage qcow2 file deletion manually. -### Implementation status (as of 2026-08-06) - -Snapshot of `oadp-dev` HEAD (`5b6f6370`) plus in-flight PRs kubevirt-datamover-controller#124 and kubevirt-datamover-plugin#44 (both open, not yet merged as of this date). - -**Kubevirt Datamover Controller** -- DataUpload/DataDownload reconcilers implement `New -> Accepted -> Prepared -> InProgress -> Completed/Failed/Canceling`, with `Spec.Cancel` handled at any non-terminal phase. -- Concurrency: `MaxConcurrentReconciles` per controller (default 3 if unset). DataUpload additionally serializes per-VM (`hasOlderActiveDUForVM` requeues a new DU if an older active one targets the same VM) so incremental checkpoint chains stay ordered even under concurrency. -- Checkpoint chain restore (`pkg/downloader/chain.go`): `rebaseChain` repoints each qcow2's backing-file to the local predecessor path (the backup-time path is meaningless on the restore pod), then `flattenToRaw` runs `qemu-img convert -O raw` onto the target — now passing `-S 0` for block-mode targets (landed today; sparse-write skip is unsafe on a reused block device). Chain resolution (`pkg/uploader` index → `resolveTargetDiskName`) prefers the newest checkpoint's disk-name mapping, falling back through older entries if the newest is malformed (also landed today). -- VMB/VMBT lifecycle (partially resolves open question below): VMB is deleted by the uploader pod itself on success (after S3 upload completes), or by the controller (`cleanupVMBackupResources`) on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it. It is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. -- PVC sizing (resolves open question below): scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). -- **Deviation / in-flight**: PR #124 adds VM run-state restore (mechanism now documented in the DataDownload reconciler section above). Not present on `oadp-dev` HEAD; lands only after both #124 and plugin#44 merge. - -**BackupItemAction/RestoreItemAction plugins** -All 6 registered in `main.go` (kubevirt-datamover-plugin repo): -- VM BIA (prio 01) — implemented: CBT check, per-PVC volume-policy conflict detection (`hasKubevirtPolicy`/`hasConflictingPolicy`), creates DataUpload, stamps `DataUploadNameAnnotation` on the VM. -- PVC BIA (prio 02) — implemented: stamps `AnnotationVMName` on the PVC (raw unstructured, to preserve unknown fields). -- VM DeleteItemAction (prio 01, separate action type) — implemented; not originally scoped in this doc. -- PVC RIA (prio 03) — implemented: clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as originally specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. -- VMBackup/VMBT discard RIA (prio 04) — implemented as designed: discards both CRs via `WithoutRestore()` so they don't re-trigger backup logic on restore. -- VM RIA (prio 05) — implemented in plugin#44 (open, not yet merged), mechanism now documented in the VirtualMachine RIA plugin and DataDownload reconciler sections above. The 10-minute first-DataDownload-appearance grace period anchors to overall restore start time rather than per-VM registration time — an unverified edge case on very large/slow restores. +### E2E coverage (as of 2026-08-06) -**E2E coverage** (`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt today) +`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt. - PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). -- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → `maxIncrementalBackups` forces a full backup (validated prior session, referenced as green in the current PR description; not independently rerun today). -- PASS: restore from a full kubevirt-datamover CBT backup (verified twice today, including after the RBAC fix). -- Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on kubevirt-datamover-controller#73 phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). -- No flakes observed across 4 runs today (small sample — not a long-term flake-free claim). +- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → `maxIncrementalBackups` forces a full backup (validated prior session, referenced as green in the current PR description; not independently rerun this session). +- PASS: restore from a full kubevirt-datamover CBT backup (verified twice this session, including after the RBAC fix). +- Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). +- No flakes observed across 4 runs this session (small sample — not a long-term flake-free claim). ### Open questions - How to determine PVC size? @@ -270,12 +261,12 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo): - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space. - Since controller is responsible for PVC creation rather than plugin, the controller may be able to respond to PVC too small errors by retrying with a larger PVC. - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - - **RESOLVED (2026-08-06)**: derived from recorded bound-PV actual capacity, not requested size — see Implementation status above. + - **RESOLVED (2026-08-06)**: derived from recorded bound-PV actual capacity, not requested size — see DataDownload reconciler above. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See Implementation status above. + - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - - **RESOLVED (2026-08-06)**: implemented via `AnnotationForceFullBackup` + `VMB.Spec.ForceFullBackup`, with e2e coverage — see Implementation status above. + - **RESOLVED (2026-08-06)**: implemented via `AnnotationForceFullBackup` + `VMB.Spec.ForceFullBackup`, with e2e coverage — see E2E coverage above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From c2ed070b02c57c76bd53c09456b578e9d38aa3f4 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:12:35 -0400 Subject: [PATCH 05/18] docs: sharpen VMB-orphan and retry-gating gaps into required fixes 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 --- docs/design/kubevirt-datamover.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index d6780379888..949fca6138a 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -72,7 +72,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Design requirement**: document (and, ideally, surface via a condition/event) that operators must delete superseded Failed/Canceled DataDownload objects before retrying a restore for the same VM, and that `Progress()`'s grace-period-expired error message doubles as the only operator-facing signal today. + - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented, needs a tracking issue)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt. This removes the manual-cleanup requirement rather than just documenting it; until it lands, operators must delete superseded DataDownload objects before retrying, and `Progress()`'s grace-period-expired error message is the only operator-facing signal. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" @@ -102,7 +102,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Save any required metadata to identify the stored data (collection of qcow2 pathnames/checkpoints, etc.), along with identifying the backup and VirtualMachine they're associated with. Save this metadata file as well (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - We need to properly handle cases where we attempt an incremental backup but a full backup is taken instead (checkpoint lost, CSI snapshot restore since last checkpoint, VM restart, etc.) - Aborted backups also need to be handled (resulting in a failed PVC backup on the Velero side) - - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. + - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Required fix (not yet implemented, needs a new tracking issue since #12 is closed)**: every `DataUploadPhaseFailed` transition site must also invoke `cleanupVMBackupResources` (or an equivalent idempotent VMB deletion), exactly as `handleCanceling` already does, so a Failed DataUpload no longer orphans its VMB. VMBT retention (never deleted) is intentional and must not change. - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup From be28ba1ab702e21dc83726f4efa140f8eed49f5d Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:22:49 -0400 Subject: [PATCH 06/18] docs: fix nested-list indentation and correct in-flight-vs-shipped precision 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 --- docs/design/kubevirt-datamover.md | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 949fca6138a..9cfea5e1b2a 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -76,8 +76,8 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - - set selector with MatchLabels to match PV that will be created by restore controller - - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as specified above — kdm-plugin flags this as unverified (kubevirt PVCs are dynamically provisioned so may be N/A), not yet confirmed either way. + - set selector with MatchLabels to match PV that will be created by restore controller + - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, mostly resolved**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. Empirically, oadp-e2e's passing full-CBT-restore runs rebind PVs correctly through this exact code path without it, confirming the omission doesn't block binding for the tested case. **Not yet closed**: nobody has inspected an actual backed-up PVC's YAML to independently confirm `spec.selector` is truly absent (rather than inferred) across kubevirt-datamover-backed PVCs — that's the concrete remaining verification step. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. @@ -97,33 +97,33 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Wait for VMBackup to complete (monitoring status) - Launch kubevirt datamover pod mounting the temporary PVC with the qcow2 file(s) from the backup. - This pod needs to be running a command that will do the datamover operation from pvc to object storage - - The datamover pod functionality should be built into the same image as the kubevirt-datamover-controller pod image. + - The datamover pod functionality should be built into the same image as the kubevirt-datamover-controller pod image. - Copy the new file to object storage (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - Save any required metadata to identify the stored data (collection of qcow2 pathnames/checkpoints, etc.), along with identifying the backup and VirtualMachine they're associated with. Save this metadata file as well (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - We need to properly handle cases where we attempt an incremental backup but a full backup is taken instead (checkpoint lost, CSI snapshot restore since last checkpoint, VM restart, etc.) - Aborted backups also need to be handled (resulting in a failed PVC backup on the Velero side) - - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Required fix (not yet implemented, needs a new tracking issue since #12 is closed)**: every `DataUploadPhaseFailed` transition site must also invoke `cleanupVMBackupResources` (or an equivalent idempotent VMB deletion), exactly as `handleCanceling` already does, so a Failed DataUpload no longer orphans its VMB. VMBT retention (never deleted) is intentional and must not change. + - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), which explicitly notes #12 as partially-delivered rather than resolving)**: every `DataUploadPhaseFailed` transition site must also invoke `cleanupVMBackupResources` (or an equivalent idempotent VMB deletion), exactly as `handleCanceling` already does, so a Failed DataUpload no longer orphans its VMB. VMBT retention (never deleted) is intentional and must not change. - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore ships, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. - Create the temporary PVC to download the qcow2 files onto. - PV here is also temporary - - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes - - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **As implemented**: scratch/work/output PVC sizes derive from the backup index's recorded *bound-PV actual capacity*, not requested size — avoids undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). + - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes + - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **In-flight, not yet on oadp-dev HEAD** (part of PR #124, unmerged): scratch/work/output PVC sizes will derive from the backup index's recorded *bound-PV actual capacity*, not requested size, to avoid undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). **Currently shipping behavior** (the DataUpload/backup-only precursor is already merged into `oadp-dev`): every backup manifest produced today records *requested* size, not bound-PV capacity. **Compat gap**: the restore-side floor (`maxDiskSizeFromIndex`) floors the manifest's recorded size against the restore target's own requested size — the same value — so it does not protect against the exact backend-bump-above-request scenario the fix was designed for. Any backup taken before #124 merges could produce an undersized scratch PVC on restore, and #124 does not retroactively correct already-stored manifests; a migration/compat note (and likely a fallback for pre-fix manifests) is needed before this ships. - Create temporary PVCs for each PVC in the VM (identified from BSL metadata). - These need to be mounted as block mode volumes. - PV will be bound to workload PVCs after restore, similar to velero datamover. - - To facilitate PV reattachment, we need a similar approach to the upstream velero exposer logic: + - To facilitate PV reattachment, we need a similar approach to the upstream velero exposer logic: - The `DynamicPVRestoreLabel` needs to be set on the restore PV - - Generic restore exposer reads the selector back from the target PVC: In , `RebindVolume()` extracts `targetPVC.Spec.Selector.MatchLabels` and passes it to `ResetPVBinding()`. - - `ResetPVBinding()` copies the labels onto the PV: In , the labels (including `DynamicPVRestoreLabel`) are copied from the PVC selector to the PV's labels, and ClaimRef is reset so Kubernetes can bind them. + - Generic restore exposer reads the selector back from the target PVC: In , `RebindVolume()` extracts `targetPVC.Spec.Selector.MatchLabels` and passes it to `ResetPVBinding()`. + - `ResetPVBinding()` copies the labels onto the PV: In , the labels (including `DynamicPVRestoreLabel`) are copied from the PVC selector to the PV's labels, and ClaimRef is reset so Kubernetes can bind them. - Size based on the `pvcSizes` metadata in the BSL. - We'll need to create another datamover pod here which will do the following: - The pod permissions will need to be the same as we have for velero datamover (run as root, selinux config etc.) - The pod will have temp PVC mounted, as well as PVCs mounted for each vm disk we're creating. - The pod running command/image will first get the list of qcow2 files to pull from object storage - - Process one PVC at a time: + - Process one PVC at a time: - Download all required qcow2 files for this PVC from object storage. - Validate the checkpoint chain from the per-VM manifest (`checkpointChain`) before rebasing: verify every intermediate file is present on disk and each @@ -166,6 +166,8 @@ The directory structure will be as follows: └── index.json # Per-VM index file ``` Example of a Per-VM Index file: + +`pvcSizes` semantics have changed across implementations and are **not yet consistent on `oadp-dev`**: the currently-shipping (merged) uploader records each PVC's *requested* size here. An in-flight fix (PR #124, unmerged) changes this to record the *bound-PV actual capacity* instead, to avoid undersizing restores when the storage backend rounds up (e.g. AWS EBS 1GiB minimum) — see DataDownload reconciler above for the full compat gap this creates for manifests written before #124 merges. Readers of this file (and any migration tooling) must not assume a fixed meaning for `pvcSizes` without checking which uploader version wrote it. ``` Per-VM Index (checkpoints///index.json): @@ -249,7 +251,7 @@ Per-Backup-oer-vm Manifest (manifests//.json): `tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt. - PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). -- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → `maxIncrementalBackups` forces a full backup (validated prior session, referenced as green in the current PR description; not independently rerun this session). +- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full (validated prior session, referenced as green in the current PR description; not independently rerun this session). **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below); verified by grepping the whole e2e suite, there is zero test coverage for the manual annotation today. - PASS: restore from a full kubevirt-datamover CBT backup (verified twice this session, including after the RBAC fix). - Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). - No flakes observed across 4 runs this session (small sample — not a long-term flake-free claim). @@ -261,12 +263,12 @@ Per-Backup-oer-vm Manifest (manifests//.json): - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space. - Since controller is responsible for PVC creation rather than plugin, the controller may be able to respond to PVC too small errors by retrying with a larger PVC. - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - - **RESOLVED (2026-08-06)**: derived from recorded bound-PV actual capacity, not requested size — see DataDownload reconciler above. + - **IN-FLIGHT, NOT YET ON oadp-dev (2026-08-06)**: PR #124 (unmerged) derives PVC sizing from recorded bound-PV actual capacity instead of requested size — see DataDownload reconciler above. Currently-shipping backups record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before merge. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - - **RESOLVED (2026-08-06)**: implemented via `AnnotationForceFullBackup` + `VMB.Spec.ForceFullBackup`, with e2e coverage — see E2E coverage above. + - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From a5b8d08e83bb87402f6df85d9b11fc62a0eef101 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:27:31 -0400 Subject: [PATCH 07/18] docs: mark original Velero-Backup force-full annotation proposal superseded 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 --- docs/design/kubevirt-datamover.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 9cfea5e1b2a..e84ba7e3f42 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -268,6 +268,7 @@ Per-Backup-oer-vm Manifest (manifests//.json): - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. + - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above was not what shipped. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. ### General notes From 7a72cb765d6f8ac0b790ca8e624a1427608cb9b8 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:29:19 -0400 Subject: [PATCH 08/18] docs: link tracking issue #169, add pvcSizes versioning as explicit 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 --- docs/design/kubevirt-datamover.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index e84ba7e3f42..d16bd224645 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -72,7 +72,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented, needs a tracking issue)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt. This removes the manual-cleanup requirement rather than just documenting it; until it lands, operators must delete superseded DataDownload objects before retrying, and `Progress()`'s grace-period-expired error message is the only operator-facing signal. + - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt. This removes the manual-cleanup requirement rather than just documenting it; until it lands, operators must delete superseded DataDownload objects before retrying, and `Progress()`'s grace-period-expired error message is the only operator-facing signal. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" @@ -270,6 +270,7 @@ Per-Backup-oer-vm Manifest (manifests//.json): - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above was not what shipped. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. +- **NEW (2026-08-06): how should `pvcSizes` manifest semantics be versioned across the requested-size (currently shipping) and bound-PV-capacity (PR #124, unmerged) writers?** No plan exists yet — kdm-controller offered this as an unreviewed sketch, explicitly not a commitment: add a `schemaVersion` (or a narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` treats its *absence* as "legacy, requested-size" and does not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — i.e. fail open to the more conservative interpretation rather than silently trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From b1a4ecf1ba160bd27693c1128a88fe2acabd9d55 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:34:26 -0400 Subject: [PATCH 09/18] docs: correct manual-retry wording, add force-full annotation provenance 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 --- docs/design/kubevirt-datamover.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index d16bd224645..8b93d4f06d0 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -72,7 +72,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM without deleting the old Failed/Canceled DataDownload objects first) will hang forever too, since the completeness check has no attempt/generation filtering and will keep finding the stale terminal-failed object. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt. This removes the manual-cleanup requirement rather than just documenting it; until it lands, operators must delete superseded DataDownload objects before retrying, and `Progress()`'s grace-period-expired error message is the only operator-facing signal. + - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM) is **not an officially supported workaround today**, even though it can work mechanically: deleting the old Failed/Canceled DataDownload objects first *does* unblock `allSiblingDataDownloadsCompleted` (confirmed by kdm-controller — the completeness check simply stops finding the stale terminal object), but this is a manual, undocumented operator step with no product-level guardrail, no visible prompt to do it, and no test coverage — not a designed recovery path. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt, removing the need for operators to manually delete anything. **Until #169 lands, treat manual cleanup-then-retry as an unsupported, undocumented workaround, not a designed recovery path**; `Progress()`'s grace-period-expired error message is the only operator-facing signal that something is stuck. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" @@ -268,7 +268,7 @@ Per-Backup-oer-vm Manifest (manifests//.json): - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above was not what shipped. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. + - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above was not what shipped. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. **Provenance**: introduced in [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) as one of several Phase 4 features in a squashed commit — no PR discussion, commit message, or linked issue documents *why* DataUpload-level was chosen over Backup-level; this deviation's rationale is not recoverable from git/GitHub history and should be attributed as undocumented rather than inferred. - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. - **NEW (2026-08-06): how should `pvcSizes` manifest semantics be versioned across the requested-size (currently shipping) and bound-PV-capacity (PR #124, unmerged) writers?** No plan exists yet — kdm-controller offered this as an unreviewed sketch, explicitly not a commitment: add a `schemaVersion` (or a narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` treats its *absence* as "legacy, requested-size" and does not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — i.e. fail open to the more conservative interpretation rather than silently trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. From 42cc10c481a7f3f93107bff9ec93e32996e5812f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:43:37 -0400 Subject: [PATCH 10/18] docs: note zero unit test coverage for PVC RIA spec.selector question 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 --- docs/design/kubevirt-datamover.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 8b93d4f06d0..42c52add69c 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -77,7 +77,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - set selector with MatchLabels to match PV that will be created by restore controller - - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, mostly resolved**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. Empirically, oadp-e2e's passing full-CBT-restore runs rebind PVs correctly through this exact code path without it, confirming the omission doesn't block binding for the tested case. **Not yet closed**: nobody has inspected an actual backed-up PVC's YAML to independently confirm `spec.selector` is truly absent (rather than inferred) across kubevirt-datamover-backed PVCs — that's the concrete remaining verification step. + - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, mostly resolved**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. Empirically, oadp-e2e's passing full-CBT-restore runs rebind PVs correctly through this exact code path without it, confirming the omission doesn't block binding for the tested case. **Not yet closed, no unit test either**: `pvc/restore_test.go` has zero fixtures with `spec.selector` set and zero assertions about `clearPVCBinding`'s handling of it (confirmed by grep — only unrelated `selector` hits exist, for `AppliesTo()`'s resource selector and DataDownload `List()` label-selector comments). Nobody has inspected an actual backed-up PVC's YAML either, to independently confirm `spec.selector` is truly absent (rather than inferred) across kubevirt-datamover-backed PVCs. A live-cluster/e2e YAML check remains the right way to close this out. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. From a5d64411efbfb11bb16008175400f53bd143d063 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:49:26 -0400 Subject: [PATCH 11/18] docs: PVC RIA selector question now unit-tested 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 --- docs/design/kubevirt-datamover.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 42c52add69c..c6b728be91d 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -77,7 +77,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - set selector with MatchLabels to match PV that will be created by restore controller - - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, mostly resolved**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. Empirically, oadp-e2e's passing full-CBT-restore runs rebind PVs correctly through this exact code path without it, confirming the omission doesn't block binding for the tested case. **Not yet closed, no unit test either**: `pvc/restore_test.go` has zero fixtures with `spec.selector` set and zero assertions about `clearPVCBinding`'s handling of it (confirmed by grep — only unrelated `selector` hits exist, for `AppliesTo()`'s resource selector and DataDownload `List()` label-selector comments). Nobody has inspected an actual backed-up PVC's YAML either, to independently confirm `spec.selector` is truly absent (rather than inferred) across kubevirt-datamover-backed PVCs. A live-cluster/e2e YAML check remains the right way to close this out. + - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, unit-tested, one verification step remains**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. `TestClearPVCBinding_LeavesSelectorUntouched` (`pvc/restore_test.go`, commit 65147c4) now pins this exactly: `clearPVCBinding` leaves `spec.selector` completely untouched whether it's set or absent going in. Empirically, oadp-e2e's passing full-CBT-restore runs also rebind PVs correctly through this exact code path without a reset. **Not yet closed**: nobody has inspected an actual backed-up PVC's YAML to independently confirm `spec.selector` is truly absent in practice (rather than inferred) across kubevirt-datamover-backed PVCs — a live-cluster/e2e YAML check remains the one remaining step, requested from oadp-e2e. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. From 1e0ad51ab73d2b59b1b4f1f75efebe98192e0eb0 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 09:55:21 -0400 Subject: [PATCH 12/18] docs: close out PVC RIA spec.selector question, now fully verified oadp-e2e PR #2350 (commit 30a3352f) 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 --- docs/design/kubevirt-datamover.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index c6b728be91d..887532be674 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -77,7 +77,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - set selector with MatchLabels to match PV that will be created by restore controller - - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, unit-tested, one verification step remains**: does *not* reset `spec.selector` as specified above. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. The plugin never sets/reads/reasons about `spec.selector` anywhere in its code, consistent with every kubevirt-datamover-backed PVC being dynamically provisioned. `TestClearPVCBinding_LeavesSelectorUntouched` (`pvc/restore_test.go`, commit 65147c4) now pins this exactly: `clearPVCBinding` leaves `spec.selector` completely untouched whether it's set or absent going in. Empirically, oadp-e2e's passing full-CBT-restore runs also rebind PVs correctly through this exact code path without a reset. **Not yet closed**: nobody has inspected an actual backed-up PVC's YAML to independently confirm `spec.selector` is truly absent in practice (rather than inferred) across kubevirt-datamover-backed PVCs — a live-cluster/e2e YAML check remains the one remaining step, requested from oadp-e2e. + - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, RESOLVED (2026-08-06)**: does *not* reset `spec.selector` as specified above — confirmed safe, not just assumed. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. `TestClearPVCBinding_LeavesSelectorUntouched` (`pvc/restore_test.go`, commit 65147c4) pins that `clearPVCBinding` leaves `spec.selector` completely untouched whether it's set or absent going in. oadp-e2e's PR #2350 (commit 30a3352f) closes the remaining live-cluster question: the actual source PVC (`cirros-test-disk`) has `spec.selector == nil` before backup, verified on a real cluster — confirming kubevirt-datamover-backed PVCs never carry a selector in practice, not just in theory. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. From 4c69a091b24f2a75f32032badccd5a84ec912d9f Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 10:59:07 -0400 Subject: [PATCH 13/18] docs: document Progress() grace-period anchor as intended design, not 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 --- docs/design/kubevirt-datamover.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 887532be674..56f00804fa1 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -72,7 +72,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM) is **not an officially supported workaround today**, even though it can work mechanically: deleting the old Failed/Canceled DataDownload objects first *does* unblock `allSiblingDataDownloadsCompleted` (confirmed by kdm-controller — the completeness check simply stops finding the stale terminal object), but this is a manual, undocumented operator step with no product-level guardrail, no visible prompt to do it, and no test coverage — not a designed recovery path. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt, removing the need for operators to manually delete anything. **Until #169 lands, treat manual cleanup-then-retry as an unsupported, undocumented workaround, not a designed recovery path**; `Progress()`'s grace-period-expired error message is the only operator-facing signal that something is stuck. + - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM) is **not an officially supported workaround today**, even though it can work mechanically: deleting the old Failed/Canceled DataDownload objects first *does* unblock `allSiblingDataDownloadsCompleted` (confirmed by kdm-controller — the completeness check simply stops finding the stale terminal object), but this is a manual, undocumented operator step with no product-level guardrail, no visible prompt to do it, and no test coverage — not a designed recovery path. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt, removing the need for operators to manually delete anything. **Until #169 lands, treat manual cleanup-then-retry as an unsupported, undocumented workaround, not a designed recovery path**; `Progress()`'s grace-period-expired error message is the only operator-facing signal that something is stuck. Note on that grace period: it's anchored to when the operation first observed an *empty* DataDownload list, not to the restore's start time (plugin commit 8b05d38) — this is the correct implemented design for this PR, not a pre-existing bug that was found and fixed. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" From bbada6d294c88f2590eacceafc579595731a79db Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 11:12:35 -0400 Subject: [PATCH 14/18] docs: reframe merged-vs-unmerged as unshipped-to-customer, not shipped-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 --- docs/design/kubevirt-datamover.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 56f00804fa1..9a22e0c5e84 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -106,11 +106,11 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup - - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore ships, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. + - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore is implemented, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. - Create the temporary PVC to download the qcow2 files onto. - PV here is also temporary - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes - - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **In-flight, not yet on oadp-dev HEAD** (part of PR #124, unmerged): scratch/work/output PVC sizes will derive from the backup index's recorded *bound-PV actual capacity*, not requested size, to avoid undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). **Currently shipping behavior** (the DataUpload/backup-only precursor is already merged into `oadp-dev`): every backup manifest produced today records *requested* size, not bound-PV capacity. **Compat gap**: the restore-side floor (`maxDiskSizeFromIndex`) floors the manifest's recorded size against the restore target's own requested size — the same value — so it does not protect against the exact backend-bump-above-request scenario the fix was designed for. Any backup taken before #124 merges could produce an undersized scratch PVC on restore, and #124 does not retroactively correct already-stored manifests; a migration/compat note (and likely a fallback for pre-fix manifests) is needed before this ships. + - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **In-flight, not yet on oadp-dev HEAD** (part of PR #124, unmerged): scratch/work/output PVC sizes will derive from the backup index's recorded *bound-PV actual capacity*, not requested size, to avoid undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). **Currently on `oadp-dev` (merged, but not shipped to any customer — `main`/`oadp-dev` has no release yet)**: the DataUpload/backup-only precursor is merged, and every backup manifest produced with it today records *requested* size, not bound-PV capacity. **Compat gap**: the restore-side floor (`maxDiskSizeFromIndex`) floors the manifest's recorded size against the restore target's own requested size — the same value — so it does not protect against the exact backend-bump-above-request scenario the fix was designed for. Any backup taken with the currently-merged code could produce an undersized scratch PVC on restore once #124 lands, since #124 does not retroactively correct already-stored manifests; a migration/compat note (and likely a fallback for pre-fix manifests) is needed before #124 merges. - Create temporary PVCs for each PVC in the VM (identified from BSL metadata). - These need to be mounted as block mode volumes. - PV will be bound to workload PVCs after restore, similar to velero datamover. @@ -167,7 +167,7 @@ The directory structure will be as follows: ``` Example of a Per-VM Index file: -`pvcSizes` semantics have changed across implementations and are **not yet consistent on `oadp-dev`**: the currently-shipping (merged) uploader records each PVC's *requested* size here. An in-flight fix (PR #124, unmerged) changes this to record the *bound-PV actual capacity* instead, to avoid undersizing restores when the storage backend rounds up (e.g. AWS EBS 1GiB minimum) — see DataDownload reconciler above for the full compat gap this creates for manifests written before #124 merges. Readers of this file (and any migration tooling) must not assume a fixed meaning for `pvcSizes` without checking which uploader version wrote it. +`pvcSizes` semantics have changed across implementations and are **not yet consistent on `oadp-dev`** (note: nothing in this doc has shipped to any customer yet — `oadp-dev`/`main` has no release; "merged" below means merged to `oadp-dev`, not customer-available): the uploader currently merged into `oadp-dev` records each PVC's *requested* size here. An in-flight fix (PR #124, unmerged) changes this to record the *bound-PV actual capacity* instead, to avoid undersizing restores when the storage backend rounds up (e.g. AWS EBS 1GiB minimum) — see DataDownload reconciler above for the full compat gap this creates for manifests written before #124 merges. Readers of this file (and any migration tooling) must not assume a fixed meaning for `pvcSizes` without checking which uploader version wrote it. ``` Per-VM Index (checkpoints///index.json): @@ -263,14 +263,14 @@ Per-Backup-oer-vm Manifest (manifests//.json): - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space. - Since controller is responsible for PVC creation rather than plugin, the controller may be able to respond to PVC too small errors by retrying with a larger PVC. - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - - **IN-FLIGHT, NOT YET ON oadp-dev (2026-08-06)**: PR #124 (unmerged) derives PVC sizing from recorded bound-PV actual capacity instead of requested size — see DataDownload reconciler above. Currently-shipping backups record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before merge. + - **IN-FLIGHT, NOT YET ON oadp-dev (2026-08-06)**: PR #124 (unmerged) derives PVC sizing from recorded bound-PV actual capacity instead of requested size — see DataDownload reconciler above. Backups taken with the code currently on `oadp-dev` (itself unshipped to any customer) record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before merge. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above was not what shipped. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. **Provenance**: introduced in [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) as one of several Phase 4 features in a squashed commit — no PR discussion, commit message, or linked issue documents *why* DataUpload-level was chosen over Backup-level; this deviation's rationale is not recoverable from git/GitHub history and should be attributed as undocumented rather than inferred. + - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above is not what was implemented. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. **Provenance**: introduced in [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) as one of several Phase 4 features in a squashed commit — no PR discussion, commit message, or linked issue documents *why* DataUpload-level was chosen over Backup-level; this deviation's rationale is not recoverable from git/GitHub history and should be attributed as undocumented rather than inferred. - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. -- **NEW (2026-08-06): how should `pvcSizes` manifest semantics be versioned across the requested-size (currently shipping) and bound-PV-capacity (PR #124, unmerged) writers?** No plan exists yet — kdm-controller offered this as an unreviewed sketch, explicitly not a commitment: add a `schemaVersion` (or a narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` treats its *absence* as "legacy, requested-size" and does not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — i.e. fail open to the more conservative interpretation rather than silently trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. +- **NEW (2026-08-06): how should `pvcSizes` manifest semantics be versioned across the requested-size (currently merged into `oadp-dev`) and bound-PV-capacity (PR #124, unmerged) writers?** No plan exists yet — kdm-controller offered this as an unreviewed sketch, explicitly not a commitment: add a `schemaVersion` (or a narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` treats its *absence* as "legacy, requested-size" and does not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — i.e. fail open to the more conservative interpretation rather than silently trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From bf9c802a4c9098e2ce5fdf1510d6f2071fdb2904 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 11:21:50 -0400 Subject: [PATCH 15/18] docs: flag e2e PASS claims as status-only, not data-integrity-verified 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 --- docs/design/kubevirt-datamover.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 9a22e0c5e84..dc43ac4a8fa 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -250,9 +250,12 @@ Per-Backup-oer-vm Manifest (manifests//.json): ### E2E coverage (as of 2026-08-06) `tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt. -- PASS: multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). -- PASS: full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full (validated prior session, referenced as green in the current PR description; not independently rerun this session). **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below); verified by grepping the whole e2e suite, there is zero test coverage for the manual annotation today. -- PASS: restore from a full kubevirt-datamover CBT backup (verified twice this session, including after the RBAC fix). + +**Critical caveat — "PASS" below means structural/status verification only, not data integrity.** Every restore scenario asserted VM `Running`, DataDownload `Completed`, block `volumeMode`, and `spec.selector` state — none of it reads back the actual restored disk contents. There is no checksum or known-file write/read check anywhere in oadp-operator's e2e suite for kubevirt-datamover today, so a restore that reports "success" could still have corrupted or wrong data on disk and none of these tests would catch it. Real data verification (checksum or known-file via `virsh`/in-guest exec, pre/post restore) is being added now; PR #124 and #44 have a **hold** placed on them pending it, and the "PASS" claims below should not be read as data-integrity-verified until that lands. + +- PASS (status-only): multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). +- PASS (status-only): full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full (validated prior session, referenced as green in the current PR description; not independently rerun this session). **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below); verified by grepping the whole e2e suite, there is zero test coverage for the manual annotation today. +- PASS (status-only): restore from a full kubevirt-datamover CBT backup (verified twice this session, including after the RBAC fix). - Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). - No flakes observed across 4 runs this session (small sample — not a long-term flake-free claim). From 161906d899bb930dbcbdaa267dbe1c7cd383c3d0 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 14:42:11 -0400 Subject: [PATCH 16/18] docs: VMB-orphan-on-Failed is intentional (debugging aid), not an unplanned 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 #168 (unplanned)', noting a future configurable opt-in cleanup is a possibility, not committed work. Signed-off-by: Tiger Kaovilai --- docs/design/kubevirt-datamover.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index dc43ac4a8fa..648da27ff11 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -102,7 +102,7 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Save any required metadata to identify the stored data (collection of qcow2 pathnames/checkpoints, etc.), along with identifying the backup and VirtualMachine they're associated with. Save this metadata file as well (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - We need to properly handle cases where we attempt an incremental backup but a full backup is taken instead (checkpoint lost, CSI snapshot restore since last checkpoint, VM restart, etc.) - Aborted backups also need to be handled (resulting in a failed PVC backup on the Velero side) - - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), which explicitly notes #12 as partially-delivered rather than resolving)**: every `DataUploadPhaseFailed` transition site must also invoke `cleanupVMBackupResources` (or an equivalent idempotent VMB deletion), exactly as `handleCanceling` already does, so a Failed DataUpload no longer orphans its VMB. VMBT retention (never deleted) is intentional and must not change. + - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap, but currently intentional-by-inaction**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), currently unplanned (2026-08-06)**: per kaovilai, leaving the VMB in place on Failed is the actual intent today, specifically to aid debugging failed backups — not simply an unfixed oversight. Automatic cleanup on Failed is not being pursued as-is; a *configurable* cleanup option (e.g. an opt-in flag/annotation for operators who don't want debugging artifacts left behind) is a possible future consideration, not committed work. VMBT retention (never deleted, on any path) remains intentional and is not part of this discussion either way. - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup @@ -268,7 +268,7 @@ Per-Backup-oer-vm Manifest (manifests//.json): - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - **IN-FLIGHT, NOT YET ON oadp-dev (2026-08-06)**: PR #124 (unmerged) derives PVC sizing from recorded bound-PV actual capacity instead of requested size — see DataDownload reconciler above. Backups taken with the code currently on `oadp-dev` (itself unshipped to any customer) record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before merge. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **Still open**: on genuine `Failed` (not canceled), the VMB is orphaned — no code path deletes it. See DataUpload reconciler above. + - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **On genuine `Failed` (not canceled), the VMB is orphaned — currently intentional**, not just an unfixed gap: per kaovilai, leaving it in place aids debugging failed backups. Tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), currently unplanned; a configurable opt-in cleanup is a possible future consideration. See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above is not what was implemented. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. **Provenance**: introduced in [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) as one of several Phase 4 features in a squashed commit — no PR discussion, commit message, or linked issue documents *why* DataUpload-level was chosen over Backup-level; this deviation's rationale is not recoverable from git/GitHub history and should be attributed as undocumented rather than inferred. From e7da310e7c50107a54bc89408d32661100fcc847 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 6 Aug 2026 23:16:41 -0400 Subject: [PATCH 17/18] docs: rewrite as first submission - strip review narrative, dates, attribution 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 ' 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 --- docs/design/kubevirt-datamover.md | 32 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 648da27ff11..12297853fbb 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -72,12 +72,13 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Create DD based on DU annotation and DU ConfigMap - Need to confirm that VM resource has the PVC name annotation added by the BIA plugin - VM run-state restore: if the backed-up VM was auto-starting (`spec.runStrategy` or the deprecated `spec.running` bool indicates running), the RIA overrides it to `RunStrategyHalted` on restore and stashes the original run state in an annotation. The VM is not flipped back to its original run state until the Kubevirt Datamover Controller confirms every sibling DataDownload for this VM has completed (see DataDownload reconciler below) — this prevents the VM from booting against partially-restored disks. - - **As implemented** (prio 05, plugin#44, open/not yet merged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) — it never reads back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. **Known gap — terminal-failure handling** (verified by kdm-controller/kdm-plugin, not the original design): if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait can never resolve on its own (this is a permanent hang by construction, not a timing race). The stash annotations are only ever cleared by the controller atomically together with a successful flip-back; on failure they're left in place unchanged. A **manual retry** (new DataDownloads created against the same still-halted VM) is **not an officially supported workaround today**, even though it can work mechanically: deleting the old Failed/Canceled DataDownload objects first *does* unblock `allSiblingDataDownloadsCompleted` (confirmed by kdm-controller — the completeness check simply stops finding the stale terminal object), but this is a manual, undocumented operator step with no product-level guardrail, no visible prompt to do it, and no test coverage — not a designed recovery path. A **full second Velero restore** (VM object deleted and recreated) is not subject to this, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix (not yet implemented — tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169), which proposes correlating by a restore-attempt id coordinated with kdm-plugin on what it can stamp at DataDownload-creation time)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt, removing the need for operators to manually delete anything. **Until #169 lands, treat manual cleanup-then-retry as an unsupported, undocumented workaround, not a designed recovery path**; `Progress()`'s grace-period-expired error message is the only operator-facing signal that something is stuck. Note on that grace period: it's anchored to when the operation first observed an *empty* DataDownload list, not to the restore's start time (plugin commit 8b05d38) — this is the correct implemented design for this PR, not a pre-existing bug that was found and fixed. + - **Implementation note** (prio 05, plugin#44, unmerged): each `Execute()` call unconditionally overwrites the stash annotations (`AnnotationOriginalRunStrategy`/`-Source`) computed fresh from that call's own `input.Item` (the backup data being restored) rather than reading back a previous value, so a stale annotation from an earlier failed restore attempt cannot leak into a new restore's halt decision. `Progress()`'s grace period for the first DataDownload to appear is anchored to when the operation first observes an *empty* DataDownload list, not to the restore's start time. + - **Known limitation — terminal-failure handling**: if a sibling DataDownload for this VM ends `Failed` or `Canceled` instead of `Completed`, the VM stays `Halted` permanently, with no visible failure signal beyond that DataDownload's own status — `allSiblingDataDownloadsCompleted` does a blanket `!= Completed` check with no special-case for terminal-failed siblings, and since Failed/Canceled DataDownloads are never reconciled again, the wait cannot resolve on its own (a permanent hang by construction, not a timing race). The stash annotations are cleared by the controller only atomically together with a successful flip-back; on failure they remain unchanged. Deleting the superseded Failed/Canceled DataDownload objects does unblock `allSiblingDataDownloadsCompleted` mechanically, but this is a manual, undocumented operator step with no product-level guardrail, no visible prompt, and no test coverage — not a designed recovery path. A full second Velero restore (VM object deleted and recreated) is unaffected, since the plugin recomputes the stash annotations from that restore's own backup data rather than reading the old ones. **Required fix, tracked as [kubevirt-datamover-controller#169](https://github.com/migtools/kubevirt-datamover-controller/issues/169)**: correlate DataDownloads to a specific restore attempt — e.g. stamp the owning Velero `Restore`'s UID/name into the correlation annotations at creation time — and scope `allSiblingDataDownloadsCompleted`'s query to that attempt only, so a retry's new DataDownloads are evaluated independently of any superseded Failed/Canceled ones from a prior attempt, removing the need for operators to manually delete anything. Until this lands, manual cleanup-then-retry remains an unsupported, undocumented workaround, not a designed recovery path. - PVC RIA plugin - If PVC has `kubevirt-datamover-vm` annotation, need to do the following: - set spec.VolumeName to "" - set selector with MatchLabels to match PV that will be created by restore controller - - **As implemented** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation, RESOLVED (2026-08-06)**: does *not* reset `spec.selector` as specified above — confirmed safe, not just assumed. `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. `TestClearPVCBinding_LeavesSelectorUntouched` (`pvc/restore_test.go`, commit 65147c4) pins that `clearPVCBinding` leaves `spec.selector` completely untouched whether it's set or absent going in. oadp-e2e's PR #2350 (commit 30a3352f) closes the remaining live-cluster question: the actual source PVC (`cirros-test-disk`) has `spec.selector == nil` before backup, verified on a real cluster — confirming kubevirt-datamover-backed PVCs never carry a selector in practice, not just in theory. + - **Implementation note** (prio 03): `clearPVCBinding` clears `spec.volumeName`, `status`, and the PV-controller bind annotations. **Deviation**: does *not* reset `spec.selector` as specified above. This is safe: `spec.selector` is a PVC field used only for the static/pre-provisioned binding pattern (a user manually creates a labeled PV and the PVC selects it by label) — Kubernetes never auto-populates it, and dynamically-provisioned PVCs (the default for KubeVirt VM disks via DataVolumes/CDI) never set it. `TestClearPVCBinding_LeavesSelectorUntouched` (`pvc/restore_test.go`) pins that `clearPVCBinding` leaves `spec.selector` untouched whether it's set or absent going in, and on a real cluster the source PVC (`cirros-test-disk`) has `spec.selector == nil` before backup — kubevirt-datamover-backed PVCs never carry a selector in practice, not just in theory. - VirtualMachineBackup/VirtualMachineBackupTracker RIA plugin - Simple RIA that discards VMB/VMBT resources on restore - We don't want to restore these because they would kick off another VMBackup action. @@ -102,15 +103,15 @@ All 6 registered in `main.go` (kubevirt-datamover-plugin repo). - Save any required metadata to identify the stored data (collection of qcow2 pathnames/checkpoints, etc.), along with identifying the backup and VirtualMachine they're associated with. Save this metadata file as well (see [Where to store qcow2 files](#wherehow-to-store-qcow2-files-and-metadata) below) - We need to properly handle cases where we attempt an incremental backup but a full backup is taken instead (checkpoint lost, CSI snapshot restore since last checkpoint, VM restart, etc.) - Aborted backups also need to be handled (resulting in a failed PVC backup on the Velero side) - - **VMB/VMBT lifecycle, as implemented**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). **Gap, but currently intentional-by-inaction**: on a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the ~20 `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned unconditionally. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") is closed as completed and covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`), but the failure-path VMB deletion it also proposed was never implemented — #12 should be treated as partially delivered, not as having resolved this. **Tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), currently unplanned (2026-08-06)**: per kaovilai, leaving the VMB in place on Failed is the actual intent today, specifically to aid debugging failed backups — not simply an unfixed oversight. Automatic cleanup on Failed is not being pursued as-is; a *configurable* cleanup option (e.g. an opt-in flag/annotation for operators who don't want debugging artifacts left behind) is a possible future consideration, not committed work. VMBT retention (never deleted, on any path) remains intentional and is not part of this discussion either way. + - **Implementation note**: the uploader pod deletes the VMB itself on success (after the S3 upload completes); the controller (`cleanupVMBackupResources`) deletes the VMB on cancel. VMBT is *never* deleted by either path — intentionally kept so KubeVirt can reuse it across VM restarts/migrations to redefine libvirt checkpoints (issue #32). On a genuine `Failed` (not `Canceled`) DataUpload, nothing deletes the VMB — `cleanupVMBackupResources` is only called from `handleCanceling`, and none of the `DataUploadPhaseFailed` transition sites in `kubevirt_dataupload_controller.go` delete it, so it is left orphaned. This is intentional: [kaovilai] leaving the VMB in place on a genuine failure aids debugging. Issue #12 ("Phase 5: Complete cleanup handling and VMB/VMBT S3 archival") covers the success-path half (S3 archival, pod self-deletes VMB, controller reads archived `vmbt.json`) but not this failure path. A *configurable* opt-in cleanup option is a possible future consideration, tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168) (currently unplanned). VMBT retention (never deleted, on any path) is separately intentional. - DataDownload reconciler (restore) - Identify the VM from the DD. - Pull BSL metadata for the VM and backup - - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated so far), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore is implemented, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. + - Once this DD reaches Completed, check whether every other DataDownload matching this VM's correlation annotations has also completed; if so, restore the VM's original run state (stashed by the VM RIA — see above). **Current scope boundary**: this check only considers DataDownloads it currently knows about, not an independently-verified expected-volume-count for the VM — race-free for single-disk VMs (the only case validated), but not yet safe for multi-disk VMs if their DataDownloads could be created in a staggered fashion. **Design requirement for multi-disk support** ([kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phase 4): before multi-disk restore is implemented, the controller must gate on an explicit expected-volume-count signal (from the VM spec or the plugin) and *reject or hold* automatic run-state restoration until that count is satisfied — it must not resume opportunistically just because every *currently discovered* DataDownload is Completed. Single-disk VMs are unaffected by this requirement (expected=discovered=1 trivially) and keep today's completion-gated behavior. - Create the temporary PVC to download the qcow2 files onto. - PV here is also temporary - PVC size based on the size of the qcow2 files in BSL needed for restore as well as the PVC sizes - - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **In-flight, not yet on oadp-dev HEAD** (part of PR #124, unmerged): scratch/work/output PVC sizes will derive from the backup index's recorded *bound-PV actual capacity*, not requested size, to avoid undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). **Currently on `oadp-dev` (merged, but not shipped to any customer — `main`/`oadp-dev` has no release yet)**: the DataUpload/backup-only precursor is merged, and every backup manifest produced with it today records *requested* size, not bound-PV capacity. **Compat gap**: the restore-side floor (`maxDiskSizeFromIndex`) floors the manifest's recorded size against the restore target's own requested size — the same value — so it does not protect against the exact backend-bump-above-request scenario the fix was designed for. Any backup taken with the currently-merged code could produce an undersized scratch PVC on restore once #124 lands, since #124 does not retroactively correct already-stored manifests; a migration/compat note (and likely a fallback for pre-fix manifests) is needed before #124 merges. + - For each PVC, calculate the sum of all qcow2 files added to the PVC size, and then add 10% as a buffer. If there are multiple PVCs, take the max value, as we can process one PVC at a time, so we don't need to hold files for all PVCs on the temp disk at the same time. **In-flight, not yet on oadp-dev HEAD** (part of PR #124, unmerged): scratch/work/output PVC sizes will derive from the backup index's recorded *bound-PV actual capacity*, not requested size, to avoid undersizing from storage-backend rounding (e.g. AWS EBS 1GiB minimum). **Currently on `oadp-dev` (merged, but not shipped to any customer — `main`/`oadp-dev` has no release yet)**: the DataUpload/backup-only precursor is merged, and every backup manifest produced with it today records *requested* size, not bound-PV capacity. **Compat gap**: the restore-side floor (`maxDiskSizeFromIndex`) floors the manifest's recorded size against the restore target's own requested size — the same value — so it does not protect against the exact backend-bump-above-request scenario the fix targets. Any backup taken with the currently-merged code could produce an undersized scratch PVC on restore once #124 lands, since #124 does not retroactively correct already-stored manifests; a migration/compat note (and likely a fallback for pre-fix manifests) is needed before #124 merges. - Create temporary PVCs for each PVC in the VM (identified from BSL metadata). - These need to be mounted as block mode volumes. - PV will be bound to workload PVCs after restore, similar to velero datamover. @@ -247,17 +248,17 @@ Per-Backup-oer-vm Manifest (manifests//.json): - We could use kopia on top of the object storage API, but it is not clear that this will provide any real benefits, since we're already working with files that represent just the data diff we need. We can just manage them as individual objects. - This will also require additional overhead around kopia maintenance, and we still may need to manage qcow2 file deletion manually. -### E2E coverage (as of 2026-08-06) +### E2E coverage -`tests/e2e/virt_backup_restore_suite_test.go`, verified on AWS + community HCO/KubeVirt. +`tests/e2e/virt_backup_restore_suite_test.go`, run on AWS + community HCO/KubeVirt. -**Critical caveat — "PASS" below means structural/status verification only, not data integrity.** Every restore scenario asserted VM `Running`, DataDownload `Completed`, block `volumeMode`, and `spec.selector` state — none of it reads back the actual restored disk contents. There is no checksum or known-file write/read check anywhere in oadp-operator's e2e suite for kubevirt-datamover today, so a restore that reports "success" could still have corrupted or wrong data on disk and none of these tests would catch it. Real data verification (checksum or known-file via `virsh`/in-guest exec, pre/post restore) is being added now; PR #124 and #44 have a **hold** placed on them pending it, and the "PASS" claims below should not be read as data-integrity-verified until that lands. +**Critical caveat — "PASS" below means structural/status verification only, not data integrity.** Every restore scenario asserts VM `Running`, DataDownload `Completed`, block `volumeMode`, and `spec.selector` state — none of it reads back the actual restored disk contents. There is no checksum or known-file write/read check anywhere in oadp-operator's e2e suite for kubevirt-datamover, so a restore that reports "success" could still have corrupted or wrong data on disk and none of these tests would catch it. Real data verification (checksum or known-file via `virsh`/in-guest exec, pre/post restore) is being added; PR #124 and #44 have a **hold** placed on them pending it, and the "PASS" claims below should not be read as data-integrity-verified until that lands. - PASS (status-only): multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). -- PASS (status-only): full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full (validated prior session, referenced as green in the current PR description; not independently rerun this session). **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below); verified by grepping the whole e2e suite, there is zero test coverage for the manual annotation today. -- PASS (status-only): restore from a full kubevirt-datamover CBT backup (verified twice this session, including after the RBAC fix). +- PASS (status-only): full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full. **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below), which has zero test coverage today. +- PASS (status-only): restore from a full kubevirt-datamover CBT backup. - Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). -- No flakes observed across 4 runs this session (small sample — not a long-term flake-free claim). +- No flakes observed in current test runs (small sample — not a long-term flake-free claim). ### Open questions - How to determine PVC size? @@ -266,14 +267,13 @@ Per-Backup-oer-vm Manifest (manifests//.json): - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space. - Since controller is responsible for PVC creation rather than plugin, the controller may be able to respond to PVC too small errors by retrying with a larger PVC. - [alitke] The safest approach is to create a PVC that is 5% larger than the combined size of all disks to be backed up. - - **IN-FLIGHT, NOT YET ON oadp-dev (2026-08-06)**: PR #124 (unmerged) derives PVC sizing from recorded bound-PV actual capacity instead of requested size — see DataDownload reconciler above. Backups taken with the code currently on `oadp-dev` (itself unshipped to any customer) record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before merge. + - **Answer**: PVC sizing derives from the backup index's recorded bound-PV actual capacity instead of requested size (PR #124, unmerged — see DataDownload reconciler above). Backups taken with the code currently on `oadp-dev` record requested size, and the fix doesn't retroactively correct their manifests, so a compat/migration note is needed before #124 merges. - The kubevirt datamover controller will be responsible for deleting the `VirtualMachineBackup` resource once it's no longer needed. When should this happen? Upon velero backup deletion? This would enable debugging in the case of failed operations. If we delete it immediately, that will make troubleshooting more difficult. If on backup deletion, we'll need to write a `DeleteItemAction` plugin. [alitke] The VirtualMachineBackup resource should be deleted after the data mover has completed. It no longer has any use and accumulating these on-cluster will harm usability. Perhaps completed ones could be garbage collected by the KubeVirt DataMover Controller. - - **PARTIALLY RESOLVED (2026-08-06)**: uploader pod deletes VMB on success, controller deletes it on cancel; VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). **On genuine `Failed` (not canceled), the VMB is orphaned — currently intentional**, not just an unfixed gap: per kaovilai, leaving it in place aids debugging failed backups. Tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168), currently unplanned; a configurable opt-in cleanup is a possible future consideration. See DataUpload reconciler above. + - **Answer**: the uploader pod deletes the VMB on success; the controller deletes it on cancel. VMBT is never deleted (kept for KubeVirt to reuse across VM lifecycle events). On genuine `Failed` (not canceled), the VMB is left orphaned — intentionally: [kaovilai] leaving it in place aids debugging failed backups. A configurable opt-in cleanup is a possible future consideration, tracked as [kubevirt-datamover-controller#168](https://github.com/migtools/kubevirt-datamover-controller/issues/168) (currently unplanned). See DataUpload reconciler above. - Do we need an option to force full backups? If we're always doing incremental, eventually the incremental backup list becomes really long, requiring applying possibly hundreds of incremental files for a single restore. - For initial release, we can add a force-full-virt-backup annotation on the velero backup. Longer-term, we can push for a general datamover feature in velero which could force full backups for both fs-backup and velero datamover if backup.Spec.ForceFullVolumeBackup is true, and once implemented, the qcow2 datamover can use this as well. - - **SUPERSEDED**: the annotation-on-the-Velero-Backup proposal above is not what was implemented. What's actually implemented is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object), honored as `VMB.Spec.ForceFullBackup`. There is no Velero-Backup-level annotation — operators must annotate the DataUpload (or whatever object the BIA plugin exposes this through), not the Backup, to force a full backup. **Provenance**: introduced in [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) as one of several Phase 4 features in a squashed commit — no PR discussion, commit message, or linked issue documents *why* DataUpload-level was chosen over Backup-level; this deviation's rationale is not recoverable from git/GitHub history and should be attributed as undocumented rather than inferred. - - **PARTIALLY RESOLVED (2026-08-06)**: implemented via the `kubevirt-datamover.io/force-full-backup` DataUpload annotation honored as `VMB.Spec.ForceFullBackup` (per kdm-controller `pkg/common/constants.go`). **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. -- **NEW (2026-08-06): how should `pvcSizes` manifest semantics be versioned across the requested-size (currently merged into `oadp-dev`) and bound-PV-capacity (PR #124, unmerged) writers?** No plan exists yet — kdm-controller offered this as an unreviewed sketch, explicitly not a commitment: add a `schemaVersion` (or a narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` treats its *absence* as "legacy, requested-size" and does not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — i.e. fail open to the more conservative interpretation rather than silently trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. + - **Answer**: the annotation-on-the-Velero-Backup proposal above is not what's implemented. The actual mechanism is a `kubevirt-datamover.io/force-full-backup` annotation on the **DataUpload** (a different object, per `pkg/common/constants.go`), honored as `VMB.Spec.ForceFullBackup` — operators annotate the DataUpload, not the Backup, to force a full backup. The rationale for DataUpload-level vs. Backup-level is undocumented: [kubevirt-datamover-controller#13](https://github.com/migtools/kubevirt-datamover-controller/pull/13) (mpryc) introduced it as one of several Phase 4 features in a squashed commit, with no discussion of the tradeoff in the PR, commit message, or linked issues. **Gap**: zero e2e coverage for this specific manual annotation — the only e2e-tested force-full path is the *automatic* `max-incremental-backups` threshold trigger (a different, unrelated annotation), see E2E coverage above. +- How should `pvcSizes` manifest semantics be versioned across the requested-size (currently merged into `oadp-dev`) and bound-PV-capacity (PR #124, unmerged) writers? No plan exists yet. One (unreviewed) sketch: add a `schemaVersion` (or narrower `sizeSemantics: "requested"|"boundPV"`) field to the per-VM backup index; on restore, `maxDiskSizeFromIndex` would treat its *absence* as "legacy, requested-size" and not trust the recorded number as a bound-PV-capacity floor for the backend-bump scenario — failing open to the more conservative interpretation rather than trusting an old number as if it were the larger bound-PV value. This needs real design review, not a unilateral decision — see the manifest schema note above. ### General notes - SnapshotMoveData must be true on the backup or DU/DD processing won't work properly From e7761ac2f0a2d5dd9dc9108dd634ff27c46e1e5e Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 7 Aug 2026 02:03:37 -0400 Subject: [PATCH 18/18] docs: e2e now hard-verifies restore data integrity for full backups 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 --- docs/design/kubevirt-datamover.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 12297853fbb..2bbd239abe6 100644 --- a/docs/design/kubevirt-datamover.md +++ b/docs/design/kubevirt-datamover.md @@ -252,11 +252,9 @@ Per-Backup-oer-vm Manifest (manifests//.json): `tests/e2e/virt_backup_restore_suite_test.go`, run on AWS + community HCO/KubeVirt. -**Critical caveat — "PASS" below means structural/status verification only, not data integrity.** Every restore scenario asserts VM `Running`, DataDownload `Completed`, block `volumeMode`, and `spec.selector` state — none of it reads back the actual restored disk contents. There is no checksum or known-file write/read check anywhere in oadp-operator's e2e suite for kubevirt-datamover, so a restore that reports "success" could still have corrupted or wrong data on disk and none of these tests would catch it. Real data verification (checksum or known-file via `virsh`/in-guest exec, pre/post restore) is being added; PR #124 and #44 have a **hold** placed on them pending it, and the "PASS" claims below should not be read as data-integrity-verified until that lands. - -- PASS (status-only): multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). -- PASS (status-only): full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full. **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below), which has zero test coverage today. -- PASS (status-only): restore from a full kubevirt-datamover CBT backup. +- PASS, including restored data-integrity verification: full kubevirt-datamover CBT backup and restore of a Block-mode-target VM. Covers: VM halts at restore and flips back to running once its DataDownload completes; Block volumeMode asserted via the restored PVC's actual `spec.volumeMode`; a forced PVC-binding-conflict failure mode correctly rejects the DataDownload (`Failed`), leaves the restore `PartiallyFailed`, and keeps the VM halted rather than silently starting it; source PVC `spec.selector` confirmed `nil`. Data integrity is hard-asserted with a checksum test: a known payload is written directly to the source PVC (`dd oflag=direct conv=fsync`), bracket-verified with `iflag=direct` reads immediately before and after the backup window to confirm that region was quiescent, then the same region is checksummed on the restored PVC — a mismatch hard-fails the test. **Known limitation**: this checksum method only covers full backups — the host-side `dd` write bypasses qemu's CBT dirty-bitmap, so it can't validate incremental-chain correctness yet; `TODO` markers are in place in the e2e code for that follow-up, which needs a guest-agent-equipped fixture for a real CBT-tracked write. +- PASS (status-only, no restored-data-content assertion): multi-PVC VM backup/restore via generic CSI-datamover (Velero built-in, not the kubevirt-datamover-specific path). +- PASS (status-only, no restored-data-content assertion): full → incremental → VM-restart-preserves-checkpoint-chain → the per-VM `kubevirt-datamover.io/max-incremental-backups` limit forces the *next* backup to fall back to full. **This is the automatic threshold-triggered path only** — it is a distinct mechanism from the manual `kubevirt-datamover.io/force-full-backup` DataUpload annotation (see Open questions below), which has zero test coverage today. - Known gaps (scaffolded `ginkgo.PIt`, blocked upstream — not flakes): multi-PVC restore from a CBT backup, and restore from an incremental CBT backup (both blocked on [kubevirt-datamover-controller#73](https://github.com/migtools/kubevirt-datamover-controller/issues/73) phases 4/5); the `maxIncrementalBackups=0` checkpoint-delete sub-case is blocked on CNV-85377 (virt-controller never falls back to full, VMB hangs `Initializing`). - No flakes observed in current test runs (small sample — not a long-term flake-free claim).