Copilot commented on code in PR #12773:
URL: https://github.com/apache/cloudstack/pull/12773#discussion_r2923378227
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePool.java:
##########
@@ -91,6 +91,9 @@ default Long getUsedIops() {
public StoragePoolType getType();
+ default void setType(StoragePoolType type) {
Review Comment:
`setType` is introduced as a default no-op method. Call sites (e.g.,
`pool.setType(type)`) appear to rely on it to actually mutate the pool, but any
`KVMStoragePool` implementation that forgets to override it will silently
ignore the update. Consider making this an abstract method (and implementing it
across pool types) or at least documenting clearly that it must be overridden
when the implementation stores `type` internally.
```suggestion
/**
* Set the type of this storage pool.
* <p>
* Implementations that store the pool type internally must override this
* method to ensure {@link #getType()} reflects the updated value.
* The default implementation throws {@link
UnsupportedOperationException}
* to avoid silently ignoring type updates.
*
* @param type the {@link StoragePoolType} to set for this pool
*/
default void setType(StoragePoolType type) {
throw new UnsupportedOperationException("setType(StoragePoolType)
must be overridden by KVMStoragePool implementations that support changing the
pool type");
```
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -0,0 +1,205 @@
+#!/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
+
+# 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
+
+#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 [ -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; 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
+ :
+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:
In read-check mode, `interval` is expected to be provided (`-t`), but if it
is empty this logic will return DEAD for almost any normal `diff > 0` (which
will be true immediately after the first second). Consider making `-t`
mandatory when `-r` is used (fail fast with usage/help), or define a sensible
default interval and use it consistently.
```suggestion
# if no interval provided, use a sensible default interval (in seconds)
interval=60
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -320,13 +321,16 @@ public void setDetails(Map<String, String> details) {
@Override
public boolean isPoolSupportHA() {
- return type == StoragePoolType.NetworkFilesystem;
+ return
HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type);
}
public String getHearthBeatPath() {
- if (type == StoragePoolType.NetworkFilesystem) {
+ if (StoragePoolType.NetworkFilesystem.equals(type)) {
String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
return Script.findScript(kvmScriptsDir, "kvmheartbeat.sh");
+ } else if (StoragePoolType.SharedMountPoint.equals(type)) {
+ String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
+ return Script.findScript(kvmScriptsDir, "kvmsmpheartbeat.sh");
Review Comment:
`Script.findScript(...)` can return `null` if the heartbeat script is
missing. Returning `null` here will later result in constructing a `Script`
with a null command, which can fail in a less obvious way at runtime. Consider
checking for a null script path and failing fast with a clear exception/error
so mispackaging/misconfiguration is detected early.
```suggestion
String scriptPath = Script.findScript(kvmScriptsDir,
"kvmheartbeat.sh");
if (scriptPath == null) {
throw new CloudRuntimeException("Unable to find heartbeat
script 'kvmheartbeat.sh' in directory: " + kvmScriptsDir);
}
return scriptPath;
} else if (StoragePoolType.SharedMountPoint.equals(type)) {
String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
String scriptPath = Script.findScript(kvmScriptsDir,
"kvmsmpheartbeat.sh");
if (scriptPath == null) {
throw new CloudRuntimeException("Unable to find heartbeat
script 'kvmsmpheartbeat.sh' in directory: " + kvmScriptsDir);
}
return scriptPath;
```
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -0,0 +1,205 @@
+#!/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
+
+# 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
Review Comment:
The script creates the mount point directory (`mkdir -p`) before verifying
that the path is actually mounted. If the shared filesystem is not mounted,
subsequent heartbeat writes will succeed on the local filesystem, which can
mask a storage outage and prevent HA fencing logic from triggering. Consider
verifying the path is a real mount (e.g., `mountpoint -q` / `findmnt -M`) and
returning a non-zero exit status in write mode when it is not mounted (and
avoid creating the directory in that case).
```suggestion
# Ensure mount point is valid; in write mode require it to be a real mount
if [ ! -d "$MountPoint" ]; then
echo "Mount point directory does not exist: $MountPoint" >&2
exit 1
fi
# In write mode (default, rflag=0) verify that the path is an actual mount.
# This avoids writing heartbeats to a local directory when shared storage is
down.
if [ "$rflag" -ne 1 ]; then
mounted_ok=1
if command -v mountpoint >/dev/null 2>&1; then
mountpoint -q "$MountPoint" >/dev/null 2>&1
mounted_ok=$?
elif command -v findmnt >/dev/null 2>&1; then
findmnt -M "$MountPoint" >/dev/null 2>&1
mounted_ok=$?
else
# If neither mountpoint nor findmnt is available, fall back to directory
check only.
mounted_ok=0
fi
if [ "$mounted_ok" -ne 0 ]; then
echo "Mount point is not a mounted filesystem: $MountPoint" >&2
```
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -0,0 +1,205 @@
+#!/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
+
+# 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
+
+#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 [ -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; 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
+ :
Review Comment:
When the mount is not present, this branch does nothing and the script
continues, which means it can still write/read heartbeat files under an
unmounted local directory. For HA safety, missing mount should be treated as a
hard failure (exit non-zero) so the caller can retry/fence instead of operating
on local disk.
```suggestion
# mount point not present — treat as hard failure to avoid using local
disk for HA heartbeat
printf "Required mount point not present: %s\n" "$MountPoint" >&2
exit 2
```
##########
scripts/vm/hypervisor/kvm/kvmsmpheartbeat.sh:
##########
@@ -0,0 +1,205 @@
+#!/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
+
+# 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
+
+#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 [ -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; 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
+ :
+fi
+
+hbFolder="$MountPoint/KVMHA/"
Review Comment:
`hbFolder` already includes a trailing slash (`.../KVMHA/`), but `hbFile`
adds another `/`, producing paths like `.../KVMHA//hb-...`. This works on Linux
but is easy to overlook and can complicate log/debug output; consider
constructing `hbFile` without the extra separator or removing the trailing
slash from `hbFolder`.
```suggestion
hbFolder="$MountPoint/KVMHA"
```
--
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]
