From f7bfe933aae8f34e56769ef2e93798bbb2939ecf Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 17 Sep 2026 02:00:58 +0000 Subject: [PATCH 1/3] 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 --- .../kvm/storage/LibvirtStorageAdaptor.java | 51 +++++++++------- .../storage/LibvirtStorageAdaptorTest.java | 58 +++++++++++++++++++ 2 files changed, 89 insertions(+), 20 deletions(-) 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(); + } + } } From d40010d622fd053411fd0c34fbb463fe97a2be52 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 1 Oct 2026 20:24:21 +0000 Subject: [PATCH 2/3] 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 --- .../kvm/storage/LibvirtStorageAdaptor.java | 24 +++++++ .../storage/LibvirtStorageAdaptorTest.java | 69 +++++++++++++++++++ 2 files changed, 93 insertions(+) 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 97157f6374f5..a23833d273db 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 @@ -86,6 +86,14 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { private String _mountPoint = "/mnt"; private String _manageSnapshotPath; private static final ConcurrentHashMap 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 storagePoolLocks = new ConcurrentHashMap<>(); private String rbdTemplateSnapName = "cloudstack-base-snap"; private static final int RBD_FEATURE_LAYERING = 1; @@ -727,8 +735,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 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 details, boolean isPrimaryStorage) { logger.info("Attempting to create storage pool {} ({}) in libvirt", name, type); StoragePool sp; Connect conn; @@ -900,6 +918,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 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 a91e9e045054..abc49eb402b8 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 @@ -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 delete = executor.submit(() -> adaptor.deleteStoragePool(deleteUuid)); + Assert.assertTrue("delete never started", deleteEntered.await(30, TimeUnit.SECONDS)); + + final Future 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()))); + } } From 7e093df62ecabd0f2435950040a0b909443b58c7 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 2 Oct 2026 18:47:34 +0000 Subject: [PATCH 3/3] 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 --- .../kvm/storage/KVMStoragePoolLocks.java | 45 +++++++++++ .../kvm/storage/KVMStoragePoolManager.java | 13 +++- .../kvm/storage/LibvirtStorageAdaptor.java | 16 +--- .../storage/KVMStoragePoolManagerTest.java | 77 +++++++++++++++++++ 4 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolLocks.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManagerTest.java diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolLocks.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolLocks.java new file mode 100644 index 000000000000..dd204e0212f7 --- /dev/null +++ b/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 LOCKS = new ConcurrentHashMap<>(); + + private KVMStoragePoolLocks() { + } + + static Object get(String uuid) { + return LOCKS.computeIfAbsent(uuid, k -> new Object()); + } +} diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java index 35cc864268c3..a660ca5baee7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java +++ b/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 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 details, boolean primaryStorage) { + protected synchronized KVMStoragePool createStoragePoolSynchronized(String name, String host, int port, String path, String userInfo, StoragePoolType type, Map details, boolean primaryStorage) { StorageAdaptor adaptor = getStorageAdaptor(type); KVMStoragePool pool = adaptor.createStoragePool(name, host, port, path, userInfo, type, details, primaryStorage); if (pool instanceof LibvirtStoragePool) { 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 a23833d273db..83545eef9b72 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 @@ -86,14 +86,6 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { private String _mountPoint = "/mnt"; private String _manageSnapshotPath; private static final ConcurrentHashMap 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 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 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); } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManagerTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManagerTest.java new file mode 100644 index 000000000000..bbb1b4d722af --- /dev/null +++ b/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 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()); + } +}