bhouse-nexthop commented on code in PR #14320:
URL: https://github.com/apache/cloudstack/pull/14320#discussion_r4516741288
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java:
##########
@@ -458,7 +465,7 @@ public synchronized boolean
deleteStoragePool(StoragePoolType type, String uuid)
public boolean deleteStoragePool(StoragePoolType type, String uuid,
Map<String, String> details) {
StorageAdaptor adaptor = getStorageAdaptor(type);
boolean deleteStatus;
- if (type == StoragePoolType.NetworkFilesystem) {
+ if
(HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type))
{
_haMonitor.unregisterStoragePool(uuid);
deleteStatus = adaptor.deleteStoragePool(uuid);
Review Comment:
This is still the delete without details for RBD and SharedMountPoint.
`LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT` now contains both, so
`ModifyStoragePoolCommand(add=false)`
(`LibvirtModifyStoragePoolCommandWrapper`) still destroys the libvirt RBD pool
and undefines its secret while VMs may be using it. fe48966d52 doesn't change
this method. Only the HA unregister should depend on the HA type list. The
delete itself should stay as it was on main, where only NFS uses the uuid-only
delete:
```java
if
(HighAvailabilityManager.LIBVIRT_STORAGE_POOL_TYPES_WITH_HA_SUPPORT.contains(type))
{
_haMonitor.unregisterStoragePool(uuid);
}
if (type == StoragePoolType.NetworkFilesystem) {
deleteStatus = adaptor.deleteStoragePool(uuid);
} else {
deleteStatus = adaptor.deleteStoragePool(uuid, details);
}
```
##########
scripts/vm/hypervisor/kvm/kvmha_rbd.py:
##########
@@ -0,0 +1,179 @@
+# 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.
+
+# Access to Ceph for kvmheartbeat_rbd.py and kvmvmactivity_rbd.py.
+#
+# The objects of the heartbeat and of the activity state, and the watchers of
the
+# volumes, are read and written over one connection to the Ceph cluster with
the
+# Python bindings of librados and librbd (the packages python3-rados and
python3-rbd).
+# An operation which does not finish in TIMEOUT seconds fails, so that it does
not
+# block forever if the cluster can not be reached.
+#
+# The cephx key is not an argument of the scripts, as it would be visible in
the
+# process list: the scripts read it from the environment variable CEPH_KEY,
and give
+# it to the bindings in their configuration.
+#
+# Nothing is written to the standard error output: the agent merges it with the
+# standard output and reads only the first line of the result, so a message of
+# librados would hide the result of the script.
+
+import os
+
+try:
+ import rados
+except ImportError:
+ rados = None
+
+try:
+ import rbd
+except ImportError:
+ rbd = None
+
+KEY_ENV = "CEPH_KEY"
+TIMEOUT = 30
+MAX_OBJECT_SIZE = 1024 * 1024
+# A heartbeat is written every <interval> seconds, but a write which starts on
time can end up to TIMEOUT seconds
+# later when Ceph is slow, so the heartbeat of a host which is alive can be
older than the interval.
+HEARTBEAT_SLACK = TIMEOUT
Review Comment:
Suggestion, not blocking. The slack covers one operation timeout, but one
heartbeat write can take connect (`client_mount_timeout`, 30 s) plus the write
(`rados_osd_op_timeout`, 30 s). `kvmheartbeat_rbd.py` takes the timestamp after
the connect, and a reader only sees it once the write lands. The monitor also
writes its pools one after another before it sleeps for the interval. So a live
host whose Ceph ops are slow but still succeed can show an age of about
interval + 60–90 s, beyond the current interval + 30. `2 * TIMEOUT` (or the
agent's `heartbeat.update.timeout`, which is what the agent kills a write at)
would match the worst case of one write. This is already more forgiving than
NFS, so it can be a follow-up.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAChecker.java:
##########
@@ -37,23 +37,38 @@ public KVMHAChecker(List<HAStoragePool> pools, HostTO host,
boolean reportIfHear
/*
* True means heart beating is on going, or we can't get it's status.
* False means heart beating is stopped definitely.
+ *
+ * The answer comes from the heartbeats only: it is what VM HA uses
(KVMInvestigator, CheckOnHostCommand), which needs no out-of-band management.
+ * The check of the activity of the VMs on their volumes
(KVMHAVMActivityChecker) is only done by Host HA, which needs it.
+ *
+ * If the host is checked on more than one storage pool, it is dead if its
heartbeat is stopped on one of them when
+ * reportIfHeartBeatFailedForOneStoragePool is set, or else only if its
heartbeat is stopped on all of them.
*/
@Override
public Boolean hasHeartBeat() {
String hostAndPools = String.format("host IP [%s] in pools [%s]",
host.getPrivateNetwork().getIp(),
storagePools.stream().map(pool ->
pool.getPoolUUID()).collect(Collectors.joining(", ")));
logger.debug("Checking heart beat with KVMHAChecker for {}",
hostAndPools);
+ if (storagePools.isEmpty()) {
+ // this host monitors no storage pool, e.g. its agent is not
upgraded yet and does not monitor the pools of the type
+ // which a newer version does: it has nothing to tell about the
host, which is not a dead host
+ logger.debug("KVMHAChecker has no storage pool to check the heart
beat for {}, so it can not tell whether the host is alive.", hostAndPools);
+ return true;
Review Comment:
Optional follow-up, not blocking. `true` here becomes
`CheckOnHostAnswer.isAlive()`, and
`KVMHostActivityChecker.checkHostStatusWithNeighbourHosts` returns `Up` on the
first such answer. So one neighbour that monitors no pool (for example, the one
RBD host without the bindings) stops every host in the cluster from being
marked Down through VM HA. That is the safe direction. `CheckOnHostAnswer`
already has a `determined` flag, though. Answering "undetermined" here, and
having the activity checker skip undetermined answers and ask the next
neighbour, would let the other hosts still decide.
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManagerTest.java:
##########
@@ -0,0 +1,145 @@
+//
+// 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.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.withSettings;
+import static org.mockito.Mockito.when;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.logging.log4j.LogManager;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.hypervisor.kvm.resource.KVMHAMonitor;
+import com.cloud.storage.Storage.StoragePoolType;
+
+@RunWith(MockitoJUnitRunner.class)
+public class KVMStoragePoolManagerTest {
+
+ private static final String POOL_UUID =
"7684f6e5-fd91-3435-b04e-59ae0efb3914";
+
+ private final KVMHAMonitor monitor = mock(KVMHAMonitor.class);
+ private final StorageAdaptor adaptor = mock(StorageAdaptor.class);
+ private final Map<String, String> details = new HashMap<>();
+ private KVMStoragePoolManager manager;
+
+ @Before
+ public void setUp() throws Exception {
+ // the constructor is not run, as it looks for all the storage
adaptors of the agent, some of which need scripts and libraries of a host
+ manager = mock(KVMStoragePoolManager.class,
withSettings().defaultAnswer(CALLS_REAL_METHODS));
+ // every type of pool gets the adaptor of the test
+ Map<String, StorageAdaptor> adaptors = new HashMap<>();
+ adaptors.put("libvirt", adaptor);
+ setField("_storageMapper", adaptors);
+ setField("_storagePools", new ConcurrentHashMap<String, Object>());
+ setField("_haMonitor", monitor);
+ setField("logger", LogManager.getLogger(KVMStoragePoolManager.class));
+ }
+
+ private void setField(String name, Object value) throws Exception {
+ Field field = KVMStoragePoolManager.class.getDeclaredField(name);
+ field.setAccessible(true);
+ field.set(manager, value);
+ }
+
+ @Test
+ public void testPoolsWithHaSupportAreRemovedFromTheMonitorWhenDeleted() {
+ for (StoragePoolType type : new StoragePoolType[]
{StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint,
StoragePoolType.RBD}) {
+ when(adaptor.deleteStoragePool(POOL_UUID)).thenReturn(true);
+ when(adaptor.deleteStoragePool(POOL_UUID,
details)).thenReturn(true);
+
+ assertTrue(manager.deleteStoragePool(type, POOL_UUID));
+ assertTrue(manager.deleteStoragePool(type, POOL_UUID, details));
+ }
+ verify(monitor, times(6)).removeStoragePool(POOL_UUID);
+ }
+
+ @Test
+ public void testPoolsAreDeletedWithDetailsAsBefore() {
+ // the delete which takes the details does not destroy the libvirt
pool, which the VMs may still use, e.g. in maintenance of the pool
+ for (StoragePoolType type : new StoragePoolType[]
{StoragePoolType.NetworkFilesystem, StoragePoolType.RBD,
StoragePoolType.SharedMountPoint}) {
Review Comment:
These tests fail against the current code (2 of 5):
- `testPoolsAreDeletedWithDetailsAsBefore` fails at line 98, because RBD
still goes to the unstubbed `deleteStoragePool(uuid)`, which returns false.
- Both delete tests verify `monitor.removeStoragePool(...)`, but the manager
calls `unregisterStoragePool(...)`. That is the right call (no umount), so the
tests should verify `unregisterStoragePool` (lines 88, 100 and 112).
- `NetworkFilesystem` doesn't belong in this "as before" list. On main, NFS
has always used `deleteStoragePool(uuid)`, which unmounts it. Sending NFS to
the details variant (a no-op default in `StorageAdaptor`) would leave NFS
mounts behind. Please split NFS into its own case that expects
`deleteStoragePool(uuid)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
