diff --git a/docs/design/kubevirt-datamover.md b/docs/design/kubevirt-datamover.md index 1f02b64a27c..2bbd239abe6 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,24 +62,33 @@ 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. + - **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 + - set selector with MatchLabels to match PV that will be created by restore controller + - **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. + - **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. @@ -88,31 +98,33 @@ Taking a VolumeSnapshot and then using kopia to process the entire volume and co - 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) + - **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), 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. + - 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 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. - - 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 @@ -126,8 +138,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) @@ -154,6 +167,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`** (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): @@ -233,6 +248,16 @@ 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 + +`tests/e2e/virt_backup_restore_suite_test.go`, run on AWS + community HCO/KubeVirt. + +- 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). + ### Open questions - How to determine PVC size? - user-configurable? configmap or annotation? @@ -240,9 +265,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. + - **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. + - **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. + - **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