diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java index 059f4f8b67af..97157f6374f5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java @@ -700,27 +700,22 @@ 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); } /** @@ -728,7 +723,7 @@ private void incStoragePoolRefCount(String uuid) { * @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); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java index 88346abd0176..a91e9e045054 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptorTest.java +++ b/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> 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(); + } + } }