Copilot commented on code in PR #12773: URL: https://github.com/apache/cloudstack/pull/12773#discussion_r2923100207
########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,212 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +help() { + printf "Usage: $0 + -i identifier (required for CLI compatibility; value ignored by local-only heartbeat) + -p path (required for CLI compatibility; value ignored by local-only heartbeat) + -m mount point (local path where heartbeat will be written) + -h host (host IP/name to include in heartbeat filename) + -r write/read hb log (read-check mode) + -c cleanup (trigger emergency reboot) + -t interval between read hb log\n" + exit 1 +} + +#set -x +NfsSvrIP= +NfsSvrPath= +MountPoint= +HostIP= +interval= +rflag=0 +cflag=0 + +while getopts 'i:p:m:h:t:rc' OPTION +do + case $OPTION in + i) + NfsSvrIP="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For local-only heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Ensure mount point exists and is writable +if [ ! -d "$MountPoint" ]; then + mkdir -p "$MountPoint" 2>/dev/null + if [ $? -ne 0 ]; then + echo "Failed to create mount point directory: $MountPoint" >&2 + exit 1 + fi +fi + +# Determine a sensible HostIP if not provided +if [ -z "$HostIP" ]; then + # try to get a non-loopback IPv4 address, fallback to hostname + ipaddr=$(hostname -I 2>/dev/null | awk '{print $1}') + if [ -n "$ipaddr" ]; then + HostIP="$ipaddr" + else + HostIP=$(hostname) + fi +fi + +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + if [ $? -gt 0 ] + then + return + fi + + if [ -z "$vmPids" ] + then + return + fi + + for pid in $vmPids + do + kill -9 $pid &> /dev/null + done +} + +#checking is there the mount point present under $MountPoint? +mounts=$(cat /proc/mounts | grep "$MountPoint") +if [ $? -gt 0 ] +then + # mount point not present — we don't remount in local-only script + # nothing to do here; keep for compatibility with original flow + : +else + # mount exists; if not in read-check mode, consider deleting VMs similar to original behavior + if [ "$rflag" == "0" ] + then + deleteVMs $MountPoint + fi +fi + Review Comment: `deleteVMs` is executed when the mount *exists* (the `grep` succeeds) and the script is in write mode. In the original `kvmheartbeat.sh`, VMs are only killed after a remount attempt when the expected mount is missing. As written, this can kill running VMs on every successful heartbeat run whenever the storage is mounted. The condition should be inverted/adjusted so VM killing is only done for the intended failure/remount scenario (or removed entirely for the local-only heartbeat flow). ```suggestion # mount exists; in local-only heartbeat we do not kill VMs based on mount state # original kvmheartbeat.sh only deleted VMs after a failed remount; there is # no such remount logic here, so VM deletion is intentionally disabled : fi ``` ########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,212 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +help() { + printf "Usage: $0 + -i identifier (required for CLI compatibility; value ignored by local-only heartbeat) + -p path (required for CLI compatibility; value ignored by local-only heartbeat) + -m mount point (local path where heartbeat will be written) + -h host (host IP/name to include in heartbeat filename) + -r write/read hb log (read-check mode) + -c cleanup (trigger emergency reboot) + -t interval between read hb log\n" + exit 1 +} + +#set -x +NfsSvrIP= +NfsSvrPath= +MountPoint= +HostIP= +interval= +rflag=0 +cflag=0 + +while getopts 'i:p:m:h:t:rc' OPTION +do + case $OPTION in + i) + NfsSvrIP="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For local-only heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Ensure mount point exists and is writable +if [ ! -d "$MountPoint" ]; then + mkdir -p "$MountPoint" 2>/dev/null + if [ $? -ne 0 ]; then + echo "Failed to create mount point directory: $MountPoint" >&2 + exit 1 + fi +fi + +# Determine a sensible HostIP if not provided +if [ -z "$HostIP" ]; then + # try to get a non-loopback IPv4 address, fallback to hostname + ipaddr=$(hostname -I 2>/dev/null | awk '{print $1}') + if [ -n "$ipaddr" ]; then + HostIP="$ipaddr" + else + HostIP=$(hostname) + fi +fi + +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + if [ $? -gt 0 ] + then + return + fi + + if [ -z "$vmPids" ] + then + return + fi + + for pid in $vmPids + do + kill -9 $pid &> /dev/null + done +} + +#checking is there the mount point present under $MountPoint? +mounts=$(cat /proc/mounts | grep "$MountPoint") +if [ $? -gt 0 ] +then + # mount point not present — we don't remount in local-only script + # nothing to do here; keep for compatibility with original flow + : +else + # mount exists; if not in read-check mode, consider deleting VMs similar to original behavior + if [ "$rflag" == "0" ] + then + deleteVMs $MountPoint + fi Review Comment: The mount-point detection uses `grep "$MountPoint"` on `/proc/mounts`, which can yield false positives when the mount point is a prefix of another path (e.g. `/mnt/cloudstack1` matching `/mnt/cloudstack10`). This can cause the script to treat the mount as present/absent incorrectly and trigger follow-on actions. Use an exact match on the mount target field (e.g. `findmnt -n -o TARGET --target "$MountPoint"` or matching the second column in `/proc/mounts`). ```suggestion if grep -q "^[^ ]\+ $MountPoint " /proc/mounts then # mount exists; if not in read-check mode, consider deleting VMs similar to original behavior if [ "$rflag" == "0" ] then deleteVMs $MountPoint fi else # mount point not present — we don't remount in local-only script # nothing to do here; keep for compatibility with original flow : ``` ########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,212 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +help() { + printf "Usage: $0 + -i identifier (required for CLI compatibility; value ignored by local-only heartbeat) + -p path (required for CLI compatibility; value ignored by local-only heartbeat) + -m mount point (local path where heartbeat will be written) + -h host (host IP/name to include in heartbeat filename) + -r write/read hb log (read-check mode) + -c cleanup (trigger emergency reboot) + -t interval between read hb log\n" + exit 1 +} + +#set -x +NfsSvrIP= +NfsSvrPath= +MountPoint= +HostIP= +interval= +rflag=0 +cflag=0 + +while getopts 'i:p:m:h:t:rc' OPTION +do + case $OPTION in + i) + NfsSvrIP="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for local-only script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For local-only heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Ensure mount point exists and is writable +if [ ! -d "$MountPoint" ]; then + mkdir -p "$MountPoint" 2>/dev/null + if [ $? -ne 0 ]; then + echo "Failed to create mount point directory: $MountPoint" >&2 + exit 1 + fi +fi + +# Determine a sensible HostIP if not provided +if [ -z "$HostIP" ]; then + # try to get a non-loopback IPv4 address, fallback to hostname + ipaddr=$(hostname -I 2>/dev/null | awk '{print $1}') + if [ -n "$ipaddr" ]; then + HostIP="$ipaddr" + else + HostIP=$(hostname) + fi +fi + +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + if [ $? -gt 0 ] + then + return + fi + + if [ -z "$vmPids" ] + then + return + fi + + for pid in $vmPids + do + kill -9 $pid &> /dev/null + done +} + +#checking is there the mount point present under $MountPoint? +mounts=$(cat /proc/mounts | grep "$MountPoint") +if [ $? -gt 0 ] +then + # mount point not present — we don't remount in local-only script + # nothing to do here; keep for compatibility with original flow + : +else + # mount exists; if not in read-check mode, consider deleting VMs similar to original behavior + if [ "$rflag" == "0" ] + then + deleteVMs $MountPoint + fi +fi + +hbFolder="$MountPoint/KVMHA/" +hbFile="$hbFolder/hb-$HostIP" + +write_hbLog() { +#write the heart beat log + stat "$hbFile" &> /dev/null + if [ $? -gt 0 ] + then + # create a new one + mkdir -p "$hbFolder" &> /dev/null + # touch will be done by atomic write below; ensure folder is writable + if [ ! -w "$hbFolder" ]; then + printf "Folder not writable: $hbFolder" >&2 + return 2 + fi + fi + + timestamp=$(date +%s) + # Write atomically to avoid partial writes (write to tmp then mv) + tmpfile="${hbFile}.$$" + printf "%s\n" "$timestamp" > "$tmpfile" 2>/dev/null + if [ $? -ne 0 ]; then + printf "Failed to write heartbeat to $tmpfile" >&2 + return 2 + fi + mv -f "$tmpfile" "$hbFile" 2>/dev/null + return $? +} + +check_hbLog() { + if [ ! -f "$hbFile" ]; then + # signal large difference if file missing + return 999999 + fi + now=$(date +%s) + hb=$(cat "$hbFile" 2>/dev/null) + if [ -z "$hb" ]; then + return 999998 + fi + diff=`expr $now - $hb 2>/dev/null` + if [ $? -ne 0 ] + then + return 999997 + fi + if [ -z "$interval" ]; then + # if no interval provided, consider 0 as success + if [ $diff -gt 0 ]; then + return $diff + else + return 0 + fi + fi + if [ $diff -gt $interval ] + then + return $diff + fi + return 0 +} + +if [ "$rflag" == "1" ] +then + check_hbLog + diff=$? + if [ $diff == 0 ] Review Comment: `check_hbLog` returns large numeric values (e.g. `return 999999` / `return $diff`) and the caller reads them via `$?`. In bash, function return/exit codes are limited to 0-255, so values wrap modulo 256 and can incorrectly report ALIVE (e.g. a diff of 512 seconds becomes exit code 0). Use stdout to pass the diff (or store it in a variable) and use the exit status purely as a boolean (alive/dead). ```suggestion echo 999999 return 1 fi now=$(date +%s) hb=$(cat "$hbFile" 2>/dev/null) if [ -z "$hb" ]; then echo 999998 return 1 fi diff=`expr $now - $hb 2>/dev/null` if [ $? -ne 0 ] then echo 999997 return 1 fi if [ -z "$interval" ]; then # if no interval provided, consider 0 as success if [ $diff -gt 0 ]; then echo "$diff" return 1 else echo 0 return 0 fi fi if [ $diff -gt $interval ] then echo "$diff" return 1 fi echo 0 return 0 } if [ "$rflag" == "1" ] then diff=$(check_hbLog) status=$? if [ "$status" -eq 0 ] ``` -- 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]
