Copilot commented on code in PR #14006:
URL: https://github.com/apache/cloudstack/pull/14006#discussion_r3887067573
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -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) {
Review Comment:
unmountBackupDirectory now uses Script.executeCommandForExitValue(String...)
without passing an explicit timeout. That overload delegates to
executeCommandForExitValue(0, ...) which falls back to Script's 1-hour default
timeout; if umount blocks (common with unreachable NFS/CIFS), restores can
still hang in the finally cleanup path far longer than the configured mount
timeout.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -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");
+ }
Review Comment:
When the mount command exits non-zero, mountBackupDirectory throws but
leaves the temporary mount directory created by Files.createTempDirectory()
behind. Since the mount failure short-circuits before the restore methods'
finally blocks run, this can accumulate stale /tmp/csbackup.* directories on
repeated failures.
Consider cleaning up the temporary directory on mount failure before
throwing (best-effort, log on failure).
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:
##########
@@ -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
Review Comment:
The Mockito stub meant to simulate an rsync failure never matches because
invocation.getArguments() contains (timeout, String[] cmd). Converting the
String[] to String via String.valueOf(...) yields an array identity (e.g.,
"[Ljava.lang.String;@...") rather than the command contents, so the "rsync"
check is always false and the test doesn't actually exercise the failure path.
This issue also appears on line 603 of the same file.
--
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]
