From 26e7c3fbdbed5f084e18d4fce6dc69b80eae4244 Mon Sep 17 00:00:00 2001 From: Wido den Hollander Date: Tue, 8 Sep 2026 14:12:47 +0200 Subject: [PATCH 01/26] kvm: let libvirt remove RBD snapshots on volume delete (#13763) LibvirtStorageAdaptor#deletePhysicalDisk manually connected to Ceph via librados/librbd to unprotect and remove every snapshot of an RBD image before asking libvirt to delete the volume. libvirt's RBD storage backend has supported VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS since 1.2.20, which does the same unprotect/remove internally. Pass that flag instead and drop the manual cleanup. --- .../kvm/storage/LibvirtStorageAdaptor.java | 72 ++++--------------- 1 file changed, 15 insertions(+), 57 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java index 4bfac31b68f9..794b88165aa5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java @@ -55,7 +55,6 @@ import com.ceph.rados.IoCTX; import com.ceph.rados.Rados; -import com.ceph.rados.exceptions.ErrorCode; import com.ceph.rados.exceptions.RadosException; import com.ceph.rbd.Rbd; import com.ceph.rbd.RbdException; @@ -95,6 +94,8 @@ public class LibvirtStorageAdaptor implements StorageAdaptor { private static final int RBD_FEATURE_DEEP_FLATTEN = 32; public static final int RBD_FEATURES = RBD_FEATURE_LAYERING + RBD_FEATURE_EXCLUSIVE_LOCK + RBD_FEATURE_OBJECT_MAP + RBD_FEATURE_FAST_DIFF + RBD_FEATURE_DEEP_FLATTEN; private int rbdOrder = 0; /* Order 0 means 4MB blocks (the default) */ + /* libvirt's VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS, not exposed as a constant by libvirt-java */ + private static final int VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS = 2; private static final Set QEMU_IMG_MANAGED_POOL_TYPES = Set.of(StoragePoolType.NetworkFilesystem, StoragePoolType.Filesystem, StoragePoolType.SharedMountPoint); @@ -1150,61 +1151,6 @@ public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.Imag logger.info("Attempting to remove volume " + uuid + " from pool " + pool.getUuid()); - /** - * RBD volume can have snapshots and while they exist libvirt - * can't remove the RBD volume - * - * We have to remove those snapshots first - */ - if (pool.getType() == StoragePoolType.RBD) { - try { - logger.info("Unprotecting and Removing RBD snapshots of image " + pool.getSourceDir() + "/" + uuid + " prior to removing the image"); - - Rados r = new Rados(pool.getAuthUserName()); - r.confSet("mon_host", pool.getSourceHost() + ":" + pool.getSourcePort()); - r.confSet("key", pool.getAuthSecret()); - r.confSet("client_mount_timeout", "30"); - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet("mon_host")); - - IoCTX io = r.ioCtxCreate(pool.getSourceDir()); - Rbd rbd = new Rbd(io); - RbdImage image = rbd.open(uuid); - logger.debug("Fetching list of snapshots of RBD image " + pool.getSourceDir() + "/" + uuid); - List snaps = image.snapList(); - try { - for (RbdSnapInfo snap : snaps) { - if (image.snapIsProtected(snap.name)) { - logger.debug("Unprotecting snapshot " + pool.getSourceDir() + "/" + uuid + "@" + snap.name); - image.snapUnprotect(snap.name); - } else { - logger.debug("Snapshot " + pool.getSourceDir() + "/" + uuid + "@" + snap.name + " is not protected."); - } - logger.debug("Removing snapshot " + pool.getSourceDir() + "/" + uuid + "@" + snap.name); - image.snapRemove(snap.name); - } - logger.info("Successfully unprotected and removed any remaining snapshots (" + snaps.size() + ") of " - + pool.getSourceDir() + "/" + uuid + " Continuing to remove the RBD image"); - } catch (RbdException e) { - logger.error("Failed to remove snapshot with exception: " + e.toString() + - ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); - throw new CloudRuntimeException(e.toString() + " - " + ErrorCode.getErrorMessage(e.getReturnValue())); - } finally { - logger.debug("Closing image and destroying context"); - rbd.close(image); - r.ioCtxDestroy(io); - } - } catch (RadosException e) { - logger.error("Failed to remove snapshot with exception: " + e.toString() + - ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); - throw new CloudRuntimeException(e.toString() + " - " + ErrorCode.getErrorMessage(e.getReturnValue())); - } catch (RbdException e) { - logger.error("Failed to remove snapshot with exception: " + e.toString() + - ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); - throw new CloudRuntimeException(e.toString() + " - " + ErrorCode.getErrorMessage(e.getReturnValue())); - } - } - LibvirtStoragePool libvirtPool = (LibvirtStoragePool)pool; try { StorageVol vol = getVolume(libvirtPool.getPool(), uuid); @@ -1721,7 +1667,19 @@ private void refreshPool(StoragePool pool) throws LibvirtException { } private void deleteVol(LibvirtStoragePool pool, StorageVol vol) throws LibvirtException { - vol.delete(0); + /** + * RBD volumes can have snapshots, and libvirt refuses to remove a volume while + * they exist. VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS tells the RBD storage backend + * to unprotect and remove any snapshots before removing the volume itself. + * + * libvirt-java has no named constant for this flag (added upstream in libvirt 1.2.20, + * commit 3c7590e0a4), so it's passed as a raw flag value here. + */ + int flags = 0; + if (pool.getType() == StoragePoolType.RBD) { + flags |= VIR_STORAGE_VOL_DELETE_WITH_SNAPSHOTS; + } + vol.delete(flags); } From b4e123d801a96cec004333a87660cea2f206b51b Mon Sep 17 00:00:00 2001 From: calvix Date: Tue, 8 Sep 2026 14:12:07 +0200 Subject: [PATCH 02/26] 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 ") 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: 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> --- .../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 11acb9546b53..fe297adb3270 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 @@ -2568,33 +2568,36 @@ 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 { + 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 /@ --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)); @@ -2881,7 +2884,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()); @@ -2891,6 +2894,50 @@ private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosExcepti 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(); @@ -3046,17 +3093,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; @@ -3072,23 +3126,26 @@ 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. + 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 0a58dfc79b0d..daa8792ed8f7 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; @@ -110,6 +115,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 @@ -666,4 +676,71 @@ public void testComputeMd5Hash_ConsistentResults() { Assert.fail("Failed to test computeMd5Hash: " + e.getMessage()); } } + + /** + * 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); + } + } } From f5e891f9d996c2777f39cd3df13bf2e93c2cc104 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:14:49 +0530 Subject: [PATCH 03/26] kvm: fix restore-and-attach of a backed up volume (#14007) Restoring a volume from a backup and attaching it to a VM has been broken since the restore commands were changed to run without a shell, in three independent ways. getDeviceToAttachDisk pipes virsh domblklist through awk, but passes the awk program still wrapped in the single quotes a shell would have stripped. Run directly, awk fails with "invalid char" and returns nothing, so the device name is empty and charAt throws StringIndexOutOfBoundsException before any attach is attempted. This affects every storage type. The exit value was also never checked, and the output not trimmed, so even a working awk would leave the trailing line separator and increment that instead of the device letter. The RBD branch passes the literal string "< virshCmd = new ArrayList<>(); virshCmd.add(Script.getExecutableAbsolutePath("virsh")); - if (volumePool.getPoolType() == Storage.StoragePoolType.RBD) { - String xmlForRbdDisk = getXmlForRbdDisk(storagePoolMgr, volumePool, volumePath, deviceToAttachDiskTo); - logger.debug("RBD disk xml to attach: {}", xmlForRbdDisk); - virshCmd.add("attach-device"); - virshCmd.add(vmName); - virshCmd.add("/dev/stdin"); - virshCmd.add("< result = Script.executePipedCommands(Arrays.asList(domblkCmd, tailCmd, headCmd, awkCmd), 0); - String currentDevice = result.second(); + // executePipedCommands appends a line separator to every line it reads, so the device + // name has to be trimmed before the last character can be incremented. + String currentDevice = result.second() == null ? "" : result.second().trim(); + if (result.first() == null || result.first() != 0 || StringUtils.isBlank(currentDevice)) { + throw new CloudRuntimeException(String.format("Failed to determine the device to attach the restored volume to on VM [%s].", vmName)); + } char lastChar = currentDevice.charAt(currentDevice.length() - 1); char incrementedChar = (char) (lastChar + 1); return currentDevice.substring(0, currentDevice.length() - 1) + incrementedChar; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java index fc2341632984..2fc9b43c12b1 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java @@ -25,9 +25,11 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.lang.reflect.Method; import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; +import java.util.List; import org.apache.cloudstack.backup.BackupAnswer; import org.apache.cloudstack.backup.RestoreBackupCommand; @@ -42,8 +44,11 @@ import com.cloud.agent.api.Answer; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; import com.cloud.storage.Storage; import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.Script; import com.cloud.vm.VirtualMachine; @@ -581,4 +586,123 @@ public void testExecuteWithMultipleVolumes() throws Exception { } } } + + private String invokeGetDeviceToAttachDisk(String vmName) throws Exception { + Method method = LibvirtRestoreBackupCommandWrapper.class.getDeclaredMethod("getDeviceToAttachDisk", String.class); + method.setAccessible(true); + try { + return (String) method.invoke(wrapper, vmName); + } catch (java.lang.reflect.InvocationTargetException e) { + throw (Exception) e.getCause(); + } + } + + private String[] captureAttachCommand(Storage.StoragePoolType poolType) throws Exception { + PrimaryDataStoreTO volumePool = Mockito.mock(PrimaryDataStoreTO.class); + lenient().when(volumePool.getPoolType()).thenReturn(poolType); + lenient().when(volumePool.getHost()).thenReturn("10.0.0.1"); + lenient().when(volumePool.getUuid()).thenReturn("pool-uuid"); + KVMStoragePoolManager storagePoolMgr = Mockito.mock(KVMStoragePoolManager.class); + KVMStoragePool primaryPool = Mockito.mock(KVMStoragePool.class); + lenient().when(storagePoolMgr.getStoragePool(any(), anyString())).thenReturn(primaryPool); + lenient().when(primaryPool.getAuthUserName()).thenReturn("cloudstack"); + + Method method = LibvirtRestoreBackupCommandWrapper.class.getDeclaredMethod("attachVolumeToVm", + KVMStoragePoolManager.class, String.class, PrimaryDataStoreTO.class, String.class); + method.setAccessible(true); + + final String[][] captured = new String[1][]; + try (MockedStatic