Nas backup: Fix restore-and-attach of a backed up volume on NFS, Linstor and Ceph primary storages - #14007
Conversation
Restoring a volume from a backup and attaching it to a VM has been broken since the restore commands were changed to run without a shell, in three independent ways. getDeviceToAttachDisk pipes virsh domblklist through awk, but passes the awk program still wrapped in the single quotes a shell would have stripped. Run directly, awk fails with "invalid char" and returns nothing, so the device name is empty and charAt throws StringIndexOutOfBoundsException before any attach is attempted. This affects every storage type. The exit value was also never checked, and the output not trimmed, so even a working awk would leave the trailing line separator and increment that instead of the device letter. The RBD branch passes the literal string "<<EOF%sEOF" as a virsh argument. The placeholder is never substituted with the disk XML, and a here-document cannot work without a shell, so virsh is handed a bogus argument and fails. The XML is now written to a temporary file that virsh reads. The Linstor branch declares "--subdriver qcow2", inverting the previous behaviour where Linstor got a raw attach and every other pool got qcow2. A Linstor volume is a raw DRBD block device, so libvirt rejects it with "Image is not in qcow2 format". The condition is restored, along with the "--driver qemu" that was dropped.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14007 +/- ##
============================================
+ Coverage 17.79% 17.80% +0.01%
- Complexity 15995 16002 +7
============================================
Files 5928 5928
Lines 534306 534323 +17
Branches 65383 65387 +4
============================================
+ Hits 95069 95130 +61
+ Misses 428467 428418 -49
- Partials 10770 10775 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19016 |
There was a problem hiding this comment.
Pull request overview
This PR fixes KVM restore-and-attach of volumes restored from backup after prior changes switched virsh/awk invocation to run without a shell, which broke device detection and certain storage-specific attach flows (notably Ceph RBD and Linstor).
Changes:
- Fixes
getDeviceToAttachDiskso awk is invoked without shell quotes, trims output before incrementing the device letter, and fails fast when no device is detected. - Fixes Ceph RBD attach by writing the generated disk XML to a temporary file and passing that file to
virsh attach-device(instead of a non-functional here-doc argument). - Restores correct attach options by omitting
--subdriver qcow2for Linstor (raw DRBD) while using qcow2 for file-backed pools; adds--driver qemu.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java | Fixes shell-less command execution issues affecting device discovery and storage-type-specific attach behavior (RBD, Linstor). |
| plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java | Adds unit tests covering device selection trimming/awk quoting and storage-specific attach command construction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
wow, I just see this now. @abh1sar do we have a issue loggeds for this? Sounds serious for any Veeam customers relying on it. Is there anything I can help with? cc @shwstppr @weizhouapache - how did this slip the 4.23 voting 😢 |
@andrijapanicsb I think this got into the security release, and then we proceeded 4.23 as there is a workaround someone referred to. |
@andrijapanicsb this only affects Nas backup provider, and only the restore and attach single volume workflow. |
thanks @abh1sar for the explanation. @andrijapanicsb it is mentioned in the 4.23.0.0 release note: https://docs.cloudstack.apache.org/en/4.23.0.0/releasenotes/about.html |
|
Ah cool, thx @abh1sar and all, so doesn't affect Veeam, and we have a workaround. Good to know! 🙏 |
abh1sar
changed the title
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19153 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
test_backup_recovery_nas.py only allowed NFS primary storage, since it reused the primary storage pool's own path as the NAS backup repository address, and always required incremental-backup semantics that only qcow2/NFS storage can provide. Neither holds on Ceph/RBD. - setUpClass now accepts RBD alongside NFS as the primary storage pool type, picking a pool that's actually Up rather than list()[0] -- environments that added Ceph/RBD after the zone's original NFS primary storage keep that old pool around in Disabled state, and it still sorts first, silently exercising its path as if it were the storage VMs actually deploy on. - The NAS backup repository's NFS export address is resolved independently of the primary storage: when the primary pool isn't NFS, reuse the nfs test data entry (services[nfs][url]) -- the same temporary NFS mount point test_primary_storage.py uses for its temporary NFS primary storage pool, and something every marvin environment already has configured. An explicit nas_backup_repository_address test data entry or NAS_BACKUP_REPO_ADDRESS environment variable, if set, takes precedence. - The external offering imported in setUpClass is matched to the repository just created by externalid (== the repository's own id for the nas provider) rather than blindly taking index 0 -- a stray repository left over from an earlier interrupted run, whose backups didn't get cleaned up so its own teardown couldn't remove it either, sorts alongside the new one with no guarantee of which comes first. - Incremental NAS backups require QEMU dirty bitmaps / libvirt checkpoints, which only exist on file-based qcow2 storage (NASBackupProvider.allVolumesOnCheckpointCapableStorage). The six incremental-chain tests now skip on RBD/Ceph, where the provider always falls back to full-only backups server-side, rather than failing on assertions that storage type can never satisfy. - Added test_restore_volume_and_attach_to_vm, which exercises restoreVolumeFromBackupAndAttachToVM end-to-end (restoring a backed-up ROOT and DATADISK volume onto a second, stopped Instance) -- the API that drives the restore-and-attach code fixed by the previous commit (apache#14007). The target Instance is stopped with forced=True: a graceful ACPI stop was observed to time out (~2 minutes) before falling back to a hard destroy anyway, and once forced to a hard destroy the domain drops out of libvirt entirely, so the periodic ping-based PowerState sync the restore call depends on falls back to a much slower heuristic well past any reasonable wait. A forced stop destroys the domain immediately and deterministically.
|
[SF] Trillian test result (tid-16932)
|
Description
Restoring a volume from a backup and attaching it to a VM doesn't work since the restore commands were changed to run without a shell in 56ad044
getDeviceToAttachDisk pipes virsh domblklist through awk, but passes the awk program still wrapped in the single quotes a shell would have stripped. Run directly, awk fails with "invalid char" and returns nothing, so the device name is empty and charAt throws StringIndexOutOfBoundsException before any attach is attempted. This affects every storage type. The exit value was also never checked, and the output not trimmed, so even a working awk would leave the trailing line separator and increment that instead of the device letter.
The RBD branch passes the literal string "<<EOF%sEOF" as a virsh argument. The placeholder is never substituted with the disk XML, and a here-document cannot work without a shell, so virsh is handed a bogus argument and fails. The XML is now written to a temporary file that virsh reads.
The Linstor branch declares "--subdriver qcow2", inverting the previous behaviour where Linstor got a raw attach and every other pool got qcow2. A Linstor volume is a raw DRBD block device, so libvirt rejects it with "Image is not in qcow2 format". The condition is restored, along with the "--driver qemu" that was dropped.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Before fix:
API
Agent log:
Agent log:
Agent log:
After fix:
Restore and attach volume works as expected on all 3 NFS, Linstor and Ceph primary storages
How did you try to break this feature and the system with this change?