abh1sar commented on code in PR #12133:
URL: https://github.com/apache/cloudstack/pull/12133#discussion_r3039940720


##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -218,6 +219,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"

Review Comment:
   Can't echo anything irrelevant here.
   the bash output in the success case is used to parse the backup size in 
LibvirtTakeBackupCommandWrapper
   ```
   
               String[] outputLines = result.second().trim().split("\n");
               for(String line : outputLines) {
                   backupSize = backupSize + Long.parseLong(line.split(" 
")[0].trim());
               }
   ```
   
   I think if unmount is failing even after waiting it's better to fail backup 
than to ignore it.
   It might lead to number of mounts building up. It's better to return error 
so that operator can resolve the underlying issue.
   If timeout is a problem, let's have a configurable unmount timeout, like we 
have mount timeout in LibvirtRestoreCommandWrapper (instead of hard coding to 
10s)
   



-- 
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]

Reply via email to