Copilot commented on code in PR #12773:
URL: https://github.com/apache/cloudstack/pull/12773#discussion_r2923702052
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -320,13 +321,24 @@ 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");
+ 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;
Review Comment:
`getHearthBeatPath()` now throws if `kvmsmpheartbeat.sh` is missing, but
agent startup validation in `LibvirtComputingResource` currently only checks
for `kvmheartbeat.sh`. This means a packaging/config issue for the new script
will surface later at runtime (during HA heartbeat) rather than failing fast
during agent configuration. Consider adding a startup-time check for
`kvmsmpheartbeat.sh` (or making `getHearthBeatPath()` return a failure message
in `createHeartBeatCommand` instead of throwing).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -390,6 +390,7 @@ public KVMStoragePool createStoragePool(String name, String
host, int port, Stri
private synchronized KVMStoragePool createStoragePool(String name, String
host, int port, String path, String userInfo, StoragePoolType type, Map<String,
String> details, boolean primaryStorage) {
StorageAdaptor adaptor = getStorageAdaptor(type);
KVMStoragePool pool = adaptor.createStoragePool(name, host, port,
path, userInfo, type, details, primaryStorage);
+ pool.setType(type);
Review Comment:
`pool.setType(type)` here looks like a workaround for libvirt pools (e.g.,
DIR pools) being mapped back to `StoragePoolType.Filesystem` in
`LibvirtStorageAdaptor.getStoragePool(...)`. To avoid having to remember to do
this in individual call sites (only some wrappers now do), consider setting the
type in `KVMStoragePoolManager.getStoragePool(StoragePoolType type, ...)`
before returning the pool (i.e., enforce the requested CloudStack pool type
centrally).
##########
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
Review Comment:
The mount check uses a regex with an unescaped `$MountPoint` (e.g., `.` or
`+` in the path would change the match), and relies on GNU `\+` in basic regex.
To avoid false positives/negatives across distros and unusual mount paths,
prefer a field-equality check (e.g., `awk '$2==mp {found=1} END{exit !found}'
mp="$MountPoint" /proc/mounts`) or `findmnt -T "$MountPoint"`.
```suggestion
if awk -v mp="$MountPoint" '$2 == mp { found=1; exit } END { exit !found }'
/proc/mounts
```
##########
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
Review Comment:
Creating the mount-point directory (`mkdir -p "$MountPoint"`) before
verifying the shared filesystem is actually mounted can mask storage outages:
the script can end up writing the heartbeat to the local root filesystem if the
mount is missing. This can prevent the agent from detecting a heartbeat write
failure (and therefore from fencing/rebooting when configured). Consider
validating that `$MountPoint` is a mount target (e.g., via `findmnt -T` or
exact field match in `/proc/mounts`) before creating/using the directory, and
fail with non-zero exit when it isn’t mounted.
```suggestion
# Ensure mount point exists and is a mounted filesystem
if [ ! -d "$MountPoint" ]; then
echo "Mount point directory does not exist: $MountPoint" >&2
exit 1
fi
# Verify that $MountPoint is an active mount point
if command -v findmnt >/dev/null 2>&1; then
# Use findmnt to ensure that the mount target exactly matches $MountPoint
if ! findmnt -n -o TARGET -T "$MountPoint" 2>/dev/null | grep -Fx --
"$MountPoint" >/dev/null 2>&1; then
echo "Mount point is not a mounted filesystem: $MountPoint" >&2
exit 1
fi
else
# Fallback: check /proc/mounts for an exact mountpoint match
if ! awk -v m="$MountPoint" '$2 == m {found=1} END {exit !found}'
/proc/mounts; then
echo "Mount point is not a mounted filesystem: $MountPoint" >&2
exit 1
fi
```
--
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]
