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..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,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()); 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 /@ --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)); @@ -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()); @@ -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 snaps = srcImage.snapList(); boolean snapFound = false; @@ -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; 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); + } + } }