From fdcace11f692bb744e63eb2b69c075a4e6491256 Mon Sep 17 00:00:00 2001 From: Pearl1594 Date: Tue, 20 Oct 2026 14:58:13 -0400 Subject: [PATCH 1/4] add host HA support and retry transient lock refusals on activation --- .../java/com/cloud/agent/api/to/HostTO.java | 9 + api/src/main/java/com/cloud/host/Host.java | 1 + .../com/cloud/ha/HighAvailabilityManager.java | 2 +- .../orchestration/VolumeOrchestrator.java | 8 +- .../orchestration/VolumeOrchestratorTest.java | 14 +- .../hypervisor/kvm/resource/KVMHAChecker.java | 24 +- .../hypervisor/kvm/resource/KVMHAMonitor.java | 14 +- .../resource/LibvirtComputingResource.java | 10 + .../kvm/storage/ClvmSanlockHaChecker.java | 160 +++++++++ .../kvm/storage/ClvmStorageAdaptor.java | 2 +- .../kvm/storage/KVMStoragePoolManager.java | 9 +- .../kvm/storage/LibvirtStoragePool.java | 14 + .../kvm/ha/KVMHostActivityChecker.java | 19 ++ .../kvm/resource/KVMHACheckerTest.java | 94 ++++++ .../kvm/storage/ClvmSanlockHaCheckerTest.java | 104 ++++++ .../kvm/ha/KVMHostActivityCheckerTest.java | 125 +++++++ .../cloud/storage/clvm/ClvmPoolManager.java | 143 ++++++-- .../cloud/storage/ClvmPoolManagerTest.java | 307 ++++++++++++++++++ 18 files changed, 1009 insertions(+), 50 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/KVMHACheckerTest.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java create mode 100644 plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/kvm/ha/KVMHostActivityCheckerTest.java diff --git a/api/src/main/java/com/cloud/agent/api/to/HostTO.java b/api/src/main/java/com/cloud/agent/api/to/HostTO.java index ee14e6597183..1e2544076ace 100644 --- a/api/src/main/java/com/cloud/agent/api/to/HostTO.java +++ b/api/src/main/java/com/cloud/agent/api/to/HostTO.java @@ -25,6 +25,7 @@ public class HostTO { private NetworkTO storageNetwork1; private NetworkTO storageNetwork2; private String parent; + private Long sanlockHostId; protected HostTO() { } @@ -46,6 +47,14 @@ public HostTO(Host vo) { } } + public Long getSanlockHostId() { + return sanlockHostId; + } + + public void setSanlockHostId(Long sanlockHostId) { + this.sanlockHostId = sanlockHostId; + } + public String getGuid() { return guid; } diff --git a/api/src/main/java/com/cloud/host/Host.java b/api/src/main/java/com/cloud/host/Host.java index 79bec7a5871a..d56ff8692d66 100644 --- a/api/src/main/java/com/cloud/host/Host.java +++ b/api/src/main/java/com/cloud/host/Host.java @@ -60,6 +60,7 @@ public static String[] toStrings(Host.Type... types) { String HOST_RBD_VOLUME_ENCRYPTION = "host.volume.encryption.rbd"; String HOST_INSTANCE_CONVERSION = "host.instance.conversion"; String HOST_VDDK_SUPPORT = "host.vddk.support"; + String HOST_SANLOCK_ID = "host.sanlock.id"; String HOST_VDDK_LIB_DIR = "vddk.lib.dir"; String HOST_VDDK_VERSION = "host.vddk.version"; String HOST_OVFTOOL_VERSION = "host.ovftool.version"; diff --git a/engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java b/engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java index 53bfcce27038..bf9b1d76b8d7 100644 --- a/engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java +++ b/engine/components-api/src/main/java/com/cloud/ha/HighAvailabilityManager.java @@ -33,7 +33,7 @@ */ public interface HighAvailabilityManager extends Manager { - List LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT = List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint); + List LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT = List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint, StoragePoolType.CLVM, StoragePoolType.CLVM_NG); ConfigKey ForceHA = new ConfigKey<>("Advanced", Boolean.class, "force.ha", "false", "Force High-Availability to happen even if the VM says no.", true, Cluster); diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 2fad96ec1da2..3391a8baa96b 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -881,6 +881,7 @@ private Long getClvmLockHostFromVmVolumes(Long vmId) { return null; } + long recoveryDeadlineMs = clvmPoolManager.newLockRecoveryDeadlineMs(); for (VolumeVO volume : vmVolumes) { if (volume.getPoolId() == null) { continue; @@ -895,7 +896,8 @@ private Long getClvmLockHostFromVmVolumes(Long vmId) { volume.getUuid(), volume.getPath(), pool, - true + true, + recoveryDeadlineMs ); if (lockHostId != null) { logger.debug("Found actual CLVM lock host {} from volume {} of VM {} via LVM query", @@ -912,6 +914,7 @@ private void transferClvmLocksForVmStart(List volumes, Long destHostId return; } + long recoveryDeadlineMs = clvmPoolManager.newLockRecoveryDeadlineMs(); for (VolumeVO volume : volumes) { if (volume.getPoolId() == null) { continue; @@ -927,7 +930,8 @@ private void transferClvmLocksForVmStart(List volumes, Long destHostId volume.getUuid(), volume.getPath(), pool, - true + true, + recoveryDeadlineMs ); if (currentLockHost == null) { diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java index d61caf16ae5d..70e207ab5fd0 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java @@ -671,9 +671,9 @@ public void testTransferClvmLocksForVmStart_WithClvmVolumes() throws Exception { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true))).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(102L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true))).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(true); @@ -743,7 +743,7 @@ public void testTransferClvmLocksForVmStart_NoLockTransferNeeded() throws Except ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), ArgumentMatchers.nullable(String.class), - ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true))).thenReturn(destHostId); + ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(destHostId); Mockito.when(storagePoolDao.findById(poolId)).thenReturn(clvmPool); @@ -776,7 +776,7 @@ public void testTransferClvmLocksForVmStart_EmptyVolumeList() throws Exception { method.invoke(volumeOrchestrator, new ArrayList(), destHostId, vmInstance); Mockito.verify(clvmPoolManager, Mockito.never()).getClvmLockHostId(Mockito.anyLong(), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()); + Mockito.anyString(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyLong()); } @Test @@ -817,7 +817,7 @@ public void testTransferClvmLocksForVmStart_SetInitialLockHost() throws Exceptio ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), ArgumentMatchers.nullable(String.class), - ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true))).thenReturn(null); + ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(null); Mockito.when(storagePoolDao.findById(poolId)).thenReturn(clvmPool); @@ -863,7 +863,7 @@ public void testTransferClvmLocksForVmStart_MixedVolumes() throws Exception { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true))).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(true); @@ -904,7 +904,7 @@ public void testTransferClvmLocksForVmStart_TransferFails() throws Throwable { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true))).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(false); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java index 0ee59c95da39..20afa31a7934 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java @@ -44,13 +44,7 @@ public Boolean hasHeartBeat() { storagePools.stream().map(pool -> pool.getPoolUUID()).collect(Collectors.joining(", "))); logger.debug("Checking heart beat with KVMHAChecker for {}", hostAndPools); - boolean heartBeatCheckResult = false; - for (HAStoragePool pool : storagePools) { - heartBeatCheckResult = pool.getPool().hasHeartBeat(pool, host); - if (reportIfHeartBeatFailedForOneStoragePool && !heartBeatCheckResult) { - break; - } - } + boolean heartBeatCheckResult = checkPoolsHeartBeat(); if (!heartBeatCheckResult) { logger.warn("All checks with KVMHAChecker for {} considered it as dead. It may cause a shutdown of the host.", hostAndPools); @@ -59,6 +53,22 @@ public Boolean hasHeartBeat() { return heartBeatCheckResult; } + /* + * With reportIfHeartBeatFailedForOneStoragePool the host is dead as soon as one pool has no heart beat, + * otherwise it is dead only when none of the pools has one. + */ + private boolean checkPoolsHeartBeat() { + boolean anyPoolHasHeartBeat = false; + for (HAStoragePool pool : storagePools) { + boolean poolHasHeartBeat = pool.getPool().hasHeartBeat(pool, host); + if (reportIfHeartBeatFailedForOneStoragePool && !poolHasHeartBeat) { + return false; + } + anyPoolHasHeartBeat = anyPoolHasHeartBeat || poolHasHeartBeat; + } + return anyPoolHasHeartBeat; + } + @Override public Boolean call() throws Exception { // logger.addAppender(new org.apache.log4j.ConsoleAppender(new 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 9f1b849e9727..a8b8a9619001 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 @@ -18,7 +18,7 @@ import com.cloud.agent.properties.AgentProperties; import com.cloud.agent.properties.AgentPropertiesFileHandler; -import com.cloud.ha.HighAvailabilityManager; +import com.cloud.storage.Storage.StoragePoolType; import com.cloud.utils.script.Script; import org.libvirt.Connect; import org.libvirt.LibvirtException; @@ -34,6 +34,8 @@ public class KVMHAMonitor extends KVMHABase implements Runnable { + private static final List MOUNT_BASED_POOL_TYPES = List.of(StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint); + private final Map haStoragePools = new ConcurrentHashMap<>(); private final boolean rebootHostAndAlertManagementOnHeartbeatTimeout; @@ -54,7 +56,9 @@ public void removeStoragePool(String uuid) { synchronized (haStoragePools) { HAStoragePool pool = haStoragePools.get(uuid); if (pool != null) { - Script.runSimpleBashScript("umount " + pool.getMountDestPath()); + if (isMountBased(pool)) { + Script.runSimpleBashScript("umount " + pool.getMountDestPath()); + } haStoragePools.remove(uuid); } } @@ -72,12 +76,16 @@ public HAStoragePool getStoragePool(String uuid) { } } + private boolean isMountBased(HAStoragePool pool) { + return MOUNT_BASED_POOL_TYPES.contains(pool.getPool().getType()); + } + protected void runHeartBeat() { synchronized (haStoragePools) { Set removedPools = new HashSet<>(); for (String uuid : haStoragePools.keySet()) { HAStoragePool primaryStoragePool = haStoragePools.get(uuid); - if (HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(primaryStoragePool.getPool().getType())) { + if (isMountBased(primaryStoragePool)) { checkForNotExistingLibvirtStoragePools(removedPools, uuid); if (removedPools.contains(uuid)) { continue; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 9009ec629ca3..10ed8d1a5cb8 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -21,6 +21,7 @@ import static com.cloud.host.Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM; import static com.cloud.host.Host.HOST_OVFTOOL_VERSION; import static com.cloud.host.Host.HOST_RBD_VOLUME_ENCRYPTION; +import static com.cloud.host.Host.HOST_SANLOCK_ID; import static com.cloud.host.Host.HOST_VDDK_LIB_DIR; import static com.cloud.host.Host.HOST_VDDK_SUPPORT; import static com.cloud.host.Host.HOST_VDDK_VERSION; @@ -217,6 +218,7 @@ import com.cloud.hypervisor.kvm.resource.rolling.maintenance.RollingMaintenanceServiceExecutor; import com.cloud.hypervisor.kvm.resource.wrapper.LibvirtRequestWrapper; import com.cloud.hypervisor.kvm.resource.wrapper.LibvirtUtilitiesHelper; +import com.cloud.hypervisor.kvm.storage.ClvmSanlockHaChecker; import com.cloud.hypervisor.kvm.storage.IscsiStorageCleanupMonitor; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; @@ -4430,6 +4432,7 @@ public StartupCommand[] initialize() { cmd.setCluster(clusterId); cmd.setGatewayIpAddress(localGateway); cmd.setIqn(getIqn()); + addSanlockHostId(cmd); cmd.getHostDetails().put(HOST_VOLUME_ENCRYPTION, String.valueOf(hostSupportsVolumeEncryption())); cmd.getHostDetails().put(HOST_RBD_VOLUME_ENCRYPTION, String.valueOf(hostSupportsRbdVolumeEncryption())); cmd.setHostTags(getHostTags()); @@ -6216,6 +6219,13 @@ public boolean isHostSecured() { return true; } + private void addSanlockHostId(StartupRoutingCommand cmd) { + Long sanlockHostId = ClvmSanlockHaChecker.readLocalHostId(); + if (sanlockHostId != null) { + cmd.getHostDetails().put(HOST_SANLOCK_ID, String.valueOf(sanlockHostId)); + } + } + /** * Test host for qemu-native LUKS volume encryption (qemu-img LUKS support + cryptsetup), * reported as {@code host.volume.encryption}. RBD/librbd encryption support is a separate diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java new file mode 100644 index 000000000000..89d7e89f97a2 --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java @@ -0,0 +1,160 @@ +// 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. + +package com.cloud.hypervisor.kvm.storage; + +import java.io.File; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.utils.script.OutputInterpreter; +import com.cloud.utils.script.Script; + +/** + * Reads a host's sanlock delta-lease state for a CLVM/CLVM_NG volume group; the lease is renewed + * by sanlock itself, and a host that stops renewing is reset by its watchdog. + */ +public class ClvmSanlockHaChecker { + + protected static final String LOCKSPACE_PREFIX = "lvm_"; + private static final String LVMLOCKD_SOCKET = "/run/lvm/lvmlockd.socket"; + protected static final long WATCHDOG_FIRE_TIMEOUT_SECONDS = 60; + protected static final long ID_RENEWAL_FAIL_IO_TIMEOUTS = 8; + private static final long CHECK_TIMEOUT_MS = 30000; + + private static final Logger LOGGER = LogManager.getLogger(ClvmSanlockHaChecker.class); + private static final Pattern HOST_ID_LINE = Pattern.compile("^host_id=(\\d+)$"); + private static final Pattern HOST_ENTRY = Pattern.compile("^(\\d+) timestamp \\d+\\s*$"); + + protected static class LeaseState { + final long lastCheck; + final long lastLive; + final long ioTimeout; + + LeaseState(long lastCheck, long lastLive, long ioTimeout) { + this.lastCheck = lastCheck; + this.lastLive = lastLive; + this.ioTimeout = ioTimeout; + } + } + + /** Returns this host's lvmlockd sanlock host_id, or null when lvmlockd is not running. */ + public static Long readLocalHostId() { + if (!new File(LVMLOCKD_SOCKET).exists()) { + return null; + } + Script cmd = new Script("lvmconfig", CHECK_TIMEOUT_MS, LOGGER); + cmd.add("local/host_id"); + OutputInterpreter.OneLineParser parser = new OutputInterpreter.OneLineParser(); + if (cmd.execute(parser) != null) { + return null; + } + return parseHostId(parser.getLine()); + } + + protected static Long parseHostId(String line) { + Matcher matcher = HOST_ID_LINE.matcher(line == null ? "" : line.trim()); + if (!matcher.matches()) { + return null; + } + try { + return Long.valueOf(matcher.group(1)); + } catch (NumberFormatException e) { + LOGGER.warn("Ignoring invalid sanlock host id [{}]", line); + return null; + } + } + + /** Returns false only when the lease stopped renewing long enough that the watchdog has reset the host. */ + public static boolean isHostLeaseAlive(String vgName, Long hostId) { + if (vgName == null || hostId == null) { + LOGGER.warn("Cannot check the sanlock lease: VG name [{}] or host id [{}] is unknown, assuming the host is alive.", vgName, hostId); + return true; + } + String output = readHostStatus(vgName); + if (output == null) { + return true; + } + LeaseState state = parseLeaseState(output, hostId); + if (state == null) { + LOGGER.warn("No sanlock lease found for host id [{}] in lockspace [{}{}], assuming the host is alive.", hostId, LOCKSPACE_PREFIX, vgName); + return true; + } + return isLeaseAlive(state); + } + + private static String readHostStatus(String vgName) { + Script cmd = new Script("sanlock", CHECK_TIMEOUT_MS, LOGGER); + cmd.add("client", "host_status", "-s", LOCKSPACE_PREFIX + vgName, "-D"); + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + String result = cmd.execute(parser); + if (result != null) { + LOGGER.warn("Failed to read sanlock host status for lockspace [{}{}]: {}", LOCKSPACE_PREFIX, vgName, result); + return null; + } + return parser.getLines(); + } + + protected static LeaseState parseLeaseState(String output, long hostId) { + try { + return readLeaseState(output, hostId); + } catch (NumberFormatException e) { + LOGGER.warn("Unexpected sanlock host status for host id [{}]: {}", hostId, e.getMessage()); + return null; + } + } + + private static LeaseState readLeaseState(String output, long hostId) { + boolean inHost = false; + Long lastCheck = null; + Long lastLive = null; + Long ioTimeout = null; + for (String line : output.split("\\r?\\n")) { + Matcher entry = HOST_ENTRY.matcher(line); + if (entry.matches()) { + if (inHost) { + break; + } + inHost = Long.parseLong(entry.group(1)) == hostId; + } else if (inHost) { + lastCheck = valueOrCurrent(line, "last_check", lastCheck); + lastLive = valueOrCurrent(line, "last_live", lastLive); + ioTimeout = valueOrCurrent(line, "io_timeout", ioTimeout); + } + } + if (lastCheck == null || lastLive == null || ioTimeout == null) { + return null; + } + return new LeaseState(lastCheck, lastLive, ioTimeout); + } + + private static Long valueOrCurrent(String line, String key, Long current) { + String trimmed = line.trim(); + if (trimmed.startsWith(key + "=")) { + return Long.parseLong(trimmed.substring(key.length() + 1)); + } + return current; + } + + protected static boolean isLeaseAlive(LeaseState state) { + long hostDeadSeconds = ID_RENEWAL_FAIL_IO_TIMEOUTS * state.ioTimeout + WATCHDOG_FIRE_TIMEOUT_SECONDS; + return state.lastCheck - state.lastLive < hostDeadSeconds; + } +} diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java index e9ec65505e40..fe2f10bdcdf1 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java @@ -413,7 +413,7 @@ private KVMPhysicalDisk getPhysicalDiskWithClvmFallback(String volumeUuid, KVMSt return getPhysicalDiskViaDirectBlockDevice(volumeUuid, pool); } - private String getVgName(String sourceDir) { + static String getVgName(String sourceDir) { String vgName = sourceDir; if (vgName.startsWith("/")) { String[] parts = vgName.split("/"); 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 d99847fd921a..922835fe7f94 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 @@ -43,6 +43,7 @@ import com.cloud.storage.Storage; import com.cloud.storage.Storage.StoragePoolType; import com.cloud.storage.StorageLayer; +import com.cloud.storage.clvm.ClvmPoolManager; import com.cloud.storage.Volume; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; @@ -443,9 +444,13 @@ public boolean disconnectPhysicalDisk(StoragePoolType type, String poolUuid, Str return adaptor.disconnectPhysicalDisk(volPath, pool); } + private boolean isHaPoolRemovedOnDelete(StoragePoolType type) { + return type == StoragePoolType.NetworkFilesystem || ClvmPoolManager.isClvmPoolType(type); + } + public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) { StorageAdaptor adaptor = getStorageAdaptor(type); - if (type == StoragePoolType.NetworkFilesystem) { + if (isHaPoolRemovedOnDelete(type)) { _haMonitor.removeStoragePool(uuid); } boolean deleteStatus = adaptor.deleteStoragePool(uuid);; @@ -457,7 +462,7 @@ public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) public boolean deleteStoragePool(StoragePoolType type, String uuid, Map details) { StorageAdaptor adaptor = getStorageAdaptor(type); - if (type == StoragePoolType.NetworkFilesystem) { + if (isHaPoolRemovedOnDelete(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 a8c32baa6ef3..ed6ba1fd78a8 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 @@ -35,6 +35,7 @@ import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool; import com.cloud.storage.Storage; import com.cloud.storage.Storage.StoragePoolType; +import com.cloud.storage.clvm.ClvmPoolManager; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.OutputInterpreter; import com.cloud.utils.script.Script; @@ -348,7 +349,14 @@ public String getHearthBeatPath() { } + private Boolean hasClvmHostLease(HostTO host) { + return ClvmSanlockHaChecker.isHostLeaseAlive(ClvmStorageAdaptor.getVgName(getLocalPath()), host.getSanlockHostId()); + } + public String createHeartBeatCommand(HAStoragePool primaryStoragePool, String hostPrivateIp, boolean hostValidation) { + if (ClvmPoolManager.isClvmPoolType(type)) { + return null; + } Script cmd = new Script(primaryStoragePool.getPool().getHearthBeatPath(), HeartBeatUpdateTimeoutInMs, logger); cmd.add("-i", primaryStoragePool.getPoolIp()); cmd.add("-p", primaryStoragePool.getPoolMountSourcePath()); @@ -375,6 +383,9 @@ public String getStorageNodeId() { @Override public Boolean hasHeartBeat(HAStoragePool pool, HostTO host) { + if (ClvmPoolManager.isClvmPoolType(type)) { + return hasClvmHostLease(host); + } String hostIp = host.getPrivateNetwork().getIp(); Script cmd = new Script(getHearthBeatPath(), HeartBeatCheckerTimeoutInMs, logger); cmd.add("-i", pool.getPoolIp()); @@ -400,6 +411,9 @@ public Boolean hasHeartBeat(HAStoragePool pool, HostTO host) { @Override public Boolean hasVmActivity(HAStoragePool pool, HostTO host, Duration activityScriptTimeout, String volumeUUIDListString, String vmActivityCheckPath, long duration) { + if (ClvmPoolManager.isClvmPoolType(type)) { + return hasClvmHostLease(host); + } String hostIp = host.getPrivateNetwork().getIp(); Script cmd = new Script(vmActivityCheckPath, activityScriptTimeout.getStandardSeconds(), logger); cmd.add("-i", pool.getPoolIp()); diff --git a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHostActivityChecker.java b/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHostActivityChecker.java index 96fd42d4d544..02327b2b1a24 100644 --- a/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHostActivityChecker.java +++ b/plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHostActivityChecker.java @@ -22,14 +22,18 @@ import com.cloud.agent.api.CheckOnHostAnswer; import com.cloud.agent.api.CheckOnHostCommand; import com.cloud.agent.api.CheckVMActivityOnStoragePoolCommand; +import com.cloud.agent.api.to.HostTO; import com.cloud.dc.dao.ClusterDao; import com.cloud.exception.StorageUnavailableException; import com.cloud.ha.HighAvailabilityManager; import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.Status; +import com.cloud.host.DetailVO; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.resource.ResourceManager; +import com.cloud.storage.clvm.ClvmPoolManager; import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; import com.cloud.storage.Volume; @@ -44,6 +48,7 @@ import org.apache.cloudstack.ha.provider.HealthCheckerInterface; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.commons.lang.ArrayUtils; +import org.apache.commons.lang3.math.NumberUtils; import javax.inject.Inject; import java.util.ArrayList; @@ -67,6 +72,8 @@ public class KVMHostActivityChecker extends AdapterBase implements ActivityCheck private StorageManager storageManager; @Inject private ResourceManager resourceManager; + @Inject + private HostDetailsDao hostDetailsDao; @Override public boolean isActive(Host r, DateTime suspectTime) throws HACheckerException { @@ -126,6 +133,7 @@ private Status checkHostStatusWithSameHost(Host host) { Status hostStatus; boolean reportFailureIfOneStorageIsDown = HighAvailabilityManager.KvmHAFenceHostIfHeartbeatFailsOnStorage.value(); final CheckOnHostCommand cmd = new CheckOnHostCommand(host, reportFailureIfOneStorageIsDown); + addSanlockHostId(cmd.getHost(), host); try { logger.debug("Checking {} status...", host.toString()); Answer answer = agentMgr.easySend(host.getId(), cmd); @@ -153,6 +161,7 @@ private Status checkHostStatusWithNeighbourHosts(Host host) { Status hostStatusFromNeighbour = Status.Unknown; boolean reportFailureIfOneStorageIsDown = HighAvailabilityManager.KvmHAFenceHostIfHeartbeatFailsOnStorage.value(); final CheckOnHostCommand cmd = new CheckOnHostCommand(host, reportFailureIfOneStorageIsDown); + addSanlockHostId(cmd.getHost(), host); List neighbors = resourceManager.listHostsInClusterByStatus(host.getClusterId(), Status.Up); for (HostVO neighbor : neighbors) { if (neighbor.getId() == host.getId() @@ -204,6 +213,9 @@ private boolean hasVMActivityOnHost(Host host, DateTime suspectTime) throws HACh protected boolean verifyActivityOfStorageOnHost(HashMap> poolVolMap, StoragePool pool, Host host, DateTime suspectTime, boolean activityStatus) throws HACheckerException, IllegalStateException { List volume_list = poolVolMap.get(pool); final CheckVMActivityOnStoragePoolCommand cmd = new CheckVMActivityOnStoragePoolCommand(host, pool, volume_list, suspectTime); + if (ClvmPoolManager.isClvmPoolType(pool.getPoolType())) { + addSanlockHostId(cmd.getHost(), host); + } logger.debug("Checking VM activity for {} on storage pool [{}].", host.toString(), pool); try { @@ -224,6 +236,13 @@ protected boolean verifyActivityOfStorageOnHost(HashMap> getStoragePoolAndVolumeInfoOnHost(Host host) { List vmListOnHost = vmInstanceDao.listByHostId(host.getId()); List volumeListOnHost = new ArrayList<>(); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/KVMHACheckerTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/KVMHACheckerTest.java new file mode 100644 index 000000000000..5f8c04f392a5 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/KVMHACheckerTest.java @@ -0,0 +1,94 @@ +/* + * 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. + */ + +package com.cloud.hypervisor.kvm.resource; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.api.to.HostTO; +import com.cloud.agent.api.to.NetworkTO; +import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool; +import com.cloud.hypervisor.kvm.resource.KVMHABase.PoolType; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; + +@RunWith(MockitoJUnitRunner.Silent.class) +public class KVMHACheckerTest { + + @Mock + private HostTO host; + @Mock + private NetworkTO privateNetwork; + + private List poolsWithHeartBeats(boolean... heartBeats) { + when(host.getPrivateNetwork()).thenReturn(privateNetwork); + List pools = new ArrayList<>(); + for (boolean heartBeat : heartBeats) { + KVMStoragePool kvmPool = mock(KVMStoragePool.class); + when(kvmPool.hasHeartBeat(any(HAStoragePool.class), any(HostTO.class))).thenReturn(heartBeat); + pools.add(new HAStoragePool(kvmPool, "host", "/path", PoolType.PrimaryStorage)); + } + return pools; + } + + private boolean checkHeartBeat(boolean reportIfFailedForOnePool, boolean... heartBeats) { + return new KVMHAChecker(poolsWithHeartBeats(heartBeats), host, reportIfFailedForOnePool).hasHeartBeat(); + } + + @Test + public void testHostIsAliveWhenAnyPoolHasHeartBeatAndOnePoolFailureIsNotReported() { + assertTrue(checkHeartBeat(false, true, false)); + assertTrue(checkHeartBeat(false, false, true)); + } + + @Test + public void testHostIsDeadWhenNoPoolHasHeartBeat() { + assertFalse(checkHeartBeat(false, false, false)); + assertFalse(checkHeartBeat(true, false, false)); + } + + @Test + public void testHostIsDeadWhenOnePoolHasNoHeartBeatAndItIsReported() { + assertFalse(checkHeartBeat(true, true, false)); + assertFalse(checkHeartBeat(true, false, true)); + } + + @Test + public void testHostIsAliveWhenAllPoolsHaveHeartBeat() { + assertTrue(checkHeartBeat(true, true, true)); + assertTrue(checkHeartBeat(false, true, true)); + } + + @Test + public void testHostIsDeadWithoutPools() { + assertFalse(checkHeartBeat(false)); + assertFalse(checkHeartBeat(true)); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java new file mode 100644 index 000000000000..c1465953e5a0 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java @@ -0,0 +1,104 @@ +// 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. + + +package com.cloud.hypervisor.kvm.storage; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +public class ClvmSanlockHaCheckerTest { + + private static final String HOST_STATUS = + "1 timestamp 437999\n" + + " last_check=438020\n" + + " last_live=438020\n" + + " last_req=0\n" + + " owner_id=1\n" + + " owner_generation=2\n" + + " timestamp=437999\n" + + " io_timeout=10\n" + + " owner_name=46a23acd-5b2c-479d-8474-583e8fced6a8.ref-trl-12\n" + + "2 timestamp 437955\n" + + " last_check=438200\n" + + " last_live=438020\n" + + " last_req=0\n" + + " owner_id=2\n" + + " owner_generation=1\n" + + " timestamp=437955\n" + + " io_timeout=10\n" + + " owner_name=31028f82-31c0-4c3d-a78a-8cc92afde498.ref-trl-12\n"; + + @Test + public void testParseLeaseStateForLiveHost() { + ClvmSanlockHaChecker.LeaseState state = ClvmSanlockHaChecker.parseLeaseState(HOST_STATUS, 1); + assertNotNull(state); + assertEquals(438020, state.lastCheck); + assertEquals(438020, state.lastLive); + assertEquals(10, state.ioTimeout); + assertTrue(ClvmSanlockHaChecker.isLeaseAlive(state)); + } + + @Test + public void testParseLeaseStateForHostWhoseLeaseStoppedRenewing() { + ClvmSanlockHaChecker.LeaseState state = ClvmSanlockHaChecker.parseLeaseState(HOST_STATUS, 2); + assertNotNull(state); + assertEquals(180, state.lastCheck - state.lastLive); + assertFalse(ClvmSanlockHaChecker.isLeaseAlive(state)); + } + + @Test + public void testLeaseStillAliveBeforeWatchdogGuarantee() { + assertTrue(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1139, 1000, 10))); + assertFalse(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1140, 1000, 10))); + } + + @Test + public void testParseLeaseStateForUnknownHostId() { + assertNull(ClvmSanlockHaChecker.parseLeaseState(HOST_STATUS, 3)); + assertNull(ClvmSanlockHaChecker.parseLeaseState("", 1)); + } + + @Test + public void testParseHostId() { + assertEquals(Long.valueOf(2), ClvmSanlockHaChecker.parseHostId("host_id=2")); + assertNull(ClvmSanlockHaChecker.parseHostId(" Configuration node local/host_id not found")); + assertNull(ClvmSanlockHaChecker.parseHostId(null)); + } + + @Test + public void testParseHostIdIgnoresNumberTooLarge() { + assertNull(ClvmSanlockHaChecker.parseHostId("host_id=99999999999999999999")); + } + + @Test + public void testParseLeaseStateIgnoresMalformedNumbers() { + assertNull(ClvmSanlockHaChecker.parseLeaseState("1 timestamp 10\n\tlast_check=abc\n\tlast_live=5\n\tio_timeout=10\n", 1)); + assertNull(ClvmSanlockHaChecker.parseLeaseState("99999999999999999999 timestamp 10\n\tlast_check=1\n", 1)); + } + + @Test + public void testUnknownVgOrHostIdAssumesAlive() { + assertTrue(ClvmSanlockHaChecker.isHostLeaseAlive(null, 1L)); + assertTrue(ClvmSanlockHaChecker.isHostLeaseAlive("sharedvg", null)); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/kvm/ha/KVMHostActivityCheckerTest.java b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/kvm/ha/KVMHostActivityCheckerTest.java new file mode 100644 index 000000000000..c4902517ae71 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/kvm/ha/KVMHostActivityCheckerTest.java @@ -0,0 +1,125 @@ +/* + * 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. + */ + +package org.apache.cloudstack.kvm.ha; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.HashMap; +import java.util.List; + +import org.joda.time.DateTime; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.CheckOnHostCommand; +import com.cloud.agent.api.CheckVMActivityOnStoragePoolCommand; +import com.cloud.host.DetailVO; +import com.cloud.host.Host; +import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.host.dao.HostDetailsDao; +import com.cloud.resource.ResourceManager; +import com.cloud.dc.dao.ClusterDao; +import com.cloud.storage.Storage.StoragePoolType; +import com.cloud.storage.StorageManager; +import com.cloud.storage.StoragePool; +import com.cloud.storage.Volume; + +@RunWith(MockitoJUnitRunner.Silent.class) +public class KVMHostActivityCheckerTest { + + @Mock + private HostDetailsDao hostDetailsDao; + @Mock + private StorageManager storageManager; + @Mock + private AgentManager agentMgr; + @Mock + private ResourceManager resourceManager; + @Mock + private ClusterDao clusterDao; + @Mock + private Host host; + @Mock + private StoragePool pool; + @Mock + private Volume volume; + @InjectMocks + private KVMHostActivityChecker checker; + + private void runActivityCheck(StoragePoolType poolType) throws Exception { + when(pool.getPoolType()).thenReturn(poolType); + when(host.getId()).thenReturn(8L); + when(volume.getPath()).thenReturn("vol"); + when(hostDetailsDao.findDetail(8L, Host.HOST_SANLOCK_ID)).thenReturn(new DetailVO(8L, Host.HOST_SANLOCK_ID, "2")); + when(storageManager.sendToPool(any(StoragePool.class), any(long[].class), any(CheckVMActivityOnStoragePoolCommand.class))).thenReturn(new Answer(null)); + HashMap> poolVolumes = new HashMap<>(); + poolVolumes.put(pool, List.of(volume)); + checker.verifyActivityOfStorageOnHost(poolVolumes, pool, host, new DateTime(), true); + } + + @Test + public void testSanlockHostIdIsNotLookedUpForNonClvmPool() throws Exception { + runActivityCheck(StoragePoolType.NetworkFilesystem); + verify(hostDetailsDao, never()).findDetail(anyLong(), any()); + } + + @Test + public void testSanlockHostIdIsSentForClvmPool() throws Exception { + runActivityCheck(StoragePoolType.CLVM_NG); + ArgumentCaptor captor = ArgumentCaptor.forClass(CheckVMActivityOnStoragePoolCommand.class); + verify(storageManager).sendToPool(any(StoragePool.class), any(long[].class), captor.capture()); + assertEquals(Long.valueOf(2), captor.getValue().getHost().getSanlockHostId()); + } + + @Test + public void testHostCheckSendsSanlockHostIdWhenHostReportsOne() throws Exception { + when(host.getId()).thenReturn(8L); + when(host.getHypervisorType()).thenReturn(HypervisorType.KVM); + when(hostDetailsDao.findDetail(8L, Host.HOST_SANLOCK_ID)).thenReturn(new DetailVO(8L, Host.HOST_SANLOCK_ID, "2")); + when(agentMgr.easySend(anyLong(), any(CheckOnHostCommand.class))).thenReturn(new Answer(null, false, "down")); + checker.getHostAgentStatus(host); + ArgumentCaptor captor = ArgumentCaptor.forClass(CheckOnHostCommand.class); + verify(agentMgr).easySend(anyLong(), captor.capture()); + assertEquals(Long.valueOf(2), captor.getValue().getHost().getSanlockHostId()); + } + + @Test + public void testHostCheckSendsNoSanlockHostIdWhenHostReportsNone() throws Exception { + when(host.getId()).thenReturn(8L); + when(host.getHypervisorType()).thenReturn(HypervisorType.KVM); + when(agentMgr.easySend(anyLong(), any(CheckOnHostCommand.class))).thenReturn(new Answer(null, false, "down")); + checker.getHostAgentStatus(host); + ArgumentCaptor captor = ArgumentCaptor.forClass(CheckOnHostCommand.class); + verify(agentMgr).easySend(anyLong(), captor.capture()); + assertNull(captor.getValue().getHost().getSanlockHostId()); + } +} diff --git a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java index febef471e683..ae3556011ba4 100644 --- a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java +++ b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java @@ -21,7 +21,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; import javax.inject.Inject; @@ -64,6 +66,14 @@ public class ClvmPoolManager implements Configurable { */ public static final String CLVM_LOCK_HOST_ID = "clvmLockHostId"; + private static final List TRANSIENT_LOCK_REFUSALS = List.of("locked by other host", "lock start in progress"); + + public static final ConfigKey ClvmLockRecoveryRetryTimeout = new ConfigKey<>("Advanced", Integer.class, "clvm.lock.recovery.retry.timeout", "180", + "Maximum time in seconds, counted from the start of the CLVM lock holder lookup, to keep retrying the exclusive activation of a volume that is refused only temporarily: the previous lock holder's lease has not expired yet, or the lockspace of the volume group is still being started on the host. The lease expires 8 x io_timeout + 60 seconds after the holder stops renewing it (140 seconds with the default lvmlockd sanlock io_timeout of 10). If lvmlockd runs with a different --sanlock-timeout, set this to at least that value plus about 40 seconds. 0 disables the retry.", true); + + public static final ConfigKey ClvmLockRecoveryRetryInterval = new ConfigKey<>("Advanced", Integer.class, "clvm.lock.recovery.retry.interval", "10", + "Time in seconds between retries of the exclusive activation of a CLVM volume that is refused only temporarily (previous lock holder's lease not expired yet, or lockspace still starting).", true); + public static final ConfigKey CLVMSecureZeroFill = new ConfigKey<>("Advanced", Boolean.class, "clvm.secure.zero.fill", "false", "When enabled, CLVM volumes to be zero-filled at the time of deletion to prevent data from being recovered by VMs reusing the space, as thick LVM volumes write data linearly. Note: This setting is propagated to hosts when they connect to the storage pool. Changing this setting requires disconnecting and reconnecting hosts or restarting the KVM agent for it to take effect.", false, ConfigKey.Scope.StoragePool); @@ -106,17 +116,28 @@ public Long getClvmLockHostId(Long volumeId, String volumeUuid) { */ public Long getClvmLockHostId(Long volumeId, String volumeUuid, String volumePath, StoragePool pool, boolean queryActual) { + return getClvmLockHostId(volumeId, volumeUuid, volumePath, pool, queryActual, newLockRecoveryDeadlineMs()); + } + + /** Same as above, with a retry deadline shared across calls, see {@link #newLockRecoveryDeadlineMs()}. */ + public Long getClvmLockHostId(Long volumeId, String volumeUuid, String volumePath, + StoragePool pool, boolean queryActual, long activationDeadlineMs) { if (queryActual) { if (volumePath == null || pool == null) { logger.warn("Cannot query actual CLVM lock state for volume {} - missing volumePath or pool", volumeUuid); return getClvmLockHostId(volumeId, volumeUuid); } - return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, true); + return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, true, activationDeadlineMs); } return getClvmLockHostId(volumeId, volumeUuid); } + /** @return the time in milliseconds until which a refused activation is retried, counted from now */ + public long newLockRecoveryDeadlineMs() { + return now() + getRetryTimeoutMillis(); + } + /** * Safely sets or updates the CLVM_LOCK_HOST_ID detail for a volume. * If the detail already exists, it will be updated. Otherwise, it will be created. @@ -153,6 +174,12 @@ public void setClvmLockHostId(long volumeId, long hostId) { */ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volumePath, StoragePool pool, boolean updateDatabase) { + return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, updateDatabase, newLockRecoveryDeadlineMs()); + } + + /** Same as above, retrying a refused recovery activation until {@code activationDeadlineMs}. */ + public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volumePath, + StoragePool pool, boolean updateDatabase, long activationDeadlineMs) { if (pool == null) { logger.error("Cannot query CLVM lock for volume {} - pool is null", volumeUuid); return null; @@ -164,6 +191,9 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu } String lvPath = String.format("/dev/%s/%s", vgName, volumePath); + // Hosts that did not answer the lock-state query + Set unreachableHostIds = new HashSet<>(); + // Fast path: trust the DB record and verify with a single host query Long dbHostId = getClvmLockHostId(volumeId, volumeUuid); if (dbHostId != null) { @@ -175,6 +205,9 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu logger.debug("Fast path: volume {} confirmed active on DB host {}", volumeUuid, dbHostId); return dbHostId; } + if (active == null) { + unreachableHostIds.add(dbHostId); + } logger.info("Fast path miss: volume {} not active on DB host {} - falling back to full fan-out", volumeUuid, dbHostId); } else { @@ -213,6 +246,8 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu if (Boolean.TRUE.equals(active)) { logger.debug("Volume {} is locally active on host {} (fan-out)", volumeUuid, host.getId()); activeHostIds.add(host.getId()); + } else if (active == null) { + unreachableHostIds.add(host.getId()); } } @@ -220,10 +255,10 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu logger.debug("Volume {} is not active on any reachable host — no exclusive lock held", volumeUuid); // Recovery: attempt exclusive activation on the best available host before giving up. - Long targetHostId = selectActivationTargetHost(dbHostId, hosts); + Long targetHostId = selectActivationTargetHost(dbHostId, hosts, unreachableHostIds); if (targetHostId != null) { Long recoveredHostId = tryActivateExclusivelyOnHost(volumeId, volumeUuid, lvPath, - targetHostId, updateDatabase); + targetHostId, updateDatabase, activationDeadlineMs); if (recoveredHostId != null) { return recoveredHostId; } @@ -302,12 +337,15 @@ private Boolean querySingleHostLockState(Long hostId, String lvPath, String volu * *

Priority 2: a random UP KVM routing host from the cluster/zone list (fallback). * - * @param dbHostId last known lock holder host ID from the DB (may be null) - * @param hosts routing hosts in the cluster or zone collected during fan-out + *

Hosts that did not answer the lock-state query are skipped in both cases. + * + * @param dbHostId last known lock holder host ID from the DB (may be null) + * @param hosts routing hosts in the cluster or zone collected during fan-out + * @param unreachableHostIds hosts that failed to answer the lock-state query * @return host ID to activate on, or null if no eligible host found */ - private Long selectActivationTargetHost(Long dbHostId, List hosts) { - if (dbHostId != null) { + private Long selectActivationTargetHost(Long dbHostId, List hosts, Set unreachableHostIds) { + if (dbHostId != null && !unreachableHostIds.contains(dbHostId)) { HostVO dbHost = _hostDao.findById(dbHostId); if (dbHost != null && dbHost.getStatus() == Status.Up && dbHost.getHypervisorType() == Hypervisor.HypervisorType.KVM) { @@ -319,7 +357,8 @@ private Long selectActivationTargetHost(Long dbHostId, List hosts) { List eligible = hosts.stream() .filter(h -> h.getStatus() == Status.Up && h.getType() == Host.Type.Routing - && h.getHypervisorType() == Hypervisor.HypervisorType.KVM) + && h.getHypervisorType() == Hypervisor.HypervisorType.KVM + && !unreachableHostIds.contains(h.getId())) .collect(Collectors.toList()); if (!eligible.isEmpty()) { Collections.shuffle(eligible); @@ -337,6 +376,9 @@ private Long selectActivationTargetHost(Long dbHostId, List hosts) { * Sends an {@code ACTIVATE_EXCLUSIVE} command to {@code targetHostId} and optionally * persists the new lock host to the database. * + *

A temporarily refused activation (lease not expired yet, lockspace still starting) is retried + * on the same host until {@code deadlineMs}. Other failures are not retried. + * *

Activation failure is non-fatal: returns null so the caller can apply its own fallback. * * @param volumeId volume DB ID @@ -344,30 +386,75 @@ private Long selectActivationTargetHost(Long dbHostId, List hosts) { * @param lvPath full LV device path, e.g. {@code /dev/vgname/vol-path} * @param targetHostId host to activate on * @param updateDatabase if true, persists the new lock host on success + * @param deadlineMs time in milliseconds after which a refused activation is no longer retried * @return {@code targetHostId} on success, {@code null} if the command failed or threw */ private Long tryActivateExclusivelyOnHost(Long volumeId, String volumeUuid, String lvPath, - Long targetHostId, boolean updateDatabase) { - try { - ClvmLockTransferCommand activateCmd = new ClvmLockTransferCommand( - ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE, lvPath, volumeUuid); - Answer activateAnswer = _agentMgr.send(targetHostId, activateCmd); - if (activateAnswer != null && activateAnswer.getResult()) { - logger.info("Recovery: exclusively activated volume {} on host {} (was inactive on all hosts)", - volumeUuid, targetHostId); - if (updateDatabase) { - setClvmLockHostId(volumeId, targetHostId); + Long targetHostId, boolean updateDatabase, long deadlineMs) { + long retryIntervalMs = getRetryIntervalMillis(); + while (true) { + try { + ClvmLockTransferCommand activateCmd = new ClvmLockTransferCommand( + ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE, lvPath, volumeUuid); + Answer activateAnswer = _agentMgr.send(targetHostId, activateCmd); + if (activateAnswer != null && activateAnswer.getResult()) { + logger.info("Recovery: exclusively activated volume {} on host {} (was inactive on all hosts)", + volumeUuid, targetHostId); + if (updateDatabase) { + setClvmLockHostId(volumeId, targetHostId); + } + return targetHostId; + } + logger.warn("Recovery activation of volume {} on host {} failed: {}", + volumeUuid, targetHostId, + activateAnswer != null ? activateAnswer.getDetails() : "null answer"); + if (!isRetryableRefusal(activateAnswer, retryIntervalMs, deadlineMs)) { + return null; } - return targetHostId; + logger.info("Activation of volume {} is refused temporarily, retrying on host {} in {} ms until the previous holder's lease expires or the lockspace is started", + volumeUuid, targetHostId, retryIntervalMs); + } catch (AgentUnavailableException | OperationTimedoutException e) { + logger.warn("Recovery activation of volume {} on host {} threw exception: {}", + volumeUuid, targetHostId, e.getMessage()); + return null; + } + if (!sleep(retryIntervalMs)) { + return null; } - logger.warn("Recovery activation of volume {} on host {} failed: {}", - volumeUuid, targetHostId, - activateAnswer != null ? activateAnswer.getDetails() : "null answer"); - } catch (AgentUnavailableException | OperationTimedoutException e) { - logger.warn("Recovery activation of volume {} on host {} threw exception: {}", - volumeUuid, targetHostId, e.getMessage()); } - return null; + } + + private boolean isRetryableRefusal(Answer activateAnswer, long retryIntervalMs, long deadlineMs) { + return retryIntervalMs > 0 + && activateAnswer != null + && activateAnswer.getDetails() != null + && TRANSIENT_LOCK_REFUSALS.stream().anyMatch(activateAnswer.getDetails()::contains) + && now() + retryIntervalMs <= deadlineMs; + } + + protected long getRetryTimeoutMillis() { + return ClvmLockRecoveryRetryTimeout.value() * 1000L; + } + + protected long getRetryIntervalMillis() { + return ClvmLockRecoveryRetryInterval.value() * 1000L; + } + + protected long now() { + return System.currentTimeMillis(); + } + + /** + * @return false if the thread was interrupted while waiting + */ + protected boolean sleep(long millis) { + try { + Thread.sleep(millis); + return true; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return false; + } } /** @@ -479,7 +566,9 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] { - CLVMSecureZeroFill + CLVMSecureZeroFill, + ClvmLockRecoveryRetryTimeout, + ClvmLockRecoveryRetryInterval }; } } diff --git a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java index 1678b3760919..6888e801676f 100644 --- a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java +++ b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java @@ -40,9 +40,13 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import org.springframework.test.util.ReflectionTestUtils; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.List; +import java.util.concurrent.atomic.AtomicLong; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; @@ -923,6 +927,309 @@ public void testQueryCurrentLockHolder_InactiveEverywhere_NoEligibleHost_Returns verify(agentMgr, never()).send(anyLong(), any(ClvmLockTransferCommand.class)); } + /** DB host is Up but times out on the query: recovery activates on the other host. */ + @Test + public void testQueryCurrentLockHolder_InactiveEverywhere_SkipsUnreachableDbHost_ActivatesOnOtherHost() + throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = mockClusterPool(); + + VolumeDetailVO detail = new VolumeDetailVO(); + detail.setValue(String.valueOf(HOST_ID_1)); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(detail); + + HostVO host1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO host2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findById(HOST_ID_1)).thenReturn(host1); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(host1, host2)); + + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenThrow(new OperationTimedoutException(null, HOST_ID_1, 0, 0, false)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, never()).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + /** No DB record, one Up host does not answer the fan-out: the fallback pick is the host that answered. */ + @Test + public void testQueryCurrentLockHolder_InactiveEverywhere_FallbackSkipsHostThatDidNotAnswer() + throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = mockClusterPool(); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(null); + + HostVO host1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO host2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(host1, host2)); + + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(null); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, never()).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + /** Every Up host fails to answer: nothing is activated. */ + @Test + public void testQueryCurrentLockHolder_AllHostsUnreachable_NoActivation() + throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = mockClusterPool(); + + VolumeDetailVO detail = new VolumeDetailVO(); + detail.setValue(String.valueOf(HOST_ID_1)); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(detail); + + HostVO host1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO host2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findById(HOST_ID_1)).thenReturn(host1); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(host1, host2)); + + when(agentMgr.send(anyLong(), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenThrow(new OperationTimedoutException(null, HOST_ID_1, 0, 0, false)); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + verify(agentMgr, never()).send(anyLong(), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + /** Fake clock: sleep only advances it and records the interval. */ + private static class RetryTestClvmPoolManager extends ClvmPoolManager { + private final AtomicLong clock = new AtomicLong(); + private final List sleeps = new ArrayList<>(); + private long timeoutMs = 180_000L; + + @Override + protected long now() { + return clock.get(); + } + + @Override + protected boolean sleep(long millis) { + sleeps.add(millis); + clock.addAndGet(millis); + return true; + } + + @Override + protected long getRetryTimeoutMillis() { + return timeoutMs; + } + + @Override + protected long getRetryIntervalMillis() { + return 10_000L; + } + } + + private RetryTestClvmPoolManager newRetryManager() { + RetryTestClvmPoolManager manager = new RetryTestClvmPoolManager(); + ReflectionTestUtils.setField(manager, "_volsDetailsDao", volsDetailsDao); + ReflectionTestUtils.setField(manager, "_agentMgr", agentMgr); + ReflectionTestUtils.setField(manager, "_hostDao", hostDao); + return manager; + } + + /** DB host is Down, HOST_ID_2 reports the volume inactive: recovery activates there. */ + private StoragePoolVO prepareRecoveryOnHost2() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = mockClusterPool(); + + VolumeDetailVO detail = new VolumeDetailVO(); + detail.setValue(String.valueOf(HOST_ID_1)); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(detail); + + HostVO downHost = createMockHost(HOST_ID_1, "host1", Status.Down, Hypervisor.HypervisorType.KVM); + HostVO upHost = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findById(HOST_ID_1)).thenReturn(downHost); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(downHost, upHost)); + + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + return pool; + } + + private static Answer lockedByOtherHost() { + return new Answer(null, false, "lvchange -aey /dev/acsvg/test-volume-path failed: LV locked by other host: acsvg/test-volume-path"); + } + + private static Answer lockStartInProgress() { + return new Answer(null, false, "lvchange -aey /dev/acsvg/test-volume-path failed: VG acsvg lock skipped: lock start in progress Cannot access VG acsvg due to failed lock."); + } + + @Test + public void testRecoveryActivation_LockStartInProgressThenGranted_RetriesOnSameHost() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockStartInProgress(), lockedByOtherHost(), new Answer(null, true, null)); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + Assert.assertEquals(Arrays.asList(10_000L, 10_000L), manager.sleeps); + verify(agentMgr, times(3)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_RefusedTwiceThenGranted_RetriesOnSameHost() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost(), lockedByOtherHost(), new Answer(null, true, null)); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + Assert.assertEquals(Arrays.asList(10_000L, 10_000L), manager.sleeps); + verify(agentMgr, times(3)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + verify(agentMgr, never()).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); + } + + @Test + public void testRecoveryActivation_RefusedUntilDeadline_GivesUp() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost()); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + Assert.assertEquals(18, manager.sleeps.size()); + Assert.assertEquals(180_000L, manager.clock.get()); + verify(agentMgr, times(19)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_DeadlineIsCountedFromLookupStart() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = mockClusterPool(); + + VolumeDetailVO detail = new VolumeDetailVO(); + detail.setValue(String.valueOf(HOST_ID_1)); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(detail); + + HostVO upHost1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO upHost2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findById(HOST_ID_1)).thenReturn(upHost1); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(upHost1, upHost2)); + + // The query to the DB host times out after 130 seconds + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenAnswer(invocation -> { + manager.clock.addAndGet(130_000L); + throw new OperationTimedoutException(null, HOST_ID_1, 0, 0, false); + }); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost()); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + Assert.assertEquals(5, manager.sleeps.size()); + Assert.assertEquals(180_000L, manager.clock.get()); + verify(agentMgr, times(6)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_OtherFailure_NotRetried() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, false, "lvchange failed: Cannot access VG acsvg due to failed lock")); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + Assert.assertTrue(manager.sleeps.isEmpty()); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_AgentException_NotRetried() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenThrow(new OperationTimedoutException(null, HOST_ID_2, 0, 0, false)); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + Assert.assertTrue(manager.sleeps.isEmpty()); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_RetryDisabledWithZeroTimeout() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + manager.timeoutMs = 0L; + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost()); + + Long result = manager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertNull(result); + Assert.assertTrue(manager.sleeps.isEmpty()); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_SharedDeadline_SecondVolumeNotRetriedOnceBudgetIsUsed() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost()); + long deadlineMs = manager.newLockRecoveryDeadlineMs(); + + Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, deadlineMs)); + Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, deadlineMs)); + + Assert.assertEquals(18, manager.sleeps.size()); + Assert.assertEquals(180_000L, manager.clock.get()); + verify(agentMgr, times(20)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testRecoveryActivation_SharedDeadline_SecondVolumeActivatedRightAfterTheFirst() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareRecoveryOnHost2(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost(), lockedByOtherHost(), new Answer(null, true, null), new Answer(null, true, null)); + long deadlineMs = manager.newLockRecoveryDeadlineMs(); + + Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, deadlineMs)); + Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, deadlineMs)); + + Assert.assertEquals(Arrays.asList(10_000L, 10_000L), manager.sleeps); + verify(agentMgr, times(4)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + private StoragePoolVO mockClusterPool() { + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + when(pool.getClusterId()).thenReturn(10L); + Mockito.lenient().when(pool.getName()).thenReturn("cluster-pool"); + when(pool.getPath()).thenReturn(VG_NAME); + return pool; + } + + private static Command operation(ClvmLockTransferCommand.Operation operation) { + return Mockito.argThat(cmd -> cmd instanceof ClvmLockTransferCommand + && ((ClvmLockTransferCommand) cmd).getOperation() == operation); + } + // Helper method to create mock hosts private HostVO createMockHost(Long id, String name, Status status, Hypervisor.HypervisorType hypervisor) { HostVO host = Mockito.mock(HostVO.class); From 799ab9253d329978b1a3d287815e4664836e0770 Mon Sep 17 00:00:00 2001 From: Pearl1594 Date: Sun, 8 Nov 2026 07:51:30 -0500 Subject: [PATCH 2/4] CLVM: find lock holder from sanlock lease instead of host fan-out - Check DB lock host first; if LV not active there, read the sanlock lease of the LV from one healthy host (max 2 readers) via QUERY_LEASE_OWNER - Map lease owner to CloudStack host through host.sanlock.id detail - Free lease or dead/restarted owner: activate on preferred host directly - Owner still renewing but agent down: fail fast instead of waiting - Fan-out to all hosts only as fallback (lease unreadable or owner unmapped), now parallel and skipping hosts that did not answer earlier - Lock transfer retries "lock start in progress" refusals - Alert on hosts sharing a sanlock host id at storage connect - Lookup cost grows with disks, not with cluster size --- .../clvm/command/ClvmLockTransferAnswer.java | 52 ++ .../clvm/command/ClvmLockTransferCommand.java | 5 +- .../orchestration/VolumeOrchestrator.java | 8 +- .../orchestration/VolumeOrchestratorTest.java | 18 +- .../provider/DefaultHostListener.java | 19 + ...LibvirtClvmLockTransferCommandWrapper.java | 26 + .../kvm/storage/ClvmSanlockHaChecker.java | 10 +- .../kvm/storage/ClvmSanlockLeaseReader.java | 196 +++++++ .../kvm/storage/ClvmSanlockHaCheckerTest.java | 4 +- .../storage/ClvmSanlockLeaseReaderTest.java | 124 +++++ .../cloud/storage/clvm/ClvmPoolManager.java | 493 ++++++++++++++---- .../cloud/storage/ClvmPoolManagerTest.java | 334 +++++++++++- 12 files changed, 1135 insertions(+), 154 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java diff --git a/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferAnswer.java b/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferAnswer.java index f3c43c400b2b..cf5d0e6475d5 100644 --- a/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferAnswer.java +++ b/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferAnswer.java @@ -17,6 +17,8 @@ package org.apache.cloudstack.storage.clvm.command; +import java.util.List; + import com.cloud.agent.api.Answer; /** @@ -25,7 +27,17 @@ */ public class ClvmLockTransferAnswer extends Answer { + /** Mode in which the sanlock lease of a volume is held. */ + public enum LeaseMode { + FREE, EXCLUSIVE, SHARED + } + private String currentLockHostname; + private LeaseMode leaseMode; + private Long leaseOwnerId; + private boolean leaseOwnerAlive; + private boolean leaseOwnerRenewing; + private List sharedOwnerIds; private boolean isActive; private boolean isOpen; private String lvAttributes; @@ -80,6 +92,46 @@ public void setOpen(boolean open) { isOpen = open; } + public static ClvmLockTransferAnswer leaseOwner(ClvmLockTransferCommand cmd, LeaseMode mode, Long ownerId, + boolean ownerAlive, List sharedOwnerIds) { + return leaseOwner(cmd, mode, ownerId, ownerAlive, ownerAlive, sharedOwnerIds); + } + + public static ClvmLockTransferAnswer leaseOwner(ClvmLockTransferCommand cmd, LeaseMode mode, Long ownerId, + boolean ownerAlive, boolean ownerRenewing, List sharedOwnerIds) { + ClvmLockTransferAnswer answer = new ClvmLockTransferAnswer(cmd, true, + String.format("Lease %s, owner=%s, alive=%s, shared=%s", mode, ownerId, ownerAlive, sharedOwnerIds)); + answer.leaseMode = mode; + answer.leaseOwnerId = ownerId; + answer.leaseOwnerAlive = ownerAlive; + answer.leaseOwnerRenewing = ownerRenewing; + answer.sharedOwnerIds = sharedOwnerIds; + return answer; + } + + public LeaseMode getLeaseMode() { + return leaseMode; + } + + /** sanlock host id of the exclusive holder, see {@link com.cloud.host.Host#HOST_SANLOCK_ID} */ + public Long getLeaseOwnerId() { + return leaseOwnerId; + } + + /** false when the exclusive holder stopped renewing its lockspace lease or was restarted since */ + public boolean isLeaseOwnerAlive() { + return leaseOwnerAlive; + } + + /** true while the exclusive holder still renews its lockspace lease, so its lease will not expire */ + public boolean isLeaseOwnerRenewing() { + return leaseOwnerRenewing; + } + + public List getSharedOwnerIds() { + return sharedOwnerIds; + } + public String getLvAttributes() { return lvAttributes; } diff --git a/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferCommand.java b/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferCommand.java index 218fb798869d..ad11894eeb2a 100644 --- a/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferCommand.java +++ b/core/src/main/java/org/apache/cloudstack/storage/clvm/command/ClvmLockTransferCommand.java @@ -46,7 +46,10 @@ public enum Operation { ACTIVATE_SHARED("-asy", "activate in shared mode"), /** Query the current lock state (lvs -o lv_attr,lv_host) */ - QUERY_LOCK_STATE("query", "query lock state"); + QUERY_LOCK_STATE("query", "query lock state"), + + /** Read the sanlock lease of the volume, which names the lock holder from any host in the lockspace */ + QUERY_LEASE_OWNER("query", "query lease owner"); private final String lvchangeFlag; private final String description; diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java index 53b5ee9d84dc..3292bfeafd79 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java @@ -881,7 +881,7 @@ private Long getClvmLockHostFromVmVolumes(Long vmId) { return null; } - long recoveryDeadlineMs = clvmPoolManager.newLockRecoveryDeadlineMs(); + ClvmPoolManager.LockLookup lockLookup = clvmPoolManager.newLockLookup(); for (VolumeVO volume : vmVolumes) { if (volume.getPoolId() == null) { continue; @@ -897,7 +897,7 @@ private Long getClvmLockHostFromVmVolumes(Long vmId) { volume.getPath(), pool, true, - recoveryDeadlineMs + lockLookup ); if (lockHostId != null) { logger.debug("Found actual CLVM lock host {} from volume {} of VM {} via LVM query", @@ -914,7 +914,7 @@ private void transferClvmLocksForVmStart(List volumes, Long destHostId return; } - long recoveryDeadlineMs = clvmPoolManager.newLockRecoveryDeadlineMs(); + ClvmPoolManager.LockLookup lockLookup = clvmPoolManager.newLockLookup(destHostId); for (VolumeVO volume : volumes) { if (volume.getPoolId() == null) { continue; @@ -931,7 +931,7 @@ private void transferClvmLocksForVmStart(List volumes, Long destHostId volume.getPath(), pool, true, - recoveryDeadlineMs + lockLookup ); if (currentLockHost == null) { diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java index 493864527bee..8ea769917782 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestratorTest.java @@ -135,7 +135,7 @@ public void setUp() throws Exception { accountPSResourceCount += increment; } return null; - }).when(resourceLimitMgr).incrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.anyLong()); + }).when(resourceLimitMgr).incrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.any()); Mockito.doAnswer((Answer) invocation -> { Resource.ResourceType type = (Resource.ResourceType)invocation.getArguments()[1]; Long decrement = (Long)invocation.getArguments()[3]; @@ -143,7 +143,7 @@ public void setUp() throws Exception { accountPSResourceCount -= decrement; } return null; - }).when(resourceLimitMgr).decrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.anyLong()); + }).when(resourceLimitMgr).decrementResourceCount(Mockito.anyLong(), Mockito.any(Resource.ResourceType.class), Mockito.anyBoolean(), Mockito.any()); } private void runCheckAndUpdateVolumeAccountResourceCountTest(Long originalSize, Long newSize) { @@ -696,9 +696,9 @@ public void testTransferClvmLocksForVmStart_WithClvmVolumes() throws Exception { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(102L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(true); @@ -768,7 +768,7 @@ public void testTransferClvmLocksForVmStart_NoLockTransferNeeded() throws Except ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), ArgumentMatchers.nullable(String.class), - ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(destHostId); + ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(destHostId); Mockito.when(storagePoolDao.findById(poolId)).thenReturn(clvmPool); @@ -801,7 +801,7 @@ public void testTransferClvmLocksForVmStart_EmptyVolumeList() throws Exception { method.invoke(volumeOrchestrator, new ArrayList(), destHostId, vmInstance); Mockito.verify(clvmPoolManager, Mockito.never()).getClvmLockHostId(Mockito.anyLong(), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.anyBoolean(), Mockito.anyLong()); + Mockito.anyString(), Mockito.any(), Mockito.anyBoolean(), Mockito.any()); } @Test @@ -842,7 +842,7 @@ public void testTransferClvmLocksForVmStart_SetInitialLockHost() throws Exceptio ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), ArgumentMatchers.nullable(String.class), - ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(null); + ArgumentMatchers.nullable(String.class), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(null); Mockito.when(storagePoolDao.findById(poolId)).thenReturn(clvmPool); @@ -888,7 +888,7 @@ public void testTransferClvmLocksForVmStart_MixedVolumes() throws Exception { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(true); @@ -929,7 +929,7 @@ public void testTransferClvmLocksForVmStart_TransferFails() throws Throwable { ClvmPoolManager clvmPoolManager = Mockito.mock(ClvmPoolManager.class); Mockito.when(clvmPoolManager.getClvmLockHostId(Mockito.eq(101L), Mockito.anyString(), - Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.anyLong())).thenReturn(currentHostId); + Mockito.anyString(), Mockito.any(), Mockito.eq(true), Mockito.any())).thenReturn(currentHostId); Mockito.when(clvmPoolManager.transferClvmVolumeLock(Mockito.anyString(), Mockito.anyLong(), Mockito.anyString(), Mockito.any(), Mockito.anyLong(), Mockito.anyLong())).thenReturn(false); diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java index 7644d4688f7e..8e28ceae6bb6 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java @@ -30,6 +30,7 @@ import com.cloud.configuration.ConfigurationManager; import com.cloud.dc.dao.DataCenterDao; import com.cloud.exception.StorageConflictException; +import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.network.NetworkModel; @@ -100,6 +101,8 @@ public class DefaultHostListener implements HypervisorHostListener { ConfigurationManager configManager; @Inject NetworkDao networkDao; + @Inject + ClvmPoolManager clvmPoolManager; @Override public boolean hostAdded(long hostId) { @@ -193,11 +196,27 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep storageService.updateStorageCapabilities(poolId, false); + if (ClvmPoolManager.isClvmPoolType(pool.getPoolType())) { + alertOnSharedSanlockId(pool, host); + } + logger.info("Connection established between storage pool {} and host {}", pool, host); return createPersistentNetworkResourcesOnHost(hostId); } + /** CLVM lock holder lookups map a sanlock host id to exactly one host of the pool. */ + private void alertOnSharedSanlockId(StoragePool pool, HostVO host) { + List otherHostIds = clvmPoolManager.findHostsSharingSanlockId(pool, host.getId()); + if (otherHostIds.isEmpty()) { + return; + } + String msg = String.format("Host %s reports the same %s as hosts %s of CLVM storage pool %s. Set a unique host_id in lvmlocal.conf on each host.", + host, Host.HOST_SANLOCK_ID, otherHostIds, pool); + logger.error(msg); + alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, pool.getDataCenterId(), pool.getPodId(), msg, msg); + } + private void updateStoragePoolHostVOAndDetails(StoragePool pool, long hostId, ModifyStoragePoolAnswer mspAnswer) { StoragePoolHostVO poolHost = storagePoolHostDao.findByPoolHost(pool.getId(), hostId); if (poolHost == null) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java index 18c4324addf5..943acaa4d45a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java @@ -17,7 +17,12 @@ package com.cloud.hypervisor.kvm.resource.wrapper; +import java.io.File; + import com.cloud.agent.api.Answer; +import com.cloud.hypervisor.kvm.storage.ClvmSanlockLeaseReader; +import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer.LeaseMode; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; @@ -44,6 +49,9 @@ public Answer execute(ClvmLockTransferCommand cmd, LibvirtComputingResource serv if (operation == ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE) { return handleQueryLockState(cmd, lvPath, volumeUuid); } + if (operation == ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER) { + return handleQueryLeaseOwner(cmd, lvPath, volumeUuid); + } String lvchangeOpt; String operationDesc; @@ -90,6 +98,24 @@ public Answer execute(ClvmLockTransferCommand cmd, LibvirtComputingResource serv } } + /** Reads the sanlock lease of the LV, which names the holder from any host in the lockspace. */ + private Answer handleQueryLeaseOwner(ClvmLockTransferCommand cmd, String lvPath, String volumeUuid) { + File lv = new File(lvPath); + String vgName = lv.getParentFile().getName(); + try { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.readLease(vgName, lv.getName()); + ClvmSanlockLeaseReader.OwnerLiveness owner = lease.getMode() == LeaseMode.EXCLUSIVE + ? ClvmSanlockLeaseReader.readOwnerLiveness(vgName, lease) : new ClvmSanlockLeaseReader.OwnerLiveness(false, false); + logger.info("Lease of volume {}: {}, owner={}, alive={}, renewing={}, shared={}", + volumeUuid, lease.getMode(), lease.getOwnerId(), owner.isAlive(), owner.isRenewing(), lease.getSharedOwnerIds()); + return ClvmLockTransferAnswer.leaseOwner(cmd, lease.getMode(), lease.getOwnerId(), owner.isAlive(), owner.isRenewing(), + lease.getSharedOwnerIds()); + } catch (CloudRuntimeException | NumberFormatException e) { + logger.warn("Failed to read the sanlock lease of volume {}: {}", volumeUuid, e.getMessage()); + return new ClvmLockTransferAnswer(cmd, false, "Failed to read sanlock lease: " + e.getMessage()); + } + } + /** * Query whether this host currently has the CLVM LV activated locally. * Executes: lvs -o lv_attr,lv_host,lv_active --noheadings diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java index 89d7e89f97a2..95eca074d3cf 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java @@ -47,11 +47,13 @@ protected static class LeaseState { final long lastCheck; final long lastLive; final long ioTimeout; + final Long generation; - LeaseState(long lastCheck, long lastLive, long ioTimeout) { + LeaseState(long lastCheck, long lastLive, long ioTimeout, Long generation) { this.lastCheck = lastCheck; this.lastLive = lastLive; this.ioTimeout = ioTimeout; + this.generation = generation; } } @@ -100,7 +102,7 @@ public static boolean isHostLeaseAlive(String vgName, Long hostId) { return isLeaseAlive(state); } - private static String readHostStatus(String vgName) { + static String readHostStatus(String vgName) { Script cmd = new Script("sanlock", CHECK_TIMEOUT_MS, LOGGER); cmd.add("client", "host_status", "-s", LOCKSPACE_PREFIX + vgName, "-D"); OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); @@ -126,6 +128,7 @@ private static LeaseState readLeaseState(String output, long hostId) { Long lastCheck = null; Long lastLive = null; Long ioTimeout = null; + Long generation = null; for (String line : output.split("\\r?\\n")) { Matcher entry = HOST_ENTRY.matcher(line); if (entry.matches()) { @@ -137,12 +140,13 @@ private static LeaseState readLeaseState(String output, long hostId) { lastCheck = valueOrCurrent(line, "last_check", lastCheck); lastLive = valueOrCurrent(line, "last_live", lastLive); ioTimeout = valueOrCurrent(line, "io_timeout", ioTimeout); + generation = valueOrCurrent(line, "owner_generation", generation); } } if (lastCheck == null || lastLive == null || ioTimeout == null) { return null; } - return new LeaseState(lastCheck, lastLive, ioTimeout); + return new LeaseState(lastCheck, lastLive, ioTimeout, generation); } private static Long valueOrCurrent(String line, String key, Long current) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java new file mode 100644 index 000000000000..84086c2de577 --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java @@ -0,0 +1,196 @@ +// 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. + +package com.cloud.hypervisor.kvm.storage; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer.LeaseMode; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.script.OutputInterpreter; +import com.cloud.utils.script.Script; + +/** + * Reads the sanlock lease of a CLVM/CLVM_NG volume from the VG's lvmlock LV. Every host in the + * lockspace can read it, so one host names the lock holder of the volume. + */ +public class ClvmSanlockLeaseReader { + + private static final Logger LOGGER = LogManager.getLogger(ClvmSanlockLeaseReader.class); + private static final long TIMEOUT_MS = 30000; + private static final long LEASE_SIZE_BYTES = 1048576; + private static final long RENEWING_IO_TIMEOUTS = 3; + private static final Pattern SHARED_HOLDER = Pattern.compile("^\\s*(\\d+)\\s+\\d+\\s+SH\\s*$"); + + /** Lease of one volume as seen on disk. */ + public static class Lease { + final LeaseMode mode; + final Long ownerId; + final Long ownerGeneration; + final List sharedOwnerIds; + + Lease(LeaseMode mode, Long ownerId, Long ownerGeneration, List sharedOwnerIds) { + this.mode = mode; + this.ownerId = ownerId; + this.ownerGeneration = ownerGeneration; + this.sharedOwnerIds = sharedOwnerIds; + } + + public LeaseMode getMode() { + return mode; + } + + public Long getOwnerId() { + return ownerId; + } + + public List getSharedOwnerIds() { + return sharedOwnerIds; + } + } + + public static Lease readLease(String vgName, String lvName) { + String[] uuidAndOffset = readLockArgs(vgName, lvName); + String dump = dumpLease(vgName, uuidAndOffset[1]); + return parseDump(dump, uuidAndOffset[0]); + } + + /** Liveness of the exclusive holder, as seen in this host's view of the lockspace. */ + public static class OwnerLiveness { + final boolean alive; + final boolean renewing; + + public OwnerLiveness(boolean alive, boolean renewing) { + this.alive = alive; + this.renewing = renewing; + } + + public boolean isAlive() { + return alive; + } + + public boolean isRenewing() { + return renewing; + } + } + + public static OwnerLiveness readOwnerLiveness(String vgName, Lease lease) { + String status = ClvmSanlockHaChecker.readHostStatus(vgName); + ClvmSanlockHaChecker.LeaseState state = status == null ? null : ClvmSanlockHaChecker.parseLeaseState(status, lease.ownerId); + return new OwnerLiveness(isOwnerAlive(state, lease), isOwnerRenewing(state, lease)); + } + + /** True while the holder renews its lockspace lease, so its locks will not expire. Unknown counts as renewing. */ + protected static boolean isOwnerRenewing(ClvmSanlockHaChecker.LeaseState state, Lease lease) { + if (state == null) { + return true; + } + if (state.generation != null && !state.generation.equals(lease.ownerGeneration)) { + return false; + } + return state.lastCheck - state.lastLive < RENEWING_IO_TIMEOUTS * state.ioTimeout; + } + + protected static boolean isOwnerAlive(ClvmSanlockHaChecker.LeaseState state, Lease lease) { + if (state == null) { + LOGGER.warn("No lockspace lease found for sanlock host id [{}], assuming it is alive.", lease.ownerId); + return true; + } + if (state.generation != null && !state.generation.equals(lease.ownerGeneration)) { + return false; + } + return ClvmSanlockHaChecker.isLeaseAlive(state); + } + + /** @return LV uuid (the sanlock resource name) and the lease offset in the lvmlock LV */ + private static String[] readLockArgs(String vgName, String lvName) { + Script cmd = new Script("lvs", TIMEOUT_MS, LOGGER); + cmd.add("--noheadings", "-o", "lv_uuid,lv_lockargs", vgName + "/" + lvName); + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + String result = cmd.execute(parser); + if (result != null) { + throw new CloudRuntimeException(String.format("lvs failed for %s/%s: %s", vgName, lvName, result)); + } + return parseLockArgs(parser.getLines()); + } + + /** Skips warning lines, e.g. "VG ... lock skipped: lock start in progress" while the lockspace is joining. */ + protected static String[] parseLockArgs(String output) { + for (String line : output == null ? new String[0] : output.split("\\r?\\n")) { + String[] parts = line.trim().split("\\s+"); + if (parts.length == 2 && parts[1].matches("^[\\d.]+:\\d+$")) { + return new String[] {parts[0], parts[1].substring(parts[1].lastIndexOf(':') + 1)}; + } + } + throw new CloudRuntimeException(String.format("No sanlock lock args in lvs output [%s]", output)); + } + + private static String dumpLease(String vgName, String offset) { + Script cmd = new Script("sanlock", TIMEOUT_MS, LOGGER); + cmd.add("direct", "dump", String.format("%s:%s:%d", lvmlockDevice(vgName), offset, LEASE_SIZE_BYTES)); + OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); + String result = cmd.execute(parser); + if (result != null) { + throw new CloudRuntimeException(String.format("sanlock direct dump failed for VG %s: %s", vgName, result)); + } + return parser.getLines(); + } + + protected static String lvmlockDevice(String vgName) { + return "/dev/mapper/" + vgName.replace("-", "--") + "-lvmlock"; + } + + /** + * Parses a {@code sanlock direct dump} of one lease: a row "offset lockspace resource timestamp own gen lver", + * followed by one "host_id gen SH" row per shared holder. A non-zero timestamp means held exclusively. + */ + protected static Lease parseDump(String output, String lvUuid) { + Long timestamp = null; + Long owner = null; + Long generation = null; + List sharedOwnerIds = new ArrayList<>(); + for (String line : output.split("\\r?\\n")) { + String[] tokens = line.trim().split("\\s+"); + if (timestamp == null) { + if (tokens.length >= 6 && tokens[2].equals(lvUuid)) { + timestamp = Long.parseLong(tokens[3]); + owner = Long.parseLong(tokens[4]); + generation = Long.parseLong(tokens[5]); + } + continue; + } + Matcher shared = SHARED_HOLDER.matcher(line); + if (!shared.matches()) { + break; + } + sharedOwnerIds.add(Long.parseLong(shared.group(1))); + } + if (timestamp == null) { + throw new CloudRuntimeException(String.format("No sanlock lease for LV uuid %s in dump output", lvUuid)); + } + if (timestamp != 0) { + return new Lease(LeaseMode.EXCLUSIVE, owner, generation, sharedOwnerIds); + } + return new Lease(sharedOwnerIds.isEmpty() ? LeaseMode.FREE : LeaseMode.SHARED, null, null, sharedOwnerIds); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java index c1465953e5a0..85d5fe3e538a 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java @@ -68,8 +68,8 @@ public void testParseLeaseStateForHostWhoseLeaseStoppedRenewing() { @Test public void testLeaseStillAliveBeforeWatchdogGuarantee() { - assertTrue(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1139, 1000, 10))); - assertFalse(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1140, 1000, 10))); + assertTrue(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1139, 1000, 10, 1L))); + assertFalse(ClvmSanlockHaChecker.isLeaseAlive(new ClvmSanlockHaChecker.LeaseState(1140, 1000, 10, 1L))); } @Test diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java new file mode 100644 index 000000000000..7a21e1a764f0 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java @@ -0,0 +1,124 @@ +// 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. + +package com.cloud.hypervisor.kvm.storage; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.Collections; + +import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer.LeaseMode; +import org.junit.Test; + +import com.cloud.utils.exception.CloudRuntimeException; + +public class ClvmSanlockLeaseReaderTest { + + private static final String HEADER = " offset lockspace resource timestamp own gen lver\n"; + + // Captured with sanlock 3.8.4: "sanlock direct dump /dev/mapper/sharedvg-lvmlock::1048576" + private static final String EXCLUSIVE = HEADER + + "75497472 lvm_sharedvg 8Oicxc-uWrd-HEQB-L1Rr-IGZF-L08Y-IMmOoK 0001213684 0002 0001 1\n"; + + private static final String SHARED = HEADER + + "74448896 lvm_sharedvg OBe1FA-KGjN-RsXD-j93O-ovWz-uyM7-Gbro7e 0000000000 0002 0001 3\n" + + " 0001 0002 SH\n" + + " 0002 0001 SH\n"; + + private static final String RELEASED = HEADER + + "69206016 lvm_sharedvg VGLK 0000000000 0002 0001 54890\n"; + + @Test + public void testParseDumpExclusive() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(EXCLUSIVE, "8Oicxc-uWrd-HEQB-L1Rr-IGZF-L08Y-IMmOoK"); + assertEquals(LeaseMode.EXCLUSIVE, lease.getMode()); + assertEquals(Long.valueOf(2), lease.getOwnerId()); + assertEquals(Long.valueOf(1), lease.ownerGeneration); + } + + @Test + public void testParseDumpShared() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(SHARED, "OBe1FA-KGjN-RsXD-j93O-ovWz-uyM7-Gbro7e"); + assertEquals(LeaseMode.SHARED, lease.getMode()); + assertNull(lease.getOwnerId()); + assertEquals(Arrays.asList(1L, 2L), lease.getSharedOwnerIds()); + } + + @Test + public void testParseDumpReleasedIgnoresStaleOwner() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(RELEASED, "VGLK"); + assertEquals(LeaseMode.FREE, lease.getMode()); + assertNull(lease.getOwnerId()); + assertEquals(Collections.emptyList(), lease.getSharedOwnerIds()); + } + + @Test(expected = CloudRuntimeException.class) + public void testParseDumpWithoutTheLease() { + ClvmSanlockLeaseReader.parseDump(EXCLUSIVE, "other-uuid"); + } + + @Test + public void testParseLockArgs() { + assertArrayEquals(new String[] {"rQXPx2-5Ecb-PdRO-oUlx-VqV9-RIYu-thyuIm", "73400320"}, + ClvmSanlockLeaseReader.parseLockArgs(" rQXPx2-5Ecb-PdRO-oUlx-VqV9-RIYu-thyuIm 1.0.0:73400320")); + } + + @Test + public void testParseLockArgsSkipsLockStartWarning() { + assertArrayEquals(new String[] {"rQXPx2-5Ecb-PdRO-oUlx-VqV9-RIYu-thyuIm", "73400320"}, + ClvmSanlockLeaseReader.parseLockArgs(" VG sharedvg2 lock skipped: lock start in progress\n" + + " rQXPx2-5Ecb-PdRO-oUlx-VqV9-RIYu-thyuIm 1.0.0:73400320\n")); + } + + @Test(expected = CloudRuntimeException.class) + public void testParseLockArgsWithoutSanlockLock() { + ClvmSanlockLeaseReader.parseLockArgs(" rQXPx2-5Ecb-PdRO-oUlx-VqV9-RIYu-thyuIm"); + } + + @Test + public void testLvmlockDeviceEscapesDashes() { + assertEquals("/dev/mapper/sharedvg-lvmlock", ClvmSanlockLeaseReader.lvmlockDevice("sharedvg")); + assertEquals("/dev/mapper/shared--vg-lvmlock", ClvmSanlockLeaseReader.lvmlockDevice("shared-vg")); + } + + @Test + public void testOwnerAliveOnlyWithSameGeneration() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(EXCLUSIVE, "8Oicxc-uWrd-HEQB-L1Rr-IGZF-L08Y-IMmOoK"); + assertTrue(ClvmSanlockLeaseReader.isOwnerAlive(new ClvmSanlockHaChecker.LeaseState(1100, 1100, 10, 1L), lease)); + assertFalse(ClvmSanlockLeaseReader.isOwnerAlive(new ClvmSanlockHaChecker.LeaseState(1100, 1100, 10, 2L), lease)); + } + + @Test + public void testOwnerRenewingOnlyWithFreshLeaseAndSameGeneration() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(EXCLUSIVE, "8Oicxc-uWrd-HEQB-L1Rr-IGZF-L08Y-IMmOoK"); + assertTrue(ClvmSanlockLeaseReader.isOwnerRenewing(new ClvmSanlockHaChecker.LeaseState(1020, 1000, 10, 1L), lease)); + assertFalse(ClvmSanlockLeaseReader.isOwnerRenewing(new ClvmSanlockHaChecker.LeaseState(1030, 1000, 10, 1L), lease)); + assertFalse(ClvmSanlockLeaseReader.isOwnerRenewing(new ClvmSanlockHaChecker.LeaseState(1020, 1000, 10, 2L), lease)); + assertTrue(ClvmSanlockLeaseReader.isOwnerRenewing(null, lease)); + } + + @Test + public void testOwnerNotAliveWhenLockspaceLeaseStoppedRenewing() { + ClvmSanlockLeaseReader.Lease lease = ClvmSanlockLeaseReader.parseDump(EXCLUSIVE, "8Oicxc-uWrd-HEQB-L1Rr-IGZF-L08Y-IMmOoK"); + assertFalse(ClvmSanlockLeaseReader.isOwnerAlive(new ClvmSanlockHaChecker.LeaseState(1140, 1000, 10, 1L), lease)); + } +} diff --git a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java index ae3556011ba4..aee5f1a33170 100644 --- a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java +++ b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java @@ -21,9 +21,18 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import javax.inject.Inject; @@ -34,15 +43,19 @@ import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.Status; +import com.cloud.host.DetailVO; import com.cloud.host.dao.HostDao; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.Storage; import com.cloud.storage.StoragePool; import com.cloud.storage.VolumeDetailVO; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDetailsDao; +import com.cloud.utils.concurrency.NamedThreadFactory; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; +import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer; import org.apache.logging.log4j.LogManager; @@ -57,6 +70,8 @@ public class ClvmPoolManager implements Configurable { private AgentManager _agentMgr; @Inject private HostDao _hostDao; + @Inject + private HostDetailsDao _hostDetailsDao; protected Logger logger = LogManager.getLogger(getClass()); @@ -66,7 +81,8 @@ public class ClvmPoolManager implements Configurable { */ public static final String CLVM_LOCK_HOST_ID = "clvmLockHostId"; - private static final List TRANSIENT_LOCK_REFUSALS = List.of("locked by other host", "lock start in progress"); + private static final String LOCK_START_IN_PROGRESS = "lock start in progress"; + private static final List TRANSIENT_LOCK_REFUSALS = List.of("locked by other host", LOCK_START_IN_PROGRESS); public static final ConfigKey ClvmLockRecoveryRetryTimeout = new ConfigKey<>("Advanced", Integer.class, "clvm.lock.recovery.retry.timeout", "180", "Maximum time in seconds, counted from the start of the CLVM lock holder lookup, to keep retrying the exclusive activation of a volume that is refused only temporarily: the previous lock holder's lease has not expired yet, or the lockspace of the volume group is still being started on the host. The lease expires 8 x io_timeout + 60 seconds after the holder stops renewing it (140 seconds with the default lvmlockd sanlock io_timeout of 10). If lvmlockd runs with a different --sanlock-timeout, set this to at least that value plus about 40 seconds. 0 disables the retry.", true); @@ -77,6 +93,34 @@ public class ClvmPoolManager implements Configurable { public static final ConfigKey CLVMSecureZeroFill = new ConfigKey<>("Advanced", Boolean.class, "clvm.secure.zero.fill", "false", "When enabled, CLVM volumes to be zero-filled at the time of deletion to prevent data from being recovered by VMs reusing the space, as thick LVM volumes write data linearly. Note: This setting is propagated to hosts when they connect to the storage pool. Changing this setting requires disconnecting and reconnecting hosts or restarting the KVM agent for it to take effect.", false, ConfigKey.Scope.StoragePool); + private static final int LOCK_QUERY_THREADS = 16; + private static final int MAX_LEASE_READERS = 2; + + private final ExecutorService lockQueryExecutor = newLockQueryExecutor(); + + /** State shared by the lock lookups of all volumes of one operation. */ + public static class LockLookup { + private final long activationDeadlineMs; + private final Long preferredHostId; + private final Set unreachableHostIds = ConcurrentHashMap.newKeySet(); + + LockLookup(long activationDeadlineMs, Long preferredHostId) { + this.activationDeadlineMs = activationDeadlineMs; + this.preferredHostId = preferredHostId; + } + + boolean isUnreachable(Long hostId) { + return unreachableHostIds.contains(hostId); + } + } + + private static ExecutorService newLockQueryExecutor() { + ThreadPoolExecutor executor = new ThreadPoolExecutor(LOCK_QUERY_THREADS, LOCK_QUERY_THREADS, 60L, TimeUnit.SECONDS, + new LinkedBlockingQueue<>(), new NamedThreadFactory("ClvmLockQuery")); + executor.allowCoreThreadTimeOut(true); + return executor; + } + public static boolean isClvmPoolType(Storage.StoragePoolType poolType) { return Arrays.asList(Storage.StoragePoolType.CLVM, Storage.StoragePoolType.CLVM_NG).contains(poolType); } @@ -116,26 +160,31 @@ public Long getClvmLockHostId(Long volumeId, String volumeUuid) { */ public Long getClvmLockHostId(Long volumeId, String volumeUuid, String volumePath, StoragePool pool, boolean queryActual) { - return getClvmLockHostId(volumeId, volumeUuid, volumePath, pool, queryActual, newLockRecoveryDeadlineMs()); + return getClvmLockHostId(volumeId, volumeUuid, volumePath, pool, queryActual, newLockLookup()); } - /** Same as above, with a retry deadline shared across calls, see {@link #newLockRecoveryDeadlineMs()}. */ + /** Same as above, sharing {@code lookup} with the other volumes of the same operation, see {@link #newLockLookup()}. */ public Long getClvmLockHostId(Long volumeId, String volumeUuid, String volumePath, - StoragePool pool, boolean queryActual, long activationDeadlineMs) { + StoragePool pool, boolean queryActual, LockLookup lookup) { if (queryActual) { if (volumePath == null || pool == null) { logger.warn("Cannot query actual CLVM lock state for volume {} - missing volumePath or pool", volumeUuid); return getClvmLockHostId(volumeId, volumeUuid); } - return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, true, activationDeadlineMs); + return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, true, lookup); } return getClvmLockHostId(volumeId, volumeUuid); } - /** @return the time in milliseconds until which a refused activation is retried, counted from now */ - public long newLockRecoveryDeadlineMs() { - return now() + getRetryTimeoutMillis(); + /** @return lookup state for one operation; its activation retry deadline is counted from now */ + public LockLookup newLockLookup() { + return newLockLookup(null); + } + + /** Same as above; a volume no host holds is activated on {@code preferredHostId} when possible. */ + public LockLookup newLockLookup(Long preferredHostId) { + return new LockLookup(now() + getRetryTimeoutMillis(), preferredHostId); } /** @@ -174,12 +223,12 @@ public void setClvmLockHostId(long volumeId, long hostId) { */ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volumePath, StoragePool pool, boolean updateDatabase) { - return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, updateDatabase, newLockRecoveryDeadlineMs()); + return queryCurrentLockHolder(volumeId, volumeUuid, volumePath, pool, updateDatabase, newLockLookup()); } - /** Same as above, retrying a refused recovery activation until {@code activationDeadlineMs}. */ + /** Same as above, sharing the retry deadline and the unreachable hosts in {@code lookup}. */ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volumePath, - StoragePool pool, boolean updateDatabase, long activationDeadlineMs) { + StoragePool pool, boolean updateDatabase, LockLookup lookup) { if (pool == null) { logger.error("Cannot query CLVM lock for volume {} - pool is null", volumeUuid); return null; @@ -190,88 +239,98 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu vgName = vgName.substring(1); } String lvPath = String.format("/dev/%s/%s", vgName, volumePath); + Long dbHostId = getClvmLockHostId(volumeId, volumeUuid); + // Hosts that did not answer the lock-state query for this volume + Set noAnswerHostIds = new HashSet<>(); - // Hosts that did not answer the lock-state query - Set unreachableHostIds = new HashSet<>(); + Boolean dbHostActive = queryDbHostLockState(dbHostId, lvPath, volumeUuid, noAnswerHostIds, lookup); + if (Boolean.TRUE.equals(dbHostActive)) { + logger.debug("Fast path: volume {} confirmed active on DB host {}", volumeUuid, dbHostId); + return dbHostId; + } - // Fast path: trust the DB record and verify with a single host query - Long dbHostId = getClvmLockHostId(volumeId, volumeUuid); - if (dbHostId != null) { - HostVO dbHost = _hostDao.findById(dbHostId); - if (dbHost != null && dbHost.getStatus() == Status.Up - && dbHost.getHypervisorType() == Hypervisor.HypervisorType.KVM) { - Boolean active = querySingleHostLockState(dbHostId, lvPath, volumeUuid); - if (Boolean.TRUE.equals(active)) { - logger.debug("Fast path: volume {} confirmed active on DB host {}", volumeUuid, dbHostId); - return dbHostId; - } - if (active == null) { - unreachableHostIds.add(dbHostId); + List hosts = findPoolHosts(pool); + + ClvmLockTransferAnswer lease = hosts.isEmpty() ? null : readLease(lvPath, volumeUuid, dbHostId, hosts, lookup); + if (lease != null) { + if (isLeaseFree(lease, volumeUuid)) { + return recoverInactiveVolume(volumeId, volumeUuid, lvPath, dbHostId, hosts, noAnswerHostIds, updateDatabase, lookup); + } + List holderSanlockIds = getLeaseHolderIds(lease); + if (holderSanlockIds.size() > 1) { + logger.warn("Volume {} is held in shared mode by sanlock hosts {}. Skipping exclusive lock transfer.", + volumeUuid, holderSanlockIds); + return null; + } + Long lockHostId = findHostBySanlockId(hosts, holderSanlockIds.get(0)); + if (lockHostId != null) { + if (isLeaseHolderInUse(lockHostId, dbHostId, dbHostActive, lease.isLeaseOwnerRenewing(), lvPath, volumeUuid, lookup)) { + logger.info("Volume {} is held by host {} (sanlock lease, DB had {})", volumeUuid, lockHostId, dbHostId); + return recordLockHolder(volumeId, volumeUuid, dbHostId, lockHostId, updateDatabase); } - logger.info("Fast path miss: volume {} not active on DB host {} - falling back to full fan-out", - volumeUuid, dbHostId); - } else { - logger.info("Fast path skip: DB host {} for volume {} is down/missing — falling back to full fan-out", - dbHostId, volumeUuid); + return recoverInactiveVolume(volumeId, volumeUuid, lvPath, dbHostId, hosts, noAnswerHostIds, updateDatabase, lookup); } } + return findLockHolderByFanOut(volumeId, volumeUuid, lvPath, pool, dbHostId, hosts, noAnswerHostIds, updateDatabase, lookup); + } - List hosts = null; - Long clusterId = pool.getClusterId(); - if (clusterId != null) { - hosts = _hostDao.findByClusterId(clusterId, Host.Type.Routing); - } else if (pool.getDataCenterId() > 0) { - hosts = _hostDao.findByDataCenterId(pool.getDataCenterId()); + /** + * Fast path: asks the host recorded in the DB, as an LV is only active on the host holding its lock. + * + * @return the lock state on the DB host, or null when it was not asked or did not answer + */ + private Boolean queryDbHostLockState(Long dbHostId, String lvPath, String volumeUuid, Set noAnswerHostIds, LockLookup lookup) { + if (dbHostId == null) { + return null; } - if (hosts == null || hosts.isEmpty()) { - logger.warn("No KVM routing hosts found to query CLVM lock state for volume {} (pool: {}, cluster: {}, zone: {})", - volumeUuid, pool.getName(), clusterId, pool.getDataCenterId()); + if (lookup.isUnreachable(dbHostId)) { + logger.info("Fast path skip: DB host {} for volume {} did not answer an earlier query", dbHostId, volumeUuid); return null; } + if (!isUpKvmRoutingHost(_hostDao.findById(dbHostId))) { + logger.info("Fast path skip: DB host {} for volume {} is down/missing", dbHostId, volumeUuid); + return null; + } + Boolean active = querySingleHostLockState(dbHostId, lvPath, volumeUuid, lookup); + if (active == null) { + noAnswerHostIds.add(dbHostId); + } else if (!active) { + logger.info("Fast path miss: volume {} not active on DB host {}", volumeUuid, dbHostId); + } + return active; + } - List activeHostIds = new ArrayList<>(); + /** Asks every host whether the volume is active there; used when the sanlock lease cannot name the holder. */ + private Long findLockHolderByFanOut(Long volumeId, String volumeUuid, String lvPath, StoragePool pool, Long dbHostId, + List hosts, Set noAnswerHostIds, boolean updateDatabase, LockLookup lookup) { + if (hosts.isEmpty()) { + logger.warn("No KVM routing hosts found to query CLVM lock state for volume {} (pool: {}, cluster: {}, zone: {})", + volumeUuid, pool.getName(), pool.getClusterId(), pool.getDataCenterId()); + return null; + } - for (HostVO host : hosts) { - if (host.getStatus() != Status.Up || - host.getType() != Host.Type.Routing || - host.getHypervisorType() != Hypervisor.HypervisorType.KVM) { - continue; - } - // Skip the DB host, already confirmed inactive in the fast path above - if (dbHostId != null && host.getId() == dbHostId) { - continue; - } + // The DB host was already queried in the fast path + List hostIdsToQuery = hosts.stream() + .filter(this::isUpKvmRoutingHost) + .map(HostVO::getId) + .filter(id -> !id.equals(dbHostId) && !lookup.isUnreachable(id)) + .collect(Collectors.toList()); + Map lockStates = queryHostsLockState(hostIdsToQuery, lvPath, volumeUuid, lookup); - Boolean active = querySingleHostLockState(host.getId(), lvPath, volumeUuid); + List activeHostIds = new ArrayList<>(); + for (Long hostId : hostIdsToQuery) { + Boolean active = lockStates.get(hostId); if (Boolean.TRUE.equals(active)) { - logger.debug("Volume {} is locally active on host {} (fan-out)", volumeUuid, host.getId()); - activeHostIds.add(host.getId()); + logger.debug("Volume {} is locally active on host {} (fan-out)", volumeUuid, hostId); + activeHostIds.add(hostId); } else if (active == null) { - unreachableHostIds.add(host.getId()); + noAnswerHostIds.add(hostId); } } if (activeHostIds.isEmpty()) { logger.debug("Volume {} is not active on any reachable host — no exclusive lock held", volumeUuid); - - // Recovery: attempt exclusive activation on the best available host before giving up. - Long targetHostId = selectActivationTargetHost(dbHostId, hosts, unreachableHostIds); - if (targetHostId != null) { - Long recoveredHostId = tryActivateExclusivelyOnHost(volumeId, volumeUuid, lvPath, - targetHostId, updateDatabase, activationDeadlineMs); - if (recoveredHostId != null) { - return recoveredHostId; - } - } - - // Activation failed or no eligible host - clean up stale DB record and give up - if (updateDatabase && dbHostId != null) { - VolumeDetailVO detail = _volsDetailsDao.findDetail(volumeId, CLVM_LOCK_HOST_ID); - if (detail != null && String.valueOf(dbHostId).equals(detail.getValue())) { - _volsDetailsDao.remove(detail.getId()); - } - } - return null; + return recoverInactiveVolume(volumeId, volumeUuid, lvPath, dbHostId, hosts, noAnswerHostIds, updateDatabase, lookup); } if (activeHostIds.size() > 1) { @@ -284,26 +343,237 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu Long lockHostId = activeHostIds.get(0); logger.info("Volume {} is exclusively active on host {} (found via fan-out, DB had {})", volumeUuid, lockHostId, dbHostId); + return recordLockHolder(volumeId, volumeUuid, dbHostId, lockHostId, updateDatabase); + } - if (updateDatabase) { - if (dbHostId == null || !dbHostId.equals(lockHostId)) { - logger.info("Correcting database: volume {} lock host: {} -> {} (actual)", - volumeUuid, dbHostId, lockHostId); - setClvmLockHostId(volumeId, lockHostId); + private Long recordLockHolder(Long volumeId, String volumeUuid, Long dbHostId, Long lockHostId, boolean updateDatabase) { + if (updateDatabase && !lockHostId.equals(dbHostId)) { + logger.info("Correcting database: volume {} lock host: {} -> {} (actual)", + volumeUuid, dbHostId, lockHostId); + setClvmLockHostId(volumeId, lockHostId); + } + return lockHostId; + } + + /** + * Reads the sanlock lease of the volume on one host; any host in the lockspace sees every lease. + * Tries a second host if the first does not answer. + * + * @return the lease, or null when no host could read it + */ + private ClvmLockTransferAnswer readLease(String lvPath, String volumeUuid, Long dbHostId, List hosts, LockLookup lookup) { + for (Long hostId : selectLeaseReaders(dbHostId, hosts, lookup)) { + try { + Answer answer = _agentMgr.send(hostId, new ClvmLockTransferCommand( + ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER, lvPath, volumeUuid)); + if (answer instanceof ClvmLockTransferAnswer && answer.getResult() + && ((ClvmLockTransferAnswer) answer).getLeaseMode() != null) { + return (ClvmLockTransferAnswer) answer; + } + logger.debug("Host {} could not read the sanlock lease of volume {}: {}", + hostId, volumeUuid, answer != null ? answer.getDetails() : "null answer"); + } catch (AgentUnavailableException | OperationTimedoutException e) { + logger.debug("Could not query host {} for the sanlock lease of volume {}: {}", hostId, volumeUuid, e.getMessage()); + lookup.unreachableHostIds.add(hostId); } } + return null; + } - return lockHostId; + /** Up hosts that can read the lease, the preferred host and then the DB host first. */ + private List selectLeaseReaders(Long dbHostId, List hosts, LockLookup lookup) { + List hostIds = hosts.stream() + .filter(this::isUpKvmRoutingHost) + .map(HostVO::getId) + .filter(id -> !lookup.isUnreachable(id)) + .collect(Collectors.toList()); + Collections.shuffle(hostIds); + for (Long firstHostId : Arrays.asList(dbHostId, lookup.preferredHostId)) { + if (firstHostId != null && hostIds.remove(firstHostId)) { + hostIds.add(0, firstHostId); + } + } + return hostIds.subList(0, Math.min(MAX_LEASE_READERS, hostIds.size())); + } + + /** + * Whether the host named by a live lease still uses the volume. A host that restarted keeps its old lease until it + * rejoins the lockspace or the lease expires, and the LV is then not active there. When the host cannot be asked, + * a lease that is still renewed means the host is alive, otherwise it is down and its lease will expire. + */ + private boolean isLeaseHolderInUse(Long lockHostId, Long dbHostId, Boolean dbHostActive, boolean ownerRenewing, + String lvPath, String volumeUuid, LockLookup lookup) { + Boolean active; + if (lockHostId.equals(dbHostId)) { + active = dbHostActive; + } else { + active = isUpKvmRoutingHost(_hostDao.findById(lockHostId)) ? querySingleHostLockState(lockHostId, lvPath, volumeUuid, lookup) : null; + } + if (Boolean.TRUE.equals(active)) { + return true; + } + if (Boolean.FALSE.equals(active)) { + logger.info("Volume {} is not active on host {} although its sanlock lease names it, the host restarted; treating the lease as free", + volumeUuid, lockHostId); + return false; + } + if (ownerRenewing) { + logger.warn("Host {} holding volume {} cannot be reached but still renews its sanlock lease; it is alive, so its lock is kept", + lockHostId, volumeUuid); + return true; + } + logger.info("Host {} holding volume {} cannot be reached and no longer renews its sanlock lease; activating elsewhere once the lease expires", + lockHostId, volumeUuid); + return false; + } + + /** A lease that is not held, or held by a host that died or restarted since, is free to take. */ + private boolean isLeaseFree(ClvmLockTransferAnswer lease, String volumeUuid) { + if (lease.getLeaseMode() == ClvmLockTransferAnswer.LeaseMode.FREE) { + logger.debug("Volume {} is not held by any host (sanlock lease)", volumeUuid); + return true; + } + if (lease.getLeaseMode() == ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE && !lease.isLeaseOwnerAlive()) { + logger.info("Volume {} is held by sanlock host {}, which is dead or restarted since; treating the lease as free", + volumeUuid, lease.getLeaseOwnerId()); + return true; + } + return false; + } + + private List getLeaseHolderIds(ClvmLockTransferAnswer lease) { + if (lease.getLeaseMode() == ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE) { + return List.of(lease.getLeaseOwnerId()); + } + return lease.getSharedOwnerIds(); + } + + /** @return the only host of the pool reporting this sanlock host id, or null when none or several do */ + private Long findHostBySanlockId(List hosts, Long sanlockId) { + List hostIds = findHostIdsBySanlockId(hosts, String.valueOf(sanlockId)); + if (hostIds.size() == 1) { + return hostIds.get(0); + } + logger.warn("Sanlock host id {} matches hosts {} of the pool, expected exactly one. Querying all hosts instead.", + sanlockId, hostIds); + return null; + } + + private List findHostIdsBySanlockId(List hosts, String sanlockId) { + return hosts.stream() + .filter(h -> h.getType() == Host.Type.Routing && h.getHypervisorType() == Hypervisor.HypervisorType.KVM) + .map(HostVO::getId) + .filter(id -> sanlockId.equals(getSanlockId(id))) + .collect(Collectors.toList()); + } + + private String getSanlockId(long hostId) { + DetailVO detail = _hostDetailsDao.findDetail(hostId, Host.HOST_SANLOCK_ID); + return detail != null ? detail.getValue() : null; + } + + /** + * Checks that the host's sanlock host id identifies it among the hosts of the pool, which the lease based + * lock holder lookup relies on. + * + * @return the other hosts of the pool reporting the same sanlock host id + */ + public List findHostsSharingSanlockId(StoragePool pool, long hostId) { + String sanlockId = getSanlockId(hostId); + if (sanlockId == null) { + logger.warn("Host {} reports no {}, so CLVM lock holder lookups in pool {} query every host", + hostId, Host.HOST_SANLOCK_ID, pool.getName()); + return Collections.emptyList(); + } + List hostIds = new ArrayList<>(findHostIdsBySanlockId(findPoolHosts(pool), sanlockId)); + hostIds.remove(Long.valueOf(hostId)); + return hostIds; + } + + private boolean isUpKvmRoutingHost(HostVO host) { + return host != null + && host.getStatus() == Status.Up + && host.getType() == Host.Type.Routing + && host.getHypervisorType() == Hypervisor.HypervisorType.KVM; + } + + private List findPoolHosts(StoragePool pool) { + List hosts = null; + if (pool.getClusterId() != null) { + hosts = _hostDao.findByClusterId(pool.getClusterId(), Host.Type.Routing); + } else if (pool.getDataCenterId() > 0) { + hosts = _hostDao.findByDataCenterId(pool.getDataCenterId()); + } + return hosts != null ? hosts : Collections.emptyList(); + } + + /** + * Attempts an exclusive activation on the best available host when no host holds the volume. + * Removes the stale DB record if that fails. + */ + private Long recoverInactiveVolume(Long volumeId, String volumeUuid, String lvPath, Long dbHostId, List hosts, + Set noAnswerHostIds, boolean updateDatabase, LockLookup lookup) { + Set skipHostIds = new HashSet<>(noAnswerHostIds); + skipHostIds.addAll(lookup.unreachableHostIds); + Long targetHostId = selectActivationTargetHost(lookup.preferredHostId, dbHostId, hosts, skipHostIds); + if (targetHostId != null) { + Long recoveredHostId = tryActivateExclusivelyOnHost(volumeId, volumeUuid, lvPath, + targetHostId, updateDatabase, lookup, TRANSIENT_LOCK_REFUSALS); + if (recoveredHostId != null) { + return recoveredHostId; + } + } + + if (updateDatabase && dbHostId != null) { + VolumeDetailVO detail = _volsDetailsDao.findDetail(volumeId, CLVM_LOCK_HOST_ID); + if (detail != null && String.valueOf(dbHostId).equals(detail.getValue())) { + _volsDetailsDao.remove(detail.getId()); + } + } + return null; + } + + /** + * Queries the hosts in parallel, so one slow host does not delay the others. + * + * @return lock state per host that answered; hosts that did not answer are absent + */ + private Map queryHostsLockState(List hostIds, String lvPath, String volumeUuid, LockLookup lookup) { + Map states = new ConcurrentHashMap<>(); + List> futures = new ArrayList<>(); + for (Long hostId : hostIds) { + futures.add(lockQueryExecutor.submit(new ManagedContextRunnable() { + @Override + protected void runInContext() { + Boolean active = querySingleHostLockState(hostId, lvPath, volumeUuid, lookup); + if (active != null) { + states.put(hostId, active); + } + } + })); + } + for (Future future : futures) { + try { + future.get(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + break; + } catch (ExecutionException e) { + logger.warn("Lock state query for volume {} failed: {}", volumeUuid, e.getMessage()); + } + } + return new HashMap<>(states); } /** * Queries a single host for the CLVM LV activation state. + * A host that cannot be reached is remembered in {@code lookup}. * * @return {@code Boolean.TRUE} if the LV is active on that host, * {@code Boolean.FALSE} if reachable but inactive, * {@code null} if the host is unreachable or returned an error */ - private Boolean querySingleHostLockState(Long hostId, String lvPath, String volumeUuid) { + private Boolean querySingleHostLockState(Long hostId, String lvPath, String volumeUuid, LockLookup lookup) { try { ClvmLockTransferCommand queryCmd = new ClvmLockTransferCommand( ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE, lvPath, volumeUuid); @@ -325,6 +595,7 @@ private Boolean querySingleHostLockState(Long hostId, String lvPath, String volu return queryAnswer.isActive(); } catch (AgentUnavailableException | OperationTimedoutException e) { logger.debug("Could not query host {} for lock state: {}", hostId, e.getMessage()); + lookup.unreachableHostIds.add(hostId); return null; } } @@ -332,6 +603,8 @@ private Boolean querySingleHostLockState(Long hostId, String lvPath, String volu /** * Selects the best host on which to exclusively activate an inactive CLVM volume. * + *

Priority 0: the preferred host of the lookup, e.g. the host a VM starts on. + * *

Priority 1: the last known lock holder ({@code clvmLockHostId} from DB), if that host * is UP and KVM. * @@ -339,26 +612,27 @@ private Boolean querySingleHostLockState(Long hostId, String lvPath, String volu * *

Hosts that did not answer the lock-state query are skipped in both cases. * + * @param preferredHostId host to use first, may be null * @param dbHostId last known lock holder host ID from the DB (may be null) * @param hosts routing hosts in the cluster or zone collected during fan-out - * @param unreachableHostIds hosts that failed to answer the lock-state query + * @param unreachableHostIds hosts that failed to answer the lock-state query or are unreachable * @return host ID to activate on, or null if no eligible host found */ - private Long selectActivationTargetHost(Long dbHostId, List hosts, Set unreachableHostIds) { + private Long selectActivationTargetHost(Long preferredHostId, Long dbHostId, List hosts, Set unreachableHostIds) { + if (preferredHostId != null && !unreachableHostIds.contains(preferredHostId) + && isUpKvmRoutingHost(_hostDao.findById(preferredHostId))) { + logger.debug("selectActivationTargetHost: using preferred host {}", preferredHostId); + return preferredHostId; + } if (dbHostId != null && !unreachableHostIds.contains(dbHostId)) { - HostVO dbHost = _hostDao.findById(dbHostId); - if (dbHost != null && dbHost.getStatus() == Status.Up - && dbHost.getHypervisorType() == Hypervisor.HypervisorType.KVM) { + if (isUpKvmRoutingHost(_hostDao.findById(dbHostId))) { logger.debug("selectActivationTargetHost: preferring DB host {} (last known lock holder)", dbHostId); return dbHostId; } } if (hosts != null) { List eligible = hosts.stream() - .filter(h -> h.getStatus() == Status.Up - && h.getType() == Host.Type.Routing - && h.getHypervisorType() == Hypervisor.HypervisorType.KVM - && !unreachableHostIds.contains(h.getId())) + .filter(h -> isUpKvmRoutingHost(h) && !unreachableHostIds.contains(h.getId())) .collect(Collectors.toList()); if (!eligible.isEmpty()) { Collections.shuffle(eligible); @@ -377,7 +651,7 @@ private Long selectActivationTargetHost(Long dbHostId, List hosts, SetA temporarily refused activation (lease not expired yet, lockspace still starting) is retried - * on the same host until {@code deadlineMs}. Other failures are not retried. + * on the same host until the deadline of {@code lookup}. Other failures are not retried. * *

Activation failure is non-fatal: returns null so the caller can apply its own fallback. * @@ -386,11 +660,12 @@ private Long selectActivationTargetHost(Long dbHostId, List hosts, Set retryableRefusals) { long retryIntervalMs = getRetryIntervalMillis(); while (true) { try { @@ -398,24 +673,24 @@ private Long tryActivateExclusivelyOnHost(Long volumeId, String volumeUuid, Stri ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE, lvPath, volumeUuid); Answer activateAnswer = _agentMgr.send(targetHostId, activateCmd); if (activateAnswer != null && activateAnswer.getResult()) { - logger.info("Recovery: exclusively activated volume {} on host {} (was inactive on all hosts)", - volumeUuid, targetHostId); + logger.info("Exclusively activated volume {} on host {}", volumeUuid, targetHostId); if (updateDatabase) { setClvmLockHostId(volumeId, targetHostId); } return targetHostId; } - logger.warn("Recovery activation of volume {} on host {} failed: {}", + logger.warn("Exclusive activation of volume {} on host {} failed: {}", volumeUuid, targetHostId, activateAnswer != null ? activateAnswer.getDetails() : "null answer"); - if (!isRetryableRefusal(activateAnswer, retryIntervalMs, deadlineMs)) { + if (!isRetryableRefusal(activateAnswer, retryableRefusals, retryIntervalMs, lookup.activationDeadlineMs)) { return null; } logger.info("Activation of volume {} is refused temporarily, retrying on host {} in {} ms until the previous holder's lease expires or the lockspace is started", volumeUuid, targetHostId, retryIntervalMs); } catch (AgentUnavailableException | OperationTimedoutException e) { - logger.warn("Recovery activation of volume {} on host {} threw exception: {}", + logger.warn("Exclusive activation of volume {} on host {} threw exception: {}", volumeUuid, targetHostId, e.getMessage()); + lookup.unreachableHostIds.add(targetHostId); return null; } if (!sleep(retryIntervalMs)) { @@ -424,11 +699,11 @@ private Long tryActivateExclusivelyOnHost(Long volumeId, String volumeUuid, Stri } } - private boolean isRetryableRefusal(Answer activateAnswer, long retryIntervalMs, long deadlineMs) { + private boolean isRetryableRefusal(Answer activateAnswer, List retryableRefusals, long retryIntervalMs, long deadlineMs) { return retryIntervalMs > 0 && activateAnswer != null && activateAnswer.getDetails() != null - && TRANSIENT_LOCK_REFUSALS.stream().anyMatch(activateAnswer.getDetails()::contains) + && retryableRefusals.stream().anyMatch(activateAnswer.getDetails()::contains) && now() + retryIntervalMs <= deadlineMs; } @@ -530,23 +805,13 @@ public boolean transferClvmVolumeLock(String volumeUuid, Long volumeId, String v logger.debug("Volume {} has no active lock holder, will directly activate on destination", volumeUuid); } - ClvmLockTransferCommand activateCmd = new ClvmLockTransferCommand( - ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE, - lvPath, - volumeUuid - ); - - Answer activateAnswer = _agentMgr.send(destHostId, activateCmd); - - if (activateAnswer == null || !activateAnswer.getResult()) { - String error = activateAnswer != null ? activateAnswer.getDetails() : "null answer"; - logger.error("Failed to activate CLVM volume {} exclusively on dest host {}: {}", - volumeUuid, destHostId, error); + // A live source refuses with "locked by other host": fail fast. Only wait for the destination's lockspace to start. + if (tryActivateExclusivelyOnHost(volumeId, volumeUuid, lvPath, destHostId, true, + newLockLookup(destHostId), List.of(LOCK_START_IN_PROGRESS)) == null) { + logger.error("Failed to activate CLVM volume {} exclusively on dest host {}", volumeUuid, destHostId); return false; } - setClvmLockHostId(volumeId, destHostId); - logger.info("Successfully transferred CLVM lock for volume {} from host {} to host {}", volumeUuid, sourceHostId != null ? sourceHostId : "none", destHostId); diff --git a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java index 6888e801676f..f813643859c0 100644 --- a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java +++ b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java @@ -25,7 +25,9 @@ import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.Status; +import com.cloud.host.DetailVO; import com.cloud.host.dao.HostDao; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.clvm.ClvmPoolManager; import com.cloud.storage.dao.VolumeDetailsDao; @@ -46,6 +48,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import static org.mockito.ArgumentMatchers.any; @@ -69,6 +74,9 @@ public class ClvmPoolManagerTest { @Mock private HostDao hostDao; + @Mock + private HostDetailsDao hostDetailsDao; + @InjectMocks private ClvmPoolManager clvmPoolManager; @@ -81,7 +89,7 @@ public class ClvmPoolManagerTest { @Before public void setUp() { - Mockito.reset(volsDetailsDao, agentMgr, hostDao); + Mockito.reset(volsDetailsDao, agentMgr, hostDao, hostDetailsDao); } @Test @@ -305,7 +313,7 @@ public void testQueryCurrentLockHolder_SuccessfulQuery() throws AgentUnavailable Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); Assert.assertEquals(HOST_ID_1, result); - verify(agentMgr, times(1)).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); verify(hostDao, never()).findByName(any()); } @@ -484,7 +492,6 @@ public void testQueryCurrentLockHolder_UpdateDatabase_MatchingValue() throws Age // Fast path succeeded - no DB write needed, no fan-out verify(volsDetailsDao, never()).update(anyLong(), any()); verify(volsDetailsDao, never()).addDetail(anyLong(), any(), any(), Mockito.anyBoolean()); - verify(hostDao, never()).findByClusterId(anyLong(), any()); } @Test @@ -601,8 +608,8 @@ public void testQueryCurrentLockHolder_SkipsNonKVMHosts() throws AgentUnavailabl Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); Assert.assertEquals(HOST_ID_1, result); - verify(agentMgr, never()).send(eq(10L), any(ClvmLockTransferCommand.class)); - verify(agentMgr, times(1)).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); + verify(agentMgr, never()).send(eq(10L), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); } @Test @@ -624,8 +631,8 @@ public void testQueryCurrentLockHolder_SkipsDownHosts() throws AgentUnavailableE Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); Assert.assertEquals(HOST_ID_1, result); - verify(agentMgr, never()).send(eq(10L), any(ClvmLockTransferCommand.class)); - verify(agentMgr, times(1)).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); + verify(agentMgr, never()).send(eq(10L), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); } @Test @@ -670,10 +677,8 @@ public void testQueryCurrentLockHolder_FastPath_HitOnDbHost() throws AgentUnavai Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); Assert.assertEquals(HOST_ID_1, result); - // Only one agent call, no cluster host lookup, no fan-out - verify(agentMgr, times(1)).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); - verify(hostDao, never()).findByClusterId(anyLong(), any()); - verify(hostDao, never()).findByDataCenterId(anyLong()); + // Lease unreadable: one lock-state query on the DB host, no fan-out + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); } /** @@ -706,8 +711,8 @@ public void testQueryCurrentLockHolder_FastPath_MissDbHost_FanOutFindsOther() th Assert.assertEquals(HOST_ID_2, result); // HOST_ID_1 queried once (fast path only), HOST_ID_2 queried once (fan-out) - verify(agentMgr, times(1)).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); - verify(agentMgr, times(1)).send(eq(HOST_ID_2), any(ClvmLockTransferCommand.class)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); } /** @@ -735,8 +740,8 @@ public void testQueryCurrentLockHolder_FastPath_DbHostDown_FanOut() throws Agent Assert.assertEquals(HOST_ID_2, result); // No query to the down host at all - verify(agentMgr, never()).send(eq(HOST_ID_1), any(ClvmLockTransferCommand.class)); - verify(agentMgr, times(1)).send(eq(HOST_ID_2), any(ClvmLockTransferCommand.class)); + verify(agentMgr, never()).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); } @@ -1037,6 +1042,7 @@ private RetryTestClvmPoolManager newRetryManager() { ReflectionTestUtils.setField(manager, "_volsDetailsDao", volsDetailsDao); ReflectionTestUtils.setField(manager, "_agentMgr", agentMgr); ReflectionTestUtils.setField(manager, "_hostDao", hostDao); + ReflectionTestUtils.setField(manager, "_hostDetailsDao", hostDetailsDao); return manager; } @@ -1192,10 +1198,10 @@ public void testRecoveryActivation_SharedDeadline_SecondVolumeNotRetriedOnceBudg StoragePoolVO pool = prepareRecoveryOnHost2(); when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) .thenReturn(lockedByOtherHost()); - long deadlineMs = manager.newLockRecoveryDeadlineMs(); + ClvmPoolManager.LockLookup lookup = manager.newLockLookup(); - Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, deadlineMs)); - Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, deadlineMs)); + Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, lookup)); + Assert.assertNull(manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, lookup)); Assert.assertEquals(18, manager.sleeps.size()); Assert.assertEquals(180_000L, manager.clock.get()); @@ -1208,15 +1214,301 @@ public void testRecoveryActivation_SharedDeadline_SecondVolumeActivatedRightAfte StoragePoolVO pool = prepareRecoveryOnHost2(); when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) .thenReturn(lockedByOtherHost(), lockedByOtherHost(), new Answer(null, true, null), new Answer(null, true, null)); - long deadlineMs = manager.newLockRecoveryDeadlineMs(); + ClvmPoolManager.LockLookup lookup = manager.newLockLookup(); - Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, deadlineMs)); - Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, deadlineMs)); + Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, lookup)); + Assert.assertEquals(HOST_ID_2, manager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, lookup)); Assert.assertEquals(Arrays.asList(10_000L, 10_000L), manager.sleeps); verify(agentMgr, times(4)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); } + @Test + public void testFanOut_UnreachableHostQueriedOnceForAllVolumesOfOneLookup() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareRecoveryOnHost2(); + Long host3 = 3L; + HostVO downHost = hostDao.findById(HOST_ID_1); + HostVO upHost2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO upHost3 = createMockHost(host3, "host3", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(downHost, upHost2, upHost3)); + when(agentMgr.send(eq(host3), any(ClvmLockTransferCommand.class))) + .thenThrow(new OperationTimedoutException(null, host3, 0, 0, false)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + ClvmPoolManager.LockLookup lookup = clvmPoolManager.newLockLookup(); + + Assert.assertEquals(HOST_ID_2, clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, lookup)); + Assert.assertEquals(HOST_ID_2, clvmPoolManager.getClvmLockHostId(VOLUME_ID + 1, "second-volume-uuid", "second-volume-path", pool, true, lookup)); + + verify(agentMgr, times(1)).send(eq(host3), any(ClvmLockTransferCommand.class)); + verify(agentMgr, times(2)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testFanOut_HostsQueriedInParallel() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = mockClusterPool(); + when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(null); + HostVO upHost1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO upHost2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(upHost1, upHost2)); + + // Each query answers only once both queries are in flight + CountDownLatch bothInFlight = new CountDownLatch(2); + AtomicBoolean sequential = new AtomicBoolean(); + when(agentMgr.send(anyLong(), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenAnswer(invocation -> { + bothInFlight.countDown(); + if (!bothInFlight.await(5, TimeUnit.SECONDS)) { + sequential.set(true); + } + boolean active = HOST_ID_2.equals(invocation.getArgument(0)); + return new ClvmLockTransferAnswer(null, true, null, null, active, active, null); + }); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertFalse("hosts were queried one after another", sequential.get()); + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, never()).send(anyLong(), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + /** Two Up hosts in the cluster reporting sanlock ids 1 and 2; DB says HOST_ID_1 holds the lock. */ + private StoragePoolVO prepareLeaseLookup() { + StoragePoolVO pool = mockClusterPool(); + VolumeDetailVO detail = new VolumeDetailVO(); + detail.setValue(String.valueOf(HOST_ID_1)); + lenient().when(volsDetailsDao.findDetail(VOLUME_ID, ClvmPoolManager.CLVM_LOCK_HOST_ID)).thenReturn(detail); + HostVO host1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO host2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + lenient().when(hostDao.findById(HOST_ID_1)).thenReturn(host1); + lenient().when(hostDao.findById(HOST_ID_2)).thenReturn(host2); + when(hostDao.findByClusterId(10L, Host.Type.Routing)).thenReturn(Arrays.asList(host1, host2)); + stubSanlockId(HOST_ID_1, "1"); + stubSanlockId(HOST_ID_2, "2"); + stubLockState(HOST_ID_1, false); + return pool; + } + + private void stubLockState(Long hostId, boolean active) { + try { + lenient().when(agentMgr.send(eq(hostId), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, active, active, null)); + } catch (AgentUnavailableException | OperationTimedoutException e) { + throw new IllegalStateException(e); + } + } + + private void stubSanlockId(Long hostId, String sanlockId) { + lenient().when(hostDetailsDao.findDetail(hostId, Host.HOST_SANLOCK_ID)).thenReturn(new DetailVO(hostId, Host.HOST_SANLOCK_ID, sanlockId)); + } + + private void stubLease(ClvmLockTransferAnswer lease) throws AgentUnavailableException, OperationTimedoutException { + when(agentMgr.send(anyLong(), operation(ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER))).thenReturn(lease); + } + + @Test + public void testLease_ExclusiveOwnerMappedToHost_NoFanOut() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 2L, true, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, true, true, null)); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(anyLong(), operation(ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(volsDetailsDao).update(anyLong(), any(VolumeDetailVO.class)); + } + + @Test + public void testLease_HolderRestarted_LvInactiveThere_ActivatedOnPreferredHost() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 2L, true, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_2)); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testLease_Free_ActivatedOnPreferredHost() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.FREE, null, false, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_2)); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER)); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, never()).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, never()).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testLease_OwnerDead_TreatedAsFree() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 1L, false, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_2)); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + verify(agentMgr, never()).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testLease_SanlockIdOnTwoHosts_FallsBackToFanOut() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubSanlockId(HOST_ID_1, "2"); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 2L, true, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, false, false, null)); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenReturn(new ClvmLockTransferAnswer(null, true, null, null, true, true, null)); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testLease_UnreadableOnTwoHosts_FallsBackToFanOut() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(new ClvmLockTransferAnswer(null, false, "No sanlock lock args")); + stubLockState(HOST_ID_2, true); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(2)).send(anyLong(), operation(ClvmLockTransferCommand.Operation.QUERY_LEASE_OWNER)); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testLease_NamesDbHostFoundInactive_NoSecondQuery() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 1L, true, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_2)); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, times(1)).send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE)); + } + + @Test + public void testLease_HolderUnreachableNotRenewing_ActivatedOnPreferredHost() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 2L, true, false, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenThrow(new OperationTimedoutException(null, HOST_ID_2, 0, 0, false)); + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(new Answer(null, true, null)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_1)); + + Assert.assertEquals(HOST_ID_1, result); + verify(agentMgr, never()).send(anyLong(), operation(ClvmLockTransferCommand.Operation.DEACTIVATE)); + } + + @Test + public void testLease_HolderUnreachableButRenewing_KeptAsHolder() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.EXCLUSIVE, 2L, true, true, Collections.emptyList())); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.QUERY_LOCK_STATE))) + .thenThrow(new OperationTimedoutException(null, HOST_ID_2, 0, 0, false)); + + Long result = clvmPoolManager.getClvmLockHostId(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true, + clvmPoolManager.newLockLookup(HOST_ID_1)); + + Assert.assertEquals(HOST_ID_2, result); + verify(agentMgr, never()).send(anyLong(), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + /** Source HOST_ID_1 is Up and deactivates; the destination is HOST_ID_2. */ + private StoragePoolVO prepareTransfer() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + when(pool.getPath()).thenReturn(VG_NAME); + HostVO source = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findById(HOST_ID_1)).thenReturn(source); + when(agentMgr.send(eq(HOST_ID_1), operation(ClvmLockTransferCommand.Operation.DEACTIVATE))) + .thenReturn(new Answer(null, true, null)); + return pool; + } + + @Test + public void testTransfer_DestinationLockStartInProgress_Retried() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareTransfer(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockStartInProgress(), new Answer(null, true, null)); + + Assert.assertTrue(manager.transferClvmVolumeLock(VOLUME_UUID, VOLUME_ID, VOLUME_PATH, pool, HOST_ID_1, HOST_ID_2)); + + Assert.assertEquals(Collections.singletonList(10_000L), manager.sleeps); + verify(agentMgr, times(2)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testTransfer_LockedByOtherHost_FailsFast() throws AgentUnavailableException, OperationTimedoutException { + RetryTestClvmPoolManager manager = newRetryManager(); + StoragePoolVO pool = prepareTransfer(); + when(agentMgr.send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE))) + .thenReturn(lockedByOtherHost()); + + Assert.assertFalse(manager.transferClvmVolumeLock(VOLUME_UUID, VOLUME_ID, VOLUME_PATH, pool, HOST_ID_1, HOST_ID_2)); + + Assert.assertTrue(manager.sleeps.isEmpty()); + verify(agentMgr, times(1)).send(eq(HOST_ID_2), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testFastPathHit_NoLeaseRead() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLockState(HOST_ID_1, true); + + Long result = clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, true); + + Assert.assertEquals(HOST_ID_1, result); + verify(agentMgr, times(1)).send(anyLong(), any(ClvmLockTransferCommand.class)); + } + + @Test + public void testLease_SharedByTwoHosts_NoTransfer() throws AgentUnavailableException, OperationTimedoutException { + StoragePoolVO pool = prepareLeaseLookup(); + stubLease(ClvmLockTransferAnswer.leaseOwner(null, ClvmLockTransferAnswer.LeaseMode.SHARED, null, false, Arrays.asList(1L, 2L))); + + Assert.assertNull(clvmPoolManager.queryCurrentLockHolder(VOLUME_ID, VOLUME_UUID, VOLUME_PATH, pool, false)); + verify(agentMgr, never()).send(anyLong(), operation(ClvmLockTransferCommand.Operation.ACTIVATE_EXCLUSIVE)); + } + + @Test + public void testFindHostsSharingSanlockId() { + StoragePoolVO pool = prepareLeaseLookup(); + Assert.assertTrue(clvmPoolManager.findHostsSharingSanlockId(pool, HOST_ID_1).isEmpty()); + + stubSanlockId(HOST_ID_2, "1"); + Assert.assertEquals(Collections.singletonList(HOST_ID_2), clvmPoolManager.findHostsSharingSanlockId(pool, HOST_ID_1)); + } + private StoragePoolVO mockClusterPool() { StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); when(pool.getClusterId()).thenReturn(10L); From b9ef8b162c98c13f631541dee74df2bfd7752b64 Mon Sep 17 00:00:00 2001 From: Pearl1594 Date: Mon, 9 Nov 2026 08:15:56 -0500 Subject: [PATCH 3/4] CLVM: fix template LV removal and protect templates used as backing file - Treat a template as in use when any volume on any CLVM pool of the same VG uses it, so a CLVM_NG backing file is never removed under a volume - Deactivate the template LV on all hosts of the pools sharing the VG before removing it, releasing stale shared locks ("LV locked by other host") - Log hosts that cannot deactivate it by name, uuid and state, with the lvchange command to run there - Keep the LV when another pool on the same VG still references it - DestroyCommand reports a failed delete as failure, so the template pool record is kept and eviction retried instead of leaking the LV - CLVM cleanup treats only "not found" as already deleted; lvs errors and exceptions now fail the delete - Add clvm.lock.query.threads (default 5, restart required) for the parallel host calls, replacing the hardcoded 16 --- .../wrapper/LibvirtDestroyCommandWrapper.java | 4 +- .../kvm/storage/ClvmStorageAdaptor.java | 16 ++- .../LibvirtComputingResourceTest.java | 17 +++ .../kvm/storage/ClvmStorageAdaptorTest.java | 33 +++++ .../cloud/storage/clvm/ClvmPoolManager.java | 133 ++++++++++++++---- .../cloud/template/TemplateManagerImpl.java | 38 ++++- .../cloud/storage/ClvmPoolManagerTest.java | 50 +++++++ .../template/TemplateManagerImplTest.java | 100 +++++++++++++ 8 files changed, 356 insertions(+), 35 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDestroyCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDestroyCommandWrapper.java index b9106cec42f6..bec03af6e6d7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDestroyCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDestroyCommandWrapper.java @@ -40,7 +40,9 @@ public Answer execute(final DestroyCommand command, final LibvirtComputingResour try { final KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr(); final KVMStoragePool pool = storagePoolMgr.getStoragePool(vol.getPoolType(), vol.getPoolUuid()); - pool.deletePhysicalDisk(vol.getPath(), null); + if (!pool.deletePhysicalDisk(vol.getPath(), null)) { + return new Answer(command, false, "Failed to delete " + vol.getPath()); + } return new Answer(command, true, "Success"); } catch (final CloudRuntimeException e) { logger.debug("Failed to delete volume: " + e.toString()); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java index fe2f10bdcdf1..b77fc87be770 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptor.java @@ -962,8 +962,12 @@ private boolean cleanupCLVMVolume(String uuid, KVMStoragePool pool) { String checkResult = checkLvs.execute(); if (checkResult != null) { - logger.info("CLVM volume {} does not exist in LVM (check returned: {}), considering it as already deleted", uuid, checkResult); - return true; + if (isLvNotFound(checkResult)) { + logger.info("CLVM volume {} does not exist in LVM (check returned: {}), considering it as already deleted", uuid, checkResult); + return true; + } + logger.warn("Cannot check CLVM volume {}: {}", uuid, checkResult); + return false; } logger.info("Volume {} exists, proceeding with cleanup", uuid); @@ -990,7 +994,7 @@ private boolean cleanupCLVMVolume(String uuid, KVMStoragePool pool) { return true; } else { logger.warn("Command 'lvremove -f {}' returned error: {}", lvPath, removeResult); - if (removeResult.contains("not found") || removeResult.contains("Failed to find")) { + if (isLvNotFound(removeResult)) { logger.info("CLVM volume {} not found during cleanup, considering it as already deleted", uuid); return true; } @@ -998,10 +1002,14 @@ private boolean cleanupCLVMVolume(String uuid, KVMStoragePool pool) { } } catch (Exception ex) { logger.error("Exception during CLVM volume cleanup for {}: {}", uuid, ex.getMessage(), ex); - return true; + return false; } } + private boolean isLvNotFound(String lvmError) { + return lvmError.contains("not found") || lvmError.contains("Failed to find"); + } + private boolean shouldSecureZeroFill(KVMStoragePool pool) { Map details = pool.getDetails(); String secureZeroFillStr = (details != null) ? details.get(KVMStoragePool.CLVM_SECURE_ZERO_FILL) : null; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 619aab27010e..b09067674dff 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -2269,6 +2269,7 @@ public void testDestroyCommand() { when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(poolManager); when(poolManager.getStoragePool(vol.getPoolType(), vol.getPoolUuid())).thenReturn(primary); + when(primary.deletePhysicalDisk(vol.getPath(), null)).thenReturn(true); final LibvirtRequestWrapper wrapper = LibvirtRequestWrapper.getInstance(); assertNotNull(wrapper); @@ -2280,6 +2281,22 @@ public void testDestroyCommand() { verify(poolManager, times(1)).getStoragePool(vol.getPoolType(), vol.getPoolUuid()); } + @Test + public void testDestroyCommandDeleteFailed() { + final DestroyCommand command = new DestroyCommand(Mockito.mock(StoragePool.class), Mockito.mock(Volume.class), "Test"); + final KVMStoragePoolManager poolManager = Mockito.mock(KVMStoragePoolManager.class); + final KVMStoragePool primary = Mockito.mock(KVMStoragePool.class); + final VolumeTO vol = command.getVolume(); + + when(libvirtComputingResourceMock.getStoragePoolMgr()).thenReturn(poolManager); + when(poolManager.getStoragePool(vol.getPoolType(), vol.getPoolUuid())).thenReturn(primary); + when(primary.deletePhysicalDisk(vol.getPath(), null)).thenReturn(false); + + final Answer answer = LibvirtRequestWrapper.getInstance().execute(command, libvirtComputingResourceMock); + + assertFalse(answer.getResult()); + } + @SuppressWarnings("unchecked") @Test public void testDestroyCommandError() { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java index 1736c1d3f980..121f6b1ce4d9 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmStorageAdaptorTest.java @@ -36,6 +36,7 @@ import java.util.Map; import java.util.UUID; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -821,6 +822,38 @@ public void testCleanupCLVMVolume_VolumeNotFound() throws Exception { assert result : "Should return true when volume not found (already deleted)"; } + @Test + public void testCleanupCLVMVolume_LockedByOtherHost() throws Exception { + Mockito.when(mockPool.getLocalPath()).thenReturn("testvg"); + Mockito.when(mockPool.getUuid()).thenReturn(UUID.randomUUID().toString()); + + mockScriptConstruction = Mockito.mockConstruction(Script.class, (mock, context) -> { + boolean lvremove = "lvremove".equals(context.arguments().get(0)); + when(mock.execute()).thenReturn(lvremove ? " LV locked by other host: testvg/template-1" : null); + }); + + Method method = ClvmStorageAdaptor.class.getDeclaredMethod( + "cleanupCLVMVolume", String.class, KVMStoragePool.class); + method.setAccessible(true); + + Assert.assertFalse((boolean) method.invoke(clvmStorageAdaptor, "template-1", mockPool)); + } + + @Test + public void testCleanupCLVMVolume_CheckFailsForOtherReason() throws Exception { + Mockito.when(mockPool.getLocalPath()).thenReturn("testvg"); + Mockito.when(mockPool.getUuid()).thenReturn(UUID.randomUUID().toString()); + + mockScriptConstruction = Mockito.mockConstruction(Script.class, + (mock, context) -> when(mock.execute()).thenReturn(" Lock manager lvmlockd is not running")); + + Method method = ClvmStorageAdaptor.class.getDeclaredMethod( + "cleanupCLVMVolume", String.class, KVMStoragePool.class); + method.setAccessible(true); + + Assert.assertFalse((boolean) method.invoke(clvmStorageAdaptor, "template-1", mockPool)); + } + @Test public void testCleanupCLVMVolume_NullSourceDir() throws Exception { String volumeUuid = UUID.randomUUID().toString(); diff --git a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java index aee5f1a33170..3bc718ff88fa 100644 --- a/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java +++ b/server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java @@ -23,8 +23,10 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; @@ -33,6 +35,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.stream.Collectors; import javax.inject.Inject; @@ -58,6 +61,8 @@ import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.springframework.stereotype.Component; @@ -72,6 +77,8 @@ public class ClvmPoolManager implements Configurable { private HostDao _hostDao; @Inject private HostDetailsDao _hostDetailsDao; + @Inject + private PrimaryDataStoreDao _storagePoolDao; protected Logger logger = LogManager.getLogger(getClass()); @@ -93,10 +100,12 @@ public class ClvmPoolManager implements Configurable { public static final ConfigKey CLVMSecureZeroFill = new ConfigKey<>("Advanced", Boolean.class, "clvm.secure.zero.fill", "false", "When enabled, CLVM volumes to be zero-filled at the time of deletion to prevent data from being recovered by VMs reusing the space, as thick LVM volumes write data linearly. Note: This setting is propagated to hosts when they connect to the storage pool. Changing this setting requires disconnecting and reconnecting hosts or restarting the KVM agent for it to take effect.", false, ConfigKey.Scope.StoragePool); - private static final int LOCK_QUERY_THREADS = 16; + public static final ConfigKey ClvmLockQueryThreads = new ConfigKey<>("Advanced", Integer.class, "clvm.lock.query.threads", "5", + "Number of threads a management server uses to send CLVM lock commands to several hosts in parallel: the fallback lock holder lookup that asks every host, and the deactivation of a template LV on all hosts before it is removed. Requires a management server restart.", false); + private static final int MAX_LEASE_READERS = 2; - private final ExecutorService lockQueryExecutor = newLockQueryExecutor(); + private ExecutorService lockQueryExecutor; /** State shared by the lock lookups of all volumes of one operation. */ public static class LockLookup { @@ -114,17 +123,86 @@ boolean isUnreachable(Long hostId) { } } - private static ExecutorService newLockQueryExecutor() { - ThreadPoolExecutor executor = new ThreadPoolExecutor(LOCK_QUERY_THREADS, LOCK_QUERY_THREADS, 60L, TimeUnit.SECONDS, - new LinkedBlockingQueue<>(), new NamedThreadFactory("ClvmLockQuery")); - executor.allowCoreThreadTimeOut(true); - return executor; + /** Created on first use, as the config value is not available when the bean is constructed. */ + private synchronized ExecutorService getLockQueryExecutor() { + if (lockQueryExecutor == null) { + int threads = Math.max(1, ClvmLockQueryThreads.value()); + ThreadPoolExecutor executor = new ThreadPoolExecutor(threads, threads, 60L, TimeUnit.SECONDS, + new LinkedBlockingQueue<>(), new NamedThreadFactory("ClvmLockQuery")); + executor.allowCoreThreadTimeOut(true); + lockQueryExecutor = executor; + } + return lockQueryExecutor; } public static boolean isClvmPoolType(Storage.StoragePoolType poolType) { return Arrays.asList(Storage.StoragePoolType.CLVM, Storage.StoragePoolType.CLVM_NG).contains(poolType); } + private static String getVgName(StoragePool pool) { + String vgName = pool.getPath(); + return vgName.startsWith("/") ? vgName.substring(1) : vgName; + } + + private static String getLvPath(StoragePool pool, String lvName) { + return String.format("/dev/%s/%s", getVgName(pool), lvName); + } + + /** + * Pools of the same zone backed by the same VG share its LVs, e.g. one template LV used as backing file by volumes of all of them. + * + * @return ids of the pools sharing the VG of {@code pool}, including {@code pool} + */ + public Set findPoolIdsSharingVg(StoragePool pool) { + String vgName = getVgName(pool); + Set poolIds = _storagePoolDao.findPoolsByStorageTypeAndZone(pool.getPoolType(), pool.getDataCenterId()).stream() + .filter(p -> vgName.equals(getVgName(p))) + .map(StoragePoolVO::getId) + .collect(Collectors.toCollection(HashSet::new)); + poolIds.add(pool.getId()); + return poolIds; + } + + /** + * Deactivates an LV on every Up host of the pools sharing its VG, releasing their shared locks so it can be removed. + * Best effort: a host still using the LV refuses, and removing it then fails on the lock. + */ + public void deactivateOnAllHosts(StoragePool pool, String lvName) { + String lvPath = getLvPath(pool, lvName); + Map hosts = new LinkedHashMap<>(); + findPoolIdsSharingVg(pool).stream() + .map(_storagePoolDao::findById) + .filter(Objects::nonNull) + .flatMap(p -> findPoolHosts(p).stream()) + .filter(h -> h.getHypervisorType() == Hypervisor.HypervisorType.KVM) + .forEach(h -> hosts.putIfAbsent(h.getId(), h)); + List upHostIds = hosts.values().stream().filter(this::isUpKvmRoutingHost).map(HostVO::getId).collect(Collectors.toList()); + Map results = callOnHosts(upHostIds, lvName, hostId -> deactivateOnHost(hosts.get(hostId), lvPath, lvName)); + List notDeactivated = hosts.values().stream() + .filter(h -> !Boolean.TRUE.equals(results.get(h.getId()))) + .map(h -> String.format("%s (%s, %s)", h.getName(), h.getUuid(), h.getStatus())) + .collect(Collectors.toList()); + if (notDeactivated.isEmpty()) { + logger.info("Deactivated {} on all hosts {}", lvPath, hosts.values().stream().map(HostVO::getName).collect(Collectors.toList())); + return; + } + logger.warn("{} could not be deactivated on hosts {}. It cannot be removed while active there; deactivate it on those hosts with 'lvchange -an {}'.", + lvPath, notDeactivated, lvPath); + } + + private Boolean deactivateOnHost(HostVO host, String lvPath, String lvName) { + try { + Answer answer = _agentMgr.send(host.getId(), new ClvmLockTransferCommand(ClvmLockTransferCommand.Operation.DEACTIVATE, lvPath, lvName)); + if (answer != null && answer.getResult()) { + return true; + } + logger.warn("Failed to deactivate {} on host {}: {}", lvPath, host.getName(), answer != null ? answer.getDetails() : "null answer"); + } catch (AgentUnavailableException | OperationTimedoutException e) { + logger.warn("Could not deactivate {} on host {}: {}", lvPath, host.getName(), e.getMessage()); + } + return false; + } + /** * Gets the CLVM lock host ID for a volume, optionally querying actual LVM state. * @@ -234,11 +312,7 @@ public Long queryCurrentLockHolder(Long volumeId, String volumeUuid, String volu return null; } - String vgName = pool.getPath(); - if (vgName.startsWith("/")) { - vgName = vgName.substring(1); - } - String lvPath = String.format("/dev/%s/%s", vgName, volumePath); + String lvPath = getLvPath(pool, volumePath); Long dbHostId = getClvmLockHostId(volumeId, volumeUuid); // Hosts that did not answer the lock-state query for this volume Set noAnswerHostIds = new HashSet<>(); @@ -533,21 +607,26 @@ private Long recoverInactiveVolume(Long volumeId, String volumeUuid, String lvPa return null; } + /** @return lock state per host that answered; hosts that did not answer are absent */ + private Map queryHostsLockState(List hostIds, String lvPath, String volumeUuid, LockLookup lookup) { + return callOnHosts(hostIds, volumeUuid, hostId -> querySingleHostLockState(hostId, lvPath, volumeUuid, lookup)); + } + /** - * Queries the hosts in parallel, so one slow host does not delay the others. + * Calls the hosts in parallel, so one slow host does not delay the others. * - * @return lock state per host that answered; hosts that did not answer are absent + * @return result per host; hosts whose call returned null are absent */ - private Map queryHostsLockState(List hostIds, String lvPath, String volumeUuid, LockLookup lookup) { - Map states = new ConcurrentHashMap<>(); + private Map callOnHosts(List hostIds, String volumeUuid, Function call) { + Map results = new ConcurrentHashMap<>(); List> futures = new ArrayList<>(); for (Long hostId : hostIds) { - futures.add(lockQueryExecutor.submit(new ManagedContextRunnable() { + futures.add(getLockQueryExecutor().submit(new ManagedContextRunnable() { @Override protected void runInContext() { - Boolean active = querySingleHostLockState(hostId, lvPath, volumeUuid, lookup); - if (active != null) { - states.put(hostId, active); + T result = call.apply(hostId); + if (result != null) { + results.put(hostId, result); } } })); @@ -559,10 +638,10 @@ protected void runInContext() { Thread.currentThread().interrupt(); break; } catch (ExecutionException e) { - logger.warn("Lock state query for volume {} failed: {}", volumeUuid, e.getMessage()); + logger.warn("Host call for volume {} failed: {}", volumeUuid, e.getMessage()); } } - return new HashMap<>(states); + return new HashMap<>(results); } /** @@ -769,12 +848,7 @@ public boolean transferClvmVolumeLock(String volumeUuid, Long volumeId, String v return false; } - String vgName = pool.getPath(); - if (vgName.startsWith("/")) { - vgName = vgName.substring(1); - } - - String lvPath = String.format("/dev/%s/%s", vgName, volumePath); + String lvPath = getLvPath(pool, volumePath); try { // sourceHostId is trusted as pre-validated by the caller @@ -833,7 +907,8 @@ public ConfigKey[] getConfigKeys() { return new ConfigKey[] { CLVMSecureZeroFill, ClvmLockRecoveryRetryTimeout, - ClvmLockRecoveryRetryInterval + ClvmLockRecoveryRetryInterval, + ClvmLockQueryThreads }; } } diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 482d0b405d72..d18c7b43d7da 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -27,6 +27,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.UUID; import java.util.concurrent.ExecutionException; @@ -187,6 +188,7 @@ import com.cloud.storage.Volume; import com.cloud.storage.VolumeApiService; import com.cloud.storage.VolumeVO; +import com.cloud.storage.clvm.ClvmPoolManager; import com.cloud.storage.dao.GuestOSDao; import com.cloud.storage.dao.LaunchPermissionDao; import com.cloud.storage.dao.SnapshotDao; @@ -258,6 +260,8 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, @Inject private PrimaryDataStoreDao _poolDao; @Inject + private ClvmPoolManager clvmPoolManager; + @Inject private StoragePoolHostDao _poolHostDao; @Inject private AccountDao _accountDao; @@ -1237,12 +1241,37 @@ private Boolean templateIsUnusedInPool(VMTemplateStoragePoolVO templatePoolVO) { return false; } - if (template.getFormat() == ImageFormat.ISO || _volumeDao.isAnyVolumeActivelyUsingTemplateOnPool(template.getId(), templatePoolVO.getPoolId())) { + if (template.getFormat() == ImageFormat.ISO || isTemplateUsedByVolumes(template.getId(), templatePoolVO.getPoolId())) { return false; } return true; } + /** CLVM pools on the same VG share one template LV, used as backing file by CLVM_NG volumes of any of them. */ + private boolean isTemplateUsedByVolumes(long templateId, long poolId) { + return getPoolIdsSharingTemplate(poolId).stream() + .anyMatch(id -> _volumeDao.isAnyVolumeActivelyUsingTemplateOnPool(templateId, id)); + } + + private Set getPoolIdsSharingTemplate(long poolId) { + StoragePoolVO pool = _poolDao.findById(poolId); + if (pool == null || !ClvmPoolManager.isClvmPoolType(pool.getPoolType())) { + return Set.of(poolId); + } + return clvmPoolManager.findPoolIdsSharingVg(pool); + } + + /** @return true when another pool on the same VG still references the template LV, so only this pool's record may go */ + private boolean isTemplateLvReferencedByOtherPool(VMTemplateStoragePoolVO templatePoolVO) { + Set otherPoolIds = new HashSet<>(getPoolIdsSharingTemplate(templatePoolVO.getPoolId())); + otherPoolIds.remove(templatePoolVO.getPoolId()); + if (otherPoolIds.isEmpty()) { + return false; + } + return _tmpltPoolDao.listByTemplateId(templatePoolVO.getTemplateId(), new ArrayList<>(otherPoolIds)).stream() + .anyMatch(ref -> Objects.equals(ref.getInstallPath(), templatePoolVO.getInstallPath())); + } + @Override public List getUnusedTemplatesInPool(StoragePoolVO pool) { List unusedTemplatesInPool = new ArrayList(); @@ -1305,7 +1334,14 @@ public void evictTemplateFromStoragePool(VMTemplateStoragePoolVO templatePoolVO) logger.debug("Successfully evicted Template {} from storage pool {}", template, pool); } } + } else if (ClvmPoolManager.isClvmPoolType(pool.getPoolType()) && isTemplateLvReferencedByOtherPool(templatePoolVO)) { + if (_tmpltPoolDao.remove(templatePoolVO.getId())) { + logger.debug("Evicted Template {} from storage pool {}, keeping its LV for the other pools on the same VG", template, pool); + } } else { + if (ClvmPoolManager.isClvmPoolType(pool.getPoolType())) { + clvmPoolManager.deactivateOnAllHosts(pool, templatePoolVO.getInstallPath()); + } DestroyCommand cmd = new DestroyCommand(pool, templatePoolVO); Answer answer = _storageMgr.sendToPool(pool, cmd); diff --git a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java index f813643859c0..a6f3dd13b849 100644 --- a/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java +++ b/server/src/test/java/com/cloud/storage/ClvmPoolManagerTest.java @@ -33,11 +33,13 @@ import com.cloud.storage.dao.VolumeDetailsDao; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; @@ -48,6 +50,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -77,6 +80,9 @@ public class ClvmPoolManagerTest { @Mock private HostDetailsDao hostDetailsDao; + @Mock + private PrimaryDataStoreDao storagePoolDao; + @InjectMocks private ClvmPoolManager clvmPoolManager; @@ -1532,4 +1538,48 @@ private HostVO createMockHost(Long id, String name, Status status, Hypervisor.Hy Mockito.lenient().when(host.getHypervisorType()).thenReturn(hypervisor); return host; } + + private StoragePoolVO mockVgPool(long id, String path, long clusterId) { + StoragePoolVO pool = Mockito.mock(StoragePoolVO.class); + lenient().when(pool.getId()).thenReturn(id); + lenient().when(pool.getPath()).thenReturn(path); + lenient().when(pool.getClusterId()).thenReturn(clusterId); + lenient().when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.CLVM_NG); + lenient().when(pool.getDataCenterId()).thenReturn(1L); + return pool; + } + + @Test + public void testFindPoolIdsSharingVg_MatchesVgNameOnly() { + StoragePoolVO pool = mockVgPool(8L, "sharedvg", 1L); + List zonePools = List.of(pool, mockVgPool(9L, "/sharedvg", 2L), mockVgPool(4L, "sharedvg2", 2L)); + when(storagePoolDao.findPoolsByStorageTypeAndZone(Storage.StoragePoolType.CLVM_NG, 1L)).thenReturn(zonePools); + + Assert.assertEquals(Set.of(8L, 9L), clvmPoolManager.findPoolIdsSharingVg(pool)); + } + + @Test + public void testDeactivateOnAllHosts_UpHostsOfPoolsSharingVg() throws Exception { + StoragePoolVO pool = mockVgPool(8L, "sharedvg", 1L); + StoragePoolVO otherPool = mockVgPool(9L, "sharedvg", 2L); + when(storagePoolDao.findPoolsByStorageTypeAndZone(Storage.StoragePoolType.CLVM_NG, 1L)).thenReturn(List.of(pool, otherPool)); + when(storagePoolDao.findById(8L)).thenReturn(pool); + when(storagePoolDao.findById(9L)).thenReturn(otherPool); + HostVO host1 = createMockHost(HOST_ID_1, "host1", Status.Up, Hypervisor.HypervisorType.KVM); + HostVO downHost = createMockHost(3L, "host3", Status.Down, Hypervisor.HypervisorType.KVM); + HostVO host2 = createMockHost(HOST_ID_2, "host2", Status.Up, Hypervisor.HypervisorType.KVM); + when(hostDao.findByClusterId(1L, Host.Type.Routing)).thenReturn(List.of(host1, downHost)); + when(hostDao.findByClusterId(2L, Host.Type.Routing)).thenReturn(List.of(host2)); + Answer ok = new Answer(null, true, "ok"); + when(agentMgr.send(anyLong(), any(ClvmLockTransferCommand.class))).thenReturn(ok); + + clvmPoolManager.deactivateOnAllHosts(pool, "template-abc"); + + ArgumentCaptor cmd = ArgumentCaptor.forClass(ClvmLockTransferCommand.class); + verify(agentMgr).send(eq(HOST_ID_1), cmd.capture()); + verify(agentMgr).send(eq(HOST_ID_2), any(ClvmLockTransferCommand.class)); + verify(agentMgr, never()).send(eq(3L), any(Command.class)); + Assert.assertEquals(ClvmLockTransferCommand.Operation.DEACTIVATE, cmd.getValue().getOperation()); + Assert.assertEquals("/dev/sharedvg/template-abc", cmd.getValue().getLvPath()); + } } diff --git a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java index 61407a9d4c2b..9ea9b5b4a9bd 100755 --- a/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java +++ b/server/src/test/java/com/cloud/template/TemplateManagerImplTest.java @@ -31,7 +31,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.UUID; import java.util.concurrent.BlockingQueue; import java.util.concurrent.ExecutorService; @@ -43,6 +45,7 @@ import org.junit.After; import org.junit.Assert; +import org.mockito.InOrder; import org.junit.Before; import org.junit.Ignore; import org.junit.Test; @@ -99,6 +102,8 @@ import com.cloud.storage.StoragePoolStatus; import com.cloud.storage.TemplateProfile; import com.cloud.storage.VMTemplateStoragePoolVO; +import com.cloud.storage.clvm.ClvmPoolManager; +import com.cloud.agent.api.storage.DestroyCommand; import com.cloud.storage.VMTemplateStorageResourceAssoc; import com.cloud.storage.VMTemplateVO; import com.cloud.storage.VolumeVO; @@ -276,6 +281,15 @@ public class TemplateManagerImplTest extends TestCase { @Mock private UserDataVO userDataMock; + @Mock + ClvmPoolManager clvmPoolManager; + + @Mock + StorageManager storageManagerMock; + + @Mock + TemplateDataFactory templateDataFactoryMock; + public class CustomThreadPoolExecutor extends ThreadPoolExecutor { AtomicInteger ai = new AtomicInteger(0); public CustomThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, @@ -1487,4 +1501,90 @@ public void prepareIsoForVmProfileUsesConsecutiveDeviceIdsWithoutConfigDrive() { Assert.assertEquals(Arrays.asList(3L, 4L), disks.stream().map(DiskTO::getDiskSeq).collect(java.util.stream.Collectors.toList())); }); } + + private VMTemplateStoragePoolVO mockClvmTemplateRef(long refId, long poolId, String installPath) { + VMTemplateStoragePoolVO ref = mock(VMTemplateStoragePoolVO.class); + Mockito.lenient().when(ref.getId()).thenReturn(refId); + Mockito.lenient().when(ref.getPoolId()).thenReturn(poolId); + Mockito.lenient().when(ref.getTemplateId()).thenReturn(202L); + Mockito.lenient().when(ref.getInstallPath()).thenReturn(installPath); + Mockito.lenient().when(ref.getDownloadState()).thenReturn(VMTemplateStorageResourceAssoc.Status.DOWNLOADED); + return ref; + } + + private PrimaryDataStore mockClvmEviction(VMTemplateStoragePoolVO ref) { + StoragePoolVO pool = mock(StoragePoolVO.class); + when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.CLVM_NG); + when(primaryDataStoreDao.findById(2L)).thenReturn(pool); + when(clvmPoolManager.findPoolIdsSharingVg(pool)).thenReturn(Set.of(2L, 3L)); + PrimaryDataStore store = mock(PrimaryDataStore.class); + Mockito.lenient().when(store.getPoolType()).thenReturn(Storage.StoragePoolType.CLVM_NG); + when(vmTemplatePoolDao.acquireInLockTable(ref.getId())).thenReturn(ref); + when(dataStoreManager.getPrimaryDataStore(2L)).thenReturn(store); + return store; + } + + @Test + public void evictClvmTemplateKeepsLvReferencedByPoolOnSameVg() throws Exception { + VMTemplateStoragePoolVO ref = mockClvmTemplateRef(5L, 2L, "template-abc"); + VMTemplateStoragePoolVO otherRef = mockClvmTemplateRef(6L, 3L, "template-abc"); + mockClvmEviction(ref); + when(vmTemplatePoolDao.listByTemplateId(202L, List.of(3L))).thenReturn(List.of(otherRef)); + + templateManager.evictTemplateFromStoragePool(ref); + + Mockito.verify(vmTemplatePoolDao).remove(5L); + Mockito.verify(storageManagerMock, Mockito.never()).sendToPool(any(StoragePool.class), any(DestroyCommand.class)); + Mockito.verify(clvmPoolManager, Mockito.never()).deactivateOnAllHosts(any(), any()); + } + + @Test + public void evictClvmTemplateDeactivatesEverywhereBeforeRemovingLv() throws Exception { + VMTemplateStoragePoolVO ref = mockClvmTemplateRef(5L, 2L, "template-abc"); + PrimaryDataStore store = mockClvmEviction(ref); + when(vmTemplatePoolDao.listByTemplateId(202L, List.of(3L))).thenReturn(Collections.emptyList()); + com.cloud.agent.api.Answer answer = mock(com.cloud.agent.api.Answer.class); + when(answer.getResult()).thenReturn(true); + when(storageManagerMock.sendToPool(any(StoragePool.class), any(DestroyCommand.class))).thenReturn(answer); + + templateManager.evictTemplateFromStoragePool(ref); + + InOrder order = Mockito.inOrder(clvmPoolManager, storageManagerMock); + order.verify(clvmPoolManager).deactivateOnAllHosts(store, "template-abc"); + order.verify(storageManagerMock).sendToPool(any(StoragePool.class), any(DestroyCommand.class)); + Mockito.verify(vmTemplatePoolDao).remove(5L); + } + + @Test + public void evictClvmTemplateKeepsRecordWhenLvRemovalFails() throws Exception { + VMTemplateStoragePoolVO ref = mockClvmTemplateRef(5L, 2L, "template-abc"); + mockClvmEviction(ref); + when(vmTemplatePoolDao.listByTemplateId(202L, List.of(3L))).thenReturn(Collections.emptyList()); + com.cloud.agent.api.Answer answer = mock(com.cloud.agent.api.Answer.class); + when(answer.getResult()).thenReturn(false); + when(storageManagerMock.sendToPool(any(StoragePool.class), any(DestroyCommand.class))).thenReturn(answer); + + templateManager.evictTemplateFromStoragePool(ref); + + Mockito.verify(vmTemplatePoolDao, Mockito.never()).remove(5L); + } + + @Test + public void clvmTemplateUsedAsBackingOnPoolWithSameVgIsNotUnused() { + VMTemplateStoragePoolVO ref = mockClvmTemplateRef(5L, 2L, "template-abc"); + StoragePoolVO pool = mock(StoragePoolVO.class); + when(pool.getId()).thenReturn(2L); + when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.CLVM_NG); + when(primaryDataStoreDao.findById(2L)).thenReturn(pool); + when(clvmPoolManager.findPoolIdsSharingVg(pool)).thenReturn(Set.of(2L, 3L)); + VMTemplateVO template = mock(VMTemplateVO.class); + when(template.getId()).thenReturn(202L); + when(template.getTemplateType()).thenReturn(Storage.TemplateType.USER); + when(template.getFormat()).thenReturn(Storage.ImageFormat.QCOW2); + when(vmTemplateDao.findByIdIncludingRemoved(202L)).thenReturn(template); + when(vmTemplatePoolDao.listByPoolId(2L)).thenReturn(List.of(ref)); + when(volumeDao.isAnyVolumeActivelyUsingTemplateOnPool(202L, 3L)).thenReturn(true); + + Assert.assertTrue(templateManager.getUnusedTemplatesInPool(pool).isEmpty()); + } } From f53764cece001d7938bf6ac49acaeb19e8fa0574 Mon Sep 17 00:00:00 2001 From: Pearl1594 Date: Mon, 9 Nov 2026 08:38:55 -0500 Subject: [PATCH 4/4] CLVM: move sanlock HA checker and lease reader to kvm.storage.clvm package, rename heartbeat check --- .../java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java | 4 ++-- .../hypervisor/kvm/resource/LibvirtComputingResource.java | 2 +- .../wrapper/LibvirtClvmLockTransferCommandWrapper.java | 2 +- .../com/cloud/hypervisor/kvm/storage/LibvirtStoragePool.java | 1 + .../kvm/storage/{ => clvm}/ClvmSanlockHaChecker.java | 2 +- .../kvm/storage/{ => clvm}/ClvmSanlockLeaseReader.java | 2 +- .../kvm/storage/{ => clvm}/ClvmSanlockHaCheckerTest.java | 2 +- .../kvm/storage/{ => clvm}/ClvmSanlockLeaseReaderTest.java | 2 +- 8 files changed, 9 insertions(+), 8 deletions(-) rename plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/{ => clvm}/ClvmSanlockHaChecker.java (99%) rename plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/{ => clvm}/ClvmSanlockLeaseReader.java (99%) rename plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/{ => clvm}/ClvmSanlockHaCheckerTest.java (98%) rename plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/{ => clvm}/ClvmSanlockLeaseReaderTest.java (99%) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java index 20afa31a7934..1cf9b8d6ab12 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java @@ -44,7 +44,7 @@ public Boolean hasHeartBeat() { storagePools.stream().map(pool -> pool.getPoolUUID()).collect(Collectors.joining(", "))); logger.debug("Checking heart beat with KVMHAChecker for {}", hostAndPools); - boolean heartBeatCheckResult = checkPoolsHeartBeat(); + boolean heartBeatCheckResult = doesAnyPoolHasHeartBeat(); if (!heartBeatCheckResult) { logger.warn("All checks with KVMHAChecker for {} considered it as dead. It may cause a shutdown of the host.", hostAndPools); @@ -57,7 +57,7 @@ public Boolean hasHeartBeat() { * With reportIfHeartBeatFailedForOneStoragePool the host is dead as soon as one pool has no heart beat, * otherwise it is dead only when none of the pools has one. */ - private boolean checkPoolsHeartBeat() { + private boolean doesAnyPoolHasHeartBeat() { boolean anyPoolHasHeartBeat = false; for (HAStoragePool pool : storagePools) { boolean poolHasHeartBeat = pool.getPool().hasHeartBeat(pool, host); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index be1cf9cdef0e..4a038034c998 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -230,7 +230,7 @@ import com.cloud.hypervisor.kvm.resource.rolling.maintenance.RollingMaintenanceServiceExecutor; import com.cloud.hypervisor.kvm.resource.wrapper.LibvirtRequestWrapper; import com.cloud.hypervisor.kvm.resource.wrapper.LibvirtUtilitiesHelper; -import com.cloud.hypervisor.kvm.storage.ClvmSanlockHaChecker; +import com.cloud.hypervisor.kvm.storage.clvm.ClvmSanlockHaChecker; import com.cloud.hypervisor.kvm.storage.IscsiStorageCleanupMonitor; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java index 943acaa4d45a..01008ca9c730 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtClvmLockTransferCommandWrapper.java @@ -20,7 +20,7 @@ import java.io.File; import com.cloud.agent.api.Answer; -import com.cloud.hypervisor.kvm.storage.ClvmSanlockLeaseReader; +import com.cloud.hypervisor.kvm.storage.clvm.ClvmSanlockLeaseReader; import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferAnswer.LeaseMode; import org.apache.cloudstack.storage.clvm.command.ClvmLockTransferCommand; 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 4e11c7dca5ec..da1cb72efb40 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 @@ -34,6 +34,7 @@ import com.cloud.agent.properties.AgentPropertiesFileHandler; import com.cloud.ha.HighAvailabilityManager; import com.cloud.hypervisor.kvm.resource.KVMHABase.HAStoragePool; +import com.cloud.hypervisor.kvm.storage.clvm.ClvmSanlockHaChecker; import com.cloud.storage.Storage; import com.cloud.storage.Storage.StoragePoolType; import com.cloud.storage.clvm.ClvmPoolManager; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaChecker.java similarity index 99% rename from plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java rename to plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaChecker.java index 95eca074d3cf..8f1630f2abb9 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaChecker.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaChecker.java @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -package com.cloud.hypervisor.kvm.storage; +package com.cloud.hypervisor.kvm.storage.clvm; import java.io.File; import java.util.regex.Matcher; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReader.java similarity index 99% rename from plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java rename to plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReader.java index 84086c2de577..7e79f241a17f 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReader.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReader.java @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -package com.cloud.hypervisor.kvm.storage; +package com.cloud.hypervisor.kvm.storage.clvm; import java.util.ArrayList; import java.util.List; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaCheckerTest.java similarity index 98% rename from plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java rename to plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaCheckerTest.java index 85d5fe3e538a..1cddff8bc888 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockHaCheckerTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockHaCheckerTest.java @@ -16,7 +16,7 @@ // under the License. -package com.cloud.hypervisor.kvm.storage; +package com.cloud.hypervisor.kvm.storage.clvm; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReaderTest.java similarity index 99% rename from plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java rename to plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReaderTest.java index 7a21e1a764f0..b935a7fd5796 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/ClvmSanlockLeaseReaderTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/clvm/ClvmSanlockLeaseReaderTest.java @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -package com.cloud.hypervisor.kvm.storage; +package com.cloud.hypervisor.kvm.storage.clvm; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals;