add workflow to use cnv qcow2 cbt #2055
Conversation
WalkthroughAdds a Cirros CNV-CBT e2e sample: documentation, three KubeVirt VM manifests, PV/PVC for backup storage, optional filebrowser, VirtualMachineBackupTracker and VirtualMachineBackup (full + incremental) manifests, plus RESULT files capturing backup outcomes and notes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.md`:
- Line 15: Fix the typo in the README: replace the incorrect string "qocw2" with
the correct disk format name "qcow2" in the README.md text ("3. Create the
pv/pvc that will store the qocw2 snapshots") so the line reads "3. Create the
pv/pvc that will store the qcow2 snapshots".
- Around line 33-39: Fix the typos in the backup section of README.md: change
"andn" to "and" in the sentence "Take a full andn incremental backup", correct
"Incremetnal" to "Incremental", and consider fixing "any new backup using the
same tracker will be incremental" to a clearer phrasing like "Any subsequent
backup using the same tracker will be incremental"; leave the oc create command
and filename virtualMachineBackupFull.yaml unchanged.
🧹 Nitpick comments (8)
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-3.yaml (1)
4-5: Remove empty annotations field and server-managedgenerationfield.The
annotations:key without a value andgeneration: 1are likely artifacts from akubectl getexport. For a clean source manifest, remove server-managed fields.Suggested fix
metadata: - annotations: - generation: 1 labels: app: cirros-test-cont-3tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-1.yaml (1)
4-5: Remove empty annotations field and server-managedgenerationfield.Same as cirros-test-3.yaml - these appear to be artifacts from
kubectl getexport rather than intentional source manifest content.Suggested fix
metadata: - annotations: - generation: 1 labels: app: cirros-test-cont-1tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-2.yaml (3)
4-5: Remove emptyannotationskey and unnecessarygenerationfield.The
annotations:key on line 4 is empty and serves no purpose. Thegeneration: 1field is auto-managed by Kubernetes and should be removed from the manifest to avoid confusion.Suggested cleanup
metadata: - annotations: - generation: 1 labels: app: cirros-test-cont-2
13-17: Remove auto-generated metadata fields from dataVolumeTemplate.
creationTimestamp: nullis auto-populated by Kubernetes and should be omitted from declarative manifests. The emptyannotations:before the actual annotation is also redundant.Suggested cleanup
- metadata: annotations: cdi.kubevirt.io/storage.deleteAfterCompletion: "false" - creationTimestamp: null name: cirros-test-cont-2-dv
32-34: Remove unnecessarycreationTimestamp: nullfrom template metadata.Same as above - this field is auto-managed by Kubernetes.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/pushBackupPV.yaml (1)
29-29: Add newline at end of file.The file is missing a trailing newline, which is a common convention for text files.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/filebrowser.yaml (2)
18-18: Avoid using:latesttag for reproducibility.Using
:latestmakes builds non-deterministic and could unexpectedly introduce breaking changes or vulnerabilities. Pin to a specific version.Suggested fix
- image: docker.io/filebrowser/filebrowser:latest + image: docker.io/filebrowser/filebrowser:v2.31.2
16-34: Consider adding securityContext to harden the container.Static analysis flagged missing security constraints. Adding a
securityContextprevents privilege escalation and runs as non-root, which is good practice even for test workloads.Suggested addition
containers: - name: browser image: docker.io/filebrowser/filebrowser:latest + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + capabilities: + drop: + - ALL env: - name: FB_NOAUTH
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.mdtests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/NOTEStests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_fulltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_inc1tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_tracker.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-1.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-2.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-3.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/filebrowser.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/pushBackupPV.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupFull.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupInc1.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupTracker.yaml
🧰 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:
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/NOTEStests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupInc1.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_inc1tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-3.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/pushBackupPV.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupTracker.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-2.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_fulltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.mdtests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupFull.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/filebrowser.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_tracker.yamltests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/cirros-test-1.yaml
🪛 Checkov (3.2.334)
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/filebrowser.yaml
[medium] 1-45: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-45: Minimize the admission of root containers
(CKV_K8S_23)
🪛 LanguageTool
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.md
[grammar] ~33-~33: Ensure spelling is correct
Context: ...eBackupTracker.yaml ``` 6. Take a full andn incremental backup Full: the first bac...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.md
4-4: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/NOTES (1)
1-8: LGTM!Useful documentation showing the size difference between full and incremental backups, demonstrating CBT effectiveness.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_tracker.yaml (1)
1-18: LGTM!Appropriate as a captured resource state documenting the expected tracker status after backup operations.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupTracker.yaml (1)
1-11: LGTM!Clean manifest with proper structure for the VirtualMachineBackupTracker resource.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupFull.yaml (1)
1-11: LGTM!The VirtualMachineBackup manifest is correctly structured with proper references to the backup tracker and target PVC.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/pushBackupPV.yaml (1)
14-15:hostPathlimits this to single-node clusters.The
hostPathvolume type only works reliably on single-node clusters since it references local node storage. This is acceptable for local testing but won't function correctly in multi-node environments. Consider documenting this limitation in the README if not already mentioned.tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/virtualMachineBackupInc1.yaml (1)
1-11: LGTM!The incremental backup manifest follows the same correct structure as the full backup, with appropriate naming to distinguish it.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_full (1)
1-36: LGTM - Expected warning about guest agent.This result file correctly documents the backup output. The warning about "QEMU guest agent is not connected" (lines 24-26, 31-33) is expected behavior since Cirros images don't include the qemu-guest-agent package. The backup completes successfully despite being unable to freeze the guest filesystem. This is useful documentation for developers testing the CNV-CBT workflow.
tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/RESULTS/vmbackup_results_inc1 (1)
1-36: LGTM - useful reference output.This sample output effectively documents the expected behavior, including the guest agent warning which is expected for Cirros VMs without QEMU guest agent installed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| oc create -f cirros-test-1.yaml , can also include test-2,3 | ||
| ``` | ||
|
|
||
| 3. Create the pv/pvc that will store the qocw2 snapshots |
There was a problem hiding this comment.
Typo: "qocw2" should be "qcow2".
-3. Create the pv/pvc that will store the qocw2 snapshots
+3. Create the pv/pvc that will store the qcow2 snapshots📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. Create the pv/pvc that will store the qocw2 snapshots | |
| 3. Create the pv/pvc that will store the qcow2 snapshots |
🤖 Prompt for AI Agents
In
`@tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.md`
at line 15, Fix the typo in the README: replace the incorrect string "qocw2"
with the correct disk format name "qcow2" in the README.md text ("3. Create the
pv/pvc that will store the qocw2 snapshots") so the line reads "3. Create the
pv/pvc that will store the qcow2 snapshots".
| 6. Take a full andn incremental backup | ||
|
|
||
| Full: the first backup will always be a full backup | ||
| ``` | ||
| oc create -f virtualMachineBackupFull.yaml | ||
| ``` | ||
| Incremetnal: any new backup using the same tracker will be incremental |
There was a problem hiding this comment.
Typos in backup section.
Suggested fixes
-6. Take a full andn incremental backup
+6. Take a full and incremental backup-Incremetnal: any new backup using the same tracker will be incremental
+Incremental: any new backup using the same tracker will be incremental📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 6. Take a full andn incremental backup | |
| Full: the first backup will always be a full backup | |
| ``` | |
| oc create -f virtualMachineBackupFull.yaml | |
| ``` | |
| Incremetnal: any new backup using the same tracker will be incremental | |
| 6. Take a full and incremental backup | |
| Full: the first backup will always be a full backup |
🧰 Tools
🪛 LanguageTool
[grammar] ~33-~33: Ensure spelling is correct
Context: ...eBackupTracker.yaml ``` 6. Take a full andn incremental backup Full: the first bac...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In
`@tests/e2e/sample-applications/virtual-machines/cirros-test/3-vms/cirros-test-with-CNV-CBT/README.md`
around lines 33 - 39, Fix the typos in the backup section of README.md: change
"andn" to "and" in the sentence "Take a full andn incremental backup", correct
"Incremetnal" to "Incremental", and consider fixing "any new backup using the
same tracker will be incremental" to a clearer phrasing like "Any subsequent
backup using the same tracker will be incremental"; leave the oc create command
and filename virtualMachineBackupFull.yaml unchanged.
|
/retest |
|
/test 4.21-e2e-test-aws |
|
@weshayutin: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| # get this party started | ||
|
|
||
| 1. ensure you have a default storage class selected | ||
| ``` |
There was a problem hiding this comment.
Note: command to set rbd as default is as follows:
oc patch storageclass ocs-storagecluster-ceph-rbd -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}'| ocs-storagecluster-ceph-rbd (default) openshift-storage.rbd.csi.ceph.com Delete Immediate true 7h50m | ||
| ``` | ||
|
|
||
| 2. Create the VM's |
There was a problem hiding this comment.
Before creating the VMs, ensure you already have the NS created for e.g. oc create ns cirros-test-cont
| 4. Create an optional filebrowsers container to inspect files on the above pv/pvc | ||
|
|
||
| ``` | ||
| oc create -f filebrowser.yaml |
There was a problem hiding this comment.
Use our own quay image to avoid getting into docker rate limiting issues:
oc set image deployment/pvc-browser browser=quay.io/konveyor/oadp-vmfr-access-filebrowser:latest| @@ -0,0 +1,11 @@ | |||
| apiVersion: backup.kubevirt.io/v1alpha1 | |||
There was a problem hiding this comment.
We will have to update this manifest if multiple VMs are used, right ?
2 participants
Why the changes were made
To test the CNV Qcow2 CBT feature and to allow OADP devs to start work
How to test the changes made
See the README.md in the new directory