kvm: hold a per-pool lock across storage pool create and delete

The refcount is atomic, but it cannot keep a pool alive on its own.
deleteStoragePool() drops the last reference and then goes on to destroy
and unmount the pool without holding anything. createStoragePool() is
serialised by KVMStoragePoolManager, but deleteStoragePool() is not, so
a create can find the still active pool and take a reference after the
delete has seen the count reach zero, and the delete then tears the pool
down underneath it.

Take a monitor per pool uuid for the whole of createStoragePool() and
deleteStoragePool(). Every delete path ends in deleteStoragePool(String),
including LibvirtStoragePool.delete(), so locking there covers callers
that bypass the manager. Pools other than the one being deleted are not
held up, which matters because a delete can sit in the five second umount
retry.

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

Author: bhouse-nexthop

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

@@ -86,6 +86,14 @@ 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;
@@ -808,8 +816,18 @@ 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)) {
+            return createStoragePoolLocked(name, host, port, path, userInfo, type, details, isPrimaryStorage);
+        }
+    }
+
+    protected KVMStoragePool createStoragePoolLocked(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map<String, String> details, boolean isPrimaryStorage) {
         logger.info("Attempting to create storage pool {} ({}) in libvirt", name, type);
         StoragePool sp;
         Connect conn;
@@ -981,6 +999,12 @@ private boolean destroyStoragePoolHandleException(Connect conn, String uuid)
 
     @Override
     public boolean deleteStoragePool(String uuid) {
+        synchronized (getStoragePoolLock(uuid)) {
+            return deleteStoragePoolLocked(uuid);
+        }
+    }
+
+    protected boolean deleteStoragePoolLocked(String uuid) {
         logger.info("Attempting to remove storage pool " + uuid + " from libvirt");
 
         // decrement and check if storage pool still in use

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

@@ -17,6 +17,9 @@
 
 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.anyList;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
@@ -28,11 +31,14 @@
 import java.util.Map;
 import java.util.UUID;
 import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.junit.After;
 import org.junit.Assert;
@@ -234,4 +240,67 @@ public void testStoragePoolRefCountCountsEveryConcurrentIncrement() throws Excep
             executor.shutdownNow();
         }
     }
+
+    /**
+     * Holds a delete of {@code deleteUuid} part way through its teardown, then starts a create of
+     * {@code createUuid} and reports whether that create completed while the delete was still held.
+     */
+    private boolean createRanDuringDelete(String deleteUuid, String createUuid) throws Exception {
+        final LibvirtStorageAdaptor adaptor = Mockito.spy(new LibvirtStorageAdaptor(null));
+        final CountDownLatch deleteEntered = new CountDownLatch(1);
+        final CountDownLatch releaseDelete = new CountDownLatch(1);
+        final AtomicBoolean deleteFinished = new AtomicBoolean();
+        final AtomicBoolean createRanBeforeDeleteFinished = new AtomicBoolean();
+
+        Mockito.doAnswer(invocation -> {
+            deleteEntered.countDown();
+            releaseDelete.await();
+            deleteFinished.set(true);
+            return true;
+        }).when(adaptor).deleteStoragePoolLocked(deleteUuid);
+        Mockito.doAnswer(invocation -> {
+            createRanBeforeDeleteFinished.set(!deleteFinished.get());
+            return mockPool;
+        }).when(adaptor).createStoragePoolLocked(Mockito.eq(createUuid), any(), anyInt(), any(), any(), any(), any(), anyBoolean());
+
+        final ExecutorService executor = Executors.newFixedThreadPool(2);
+        try {
+            final Future<Boolean> delete = executor.submit(() -> adaptor.deleteStoragePool(deleteUuid));
+            Assert.assertTrue("delete never started", deleteEntered.await(30, TimeUnit.SECONDS));
+
+            final Future<KVMStoragePool> create = executor.submit(() -> adaptor.createStoragePool(createUuid, "127.0.0.1", 0,
+                    "/export/secondary", null, Storage.StoragePoolType.NetworkFilesystem, null, false));
+            try {
+                create.get(2, TimeUnit.SECONDS);
+            } catch (TimeoutException e) {
+                // still waiting on the delete, which is what a create of the same pool must do
+            }
+
+            releaseDelete.countDown();
+            Assert.assertTrue(delete.get(30, TimeUnit.SECONDS));
+            Assert.assertSame(mockPool, create.get(30, TimeUnit.SECONDS));
+            return createRanBeforeDeleteFinished.get();
+        } finally {
+            releaseDelete.countDown();
+            executor.shutdownNow();
+        }
+    }
+
+    @Test(timeout = 120000)
+    public void testCreateStoragePoolWaitsForDeleteOfSamePool() throws Exception {
+        /*
+         * A delete that has dropped the last reference goes on to destroy and unmount the pool. A
+         * create that found the still active pool and took a reference in between would have it torn
+         * down underneath it, so the create has to wait for the teardown to finish.
+         */
+        final String uuid = String.valueOf(UUID.randomUUID());
+        Assert.assertFalse("create of a pool ran while that pool was being deleted",
+                createRanDuringDelete(uuid, new String(uuid)));
+    }
+
+    @Test(timeout = 120000)
+    public void testCreateStoragePoolDoesNotWaitForDeleteOfAnotherPool() throws Exception {
+        Assert.assertTrue("create of one pool waited for the delete of another",
+                createRanDuringDelete(String.valueOf(UUID.randomUUID()), String.valueOf(UUID.randomUUID())));
+    }
 }