Skip to content

Fix restore-and-attach of a backed up volume on NFS, Linstor and Ceph primary storages #14007

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
abh1sar wants to merge 1 commit into
base: 4.22
Choose a base branch
from
+175 −20
Open

Fix restore-and-attach of a backed up volume on NFS, Linstor and Ceph primary storages #14007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 51 additions & 20 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 @@ -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;
Expand Down Expand Up @@ -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));
}
Comment on lines +402 to +405
char lastChar = currentDevice.charAt(currentDevice.length() - 1);
char incrementedChar = (char) (lastChar + 1);
return currentDevice.substring(0, currentDevice.length() - 1) + incrementedChar;
Expand Down
124 changes: 124 additions & 0 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 @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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");
}
Comment on lines +613 to +625
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"));
}
}
Loading