This is an automated email from the ASF dual-hosted git repository.
weizhouapache pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.22 by this push:
new c860e0c214e kvm: fix RBD exclusive-lock leak that breaks
revertSnapshot on Ceph (#13835)
c860e0c214e is described below
commit c860e0c214e6377399c3d4b9c64798286df360d9
Author: calvix <[email protected]>
AuthorDate: Tue Sep 8 14:12:07 2026 +0200
kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph (#13835)
* 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 <uuid>") 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 <uuid>". 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: 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: 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: extract RBD handle teardown into null-safe helper methods
Address review feedback: the nested try-catch teardown blocks in the
finally clauses of takeRbdVolumeSnapshotOfStoppedVm and
createRBDvolumeFromRBDSnapshot are extracted into two reusable,
null-safe helpers that log but never throw:
- closeRbdImage (3 call sites)
- destroyRadosIoCtx (2 call sites)
No behavior change.
* kvm: extract RBD snapshot unprotect into a helper method
Follow-up to the teardown helpers: the snapUnprotect block in the
finally clause of createRBDvolumeFromRBDSnapshot moves into a
never-throwing unprotectRbdSnapshot helper, so the finally clause is
now free of inline try-catch constructions entirely.
No behavior change.
---------
Co-authored-by: calvix <[email protected]>
---
.../kvm/storage/KVMStorageProcessor.java | 99 +++++++++++++++++-----
.../kvm/storage/KVMStorageProcessorTest.java | 77 +++++++++++++++++
2 files changed, 155 insertions(+), 21 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 37f45522fd5..1fba9f3e96f 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,33 +2339,36 @@ public class KVMStorageProcessor implements
StorageProcessor {
* 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;
+ 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 {
+ closeRbdImage(rbd, image, disk.getName());
+ destroyRadosIoCtx(r, io, disk.getName());
}
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 <pool>/<disk-name>@<snapshot-name> --format json
--mon-host <monitor-host> --id <user> --key <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));
@@ -2652,7 +2655,7 @@ public class KVMStorageProcessor implements
StorageProcessor {
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());
@@ -2662,6 +2665,50 @@ public class KVMStorageProcessor implements
StorageProcessor {
return r;
}
+ /**
+ * Closes an RBD image if it was opened; never throws. 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.
+ */
+ protected void closeRbdImage(Rbd rbd, RbdImage image, String imageName) {
+ if (image == null) {
+ return;
+ }
+ try {
+ rbd.close(image);
+ } catch (final Exception e) {
+ logger.warn("Failed to close RBD image [{}]. The error was: {}",
imageName, e.getMessage(), e);
+ }
+ }
+
+ /** Destroys a RADOS IO context if it was created; never throws. */
+ protected void destroyRadosIoCtx(Rados r, IoCTX io, String
contextDescription) {
+ if (io == null) {
+ return;
+ }
+ try {
+ r.ioCtxDestroy(io);
+ } catch (final Exception e) {
+ logger.warn("Failed to destroy the RADOS IO context used for [{}].
The error was: {}", contextDescription, e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Unprotects an RBD snapshot if it was protected; never throws. A
snapshot left protected cannot
+ * be deleted, and neither can its volume.
+ */
+ protected void unprotectRbdSnapshot(RbdImage image, String snapshotName,
boolean snapProtected) {
+ if (!snapProtected) {
+ return;
+ }
+ try {
+ image.snapUnprotect(snapshotName);
+ } catch (final Exception e) {
+ logger.error("Failed to unprotect RBD snapshot [{}]; it and its
volume cannot be deleted until this is resolved manually. The error was: {}",
+ snapshotName, e.getMessage(), e);
+ }
+ }
+
@Override
public Answer deleteVolume(final DeleteCommand cmd) {
final VolumeObjectTO vol = (VolumeObjectTO)cmd.getData();
@@ -2811,17 +2858,24 @@ public class KVMStorageProcessor implements
StorageProcessor {
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<RbdSnapInfo> snaps = srcImage.snapList();
boolean snapFound = false;
@@ -2837,23 +2891,26 @@ public class KVMStorageProcessor implements
StorageProcessor {
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.
+ closeRbdImage(rbd, diskImage, newUuid);
+ unprotectRbdSnapshot(srcImage, snapshotName, snapProtected);
+ closeRbdImage(rbd, srcImage, volume.getName());
+ destroyRadosIoCtx(r, io, snapshotName);
}
return disk;
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 cc1e38a908b..11d508d1646 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 class KVMStorageProcessorTest {
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> 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> 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);
+ }
+ }
}
