Skip to content

kvm: fix restore-attach picking a CD-ROM slot as the target device (NAS backup) #14266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
slavkap wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −7
Open

kvm: fix restore-attach picking a CD-ROM slot as the target device (NAS backup) #14266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
132f351
kvm: fix restore-attach picking a CD-ROM slot as the target device
slavkap Sep 29, 2026
c737927
address copilot comments
slavkap Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions ...in/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,14 +431,16 @@ private boolean attachRbdVolumeToVm(KVMStoragePoolManager storagePoolMgr, String
}

private String getDeviceToAttachDisk(String vmName) {
String[] domblkCmd = new String[] { Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName };
String[] tailCmd = new String[] { Script.getExecutableAbsolutePath("tail"), "-n", "3" };
String[] headCmd = new String[] { Script.getExecutableAbsolutePath("head"), "-n", "1" };
// --details adds a Type column so cdrom slots (hdc, hdd) can be filtered out — they sort

@abh1sar abh1sar Sep 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is not right. --details doesn't change the ordering in any way:


[root@ref-trl-12455-k-Mr9-abhisar-sinha-kvm1 ~]# virsh  domblklist --domain  i-2-5-VM --details
 Type   Device   Target   Source
----------------------------------------------------------------------------------------------------------
 file   disk     vda      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/250bd073-3a2c-4875-9ba0-29bf632801b7
 file   disk     vdb      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/6f259e43-74a5-461e-b857-e945bcff7b45
 file   disk     vdc      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/bca15b84-3acf-4b1d-8466-3866e563c21f
 file   cdrom    hdc      -
 file   cdrom    hdd      -

[root@ref-trl-12455-k-Mr9-abhisar-sinha-kvm1 ~]# virsh  domblklist --domain  i-2-5-VM 
 Target   Source
------------------------------------------------------------------------------------------
 vda      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/250bd073-3a2c-4875-9ba0-29bf632801b7
 vdb      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/6f259e43-74a5-461e-b857-e945bcff7b45
 vdc      /mnt/eaae5267-a60a-369d-a4da-a13db16e3aa2/bca15b84-3acf-4b1d-8466-3866e563c21f
 hdc      -
 hdd      -

[root@ref-trl-12455-k-Mr9-abhisar-sinha-kvm1 ~]#

--details does add the type Device column (not Type) which awk $2==\"disk\" uses to filter by type.

// alphabetically ahead of virtio disks, so without this the selected row is a cdrom, not
// an actual disk.
String[] domblkCmd = new String[] { Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName, "--details" };
Comment thread
slavkap marked this conversation as resolved.
// The commands are executed without a shell, so the awk program must be passed as a plain
// argument. Keeping the quotes a shell would have stripped makes awk fail with
// "invalid char" and produce no output.
String[] awkCmd = new String[] { Script.getExecutableAbsolutePath("awk"), "{print $1}" };
Pair<Integer, String> result = Script.executePipedCommands(Arrays.asList(domblkCmd, tailCmd, headCmd, awkCmd), 0);
String[] awkCmd = new String[] { Script.getExecutableAbsolutePath("awk"), "$2==\"disk\"{print $3}" };
String[] tailCmd = new String[] { Script.getExecutableAbsolutePath("tail"), "-n", "1" };
Pair<Integer, String> result = Script.executePipedCommands(Arrays.asList(domblkCmd, awkCmd, tailCmd), 0);
// executePipedCommands appends a line separator to every line it reads, so the device
// name has to be trimmed before the last character can be incremented.
String currentDevice = result.second() == null ? "" : result.second().trim();
Expand Down
28 changes: 26 additions & 2 deletions ...ava/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;

import org.apache.cloudstack.backup.BackupAnswer;
import org.apache.cloudstack.backup.RestoreBackupCommand;
Expand Down Expand Up @@ -655,10 +656,33 @@ public void testGetDeviceToAttachDiskPassesUnquotedAwkProgram() throws Exception

invokeGetDeviceToAttachDisk("test-vm");

String[] awkCmd = captured[0].get(captured[0].size() - 1);
String[] awkCmd = captured[0].get(1);
// The commands are executed without a shell, so the program must carry no shell quotes.
Assert.assertEquals("awk", awkCmd[0]);
Assert.assertEquals("{print $1}", awkCmd[1]);
Assert.assertEquals("$2==\"disk\"{print $3}", awkCmd[1]);
}
}

@Test
public void testGetDeviceToAttachDiskRequestsDomblklistDetails() throws Exception {
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
.thenAnswer(invocation -> invocation.getArgument(0));
final AtomicReference<List<String[]>> captured = new AtomicReference<>();
scriptMock.when(() -> Script.executePipedCommands(anyList(), anyLong()))
.thenAnswer(invocation -> {
captured.set(invocation.getArgument(0));
return new Pair<>(0, "vdb" + System.lineSeparator());
});

Assert.assertEquals("vdc", invokeGetDeviceToAttachDisk("test-vm"));

// --details adds the Type column the awk program filters on (verified separately in
// testGetDeviceToAttachDiskPassesUnquotedAwkProgram); without it cdrom rows can't be
// told apart from real disks at all.
String[] domblkCmd = captured.get().get(0);
Assert.assertTrue("domblklist must request --details so the Type column is available to filter on",
Arrays.asList(domblkCmd).contains("--details"));
}
}

Expand Down
Loading