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 1453155a52da304ccb224e5a23fce008664c5983 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 09d1745406a..ccd0ec63452 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 @@ -104,11 +104,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() : ""; @@ -129,7 +129,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++) { @@ -146,13 +146,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++) { @@ -168,13 +168,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 { @@ -191,7 +191,7 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper<RestoreBa } } } finally { - unmountBackupDirectory(mountDirectory); + unmountBackupDirectory(mountDirectory, mountTimeout); deleteTemporaryDirectory(mountDirectory); } } @@ -207,6 +207,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<>(); @@ -227,23 +228,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) { @@ -294,7 +314,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 2fc9b43c12b..3bdc23d27a1 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 @@ -705,4 +705,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]); + } + } + } }
