From ad63ada649c6d2cd3dda8910b42ac0ec22863e1d Mon Sep 17 00:00:00 2001
From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
Date: Sat, 29 Aug 2026 15:44:27 +0530
Subject: [PATCH 1/2] 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.
---
 .../LibvirtRestoreBackupCommandWrapper.java   | 17 ++++++--
 ...ibvirtRestoreBackupCommandWrapperTest.java | 40 +++++++++++++++++--
 2 files changed, 51 insertions(+), 6 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 9ebb180b055f..10fa664ef0d7 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
@@ -201,6 +201,7 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
             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<>();
@@ -221,23 +222,33 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
                 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);
+            throw new CloudRuntimeException("Failed to mount the backup repository on the KVM host");
+        }
         return mountDirectory;
     }
 
     private void unmountBackupDirectory(String backupDirectory) {
+        int exitValue;
         try {
             String umountPath = Script.getExecutableAbsolutePath("umount");
             String[] umountCmd = new String[] { "sudo", umountPath, backupDirectory };
-            Script.executeCommand(umountCmd);
+            exitValue = Script.executeCommandForExitValue(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 deleteTemporaryDirectory(String backupDirectory) {
@@ -276,7 +287,7 @@ private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, Pr
         }
 
         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 f028035e8dcc..d4fffb2bd20e 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
@@ -261,8 +261,8 @@ public void testExecuteWithMountFailure() throws Exception {
             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);
 
@@ -404,7 +404,7 @@ public void testExecuteWithRsyncFailure() throws Exception {
                         .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
@@ -579,4 +579,38 @@ public void testExecuteWithMultipleVolumes() throws Exception {
             }
         }
     }
+
+    @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]);
+            }
+        }
+    }
 }

From 30b30f2014aaf519b3f961f5743e66d5e481c3ac Mon Sep 17 00:00:00 2001
From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
Date: Sun, 30 Aug 2026 21:05:13 +0530
Subject: [PATCH 2/2] 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   | 31 +++++---
 ...ibvirtRestoreBackupCommandWrapperTest.java | 73 +++++++++++++++++++
 2 files changed, 93 insertions(+), 11 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 10fa664ef0d7..7519d7f390c5 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
@@ -98,11 +98,11 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser
                 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() : "";
@@ -123,7 +123,7 @@ private void verifyBackupFile(String backupPath, String volUuid) {
 
     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++) {
@@ -140,13 +140,13 @@ private void restoreVolumesOfExistingVM(KVMStoragePoolManager storagePoolMgr, Li
                 }
             }
         } 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++) {
@@ -162,13 +162,13 @@ private void restoreVolumesOfDestroyedVMs(KVMStoragePoolManager storagePoolMgr,
                 }
             }
         } 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 {
@@ -185,7 +185,7 @@ private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPa
                 }
             }
         } finally {
-            unmountBackupDirectory(mountDirectory);
+            unmountBackupDirectory(mountDirectory, mountTimeout);
             deleteTemporaryDirectory(mountDirectory);
         }
     }
@@ -230,17 +230,18 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
         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 };
-            exitValue = Script.executeCommandForExitValue(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");
@@ -251,6 +252,14 @@ private void unmountBackupDirectory(String backupDirectory) {
         }
     }
 
+    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) {
         try {
             Files.deleteIfExists(Paths.get(backupDirectory));
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 d4fffb2bd20e..94aa0920d114 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
@@ -613,4 +613,77 @@ public void testMountUsesTheConfiguredTimeout() throws Exception {
             }
         }
     }
+
+    @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]);
+            }
+        }
+    }
 }