From f3941973fabdb6d8ce16009261301588f0998842 Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:41:33 +0200 Subject: [PATCH 1/3] kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph takeRbdVolumeSnapshotOfStoppedVm() called image.snapCreate(snapshotName) twice. The first call creates the RBD snapshot, the second one always throws RbdException ("Failed to create snapshot ") because the snapshot already exists. The duplicate is a merge artifact: 30d306622a9 ("Merge branch '4.20' into 4.22") resolved a conflict by keeping the call from both sides - each parent had exactly one. Because there was no finally block, that exception skipped rbd.close(image) and r.ioCtxDestroy(io), so the agent kept the image open and held its RBD exclusive-lock indefinitely. The exception is only logged, so the snapshot job still reported success and the fault stayed invisible. Consequences observed on a KVM + Ceph/RBD cluster: - revertSnapshot fails with "com.ceph.rbd.RbdException: Failed to rollback snapshot ". librbd returns EROFS because a live peer holds the exclusive-lock; 'rbd snap rollback' only succeeds once that client dies and librbd can break the lock, which makes the failure look intermittent. - getRbdSnapshotSize() is never reached, so every snapshot is reported with physical size 0 when snapshot.backup.to.secondary is false. - The leaked watchers keep the image busy, so 'rbd rm' fails and the volume cannot be expunged - it stays stuck in state Destroy. Note the method also runs for RUNNING VMs: createSnapshot() branches on "RUNNING && !primaryPool.isExternalSnapshot()", and RBD is an external-snapshot pool, so every RBD volume snapshot took this path. Remove the duplicated call and move the image/IO-context cleanup into a finally block so the lock is released even if the snapshot itself fails. --- .../kvm/storage/KVMStorageProcessor.java | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index bc82744dd857..ffcb7d88d5f5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -2341,26 +2341,44 @@ private CreateObjectAnswer takeClvmVolumeSnapshotOfStoppedVm(KVMPhysicalDisk dis */ private Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhysicalDisk disk, String snapshotName) { Long snapshotSize = null; + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; try { - Rados r = radosConnect(primaryPool); + r = radosConnect(primaryPool); - final IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir()); - final Rbd rbd = new Rbd(io); - final RbdImage image = rbd.open(disk.getName()); + io = r.ioCtxCreate(primaryPool.getSourceDir()); + rbd = new Rbd(io); + image = rbd.open(disk.getName()); logger.debug("Attempting to create RBD snapshot {}@{}", disk.getName(), snapshotName); image.snapCreate(snapshotName); - image.snapCreate(snapshotName); long rbdSnapshotSize = getRbdSnapshotSize(primaryPool.getSourceDir(), disk.getName(), snapshotName, primaryPool.getSourceHost(), primaryPool.getAuthUserName(), primaryPool.getAuthSecret()); if (rbdSnapshotSize > 0) { snapshotSize = rbdSnapshotSize; } - - rbd.close(image); - r.ioCtxDestroy(io); } catch (final Exception e) { logger.error("A RBD snapshot operation on [{}] failed. The error was: {}", disk.getName(), e.getMessage(), e); + } finally { + // The image MUST be closed on every path. While it stays open this client holds the RBD + // exclusive-lock, and a later 'rbd snap rollback' (revertSnapshot) issued from any other host + // cannot take a live peer's lock - librbd then fails it with EROFS. + if (image != null) { + try { + rbd.close(image); + } catch (final Exception e) { + logger.warn("Failed to close RBD image [{}] after a snapshot operation. The error was: {}", disk.getName(), e.getMessage(), e); + } + } + if (io != null) { + try { + r.ioCtxDestroy(io); + } catch (final Exception e) { + logger.warn("Failed to destroy the RADOS IO context used to snapshot [{}]. The error was: {}", disk.getName(), e.getMessage(), e); + } + } } return snapshotSize; } From 3e8014637ad9065658ed8653f1a9d0f251582cff Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:41:33 +0200 Subject: [PATCH 2/3] kvm: release RBD handles on every path when cloning a volume from a snapshot createRBDvolumeFromRBDSnapshot() closed the source image, the cloned image and the RADOS IO context only on the success path, and called snapUnprotect() only there too. Two paths escaped that cleanup: - the early "Could not find snapshot ... on RBD" return, and - any RadosException/RbdException from clone(), resize() or flatten(), which is caught and turned into a null disk. Both leave the images open, so this client keeps the RBD exclusive-lock. That later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS from another host, and keeps the image busy so 'rbd rm' cannot remove it - the volume then stays stuck in state Destroy. The failure paths after snapProtect() are worse: the snapshot stays protected, and a protected snapshot can be deleted neither on its own nor together with its volume. Move the cleanup into a finally block, tracking whether the snapshot was actually protected so it is unprotected exactly when it needs to be. Failures during cleanup are logged and never mask the original outcome; a failed snapUnprotect is logged at ERROR since it needs manual intervention. This is the same class of defect as the leak fixed in takeRbdVolumeSnapshotOfStoppedVm(); no behaviour changes on the success path. --- .../kvm/storage/KVMStorageProcessor.java | 58 +++++++++++++++---- 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index ffcb7d88d5f5..d06252ee3645 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -2829,17 +2829,24 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S disk.setSize(size > volume.getVirtualSize() ? size : volume.getVirtualSize()); disk.setVirtualSize(size > volume.getVirtualSize() ? size : disk.getSize()); + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage srcImage = null; + RbdImage diskImage = null; + boolean snapProtected = false; + try { - Rados r = new Rados(srcPool.getAuthUserName()); + r = new Rados(srcPool.getAuthUserName()); r.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort()); r.confSet("key", srcPool.getAuthSecret()); r.confSet("client_mount_timeout", "30"); r.connect(); - IoCTX io = r.ioCtxCreate(srcPool.getSourceDir()); - Rbd rbd = new Rbd(io); - RbdImage srcImage = rbd.open(volume.getName()); + io = r.ioCtxCreate(srcPool.getSourceDir()); + rbd = new Rbd(io); + srcImage = rbd.open(volume.getName()); List snaps = srcImage.snapList(); boolean snapFound = false; @@ -2855,23 +2862,54 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S return null; } srcImage.snapProtect(snapshotName); + snapProtected = true; logger.debug(String.format("Try to clone snapshot %s on RBD", snapshotName)); rbd.clone(volume.getName(), snapshotName, io, disk.getName(), LibvirtStorageAdaptor.RBD_FEATURES, 0); - RbdImage diskImage = rbd.open(disk.getName()); + diskImage = rbd.open(disk.getName()); if (disk.getVirtualSize() > volume.getVirtualSize()) { diskImage.resize(disk.getVirtualSize()); } diskImage.flatten(); - rbd.close(diskImage); - - srcImage.snapUnprotect(snapshotName); - rbd.close(srcImage); - r.ioCtxDestroy(io); } catch (RadosException | RbdException e) { logger.error(String.format("Failed due to %s", e.getMessage()), e); disk = null; + } finally { + // Every handle has to be released on all paths, including the "snapshot not found" return and + // any failure of clone/resize/flatten. An image left open keeps this client's RBD + // exclusive-lock, which later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS and + // keeps the image busy so it cannot be removed. + if (diskImage != null) { + try { + rbd.close(diskImage); + } catch (final Exception e) { + logger.warn(String.format("Failed to close the cloned RBD image %s. The error was: %s", newUuid, e.getMessage()), e); + } + } + // A snapshot left protected cannot be deleted, and neither can its volume. + if (snapProtected) { + try { + srcImage.snapUnprotect(snapshotName); + } catch (final Exception e) { + logger.error(String.format("Failed to unprotect RBD snapshot %s; it and its volume cannot be deleted until this is " + + "resolved manually. The error was: %s", snapshotName, e.getMessage()), e); + } + } + if (srcImage != null) { + try { + rbd.close(srcImage); + } catch (final Exception e) { + logger.warn(String.format("Failed to close the source RBD image %s. The error was: %s", volume.getName(), e.getMessage()), e); + } + } + if (io != null) { + try { + r.ioCtxDestroy(io); + } catch (final Exception e) { + logger.warn(String.format("Failed to destroy the RADOS IO context used to clone %s. The error was: %s", snapshotName, e.getMessage()), e); + } + } } return disk; From 48b5fda8181983d8f01e2b4c0f969c55a90bbb79 Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:41:33 +0200 Subject: [PATCH 3/3] kvm: add regression tests for the RBD snapshot handle leak Two tests around takeRbdVolumeSnapshotOfStoppedVm, using the MockedConstruction pattern already used in this test class (the Rbd instance is created inside the method under test, so it cannot be injected): - createsSnapshotExactlyOnce guards the duplicated snapCreate call from coming back, and checks the image and IO context are released. - releasesHandlesWhenSnapshotFails makes snapCreate throw and asserts the image is still closed and the IO context destroyed, so a future failure cannot leak the RBD exclusive-lock again. takeRbdVolumeSnapshotOfStoppedVm, radosConnect and getRbdSnapshotSize widened from private to protected so the test can stub the Ceph interactions. --- .../kvm/storage/KVMStorageProcessor.java | 6 +- .../kvm/storage/KVMStorageProcessorTest.java | 77 +++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index d06252ee3645..836486c219cc 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java @@ -2339,7 +2339,7 @@ private CreateObjectAnswer takeClvmVolumeSnapshotOfStoppedVm(KVMPhysicalDisk dis * barriers properly (>2.6.32) this won't be any different then pulling the power * cord out of a running machine. */ - private Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhysicalDisk disk, String snapshotName) { + protected Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhysicalDisk disk, String snapshotName) { Long snapshotSize = null; Rados r = null; IoCTX io = null; @@ -2383,7 +2383,7 @@ private Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhy return snapshotSize; } - private long getRbdSnapshotSize(String poolPath, String diskName, String snapshotName, String rbdMonitor, String authUser, String authSecret) { + protected long getRbdSnapshotSize(String poolPath, String diskName, String snapshotName, String rbdMonitor, String authUser, String authSecret) { logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName, snapshotName); //cmd: rbd du /@ --format json --mon-host --id --key 2>/dev/null String snapshotDetailsInJson = Script.runSimpleBashScript(String.format("rbd du %s/%s@%s --format json --mon-host %s --id %s --key %s 2>/dev/null", poolPath, diskName, snapshotName, rbdMonitor, authUser, authSecret)); @@ -2670,7 +2670,7 @@ protected boolean isAvailablePoolSizeDividedByDiskSizeLesserThanMinRate(long ava return ((availablePoolSize * 1d) / (diskSize * 1d)) < MIN_RATE_BETWEEN_AVAILABLE_POOL_AND_DISK_SIZE_TO_TAKE_DISK_SNAPSHOT; } - private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosException { + protected Rados radosConnect(final KVMStoragePool primaryPool) throws RadosException { Rados r = new Rados(primaryPool.getAuthUserName()); r.confSet(CEPH_MON_HOST, primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort()); r.confSet(CEPH_AUTH_KEY, primaryPool.getAuthSecret()); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java index cc1e38a908b3..11d508d16468 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java @@ -18,6 +18,11 @@ */ package com.cloud.hypervisor.kvm.storage; +import com.ceph.rados.IoCTX; +import com.ceph.rados.Rados; +import com.ceph.rbd.Rbd; +import com.ceph.rbd.RbdException; +import com.ceph.rbd.RbdImage; import com.cloud.exception.InternalErrorException; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser; @@ -108,6 +113,11 @@ public class KVMStorageProcessorTest { private static final String directDownloadTemporaryPath = "/var/lib/libvirt/images/dd"; private static final long templateSize = 80000L; + private static final String RBD_POOL_NAME = "cloudstack"; + private static final String RBD_IMAGE_NAME = "b7a1f0a9-0f0e-4a1a-9a35-1c1a2e0f1b5e"; + private static final String SNAPSHOT_NAME = "8f1c1f0b-9d3e-4c2a-8a3d-6f0b2c9e1d47"; + private static final long SNAPSHOT_SIZE = 196624L; + private AutoCloseable closeable; @Before @@ -499,4 +509,71 @@ public void getDiskLabelToSnapshotTestDiskMatches() throws LibvirtException { Assert.assertEquals("vda", result); } + + /** + * Wires a mocked Ceph stack for {@link KVMStorageProcessor#takeRbdVolumeSnapshotOfStoppedVm} and returns the + * mocked disk. The Rbd instance is created inside the method under test, so it is mocked by construction. + */ + private KVMPhysicalDisk prepareRbdSnapshotMocks(Rados radosMock, IoCTX ioCtxMock) throws Exception { + KVMPhysicalDisk diskMock = Mockito.mock(KVMPhysicalDisk.class); + Mockito.lenient().doReturn(RBD_IMAGE_NAME).when(diskMock).getName(); + + Mockito.lenient().doReturn(RBD_POOL_NAME).when(kvmStoragePoolMock).getSourceDir(); + Mockito.lenient().doReturn("10.0.0.1").when(kvmStoragePoolMock).getSourceHost(); + Mockito.lenient().doReturn("cloudstack").when(kvmStoragePoolMock).getAuthUserName(); + Mockito.lenient().doReturn("secret").when(kvmStoragePoolMock).getAuthSecret(); + + Mockito.doReturn(radosMock).when(storageProcessorSpy).radosConnect(kvmStoragePoolMock); + Mockito.doReturn(ioCtxMock).when(radosMock).ioCtxCreate(RBD_POOL_NAME); + Mockito.lenient().doReturn(SNAPSHOT_SIZE).when(storageProcessorSpy).getRbdSnapshotSize(Mockito.anyString(), Mockito.anyString(), + Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), Mockito.anyString()); + + return diskMock; + } + + /** + * A duplicated snapCreate call used to throw "snapshot already exists" on every single RBD snapshot, which then + * skipped the cleanup below and leaked the image's exclusive-lock. + */ + @Test + public void takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce() throws Exception { + Rados radosMock = Mockito.mock(Rados.class); + IoCTX ioCtxMock = Mockito.mock(IoCTX.class); + RbdImage rbdImageMock = Mockito.mock(RbdImage.class); + KVMPhysicalDisk diskMock = prepareRbdSnapshotMocks(radosMock, ioCtxMock); + + try (MockedConstruction rbd = Mockito.mockConstruction(Rbd.class, ((mock, context) -> + Mockito.doReturn(rbdImageMock).when(mock).open(RBD_IMAGE_NAME)))) { + + Long result = storageProcessorSpy.takeRbdVolumeSnapshotOfStoppedVm(kvmStoragePoolMock, diskMock, SNAPSHOT_NAME); + + Assert.assertEquals(Long.valueOf(SNAPSHOT_SIZE), result); + Mockito.verify(rbdImageMock, Mockito.times(1)).snapCreate(SNAPSHOT_NAME); + Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock); + Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock); + } + } + + /** + * While the image stays open this client holds the RBD exclusive-lock, and a later 'rbd snap rollback' + * (revertSnapshot) from another host fails with EROFS. The handles must be released even when the snapshot fails. + */ + @Test + public void takeRbdVolumeSnapshotOfStoppedVmTestReleasesHandlesWhenSnapshotFails() throws Exception { + Rados radosMock = Mockito.mock(Rados.class); + IoCTX ioCtxMock = Mockito.mock(IoCTX.class); + RbdImage rbdImageMock = Mockito.mock(RbdImage.class); + KVMPhysicalDisk diskMock = prepareRbdSnapshotMocks(radosMock, ioCtxMock); + Mockito.doThrow(new RbdException("Failed to create snapshot")).when(rbdImageMock).snapCreate(SNAPSHOT_NAME); + + try (MockedConstruction rbd = Mockito.mockConstruction(Rbd.class, ((mock, context) -> + Mockito.doReturn(rbdImageMock).when(mock).open(RBD_IMAGE_NAME)))) { + + Long result = storageProcessorSpy.takeRbdVolumeSnapshotOfStoppedVm(kvmStoragePoolMock, diskMock, SNAPSHOT_NAME); + + Assert.assertNull(result); + Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock); + Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock); + } + } }