Copilot commented on code in PR #14320:
URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4208471544
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -330,29 +330,55 @@ 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
heartbeat script '%s' in directory: %s", scriptName, kvmScriptsDir));
+ }
+ return scriptPath;
+ }
+
+ /**
+ * Adds the Ceph cluster connection details (monitors, pool and, if cephx
is enabled, credentials)
+ * to a heartbeat/VM-activity check {@link Script} for a RBD storage pool.
Mirrors the "mon_host"/"id"/"key"
+ * options that qemu itself uses to talk to RBD (see {@link
KVMPhysicalDisk#RBDStringBuilder}).
+ */
+ private void addRbdConnectionArgs(Script cmd) {
+ cmd.add("-s", sourceHost);
+ cmd.add("-o", sourceDir);
Review Comment:
`sourcePort` is not passed to either RBD script. LibvirtStoragePoolDef
supports non-default Ceph monitor ports and KVMPhysicalDisk includes the port
in its mon_host value; with a pool configured on (for example) 3300,
`--mon-host` receives bare addresses and rados/rbd defaults to 6789, so every
heartbeat/activity check fails. Add a port argument and propagate it, including
to all comma-separated monitors.
This issue also appears on line 361 of the same file.
##########
scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh:
##########
@@ -0,0 +1,143 @@
+#!/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 kvmheartbeat.sh/kvmsmpheartbeat.sh.
+#
+# There is no shared POSIX mount point to write a heartbeat file to when the
+# primary storage pool is Ceph RBD, so the heartbeat timestamp is instead
+# stored as a small RADOS object (one object per host) in the same RBD pool.
+# Any host with a working path to the Ceph cluster can write/read this object,
+# which gives the same semantics as the NFS/SharedMountPoint heartbeat file.
+
+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, base64 (optional, required if -n is set)
+ -h host
+ -r write/read hb log
+ -c cleanup
+ -t interval between read hb log\n"
+ exit 1
+}
+#set -x
+MonHosts=
+PoolName=
+CephUser=
+CephKey=
+HostIP=
+interval=
+rflag=0
+cflag=0
+
+while getopts 's:o:n:k:h:t:rc' OPTION
+do
+ case $OPTION in
+ s)
+ MonHosts="$OPTARG"
+ ;;
+ o)
+ PoolName="$OPTARG"
+ ;;
+ n)
+ CephUser="$OPTARG"
+ ;;
+ k)
+ CephKey="$OPTARG"
+ ;;
+ h)
+ HostIP="$OPTARG"
+ ;;
+ r)
+ rflag=1
+ ;;
+ t)
+ interval="$OPTARG"
+ ;;
+ c)
+ cflag=1
+ ;;
+ *)
+ help
+ ;;
+ esac
+done
+
+if [ -z "$MonHosts" ] || [ -z "$PoolName" ]
+then
+ exit 1
+fi
+
+RadosOpts=(--mon-host "$MonHosts")
+if [ -n "$CephUser" ]
+then
+ RadosOpts+=(--id "$CephUser" --key "$CephKey")
+fi
+
+hbObject="KVMHA-hb-$HostIP"
+
+write_hbLog() {
+ tmpFile=$(mktemp)
+ date +%s > "$tmpFile"
+ rados -p "$PoolName" "${RadosOpts[@]}" put "$hbObject" "$tmpFile" &>
/dev/null
+ rc=$?
+ rm -f "$tmpFile"
+ return $rc
+}
+
+check_hbLog() {
+ now=$(date +%s)
+ hb=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$hbObject" - 2> /dev/null)
+ if ! [[ "$hb" =~ ^[0-9]+$ ]]
+ then
+ # Either the RADOS object doesn't exist yet (host never wrote a heartbeat)
+ # or the Ceph cluster can't be reached right now. Either way we can't
+ # confirm the host is alive, so fail safe and report it as DEAD.
+ return 255
+ fi
+ diff=$(expr $now - $hb)
+ if [ $diff -gt $interval ]
+ then
+ return $diff
Review Comment:
Returning `$diff` as a shell function's exit status wraps values above 255.
After the heartbeat is stale for 256 seconds, `check_hbLog` returns 0, so the
caller treats the host as ALIVE even though the heartbeat is missing/stale.
Return a bounded failure status and pass the measured delay separately, as the
SharedMountPoint script does, so every stale heartbeat remains DEAD.
##########
engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java:
##########
@@ -33,7 +33,8 @@
*/
public interface HighAvailabilityManager extends Manager {
- List<StoragePoolType> LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT =
List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint);
+ List<StoragePoolType> LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT =
List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint,
+ StoragePoolType.RBD);
Review Comment:
Adding RBD here also causes `KVMStoragePoolManager.createStoragePool` to
register it with `KVMHAMonitor`, but both `deleteStoragePool` overloads still
remove HA state only for `NetworkFilesystem`
(KVMStoragePoolManager.java:446-462). Deleting an RBD pool therefore leaves a
stale monitor entry, so the monitor continues heartbeat writes/checks against
removed storage. Make cleanup use the same HA-capable type predicate (or
include RBD) in both delete paths.
--
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]
