From 180d29d0b689c44332f982c500cdba2b1edf714d Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 10:55:50 +0000 Subject: [PATCH 1/5] kvm: bound Ceph operations and release cluster handles deterministically A clone from a template opens the template with rbd_open(). librados leaves rados_osd_op_timeout and rados_mon_op_timeout at 0, which means an operation waits forever, so a completion that never arrives parks the agent thread that issued it for the life of the process. The agent runs a host's storage commands in sequence, so every command queued behind that thread stops with it. Nothing released the cluster handles either. Rados only calls rados_shutdown() from finalize(), so a handle the KVM plugin opened stayed alive until the garbage collector happened to reach it, and then shut down on the finalizer thread rather than on the thread that did the work. Several paths also leaked the IO context and the open image when a call failed part way through, because the clean up ran only on the success path, and the snapshot backup never closed its image at all. Add CephUtil, which opens a connected handle with the timeouts applied and offers non-throwing helpers to close an image, destroy an IO context and shut a handle down. Route every handle in the plugin through it and clean up from finally blocks, so that each path releases what it opened. The timeouts bound a single operation rather than a whole request, so a long copy or flatten is made up of many operations that are each well inside the limit. They are exposed as rados.osd.op.timeout and rados.mon.op.timeout in agent.properties; setting either to 0 restores the previous behaviour of waiting forever. Signed-off-by: Brad House --- .../agent/properties/AgentProperties.java | 25 ++ .../LibvirtBackupSnapshotCommandWrapper.java | 25 +- .../LibvirtManageSnapshotCommandWrapper.java | 25 +- .../LibvirtRevertSnapshotCommandWrapper.java | 44 +-- .../hypervisor/kvm/storage/CephUtil.java | 151 ++++++++++ .../kvm/storage/KVMStorageProcessor.java | 39 +-- .../kvm/storage/LibvirtStorageAdaptor.java | 257 +++++++++--------- 7 files changed, 376 insertions(+), 190 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java index 1cb9232eec14..04b0657209d0 100644 --- a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java +++ b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java @@ -914,6 +914,31 @@ public Property getWorkers() { * */ public static final Property INCREMENTAL_SNAPSHOT_RETRY_REBASE_WAIT = new Property<>("incremental.snapshot.retry.rebase.wait", 60); + /** + * Time, in seconds, that a single Ceph OSD operation may block before it fails.
+ * This bounds each operation, not a whole request, so a long running copy or flatten is made up of + * many operations that are each well inside the limit.
+ * Set to 0 to keep the librados default of waiting forever.
+ * Data type: Integer.
+ * Default value: 60 + */ + public static final Property RADOS_OSD_OP_TIMEOUT = new Property<>("rados.osd.op.timeout", 60); + + /** + * Time, in seconds, that a single Ceph monitor operation may block before it fails.
+ * Set to 0 to keep the librados default of waiting forever.
+ * Data type: Integer.
+ * Default value: 30 + */ + public static final Property RADOS_MON_OP_TIMEOUT = new Property<>("rados.mon.op.timeout", 30); + + /** + * Time, in seconds, that the Ceph client may spend connecting to the monitors.
+ * Data type: Integer.
+ * Default value: 30 + */ + public static final Property RADOS_CLIENT_MOUNT_TIMEOUT = new Property<>("rados.client.mount.timeout", 30); + public static class Property { private String name; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java index 964e6591878c..3b4856b085de 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java @@ -41,6 +41,7 @@ import com.cloud.agent.api.BackupSnapshotAnswer; import com.cloud.agent.api.BackupSnapshotCommand; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.CephUtil; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; @@ -96,17 +97,16 @@ public Answer execute(final BackupSnapshotCommand command, final LibvirtComputin * cmds.timeout */ if (primaryPool.getType() == StoragePoolType.RBD) { + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; try { - final Rados r = new Rados(primaryPool.getAuthUserName()); - r.confSet("mon_host", primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort()); - r.confSet("key", primaryPool.getAuthSecret()); - r.confSet("client_mount_timeout", "30"); - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet("mon_host")); - - final IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir()); - final Rbd rbd = new Rbd(io); - final RbdImage image = rbd.open(snapshotDisk.getName(), snapshotName); + r = CephUtil.connect(primaryPool.getAuthUserName(), primaryPool.getSourceHost(), primaryPool.getSourcePort(), primaryPool.getAuthSecret()); + + io = r.ioCtxCreate(primaryPool.getSourceDir()); + rbd = new Rbd(io); + image = rbd.open(snapshotDisk.getName(), snapshotName); final File fh = new File(snapshotDestPath); try(BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(fh));) { final int chunkSize = 4194304; @@ -126,13 +126,16 @@ public Answer execute(final BackupSnapshotCommand command, final LibvirtComputin { logger.error("BackupSnapshotAnswer:Exception:"+ ex.getMessage()); } - r.ioCtxDestroy(io); } catch (final RadosException e) { logger.error("A RADOS operation failed. The error was: " + e.getMessage()); return new BackupSnapshotAnswer(command, false, e.toString(), null, true); } catch (final RbdException e) { logger.error("A RBD operation on " + snapshotDisk.getName() + " failed. The error was: " + e.getMessage()); return new BackupSnapshotAnswer(command, false, e.toString(), null, true); + } finally { + CephUtil.closeQuietly(rbd, image, snapshotDisk.getName()); + CephUtil.ioCtxDestroyQuietly(r, io); + CephUtil.shutDownQuietly(r); } } else { final Script scriptCommand = new Script(manageSnapshotPath, cmdsTimeout, logger); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java index ec900e9981e0..ebd7351b1c21 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java @@ -37,6 +37,7 @@ import com.cloud.agent.api.ManageSnapshotCommand; import com.cloud.agent.api.to.StorageFilerTO; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.CephUtil; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; @@ -113,17 +114,16 @@ public Answer execute(final ManageSnapshotCommand command, final LibvirtComputin * cord out of a running machine. */ if (primaryPool.getType() == StoragePoolType.RBD) { + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; try { - final Rados r = new Rados(primaryPool.getAuthUserName()); - r.confSet("mon_host", primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort()); - r.confSet("key", primaryPool.getAuthSecret()); - r.confSet("client_mount_timeout", "30"); - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet("mon_host")); + r = CephUtil.connect(primaryPool.getAuthUserName(), primaryPool.getSourceHost(), primaryPool.getSourcePort(), primaryPool.getAuthSecret()); - 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()); if (command.getCommandSwitch().equalsIgnoreCase(ManageSnapshotCommand.CREATE_SNAPSHOT)) { logger.debug("Attempting to create RBD snapshot " + disk.getName() + "@" + snapshotName); @@ -132,11 +132,12 @@ public Answer execute(final ManageSnapshotCommand command, final LibvirtComputin logger.debug("Attempting to remove RBD snapshot " + disk.getName() + "@" + snapshotName); image.snapRemove(snapshotName); } - - rbd.close(image); - r.ioCtxDestroy(io); } catch (final Exception e) { logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage()); + } finally { + CephUtil.closeQuietly(rbd, image, disk.getName()); + CephUtil.ioCtxDestroyQuietly(r, io); + CephUtil.shutDownQuietly(r); } } else { /* VM is not running, create a snapshot by ourself */ diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java index 5d76d140f229..3f8e85ef053c 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java @@ -43,6 +43,7 @@ import com.cloud.agent.api.to.DataStoreTO; import com.cloud.agent.api.to.NfsTO; import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.CephUtil; import com.cloud.hypervisor.kvm.storage.KVMPhysicalDisk; import com.cloud.hypervisor.kvm.storage.KVMStoragePool; import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; @@ -92,26 +93,29 @@ public Answer execute(final RevertSnapshotCommand command, final LibvirtComputin KVMStoragePool primaryPool = snapshotDisk.getPool(); if (primaryPool.getType() == StoragePoolType.RBD) { - Rados rados = new Rados(primaryPool.getAuthUserName()); - rados.confSet(MON_HOST, primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort()); - rados.confSet(KEY, primaryPool.getAuthSecret()); - rados.confSet(CLIENT_MOUNT_TIMEOUT, RADOS_CONNECTION_TIMEOUT); - rados.connect(); - - String[] rbdPoolAndVolumeAndSnapshot = snapshotRelPath.split("/"); - int snapshotIndex = rbdPoolAndVolumeAndSnapshot.length - 1; - String rbdSnapshotId = rbdPoolAndVolumeAndSnapshot[snapshotIndex]; - - IoCTX io = rados.ioCtxCreate(primaryPool.getSourceDir()); - Rbd rbd = new Rbd(io); - - logger.debug(String.format("Attempting to rollback RBD snapshot [name:%s], [volumeid:%s], [snapshotid:%s]", snapshot.getName(), volumePath, rbdSnapshotId)); - - RbdImage image = rbd.open(volumePath); - image.snapRollBack(rbdSnapshotId); - - rbd.close(image); - rados.ioCtxDestroy(io); + Rados rados = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; + try { + rados = CephUtil.connect(primaryPool.getAuthUserName(), primaryPool.getSourceHost(), primaryPool.getSourcePort(), primaryPool.getAuthSecret()); + + String[] rbdPoolAndVolumeAndSnapshot = snapshotRelPath.split("/"); + int snapshotIndex = rbdPoolAndVolumeAndSnapshot.length - 1; + String rbdSnapshotId = rbdPoolAndVolumeAndSnapshot[snapshotIndex]; + + io = rados.ioCtxCreate(primaryPool.getSourceDir()); + rbd = new Rbd(io); + + logger.debug(String.format("Attempting to rollback RBD snapshot [name:%s], [volumeid:%s], [snapshotid:%s]", snapshot.getName(), volumePath, rbdSnapshotId)); + + image = rbd.open(volumePath); + image.snapRollBack(rbdSnapshotId); + } finally { + CephUtil.closeQuietly(rbd, image, volumePath); + CephUtil.ioCtxDestroyQuietly(rados, io); + CephUtil.shutDownQuietly(rados); + } } else { if (snapshotImageStore != null && DataStoreRole.Primary != snapshotImageStore.getRole()) { secondaryStoragePool = storagePoolMgr.getStoragePoolByURI(snapshotImageStore.getUrl()); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java new file mode 100644 index 000000000000..c49612c5b71d --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java @@ -0,0 +1,151 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.hypervisor.kvm.storage; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import com.ceph.rados.IoCTX; +import com.ceph.rados.Rados; +import com.ceph.rados.exceptions.RadosException; +import com.ceph.rbd.Rbd; +import com.ceph.rbd.RbdImage; +import com.cloud.agent.properties.AgentProperties; +import com.cloud.agent.properties.AgentPropertiesFileHandler; + +/** + * Helpers for the Ceph cluster handles the KVM agent opens. + * + * Callers get two things here that are easy to leave out by hand: + * + *
    + *
  • Bounded operations. librados defaults rados_osd_op_timeout and + * rados_mon_op_timeout to 0, which means an operation waits forever. A completion that never + * arrives parks the calling agent thread for the life of the process, and because the agent runs storage + * commands for a host in sequence, every command queued behind it stops with it.
    • + *
  • Deterministic clean up. {@link Rados} releases its native cluster handle from + * finalize(), so a handle that is not shut down explicitly survives until the garbage + * collector happens to reach it, at which point rados_shutdown runs on the finalizer + * thread rather than on the thread that did the work.
    • + *
+ */ +public final class CephUtil { + + protected static Logger logger = LogManager.getLogger(CephUtil.class); + + private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout"; + private static final String RADOS_OSD_OP_TIMEOUT = "rados_osd_op_timeout"; + private static final String RADOS_MON_OP_TIMEOUT = "rados_mon_op_timeout"; + + private CephUtil() { + } + + /** + * Opens a connected cluster handle with the agent's configured timeouts applied. + * + * @param authUserName the cephx user + * @param monHost monitor host + * @param monPort monitor port + * @param authSecret the cephx secret + * @return a connected handle the caller must pass to {@link #shutDownQuietly(Rados)} when done + */ + public static Rados connect(String authUserName, String monHost, int monPort, String authSecret) throws RadosException { + return connect(authUserName, monHost, monPort, authSecret, null); + } + + /** + * Opens a connected cluster handle with the agent's configured timeouts applied, optionally placing new + * images' data in a separate pool. + * + * @param dataPool the RBD data pool for images created on this handle, or null to leave it unset + */ + public static Rados connect(String authUserName, String monHost, int monPort, String authSecret, String dataPool) throws RadosException { + Rados r = new Rados(authUserName); + r.confSet("mon_host", monHost + ":" + monPort); + r.confSet("key", authSecret); + applyTimeouts(r); + if (dataPool != null) { + logger.debug("Setting RBD data pool to [{}] for images created on this connection.", dataPool); + r.confSet(KVMPhysicalDisk.RBD_DEFAULT_DATA_POOL, dataPool); + } + r.connect(); + logger.debug("Successfully connected to Ceph cluster at [{}].", r.confGet("mon_host")); + return r; + } + + /** + * Applies the connect and operation timeouts from the agent properties. A timeout configured as 0 is + * left unset, which keeps the librados default of waiting forever. + */ + public static void applyTimeouts(Rados r) throws RadosException { + r.confSet(CLIENT_MOUNT_TIMEOUT, String.valueOf(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_CLIENT_MOUNT_TIMEOUT))); + + int osdOpTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_OSD_OP_TIMEOUT); + if (osdOpTimeout > 0) { + r.confSet(RADOS_OSD_OP_TIMEOUT, String.valueOf(osdOpTimeout)); + } + + int monOpTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_MON_OP_TIMEOUT); + if (monOpTimeout > 0) { + r.confSet(RADOS_MON_OP_TIMEOUT, String.valueOf(monOpTimeout)); + } + } + + /** + * Releases the native cluster handle. Safe to call with null and on a handle that never connected, so it + * can be used from a finally block without guarding the happy path. + */ + public static void shutDownQuietly(Rados r) { + if (r == null) { + return; + } + try { + r.shutDown(); + } catch (Exception e) { + logger.warn("Failed to shut down the Ceph connection, it will be released when the handle is collected.", e); + } + } + + /** + * Destroys an IO context. Safe to call with nulls so it can be used from a finally block. + */ + public static void ioCtxDestroyQuietly(Rados r, IoCTX io) { + if (r == null || io == null) { + return; + } + try { + r.ioCtxDestroy(io); + } catch (Exception e) { + logger.warn("Failed to destroy the Ceph IO context.", e); + } + } + + /** + * Closes an RBD image without throwing, so that a failure to close cannot discard a successful result or + * mask an exception that is already on its way out. + */ + public static void closeQuietly(Rbd rbd, RbdImage image, String imageName) { + if (rbd == null || image == null) { + return; + } + try { + rbd.close(image); + } catch (Exception e) { + logger.warn("Failed to close RBD image [{}].", imageName, e); + } + } +} 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 1fba9f3e96f0..58b91e2b9dbf 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 @@ -2364,6 +2364,7 @@ protected Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMP } finally { closeRbdImage(rbd, image, disk.getName()); destroyRadosIoCtx(r, io, disk.getName()); + shutDownRados(r); } return snapshotSize; } @@ -2656,13 +2657,12 @@ protected boolean isAvailablePoolSizeDividedByDiskSizeLesserThanMinRate(long ava } 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()); - r.confSet(CEPH_CLIENT_MOUNT_TIMEOUT, CEPH_DEFAULT_MOUNT_TIMEOUT); - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet(CEPH_MON_HOST)); - return r; + return CephUtil.connect(primaryPool.getAuthUserName(), primaryPool.getSourceHost(), primaryPool.getSourcePort(), primaryPool.getAuthSecret()); + } + + /** Releases a RADOS cluster handle if it was opened; never throws. */ + protected void shutDownRados(Rados r) { + CephUtil.shutDownQuietly(r); } /** @@ -2867,11 +2867,7 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S try { - 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(); + r = radosConnect(srcPool); io = r.ioCtxCreate(srcPool.getSourceDir()); rbd = new Rbd(io); @@ -2911,6 +2907,7 @@ private KVMPhysicalDisk createRBDvolumeFromRBDSnapshot(KVMPhysicalDisk volume, S unprotectRbdSnapshot(srcImage, snapshotName, snapProtected); closeRbdImage(rbd, srcImage, volume.getName()); destroyRadosIoCtx(r, io, snapshotName); + shutDownRados(r); } return disk; @@ -2930,11 +2927,16 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { if (primaryPool.getType() == StoragePoolType.RBD) { KVMPhysicalDisk disk = storagePoolMgr.getPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), volume.getPath()); snapshotFullName = disk.getName() + "@" + snapshotName; - Rados r = radosConnect(primaryPool); - IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir()); - Rbd rbd = new Rbd(io); - RbdImage image = rbd.open(disk.getName()); + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; try { + r = radosConnect(primaryPool); + io = r.ioCtxCreate(primaryPool.getSourceDir()); + rbd = new Rbd(io); + image = rbd.open(disk.getName()); + logger.info("Attempting to remove RBD snapshot " + snapshotFullName); if (image.snapIsProtected(snapshotName)) { logger.debug("Unprotecting RBD snapshot " + snapshotFullName); @@ -2947,8 +2949,9 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { logger.error("Failed to remove snapshot " + snapshotFullName + ", with exception: " + e.toString() + ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); } finally { - rbd.close(image); - r.ioCtxDestroy(io); + closeRbdImage(rbd, image, disk.getName()); + destroyRadosIoCtx(r, io, snapshotFullName); + shutDownRados(r); } } else if (storagePoolTypesToDeleteSnapshotFile.contains(primaryPool.getType())) { 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 059f4f8b67af..abfae7bb45ae 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 @@ -1174,19 +1174,18 @@ public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.Imag * We have to remove those snapshots first */ if (pool.getType() == StoragePoolType.RBD) { + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; 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")); + r = CephUtil.connect(pool.getAuthUserName(), pool.getSourceHost(), pool.getSourcePort(), pool.getAuthSecret()); - IoCTX io = r.ioCtxCreate(pool.getSourceDir()); - Rbd rbd = new Rbd(io); - RbdImage image = rbd.open(uuid); + io = r.ioCtxCreate(pool.getSourceDir()); + rbd = new Rbd(io); + image = rbd.open(uuid); logger.debug("Fetching list of snapshots of RBD image " + pool.getSourceDir() + "/" + uuid); List snaps = image.snapList(); try { @@ -1206,10 +1205,6 @@ public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.Imag 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() + @@ -1219,6 +1214,11 @@ public boolean deletePhysicalDisk(String uuid, KVMStoragePool pool, Storage.Imag 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"); + CephUtil.closeQuietly(rbd, image, uuid); + CephUtil.ioCtxDestroyQuietly(r, io); + CephUtil.shutDownQuietly(r); } } @@ -1394,121 +1394,120 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, /* We are on the same Ceph cluster, but we require RBD format 2 on the source image */ logger.debug("Trying to perform a RBD clone (layering) since we are operating in the same storage pool"); - Rados r = new Rados(srcPool.getAuthUserName()); - r.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort()); - r.confSet("key", srcPool.getAuthSecret()); - r.confSet("client_mount_timeout", "30"); - if (dataPool != null) { - logger.debug("Setting RBD data pool to " + dataPool + " for the new image " + disk.getName()); - r.confSet(KVMPhysicalDisk.RBD_DEFAULT_DATA_POOL, dataPool); - } - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet("mon_host")); - - IoCTX io = r.ioCtxCreate(srcPool.getSourceDir()); - Rbd rbd = new Rbd(io); - RbdImage srcImage = rbd.open(template.getName()); - - if (srcImage.isOldFormat()) { - /* The source image is RBD format 1, we have to do a regular copy */ - logger.debug("The source image " + srcPool.getSourceDir() + "/" + template.getName() + - " is RBD format 1. We have to perform a regular copy (" + toHumanReadableSize(disk.getVirtualSize()) + " bytes)"); - - rbd.create(disk.getName(), disk.getVirtualSize(), RBD_FEATURES, rbdOrder); - RbdImage destImage = rbd.open(disk.getName()); - - logger.debug("Starting to copy " + srcImage.getName() + " to " + destImage.getName() + " in Ceph pool " + srcPool.getSourceDir()); - rbd.copy(srcImage, destImage); + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage srcImage = null; + try { + r = CephUtil.connect(srcPool.getAuthUserName(), srcPool.getSourceHost(), srcPool.getSourcePort(), srcPool.getAuthSecret(), dataPool); + + io = r.ioCtxCreate(srcPool.getSourceDir()); + rbd = new Rbd(io); + srcImage = rbd.open(template.getName()); + + if (srcImage.isOldFormat()) { + /* The source image is RBD format 1, we have to do a regular copy */ + logger.debug("The source image " + srcPool.getSourceDir() + "/" + template.getName() + + " is RBD format 1. We have to perform a regular copy (" + toHumanReadableSize(disk.getVirtualSize()) + " bytes)"); + + rbd.create(disk.getName(), disk.getVirtualSize(), RBD_FEATURES, rbdOrder); + RbdImage destImage = rbd.open(disk.getName()); + try { + logger.debug("Starting to copy " + srcImage.getName() + " to " + destImage.getName() + " in Ceph pool " + srcPool.getSourceDir()); + rbd.copy(srcImage, destImage); + + logger.debug("Finished copying " + srcImage.getName() + " to " + destImage.getName() + " in Ceph pool " + srcPool.getSourceDir()); + } finally { + CephUtil.closeQuietly(rbd, destImage, disk.getName()); + } + } else { + logger.debug("The source image " + srcPool.getSourceDir() + "/" + template.getName() + + " is RBD format 2. We will perform a RBD clone using snapshot " + + rbdTemplateSnapName); + /* The source image is format 2, we can do a RBD snapshot+clone (layering) */ + + + logger.debug("Checking if RBD snapshot " + srcPool.getSourceDir() + "/" + template.getName() + + "@" + rbdTemplateSnapName + " exists prior to attempting a clone operation."); + + List snaps = srcImage.snapList(); + logger.debug("Found " + snaps.size() + " snapshots on RBD image " + srcPool.getSourceDir() + "/" + template.getName()); + boolean snapFound = false; + for (RbdSnapInfo snap : snaps) { + if (rbdTemplateSnapName.equals(snap.name)) { + logger.debug("RBD snapshot " + srcPool.getSourceDir() + "/" + template.getName() + + "@" + rbdTemplateSnapName + " already exists."); + snapFound = true; + break; + } + } - logger.debug("Finished copying " + srcImage.getName() + " to " + destImage.getName() + " in Ceph pool " + srcPool.getSourceDir()); - rbd.close(destImage); - } else { - logger.debug("The source image " + srcPool.getSourceDir() + "/" + template.getName() - + " is RBD format 2. We will perform a RBD clone using snapshot " - + rbdTemplateSnapName); - /* The source image is format 2, we can do a RBD snapshot+clone (layering) */ - - - logger.debug("Checking if RBD snapshot " + srcPool.getSourceDir() + "/" + template.getName() - + "@" + rbdTemplateSnapName + " exists prior to attempting a clone operation."); - - List snaps = srcImage.snapList(); - logger.debug("Found " + snaps.size() + " snapshots on RBD image " + srcPool.getSourceDir() + "/" + template.getName()); - boolean snapFound = false; - for (RbdSnapInfo snap : snaps) { - if (rbdTemplateSnapName.equals(snap.name)) { - logger.debug("RBD snapshot " + srcPool.getSourceDir() + "/" + template.getName() - + "@" + rbdTemplateSnapName + " already exists."); - snapFound = true; - break; + if (!snapFound) { + logger.debug("Creating RBD snapshot " + rbdTemplateSnapName + " on image " + name); + srcImage.snapCreate(rbdTemplateSnapName); + logger.debug("Protecting RBD snapshot " + rbdTemplateSnapName + " on image " + name); + srcImage.snapProtect(rbdTemplateSnapName); } - } - if (!snapFound) { - logger.debug("Creating RBD snapshot " + rbdTemplateSnapName + " on image " + name); - srcImage.snapCreate(rbdTemplateSnapName); - logger.debug("Protecting RBD snapshot " + rbdTemplateSnapName + " on image " + name); - srcImage.snapProtect(rbdTemplateSnapName); - } + rbd.clone(template.getName(), rbdTemplateSnapName, io, disk.getName(), RBD_FEATURES, rbdOrder); + logger.debug("Successfully cloned " + template.getName() + "@" + rbdTemplateSnapName + " to " + disk.getName()); + /* We also need to resize the image if the VM was deployed with a larger root disk size */ + if (disk.getVirtualSize() > template.getVirtualSize()) { + RbdImage diskImage = rbd.open(disk.getName()); + try { + diskImage.resize(disk.getVirtualSize()); + logger.debug("Resized " + disk.getName() + " to " + toHumanReadableSize(disk.getVirtualSize())); + } finally { + CephUtil.closeQuietly(rbd, diskImage, disk.getName()); + } + } - rbd.clone(template.getName(), rbdTemplateSnapName, io, disk.getName(), RBD_FEATURES, rbdOrder); - logger.debug("Successfully cloned " + template.getName() + "@" + rbdTemplateSnapName + " to " + disk.getName()); - /* We also need to resize the image if the VM was deployed with a larger root disk size */ - if (disk.getVirtualSize() > template.getVirtualSize()) { - RbdImage diskImage = rbd.open(disk.getName()); - diskImage.resize(disk.getVirtualSize()); - rbd.close(diskImage); - logger.debug("Resized " + disk.getName() + " to " + toHumanReadableSize(disk.getVirtualSize())); } - + } finally { + CephUtil.closeQuietly(rbd, srcImage, template.getName()); + CephUtil.ioCtxDestroyQuietly(r, io); + CephUtil.shutDownQuietly(r); } - - rbd.close(srcImage); - r.ioCtxDestroy(io); } else { /* The source pool or host is not the same Ceph cluster, we do a simple copy with Qemu-Img */ logger.debug("Both the source and destination are RBD, but not the same Ceph cluster. Performing a copy"); - Rados rSrc = new Rados(srcPool.getAuthUserName()); - rSrc.confSet("mon_host", srcPool.getSourceHost() + ":" + srcPool.getSourcePort()); - rSrc.confSet("key", srcPool.getAuthSecret()); - rSrc.confSet("client_mount_timeout", "30"); - rSrc.connect(); - logger.debug("Successfully connected to source Ceph cluster at " + rSrc.confGet("mon_host")); - - Rados rDest = new Rados(destPool.getAuthUserName()); - rDest.confSet("mon_host", destPool.getSourceHost() + ":" + destPool.getSourcePort()); - rDest.confSet("key", destPool.getAuthSecret()); - rDest.confSet("client_mount_timeout", "30"); - if (dataPool != null) { - logger.debug("Setting RBD data pool to " + dataPool + " on the destination cluster for the new image " + disk.getName()); - rDest.confSet(KVMPhysicalDisk.RBD_DEFAULT_DATA_POOL, dataPool); + Rados rSrc = null; + Rados rDest = null; + IoCTX sIO = null; + IoCTX dIO = null; + Rbd sRbd = null; + Rbd dRbd = null; + RbdImage srcImage = null; + RbdImage destImage = null; + try { + rSrc = CephUtil.connect(srcPool.getAuthUserName(), srcPool.getSourceHost(), srcPool.getSourcePort(), srcPool.getAuthSecret()); + rDest = CephUtil.connect(destPool.getAuthUserName(), destPool.getSourceHost(), destPool.getSourcePort(), destPool.getAuthSecret(), dataPool); + + sIO = rSrc.ioCtxCreate(srcPool.getSourceDir()); + sRbd = new Rbd(sIO); + + dIO = rDest.ioCtxCreate(destPool.getSourceDir()); + dRbd = new Rbd(dIO); + + logger.debug("Creating " + disk.getName() + " on the destination cluster " + rDest.confGet("mon_host") + " in pool " + + destPool.getSourceDir()); + dRbd.create(disk.getName(), disk.getVirtualSize(), RBD_FEATURES, rbdOrder); + + srcImage = sRbd.open(template.getName()); + destImage = dRbd.open(disk.getName()); + + logger.debug("Copying " + template.getName() + " from Ceph cluster " + rSrc.confGet("mon_host") + " to " + disk.getName() + + " on cluster " + rDest.confGet("mon_host")); + sRbd.copy(srcImage, destImage); + } finally { + CephUtil.closeQuietly(sRbd, srcImage, template.getName()); + CephUtil.closeQuietly(dRbd, destImage, disk.getName()); + CephUtil.ioCtxDestroyQuietly(rSrc, sIO); + CephUtil.ioCtxDestroyQuietly(rDest, dIO); + CephUtil.shutDownQuietly(rSrc); + CephUtil.shutDownQuietly(rDest); } - rDest.connect(); - logger.debug("Successfully connected to source Ceph cluster at " + rDest.confGet("mon_host")); - - IoCTX sIO = rSrc.ioCtxCreate(srcPool.getSourceDir()); - Rbd sRbd = new Rbd(sIO); - - IoCTX dIO = rDest.ioCtxCreate(destPool.getSourceDir()); - Rbd dRbd = new Rbd(dIO); - - logger.debug("Creating " + disk.getName() + " on the destination cluster " + rDest.confGet("mon_host") + " in pool " + - destPool.getSourceDir()); - dRbd.create(disk.getName(), disk.getVirtualSize(), RBD_FEATURES, rbdOrder); - - RbdImage srcImage = sRbd.open(template.getName()); - RbdImage destImage = dRbd.open(disk.getName()); - - logger.debug("Copying " + template.getName() + " from Ceph cluster " + rSrc.confGet("mon_host") + " to " + disk.getName() - + " on cluster " + rDest.confGet("mon_host")); - sRbd.copy(srcImage, destImage); - - sRbd.close(srcImage); - dRbd.close(destImage); - - rSrc.ioCtxDestroy(sIO); - rDest.ioCtxDestroy(dIO); } } catch (RadosException e) { logger.error("Failed to perform a RADOS action on the Ceph cluster, the error was: " + e.getMessage()); @@ -1649,6 +1648,10 @@ to support snapshots(backuped) as qcow2 files. */ * To do so it's mandatory that librbd on the system is at least 0.67.7 (Ceph Dumpling) */ logger.debug("The source image is not RBD, but the destination is. We will convert into RBD format 2"); + Rados r = null; + IoCTX io = null; + Rbd rbd = null; + RbdImage image = null; try { srcFile = new QemuImgFile(sourcePath, sourceFormat); String rbdDestPath = destPool.getSourceDir() + "/" + name; @@ -1660,24 +1663,16 @@ to support snapshots(backuped) as qcow2 files. */ logger.debug("Successfully converted source image " + srcFile.getFileName() + " to RBD image " + rbdDestPath); /* We have to stat the RBD image to see how big it became afterwards */ - Rados r = new Rados(destPool.getAuthUserName()); - r.confSet("mon_host", destPool.getSourceHost() + ":" + destPool.getSourcePort()); - r.confSet("key", destPool.getAuthSecret()); - r.confSet("client_mount_timeout", "30"); - r.connect(); - logger.debug("Successfully connected to Ceph cluster at " + r.confGet("mon_host")); + r = CephUtil.connect(destPool.getAuthUserName(), destPool.getSourceHost(), destPool.getSourcePort(), destPool.getAuthSecret()); - IoCTX io = r.ioCtxCreate(destPool.getSourceDir()); - Rbd rbd = new Rbd(io); + io = r.ioCtxCreate(destPool.getSourceDir()); + rbd = new Rbd(io); - RbdImage image = rbd.open(name); + image = rbd.open(name); RbdImageInfo rbdInfo = image.stat(); newDisk.setSize(rbdInfo.size); newDisk.setVirtualSize(rbdInfo.size); logger.debug("After copy the resulting RBD image " + rbdDestPath + " is " + toHumanReadableSize(rbdInfo.size) + " bytes long"); - rbd.close(image); - - r.ioCtxDestroy(io); } catch (QemuImgException | LibvirtException e) { String srcFilename = srcFile != null ? srcFile.getFileName() : null; String destFilename = destFile != null ? destFile.getFileName() : null; @@ -1689,6 +1684,10 @@ to support snapshots(backuped) as qcow2 files. */ } catch (RbdException e) { logger.error("A Ceph RBD operation failed (" + e.getReturnValue() + "). The error was: " + e.getMessage()); newDisk = null; + } finally { + CephUtil.closeQuietly(rbd, image, name); + CephUtil.ioCtxDestroyQuietly(r, io); + CephUtil.shutDownQuietly(r); } } else { /** From 9af2d037489603fe7b2d25699b006154b87dedc5 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 10:59:17 +0000 Subject: [PATCH 2/5] kvm: open RBD images read only where they are only read from A read write open registers a watcher on the image header and lets the image take its exclusive lock. A clone parent, and an image that is only read or stat'ed, need neither, and the extra work is done inside rbd_open() while the caller waits. Open the template read only when cloning from it, on both the same cluster and the cross cluster path, and open the snapshot read only when backing it up. The base snapshot still has to be created and protected on a writable handle, so the first clone of a template reopens it read write for that step alone; every later clone of the same template finds the snapshot and keeps the read only handle. Signed-off-by: Brad House --- .../LibvirtBackupSnapshotCommandWrapper.java | 3 ++- .../kvm/storage/LibvirtStorageAdaptor.java | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java index 3b4856b085de..e9e223fcfe40 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java @@ -106,7 +106,8 @@ public Answer execute(final BackupSnapshotCommand command, final LibvirtComputin io = r.ioCtxCreate(primaryPool.getSourceDir()); rbd = new Rbd(io); - image = rbd.open(snapshotDisk.getName(), snapshotName); + // The snapshot is only read from here, so it is opened read only. + image = rbd.openReadOnly(snapshotDisk.getName(), snapshotName); final File fh = new File(snapshotDestPath); try(BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(fh));) { final int chunkSize = 4194304; 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 abfae7bb45ae..246933feb605 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 @@ -1403,7 +1403,10 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, io = r.ioCtxCreate(srcPool.getSourceDir()); rbd = new Rbd(io); - srcImage = rbd.open(template.getName()); + // The template is the parent of the clone and is not written to on this path, so it + // is opened read only. A read write open registers a watcher on the image header and + // lets the image take the exclusive lock, neither of which a clone needs. + srcImage = rbd.openReadOnly(template.getName()); if (srcImage.isOldFormat()) { /* The source image is RBD format 1, we have to do a regular copy */ @@ -1443,6 +1446,14 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, } if (!snapFound) { + /* + * Creating and protecting the base snapshot are writes, so they need a + * writable handle. Only the first clone of a template comes through here; + * every later clone finds the snapshot and keeps the read only handle. + */ + CephUtil.closeQuietly(rbd, srcImage, template.getName()); + srcImage = rbd.open(template.getName()); + logger.debug("Creating RBD snapshot " + rbdTemplateSnapName + " on image " + name); srcImage.snapCreate(rbdTemplateSnapName); logger.debug("Protecting RBD snapshot " + rbdTemplateSnapName + " on image " + name); @@ -1494,7 +1505,8 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, destPool.getSourceDir()); dRbd.create(disk.getName(), disk.getVirtualSize(), RBD_FEATURES, rbdOrder); - srcImage = sRbd.open(template.getName()); + // The template is only read from on this path, so it is opened read only. + srcImage = sRbd.openReadOnly(template.getName()); destImage = dRbd.open(disk.getName()); logger.debug("Copying " + template.getName() + " from Ceph cluster " + rSrc.confGet("mon_host") + " to " + disk.getName() From 7932d9491cb1fc0dfedb832b31624d12140cf716 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 11:27:14 +0000 Subject: [PATCH 3/5] kvm: fix review findings in the Ceph bounding and cleanup changes Correctness fixes to the two preceding commits: - deleteSnapshot reported success when the image could not be opened. Moving the connect and open inside the inner try put rbd_open() failures into a catch that only logs, so the method fell through to its success return. The management server would drop the snapshot record while the snapshot stayed on the cluster, and a protected one then blocks removal of its parent volume. The open is back outside that catch. - The template reopen closed the read only handle and left the field pointing at it, so a failure of the reopen sent the already freed pointer to rbd_close() a second time in the finally. That is a use after free in librbd, which takes the agent down rather than raising an exception. The field is cleared first. - CephUtil.connect() abandoned the cluster handle if anything between rados_create() and the return threw, which is the leak the class exists to prevent, and the new connect timeout makes that path ordinary rather than rare. It now releases the handle before rethrowing. - The cephx secret is null for a pool with no cephx user, and librados aborts the process rather than returning an error if it is handed one. Guard as getRbdPhysicalDisk already does. Failures that were silent are now reported, which matters because a timeout makes them reachable: - rbd_read returns a negative errno rather than throwing, and the snapshot backup loop treated that as end of image: a short file on secondary storage, recorded as a good backup. It now fails, as does a write error, which was swallowed too. - A failed RBD snapshot create or remove logged and then returned a success answer, recording a snapshot in CloudStack with nothing behind it on the cluster. Also: client_mount_timeout now honours 0 like the other two, ioCtxDestroyQuietly guards the handle it dereferences, the constants left unused by the previous commits are removed, the three properties are documented in agent.properties, and the RBD cleanup tests assert the handle is shut down. Signed-off-by: Brad House --- agent/conf/agent.properties | 13 +++++ .../LibvirtBackupSnapshotCommandWrapper.java | 19 ++++++-- .../LibvirtManageSnapshotCommandWrapper.java | 7 ++- .../LibvirtRevertSnapshotCommandWrapper.java | 4 -- .../hypervisor/kvm/storage/CephUtil.java | 48 ++++++++++++++----- .../kvm/storage/KVMStorageProcessor.java | 31 ++++++------ .../kvm/storage/LibvirtStorageAdaptor.java | 5 ++ .../kvm/storage/KVMStorageProcessorTest.java | 23 +++++++++ 8 files changed, 115 insertions(+), 35 deletions(-) diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties index ba4a3874664a..078b0c79b44b 100644 --- a/agent/conf/agent.properties +++ b/agent/conf/agent.properties @@ -461,6 +461,19 @@ iscsi.session.cleanup.enabled=false # Time, in seconds, to wait before retrying to rebase during the incremental snapshot process. # incremental.snapshot.retry.rebase.wait=60 +# Time, in seconds, that a single Ceph OSD operation may block before it fails. This bounds one operation, +# not a whole request, so a long running copy or flatten is made up of many operations that are each well +# inside the limit. Set to 0 to keep the librados default of waiting forever. +# rados.osd.op.timeout=60 + +# Time, in seconds, that a single Ceph monitor operation may block before it fails. +# Set to 0 to keep the librados default of waiting forever. +# rados.mon.op.timeout=30 + +# Time, in seconds, that the Ceph client may spend connecting to the monitors. +# Set to 0 to keep the librados default. +# rados.client.mount.timeout=30 + # Path to the VDDK library directory for VMware to KVM conversion via VDDK, # passed to virt-v2v as -io vddk-libdir= #vddk.lib.dir= diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java index e9e223fcfe40..bb179b113d74 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtBackupSnapshotCommandWrapper.java @@ -116,16 +116,27 @@ public Answer execute(final BackupSnapshotCommand command, final LibvirtComputin while (true) { final byte[] buf = new byte[chunkSize]; final int bytes = image.read(offset, buf, chunkSize); - if (bytes <= 0) { + if (bytes < 0) { + /* + * rbd_read returns a negative errno rather than throwing. Treating that as + * end of image would store a short backup and report it as a success. + */ + throw new RbdException("Failed to read " + snapshotDisk.getName() + " at offset " + offset, bytes); + } + if (bytes == 0) { break; } bos.write(buf, 0, bytes); offset += bytes; } logger.debug("Completed backing up RBD snapshot " + snapshotName + " to " + snapshotDestPath + ". Bytes written: " + toHumanReadableSize(offset)); - }catch(final IOException ex) - { - logger.error("BackupSnapshotAnswer:Exception:"+ ex.getMessage()); + } catch (final IOException ex) { + /* + * A failed read or write leaves a short file on secondary storage. Reporting success + * here would record a backup that cannot be restored from. + */ + logger.error("Failed to back up " + snapshotDisk.getName() + " to " + snapshotDestPath + ". The error was: " + ex.getMessage(), ex); + return new BackupSnapshotAnswer(command, false, ex.toString(), null, true); } } catch (final RadosException e) { logger.error("A RADOS operation failed. The error was: " + e.getMessage()); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java index ebd7351b1c21..36b61b0190a7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java @@ -133,7 +133,12 @@ public Answer execute(final ManageSnapshotCommand command, final LibvirtComputin image.snapRemove(snapshotName); } } catch (final Exception e) { - logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage()); + /* + * Reporting success here would record a snapshot in CloudStack that does not exist + * on the cluster. + */ + logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage(), e); + return new ManageSnapshotAnswer(command, false, "Failed to manage snapshot: " + e.toString()); } finally { CephUtil.closeQuietly(rbd, image, disk.getName()); CephUtil.ioCtxDestroyQuietly(r, io); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java index 3f8e85ef053c..776abb339ad5 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java @@ -62,10 +62,6 @@ @ResourceWrapper(handles = RevertSnapshotCommand.class) public class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper { - private static final String MON_HOST = "mon_host"; - private static final String KEY = "key"; - private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout"; - private static final String RADOS_CONNECTION_TIMEOUT = "30"; protected Set storagePoolTypesThatSupportRevertSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD, StoragePoolType.Filesystem, StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint)); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java index c49612c5b71d..6618e1744e88 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java @@ -75,24 +75,44 @@ public static Rados connect(String authUserName, String monHost, int monPort, St */ public static Rados connect(String authUserName, String monHost, int monPort, String authSecret, String dataPool) throws RadosException { Rados r = new Rados(authUserName); - r.confSet("mon_host", monHost + ":" + monPort); - r.confSet("key", authSecret); - applyTimeouts(r); - if (dataPool != null) { - logger.debug("Setting RBD data pool to [{}] for images created on this connection.", dataPool); - r.confSet(KVMPhysicalDisk.RBD_DEFAULT_DATA_POOL, dataPool); + try { + r.confSet("mon_host", monHost + ":" + monPort); + /* + * The secret is null when the pool has no cephx user, and librados aborts the process rather + * than returning an error if it is handed a null value here. + */ + if (authUserName != null) { + r.confSet("key", authSecret); + } else { + r.confSet("auth_client_required", "none"); + } + applyTimeouts(r); + if (dataPool != null) { + logger.debug("Setting RBD data pool to [{}] for images created on this connection.", dataPool); + r.confSet(KVMPhysicalDisk.RBD_DEFAULT_DATA_POOL, dataPool); + } + r.connect(); + logger.debug("Successfully connected to Ceph cluster at [{}].", r.confGet("mon_host")); + return r; + } catch (Exception e) { + /* + * The handle never reaches the caller, so nothing else can release it. rados_create() has + * already run by this point, so without this it would survive until finalize(). + */ + shutDownQuietly(r); + throw e; } - r.connect(); - logger.debug("Successfully connected to Ceph cluster at [{}].", r.confGet("mon_host")); - return r; } /** * Applies the connect and operation timeouts from the agent properties. A timeout configured as 0 is * left unset, which keeps the librados default of waiting forever. */ - public static void applyTimeouts(Rados r) throws RadosException { - r.confSet(CLIENT_MOUNT_TIMEOUT, String.valueOf(AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_CLIENT_MOUNT_TIMEOUT))); + private static void applyTimeouts(Rados r) throws RadosException { + int mountTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_CLIENT_MOUNT_TIMEOUT); + if (mountTimeout > 0) { + r.confSet(CLIENT_MOUNT_TIMEOUT, String.valueOf(mountTimeout)); + } int osdOpTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_OSD_OP_TIMEOUT); if (osdOpTimeout > 0) { @@ -124,7 +144,11 @@ public static void shutDownQuietly(Rados r) { * Destroys an IO context. Safe to call with nulls so it can be used from a finally block. */ public static void ioCtxDestroyQuietly(Rados r, IoCTX io) { - if (r == null || io == null) { + if (io == null) { + return; + } + if (r == null) { + logger.warn("Cannot destroy the Ceph IO context without its cluster handle."); return; } try { 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 58b91e2b9dbf..84637109627f 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 @@ -174,10 +174,6 @@ public class KVMStorageProcessor implements StorageProcessor { private static final String MANAGE_SNAPSTHOT_CREATE_OPTION = "-c"; private static final String NAME_OPTION = "-n"; - private static final String CEPH_MON_HOST = "mon_host"; - private static final String CEPH_AUTH_KEY = "key"; - private static final String CEPH_CLIENT_MOUNT_TIMEOUT = "client_mount_timeout"; - private static final String CEPH_DEFAULT_MOUNT_TIMEOUT = "30"; /** * Time interval before rechecking virsh commands */ @@ -2932,22 +2928,29 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { Rbd rbd = null; RbdImage image = null; try { + /* + * Opening the image stays outside the inner catch. A failure here means the snapshot was + * not removed, and it has to reach the caller rather than be logged and reported as a + * successful delete. + */ r = radosConnect(primaryPool); io = r.ioCtxCreate(primaryPool.getSourceDir()); rbd = new Rbd(io); image = rbd.open(disk.getName()); - logger.info("Attempting to remove RBD snapshot " + snapshotFullName); - if (image.snapIsProtected(snapshotName)) { - logger.debug("Unprotecting RBD snapshot " + snapshotFullName); - image.snapUnprotect(snapshotName); + try { + logger.info("Attempting to remove RBD snapshot " + snapshotFullName); + if (image.snapIsProtected(snapshotName)) { + logger.debug("Unprotecting RBD snapshot " + snapshotFullName); + image.snapUnprotect(snapshotName); + } + image.snapRemove(snapshotName); + logger.info("Snapshot " + snapshotFullName + " successfully removed from " + + primaryPool.getType().toString() + " pool."); + } catch (RbdException e) { + logger.error("Failed to remove snapshot " + snapshotFullName + ", with exception: " + e.toString() + + ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); } - image.snapRemove(snapshotName); - logger.info("Snapshot " + snapshotFullName + " successfully removed from " + - primaryPool.getType().toString() + " pool."); - } catch (RbdException e) { - logger.error("Failed to remove snapshot " + snapshotFullName + ", with exception: " + e.toString() + - ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); } finally { closeRbdImage(rbd, image, disk.getName()); destroyRadosIoCtx(r, io, snapshotFullName); 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 246933feb605..7f71da2f31d6 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 @@ -1452,6 +1452,11 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, * every later clone finds the snapshot and keeps the read only handle. */ CephUtil.closeQuietly(rbd, srcImage, template.getName()); + /* + * The handle is dead once it is closed, and closing it twice frees the same + * native pointer twice. Clear it before the reopen, which can throw. + */ + srcImage = null; srcImage = rbd.open(template.getName()); logger.debug("Creating RBD snapshot " + rbdTemplateSnapName + " on image " + name); 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 11d508d16468..b2a824ee612d 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 @@ -551,6 +551,7 @@ public void takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce() thr Mockito.verify(rbdImageMock, Mockito.times(1)).snapCreate(SNAPSHOT_NAME); Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock); Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock); + Mockito.verify(radosMock).shutDown(); } } @@ -574,6 +575,28 @@ public void takeRbdVolumeSnapshotOfStoppedVmTestReleasesHandlesWhenSnapshotFails Assert.assertNull(result); Mockito.verify(rbd.constructed().get(0)).close(rbdImageMock); Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock); + Mockito.verify(radosMock).shutDown(); + } + } + + /** + * The cluster handle is only released from Rados.finalize() otherwise, so a path that opens one and + * fails has to shut it down itself rather than leave it for the garbage collector. + */ + @Test + public void takeRbdVolumeSnapshotOfStoppedVmTestReleasesClusterHandleWhenOpenFails() throws Exception { + Rados radosMock = Mockito.mock(Rados.class); + IoCTX ioCtxMock = Mockito.mock(IoCTX.class); + KVMPhysicalDisk diskMock = prepareRbdSnapshotMocks(radosMock, ioCtxMock); + + try (MockedConstruction rbd = Mockito.mockConstruction(Rbd.class, ((mock, context) -> + Mockito.doThrow(new RbdException("Failed to open image")).when(mock).open(RBD_IMAGE_NAME)))) { + + Long result = storageProcessorSpy.takeRbdVolumeSnapshotOfStoppedVm(kvmStoragePoolMock, diskMock, SNAPSHOT_NAME); + + Assert.assertNull(result); + Mockito.verify(radosMock).ioCtxDestroy(ioCtxMock); + Mockito.verify(radosMock).shutDown(); } } } From 5759f20ba8caa37d69a1b3a54fa7832a8a65fa13 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 11:30:52 +0000 Subject: [PATCH 4/5] kvm: make the Ceph operation timeouts opt in Default rados.osd.op.timeout and rados.mon.op.timeout to 0, which leaves the options unset and keeps the librados behaviour of waiting forever. Nothing about how an existing deployment talks to Ceph changes unless an operator sets them. rados.client.mount.timeout keeps its default of 30, which is the value the code passed before it became a property. agent.properties carries the reasoning and a suggested starting point, since an operator has no way to pick a value from the property name alone. Signed-off-by: Brad House --- agent/conf/agent.properties | 20 ++++++++++++------- .../agent/properties/AgentProperties.java | 15 ++++++++------ 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties index 078b0c79b44b..f6e3d8cf1ac0 100644 --- a/agent/conf/agent.properties +++ b/agent/conf/agent.properties @@ -461,17 +461,23 @@ iscsi.session.cleanup.enabled=false # Time, in seconds, to wait before retrying to rebase during the incremental snapshot process. # incremental.snapshot.retry.rebase.wait=60 -# Time, in seconds, that a single Ceph OSD operation may block before it fails. This bounds one operation, -# not a whole request, so a long running copy or flatten is made up of many operations that are each well -# inside the limit. Set to 0 to keep the librados default of waiting forever. -# rados.osd.op.timeout=60 +# Time, in seconds, that a single Ceph OSD operation may block before it fails. This bounds one operation +# and not a whole request, so a long running copy or flatten is made up of many operations that are each +# well inside the limit. +# The default of 0 leaves the option unset, which is the librados behaviour of waiting forever. An +# operation that never completes parks the agent thread that issued it, and because the agent runs a +# host's storage commands in sequence, every command queued behind it stops with it. +# Setting a limit is recommended. 60 is a reasonable starting point: it is far above a normal operation +# on a healthy cluster, while still bounding one that will never return. +# rados.osd.op.timeout=0 # Time, in seconds, that a single Ceph monitor operation may block before it fails. -# Set to 0 to keep the librados default of waiting forever. -# rados.mon.op.timeout=30 +# The default of 0 leaves the option unset, which is the librados behaviour of waiting forever. +# 30 is a reasonable starting point. +# rados.mon.op.timeout=0 # Time, in seconds, that the Ceph client may spend connecting to the monitors. -# Set to 0 to keep the librados default. +# Set to 0 to leave the option unset and keep the librados default. # rados.client.mount.timeout=30 # Path to the VDDK library directory for VMware to KVM conversion via VDDK, diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java index 04b0657209d0..0a463cd1bf7b 100644 --- a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java +++ b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java @@ -918,19 +918,22 @@ public Property getWorkers() { * Time, in seconds, that a single Ceph OSD operation may block before it fails.
* This bounds each operation, not a whole request, so a long running copy or flatten is made up of * many operations that are each well inside the limit.
- * Set to 0 to keep the librados default of waiting forever.
+ * The default of 0 leaves the option unset, which keeps the librados behaviour of waiting + * forever. A stuck operation then blocks the agent thread that issued it, and with it every storage + * command queued behind it for that host, so setting a limit is recommended.
* Data type: Integer.
- * Default value: 60 + * Default value: 0 */ - public static final Property RADOS_OSD_OP_TIMEOUT = new Property<>("rados.osd.op.timeout", 60); + public static final Property RADOS_OSD_OP_TIMEOUT = new Property<>("rados.osd.op.timeout", 0); /** * Time, in seconds, that a single Ceph monitor operation may block before it fails.
- * Set to 0 to keep the librados default of waiting forever.
+ * The default of 0 leaves the option unset, which keeps the librados behaviour of waiting + * forever.
* Data type: Integer.
- * Default value: 30 + * Default value: 0 */ - public static final Property RADOS_MON_OP_TIMEOUT = new Property<>("rados.mon.op.timeout", 30); + public static final Property RADOS_MON_OP_TIMEOUT = new Property<>("rados.mon.op.timeout", 0); /** * Time, in seconds, that the Ceph client may spend connecting to the monitors.
From 7f56e8ee6433247213824939d01a47df9a866869 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 18 Sep 2026 11:48:21 +0000 Subject: [PATCH 5/5] kvm: do not let a failed flush or a failed snapshot removal report success Closing an RBD image is where librbd flushes, so a close that fails on an image just written is the last chance a lost write has to surface. Routing those two closes through the non-throwing helper turned that into a warning and returned the disk anyway, which would register a volume whose tail writes never landed. Close the written destination explicitly and let it fail the copy; the finally still covers the paths that did not get that far. deleteSnapshot had the same shape: a failed snapUnprotect or snapRemove was logged and the method still answered "removed successfully", dropping the record while the snapshot kept pinning space on the cluster, and one left protected also blocks removal of its parent volume. It now fails, except for ENOENT, where the snapshot is already gone and the delete has nothing left to do. The same ENOENT tolerance keeps the RBD branch of ManageSnapshotCommand idempotent for deletes. Also corrects the applyTimeouts javadoc, which said 0 means waiting forever for all three options; that is true of the two operation timeouts but the connect timeout falls back to the librados default of 300s. The snapshot test now pins the unwind order, since destroying an IO context after its cluster handle has been shut down would be a use after free. Signed-off-by: Brad House --- .../agent/properties/AgentProperties.java | 1 - .../LibvirtManageSnapshotCommandWrapper.java | 22 +++++++++++++++---- .../LibvirtRevertSnapshotCommandWrapper.java | 1 - .../hypervisor/kvm/storage/CephUtil.java | 5 +++-- .../kvm/storage/KVMStorageProcessor.java | 21 ++++++++++++++++-- .../kvm/storage/LibvirtStorageAdaptor.java | 15 +++++++++++++ .../kvm/storage/KVMStorageProcessorTest.java | 13 ++++++++--- 7 files changed, 65 insertions(+), 13 deletions(-) diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java index 0a463cd1bf7b..e67284f6118b 100644 --- a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java +++ b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java @@ -942,7 +942,6 @@ public Property getWorkers() { */ public static final Property RADOS_CLIENT_MOUNT_TIMEOUT = new Property<>("rados.client.mount.timeout", 30); - public static class Property { private String name; private T defaultValue; diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java index 36b61b0190a7..06bf3114c4e3 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtManageSnapshotCommandWrapper.java @@ -31,6 +31,7 @@ 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.agent.api.Answer; import com.cloud.agent.api.ManageSnapshotAnswer; @@ -49,6 +50,8 @@ @ResourceWrapper(handles = ManageSnapshotCommand.class) public final class LibvirtManageSnapshotCommandWrapper extends CommandWrapper { + /** librados reports a missing object as -ENOENT. */ + private static final int RBD_ENOENT = -2; @Override public Answer execute(final ManageSnapshotCommand command, final LibvirtComputingResource libvirtComputingResource) { @@ -132,11 +135,22 @@ public Answer execute(final ManageSnapshotCommand command, final LibvirtComputin logger.debug("Attempting to remove RBD snapshot " + disk.getName() + "@" + snapshotName); image.snapRemove(snapshotName); } + } catch (final RbdException e) { + if (ManageSnapshotCommand.DESTROY_SNAPSHOT.equalsIgnoreCase(command.getCommandSwitch()) && e.getReturnValue() == RBD_ENOENT) { + /* + * Already gone. A delete whose end state is "the snapshot is not there" has + * succeeded, and failing here would break a retried delete. + */ + logger.info("RBD snapshot " + disk.getName() + "@" + snapshotName + " was already gone."); + } else { + /* + * Reporting success here would record a snapshot in CloudStack that does not + * exist on the cluster, or drop one that is still there. + */ + logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage(), e); + return new ManageSnapshotAnswer(command, false, "Failed to manage snapshot: " + e.toString()); + } } catch (final Exception e) { - /* - * Reporting success here would record a snapshot in CloudStack that does not exist - * on the cluster. - */ logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage(), e); return new ManageSnapshotAnswer(command, false, "Failed to manage snapshot: " + e.toString()); } finally { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java index 776abb339ad5..3ffd429e253c 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java @@ -62,7 +62,6 @@ @ResourceWrapper(handles = RevertSnapshotCommand.class) public class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper { - protected Set storagePoolTypesThatSupportRevertSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD, StoragePoolType.Filesystem, StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint)); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java index 6618e1744e88..93ad1c55966c 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java @@ -45,7 +45,7 @@ */ public final class CephUtil { - protected static Logger logger = LogManager.getLogger(CephUtil.class); + private static final Logger logger = LogManager.getLogger(CephUtil.class); private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout"; private static final String RADOS_OSD_OP_TIMEOUT = "rados_osd_op_timeout"; @@ -106,7 +106,8 @@ public static Rados connect(String authUserName, String monHost, int monPort, St /** * Applies the connect and operation timeouts from the agent properties. A timeout configured as 0 is - * left unset, which keeps the librados default of waiting forever. + * left unset, so librados uses its own default: waiting forever for the two operation timeouts, and + * 300 seconds for the connect timeout. */ private static void applyTimeouts(Rados r) throws RadosException { int mountTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.RADOS_CLIENT_MOUNT_TIMEOUT); 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 84637109627f..f59ec02b11aa 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 @@ -172,6 +172,9 @@ public class KVMStorageProcessor implements StorageProcessor { private String _manageSnapshotPath; private int _cmdsTimeout; + /** librados reports a missing object as -ENOENT. */ + private static final int RBD_ENOENT = -2; + private static final String MANAGE_SNAPSTHOT_CREATE_OPTION = "-c"; private static final String NAME_OPTION = "-n"; /** @@ -2948,8 +2951,22 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { logger.info("Snapshot " + snapshotFullName + " successfully removed from " + primaryPool.getType().toString() + " pool."); } catch (RbdException e) { - logger.error("Failed to remove snapshot " + snapshotFullName + ", with exception: " + e.toString() + - ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); + if (e.getReturnValue() == RBD_ENOENT) { + /* + * Already gone. A delete whose end state is "the snapshot is not there" has + * succeeded, and failing here would break a retried delete. + */ + logger.info("RBD snapshot " + snapshotFullName + " was already gone."); + } else { + /* + * Anything else means the snapshot is still on the cluster. Reporting success + * would drop the record while it keeps pinning space, and one left protected + * also blocks removal of its parent volume. + */ + logger.error("Failed to remove snapshot " + snapshotFullName + ", with exception: " + e.toString() + + ", RBD error: " + ErrorCode.getErrorMessage(e.getReturnValue())); + throw e; + } } } finally { closeRbdImage(rbd, image, disk.getName()); 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 7f71da2f31d6..f32a786305c6 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 @@ -1420,6 +1420,14 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, rbd.copy(srcImage, destImage); logger.debug("Finished copying " + srcImage.getName() + " to " + destImage.getName() + " in Ceph pool " + srcPool.getSourceDir()); + + /* + * rbd_close is where librbd flushes, so closing an image that was just + * written is the last point a lost write can surface. It has to fail the + * copy rather than be logged and forgotten. + */ + rbd.close(destImage); + destImage = null; } finally { CephUtil.closeQuietly(rbd, destImage, disk.getName()); } @@ -1517,6 +1525,13 @@ private KVMPhysicalDisk createDiskFromTemplateOnRBD(KVMPhysicalDisk template, logger.debug("Copying " + template.getName() + " from Ceph cluster " + rSrc.confGet("mon_host") + " to " + disk.getName() + " on cluster " + rDest.confGet("mon_host")); sRbd.copy(srcImage, destImage); + + /* + * rbd_close is where librbd flushes, so closing the destination is the last point + * a lost write can surface. It has to fail the copy rather than be logged. + */ + dRbd.close(destImage); + destImage = null; } finally { CephUtil.closeQuietly(sRbd, srcImage, template.getName()); CephUtil.closeQuietly(dRbd, destImage, disk.getName()); 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 b2a824ee612d..c9a4a3fef932 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 @@ -50,6 +50,7 @@ import org.mockito.Mock; import org.mockito.MockedConstruction; import org.mockito.MockedStatic; +import org.mockito.InOrder; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; import org.mockito.Spy; @@ -549,9 +550,15 @@ public void takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce() thr 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); - Mockito.verify(radosMock).shutDown(); + + /* + * Order matters: Rados.shutDown() releases the native cluster handle, so destroying the IO + * context after it would be a use after free. + */ + InOrder unwind = Mockito.inOrder(rbd.constructed().get(0), radosMock); + unwind.verify(rbd.constructed().get(0)).close(rbdImageMock); + unwind.verify(radosMock).ioCtxDestroy(ioCtxMock); + unwind.verify(radosMock).shutDown(); } }