Copilot commented on code in PR #14007:
URL: https://github.com/apache/cloudstack/pull/14007#discussion_r3887065692
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:
##########
@@ -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");
+ }
Review Comment:
captureAttachCommand() indirectly triggers real filesystem I/O for the RBD
path (Files.createTempFile/write/deleteIfExists) because those calls are not
mocked here. This is inconsistent with the rest of this test class (which mocks
Files) and can make the unit test flaky or fail in restricted CI environments.
Mock these Files calls inside captureAttachCommand so the attach tests stay
hermetic.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -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));
+ }
Review Comment:
The exception thrown when device detection fails omits the command exit code
and raw output, which makes restore-and-attach failures hard to diagnose
(especially since execute() surfaces only the exception message). Include the
exit value and raw stdout in the message before trimming so operators can see
what domblklist/awk returned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
