kvm: fix storage pool refcount race and misreported umount result

Two bugs in the storage pool teardown path.

adjustStoragePoolRefCount() means to lock on the single String instance
held as the map key, so that all callers share a monitor. When the map
has no entry for the pool, orElse(uuid) returns the caller's own instance
instead and the synchronized block guards nothing.

Increments cannot race each other: they happen inside createStoragePool,
which is only reached through KVMStoragePoolManager.createStoragePool,
and that is synchronized. Decrements are not covered, because
KVMStoragePoolManager.deleteStoragePool is not. So an increment and a
decrement can run at the same time, and while the map has no entry for
the pool they take different monitors and the decrement's remove() can
erase the increment. The count then reaches zero while the pool is still
in use. Use ConcurrentHashMap.compute(), which is atomic for the key.

deleteStoragePool() decided whether the retried umount had worked from
the return of runSimpleBashScript(), which is null both when the command
fails, because runScript() discards the output on a non-zero exit, and
when it succeeds without printing anything. A failed umount was therefore
logged and returned as a success. Take the outcome from whether the path
is still a mount point, which also covers the pool having been unmounted
by something else in the meantime, and return false rather than throwing
when it is still mounted: deleteStoragePool() is called from finally
blocks, where a throw would discard the result of an operation that has
already succeeded.

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

Author: bhouse-nexthop

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

@@ -700,35 +700,30 @@ public KVMPhysicalDisk getPhysicalDisk(String volumeUuid, KVMStoragePool pool) {
      * adjust refcount
      */
     private int adjustStoragePoolRefCount(String uuid, int adjustment) {
-        final String mutexKey = storagePoolRefCounts.keySet().stream()
-                .filter(k -> k.equals(uuid))
-                .findFirst()
-                .orElse(uuid);
-        synchronized (mutexKey) {
-            // some access on the storagePoolRefCounts.key(mutexKey) element
-            int refCount = storagePoolRefCounts.computeIfAbsent(mutexKey, k -> 0);
-            refCount += adjustment;
-            if (refCount < 1) {
-                storagePoolRefCounts.remove(mutexKey);
-            } else {
-                storagePoolRefCounts.put(mutexKey, refCount);
-            }
-            return refCount;
-        }
+        /*
+         * compute() is atomic for the key, so concurrent callers cannot lose an
+         * update. Returning null from the remapping function removes the entry,
+         * which keeps the map free of pools that are no longer in use.
+         */
+        Integer refCount = storagePoolRefCounts.compute(uuid, (key, count) -> {
+            int adjusted = (count == null ? 0 : count) + adjustment;
+            return adjusted < 1 ? null : adjusted;
+        });
+        return refCount == null ? 0 : refCount;
     }
     /**
      * Thread-safe increment storage pool usage refcount
      * @param uuid UUID of the storage pool to increment the count
      */
-    private void incStoragePoolRefCount(String uuid) {
+    protected void incStoragePoolRefCount(String uuid) {
         adjustStoragePoolRefCount(uuid, 1);
     }
     /**
      * Thread-safe decrement storage pool usage refcount for the given uuid and return if storage pool still in use.
      * @param uuid UUID of the storage pool to decrement the count
      * @return true if the storage pool is still used, else false.
      */
-    private boolean decStoragePoolRefCount(String uuid) {
+    protected boolean decStoragePoolRefCount(String uuid) {
         return adjustStoragePoolRefCount(uuid, -1) > 0;
     }
 
@@ -948,13 +943,29 @@ public boolean deleteStoragePool(String uuid) {
                 String targetPath = _mountPoint + File.separator + uuid;
                     logger.error("deleteStoragePool removed pool from libvirt, but libvirt had trouble unmounting the pool. Trying umount location " + targetPath +
                         " again in a few seconds");
-                String result = Script.runSimpleBashScript("sleep 5 && umount " + targetPath);
-                if (result == null) {
+                /*
+                 * runSimpleBashScript() returns null both when the command fails,
+                 * because runScript() discards the output on a non-zero exit, and
+                 * when it succeeds without printing anything. Its result therefore
+                 * cannot say whether the umount worked. It is still used to run the
+                 * umount, because it logs the failure reason, which is the useful
+                 * diagnostic, but the outcome is taken from whether the path is
+                 * still a mount point. That also covers the pool having been
+                 * unmounted by something else in the meantime.
+                 */
+                Script.runSimpleBashScript("sleep 5 && umount " + targetPath);
+                if (Script.runSimpleBashScriptForExitValue("mountpoint -q " + targetPath) != 0) {
                     logger.info("Succeeded in unmounting " + targetPath);
                     destroyStoragePoolHandleException(conn, uuid);
                     return true;
                 }
-                logger.error("Failed to unmount " + targetPath);
+                /*
+                 * Do not throw here. deleteStoragePool() is called from finally
+                 * blocks, where a throw would discard the result of an operation
+                 * that has already succeeded.
+                 */
+                logger.error("Failed to unmount " + targetPath + ", it is still a mount point");
+                return false;
             }
             throw new CloudRuntimeException(e.toString(), e);
         }

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

@@ -22,11 +22,20 @@
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.never;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.UUID;
+import java.util.concurrent.BrokenBarrierException;
+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 org.junit.After;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -176,4 +185,53 @@ public void testUpdateLocalPoolIops_NullResultFromScript() {
 
         Mockito.verify(mockPool, never()).setUsedIops(anyLong());
     }
+
+    @Test(timeout = 120000)
+    public void testStoragePoolRefCountCountsEveryConcurrentIncrement() throws Exception {
+        final LibvirtStorageAdaptor adaptor = new LibvirtStorageAdaptor(null);
+        final int threads = 16;
+        final int rounds = 500;
+        final CyclicBarrier barrier = new CyclicBarrier(threads);
+        final ExecutorService executor = Executors.newFixedThreadPool(threads);
+
+        try {
+            for (int round = 0; round < rounds; round++) {
+                // A fresh uuid each round, so every round starts with no entry for the pool.
+                final String uuid = String.valueOf(UUID.randomUUID());
+                final List<Future<?>> futures = new ArrayList<>();
+
+                for (int i = 0; i < threads; i++) {
+                    futures.add(executor.submit(() -> {
+                        /*
+                         * Every caller arrives with its own String instance, the way the
+                         * agent does when the uuid is parsed out of a separate command
+                         * payload for each request. The instances are equal but they are
+                         * not the same object.
+                         */
+                        final String ownInstance = new String(uuid);
+                        try {
+                            barrier.await();
+                        } catch (InterruptedException | BrokenBarrierException e) {
+                            Thread.currentThread().interrupt();
+                            throw new IllegalStateException(e);
+                        }
+                        adaptor.incStoragePoolRefCount(ownInstance);
+                    }));
+                }
+                for (Future<?> future : futures) {
+                    future.get(60, TimeUnit.SECONDS);
+                }
+
+                // Every increment must be counted, so the pool stays in use until the last release.
+                for (int i = 1; i < threads; i++) {
+                    Assert.assertTrue("Round " + round + ": pool should still be in use after " + i
+                            + " of " + threads + " releases", adaptor.decStoragePoolRefCount(uuid));
+                }
+                Assert.assertFalse("Round " + round + ": pool should no longer be in use after the last release",
+                        adaptor.decStoragePoolRefCount(uuid));
+            }
+        } finally {
+            executor.shutdownNow();
+        }
+    }
 }