This is an automated email from the ASF dual-hosted git repository. weizhouapache pushed a commit to branch 4.23.0-ceph in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit f5e891f9d996c2777f39cd3df13bf2e93c2cc104 Author: Abhisar Sinha <[email protected]> AuthorDate: Wed Sep 9 15:14:49 2026 +0530 kvm: fix restore-and-attach of a backed up volume (#14007) 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. --- .../LibvirtRestoreBackupCommandWrapper.java | 71 ++++++++---- .../LibvirtRestoreBackupCommandWrapperTest.java | 124 +++++++++++++++++++++ 2 files changed, 175 insertions(+), 20 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java index 3839d7f6bdb..09d1745406a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java +++ b/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; @@ -362,38 +363,68 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper<RestoreBa 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; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java index fc234163298..2fc9b43c12b 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java @@ -25,9 +25,11 @@ import static org.mockito.Mockito.mockStatic; 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 org.mockito.junit.MockitoJUnitRunner; 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; @@ -581,4 +586,123 @@ public class LibvirtRestoreBackupCommandWrapperTest { } } } + + 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")); + } }
