Copilot commented on code in PR #13424:
URL: https://github.com/apache/cloudstack/pull/13424#discussion_r3492790891
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -196,10 +196,12 @@ backup_running_vm() {
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
- du -sb $dest | cut -f1
-
- umount $mount_point
- rmdir $mount_point
+ backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING:
du failed for $dest, reporting size as 0"; backup_size=0; }
+
+ timeout "$UNMOUNT_TIMEOUT" umount "$mount_point" 2>>"$logFile" || { log
"WARNING: umount of $mount_point failed or timed out"; true; }
+ rmdir "$mount_point" 2>>"$logFile" || { log "WARNING: rmdir of $mount_point
failed"; true; }
Review Comment:
`umount`/`rmdir` failures are now fully suppressed (only logged). This
bypasses the script’s existing cleanup-failure signaling
(`EXIT_CLEANUP_FAILED=20` / `cleanup()`), so CloudStack will record the backup
as successful even when the NAS mount cannot be torn down, which risks
accumulating leaked mounts without any control-plane visibility. If the intent
is “backup succeeded but cleanup failed”, consider extending the contract to
surface that state (e.g., return a distinct exit code and handle it in
`LibvirtTakeBackupCommandWrapper`/`BackupAnswer`) instead of silently
continuing.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -196,10 +196,12 @@ backup_running_vm() {
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
- du -sb $dest | cut -f1
-
- umount $mount_point
- rmdir $mount_point
+ backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING:
du failed for $dest, reporting size as 0"; backup_size=0; }
+
+ timeout "$UNMOUNT_TIMEOUT" umount "$mount_point" 2>>"$logFile" || { log
"WARNING: umount of $mount_point failed or timed out"; true; }
Review Comment:
`timeout` is used to wrap `umount`, but if `timeout` is not installed (or
not in PATH), `umount` will never be attempted and the mount will be left
behind (the code only logs a warning and continues). Consider falling back to a
direct `umount` when `timeout` is unavailable, while still suppressing failures
to avoid failing an otherwise successful backup.
--
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]
