Copilot commented on code in PR #12773: URL: https://github.com/apache/cloudstack/pull/12773#discussion_r2924150004
########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,218 @@ +#!/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 this script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for this script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Validate mount point exists, is (if possible) a mounted filesystem, and is writable +if [ ! -d "$MountPoint" ]; then + echo "Mount point directory does not exist: $MountPoint" >&2 + 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 + fi +fi + +# Ensure the mount point is writable +if [ ! -w "$MountPoint" ]; then + echo "Mount point is not writable: $MountPoint" >&2 + exit 1 +fi +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + # ensure it ends with a single trailing slash + mountPoint="${mountPoint%/}/" + + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + + 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? +if grep -q "^[^ ]\+ $MountPoint " /proc/mounts +then + # mount exists; nothing to do here; keep for compatibility with original flow + : +else + # mount point not present + # 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() { + hb_diff=0 + if [ ! -f "$hbFile" ]; then + # signal large difference if file missing + hb_diff=999999 + return 1 + fi + now=$(date +%s) + hb=$(cat "$hbFile" 2>/dev/null) + if [ -z "$hb" ]; then + hb_diff=999998 + return 1 + fi + diff=`expr $now - $hb 2>/dev/null` + if [ $? -ne 0 ] + then + hb_diff=999997 + return 1 + fi + if [ -z "$interval" ]; then + # if no interval provided, consider 0 as success + if [ $diff -gt 0 ]; then + hb_diff=$diff + return 1 + else + hb_diff=0 + return 0 + fi Review Comment: The `-t` (interval) empty-path logic contradicts the comment and will almost always mark the host as DEAD because `diff` will normally be > 0. Either make `-t` mandatory for read-check mode (`-r`) and fail fast when missing, or treat a missing interval as a success case (e.g., only validate that the file contains a valid timestamp, optionally returning `hb_diff=$diff` without failing). ```suggestion # if no interval provided, treat any valid timestamp as success; # just report the time difference without failing based on staleness hb_diff=$diff return 0 ``` ########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,218 @@ +#!/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 this script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for this script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Validate mount point exists, is (if possible) a mounted filesystem, and is writable +if [ ! -d "$MountPoint" ]; then + echo "Mount point directory does not exist: $MountPoint" >&2 + 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 + fi +fi + +# Ensure the mount point is writable +if [ ! -w "$MountPoint" ]; then + echo "Mount point is not writable: $MountPoint" >&2 + exit 1 +fi +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + # ensure it ends with a single trailing slash + mountPoint="${mountPoint%/}/" + + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + + if [ -z "$vmPids" ] + then + return + fi + + for pid in $vmPids + do + kill -9 $pid &> /dev/null + done Review Comment: This process selection is fragile (can match the grep pipeline itself and may miss qemu processes depending on cmdline format), and always uses `kill -9` (no grace period), which can increase corruption risk. Prefer a more reliable match (e.g., a single pattern match that avoids self-matching) and attempt a graceful shutdown first (SIGTERM with a short wait) before SIGKILL as a fallback. ########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,218 @@ +#!/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 this script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for this script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Validate mount point exists, is (if possible) a mounted filesystem, and is writable +if [ ! -d "$MountPoint" ]; then + echo "Mount point directory does not exist: $MountPoint" >&2 + 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 + fi +fi + +# Ensure the mount point is writable +if [ ! -w "$MountPoint" ]; then + echo "Mount point is not writable: $MountPoint" >&2 + exit 1 +fi +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + # ensure it ends with a single trailing slash + mountPoint="${mountPoint%/}/" + + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + + 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? +if grep -q "^[^ ]\+ $MountPoint " /proc/mounts +then + # mount exists; nothing to do here; keep for compatibility with original flow + : +else + # mount point not present + # 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 Review Comment: These `printf` error messages do not include a trailing newline, which can make logs harder to read/parse. Add `\\n` to both messages (and consider including context like mount point or hbFile) to improve operational readability. ########## plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java: ########## @@ -410,4 +422,8 @@ public Boolean vmActivityCheck(HAStoragePool pool, HostTO host, Duration activit return true; } } + + public void setType(StoragePoolType type) { Review Comment: Introducing a public setter for `type` makes `LibvirtStoragePool` mutable after construction. Since pools are commonly cached and accessed across threads, mutating `type` can lead to races/inconsistent HA behavior (e.g., `isPoolSupportHA()` or `getHearthBeatPath()` observing an unexpected or transient value). Prefer initializing `type` during pool construction (or via a single-threaded initialization path) and make it immutable; if mutation is unavoidable, enforce thread-safety (e.g., synchronization or `volatile`) and constrain visibility of the setter. ```suggestion void setType(StoragePoolType type) { ``` ########## scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh: ########## @@ -0,0 +1,218 @@ +#!/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 this script + p) + NfsSvrPath="$OPTARG" + ;; # retained for CLI compatibility but unused for this script + m) + MountPoint="$OPTARG" + ;; + h) + HostIP="$OPTARG" + ;; + r) + rflag=1 + ;; + t) + interval="$OPTARG" + ;; + c) + cflag=1 + ;; + *) + help + ;; + esac +done + +# For heartbeat we require a mountpoint +if [ -z "$MountPoint" ] +then + echo "Mount point (-m) is required" + help +fi + +# Validate mount point exists, is (if possible) a mounted filesystem, and is writable +if [ ! -d "$MountPoint" ]; then + echo "Mount point directory does not exist: $MountPoint" >&2 + 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 + fi +fi + +# Ensure the mount point is writable +if [ ! -w "$MountPoint" ]; then + echo "Mount point is not writable: $MountPoint" >&2 + exit 1 +fi +#delete VMs on this mountpoint (best-effort) +deleteVMs() { + local mountPoint=$1 + # ensure it ends with a single trailing slash + mountPoint="${mountPoint%/}/" + + vmPids=$(ps aux | grep qemu | grep "$mountPoint" | awk '{print $2}' 2> /dev/null) + + 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? +if grep -q "^[^ ]\+ $MountPoint " /proc/mounts +then + # mount exists; nothing to do here; keep for compatibility with original flow + : +else + # mount point not present + # 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 Review Comment: These `printf` error messages do not include a trailing newline, which can make logs harder to read/parse. Add `\\n` to both messages (and consider including context like mount point or hbFile) to improve operational readability. ########## engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java: ########## @@ -32,6 +33,8 @@ */ public interface HighAvailabilityManager extends Manager { + List<StoragePoolType> LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT = List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint); Review Comment: This constant is used as a membership test (`contains`) across the codebase. Representing it as a `Set<StoragePoolType>` (ideally `EnumSet`) makes intent clearer and avoids linear scans as the supported types list grows. Consider changing this to a `Set` and updating call sites accordingly. -- 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]
