Visitar URL original
Nas backup: Fix mount/unmount error handling and timeout in LibvirtRe… · apache/cloudstack@10037c8 · GitHub
Skip to content

Commit 10037c8

Browse files
abh1sarabh1sar
authored
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.
1 parent e38b7a7 commit 10037c8

2 files changed

Lines changed: 143 additions & 16 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,11 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser
9999
newVolumeId = getVolumeUuidFromPath(volumePath, volumePool);
100100
Long size = command.getRestoreVolumeSizes().get(0);
101101
restoreVolume(storagePoolMgr, backupPath, volumePool, volumePath, diskType, backupFile, size,
102-
new Pair<>(vmName, command.getVmState()), mountDirectory, timeout);
102+
new Pair<>(vmName, command.getVmState()), mountDirectory, timeout, mountTimeout);
103103
} else if (Boolean.TRUE.equals(vmExists)) {
104-
restoreVolumesOfExistingVM(storagePoolMgr, restoreVolumePools, restoreVolumePaths, backedVolumeUUIDs, backupPath, backupFiles, mountDirectory, timeout);
104+
restoreVolumesOfExistingVM(storagePoolMgr, restoreVolumePools, restoreVolumePaths, backedVolumeUUIDs, backupPath, backupFiles, mountDirectory, timeout, mountTimeout);
105105
} else {
106-
restoreVolumesOfDestroyedVMs(storagePoolMgr, restoreVolumePools, restoreVolumePaths, backupPath, backupFiles, mountDirectory, timeout);
106+
restoreVolumesOfDestroyedVMs(storagePoolMgr, restoreVolumePools, restoreVolumePaths, backupPath, backupFiles, mountDirectory, timeout, mountTimeout);
107107
}
108108
} catch (CloudRuntimeException e) {
109109
String errorMessage = e.getMessage() != null ? e.getMessage() : "";
@@ -124,7 +124,7 @@ private void verifyBackupFile(String backupPath, String volUuid) {
124124

125125
private void restoreVolumesOfExistingVM(KVMStoragePoolManager storagePoolMgr, List<PrimaryDataStoreTO> restoreVolumePools,
126126
List<String> restoreVolumePaths, List<String> backedVolumesUUIDs,
127-
String backupPath, List<String> backupFiles, String mountDirectory, int timeout) {
127+
String backupPath, List<String> backupFiles, String mountDirectory, int timeout, Integer mountTimeout) {
128128
String diskType = "root";
129129
try {
130130
for (int idx = 0; idx < restoreVolumePaths.size(); idx++) {
@@ -141,13 +141,13 @@ private void restoreVolumesOfExistingVM(KVMStoragePoolManager storagePoolMgr, Li
141141
}
142142
}
143143
} finally {
144-
unmountBackupDirectory(mountDirectory);
144+
unmountBackupDirectory(mountDirectory, mountTimeout);
145145
deleteTemporaryDirectory(mountDirectory);
146146
}
147147
}
148148

149149
private void restoreVolumesOfDestroyedVMs(KVMStoragePoolManager storagePoolMgr, List<PrimaryDataStoreTO> volumePools,
150-
List<String> volumePaths, String backupPath, List<String> backupFiles, String mountDirectory, int timeout) {
150+
List<String> volumePaths, String backupPath, List<String> backupFiles, String mountDirectory, int timeout, Integer mountTimeout) {
151151
String diskType = "root";
152152
try {
153153
for (int i = 0; i < volumePaths.size(); i++) {
@@ -163,13 +163,13 @@ private void restoreVolumesOfDestroyedVMs(KVMStoragePoolManager storagePoolMgr,
163163
}
164164
}
165165
} finally {
166-
unmountBackupDirectory(mountDirectory);
166+
unmountBackupDirectory(mountDirectory, mountTimeout);
167167
deleteTemporaryDirectory(mountDirectory);
168168
}
169169
}
170170

171171
private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType, String backupFile,
172-
Long size, Pair<String, VirtualMachine.State> vmNameAndState, String mountDirectory, int timeout) {
172+
Long size, Pair<String, VirtualMachine.State> vmNameAndState, String mountDirectory, int timeout, Integer mountTimeout) {
173173
String bkpPath;
174174
String volumeUuid;
175175
try {
@@ -186,7 +186,7 @@ private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPa
186186
}
187187
}
188188
} finally {
189-
unmountBackupDirectory(mountDirectory);
189+
unmountBackupDirectory(mountDirectory, mountTimeout);
190190
deleteTemporaryDirectory(mountDirectory);
191191
}
192192
}
@@ -202,6 +202,7 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
202202
logger.error("Failed to create the tmp mount directory {} for restore", mountDirectory, e);
203203
throw new CloudRuntimeException("Failed to create the tmp mount directory for restore on the KVM host");
204204
}
205+
int exitValue;
205206
try {
206207
String mountPath = Script.getExecutableAbsolutePath("mount");
207208
List<String> mountCmd = new ArrayList<>();
@@ -222,23 +223,42 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
222223
mountCmd.add("-o");
223224
mountCmd.add(mountOptions);
224225
}
225-
Script.executeCommand(mountCmd.toArray(new String[0]));
226+
exitValue = Script.executeCommandForExitValue(mountTimeout, mountCmd.toArray(new String[0]));
226227
} catch (Exception e) {
227228
logger.error("Failed to mount repository {} of type {} to the directory {}", backupRepoAddress, backupRepoType, mountDirectory, e);
228229
throw new CloudRuntimeException("Failed to mount the backup repository on the KVM host");
229230
}
231+
if (exitValue != 0) {
232+
logger.error("Failed to mount repository {} of type {} to the directory {}, mount exited with {}", backupRepoAddress,
233+
backupRepoType, mountDirectory, exitValue);
234+
removeTemporaryDirectoryQuietly(mountDirectory);
235+
throw new CloudRuntimeException("Failed to mount the backup repository on the KVM host");
236+
}
230237
return mountDirectory;
231238
}
232239

233-
private void unmountBackupDirectory(String backupDirectory) {
240+
private void unmountBackupDirectory(String backupDirectory, Integer mountTimeout) {
241+
int exitValue;
234242
try {
235243
String umountPath = Script.getExecutableAbsolutePath("umount");
236244
String[] umountCmd = new String[] { "sudo", umountPath, backupDirectory };
237-
Script.executeCommand(umountCmd);
245+
exitValue = Script.executeCommandForExitValue(mountTimeout, umountCmd);
238246
} catch (Exception e) {
239247
logger.error("Failed to unmount backup directory {}", backupDirectory, e);
240248
throw new CloudRuntimeException("Failed to unmount the backup directory");
241249
}
250+
if (exitValue != 0) {
251+
logger.error("Failed to unmount backup directory {}, umount exited with {}", backupDirectory, exitValue);
252+
throw new CloudRuntimeException("Failed to unmount the backup directory");
253+
}
254+
}
255+
256+
private void removeTemporaryDirectoryQuietly(String backupDirectory) {
257+
try {
258+
Files.deleteIfExists(Paths.get(backupDirectory));
259+
} catch (IOException e) {
260+
logger.warn("Failed to remove the temporary mount directory {} after the mount failed.", backupDirectory, e);
261+
}
242262
}
243263

244264
private void deleteTemporaryDirectory(String backupDirectory) {
@@ -277,7 +297,7 @@ private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, Pr
277297
}
278298

279299
String[] rsyncCmd = new String[] { Script.getExecutableAbsolutePath("rsync"), "-az", backupPath, volumePath };
280-
int exitValue = Script.executeCommandForExitValue(rsyncCmd);
300+
int exitValue = Script.executeCommandForExitValue(timeout, rsyncCmd);
281301
return exitValue == 0;
282302
}
283303

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java‎

Lines changed: 110 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,8 @@ public void testExecuteWithMountFailure() throws Exception {
266266
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
267267

268268
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
269-
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
270-
.thenThrow(new RuntimeException("failure")); // Mount failure
269+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
270+
.thenReturn(1); // Mount failure
271271

272272
Answer result = wrapper.execute(command, libvirtComputingResource);
273273

@@ -409,7 +409,7 @@ public void testExecuteWithRsyncFailure() throws Exception {
409409
.thenAnswer(invocation -> invocation.getArgument(0));
410410
scriptMock.when(() -> Script.executeCommand(any(String[].class)))
411411
.thenReturn(null);
412-
scriptMock.when(() -> Script.executeCommandForExitValue(any(String[].class)))
412+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
413413
.thenAnswer(invocation -> {
414414
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("rsync"::equals)) {
415415
return 1; // Rsync failure
@@ -703,4 +703,111 @@ public void testAttachVolumePassesRbdXmlThroughAFile() throws Exception {
703703
Assert.assertFalse(args.stream().anyMatch(arg -> arg.contains("EOF")));
704704
Assert.assertTrue(args.get(args.size() - 1).endsWith(".xml"));
705705
}
706+
707+
@Test
708+
public void testMountUsesTheConfiguredTimeout() throws Exception {
709+
when(command.getVmName()).thenReturn("test-vm");
710+
when(command.getBackupPath()).thenReturn("backup/path");
711+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
712+
when(command.getBackupRepoType()).thenReturn("nfs");
713+
when(command.getMountOptions()).thenReturn("rw");
714+
when(command.getMountTimeout()).thenReturn(30);
715+
716+
try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
717+
Path tempPath = Mockito.mock(Path.class);
718+
when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
719+
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
720+
721+
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
722+
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
723+
.thenAnswer(invocation -> invocation.getArgument(0));
724+
final long[] mountTimeout = new long[1];
725+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
726+
.thenAnswer(invocation -> {
727+
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("mount"::equals)) {
728+
mountTimeout[0] = invocation.getArgument(0);
729+
return 1; // stop the restore right after the mount
730+
}
731+
return 0;
732+
});
733+
734+
wrapper.execute(command, libvirtComputingResource);
735+
736+
Assert.assertEquals(30 * 1000L, mountTimeout[0]);
737+
}
738+
}
739+
}
740+
741+
@Test
742+
public void testMountFailureRemovesTheTemporaryDirectory() throws Exception {
743+
when(command.getVmName()).thenReturn("test-vm");
744+
when(command.getBackupPath()).thenReturn("backup/path");
745+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
746+
when(command.getBackupRepoType()).thenReturn("nfs");
747+
when(command.getMountOptions()).thenReturn("rw");
748+
when(command.getMountTimeout()).thenReturn(30);
749+
750+
try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
751+
Path tempPath = Mockito.mock(Path.class);
752+
when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
753+
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
754+
filesMock.when(() -> Files.deleteIfExists(any(Path.class))).thenReturn(true);
755+
756+
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
757+
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
758+
.thenAnswer(invocation -> invocation.getArgument(0));
759+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
760+
.thenReturn(1); // the mount fails
761+
762+
wrapper.execute(command, libvirtComputingResource);
763+
764+
// the directory created for the mount must not be left behind
765+
filesMock.verify(() -> Files.deleteIfExists(any(Path.class)));
766+
}
767+
}
768+
}
769+
770+
@Test
771+
public void testUnmountIsBoundedByTheConfiguredTimeout() throws Exception {
772+
when(command.getVmName()).thenReturn("test-vm");
773+
when(command.getBackupPath()).thenReturn("backup/path");
774+
when(command.getBackupRepoAddress()).thenReturn("192.168.1.100:/backup");
775+
when(command.getBackupRepoType()).thenReturn("nfs");
776+
when(command.getMountOptions()).thenReturn("rw");
777+
when(command.isVmExists()).thenReturn(true);
778+
when(command.getDiskType()).thenReturn("root");
779+
PrimaryDataStoreTO primaryDataStore = Mockito.mock(PrimaryDataStoreTO.class);
780+
when(primaryDataStore.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
781+
when(command.getRestoreVolumePools()).thenReturn(Arrays.asList(primaryDataStore));
782+
when(command.getRestoreVolumePaths()).thenReturn(Arrays.asList("/var/lib/libvirt/images/volume-123"));
783+
when(command.getBackupVolumesUUIDs()).thenReturn(Arrays.asList("volume-123"));
784+
when(command.getBackupFiles()).thenReturn(Arrays.asList("volume-123"));
785+
when(command.getMountTimeout()).thenReturn(30);
786+
787+
try (MockedStatic<Files> filesMock = mockStatic(Files.class)) {
788+
Path tempPath = Mockito.mock(Path.class);
789+
when(tempPath.toString()).thenReturn("/tmp/csbackup.abc123");
790+
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);
791+
filesMock.when(() -> Files.deleteIfExists(any(Path.class))).thenReturn(true);
792+
793+
try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
794+
scriptMock.when(() -> Script.getExecutableAbsolutePath(anyString()))
795+
.thenAnswer(invocation -> invocation.getArgument(0));
796+
final long[] umountTimeout = new long[] { -1 };
797+
scriptMock.when(() -> Script.executeCommandForExitValue(anyLong(), any(String[].class)))
798+
.thenAnswer(invocation -> {
799+
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("umount"::equals)) {
800+
umountTimeout[0] = invocation.getArgument(0);
801+
}
802+
return 0;
803+
});
804+
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString())).thenReturn(0);
805+
806+
wrapper.execute(command, libvirtComputingResource);
807+
808+
// an unreachable repository blocks umount just as it blocks mount
809+
Assert.assertEquals(30 * 1000L, umountTimeout[0]);
810+
}
811+
}
812+
}
706813
}

0 commit comments

Comments
 (0)