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

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java

@@ -25,10 +25,12 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Objects;
+import java.util.Set;
 
 import org.apache.cloudstack.backup.BackupAnswer;
 import org.apache.cloudstack.backup.RestoreBackupCommand;
@@ -431,23 +433,30 @@ 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" };
-        // 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);
-        // 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();
-        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));
+        // virsh runs on its own so its exit code is checked; behind a pipe only the last command's is.
+        // --details adds the Device column (disk/cdrom) used below to skip the cdrom slots.
+        String[] domblkCmd = new String[] { Script.getExecutableAbsolutePath("virsh"), "domblklist", "--domain", vmName, "--details" };
+        Pair<Integer, String> result = Script.executePipedCommands(Collections.singletonList(domblkCmd), 0);
+        if (result.first() == null || result.first() != 0 || result.second() == null) {
+            throw new CloudRuntimeException(String.format("Failed to list the disks of VM [%s] to determine the device to attach the restored volume to.", vmName));
         }
-        char lastChar = currentDevice.charAt(currentDevice.length() - 1);
-        char incrementedChar = (char) (lastChar + 1);
-        return currentDevice.substring(0, currentDevice.length() - 1) + incrementedChar;
+        // Rows are "<Type> <Device> <Target> <Source>". Only virtio (vd*) targets are collected: IDE/SATA
+        // disks cannot be hot-plugged. domblklist follows the domain XML order, where hot-plugged disks
+        // are appended, so the last target is not necessarily the highest one; pick the first free name.
+        Set<String> usedDevices = new HashSet<>();
+        for (String line : result.second().split("\\R")) {
+            String[] columns = line.trim().split("\\s+");
+            if (columns.length >= 3 && "disk".equals(columns[1]) && columns[2].startsWith("vd")) {
+                usedDevices.add(columns[2]);
+            }
+        }
+        for (char c = 'a'; c <= 'z'; c++) {
+            String device = "vd" + c;
+            if (!usedDevices.contains(device)) {
+                return device;
+            }
+        }
+        throw new CloudRuntimeException(String.format("No free virtio device is left to attach the restored volume to on VM [%s].", vmName));
     }
 
     private String getXmlForRbdDisk(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String deviceToAttachDiskTo) {

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java

@@ -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;
@@ -628,42 +629,78 @@ private String[] captureAttachCommand(Storage.StoragePoolType poolType) throws E
         return captured[0];
     }
 
-    @Test
-    public void testGetDeviceToAttachDiskTrimsOutputBeforeIncrementing() throws Exception {
+    private static String domblklistOutput(String... rows) {
+        String sep = System.lineSeparator();
+        StringBuilder output = new StringBuilder(" Type   Device   Target   Source").append(sep)
+                .append("------------------------------------------------").append(sep);
+        for (String row : rows) {
+            output.append(row).append(sep);
+        }
+        return output.toString();
+    }
+
+    private String getDeviceToAttachDiskFor(String domblklistOutput) throws Exception {
         try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
             scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
                     .thenAnswer(invocation -> invocation.getArgument(0));
-            // executePipedCommands appends a line separator to each line it reads.
             scriptMock.when(() -> Script.executePipedCommands(anyList(), anyLong()))
-                    .thenReturn(new Pair<>(0, "vda" + System.lineSeparator()));
-
-            Assert.assertEquals("vdb", invokeGetDeviceToAttachDisk("test-vm"));
+                    .thenReturn(new Pair<>(0, domblklistOutput));
+            return invokeGetDeviceToAttachDisk("test-vm");
         }
     }
 
     @Test
-    public void testGetDeviceToAttachDiskPassesUnquotedAwkProgram() throws Exception {
+    public void testGetDeviceToAttachDiskRunsOnlyDomblklistWithDetails() throws Exception {
         try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
             scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
                     .thenAnswer(invocation -> invocation.getArgument(0));
-            final List<String[]>[] captured = new List[1];
+            final AtomicReference<List<String[]>> captured = new AtomicReference<>();
             scriptMock.when(() -> Script.executePipedCommands(anyList(), anyLong()))
                     .thenAnswer(invocation -> {
-                        captured[0] = invocation.getArgument(0);
-                        return new Pair<>(0, "vda" + System.lineSeparator());
+                        captured.set(invocation.getArgument(0));
+                        return new Pair<>(0, domblklistOutput(" file   disk     vda      /path/root"));
                     });
 
-            invokeGetDeviceToAttachDisk("test-vm");
+            Assert.assertEquals("vdb", invokeGetDeviceToAttachDisk("test-vm"));
 
-            String[] awkCmd = captured[0].get(captured[0].size() - 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]);
+            // virsh must be the only command, so the exit code checked is its own and not that of a
+            // command it is piped into. --details adds the Device column used to skip cdrom rows.
+            Assert.assertEquals(1, captured.get().size());
+            Assert.assertArrayEquals(new String[] { "virsh", "domblklist", "--domain", "test-vm", "--details" }, captured.get().get(0));
         }
     }
 
+    @Test
+    public void testGetDeviceToAttachDiskSkipsCdromAndIdeDisks() throws Exception {
+        // IDE root disk and cdroms, as on a VM with rootDiskController=ide; vda is not taken by the root disk.
+        Assert.assertEquals("vda", getDeviceToAttachDiskFor(domblklistOutput(
+                " file   disk     hda      /path/root",
+                " file   cdrom    hdc      -",
+                " file   cdrom    hdd      -",
+                " file   disk     vdb      /path/data")));
+    }
+
+    @Test
+    public void testGetDeviceToAttachDiskUsesVdaWhenVmHasNoVirtioDisk() throws Exception {
+        // hdb would be picked otherwise, and IDE disks cannot be hot-plugged.
+        Assert.assertEquals("vda", getDeviceToAttachDiskFor(domblklistOutput(
+                " file   disk     hda      /path/root",
+                " file   cdrom    hdc      -",
+                " file   cdrom    hdd      -")));
+    }
+
+    @Test
+    public void testGetDeviceToAttachDiskDoesNotRelyOnTargetOrder() throws Exception {
+        // vdb was detached and re-attached, so it is listed after vdc and vdd.
+        Assert.assertEquals("vde", getDeviceToAttachDiskFor(domblklistOutput(
+                " file   disk     vda      /path/root",
+                " file   disk     vdc      /path/data1",
+                " file   disk     vdd      /path/data2",
+                " block  disk     vdb      /dev/data3")));
+    }
+
     @Test(expected = CloudRuntimeException.class)
-    public void testGetDeviceToAttachDiskFailsWhenNoDeviceIsReturned() throws Exception {
+    public void testGetDeviceToAttachDiskFailsWhenDomblklistFails() throws Exception {
         try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
             scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
                     .thenAnswer(invocation -> invocation.getArgument(0));