danivogel90 commented on code in PR #14331:
URL: https://github.com/apache/cloudstack/pull/14331#discussion_r4216599761
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -80,51 +80,30 @@ if [ ! -d "$MountPoint" ]; then
exit 1
fi
-# If the 'mountpoint' utility is available, ensure this is an actual mount
-if command -v mountpoint >/dev/null 2>&1; then
- if ! mountpoint -q "$MountPoint"; then
- echo "Mount point is not a mounted filesystem: $MountPoint" >&2
- exit 1
+# Returns 0 if the given path resides on a mounted filesystem other than the
+# root filesystem. A SharedMountPoint path does not need to be a mount point
+# itself, it may be a subdirectory of a mounted (e.g. clustered) filesystem.
+is_on_mounted_fs() {
+ local target
+ if command -v findmnt >/dev/null 2>&1; then
+ target=$(findmnt -n -o TARGET -T "$1" 2>/dev/null)
+ else
+ target=$(df -P "$1" 2>/dev/null | awk 'NR==2 {print $6}')
fi
+ [ -n "$target" ] && [ "$target" != "/" ]
Review Comment:
Thanks, valid point for an exact pool mount nested below a separate local
filesystem. Fixed in d089836:
- If the path is a mount point itself, it is accepted as before (`mountpoint
-q`).
- Otherwise a subdirectory is only accepted if its filesystem is neither `/`
nor a local disk filesystem (ext*, xfs, btrfs, tmpfs, …).
The script does not receive the expected mount source for SharedMountPoint
(`-i`/`-p` are unused here), so it cannot compare against it; the filesystem
type is the closest signal available.
Tested: pool mounted exactly below a separate local filesystem → accepted;
after unmounting the pool → refused, nothing written locally. With acd5ba4 the
same case returned 0 and wrote the heartbeat to the local filesystem.
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -146,6 +125,9 @@ write_hbLog() {
timestamp=$(date +%s)
# Write atomically to avoid partial writes (write to tmp then mv)
tmpfile="${hbFile}.$$"
+ # remove the temporary file if the script is interrupted (e.g. on timeout)
+ trap 'rm -f "$tmpfile"' EXIT
+ trap 'exit 1' INT TERM
Review Comment:
Correct, the trap cannot run after `destroyForcibly()`. Rather than changing
`Script.execute()` for all scripts, d089836 removes leftover `hb-<host>.*` temp
files older than one minute at the start of the next write. The trap stays for
normal exits and TERM.
Tested: a 5-minute-old leftover is removed, a fresh one is kept.
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -168,12 +150,14 @@ check_hbLog() {
hb_diff=999998
return 1
fi
- diff=`expr $now - $hb 2>/dev/null`
- if [ $? -ne 0 ]
- then
- hb_diff=999997
- return 1
- fi
+ # note: 'expr' exits with 1 when the result is 0, so use shell arithmetic
+ case "$hb" in
+ ''|*[!0-9]*)
+ hb_diff=999997
+ return 1
+ ;;
+ esac
+ diff=$((now - hb))
Review Comment:
Fixed in d089836: the timestamp is limited to 12 digits and evaluated as
base 10 (`10#$hb`). `08`, a 19-digit value and `abc` now report DEAD; with
acd5ba4, `08` failed with `value too great for base`.
--
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]
