Skip to content

nasbackup.sh: add timeout, cleanup trap, space check, quiesce support #12843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jmsperu wants to merge 2 commits into
base: main
Choose a base branch
from
+133 −17
Open

nasbackup.sh: add timeout, cleanup trap, space check, quiesce support #12843

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27c7dc6
nasbackup.sh: add timeout, cleanup trap, free-space check (#12843)
jmsperu Oct 21, 2026
84c227c
address review (#12843): never delete a completed backup from the cle…
jmsperu Oct 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 133 additions & 17 deletions scripts/vm/hypervisor/kvm/nasbackup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,30 @@ PARENT_PATHS="" # For incremental: comma-separated list of parent backup f
# each disk must be rebased onto its own parent.
logFile="/var/log/cloudstack/agent/agent.log"
UNMOUNT_TIMEOUT=60
UNMOUNT_RETRY_DELAY=5
EXIT_CLEANUP_FAILED=20
BACKUP_JOB_ACTIVE=0

# Backup job timeout in seconds (default: 6 hours). Guards the libvirt
# domjobinfo wait loop so a stuck QEMU backup eventually fails the script
# instead of holding the agent's command slot indefinitely.
BACKUP_TIMEOUT=${BACKUP_TIMEOUT:-21600}

# Minimum free space required on the mounted backup target, in bytes
# (default: 1 GiB). Checked after mount, before any qemu-img convert, so
# we fail fast rather than mid-write when the NAS is near-full.
MIN_FREE_SPACE=${MIN_FREE_SPACE:-1073741824}

# Re-entrancy guard for cleanup(). It is set the moment cleanup() is ENTERED, not when it
# finishes: cleanup() can exit the script itself (EXIT_CLEANUP_FAILED), and that exit fires
# the EXIT trap, which would call cleanup() a second time while the first is still running.
CLEANUP_STARTED=0

# Set to 1 by the backup functions once every backup file is on the share, just before the
# final unmount. From then on cleanup() must never remove $dest: a failed or slow unmount
# after that point is an operational warning, not a failed backup.
BACKUP_COMPLETE=0

log() {
[[ "$verb" -eq 1 ]] && builtin echo "$@"
if [[ "$1" == "-ne" || "$1" == "-e" || "$1" == "-n" ]]; then
Expand Down Expand Up @@ -137,6 +158,41 @@ get_linstor_uuid_from_device() {
return 1
}

# Runs a command under timeout(1) when it is available, otherwise runs it directly.
run_with_timeout() {
local secs=$1
shift
if command -v timeout > /dev/null 2>&1; then
timeout "$secs" "$@"
else
"$@"
fi
}

# Unmounts the share after a completed backup. Tries a bounded umount twice, then a lazy
# umount as a last resort. Always returns 0: the backup on the share is intact, so a mount
# that lingers is logged as a warning and does not fail the backup.
unmount_completed_backup() {
local attempt
for ((attempt = 1; attempt <= 2; attempt++)); do
if run_with_timeout "$UNMOUNT_TIMEOUT" umount "$mount_point" 2>>"$logFile"; then
rmdir "$mount_point" 2>>"$logFile" || log "WARNING: rmdir of $mount_point failed"
return 0
fi
log "WARNING: umount attempt $attempt of $mount_point failed or timed out"
if [[ $attempt -eq 1 ]]; then
sleep "$UNMOUNT_RETRY_DELAY"
fi
done
if run_with_timeout "$UNMOUNT_TIMEOUT" umount -l "$mount_point" 2>>"$logFile"; then
log "WARNING: lazily unmounted $mount_point after umount failures, the backup is intact"
rmdir "$mount_point" 2>>"$logFile" || log "WARNING: rmdir of $mount_point failed"
else
log "WARNING: $mount_point is still mounted after umount failures, the backup is intact"
fi
return 0
}

qemu_agent_command() {
local timeout_args=()
if [[ -n "$QUIESCE_TIMEOUT" ]]; then
Expand Down Expand Up @@ -186,6 +242,7 @@ thaw_guest() {

backup_running_vm() {
mount_operation
check_free_space
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; }

# Determine effective mode for this run.
Expand Down Expand Up @@ -352,8 +409,18 @@ print(len(files))
virsh -c qemu:///system domiflist $VM > $dest/domiflist.xml 2>/dev/null
virsh -c qemu:///system domblklist $VM > $dest/domblklist.xml 2>/dev/null

# Bound the wait so a stuck QEMU backup eventually aborts instead of holding
# the agent's command slot until the orchestrator-level timeout fires. The
# deadline also covers the query retries below.
local info query_failures=0
local deadline=$((SECONDS + BACKUP_TIMEOUT))
while true; do
if (( SECONDS >= deadline )); then
echo "Backup timed out after ${BACKUP_TIMEOUT}s for VM $VM"
# cleanup aborts the job (abort_backup_job) before it removes $dest
cleanup
exit 1
fi
# A failed query says nothing about the job, so retry before giving up on it.
if ! info=$(virsh -c qemu:///system domjobinfo $VM --completed --keep-completed 2>&1); then
log -e "Backup job query $((query_failures + 1)) for vm $VM failed: $info"
Expand Down Expand Up @@ -485,15 +552,17 @@ for dev in data.get("return", []) or []:
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
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; }
# The backup is complete from here on: cleanup() must not delete it if the unmount fails.
BACKUP_COMPLETE=1
unmount_completed_backup
echo "$backup_size"
}

backup_stopped_vm() {
# Stopped VMs cannot use libvirt's backup-begin (no QEMU process); take a full backup via
# qemu-img convert. The orchestrator never sends incremental mode for a stopped VM.
mount_operation
check_free_space
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; }

IFS=","
Comment thread
jmsperu marked this conversation as resolved.
Expand Down Expand Up @@ -541,6 +610,10 @@ backup_stopped_vm() {
sync

find "$dest" -type f -exec stat -c '%s' {} +

# The backup is complete from here on: cleanup() must not delete it if the unmount fails.
BACKUP_COMPLETE=1
unmount_completed_backup
}

delete_backup() {
Expand All @@ -567,8 +640,9 @@ mount_operation() {
if [ ${NAS_TYPE} == "cifs" ]; then
MOUNT_OPTS="${MOUNT_OPTS},nobrl"
fi
mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) 2>&1 | tee -a "$logFile"
if [ $? -eq 0 ]; then
# Test mount's exit directly: the old `mount | tee` form let pipefail abort before the
# check, and `$?` saw tee's status (always 0), masking mount failures.
if mount -t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}" $([[ -n "${MOUNT_OPTS}" ]] && echo -o "${MOUNT_OPTS}") >> "$logFile" 2>&1; then
log -ne "Successfully mounted ${NAS_TYPE} store"
else
echo "Failed to mount ${NAS_TYPE} store"
Expand All @@ -595,19 +669,40 @@ abort_backup_job() {
return 1
}

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() {
# Mark "started" first (see CLEANUP_STARTED above): an explicit call followed by exit, or an
# 'exit $EXIT_CLEANUP_FAILED' from inside this function, both re-enter here via the EXIT trap.
[[ $CLEANUP_STARTED -eq 1 ]] && return 0
CLEANUP_STARTED=1

local status=0

# Resume the VM if it was paused during backup to prevent it from
# remaining indefinitely paused when the backup job fails (e.g. due
# to storage full or I/O errors on the backup target)
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
# 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

Expand All @@ -617,16 +712,37 @@ 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; }
# Only an unfinished backup is removed. A completed backup is kept even when its unmount
# failed, and delete/stats never remove anything here: for stats $dest can be the root of
# the backup store, and delete has already removed what it was asked to.
if [[ "$OP" == "backup" && $BACKUP_COMPLETE -eq 0 && -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
if [[ $BACKUP_COMPLETE -eq 1 ]]; then
# unmount_completed_backup already retried and logged; do not fail a good backup.
umount "$mount_point" 2>/dev/null || log "WARNING: $mount_point is still mounted, the backup is intact"
else
umount "$mount_point" 2>/dev/null || { echo "Failed to unmount $mount_point"; status=1; }
fi
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

@Damans227 Damans227 Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmsperu if the last unmount of a running vm backup fails or times out, wont this trap delete the finished backup? the script carries on, so it can still report success

@jmsperu jmsperu Oct 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 84c227c:

  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.


function usage {
echo ""
echo "Usage: $0 -o <operation> -v|--vm <domain name> -t <storage type> -s <storage address> -m <mount options> -p <backup path> -d <disks path> -q|--quiesce <true|false>"
Expand Down
Loading