Design for virt qcow2 datamover #2037

sseago · 2025-12-02T20:35:43Z

Why the changes were made

This PR adds a design document for the VM qcow2 incremental backup feature.

How to test the changes made

coderabbitai · 2025-12-02T20:35:59Z

Walkthrough

A new design documentation file for an incremental qcow2 datamover architecture integrating KubeVirt with OADP/Velero. The document specifies proposed components including BackupItemAction/RestoreItemAction plugins, a Virt Backup Controller, and a qcow2 storage/metadata strategy for QEMU CBT-based backups.

Changes

Cohort / File(s)	Summary
Design Documentation `docs/design/virt-qcow2-datamover.md`	New design document detailing incremental qcow2 datamover architecture: background and goals; BackupItemAction/RestoreItemAction plugin specifications; Virt Backup Controller workflows for backup and restore operations; qcow2 file and metadata storage strategy; DataUpload/DataDownload reconciliation expectations; open questions on PVC sizing, full backup policies, and lifecycle management.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Design scope and component interactions: The document specifies multiple components (VMI BIA/RIA plugins, Virt Backup Controller, reconcilers) with complex backup/restore workflows requiring careful validation of sequencing and state management.
Specialized domain knowledge: Review requires familiarity with KubeVirt, Velero/OADP, QEMU CBT mechanics, and CR lifecycle patterns.
Open questions and gaps: Pay attention to items flagged as open questions, particularly around PVC sizing, force full backup mechanics, and VirtualMachineBackup lifecycle assumptions.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-12-02T20:36:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sseago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sseago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (3)

docs/design/virt-qcow2-datamover.md (3)

1-25: Fix markdown formatting issues and correct spelling in intro section.

Line 13 contains a bare URL; wrap it in angle brackets or use markdown link syntax. Lines 20–22 have incorrect list indentation (4 spaces instead of 2). Line 16 has a spelling error: "mantainability" should be "maintainability".

Apply these fixes:

- Implementation based on kubevirt enhancement defined at https://github.com/kubevirt/enhancements/blob/main/veps/sig-storage/incremental-backup.md
+ Implementation based on kubevirt enhancement defined at <https://github.com/kubevirt/enhancements/blob/main/veps/sig-storage/incremental-backup.md>

- Deep integration with velero data mover pluggability (this could be considered in the long-term though, which would minimize duplication of effort and enhance mantainability)
+ Deep integration with velero data mover pluggability (this could be considered in the long-term though, which would minimize duplication of effort and enhance maintainability)

- As a user I want to use OADP to trigger backups that will back up volume data using libvirt tooling rather than CSI snapshots
-    - Volume backups will be incremental when possible (first backup for a given volume will be full, subsequent backups for that same volume will be incremental)
-    - Based on the assumption that libvirt incremental volume backups should be much faster than CSI snapshots followed by incremental kopia snapshot copy, the expectation is that users might run libvirt-based OADP backups more frequently than they would for CSI-based backups.
-    - If users are backing up more frequently with this method, they should ensure that they are not using CSI snapshots or fs-backup via the resource/volume policy configuration in the backup.
+ As a user I want to use OADP to trigger backups that will back up volume data using libvirt tooling rather than CSI snapshots
+  - Volume backups will be incremental when possible (first backup for a given volume will be full, subsequent backups for that same volume will be incremental)
+  - Assuming libvirt incremental backups are faster than CSI snapshots with incremental kopia, users may run these backups more frequently.
+  - If users are backing up more frequently with this method, they should ensure that they are not using CSI snapshots or fs-backup via the resource/volume policy configuration in the backup.

82-98: Correct spelling errors and hyphenation in storage strategy section.

Line 86 should use a hyphen: "top level" → "top-level" (compound adjective). Line 96 has a spelling error: "invidivual" should be "individual". Consider also tightening line 96–97 for clarity by replacing "I'm not sure we get any real benefits" with a more direct phrasing, and simplifying "on top of that" to "additionally" or "furthermore".

Apply these fixes:

- Create a top level dir in the BSL (under the BSL prefix, parallel to backups/restores/kopia) for libvirt datamover.
+ Create a top-level dir in the BSL (under the BSL prefix, parallel to backups/restores/kopia) for libvirt datamover.

- We could use kopia on top of the object storage API, but I'm not sure we get any real benefits, since we're already working with files that represent just the data diff we need. We can just manage them as invidivual objects
+ We could use kopia on top of the object storage API. However, since we're already working with files representing just the data diff needed, we can manage them as individual objects

- This will also require additional overhead around kopia maintenance, etc. and on top of that, we still may need to manage qcow2 file deletion manually.
+ This will also require additional overhead around kopia maintenance, and additionally, we still may need to manage qcow2 file deletion manually.

99-113: Correct spelling error in force full backup discussion.

Line 108 has a spelling error: "datmover" should be "datamover". Also apply consistent 2-space list indentation throughout lines 101–104.

Apply this fix:

- 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 datmover can use this as well.
+ 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.

For the Open Questions section, ensure nested lists use 2-space indentation:

- ### Open questions
- - How to determine PVC size?
-     - user-configurable? configmap or annotation?
-     - From the kubevirt enhancement: "Before the process begins, an estimation of the required backup size will be performed. If the provided PVC size is insufficient, an error will be returned"
-     - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space.
-     - If controller is responsible for PVC creation rather than plugin, then the controller may be able to respond to PVC too small errors by retrying with a larger PVC.
+ ### Open questions
+ - How to determine PVC size?
+   - user-configurable? configmap or annotation?
+   - From the kubevirt enhancement: "Before the process begins, an estimation of the required backup size will be performed. If the provided PVC size is insufficient, an error will be returned"
+   - If the PVC is too small, we need a clear error on the backup indicating that it failed due to insufficient PVC space.
+   - If controller is responsible for PVC creation rather than plugin, then the controller may be able to respond to PVC too small errors by retrying with a larger PVC.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 02a9165 and 43551b2.

📒 Files selected for processing (1)

docs/design/virt-qcow2-datamover.md (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

docs/design/virt-qcow2-datamover.md

🪛 LanguageTool

docs/design/virt-qcow2-datamover.md

[style] ~21-~21: ‘Based on the assumption that’ might be wordy. Consider a shorter alternative.
Context: ... same volume will be incremental) - Based on the assumption that libvirt incremental volume backups shou...

(EN_WORDINESS_PREMIUM_BASED_ON_THE_ASSUMPTION_THAT)

[grammar] ~31-~31: Ensure spelling is correct
Context: ...ckupItemAction/RestoreItemAction plugins - VitualMachineInstance BIA plugin - This plugin will check...