Copilot commented on code in PR #14320:
URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4230582510
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java:
##########
@@ -54,7 +55,11 @@ public void removeStoragePool(String uuid) {
synchronized (haStoragePools) {
HAStoragePool pool = haStoragePools.get(uuid);
if (pool != null) {
- Script.runSimpleBashScript("umount " +
pool.getMountDestPath());
+ if (StringUtils.isNotEmpty(pool.getMountDestPath())) {
+ Script umount = new Script("umount", logger);
+ umount.add(pool.getMountDestPath());
+ umount.execute();
Review Comment:
The `umount` result is currently ignored. If unmount fails (busy mount,
transient issue), the pool is still removed from the HA monitor and the mount
may leak, potentially impacting future operations on that path. Consider
checking the `execute()` return value and logging a warning/error (and possibly
keeping the pool entry until unmount succeeds, depending on desired semantics).
##########
scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh:
##########
@@ -0,0 +1,229 @@
+#!/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.
+
+# Ceph RBD flavor of kvmvmactivity.sh.
+#
+# On NFS/SharedMountPoint storage, VM disk activity is detected via the mtime
+# of the volume files on the shared mount point. RBD volumes aren't files on
+# a mount point, so instead activity is detected via RBD watchers: as long as
+# qemu has an RBD image open (i.e. a VM using that volume is running
+# somewhere), the image will have a live watcher. The most recent
+# suspect-time/watcher-state is persisted as a RADOS object (per host)
+# in place of the "ac-<host>" file used by the NFS/SMP scripts.
+
+help() {
+ printf "Usage: $0
+ -s ceph monitor host(s), comma separated
+ -o ceph/rbd pool name
+ -n cephx auth user (optional)
+ -k cephx auth key (optional, required if -n is set)
+ -h host
+ -u volume (rbd image) uuid list
+ -t current time in seconds (accepted for compatibility
with kvmvmactivity.sh, not used)
+ -d suspect time\n"
+ exit 1
+}
+
+#set -x
+
+MonHosts=
+PoolName=
+CephUser=
+CephKey=
+HostIP=
+UUIDList=
+SuspectTime=
+
+while getopts 's:o:n:k:h:u:t:d:' OPTION
+do
+ case $OPTION in
+ s)
+ MonHosts="$OPTARG"
+ ;;
+ o)
+ PoolName="$OPTARG"
+ ;;
+ n)
+ CephUser="$OPTARG"
+ ;;
+ k)
+ CephKey="$OPTARG"
+ ;;
+ h)
+ HostIP="$OPTARG"
+ ;;
+ u)
+ UUIDList="$OPTARG"
+ ;;
+ t)
+ # not used, see help
+ ;;
+ d)
+ SuspectTime="$OPTARG"
+ ;;
+ *)
+ help
+ ;;
+ esac
+done
+
+if [ -z "$MonHosts" ] || [ -z "$PoolName" ]
+then
+ exit 2
+fi
+
+if [ -z "$SuspectTime" ]
+then
+ exit 2
+fi
+
+# the host IP names the heartbeat and activity objects
+if [ -z "$HostIP" ]
+then
+ exit 2
+fi
+
+if [ -n "$CephUser" ] && [ -z "$CephKey" ]
+then
+ exit 2
+fi
+
+RadosOpts=(--mon-host "$MonHosts")
+RbdOpts=(--mon-host "$MonHosts")
+if [ -n "$CephUser" ]
+then
+ # the key is given to rados and rbd in a file, to keep it out of the
process list
+ KeyFile=$(mktemp)
+ trap 'rm -f "$KeyFile"' EXIT
+ printf '%s' "$CephKey" > "$KeyFile"
+ RadosOpts+=(--id "$CephUser" --keyfile "$KeyFile")
+ RbdOpts+=(--id "$CephUser" --keyfile "$KeyFile")
+fi
+
+hbObject="KVMHA-hb-$HostIP"
+acObject="KVMHA-ac-$HostIP"
+
+# the reason why the heartbeat did not show that the host is alive, added to
the messages about a DEAD host
+hbNote=
+
+dead() {
+ echo "=====> Considering host as DEAD due to $1.$hbNote <======"
+}
+
+# First check: heartbeat object, same as kvmheartbeat_rbd.sh
+now=$(date +%s)
+if hb=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$hbObject" - 2> /dev/null)
+then
+ if [[ "$hb" =~ ^[0-9]+$ ]]
+ then
+ diff=$(expr $now - $hb)
+ if [ $diff -lt 61 ]
+ then
+ echo "=====> ALIVE <====="
+ exit 0
+ fi
+ hbNote=" The heartbeat in RADOS object [$hbObject] is [$diff] seconds old."
+ else
+ hbNote=" The RADOS object [$hbObject] does not hold a heartbeat."
+ fi
+else
+ hbNote=" The RADOS object [$hbObject] could not be read, it does not exist
or Ceph can not be reached."
+fi
+
+if [ -z "$UUIDList" ]
+then
+ dead "empty UUIDList"
+ exit 0
+fi
+
+# Second check: RBD watcher based disk activity check.
+# If any of the host's volumes still has a live watcher, something (most
+# likely qemu on the host being checked) is actively using it right now.
+latestUpdateTime=0
+IFS=',' read -ra images <<< "$UUIDList"
+for image in "${images[@]}"
+do
+ image=${image//[[:space:]]/}
+ if [ -z "$image" ]
+ then
+ continue
+ fi
+ # when the watchers can not be listed, it is unknown whether the host is
alive: the script fails, which
+ # is not a DEAD host
+ if ! watchers=$(rbd status "$PoolName/$image" "${RbdOpts[@]}" 2> /dev/null)
+ then
+ echo "=====> Unable to get the watchers of the image [$PoolName/$image]
<======"
+ exit 2
+ fi
+ # "Watchers: none", or a line "watcher=<address> client.<id>
cookie=<cookie>" per watcher
+ if grep -q '^[[:space:]]*watcher=' <<< "$watchers"
+ then
+ latestUpdateTime=$now
+ break
+ fi
+done
+
+acTime=
+if rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" &> /dev/null
+then
+ if ! acTime=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$acObject" - 2>
/dev/null)
+ then
+ echo "=====> Unable to read the RADOS object [$acObject] <======"
+ exit 2
+ fi
+fi
+# a state which is not "<suspect time>:<update time>" (with an optional third
field of older versions)
+# is ignored, as if there was none
+if ! [[ "$acTime" =~ ^[0-9]+:[0-9]+(:[0-9]+)?$ ]]
+then
+ acTime=
+fi
+
+tmpFile=$(mktemp)
+echo "$SuspectTime:$latestUpdateTime" > "$tmpFile"
+rados -p "$PoolName" "${RadosOpts[@]}" put "$acObject" "$tmpFile" &> /dev/null
Review Comment:
The script ignores the result of updating the activity state object (`rados
... put ...`). If this write fails, the next run may use stale/missing state
and incorrectly conclude DEAD/ALIVE. Capture and check the `rados put` exit
status; if it fails, print an error and exit with a non-zero status used for
'unknown/inconclusive' (consistent with the earlier `exit 2` on watcher-read
failures).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -330,29 +332,94 @@ public boolean isPoolSupportHA() {
public String getHearthBeatPath() {
if (StoragePoolType.NetworkFilesystem.equals(type)) {
- String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
- 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;
+ return findKvmHaScript("kvmheartbeat.sh");
} 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;
+ return findKvmHaScript("kvmsmpheartbeat.sh");
+ } else if (StoragePoolType.RBD.equals(type)) {
+ return findKvmHaScript("kvmheartbeat_rbd.sh");
}
return null;
}
+ private String findKvmHaScript(String scriptName) {
+ String kvmScriptsDir =
AgentPropertiesFileHandler.getPropertyValue(AgentProperties.KVM_SCRIPTS_DIR);
+ String scriptPath = Script.findScript(kvmScriptsDir, scriptName);
+ if (scriptPath == null) {
+ throw new CloudRuntimeException(String.format("Unable to find
script '%s' in directory: %s", scriptName, kvmScriptsDir));
+ }
+ return scriptPath;
+ }
+
+ /**
+ * Returns the Ceph monitors as expected by "--mon-host": the
comma-separated monitors of the pool, trimmed
+ * and without empty entries. If the pool has a monitor port, it is added
to each monitor which has none yet.
+ */
+ protected String getRbdMonitors() {
+ List<String> monitors = new ArrayList<>();
+ for (String monitor : sourceHost.split(",")) {
+ monitor = monitor.trim();
+ if (monitor.isEmpty()) {
+ continue;
+ }
+ monitors.add(sourcePort > 0 ? addPortToRbdMonitor(monitor) :
monitor);
+ }
+ return String.join(",", monitors);
+ }
Review Comment:
`sourceHost.split(\",\")` will throw a `NullPointerException` if
`sourceHost` is null, and it can also yield an empty monitor list if
`sourceHost` is blank/only commas. Since this value is used to build
`--mon-host` for HA scripts, add a defensive validation (e.g., throw a
`CloudRuntimeException` with a clear message) when `sourceHost` is blank/null
or when the resulting monitor list is empty.
--
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]
