kvm: fix restore-and-attach of a backed up volume

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.

Author: abh1sar

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

@@ -20,6 +20,7 @@
 package com.cloud.hypervisor.kvm.resource.wrapper;
 
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -340,38 +341,68 @@ private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMg
 
     private boolean attachVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vmName, PrimaryDataStoreTO volumePool, String volumePath) {
         String deviceToAttachDiskTo = getDeviceToAttachDisk(vmName);
+        if (Storage.StoragePoolType.RBD.equals(volumePool.getPoolType())) {
+            return attachRbdVolumeToVm(storagePoolMgr, vmName, volumePool, volumePath, deviceToAttachDiskTo);
+        }
         List<String> virshCmd = new ArrayList<>();
         virshCmd.add(Script.getExecutableAbsolutePath("virsh"));
-        if (volumePool.getPoolType() == Storage.StoragePoolType.RBD) {
-            String xmlForRbdDisk = getXmlForRbdDisk(storagePoolMgr, volumePool, volumePath, deviceToAttachDiskTo);
-            logger.debug("RBD disk xml to attach: {}", xmlForRbdDisk);
-            virshCmd.add("attach-device");
-            virshCmd.add(vmName);
-            virshCmd.add("/dev/stdin");
-            virshCmd.add("<<EOF%sEOF");
-        } else {
-            virshCmd.add("attach-disk");
-            virshCmd.add(vmName);
-            virshCmd.add(volumePath);
-            virshCmd.add(deviceToAttachDiskTo);
-            if (Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
-                virshCmd.add("--subdriver");
-                virshCmd.add("qcow2");
-            }
-            virshCmd.add("--cache");
-            virshCmd.add("none");
+        virshCmd.add("attach-disk");
+        virshCmd.add(vmName);
+        virshCmd.add(volumePath);
+        virshCmd.add(deviceToAttachDiskTo);
+        virshCmd.add("--driver");
+        virshCmd.add("qemu");
+        if (!Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
+            virshCmd.add("--subdriver");
+            virshCmd.add("qcow2");
         }
+        virshCmd.add("--cache");
+        virshCmd.add("none");
         int exitValue = Script.executeCommandForExitValue(virshCmd.toArray(new String[0]));
         return exitValue == 0;
     }
 
+    private boolean attachRbdVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vmName, PrimaryDataStoreTO volumePool, String volumePath,
+            String deviceToAttachDiskTo) {
+        String xmlForRbdDisk = getXmlForRbdDisk(storagePoolMgr, volumePool, volumePath, deviceToAttachDiskTo);
+        logger.debug("RBD disk xml to attach: {}", xmlForRbdDisk);
+        // The command is executed without a shell, so the XML cannot be piped in through a
+        // here-document. Write it to a temporary file and pass virsh the path instead.
+        Path xmlFile = null;
+        try {
+            xmlFile = Files.createTempFile("csrestore-rbd-", ".xml");
+            Files.write(xmlFile, xmlForRbdDisk.getBytes(StandardCharsets.UTF_8));
+            String[] virshCmd = new String[] { Script.getExecutableAbsolutePath("virsh"), "attach-device", vmName, xmlFile.toString() };
+            return Script.executeCommandForExitValue(virshCmd) == 0;
+        } catch (IOException e) {
+            logger.error("Failed to write the RBD disk XML used to attach volume [{}] to VM [{}]", volumePath, vmName, e);
+            return false;
+        } finally {
+            if (xmlFile != null) {
+                try {
+                    Files.deleteIfExists(xmlFile);
+                } catch (IOException e) {
+                    logger.warn("Failed to delete the temporary RBD disk XML file [{}].", xmlFile, e);
+                }
+            }
+        }
+    }
+
     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" };
-        String[] awkCmd = new String[] { Script.getExecutableAbsolutePath("awk"), "'{print $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);
-        String currentDevice = result.second();
+        // 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));
+        }
         char lastChar = currentDevice.charAt(currentDevice.length() - 1);
         char incrementedChar = (char) (lastChar + 1);
         return currentDevice.substring(0, currentDevice.length() - 1) + incrementedChar;

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

@@ -25,9 +25,11 @@
 import static org.mockito.Mockito.when;
 
 import java.io.IOException;
+import java.lang.reflect.Method;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.List;
 
 import org.apache.cloudstack.backup.BackupAnswer;
 import org.apache.cloudstack.backup.RestoreBackupCommand;
@@ -42,8 +44,11 @@
 
 import com.cloud.agent.api.Answer;
 import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.storage.KVMStoragePool;
+import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
 import com.cloud.storage.Storage;
 import com.cloud.utils.Pair;
+import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.script.Script;
 import com.cloud.vm.VirtualMachine;
 
@@ -579,4 +584,123 @@ public void testExecuteWithMultipleVolumes() throws Exception {
             }
         }
     }
+
+    private String invokeGetDeviceToAttachDisk(String vmName) throws Exception {
+        Method method = LibvirtRestoreBackupCommandWrapper.class.getDeclaredMethod("getDeviceToAttachDisk", String.class);
+        method.setAccessible(true);
+        try {
+            return (String) method.invoke(wrapper, vmName);
+        } catch (java.lang.reflect.InvocationTargetException e) {
+            throw (Exception) e.getCause();
+        }
+    }
+
+    private String[] captureAttachCommand(Storage.StoragePoolType poolType) throws Exception {
+        PrimaryDataStoreTO volumePool = Mockito.mock(PrimaryDataStoreTO.class);
+        lenient().when(volumePool.getPoolType()).thenReturn(poolType);
+        lenient().when(volumePool.getHost()).thenReturn("10.0.0.1");
+        lenient().when(volumePool.getUuid()).thenReturn("pool-uuid");
+        KVMStoragePoolManager storagePoolMgr = Mockito.mock(KVMStoragePoolManager.class);
+        KVMStoragePool primaryPool = Mockito.mock(KVMStoragePool.class);
+        lenient().when(storagePoolMgr.getStoragePool(any(), anyString())).thenReturn(primaryPool);
+        lenient().when(primaryPool.getAuthUserName()).thenReturn("cloudstack");
+
+        Method method = LibvirtRestoreBackupCommandWrapper.class.getDeclaredMethod("attachVolumeToVm",
+                KVMStoragePoolManager.class, String.class, PrimaryDataStoreTO.class, String.class);
+        method.setAccessible(true);
+
+        final String[][] captured = new String[1][];
+        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");
+        }
+        return captured[0];
+    }
+
+    @Test
+    public void testGetDeviceToAttachDiskTrimsOutputBeforeIncrementing() 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"));
+        }
+    }
+
+    @Test
+    public void testGetDeviceToAttachDiskPassesUnquotedAwkProgram() 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];
+            scriptMock.when(() -> Script.executePipedCommands(anyList(), anyLong()))
+                    .thenAnswer(invocation -> {
+                        captured[0] = invocation.getArgument(0);
+                        return new Pair<>(0, "vda" + System.lineSeparator());
+                    });
+
+            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]);
+        }
+    }
+
+    @Test(expected = CloudRuntimeException.class)
+    public void testGetDeviceToAttachDiskFailsWhenNoDeviceIsReturned() throws Exception {
+        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<>(1, ""));
+
+            invokeGetDeviceToAttachDisk("test-vm");
+        }
+    }
+
+    @Test
+    public void testAttachVolumeUsesQcow2SubdriverForFileBackedPool() throws Exception {
+        String[] cmd = captureAttachCommand(Storage.StoragePoolType.NetworkFilesystem);
+        List<String> args = Arrays.asList(cmd);
+
+        Assert.assertTrue(args.contains("attach-disk"));
+        Assert.assertTrue(args.contains("--driver"));
+        Assert.assertTrue(args.contains("qemu"));
+        Assert.assertEquals("qcow2", args.get(args.indexOf("--subdriver") + 1));
+    }
+
+    @Test
+    public void testAttachVolumeOmitsQcow2SubdriverForLinstor() throws Exception {
+        String[] cmd = captureAttachCommand(Storage.StoragePoolType.Linstor);
+        List<String> args = Arrays.asList(cmd);
+
+        // Linstor volumes are raw DRBD block devices, declaring qcow2 makes libvirt reject them.
+        Assert.assertTrue(args.contains("attach-disk"));
+        Assert.assertFalse(args.contains("--subdriver"));
+    }
+
+    @Test
+    public void testAttachVolumePassesRbdXmlThroughAFile() throws Exception {
+        String[] cmd = captureAttachCommand(Storage.StoragePoolType.RBD);
+        List<String> args = Arrays.asList(cmd);
+
+        Assert.assertTrue(args.contains("attach-device"));
+        // The XML has to reach virsh as a file, a here-document cannot work without a shell.
+        Assert.assertFalse(args.stream().anyMatch(arg -> arg.contains("EOF")));
+        Assert.assertTrue(args.get(args.size() - 1).endsWith(".xml"));
+    }
 }