Copilot commented on code in PR #12133:
URL: https://github.com/apache/cloudstack/pull/12133#discussion_r2559957487
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -219,6 +220,34 @@ mount_operation() {
fi
}
Review Comment:
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).
```suggestion
# 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.
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -219,6 +220,34 @@ mount_operation() {
fi
}
+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
+ 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
+
+ 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"
Review Comment:
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.
```suggestion
log -ne "Warning: failed to unmount $mount_point, error: $umount_output"
return 1
```
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -219,6 +220,34 @@ mount_operation() {
fi
}
+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
Review Comment:
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.
```suggestion
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
```
--
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]
