Copilot commented on code in PR #14320:
URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4217310751


##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -330,29 +331,74 @@ 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,
+     * each with the pool's monitor port if one is set (IPv6 addresses are 
enclosed in square brackets).
+     */
+    protected String getRbdMonitors() {
+        if (sourcePort <= 0) {
+            return sourceHost;
+        }
+        List<String> monitors = new ArrayList<>();
+        for (String monitor : sourceHost.split(",")) {
+            monitor = monitor.trim();
+            if (monitor.contains(":") && !monitor.startsWith("[")) {
+                monitor = "[" + monitor + "]";
+            }
+            monitors.add(monitor + ":" + sourcePort);
+        }
+        return String.join(",", monitors);
+    }
+
+    /**
+     * 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", getRbdMonitors());
+        cmd.add("-o", sourceDir);
+        if (authUsername != null) {
+            cmd.add("-n", authUsername);
+            cmd.add("-k", authSecret);
+        }
+    }
+
+    /**
+     * Adds the arguments identifying the storage to a heartbeat/VM-activity 
check {@link Script}:
+     * the Ceph connection details for a RBD pool, or the NFS server, path and 
mount point otherwise.
+     */
+    private void addPoolConnectionArgs(Script cmd, HAStoragePool pool) {
+        if (StoragePoolType.RBD.equals(type)) {
+            addRbdConnectionArgs(cmd);
+        } else {
+            cmd.add("-i", pool.getPoolIp());
+            cmd.add("-p", pool.getPoolMountSourcePath());
+            cmd.add("-m", pool.getMountDestPath());
+        }
+    }
 
     public String createHeartBeatCommand(HAStoragePool primaryStoragePool, 
String hostPrivateIp, boolean hostValidation) {
         Script cmd = new 
Script(primaryStoragePool.getPool().getHearthBeatPath(), 
HeartBeatUpdateTimeoutInMs, logger);
-        cmd.add("-i", primaryStoragePool.getPoolIp());
-        cmd.add("-p", primaryStoragePool.getPoolMountSourcePath());
-        cmd.add("-m", primaryStoragePool.getMountDestPath());
+        addPoolConnectionArgs(cmd, primaryStoragePool);
 
         if (hostValidation) {
             cmd.add("-h", hostPrivateIp);

Review Comment:
   For RBD, `kvmheartbeat_rbd.sh` uses `-h` to derive the per-host RADOS object 
name (`KVMHA-hb-$HostIP`). In `createHeartBeatCommand`, `-h` is only added when 
`hostValidation` is true, which can result in an empty/incorrect object name 
during heartbeat writes (e.g., all hosts writing `KVMHA-hb-`). For RBD pools, 
pass `-h` unconditionally (or change the script to reliably derive the host 
identity when `-h` is omitted).



##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -330,29 +331,74 @@ 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,
+     * each with the pool's monitor port if one is set (IPv6 addresses are 
enclosed in square brackets).
+     */
+    protected String getRbdMonitors() {
+        if (sourcePort <= 0) {
+            return sourceHost;
+        }
+        List<String> monitors = new ArrayList<>();
+        for (String monitor : sourceHost.split(",")) {
+            monitor = monitor.trim();
+            if (monitor.contains(":") && !monitor.startsWith("[")) {
+                monitor = "[" + monitor + "]";
+            }
+            monitors.add(monitor + ":" + sourcePort);
+        }
+        return String.join(",", monitors);

Review Comment:
   When `sourcePort <= 0`, this returns `sourceHost` without normalization. If 
`sourceHost` contains spaces (e.g. `\"10.0.0.1, 10.0.0.2\"`), the value passed 
to `--mon-host` may include whitespace and break parsing. Consider normalizing 
(split/trim/join) in both branches, and also skipping empty entries after 
trimming.



##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java:
##########
@@ -54,7 +55,9 @@ 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.runSimpleBashScript("umount " + 
pool.getMountDestPath());
+                }

Review Comment:
   Passing `pool.getMountDestPath()` directly into a shell command string can 
break if the path contains spaces and can be unsafe if the value is ever 
influenced by configuration. Prefer invoking `umount` with proper argument 
escaping (e.g., via a `Script` with args / non-shell execution) or at minimum 
shell-quoting/escaping the mount path.



##########
scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh:
##########
@@ -0,0 +1,146 @@
+#!/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"

Review Comment:
   This script never validates that `-h` was provided; if `HostIP` is empty, 
the heartbeat object becomes `KVMHA-hb-`, potentially causing different hosts 
to overwrite the same object. Also, `-k` is documented as required when `-n` is 
set, but the script doesn’t enforce it (it will pass an empty key to `rados`). 
Add explicit validation for non-empty `HostIP`, and enforce `CephKey` presence 
when `CephUser` is set (ideally with a clear error message and non-zero exit 
code).



##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java:
##########
@@ -330,29 +331,74 @@ 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,
+     * each with the pool's monitor port if one is set (IPv6 addresses are 
enclosed in square brackets).
+     */
+    protected String getRbdMonitors() {
+        if (sourcePort <= 0) {
+            return sourceHost;
+        }
+        List<String> monitors = new ArrayList<>();
+        for (String monitor : sourceHost.split(",")) {
+            monitor = monitor.trim();
+            if (monitor.contains(":") && !monitor.startsWith("[")) {
+                monitor = "[" + monitor + "]";
+            }
+            monitors.add(monitor + ":" + sourcePort);
+        }
+        return String.join(",", monitors);
+    }
+
+    /**
+     * 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", getRbdMonitors());
+        cmd.add("-o", sourceDir);
+        if (authUsername != null) {
+            cmd.add("-n", authUsername);
+            cmd.add("-k", authSecret);

Review Comment:
   `authUsername != null` is not sufficient to safely add cephx args: 
`authUsername` could be empty/blank, and `authSecret` could be null/blank. That 
can lead to malformed script invocations (or potential NPEs depending on 
`Script.add` implementation). Consider requiring both to be non-blank before 
adding `-n/-k`, and throw a clear `CloudRuntimeException` if only one is set.



##########
scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh:
##########
@@ -0,0 +1,184 @@
+#!/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, base64 (optional, required if -n is set)
+                    -h host
+                    -u volume (rbd image) uuid list
+                    -t time on ms
+                    -d suspect time\n"
+  exit 1
+}

Review Comment:
   The `-t` help text says 'time on ms', but the caller (`LibvirtStoragePool`) 
passes seconds (`System.currentTimeMillis() / 1000`). Also, the persisted RADOS 
value writes three fields (`SuspectTime:latestUpdateTime:MSTime`), but only the 
first two are read back, leaving `MSTime` unused. Align the documented units 
with actual usage, and either remove the unused third field (and variable) or 
read/use it consistently to avoid confusing state formats.



##########
scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh:
##########
@@ -0,0 +1,184 @@
+#!/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, base64 (optional, required if -n is set)
+                    -h host
+                    -u volume (rbd image) uuid list
+                    -t time on ms
+                    -d suspect time\n"
+  exit 1
+}
+
+#set -x
+
+MonHosts=
+PoolName=
+CephUser=
+CephKey=
+HostIP=
+UUIDList=
+MSTime=
+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)
+     MSTime="$OPTARG"
+     ;;
+  d)
+     SuspectTime="$OPTARG"
+     ;;
+  *)
+     help
+     ;;
+  esac
+done
+
+if [ -z "$MonHosts" ] || [ -z "$PoolName" ]
+then
+   exit 2
+fi
+
+if [ -z "$SuspectTime" ]
+then
+   exit 2
+fi
+
+RadosOpts=(--mon-host "$MonHosts")
+RbdOpts=(--mon-host "$MonHosts")
+if [ -n "$CephUser" ]
+then
+   RadosOpts+=(--id "$CephUser" --key "$CephKey")
+   RbdOpts+=(--id "$CephUser" --key "$CephKey")
+fi
+
+hbObject="KVMHA-hb-$HostIP"
+acObject="KVMHA-ac-$HostIP"
+
+# First check: heartbeat object, same as kvmheartbeat_rbd.sh
+now=$(date +%s)
+hb=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$hbObject" - 2> /dev/null)
+if [[ "$hb" =~ ^[0-9]+$ ]]
+then
+  diff=$(expr $now - $hb)
+  if [ $diff -lt 61 ]
+  then
+    echo "=====> ALIVE <====="
+    exit 0
+  fi
+fi
+
+if [ -z "$UUIDList" ]
+then
+  echo "=====> Considering host as DEAD due to 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
+for image in ${UUIDList//,/ }
+do
+  watcherCount=$(rbd status "$PoolName/$image" "${RbdOpts[@]}" --format json 
2> /dev/null | \
+    python3 -c 'import json,sys
+try:
+    print(len(json.load(sys.stdin).get("watchers", [])))
+except Exception:
+    print(0)' 2> /dev/null)
+  if [ -n "$watcherCount" ] && [ "$watcherCount" -gt 0 ] 2> /dev/null
+  then
+    latestUpdateTime=$now
+    break
+  fi
+done
+
+if [ ! -z "$(rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" 2> 
/dev/null)" ]
+then
+  acTime=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$acObject" - 2> 
/dev/null)
+else
+  acTime=
+fi
+
+tmpFile=$(mktemp)
+echo "$SuspectTime:$latestUpdateTime:$MSTime" > "$tmpFile"
+rados -p "$PoolName" "${RadosOpts[@]}" put "$acObject" "$tmpFile" &> /dev/null
+rm -f "$tmpFile"
+
+if [ -z "$acTime" ]; then
+    if [[ $latestUpdateTime -gt $SuspectTime ]]; then
+        echo "=====> ALIVE <====="
+    else
+        echo "=====> Considering host as DEAD due to RADOS object [$acObject] 
did not exist and condition [latestUpdateTime -gt SuspectTime] has not been 
satisfied. <======"
+    fi
+else
+    arrTime=(${acTime//:/ })
+    lastSuspectTime=${arrTime[0]}
+    lastUpdateTime=${arrTime[1]}

Review Comment:
   The `-t` help text says 'time on ms', but the caller (`LibvirtStoragePool`) 
passes seconds (`System.currentTimeMillis() / 1000`). Also, the persisted RADOS 
value writes three fields (`SuspectTime:latestUpdateTime:MSTime`), but only the 
first two are read back, leaving `MSTime` unused. Align the documented units 
with actual usage, and either remove the unused third field (and variable) or 
read/use it consistently to avoid confusing state formats.



##########
scripts/vm/hypervisor/kvm/kvmvmactivity_rbd.sh:
##########
@@ -0,0 +1,184 @@
+#!/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, base64 (optional, required if -n is set)
+                    -h host
+                    -u volume (rbd image) uuid list
+                    -t time on ms
+                    -d suspect time\n"
+  exit 1
+}
+
+#set -x
+
+MonHosts=
+PoolName=
+CephUser=
+CephKey=
+HostIP=
+UUIDList=
+MSTime=
+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)
+     MSTime="$OPTARG"
+     ;;
+  d)
+     SuspectTime="$OPTARG"
+     ;;
+  *)
+     help
+     ;;
+  esac
+done
+
+if [ -z "$MonHosts" ] || [ -z "$PoolName" ]
+then
+   exit 2
+fi
+
+if [ -z "$SuspectTime" ]
+then
+   exit 2
+fi
+
+RadosOpts=(--mon-host "$MonHosts")
+RbdOpts=(--mon-host "$MonHosts")
+if [ -n "$CephUser" ]
+then
+   RadosOpts+=(--id "$CephUser" --key "$CephKey")
+   RbdOpts+=(--id "$CephUser" --key "$CephKey")
+fi
+
+hbObject="KVMHA-hb-$HostIP"
+acObject="KVMHA-ac-$HostIP"
+
+# First check: heartbeat object, same as kvmheartbeat_rbd.sh
+now=$(date +%s)
+hb=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$hbObject" - 2> /dev/null)
+if [[ "$hb" =~ ^[0-9]+$ ]]
+then
+  diff=$(expr $now - $hb)
+  if [ $diff -lt 61 ]
+  then
+    echo "=====> ALIVE <====="
+    exit 0
+  fi
+fi
+
+if [ -z "$UUIDList" ]
+then
+  echo "=====> Considering host as DEAD due to 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
+for image in ${UUIDList//,/ }
+do
+  watcherCount=$(rbd status "$PoolName/$image" "${RbdOpts[@]}" --format json 
2> /dev/null | \
+    python3 -c 'import json,sys
+try:
+    print(len(json.load(sys.stdin).get("watchers", [])))
+except Exception:
+    print(0)' 2> /dev/null)
+  if [ -n "$watcherCount" ] && [ "$watcherCount" -gt 0 ] 2> /dev/null
+  then
+    latestUpdateTime=$now
+    break
+  fi
+done
+
+if [ ! -z "$(rados -p "$PoolName" "${RadosOpts[@]}" stat "$acObject" 2> 
/dev/null)" ]
+then
+  acTime=$(rados -p "$PoolName" "${RadosOpts[@]}" get "$acObject" - 2> 
/dev/null)
+else
+  acTime=
+fi
+
+tmpFile=$(mktemp)
+echo "$SuspectTime:$latestUpdateTime:$MSTime" > "$tmpFile"

Review Comment:
   The `-t` help text says 'time on ms', but the caller (`LibvirtStoragePool`) 
passes seconds (`System.currentTimeMillis() / 1000`). Also, the persisted RADOS 
value writes three fields (`SuspectTime:latestUpdateTime:MSTime`), but only the 
first two are read back, leaving `MSTime` unused. Align the documented units 
with actual usage, and either remove the unused third field (and variable) or 
read/use it consistently to avoid confusing state formats.



-- 
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]

Reply via email to