kvm: take a pool's lock before the storage pool manager's

The previous commit took the per-pool lock inside
LibvirtStorageAdaptor.createStoragePool(), which is only reached through
KVMStoragePoolManager.createStoragePool(), and that is synchronized on
the manager. A create of a pool that is being torn down therefore waited
for the teardown while holding the manager wide lock. The umount retry in
deleteStoragePool() runs with no timeout, so a umount hung on storage
that has gone away held up the creates of every pool on the host, not
just the one being deleted.

Take the pool's lock in the manager first, before its own monitor, so a
create waiting for a teardown holds nothing that other pools need. The
locks move to KVMStoragePoolLocks so the manager and the adaptor share
them; the adaptor still takes the same lock, reentrantly on the create
path and on its own for deletes, including LibvirtStoragePool.delete(),
which does not go through the manager.

Signed-off-by: Brad House <bhouse@nexthop.ai>

Author: bhouse-nexthop

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

@@ -0,0 +1,45 @@
+// 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.concurrent.ConcurrentHashMap;
+
+/**
+ * One monitor per storage pool uuid, shared by {@link KVMStoragePoolManager} and
+ * {@link LibvirtStorageAdaptor} so that both take the same lock for a pool.
+ *
+ * The adaptor holds it across the whole of createStoragePool and deleteStoragePool. The refcount
+ * alone cannot keep a pool alive: deleteStoragePool decides to tear the pool down when the count
+ * reaches zero and then destroys and unmounts it, and a createStoragePool that finds the still
+ * active pool and takes a reference in between would have it torn down underneath it.
+ *
+ * Whoever also takes the manager wide lock must take this one first. A teardown can sit in an
+ * umount for as long as the storage takes to answer, and a create of that pool that waited for it
+ * while holding the manager wide lock would hold up the creates of every other pool on the host.
+ *
+ * Entries are never removed, as there is one per pool the host has ever used.
+ */
+final class KVMStoragePoolLocks {
+    private static final ConcurrentHashMap<String, Object> LOCKS = new ConcurrentHashMap<>();
+
+    private KVMStoragePoolLocks() {
+    }
+
+    static Object get(String uuid) {
+        return LOCKS.computeIfAbsent(uuid, k -> new Object());
+    }
+}

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

@@ -387,8 +387,19 @@ public KVMStoragePool createStoragePool(String name, String host, int port, Stri
         return createStoragePool(name, host, port, path, userInfo, type, details, true);
     }
 
+    private KVMStoragePool createStoragePool(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map<String, String> details, boolean primaryStorage) {
+        /*
+         * Take the pool's own lock before the manager wide one. A create of a pool that is being torn
+         * down waits here for the teardown, which can sit in an umount, and must not hold up the
+         * creates of every other pool on the host while it does. See KVMStoragePoolLocks.
+         */
+        synchronized (KVMStoragePoolLocks.get(name)) {
+            return createStoragePoolSynchronized(name, host, port, path, userInfo, type, details, primaryStorage);
+        }
+    }
+
     //Note: due to bug CLOUDSTACK-4459, createStoragepool can be called in parallel, so need to be synced.
-    private synchronized KVMStoragePool createStoragePool(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map<String, String> details, boolean primaryStorage) {
+    protected synchronized KVMStoragePool createStoragePoolSynchronized(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map<String, String> details, boolean primaryStorage) {
         StorageAdaptor adaptor = getStorageAdaptor(type);
         KVMStoragePool pool = adaptor.createStoragePool(name, host, port, path, userInfo, type, details, primaryStorage);
         if (pool instanceof LibvirtStoragePool) {

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

@@ -86,14 +86,6 @@ public class LibvirtStorageAdaptor implements StorageAdaptor {
     private String _mountPoint = "/mnt";
     private String _manageSnapshotPath;
     private static final ConcurrentHashMap<String, Integer> storagePoolRefCounts = new ConcurrentHashMap<>();
-    /*
-     * One monitor per pool, held across the whole of createStoragePool and deleteStoragePool. The
-     * refcount alone cannot keep a pool alive: deleteStoragePool decides to tear the pool down when
-     * the count reaches zero and then destroys and unmounts it, and a createStoragePool that finds the
-     * still active pool and takes a reference in between would have it torn down underneath it.
-     * Entries are never removed, as there is one per pool the host has ever used.
-     */
-    private static final ConcurrentHashMap<String, Object> storagePoolLocks = new ConcurrentHashMap<>();
 
     private String rbdTemplateSnapName = "cloudstack-base-snap";
     private static final int RBD_FEATURE_LAYERING = 1;
@@ -735,13 +727,9 @@ protected boolean decStoragePoolRefCount(String uuid) {
         return adjustStoragePoolRefCount(uuid, -1) > 0;
     }
 
-    private static Object getStoragePoolLock(String uuid) {
-        return storagePoolLocks.computeIfAbsent(uuid, k -> new Object());
-    }
-
     @Override
     public KVMStoragePool createStoragePool(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map<String, String> details, boolean isPrimaryStorage) {
-        synchronized (getStoragePoolLock(name)) {
+        synchronized (KVMStoragePoolLocks.get(name)) {
             return createStoragePoolLocked(name, host, port, path, userInfo, type, details, isPrimaryStorage);
         }
     }
@@ -918,7 +906,7 @@ private boolean destroyStoragePoolHandleException(Connect conn, String uuid)
 
     @Override
     public boolean deleteStoragePool(String uuid) {
-        synchronized (getStoragePoolLock(uuid)) {
+        synchronized (KVMStoragePoolLocks.get(uuid)) {
             return deleteStoragePoolLocked(uuid);
         }
     }

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

@@ -0,0 +1,77 @@
+// 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.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.util.UUID;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import com.cloud.storage.Storage.StoragePoolType;
+
+public class KVMStoragePoolManagerTest {
+
+    @Test(timeout = 60000)
+    public void testCreateWaitingForPoolLockDoesNotHoldManagerLock() throws Exception {
+        /*
+         * A teardown of a pool holds that pool's lock, and can sit in an umount for as long as the
+         * storage takes to answer. A create of the same pool has to wait for it, but must do so
+         * without holding the manager wide lock, or the creates of every other pool on the host wait
+         * behind the umount too.
+         */
+        final KVMStoragePoolManager manager = Mockito.mock(KVMStoragePoolManager.class, Mockito.CALLS_REAL_METHODS);
+        final KVMStoragePool pool = Mockito.mock(KVMStoragePool.class);
+        final String uuid = String.valueOf(UUID.randomUUID());
+        Mockito.doReturn(pool).when(manager).createStoragePoolSynchronized(eq(uuid), any(), anyInt(), any(), any(), any(), any(), anyBoolean());
+
+        final AtomicReference<KVMStoragePool> created = new AtomicReference<>();
+        final Thread create;
+        synchronized (KVMStoragePoolLocks.get(uuid)) {
+            // the teardown in progress
+            create = new Thread(() -> created.set(manager.createStoragePool(uuid, "127.0.0.1", 0, "/export/primary",
+                    null, StoragePoolType.NetworkFilesystem)));
+            create.start();
+            while (create.getState() != Thread.State.BLOCKED) {
+                Assert.assertTrue("create finished while the pool's lock was held", create.isAlive());
+                Thread.sleep(10);
+            }
+
+            final CountDownLatch managerLockTaken = new CountDownLatch(1);
+            final Thread otherPool = new Thread(() -> {
+                synchronized (manager) {
+                    managerLockTaken.countDown();
+                }
+            });
+            otherPool.start();
+            Assert.assertTrue("the manager wide lock was held by a create waiting for its pool's lock",
+                    managerLockTaken.await(30, TimeUnit.SECONDS));
+            Assert.assertNull(created.get());
+        }
+
+        create.join(30000);
+        Assert.assertSame(pool, created.get());
+    }
+}