Skip to content

Creating a unmount_operation with safety checks for nasbackup.sh #12133

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
Hanarion wants to merge 1 commit into
base: 4.22
Choose a base branch
from
+31 −2
Open

Creating a unmount_operation with safety checks for nasbackup.sh #12133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 31 additions & 2 deletions scripts/vm/hypervisor/kvm/nasbackup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ backup_running_vm() {
virsh -c qemu:///system domjobinfo $VM --completed
du -sb $dest | cut -f1

umount $mount_point
rmdir $mount_point
umount_operation
}

backup_stopped_vm() {
Expand Down Expand Up @@ -183,6 +182,8 @@ backup_stopped_vm() {
sync

ls -l --numeric-uid-gid $dest | awk '{print $5}'

umount_operation
}

delete_backup() {
Expand Down Expand Up @@ -218,6 +219,34 @@ mount_operation() {
fi
}

Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The umount_operation() function lacks documentation explaining its purpose, behavior, and return value. Consider adding a comment block describing: (1) that it waits up to 10 seconds for the mount point to become idle, (2) that it attempts to unmount and remove the directory, and (3) its error handling behavior (currently does not fail the script on unmount failure).

Suggested change
# umount_operation
# ----------------
# Waits up to 10 seconds for the mount point to become idle (not in use).
# Attempts to unmount the specified mount point. If unmounting succeeds,
# removes the mount directory. If unmounting fails, logs a warning and
# does not remove the directory. This function does not cause the script
# to exit on unmount failure; the script continues execution.

Copilot uses AI. Check for mistakes.
umount_operation() {
elapsed=0
while fuser -m "$mount_point" >/dev/null 2>&1 && (( elapsed < 10 )); do
sleep 1
elapsed=$((elapsed + 1))
done

# Check if timeout was reached
if (( elapsed >= 10 )); then
Comment on lines +223 to +230
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The timeout value 10 is hardcoded twice in the function (line 225 and line 231). If the timeout needs to be changed in the future, it must be updated in both places, increasing the risk of inconsistency. Consider using a local variable at the start of the function (e.g., local timeout=10) and referencing it in both locations.

Suggested change
elapsed=0
while fuser -m "$mount_point" >/dev/null 2>&1 && (( elapsed < 10 )); do
sleep 1
elapsed=$((elapsed + 1))
done
# Check if timeout was reached
if (( elapsed >= 10 )); then
local timeout=10
elapsed=0
while fuser -m "$mount_point" >/dev/null 2>&1 && (( elapsed < timeout )); do
sleep 1
elapsed=$((elapsed + 1))
done
# Check if timeout was reached
if (( elapsed >= timeout )); then

Copilot uses AI. Check for mistakes.
echo "Timeout for unmounting reached: still busy"
fi

# Attempt to unmount safely and capture output
set +e
umount_output=$(umount "$mount_point" 2>&1)
umount_exit=$?
set -e
Copy link
Contributor

@DaanHoogland DaanHoogland Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

not sure if we should exit on error. Am I reading this wrong?

Copy link
Author

@Hanarion Hanarion Mar 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The script was set to exit on error at the start of the file, but when unmounting fails it does not give any meaningful error output, capturing the output allows to generate a log and an error message that is verbose and allows the administrator to know there is an issue.
Also when umount fails and stops the script, the backup is reported as failed and can't be deleted from Cloudstack as in the DB of Cloudstack this backup is failed and not created. Removing the exit on error and instead logging allows the backup to be marked as success.
In most of the cases, the umount should not fail, as we are waiting before for the fs to be synced. But in the case it does, it allows the backup to be usable


if [ "$umount_exit" -eq 0 ]; then
# Only remove directory if unmount succeeded
rmdir "$mount_point"
else
echo "Warning: failed to unmount $mount_point, skipping rmdir"
echo "umount error message: $umount_output"
log -ne "Warning: failed to unmount $mount_point, error: $umount_output"
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The function does not return a non-zero exit code when unmount fails. Since the script uses set -eo pipefail at the top, and this function temporarily disables errors with set +e, a failed unmount will not cause the script to exit with an error status. This means backup jobs will report success even when the unmount fails, which contradicts the PR's goal of fixing "random backup failures." Consider adding return 1 or exit 1 in the else branch (lines 244-248) to ensure proper error propagation.

Suggested change
log -ne "Warning: failed to unmount $mount_point, error: $umount_output"
log -ne "Warning: failed to unmount $mount_point, error: $umount_output"
return 1

Copilot uses AI. Check for mistakes.
fi
}

cleanup() {
local status=0

Expand Down
Loading