diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties index ba4a3874664a..f6e3d8cf1ac0 100644 --- a/agent/conf/agent.properties +++ b/agent/conf/agent.properties @@ -461,6 +461,25 @@ 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 +# 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. +# 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 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, # passed to virt-v2v as -io vddk-libdir= #vddk.lib.dir= 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..e67284f6118b 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,33 @@ 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.
+ * 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: 0 + */ + 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.
+ * The default of 0 leaves the option unset, which keeps the librados behaviour of waiting + * forever.
+ * Data type: Integer.
+ * Default value: 0 + */ + 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.
+ * 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..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 @@ -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,17 @@ 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); + // 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; @@ -115,24 +116,38 @@ 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); } - 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..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,12 +31,14 @@ 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; 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; @@ -48,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) { @@ -113,17 +117,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 +135,28 @@ 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 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) { - logger.error("A RBD snapshot operation on " + disk.getName() + " failed. The error was: " + e.getMessage()); + 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); + 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..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 @@ -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; @@ -61,11 +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)); @@ -92,26 +88,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..93ad1c55966c --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/CephUtil.java @@ -0,0 +1,176 @@ +// 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 { + + 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"; + 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); + 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; + } + } + + /** + * Applies the connect and operation timeouts from the agent properties. A timeout configured as 0 is + * 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); + if (mountTimeout > 0) { + r.confSet(CLIENT_MOUNT_TIMEOUT, String.valueOf(mountTimeout)); + } + + 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 (io == null) { + return; + } + if (r == null) { + logger.warn("Cannot destroy the Ceph IO context without its cluster handle."); + 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..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,12 +172,11 @@ 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"; - 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 */ @@ -2364,6 +2363,7 @@ protected Long takeRbdVolumeSnapshotOfStoppedVm(KVMStoragePool primaryPool, KVMP } finally { closeRbdImage(rbd, image, disk.getName()); destroyRadosIoCtx(r, io, disk.getName()); + shutDownRados(r); } return snapshotSize; } @@ -2656,13 +2656,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 +2866,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 +2906,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,25 +2926,52 @@ 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 { - logger.info("Attempting to remove RBD snapshot " + snapshotFullName); - if (image.snapIsProtected(snapshotName)) { - logger.debug("Unprotecting RBD snapshot " + snapshotFullName); - image.snapUnprotect(snapshotName); + /* + * 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()); + + 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) { + 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; + } } - 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 { - 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..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 @@ -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,152 @@ 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); + // 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 */ + 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()); + + /* + * 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()); + } + } 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) { + /* + * 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()); + /* + * 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); + 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); + + // 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() + + " 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()); + 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 +1680,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 +1695,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 +1716,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 { /** 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..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,8 +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); + + /* + * 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(); } } @@ -574,6 +582,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(); } } }