Fix restore-and-attach of a backed up volume on NFS, Linstor and Ceph primary storages - #14007
Open
abh1sar wants to merge 1 commit into
Open
Fix restore-and-attach of a backed up volume on NFS, Linstor and Ceph primary storages#14007abh1sar wants to merge 1 commit into
abh1sar wants to merge 1 commit into
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:
|
Contributor
Author
|
@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 |
abh1sar
requested review from
shwstppr and
weizhouapache
and
a lite review from Copilot
August 29, 2026 16:15
Contributor
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.
Comment on lines
+402
to
+405
| String currentDevice = result.second() == null ? "" : result.second().trim(); | ||
| if (result.first() == null || result.first() != 0 || StringUtils.isBlank(currentDevice)) { | ||
| throw new CloudRuntimeException(String.format("Failed to determine the device to attach the restored volume to on VM [%s].", vmName)); | ||
| } |
Comment on lines
+613
to
+625
| try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) { | ||
| scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString())) | ||
| .thenAnswer(invocation -> invocation.getArgument(0)); | ||
| scriptMock.when(() -> Script.executePipedCommands(anyList(), anyLong())) | ||
| .thenReturn(new Pair<>(0, "vda" + System.lineSeparator())); | ||
| scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class))) | ||
| .thenAnswer(invocation -> { | ||
| // Mockito expands varargs, so the command comes back as individual arguments. | ||
| captured[0] = Arrays.stream(invocation.getArguments()).map(String::valueOf).toArray(String[]::new); | ||
| return 0; | ||
| }); | ||
| method.invoke(wrapper, storagePoolMgr, "test-vm", volumePool, "/path/to/volume"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?