Skip to content

Commit c860e0c

Browse files
calvixcalvix
authored
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: 30d3066 ("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 <7136358+calvix@users.noreply.github.com>
1 parent cdbda0a commit c860e0c

2 files changed

Lines changed: 155 additions & 21 deletions

File tree

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java‎

Lines changed: 78 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2339,33 +2339,36 @@ private CreateObjectAnswer takeClvmVolumeSnapshotOfStoppedVm(KVMPhysicalDisk dis
23392339
* barriers properly (>2.6.32) this won't be any different then pulling the power
23402340
* cord out of a running machine.
23412341
*/
2342-
private Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhysicalDisk disk, String snapshotName) {
2342+
protected Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMPhysicalDisk disk, String snapshotName) {
23432343
Long snapshotSize = null;
2344+
Rados r = null;
2345+
IoCTX io = null;
2346+
Rbd rbd = null;
2347+
RbdImage image = null;
23442348
try {
2345-
Rados r = radosConnect(primaryPool);
2349+
r = radosConnect(primaryPool);
23462350

2347-
final IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
2348-
final Rbd rbd = new Rbd(io);
2349-
final RbdImage image = rbd.open(disk.getName());
2351+
io = r.ioCtxCreate(primaryPool.getSourceDir());
2352+
rbd = new Rbd(io);
2353+
image = rbd.open(disk.getName());
23502354

23512355
logger.debug("Attempting to create RBD snapshot {}@{}", disk.getName(), snapshotName);
23522356
image.snapCreate(snapshotName);
23532357

2354-
image.snapCreate(snapshotName);
23552358
long rbdSnapshotSize = getRbdSnapshotSize(primaryPool.getSourceDir(), disk.getName(), snapshotName, primaryPool.getSourceHost(), primaryPool.getAuthUserName(), primaryPool.getAuthSecret());
23562359
if (rbdSnapshotSize > 0) {
23572360
snapshotSize = rbdSnapshotSize;
23582361
}
2359-
2360-
rbd.close(image);
2361-
r.ioCtxDestroy(io);
23622362
} catch (final Exception e) {
23632363
logger.error("A RBD snapshot operation on [{}] failed. The error was: {}", disk.getName(), e.getMessage(), e);
2364+
} finally {
2365+
closeRbdImage(rbd, image, disk.getName());
2366+
destroyRadosIoCtx(r, io, disk.getName());
23642367
}
23652368
return snapshotSize;
23662369
}
23672370

2368-
private long getRbdSnapshotSize(String poolPath, String diskName, String snapshotName, String rbdMonitor, String authUser, String authSecret) {
2371+
protected long getRbdSnapshotSize(String poolPath, String diskName, String snapshotName, String rbdMonitor, String authUser, String authSecret) {
23692372
logger.debug("Get RBD snapshot size for {}/{}@{}", poolPath, diskName, snapshotName);
23702373
//cmd: rbd du <pool>/<disk-name>@<snapshot-name> --format json --mon-host <monitor-host> --id <user> --key <key> 2>/dev/null
23712374
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 @@ protected boolean isAvailablePoolSizeDividedByDiskSizeLesserThanMinRate(long ava
26522655
return ((availablePoolSize * 1d) / (diskSize * 1d)) < MIN_RATE_BETWEEN_AVAILABLE_POOL_AND_DISK_SIZE_TO_TAKE_DISK_SNAPSHOT;
26532656
}
26542657

2655-
private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosException {
2658+
protected Rados radosConnect(final KVMStoragePool primaryPool) throws RadosException {
26562659
Rados r = new Rados(primaryPool.getAuthUserName());
26572660
r.confSet(CEPH_MON_HOST, primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort());
26582661
r.confSet(CEPH_AUTH_KEY, primaryPool.getAuthSecret());
@@ -2662,6 +2665,50 @@ private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosExcepti
26622665
return r;
26632666
}
26642667

2668+
/**
2669+
* Closes an RBD image if it was opened; never throws. An image left open keeps this client's RBD
2670+
* exclusive-lock, which later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS and keeps
2671+
* the image busy so it cannot be removed.
2672+
*/
2673+
protected void closeRbdImage(Rbd rbd, RbdImage image, String imageName) {
2674+
if (image == null) {
2675+
return;
2676+
}
2677+
try {
2678+
rbd.close(image);
2679+
} catch (final Exception e) {
2680+
logger.warn("Failed to close RBD image [{}]. The error was: {}", imageName, e.getMessage(), e);
2681+
}
2682+
}
2683+
2684+
/** Destroys a RADOS IO context if it was created; never throws. */
2685+
protected void destroyRadosIoCtx(Rados r, IoCTX io, String contextDescription) {
2686+
if (io == null) {
2687+
return;
2688+
}
2689+
try {
2690+
r.ioCtxDestroy(io);
2691+
} catch (final Exception e) {
2692+
logger.warn("Failed to destroy the RADOS IO context used for [{}]. The error was: {}", contextDescription, e.getMessage(), e);
2693+
}
2694+
}
2695+
2696+
/**
2697+
* Unprotects an RBD snapshot if it was protected; never throws. A snapshot left protected cannot
2698+
* be deleted, and neither can its volume.
2699+
*/
2700+
protected void unprotectRbdSnapshot(RbdImage image, String snapshotName, boolean snapProtected) {
2701+
if (!snapProtected) {
2702+
return;
2703+
}
2704+
try {
2705+
image.snapUnprotect(snapshotName);
2706+
} catch (final Exception e) {
2707+
logger.error("Failed to unprotect RBD snapshot [{}]; it and its volume cannot be deleted until this is resolved manually. The error was: {}",
2708+
snapshotName, e.getMessage(), e);
2709+
}
2710+
}
2711+
26652712
@Override
26662713
public Answer deleteVolume(final DeleteCommand cmd) {
26672714
final VolumeObjectTO vol = (VolumeObjectTO)cmd.getData();
@@ -2811,17 +2858,24 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S
28112858
disk.setSize(size > volume.getVirtualSize() ? size : volume.getVirtualSize());
28122859
disk.setVirtualSize(size > volume.getVirtualSize() ? size : disk.getSize());
28132860

2861+
Rados r = null;
2862+
IoCTX io = null;
2863+
Rbd rbd = null;
2864+
RbdImage srcImage = null;
2865+
RbdImage diskImage = null;
2866+
boolean snapProtected = false;
2867+
28142868
try {
28152869

2816-
Rados r = new Rados(srcPool.getAuthUserName());
2870+
r = new Rados(srcPool.getAuthUserName());
28172871
r.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort());
28182872
r.confSet("key", srcPool.getAuthSecret());
28192873
r.confSet("client_mount_timeout", "30");
28202874
r.connect();
28212875

2822-
IoCTX io = r.ioCtxCreate(srcPool.getSourceDir());
2823-
Rbd rbd = new Rbd(io);
2824-
RbdImage srcImage = rbd.open(volume.getName());
2876+
io = r.ioCtxCreate(srcPool.getSourceDir());
2877+
rbd = new Rbd(io);
2878+
srcImage = rbd.open(volume.getName());
28252879

28262880
List<RbdSnapInfo> snaps = srcImage.snapList();
28272881
boolean snapFound = false;
@@ -2837,23 +2891,26 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S
28372891
return null;
28382892
}
28392893
srcImage.snapProtect(snapshotName);
2894+
snapProtected = true;
28402895

28412896
logger.debug(String.format("Try to clone snapshot %s on RBD", snapshotName));
28422897
rbd.clone(volume.getName(), snapshotName, io, disk.getName(), LibvirtStorageAdaptor.RBD_FEATURES, 0);
2843-
RbdImage diskImage = rbd.open(disk.getName());
2898+
diskImage = rbd.open(disk.getName());
28442899
if (disk.getVirtualSize() > volume.getVirtualSize()) {
28452900
diskImage.resize(disk.getVirtualSize());
28462901
}
28472902

28482903
diskImage.flatten();
2849-
rbd.close(diskImage);
2850-
2851-
srcImage.snapUnprotect(snapshotName);
2852-
rbd.close(srcImage);
2853-
r.ioCtxDestroy(io);
28542904
} catch (RadosException | RbdException e) {
28552905
logger.error(String.format("Failed due to %s", e.getMessage()), e);
28562906
disk = null;
2907+
} finally {
2908+
// Every handle has to be released on all paths, including the "snapshot not found" return and
2909+
// any failure of clone/resize/flatten.
2910+
closeRbdImage(rbd, diskImage, newUuid);
2911+
unprotectRbdSnapshot(srcImage, snapshotName, snapProtected);
2912+
closeRbdImage(rbd, srcImage, volume.getName());
2913+
destroyRadosIoCtx(r, io, snapshotName);
28572914
}
28582915

28592916
return disk;

‎plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessorTest.java‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@
1818
*/
1919
package com.cloud.hypervisor.kvm.storage;
2020

21+
import com.ceph.rados.IoCTX;
22+
import com.ceph.rados.Rados;
23+
import com.ceph.rbd.Rbd;
24+
import com.ceph.rbd.RbdException;
25+
import com.ceph.rbd.RbdImage;
2126
import com.cloud.exception.InternalErrorException;
2227
import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
2328
import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
@@ -108,6 +113,11 @@ public class KVMStorageProcessorTest {
108113
private static final String directDownloadTemporaryPath = "/var/lib/libvirt/images/dd";
109114
private static final long templateSize = 80000L;
110115

116+
private static final String RBD_POOL_NAME = "cloudstack";
117+
private static final String RBD_IMAGE_NAME = "b7a1f0a9-0f0e-4a1a-9a35-1c1a2e0f1b5e";
118+
private static final String SNAPSHOT_NAME = "8f1c1f0b-9d3e-4c2a-8a3d-6f0b2c9e1d47";
119+
private static final long SNAPSHOT_SIZE = 196624L;
120+
111121
private AutoCloseable closeable;
112122

113123
@Before
@@ -499,4 +509,71 @@ public void getDiskLabelToSnapshotTestDiskMatches() throws LibvirtException {
499509

500510
Assert.assertEquals("vda", result);
501511
}
512+
513+
/**
514+
* Wires a mocked Ceph stack for {@link KVMStorageProcessor#takeRbdVolumeSnapshotOfStoppedVm} and returns the
515+
* mocked disk. The Rbd instance is created inside the method under test, so it is mocked by construction.
516+
*/
517+
private KVMPhysicalDisk prepareRbdSnapshotMocks(Rados radosMock, IoCTX ioCtxMock) throws Exception {
518+
KVMPhysicalDisk diskMock = Mockito.mock(KVMPhysicalDisk.class);
519+
Mockito.lenient().doReturn(RBD_IMAGE_NAME).when(diskMock).getName();
520+
521+
Mockito.lenient().doReturn(RBD_POOL_NAME).when(kvmStoragePoolMock).getSourceDir();
522+
Mockito.lenient().doReturn("10.0.0.1").when(kvmStoragePoolMock).getSourceHost();
523+
Mockito.lenient().doReturn("cloudstack").when(kvmStoragePoolMock).getAuthUserName();
524+
Mockito.lenient().doReturn("secret").when(kvmStoragePoolMock).getAuthSecret();
525+
526+
Mockito.doReturn(radosMock).when(storageProcessorSpy).radosConnect(kvmStoragePoolMock);
527+
Mockito.doReturn(ioCtxMock).when(radosMock).ioCtxCreate(RBD_POOL_NAME);
528+
Mockito.lenient().doReturn(SNAPSHOT_SIZE).when(storageProcessorSpy).getRbdSnapshotSize(Mockito.anyString(), Mockito.anyString(),
529+
Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), Mockito.anyString());
530+
531+
return diskMock;
532+
}
533+
534+
/**
535+
* A duplicated snapCreate call used to throw "snapshot already exists" on every single RBD snapshot, which then
536+
* skipped the cleanup below and leaked the image's exclusive-lock.
537+
*/
538+
@Test
539+
public void takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce() throws Exception {
540+
Rados radosMock = Mockito.mock(Rados.class);
541+
IoCTX ioCtxMock = Mockito.mock(IoCTX.class);
542+
RbdImage rbdImageMock = Mockito.mock(RbdImage.class);
543+
KVMPhysicalDisk diskMock = prepareRbdSnapshotMocks(radosMock, ioCtxMock);
544+
545+
try (MockedConstruction<Rbd> rbd = Mockito.mockConstruction(Rbd.class, ((mock, context) ->
546+
Mockito.doReturn(rbdImageMock).when(mock).open(RBD_IMAGE_NAME)))) {
547+
548+
Long result = storageProcessorSpy.takeRbdVolumeSnapshotOfStoppedVm(kvmStoragePoolMock, diskMock, SNAPSHOT_NAME);
549+
550+
Assert.assertEquals(Long.valueOf(SNAPSHOT_SIZE), result);
551+
Mockito.verify(rbdImageMock, Mockito.times(1)).snapCreate(SNAPSHOT_NAME);
552+
Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock);
553+
Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock);
554+
}
555+
}
556+
557+
/**
558+
* While the image stays open this client holds the RBD exclusive-lock, and a later 'rbd snap rollback'
559+
* (revertSnapshot) from another host fails with EROFS. The handles must be released even when the snapshot fails.
560+
*/
561+
@Test
562+
public void takeRbdVolumeSnapshotOfStoppedVmTestReleasesHandlesWhenSnapshotFails() throws Exception {
563+
Rados radosMock = Mockito.mock(Rados.class);
564+
IoCTX ioCtxMock = Mockito.mock(IoCTX.class);
565+
RbdImage rbdImageMock = Mockito.mock(RbdImage.class);
566+
KVMPhysicalDisk diskMock = prepareRbdSnapshotMocks(radosMock, ioCtxMock);
567+
Mockito.doThrow(new RbdException("Failed to create snapshot")).when(rbdImageMock).snapCreate(SNAPSHOT_NAME);
568+
569+
try (MockedConstruction<Rbd> rbd = Mockito.mockConstruction(Rbd.class, ((mock, context) ->
570+
Mockito.doReturn(rbdImageMock).when(mock).open(RBD_IMAGE_NAME)))) {
571+
572+
Long result = storageProcessorSpy.takeRbdVolumeSnapshotOfStoppedVm(kvmStoragePoolMock, diskMock, SNAPSHOT_NAME);
573+
574+
Assert.assertNull(result);
575+
Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock);
576+
Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock);
577+
}
578+
}
502579
}

0 commit comments

Comments
 (0)