jmsperu commented on code in PR #12843:
URL: https://github.com/apache/cloudstack/pull/12843#discussion_r4348449739
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -617,16 +671,29 @@ cleanup() {
exit $EXIT_CLEANUP_FAILED
fi
- rm -rf "$dest" || { echo "Failed to delete $dest"; status=1; }
- umount "$mount_point" || { echo "Failed to unmount $mount_point"; status=1; }
- rmdir "$mount_point" || { echo "Failed to remove mount point $mount_point";
status=1; }
+ if [[ -n "$dest" && -d "$dest" ]]; then
+ rm -rf "$dest" || { echo "Failed to delete $dest"; status=1; }
+ fi
+ if [[ -n "$mount_point" && -d "$mount_point" ]]; then
+ # Only umount if it is actually a mount. Otherwise (mount failed, or
stats/delete
+ # already unmounted) a umount error would wrongly flag cleanup as failed
via the EXIT trap.
+ if mountpoint -q "$mount_point"; then
+ umount "$mount_point" 2>/dev/null || { echo "Failed to unmount
$mount_point"; status=1; }
+ fi
+ rmdir "$mount_point" 2>/dev/null || true
+ fi
if [[ $status -ne 0 ]]; then
echo "Backup cleanup failed"
exit $EXIT_CLEANUP_FAILED
fi
}
+# Trap ensures cleanup runs on any exit path, including SIGTERM/SIGINT and
+# unexpected errors caught by set -e, not just the explicit failure branches.
+# Prevents orphan NFS mounts from accumulating after non-graceful exits.
+trap cleanup EXIT
Review Comment:
Good catch, you are right, and it was worse than one path. Once the share
was still mounted, the EXIT trap ran rm -rf on $dest, which at that point is
the finished backup on the NAS. For a running VM, if the final umount timed out
and the trap's own umount retry then succeeded, the script deleted the backup
and still exited 0. A stopped VM backup lost its backup the same way, but at
least reported a failure. In stats, $dest can be the root of the backup store,
so a failed umount there could delete existing backups.
Fixed in 84c227c979:
1. A BACKUP_COMPLETE flag is set just before the final unmount. cleanup()
removes $dest only for a backup that did not complete. For delete and stats the
trap now only unmounts and removes the temporary mount directory.
2. Both backup paths unmount through one helper. It tries umount with a time
limit twice, then umount -l. If the share still lingers it logs a WARNING and
keeps the success status, because the backup is intact.
3. If timeout is not installed, plain umount is used.
4. An interrupted backup is still aborted and removed as before.
I checked this with a stubbed harness (fake virsh, mount, umount, df, du and
a fake NAS directory). Running VM backup with an umount that times out once:
before, exit 0 with no files left on the NAS; after, exit 0 with all backup
files present. With every umount failing, including the lazy one, the files are
kept and the run succeeds with warnings. A failed backup job still exits 1 and
its partial files are removed. Stats with a failing umount no longer deletes
anything. Shellcheck reports nothing new.
--
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]
