This is an automated email from the ASF dual-hosted git repository. weizhouapache pushed a commit to branch 24-kvm-ha-on-ceph in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit ebc71b7a011ccfadc8f30f64654c142d392b67c3 Author: Wei Zhou <[email protected]> AuthorDate: Thu Oct 8 11:18:47 2026 +0200 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) --- .../hypervisor/kvm/resource/KVMHAMonitor.java | 5 +++- .../kvm/storage/KVMStoragePoolManager.java | 5 ++-- .../hypervisor/kvm/storage/LibvirtStoragePool.java | 24 +++++++++++++-- .../kvm/storage/LibvirtStoragePoolTest.java | 35 ++++++++++++++++++++++ scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh | 19 +++++++----- 5 files changed, 75 insertions(+), 13 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java index 7a7662f8b72..17b403c4965 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java @@ -20,6 +20,7 @@ import com.cloud.agent.properties.AgentProperties; 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 class KVMHAMonitor extends KVMHABase implements Runnable { 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); } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java index d99847fd921..c56171577e0 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java @@ -37,6 +37,7 @@ import org.reflections.Reflections; 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 class KVMStoragePoolManager { 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 class KVMStoragePoolManager { 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); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java index 7053dc4e4d9..dec006bf888 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java +++ b/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 @@ public class LibvirtStoragePool implements KVMStoragePool { 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); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java index e93f8177b88..2d18a62aed9 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStoragePoolTest.java @@ -117,4 +117,39 @@ public class LibvirtStoragePoolTest extends TestCase { 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)); + } } diff --git a/scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh b/scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh index a3b50717590..181194482ce 100755 --- a/scripts/vm/hypervisor/kvm/kvmheartbeat_rbd.sh +++ b/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" ]
