CLVM: send the sanlock host id with the fence command and report hosts without one

- KVMFencer sets the sanlock host id of the fenced host in the FenceCommand, so the agents can check its CLVM leases instead of assuming it is alive
- ClvmPoolManager.addSanlockHostId is shared by KVMFencer and KVMHostActivityChecker
- raise an alert when a host connects to a CLVM pool without a sanlock host id, and log the cause when fencing fails for it

Author: Pearl1594

engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/provider/DefaultHostListener.java

@@ -197,22 +197,28 @@ public boolean hostConnect(long hostId, long poolId) throws StorageConflictExcep
         storageService.updateStorageCapabilities(poolId, false);
 
         if (ClvmPoolManager.isClvmPoolType(pool.getPoolType())) {
-            alertOnSharedSanlockId(pool, host);
+            alertOnSanlockHostId(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<Long> otherHostIds = clvmPoolManager.findHostsSharingSanlockId(pool, host.getId());
-        if (otherHostIds.isEmpty()) {
-            return;
+    /** The sanlock host id is needed to check the leases of a host, and must identify exactly one host of the pool. */
+    private void alertOnSanlockHostId(StoragePool pool, HostVO host) {
+        String msg;
+        if (!clvmPoolManager.hasSanlockHostId(host.getId())) {
+            msg = String.format("Host %s reports no %s, so the CLVM leases of storage pool %s cannot be checked and the host cannot be fenced. "
+                    + "Restart its agent and check that lvmlockd is running on it.", host, Host.HOST_SANLOCK_ID, pool);
+        } else {
+            List<Long> otherHostIds = clvmPoolManager.findHostsSharingSanlockId(pool, host.getId());
+            if (otherHostIds.isEmpty()) {
+                return;
+            }
+            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);
         }
-        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);
     }

plugins/hypervisors/kvm/src/main/java/org/apache/cloudstack/kvm/ha/KVMHostActivityChecker.java

@@ -22,15 +22,12 @@
 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;
@@ -48,7 +45,6 @@
 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;
@@ -73,7 +69,7 @@ public class KVMHostActivityChecker extends AdapterBase implements ActivityCheck
     @Inject
     private ResourceManager resourceManager;
     @Inject
-    private HostDetailsDao hostDetailsDao;
+    private ClvmPoolManager clvmPoolManager;
 
     @Override
     public boolean isActive(Host r, DateTime suspectTime) throws HACheckerException {
@@ -133,7 +129,7 @@ private Status checkHostStatusWithSameHost(Host host) {
         Status hostStatus;
         boolean reportFailureIfOneStorageIsDown = HighAvailabilityManager.KvmHAFenceHostIfHeartbeatFailsOnStorage.value();
         final CheckOnHostCommand cmd = new CheckOnHostCommand(host, reportFailureIfOneStorageIsDown);
-        addSanlockHostId(cmd.getHost(), host);
+        clvmPoolManager.addSanlockHostId(cmd.getHost(), host);
         try {
             logger.debug("Checking {} status...", host.toString());
             Answer answer = agentMgr.easySend(host.getId(), cmd);
@@ -161,7 +157,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);
+        clvmPoolManager.addSanlockHostId(cmd.getHost(), host);
         List<HostVO> neighbors = resourceManager.listHostsInClusterByStatus(host.getClusterId(), Status.Up);
         for (HostVO neighbor : neighbors) {
             if (neighbor.getId() == host.getId()
@@ -214,7 +210,7 @@ protected boolean verifyActivityOfStorageOnHost(HashMap<StoragePool, List<Volume
         List<Volume> volume_list = poolVolMap.get(pool);
         final CheckVMActivityOnStoragePoolCommand cmd = new CheckVMActivityOnStoragePoolCommand(host, pool, volume_list, suspectTime);
         if (ClvmPoolManager.isClvmPoolType(pool.getPoolType())) {
-            addSanlockHostId(cmd.getHost(), host);
+            clvmPoolManager.addSanlockHostId(cmd.getHost(), host);
         }
 
         logger.debug("Checking VM activity for {} on storage pool [{}].", host.toString(), pool);
@@ -236,13 +232,6 @@ protected boolean verifyActivityOfStorageOnHost(HashMap<StoragePool, List<Volume
         return activityStatus;
     }
 
-    private void addSanlockHostId(HostTO hostTO, Host host) {
-        DetailVO detail = hostDetailsDao.findDetail(host.getId(), Host.HOST_SANLOCK_ID);
-        if (detail != null && NumberUtils.isDigits(detail.getValue())) {
-            hostTO.setSanlockHostId(Long.valueOf(detail.getValue()));
-        }
-    }
-
     private HashMap<StoragePool, List<Volume>> getStoragePoolAndVolumeInfoOnHost(Host host) {
         List<VMInstanceVO> vmListOnHost = vmInstanceDao.listByHostId(host.getId());
         List<VolumeVO> volumeListOnHost = new ArrayList<>();

plugins/hypervisors/kvm/src/test/java/org/apache/cloudstack/kvm/ha/KVMHostActivityCheckerTest.java

@@ -23,6 +23,8 @@
 import static org.junit.Assert.assertNull;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -42,22 +44,22 @@
 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.agent.api.to.HostTO;
 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.clvm.ClvmPoolManager;
 import com.cloud.storage.Volume;
 
 @RunWith(MockitoJUnitRunner.Silent.class)
 public class KVMHostActivityCheckerTest {
 
     @Mock
-    private HostDetailsDao hostDetailsDao;
+    private ClvmPoolManager clvmPoolManager;
     @Mock
     private StorageManager storageManager;
     @Mock
@@ -75,11 +77,18 @@ public class KVMHostActivityCheckerTest {
     @InjectMocks
     private KVMHostActivityChecker checker;
 
+    private void reportSanlockHostId(Long sanlockHostId) {
+        doAnswer(invocation -> {
+            invocation.<HostTO>getArgument(0).setSanlockHostId(sanlockHostId);
+            return null;
+        }).when(clvmPoolManager).addSanlockHostId(any(HostTO.class), eq(host));
+    }
+
     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"));
+        reportSanlockHostId(2L);
         when(storageManager.sendToPool(any(StoragePool.class), any(long[].class), any(CheckVMActivityOnStoragePoolCommand.class))).thenReturn(new Answer(null));
         HashMap<StoragePool, List<Volume>> poolVolumes = new HashMap<>();
         poolVolumes.put(pool, List.of(volume));
@@ -89,7 +98,7 @@ private void runActivityCheck(StoragePoolType poolType) throws Exception {
     @Test
     public void testSanlockHostIdIsNotLookedUpForNonClvmPool() throws Exception {
         runActivityCheck(StoragePoolType.NetworkFilesystem);
-        verify(hostDetailsDao, never()).findDetail(anyLong(), any());
+        verify(clvmPoolManager, never()).addSanlockHostId(any(), any());
     }
 
     @Test
@@ -104,7 +113,7 @@ public void testSanlockHostIdIsSentForClvmPool() throws Exception {
     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"));
+        reportSanlockHostId(2L);
         when(agentMgr.easySend(anyLong(), any(CheckOnHostCommand.class))).thenReturn(new Answer(null, false, "down"));
         checker.getHostAgentStatus(host);
         ArgumentCaptor<CheckOnHostCommand> captor = ArgumentCaptor.forClass(CheckOnHostCommand.class);

server/src/main/java/com/cloud/ha/KVMFencer.java

@@ -35,6 +35,7 @@
 import com.cloud.host.dao.HostDao;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.resource.ResourceManager;
+import com.cloud.storage.clvm.ClvmPoolManager;
 import com.cloud.utils.component.AdapterBase;
 import com.cloud.vm.VirtualMachine;
 
@@ -48,6 +49,8 @@ public class KVMFencer extends AdapterBase implements FenceBuilder {
     AlertManager _alertMgr;
     @Inject
     ResourceManager _resourceMgr;
+    @Inject
+    ClvmPoolManager _clvmPoolManager;
 
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
@@ -80,6 +83,7 @@ public Boolean fenceOff(VirtualMachine vm, Host host) {
 
         List<HostVO> hosts = _resourceMgr.listAllHostsInCluster(host.getClusterId());
         FenceCommand fence = new FenceCommand(vm, host);
+        _clvmPoolManager.addSanlockHostId(fence.getHost(), host);
         fence.setReportCheckFailureIfOneStorageIsDown(HighAvailabilityManager.KvmHAFenceHostIfHeartbeatFailsOnStorage.value());
 
         int i = 0;
@@ -107,6 +111,11 @@ public Boolean fenceOff(VirtualMachine vm, Host host) {
             }
         }
 
+        if (fence.getHost().getSanlockHostId() == null && _clvmPoolManager.hasClvmPool(host)) {
+            logger.error("Host {} reports no {}, so the CLVM leases of its pools cannot be checked and it is assumed alive. "
+                    + "Restart its agent and check that lvmlockd is running on it.", host, Host.HOST_SANLOCK_ID);
+        }
+
         _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_HOST, host.getDataCenterId(), host.getPodId(),
                             "Unable to fence off host: " + host.getId(),
                             "Fencing off host " + host.getId() + " did not succeed after asking " + i + " hosts. " +

server/src/main/java/com/cloud/storage/clvm/ClvmPoolManager.java

@@ -41,6 +41,7 @@
 
 import com.cloud.agent.AgentManager;
 import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.to.HostTO;
 import com.cloud.exception.AgentUnavailableException;
 import com.cloud.exception.OperationTimedoutException;
 import com.cloud.host.Host;
@@ -63,6 +64,7 @@
 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.commons.lang3.math.NumberUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.springframework.stereotype.Component;
@@ -546,6 +548,27 @@ private String getSanlockId(long hostId) {
         return detail != null ? detail.getValue() : null;
     }
 
+    /** Sets the sanlock host id of the host in the command's host, so the agents can check its CLVM leases. */
+    public void addSanlockHostId(HostTO hostTO, Host host) {
+        String sanlockId = getSanlockId(host.getId());
+        if (NumberUtils.isDigits(sanlockId)) {
+            hostTO.setSanlockHostId(Long.valueOf(sanlockId));
+        }
+    }
+
+    public boolean hasSanlockHostId(long hostId) {
+        return NumberUtils.isDigits(getSanlockId(hostId));
+    }
+
+    /** Whether the host can use a CLVM or CLVM_NG pool: one of its cluster or a zone-wide one. */
+    public boolean hasClvmPool(Host host) {
+        List<StoragePoolVO> pools = new ArrayList<>(_storagePoolDao.findZoneWideStoragePoolsByHypervisor(host.getDataCenterId(), host.getHypervisorType()));
+        if (host.getClusterId() != null) {
+            pools.addAll(_storagePoolDao.findPoolsInClusters(Collections.singletonList(host.getClusterId()), null));
+        }
+        return pools.stream().anyMatch(pool -> isClvmPoolType(pool.getPoolType()));
+    }
+
     /**
      * 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.

server/src/test/java/com/cloud/ha/KVMFencerTest.java

@@ -21,6 +21,7 @@
 import com.cloud.agent.AgentManager;
 import com.cloud.agent.api.FenceAnswer;
 import com.cloud.agent.api.FenceCommand;
+import com.cloud.agent.api.to.HostTO;
 import com.cloud.alert.AlertManager;
 import com.cloud.exception.AgentUnavailableException;
 import com.cloud.exception.OperationTimedoutException;
@@ -29,11 +30,13 @@
 import com.cloud.host.dao.HostDao;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.resource.ResourceManager;
+import com.cloud.storage.clvm.ClvmPoolManager;
 import com.cloud.vm.VirtualMachine;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.ArgumentMatchers;
 import org.mockito.Mock;
 import org.mockito.Mockito;
@@ -53,6 +56,8 @@ public class KVMFencerTest {
     AlertManager alertMgr;
     @Mock
     ResourceManager resourceManager;
+    @Mock
+    ClvmPoolManager clvmPoolManager;
 
     KVMFencer fencer;
 
@@ -63,6 +68,7 @@ public void setup() {
         fencer._alertMgr = alertMgr;
         fencer._hostDao = hostDao;
         fencer._resourceMgr = resourceManager;
+        fencer._clvmPoolManager = clvmPoolManager;
     }
 
     @Test
@@ -192,4 +198,63 @@ public void testWithSingleNotKVM() {
         Assert.assertNull(fencer.fenceOff(virtualMachine, host));
     }
 
+    @Test
+    public void testFenceCommandCarriesSanlockHostIdOfFencedHost() throws AgentUnavailableException, OperationTimedoutException {
+        HostVO host = Mockito.mock(HostVO.class);
+        Mockito.when(host.getClusterId()).thenReturn(1l);
+        Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.KVM);
+        Mockito.lenient().when(host.getStatus()).thenReturn(Status.Down);
+        Mockito.when(host.getId()).thenReturn(2l);
+        HostVO otherHost = Mockito.mock(HostVO.class);
+        Mockito.when(otherHost.getHypervisorType()).thenReturn(HypervisorType.KVM);
+        Mockito.when(otherHost.getStatus()).thenReturn(Status.Up);
+        Mockito.when(otherHost.getId()).thenReturn(3l);
+        Mockito.doAnswer(invocation -> {
+            invocation.<HostTO>getArgument(0).setSanlockHostId(7L);
+            return null;
+        }).when(clvmPoolManager).addSanlockHostId(ArgumentMatchers.any(HostTO.class), ArgumentMatchers.eq(host));
+        Mockito.when(resourceManager.listAllHostsInCluster(1l)).thenReturn(Arrays.asList(host, otherHost));
+        Mockito.when(agentManager.send(ArgumentMatchers.anyLong(), ArgumentMatchers.any(FenceCommand.class))).thenReturn(new FenceAnswer(null, true, "ok"));
+
+        Assert.assertTrue(fencer.fenceOff(Mockito.mock(VirtualMachine.class), host));
+
+        ArgumentCaptor<FenceCommand> captor = ArgumentCaptor.forClass(FenceCommand.class);
+        Mockito.verify(agentManager).send(ArgumentMatchers.eq(3l), captor.capture());
+        Assert.assertEquals(Long.valueOf(7), captor.getValue().getHost().getSanlockHostId());
+    }
+
+    private HostVO mockDownHostInCluster(long id) {
+        HostVO host = Mockito.mock(HostVO.class);
+        Mockito.when(host.getClusterId()).thenReturn(1l);
+        Mockito.when(host.getHypervisorType()).thenReturn(HypervisorType.KVM);
+        Mockito.lenient().when(host.getDataCenterId()).thenReturn(1l);
+        Mockito.lenient().when(host.getPodId()).thenReturn(1l);
+        Mockito.lenient().when(host.getStatus()).thenReturn(Status.Down);
+        Mockito.when(host.getId()).thenReturn(id);
+        return host;
+    }
+
+    @Test
+    public void testFailedFenceChecksForClvmPoolWhenHostReportsNoSanlockHostId() {
+        HostVO host = mockDownHostInCluster(2l);
+        Mockito.when(resourceManager.listAllHostsInCluster(1l)).thenReturn(Collections.singletonList(host));
+
+        Assert.assertFalse(fencer.fenceOff(Mockito.mock(VirtualMachine.class), host));
+
+        Mockito.verify(clvmPoolManager).hasClvmPool(host);
+    }
+
+    @Test
+    public void testFailedFenceDoesNotCheckForClvmPoolWhenHostReportsSanlockHostId() {
+        HostVO host = mockDownHostInCluster(2l);
+        Mockito.when(resourceManager.listAllHostsInCluster(1l)).thenReturn(Collections.singletonList(host));
+        Mockito.doAnswer(invocation -> {
+            invocation.<HostTO>getArgument(0).setSanlockHostId(7L);
+            return null;
+        }).when(clvmPoolManager).addSanlockHostId(ArgumentMatchers.any(HostTO.class), ArgumentMatchers.eq(host));
+
+        Assert.assertFalse(fencer.fenceOff(Mockito.mock(VirtualMachine.class), host));
+
+        Mockito.verify(clvmPoolManager, Mockito.never()).hasClvmPool(ArgumentMatchers.any());
+    }
 }