Skip to content

kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph #13835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
calvix wants to merge 3 commits into
base: 4.22
Choose a base branch
from
+154 −21
Open

kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph #13835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f394197
kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph
calvix Aug 10, 2026
3e80146
kvm: release RBD handles on every path when cloning a volume from a s…
calvix Aug 10, 2026
48b5fda
kvm: add regression tests for the RBD snapshot handle leak
calvix Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 77 additions & 21 deletions ...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2339,33 +2339,51 @@ 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;
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());
Comment on lines +2351 to +2353

@DaanHoogland DaanHoogland Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io and image are torn down in nested try-catch constructions in a finally clause of the parent try-catch clause. I think this should be re-structured to be in called methods.

In addition the same pattern happens below. a lot of re-use can be gained,


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;
}

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));
Expand Down Expand Up @@ -2652,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());
Expand Down Expand Up @@ -2811,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<RbdSnapInfo> snaps = srcImage.snapList();
boolean snapFound = false;
Expand All @@ -2837,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;
Expand Down
77 changes: 77 additions & 0 deletions ...pervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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> 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);
}
}
}
Loading