This is an automated email from the ASF dual-hosted git repository.
weizhouapache pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.22 by this push:
new 10037c85d61 Nas backup: Fix mount/unmount error handling and timeout
in LibvirtRestoreBackupCommandWrapper (#14006)
10037c85d61 is described below
commit 10037c85d61f9c79e0b357aa5a4f372940ad56d2
Author: Abhisar Sinha <[email protected]>
AuthorDate: Tue Sep 15 11:34:51 2026 +0530
Nas backup: Fix mount/unmount error handling and timeout in
LibvirtRestoreBackupCommandWrapper (#14006)
* kvm: detect mount failures and honour the configured timeouts on backup
restore
Script.executeCommand returns null when the command fails, it does not
throw,
so the try/catch around the mount and umount of the backup repository could
never fire and the return value was discarded. A repository that fails to
mount
was therefore treated as mounted, and the restore carried on against an
empty
directory until it failed later with a misleading "backup file not found". A
failed umount was ignored the same way, leaking the mount. Both now go
through
executeCommandForExitValue and check the exit value.
The same refactor also dropped the timeouts. mountTimeout was still passed
into
mountBackupDirectory but never used, and the rsync of the volume lost the
command timeout, so both fell back to the one hour default in Script
instead of
the configured values. An unresponsive repository could hold a restore up
for
an hour rather than failing after nas.backup.restore.mount.timeout seconds.
* kvm: clean up after a failed mount and bound the unmount of a backup
repository
The directory created for the mount is removed by the caller in a finally
block,
but that block is only reached once the mount has succeeded, so a repository
that cannot be mounted left an empty directory behind on every attempt. It
is
now removed before the failure is reported, best effort and logged if it
cannot
be.
The unmount ran without a timeout and so fell back to the one hour default
in
Script. Unmounting a repository that has become unreachable blocks as
easily as
mounting one, and this runs in the cleanup path of a restore, so it is now
bounded by the configured mount timeout like the mount itself.
---
.../LibvirtRestoreBackupCommandWrapper.java | 46 ++++++---
.../LibvirtRestoreBackupCommandWrapperTest.java | 113 ++++++++++++++++++++-
2 files changed, 143 insertions(+), 16 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 7f2a3c28cd1..6ca9d329fba 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
@@ -99,11 +99,11 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
newVolumeId = getVolumeUuidFromPath(volumePath, volumePool);
Long size = command.getRestoreVolumeSizes().get(0);
restoreVolume(storagePoolMgr, backupPath, volumePool,
volumePath, diskType, backupFile, size,
- new Pair<>(vmName, command.getVmState()),
mountDirectory, timeout);
+ new Pair<>(vmName, command.getVmState()),
mountDirectory, timeout, mountTimeout);
} else if (Boolean.TRUE.equals(vmExists)) {
- restoreVolumesOfExistingVM(storagePoolMgr, restoreVolumePools,
restoreVolumePaths, backedVolumeUUIDs, backupPath, backupFiles, mountDirectory,
timeout);
+ restoreVolumesOfExistingVM(storagePoolMgr, restoreVolumePools,
restoreVolumePaths, backedVolumeUUIDs, backupPath, backupFiles, mountDirectory,
timeout, mountTimeout);
} else {
- restoreVolumesOfDestroyedVMs(storagePoolMgr,
restoreVolumePools, restoreVolumePaths, backupPath, backupFiles,
mountDirectory, timeout);
+ restoreVolumesOfDestroyedVMs(storagePoolMgr,
restoreVolumePools, restoreVolumePaths, backupPath, backupFiles,
mountDirectory, timeout, mountTimeout);
}
} catch (CloudRuntimeException e) {
String errorMessage = e.getMessage() != null ? e.getMessage() : "";
@@ -124,7 +124,7 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
private void restoreVolumesOfExistingVM(KVMStoragePoolManager
storagePoolMgr, List<PrimaryDataStoreTO> restoreVolumePools,
List<String> restoreVolumePaths,
List<String> backedVolumesUUIDs,
- String backupPath, List<String>
backupFiles, String mountDirectory, int timeout) {
+ String backupPath, List<String>
backupFiles, String mountDirectory, int timeout, Integer mountTimeout) {
String diskType = "root";
try {
for (int idx = 0; idx < restoreVolumePaths.size(); idx++) {
@@ -141,13 +141,13 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
}
}
} finally {
- unmountBackupDirectory(mountDirectory);
+ unmountBackupDirectory(mountDirectory, mountTimeout);
deleteTemporaryDirectory(mountDirectory);
}
}
private void restoreVolumesOfDestroyedVMs(KVMStoragePoolManager
storagePoolMgr, List<PrimaryDataStoreTO> volumePools,
- List<String> volumePaths, String
backupPath, List<String> backupFiles, String mountDirectory, int timeout) {
+ List<String> volumePaths, String
backupPath, List<String> backupFiles, String mountDirectory, int timeout,
Integer mountTimeout) {
String diskType = "root";
try {
for (int i = 0; i < volumePaths.size(); i++) {
@@ -163,13 +163,13 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
}
}
} finally {
- unmountBackupDirectory(mountDirectory);
+ unmountBackupDirectory(mountDirectory, mountTimeout);
deleteTemporaryDirectory(mountDirectory);
}
}
private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String
backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType,
String backupFile,
- Long size, Pair<String, VirtualMachine.State>
vmNameAndState, String mountDirectory, int timeout) {
+ Long size, Pair<String, VirtualMachine.State>
vmNameAndState, String mountDirectory, int timeout, Integer mountTimeout) {
String bkpPath;
String volumeUuid;
try {
@@ -186,7 +186,7 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
}
}
} finally {
- unmountBackupDirectory(mountDirectory);
+ unmountBackupDirectory(mountDirectory, mountTimeout);
deleteTemporaryDirectory(mountDirectory);
}
}
@@ -202,6 +202,7 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
logger.error("Failed to create the tmp mount directory {} for
restore", mountDirectory, e);
throw new CloudRuntimeException("Failed to create the tmp mount
directory for restore on the KVM host");
}
+ int exitValue;
try {
String mountPath = Script.getExecutableAbsolutePath("mount");
List<String> mountCmd = new ArrayList<>();
@@ -222,23 +223,42 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
mountCmd.add("-o");
mountCmd.add(mountOptions);
}
- Script.executeCommand(mountCmd.toArray(new String[0]));
+ exitValue = Script.executeCommandForExitValue(mountTimeout,
mountCmd.toArray(new String[0]));
} catch (Exception e) {
logger.error("Failed to mount repository {} of type {} to the
directory {}", backupRepoAddress, backupRepoType, mountDirectory, e);
throw new CloudRuntimeException("Failed to mount the backup
repository on the KVM host");
}
+ if (exitValue != 0) {
+ logger.error("Failed to mount repository {} of type {} to the
directory {}, mount exited with {}", backupRepoAddress,
+ backupRepoType, mountDirectory, exitValue);
+ removeTemporaryDirectoryQuietly(mountDirectory);
+ throw new CloudRuntimeException("Failed to mount the backup
repository on the KVM host");
+ }
return mountDirectory;
}
- private void unmountBackupDirectory(String backupDirectory) {
+ private void unmountBackupDirectory(String backupDirectory, Integer
mountTimeout) {
+ int exitValue;
try {
String umountPath = Script.getExecutableAbsolutePath("umount");
String[] umountCmd = new String[] { "sudo", umountPath,
backupDirectory };
- Script.executeCommand(umountCmd);
+ exitValue = Script.executeCommandForExitValue(mountTimeout,
umountCmd);
} catch (Exception e) {
logger.error("Failed to unmount backup directory {}",
backupDirectory, e);
throw new CloudRuntimeException("Failed to unmount the backup
directory");
}
+ if (exitValue != 0) {
+ logger.error("Failed to unmount backup directory {}, umount exited
with {}", backupDirectory, exitValue);
+ throw new CloudRuntimeException("Failed to unmount the backup
directory");
+ }
+ }
+
+ private void removeTemporaryDirectoryQuietly(String backupDirectory) {
+ try {
+ Files.deleteIfExists(Paths.get(backupDirectory));
+ } catch (IOException e) {
+ logger.warn("Failed to remove the temporary mount directory {}
after the mount failed.", backupDirectory, e);
+ }
}
private void deleteTemporaryDirectory(String backupDirectory) {
@@ -277,7 +297,7 @@ public class LibvirtRestoreBackupCommandWrapper extends
CommandWrapper<RestoreBa
}
String[] rsyncCmd = new String[] {
Script.getExecutableAbsolutePath("rsync"), "-az", backupPath, volumePath };
- int exitValue = Script.executeCommandForExitValue(rsyncCmd);
+ int exitValue = Script.executeCommandForExitValue(timeout, rsyncCmd);
return exitValue == 0;
}
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 4bbd040b0d1..15f6e919d79 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
@@ -266,8 +266,8 @@ public class LibvirtRestoreBackupCommandWrapperTest {
filesMock.when(() ->
Files.createTempDirectory(anyString())).thenReturn(tempPath);
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
- scriptMock.when(() ->
Script.executeCommand(any(String[].class)))
- .thenThrow(new RuntimeException("failure")); // Mount
failure
+ scriptMock.when(() ->
Script.executeCommandForExitValue(anyLong(), any(String[].class)))
+ .thenReturn(1); // Mount failure
Answer result = wrapper.execute(command,
libvirtComputingResource);
@@ -409,7 +409,7 @@ public class LibvirtRestoreBackupCommandWrapperTest {
.thenAnswer(invocation -> invocation.getArgument(0));
scriptMock.when(() ->
Script.executeCommand(any(String[].class)))
.thenReturn(null);
- scriptMock.when(() ->
Script.executeCommandForExitValue(any(String[].class)))
+ scriptMock.when(() ->
Script.executeCommandForExitValue(anyLong(), any(String[].class)))
.thenAnswer(invocation -> {
if
(Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("rsync"::equals))
{
return 1; // Rsync failure
@@ -703,4 +703,111 @@ public class LibvirtRestoreBackupCommandWrapperTest {
Assert.assertFalse(args.stream().anyMatch(arg -> arg.contains("EOF")));
Assert.assertTrue(args.get(args.size() - 1).endsWith(".xml"));
}
+
+ @Test
+ public void testMountUsesTheConfiguredTimeout() throws Exception {
+ when(command.getVmName()).thenReturn("test-vm");
+ when(command.getBackupPath()).thenReturn("backup/path");
+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
+ when(command.getBackupRepoType()).thenReturn("nfs");
+ when(command.getMountOptions()).thenReturn("rw");
+ when(command.getMountTimeout()).thenReturn(30);
+
+ try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
+ Path tempPath = Mockito.mock(Path.class);
+ when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
+ filesMock.when(() ->
Files.createTempDirectory(anyString())).thenReturn(tempPath);
+
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ final long[] mountTimeout = new long[1];
+ scriptMock.when(() ->
Script.executeCommandForExitValue(anyLong(), any(String[].class)))
+ .thenAnswer(invocation -> {
+ if
(Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("mount"::equals))
{
+ mountTimeout[0] = invocation.getArgument(0);
+ return 1; // stop the restore right after the
mount
+ }
+ return 0;
+ });
+
+ wrapper.execute(command, libvirtComputingResource);
+
+ Assert.assertEquals(30 * 1000L, mountTimeout[0]);
+ }
+ }
+ }
+
+ @Test
+ public void testMountFailureRemovesTheTemporaryDirectory() throws
Exception {
+ when(command.getVmName()).thenReturn("test-vm");
+ when(command.getBackupPath()).thenReturn("backup/path");
+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
+ when(command.getBackupRepoType()).thenReturn("nfs");
+ when(command.getMountOptions()).thenReturn("rw");
+ when(command.getMountTimeout()).thenReturn(30);
+
+ try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
+ Path tempPath = Mockito.mock(Path.class);
+ when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
+ filesMock.when(() ->
Files.createTempDirectory(anyString())).thenReturn(tempPath);
+ filesMock.when(() ->
Files.deleteIfExists(any(Path.class))).thenReturn(true);
+
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ scriptMock.when(() ->
Script.executeCommandForExitValue(anyLong(), any(String[].class)))
+ .thenReturn(1); // the mount fails
+
+ wrapper.execute(command, libvirtComputingResource);
+
+ // the directory created for the mount must not be left behind
+ filesMock.verify(() -> Files.deleteIfExists(any(Path.class)));
+ }
+ }
+ }
+
+ @Test
+ public void testUnmountIsBoundedByTheConfiguredTimeout() throws Exception {
+ when(command.getVmName()).thenReturn("test-vm");
+ when(command.getBackupPath()).thenReturn("backup/path");
+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
+ when(command.getBackupRepoType()).thenReturn("nfs");
+ when(command.getMountOptions()).thenReturn("rw");
+ when(command.isVmExists()).thenReturn(true);
+ when(command.getDiskType()).thenReturn("root");
+ PrimaryDataStoreTO primaryDataStore =
Mockito.mock(PrimaryDataStoreTO.class);
+
when(primaryDataStore.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
+
when(command.getRestoreVolumePools()).thenReturn(Arrays.asList(primaryDataStore));
+
when(command.getRestoreVolumePaths()).thenReturn(Arrays.asList("/var/lib/libvirt/images/volume-123"));
+
when(command.getBackupVolumesUUIDs()).thenReturn(Arrays.asList("volume-123"));
+ when(command.getBackupFiles()).thenReturn(Arrays.asList("volume-123"));
+ when(command.getMountTimeout()).thenReturn(30);
+
+ try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
+ Path tempPath = Mockito.mock(Path.class);
+ when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
+ filesMock.when(() ->
Files.createTempDirectory(anyString())).thenReturn(tempPath);
+ filesMock.when(() ->
Files.deleteIfExists(any(Path.class))).thenReturn(true);
+
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() ->
Script.getExecutableAbsolutePath(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ final long[] umountTimeout = new long[] { -1 };
+ scriptMock.when(() ->
Script.executeCommandForExitValue(anyLong(), any(String[].class)))
+ .thenAnswer(invocation -> {
+ if
(Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("umount"::equals))
{
+ umountTimeout[0] = invocation.getArgument(0);
+ }
+ return 0;
+ });
+ scriptMock.when(() ->
Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
+
+ wrapper.execute(command, libvirtComputingResource);
+
+ // an unreachable repository blocks umount just as it blocks
mount
+ Assert.assertEquals(30 * 1000L, umountTimeout[0]);
+ }
+ }
+ }
}
