jmsperu commented on code in PR #12843:
URL: https://github.com/apache/cloudstack/pull/12843#discussion_r3844109368
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -262,19 +287,62 @@ mount_operation() {
fi
}
+check_free_space() {
+ local free_bytes
+ free_bytes=$(df -P "$mount_point" 2>/dev/null | awk 'NR==2 {print $4}')
+ if [[ -n "$free_bytes" ]]; then
+ # df -P reports 1K blocks; convert to bytes.
+ free_bytes=$((free_bytes * 1024))
+ if [[ $free_bytes -lt $MIN_FREE_SPACE ]]; then
+ echo "Insufficient free space on backup target: $((free_bytes /
1048576)) MB available, $((MIN_FREE_SPACE / 1048576)) MB required"
+ exit 1
+ fi
+ log -ne "Backup target has $((free_bytes / 1073741824)) GB free space"
+ fi
+}
+
cleanup() {
+ # Idempotent: skip if a prior explicit call already ran. Without this guard,
+ # the EXIT trap would re-run cleanup and fail on the already-unmounted point.
+ [[ $CLEANUP_DONE -eq 1 ]] && return 0
+ CLEANUP_DONE=1
+
local status=0
- 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 the VM was paused mid-backup (e.g. backup-begin succeeded but the script
+ # is exiting on error or signal), resume it. Without this a failed backup
+ # leaves the guest stuck in 'paused' state until an operator intervenes.
+ if [[ -n "$VM" ]]; then
+ local vm_state
+ vm_state=$(virsh -c qemu:///system domstate "$VM" 2>/dev/null || true)
+ if [[ "$vm_state" == "paused" ]]; then
+ log -ne "Resuming paused VM $VM during backup cleanup"
+ if ! virsh -c qemu:///system resume "$VM" > /dev/null 2>&1; then
+ echo "Failed to resume VM $VM"
+ status=1
+ fi
+ fi
+ fi
+
+ 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
+ umount "$mount_point" 2>/dev/null || { echo "Failed to unmount
$mount_point"; status=1; }
+ rmdir "$mount_point" 2>/dev/null || true
+ fi
Review Comment:
Done: cleanup() now only calls umount when `mountpoint -q "$mount_point"`
says the directory is actually mounted; a leftover directory that is no longer
a mount is just rmdir'd.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -262,19 +287,62 @@ mount_operation() {
fi
}
Review Comment:
Done: mount_operation() tests `mount` directly in the `if` (no `| tee`), so
a failure is reported as a mount failure and the status checked is mount's, not
tee's.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -204,6 +228,7 @@ backup_running_vm() {
backup_stopped_vm() {
mount_operation
+ check_free_space
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
IFS=","
Review Comment:
Done: backup_stopped_vm() unmounts on success (mirroring
backup_running_vm/delete_backup) so the EXIT trap cannot remove the finished
backup, and the qemu-img convert failure path is `cleanup; exit 1`.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -262,19 +287,62 @@ mount_operation() {
fi
}
+check_free_space() {
+ local free_bytes
+ free_bytes=$(df -P "$mount_point" 2>/dev/null | awk 'NR==2 {print $4}')
+ if [[ -n "$free_bytes" ]]; then
+ # df -P reports 1K blocks; convert to bytes.
+ free_bytes=$((free_bytes * 1024))
+ if [[ $free_bytes -lt $MIN_FREE_SPACE ]]; then
+ echo "Insufficient free space on backup target: $((free_bytes /
1048576)) MB available, $((MIN_FREE_SPACE / 1048576)) MB required"
+ exit 1
+ fi
+ log -ne "Backup target has $((free_bytes / 1073741824)) GB free space"
+ fi
+}
+
cleanup() {
+ # Idempotent: skip if a prior explicit call already ran. Without this guard,
+ # the EXIT trap would re-run cleanup and fail on the already-unmounted point.
+ [[ $CLEANUP_DONE -eq 1 ]] && return 0
+ CLEANUP_DONE=1
Review Comment:
It deliberately sits at the top, and I've renamed it in af4fbb8 so that is
obvious. The flag means "cleanup has been entered", not "cleanup finished":
cleanup() can itself `exit $EXIT_CLEANUP_FAILED`, and that exit fires the EXIT
trap, which calls cleanup() again while the first call is still on the stack.
If the flag were set at the end, that second call would run the umount/rmdir a
second time (and fail, because the first call already unmounted). Now called
`CLEANUP_STARTED`, with the comment spelling this out.
--
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]
