kvm: address review comments on Host HA for Ceph RBD

- pass the pool's monitor port to the RBD heartbeat/activity scripts, for
  every monitor, instead of relying on the default port
- kvmheartbeat_rbd.sh: keep the heartbeat age out of the function return
  status, which wraps above 255 and made a long-stale heartbeat look ALIVE
- remove pools from the HA monitor on deletion for all storage pool types
  with HA support, without trying to umount an empty mount path
- make the missing-script error message generic
- add unit tests for the RBD monitor list (IPv4, IPv6, mixed, with/without port)

Author: weizhouapache

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java

@@ -20,6 +20,7 @@
 import com.cloud.agent.properties.AgentPropertiesFileHandler;
 import com.cloud.ha.HighAvailabilityManager;
 import com.cloud.utils.script.Script;
+import org.apache.commons.lang3.StringUtils;
 import org.libvirt.Connect;
 import org.libvirt.LibvirtException;
 import org.libvirt.StoragePool;
@@ -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());
+                }
                 haStoragePools.remove(uuid);
             }
         }

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java

@@ -37,6 +37,7 @@
 
 import com.cloud.agent.api.to.DiskTO;
 import com.cloud.agent.api.to.VirtualMachineTO;
+import com.cloud.ha.HighAvailabilityManager;
 import com.cloud.hypervisor.kvm.resource.KVMHABase;
 import com.cloud.hypervisor.kvm.resource.KVMHABase.PoolType;
 import com.cloud.hypervisor.kvm.resource.KVMHAMonitor;
@@ -445,7 +446,7 @@ public boolean disconnectPhysicalDisk(StoragePoolType type, String poolUuid, Str
 
     public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) {
         StorageAdaptor adaptor = getStorageAdaptor(type);
-        if (type == StoragePoolType.NetworkFilesystem) {
+        if (HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type)) {
             _haMonitor.removeStoragePool(uuid);
         }
         boolean deleteStatus = adaptor.deleteStoragePool(uuid);;
@@ -457,7 +458,7 @@ public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid)
 
     public boolean deleteStoragePool(StoragePoolType type, String uuid, Map<String, String> details) {
         StorageAdaptor adaptor = getStorageAdaptor(type);
-        if (type == StoragePoolType.NetworkFilesystem) {
+        if (HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type)) {
             _haMonitor.removeStoragePool(uuid);
         }
         boolean deleteStatus = adaptor.deleteStoragePool(uuid, details);

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java

@@ -17,6 +17,7 @@
 package com.cloud.hypervisor.kvm.storage;
 
 import java.io.File;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
@@ -343,18 +344,37 @@ 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));
+            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", sourceHost);
+        cmd.add("-s", getRbdMonitors());
         cmd.add("-o", sourceDir);
         if (authUsername != null) {
             cmd.add("-n", authUsername);

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java

@@ -117,4 +117,39 @@ public void testIsPoolSupportHA() {
         assertFalse(new LibvirtStoragePool(uuid, name, StoragePoolType.CLVM, adapter, storage).isPoolSupportHA());
         assertFalse(new LibvirtStoragePool(uuid, name, StoragePoolType.Filesystem, adapter, storage).isPoolSupportHA());
     }
+
+    private String getRbdMonitors(String sourceHost, int sourcePort) {
+        LibvirtStoragePool pool = new LibvirtStoragePool("0f7a58bd-1a85-4b1f-9f91-12f3d1ecf5a5", "myfirstpool", StoragePoolType.RBD,
+                Mockito.mock(LibvirtStorageAdaptor.class), Mockito.mock(StoragePool.class));
+        pool.setSourceHost(sourceHost);
+        pool.setSourcePort(sourcePort);
+        return pool.getRbdMonitors();
+    }
+
+    @Test
+    public void testRbdMonitorsWithoutPort() {
+        assertEquals("10.0.0.1", getRbdMonitors("10.0.0.1", 0));
+        assertEquals("10.0.0.1,10.0.0.2,10.0.0.3", getRbdMonitors("10.0.0.1,10.0.0.2,10.0.0.3", 0));
+        assertEquals("fd00::1,fd00::2", getRbdMonitors("fd00::1,fd00::2", 0));
+    }
+
+    @Test
+    public void testRbdMonitorsIpv4WithPort() {
+        assertEquals("10.0.0.1:6789", getRbdMonitors("10.0.0.1", 6789));
+        assertEquals("10.0.0.1:3300,10.0.0.2:3300,10.0.0.3:3300", getRbdMonitors("10.0.0.1,10.0.0.2,10.0.0.3", 3300));
+    }
+
+    @Test
+    public void testRbdMonitorsIpv6WithPort() {
+        assertEquals("[fd00::1]:3300", getRbdMonitors("fd00::1", 3300));
+        assertEquals("[fd00::1]:3300,[fd00::2]:3300", getRbdMonitors("fd00::1,fd00::2", 3300));
+        // already enclosed in square brackets
+        assertEquals("[fd00::1]:3300,[fd00::2]:3300", getRbdMonitors("[fd00::1],[fd00::2]", 3300));
+    }
+
+    @Test
+    public void testRbdMonitorsMixedIpv4AndIpv6WithPort() {
+        assertEquals("10.0.0.1:3300,[fd00::1]:3300,[fd00::2]:3300,mon4.example.com:3300",
+                getRbdMonitors("10.0.0.1, fd00::1,[fd00::2] ,mon4.example.com", 3300));
+    }
 }

scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh

@@ -109,25 +109,28 @@ check_hbLog() {
     # 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
+    hbAge=
+    return 1
   fi
-  diff=$(expr $now - $hb)
-  if [ $diff -gt $interval ]
+  # the age is kept in a variable, not in the return status, as a status above 255 wraps around
+  hbAge=$(expr $now - $hb)
+  if [ $hbAge -gt $interval ]
   then
-    return $diff
+    return 1
   fi
   return 0
 }
 
 if [ "$rflag" == "1" ]
 then
-  check_hbLog
-  diff=$?
-  if [ $diff == 0 ]
+  if check_hbLog
   then
     echo "=====> ALIVE <====="
+  elif [ -z "$hbAge" ]
+  then
+    echo "=====> Considering host as DEAD because RADOS object [$hbObject] in pool [$PoolName] could not be read <======"
   else
-    echo "=====> Considering host as DEAD because last write to RADOS object [$hbObject] in pool [$PoolName] was [$diff] seconds ago, but the max interval is [$interval] <======"
+    echo "=====> Considering host as DEAD because last write to RADOS object [$hbObject] in pool [$PoolName] was [$hbAge] seconds ago, but the max interval is [$interval] <======"
   fi
   exit 0
 elif [ "$cflag" == "1" ]