Copilot commented on code in PR #14331:
URL: https://github.com/apache/cloudstack/pull/14331#discussion_r4207915715
##########
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:
The digit check still admits values that Bash arithmetic cannot parse as
written. A corrupt heartbeat such as `08` is treated as octal and aborts here
with `value too great for base`, so this path emits no `DEAD` result and does
not satisfy the new corrupt-file handling. Parse explicitly as base 10 and
bound the length before arithmetic to avoid both octal errors and integer
overflow.
##########
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:
The timeout path cannot trigger this cleanup: the heartbeat script is
launched through `Script.execute()`, whose timeout handling ends by calling
`Process.destroyForcibly()`
(`utils/src/main/java/com/cloud/utils/script/Script.java:393-400`). A forced
kill cannot run `TERM` or `EXIT` traps, so the temporary file remains in the
exact recovery-timeout scenario this change is intended to fix. The caller
needs a graceful termination phase (with forced kill only after a grace
period), or the temp-file strategy needs cleanup that does not depend on
process-exit traps.
##########
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:
This only proves that the path is on some non-root filesystem, not that the
configured shared filesystem is still mounted. For example, with `/var` on
local ext4 and the shared pool mounted below it, loss of the shared mount makes
`findmnt` return `/var`; this condition still succeeds and the heartbeat is
written locally, suppressing the writer's fencing path. Unlike the previous
exact `mountpoint` check, that also regresses exact pool mounts nested under a
local filesystem. Please validate the expected mount/source or a
shared-filesystem identity rather than accepting every non-root target.
--
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]
