From afd060b54f48744caf4836e4a6334dc1711b2b17 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Thu, 17 Oct 2019 17:16:48 -0300 Subject: [PATCH 01/18] Fixes snapshot deletion --- .../test/resources/fakeDriverTestContext.xml | 2 +- .../src/test/resources/storageContext.xml | 2 +- ...tegy.java => DefaultSnapshotStrategy.java} | 114 ++++++++++-------- ...ngine-storage-snapshot-storage-context.xml | 4 +- 4 files changed, 66 insertions(+), 56 deletions(-) rename engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/{XenserverSnapshotStrategy.java => DefaultSnapshotStrategy.java} (84%) diff --git a/engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml b/engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml index 944196da1fc1..b8a227476ac9 100644 --- a/engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml +++ b/engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml @@ -49,7 +49,7 @@ - + diff --git a/engine/storage/integration-test/src/test/resources/storageContext.xml b/engine/storage/integration-test/src/test/resources/storageContext.xml index abf08767d9de..f65e2accde97 100644 --- a/engine/storage/integration-test/src/test/resources/storageContext.xml +++ b/engine/storage/integration-test/src/test/resources/storageContext.xml @@ -50,7 +50,7 @@ - + diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java similarity index 84% rename from engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java rename to engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 3ab2129bc0b1..265116209d27 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -34,7 +34,6 @@ import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.framework.jobs.AsyncJob; -import org.apache.cloudstack.framework.jobs.dao.SyncQueueItemDao; import org.apache.cloudstack.storage.command.CreateObjectAnswer; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; @@ -72,8 +71,8 @@ import com.cloud.utils.fsm.NoTransitionException; @Component -public class XenserverSnapshotStrategy extends SnapshotStrategyBase { - private static final Logger s_logger = Logger.getLogger(XenserverSnapshotStrategy.class); +public class DefaultSnapshotStrategy extends SnapshotStrategyBase { + private static final Logger s_logger = Logger.getLogger(DefaultSnapshotStrategy.class); @Inject SnapshotService snapshotSvr; @@ -90,12 +89,8 @@ public class XenserverSnapshotStrategy extends SnapshotStrategyBase { @Inject SnapshotDataFactory snapshotDataFactory; @Inject - private SnapshotDao _snapshotDao; - @Inject private SnapshotDetailsDao _snapshotDetailsDao; @Inject - private SyncQueueItemDao _syncQueueItemDao; - @Inject VolumeDetailsDao _volumeDetailsDaoImpl; @Override @@ -269,63 +264,78 @@ public boolean deleteSnapshot(Long snapshotId) { throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status"); } - // first mark the snapshot as destroyed, so that ui can't see it, but we - // may not destroy the snapshot on the storage, as other snapshots may - // depend on it. SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); - if (snapshotOnImage == null) { - s_logger.debug("Can't find snapshot on backup storage, delete it in db"); - snapshotDao.remove(snapshotId); - return true; - } - - SnapshotObject obj = (SnapshotObject)snapshotOnImage; - try { - obj.processEvent(Snapshot.Event.DestroyRequested); - List volumesFromSnapshot; - volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null); - if (volumesFromSnapshot.size() > 0) { - try { - obj.processEvent(Snapshot.Event.OperationFailed); - } catch (NoTransitionException e1) { - s_logger.debug("Failed to change snapshot state: " + e1.toString()); + boolean deletedOnSecondary = false; + if (snapshotOnImage == null) { + s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage")); + } else { + SnapshotObject obj = (SnapshotObject)snapshotOnImage; + try { + deletedOnSecondary = deleteSnapshotOnSecundaryStorage(snapshotId, snapshotOnImage, obj); + if (!deletedOnSecondary) { + s_logger.debug( + String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.", + snapshotId)); + } else { + s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId)); } - throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use "); + } catch (NoTransitionException e) { + s_logger.debug("Failed to set the state to destroying: ", e); + return false; } - } catch (NoTransitionException e) { - s_logger.debug("Failed to set the state to destroying: ", e); - return false; } - try { - boolean result = deleteSnapshotChain(snapshotOnImage); - obj.processEvent(Snapshot.Event.OperationSucceeded); - if (result) { - //snapshot is deleted on backup storage, need to delete it on primary storage - SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); - if (snapshotOnPrimary != null) { - SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); - long volumeId = snapshotOnPrimary.getVolumeId(); - VolumeVO volumeVO = volumeDao.findById(volumeId); - if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD && volumeVO != null) { - snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo); - } - snapshotOnPrimary.setState(State.Destroyed); - snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); - } - } - } catch (Exception e) { - s_logger.debug("Failed to delete snapshot: ", e); + boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId); + if (deletedOnPrimary) { + s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId)); + } else if (deletedOnSecondary) { + s_logger.debug(String.format("The snapshot was deleted on secondary storage. Could not find/delete snapshot (id: %d) on primary storage.", snapshotId)); + } + return deletedOnSecondary || deletedOnPrimary; + } + + /** + * Deletes the snapshot on secondary storage. + * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false. + */ + private boolean deleteSnapshotOnSecundaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException { + obj.processEvent(Snapshot.Event.DestroyRequested); + List volumesFromSnapshot; + volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null); + + if (volumesFromSnapshot.size() > 0) { try { obj.processEvent(Snapshot.Event.OperationFailed); } catch (NoTransitionException e1) { - s_logger.debug("Failed to change snapshot state: " + e.toString()); + s_logger.debug("Failed to change snapshot state: " + e1.toString()); } - return false; + throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use "); } - return true; + boolean result = deleteSnapshotChain(snapshotOnImage); + obj.processEvent(Snapshot.Event.OperationSucceeded); + return result; + } + + /** + * Deletes the snapshot on primary storage. It can return false when the snapshot was not stored on primary storage; however this does not means that it failed to delete the snapshot.
+ * In case of failure, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException.
+ */ + private boolean deleteSnapshotOnPrimary(Long snapshotId) { + boolean result = false; + SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); + if (snapshotOnPrimary != null) { + SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); + long volumeId = snapshotOnPrimary.getVolumeId(); + VolumeVO volumeVO = volumeDao.findById(volumeId); + if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD && volumeVO != null) { + result = snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo); + } + snapshotOnPrimary.setState(State.Destroyed); + snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + } + return result; } @Override diff --git a/engine/storage/snapshot/src/main/resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml b/engine/storage/snapshot/src/main/resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml index b295398144c6..2bfb3c368a56 100644 --- a/engine/storage/snapshot/src/main/resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml +++ b/engine/storage/snapshot/src/main/resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml @@ -27,8 +27,8 @@ http://www.springframework.org/schema/context/spring-context.xsd" > - + From 8191b4da4815af8a3a13adcdd9f4692faf24d284 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Mon, 21 Oct 2019 14:24:00 -0300 Subject: [PATCH 02/18] Remove legacy '@Component', it is not necessary in this bean/class. --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 265116209d27..3e8e473e9cce 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -21,7 +21,6 @@ import javax.inject.Inject; import org.apache.log4j.Logger; -import org.springframework.stereotype.Component; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event; @@ -70,7 +69,6 @@ import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.NoTransitionException; -@Component public class DefaultSnapshotStrategy extends SnapshotStrategyBase { private static final Logger s_logger = Logger.getLogger(DefaultSnapshotStrategy.class); From bc9387fb91918e02cc1296a3c82f563795bf89ea Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Mon, 25 Nov 2019 17:21:09 -0200 Subject: [PATCH 03/18] Fix log message missing %d and remove snapshot on DB --- .../snapshot/DefaultSnapshotStrategy.java | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 3e8e473e9cce..a8193842b915 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -266,7 +266,7 @@ public boolean deleteSnapshot(Long snapshotId) { boolean deletedOnSecondary = false; if (snapshotOnImage == null) { - s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage")); + s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); } else { SnapshotObject obj = (SnapshotObject)snapshotOnImage; try { @@ -321,19 +321,29 @@ private boolean deleteSnapshotOnSecundaryStorage(Long snapshotId, SnapshotInfo s * In case of failure, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException.
*/ private boolean deleteSnapshotOnPrimary(Long snapshotId) { - boolean result = false; + SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); + SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); + if (isSnapshotOnPrimaryStorage(snapshotId) && snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { + snapshotOnPrimary.setState(State.Destroyed); + snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + snapshotDao.remove(snapshotId); + return true; + } + return false; + } + + /** + * Returns true if the snapshot volume is on primary storage. + */ + private boolean isSnapshotOnPrimaryStorage(long snapshotId) { SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); if (snapshotOnPrimary != null) { - SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); long volumeId = snapshotOnPrimary.getVolumeId(); VolumeVO volumeVO = volumeDao.findById(volumeId); - if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD && volumeVO != null) { - result = snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo); - } - snapshotOnPrimary.setState(State.Destroyed); - snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + boolean isVolumeOnPrimary = volumeVO != null && volumeVO.getRemoved() == null; + return isVolumeOnPrimary; } - return result; + return false; } @Override From e109949e3a9e386b810c28132de6de420f5d6a03 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Wed, 27 Nov 2019 12:24:29 -0200 Subject: [PATCH 04/18] Remove "dummy" boolean return statement --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index a8193842b915..b2ff46c10064 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -340,8 +340,7 @@ private boolean isSnapshotOnPrimaryStorage(long snapshotId) { if (snapshotOnPrimary != null) { long volumeId = snapshotOnPrimary.getVolumeId(); VolumeVO volumeVO = volumeDao.findById(volumeId); - boolean isVolumeOnPrimary = volumeVO != null && volumeVO.getRemoved() == null; - return isVolumeOnPrimary; + return volumeVO != null && volumeVO.getRemoved() == null; } return false; } From d31ecb8cfccf13096f8d6013e804b598cbfe0b38 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Fri, 13 Mar 2020 01:33:54 -0300 Subject: [PATCH 05/18] Manage snapshot deletion for KVM + NFS (primary storage) --- .../kvm/storage/KVMStorageProcessor.java | 62 ++++++++++++------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 9a2fd275dd3c..2bd55adff1e6 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 @@ -127,6 +127,14 @@ public class KVMStorageProcessor implements StorageProcessor { private String _createTmplPath; private String _manageSnapshotPath; private int _cmdsTimeout; + + private static final String MANAGE_SNAPSTHOT_CREATE = "-c"; + private static final String MANAGE_SNAPSTHOT_DESTROY = "-d"; + private static final String NAME = "-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"; public KVMStorageProcessor(final KVMStoragePoolManager storagePoolMgr, final LibvirtComputingResource resource) { this.storagePoolMgr = storagePoolMgr; @@ -563,7 +571,7 @@ public Answer createTemplateFromVolume(final CopyCommand cmd) { final Script command = new Script(_createTmplPath, wait, s_logger); command.add("-f", disk.getPath()); command.add("-t", tmpltPath); - command.add("-n", templateName + ".qcow2"); + command.add(NAME, templateName + ".qcow2"); final String result = command.execute(); @@ -949,7 +957,7 @@ public Answer backupSnapshot(final CopyCommand cmd) { } else { final Script command = new Script(_manageSnapshotPath, cmd.getWaitInMillSeconds(), s_logger); command.add("-b", snapshotDisk.getPath()); - command.add("-n", snapshotName); + command.add(NAME, snapshotName); command.add("-p", snapshotDestPath); if (isCreatedFromVmSnapshot) { descName = UUID.randomUUID().toString(); @@ -1010,14 +1018,7 @@ public Answer backupSnapshot(final CopyCommand cmd) { } } else { if (primaryPool.getType() != StoragePoolType.RBD) { - final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger); - command.add("-d", snapshotDisk.getPath()); - command.add("-n", snapshotName); - final String result = command.execute(); - if (result != null) { - s_logger.debug("Failed to delete snapshot on primary: " + result); - // return new CopyCmdAnswer("Failed to backup snapshot: " + result); - } + deleteSnapshotViaManageSnapshotScript(snapshotName, snapshotDisk); } } } catch (final Exception ex) { @@ -1035,6 +1036,16 @@ public Answer backupSnapshot(final CopyCommand cmd) { } } + private void deleteSnapshotViaManageSnapshotScript(final String snapshotName, KVMPhysicalDisk snapshotDisk) { + final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger); + command.add(MANAGE_SNAPSTHOT_DESTROY, snapshotDisk.getPath()); + command.add(NAME, snapshotName); + final String result = command.execute(); + if (result != null) { + s_logger.debug("Failed to delete snapshot on primary: " + result); + } + } + protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach) throws LibvirtException, URISyntaxException, InternalErrorException { String isoXml = null; @@ -1489,12 +1500,7 @@ public Answer createSnapshot(final CreateObjectCommand cmd) { */ if (primaryPool.getType() == StoragePoolType.RBD) { 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(); - s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet("mon_host")); + Rados r = radosConnect(primaryPool); final IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir()); final Rbd rbd = new Rbd(io); @@ -1511,8 +1517,8 @@ public Answer createSnapshot(final CreateObjectCommand cmd) { } else { /* VM is not running, create a snapshot by ourself */ final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger); - command.add("-c", disk.getPath()); - command.add("-n", snapshotName); + command.add(MANAGE_SNAPSTHOT_CREATE, disk.getPath()); + command.add(NAME, snapshotName); final String result = command.execute(); if (result != null) { s_logger.debug("Failed to manage snapshot: " + result); @@ -1531,6 +1537,16 @@ public Answer createSnapshot(final CreateObjectCommand cmd) { } } + private 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(); + s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet(CEPH_MON_HOST)); + return r; + } + @Override public Answer deleteVolume(final DeleteCommand cmd) { final VolumeObjectTO vol = (VolumeObjectTO)cmd.getData(); @@ -1619,12 +1635,7 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { String snapshotName = snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1); snap_full_name = disk.getName() + "@" + snapshotName; if (primaryPool.getType() == StoragePoolType.RBD) { - 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(); - s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet("mon_host")); + Rados r = radosConnect(primaryPool); IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir()); Rbd rbd = new Rbd(io); RbdImage image = rbd.open(disk.getName()); @@ -1644,6 +1655,9 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { rbd.close(image); r.ioCtxDestroy(io); } + } else if (primaryPool.getType() == StoragePoolType.NetworkFilesystem) { + s_logger.info(String.format("Attempting to remove snapshot on primary storage (id=%s, snapshot=%s, storage type=%s)", snapshotTO.getId(), snap_full_name, primaryPool.getType())); + deleteSnapshotViaManageSnapshotScript(snapshotName, disk); } else { s_logger.warn("Operation not implemented for storage pool type of " + primaryPool.getType().toString()); throw new InternalErrorException("Operation not implemented for storage pool type of " + primaryPool.getType().toString()); From 814af6a1829b5835e240cc2b50aa9bde20ce2637 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Mon, 16 Mar 2020 08:31:45 -0300 Subject: [PATCH 06/18] checkstyle trailing spaces --- .../com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index 2bd55adff1e6..f9f96d97e38d 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 @@ -127,7 +127,7 @@ public class KVMStorageProcessor implements StorageProcessor { private String _createTmplPath; private String _manageSnapshotPath; private int _cmdsTimeout; - + private static final String MANAGE_SNAPSTHOT_CREATE = "-c"; private static final String MANAGE_SNAPSTHOT_DESTROY = "-d"; private static final String NAME = "-n"; @@ -1546,7 +1546,7 @@ private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosExcepti s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet(CEPH_MON_HOST)); return r; } - + @Override public Answer deleteVolume(final DeleteCommand cmd) { final VolumeObjectTO vol = (VolumeObjectTO)cmd.getData(); From a0bc85ba97549bc72bf9faf843238b7e4ac5a40d Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 16 Mar 2020 15:38:21 +0100 Subject: [PATCH 07/18] rename options strings to *_OPTION --- .../kvm/storage/KVMStorageProcessor.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java index f9f96d97e38d..9eee58746442 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 @@ -128,9 +128,9 @@ public class KVMStorageProcessor implements StorageProcessor { private String _manageSnapshotPath; private int _cmdsTimeout; - private static final String MANAGE_SNAPSTHOT_CREATE = "-c"; - private static final String MANAGE_SNAPSTHOT_DESTROY = "-d"; - private static final String NAME = "-n"; + private static final String MANAGE_SNAPSTHOT_CREATE_OPTION = "-c"; + private static final String MANAGE_SNAPSTHOT_DESTROY_OPTION = "-d"; + 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"; @@ -571,7 +571,7 @@ public Answer createTemplateFromVolume(final CopyCommand cmd) { final Script command = new Script(_createTmplPath, wait, s_logger); command.add("-f", disk.getPath()); command.add("-t", tmpltPath); - command.add(NAME, templateName + ".qcow2"); + command.add(NAME_OPTION, templateName + ".qcow2"); final String result = command.execute(); @@ -957,7 +957,7 @@ public Answer backupSnapshot(final CopyCommand cmd) { } else { final Script command = new Script(_manageSnapshotPath, cmd.getWaitInMillSeconds(), s_logger); command.add("-b", snapshotDisk.getPath()); - command.add(NAME, snapshotName); + command.add(NAME_OPTION, snapshotName); command.add("-p", snapshotDestPath); if (isCreatedFromVmSnapshot) { descName = UUID.randomUUID().toString(); @@ -1038,8 +1038,8 @@ public Answer backupSnapshot(final CopyCommand cmd) { private void deleteSnapshotViaManageSnapshotScript(final String snapshotName, KVMPhysicalDisk snapshotDisk) { final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger); - command.add(MANAGE_SNAPSTHOT_DESTROY, snapshotDisk.getPath()); - command.add(NAME, snapshotName); + command.add(MANAGE_SNAPSTHOT_DESTROY_OPTION, snapshotDisk.getPath()); + command.add(NAME_OPTION, snapshotName); final String result = command.execute(); if (result != null) { s_logger.debug("Failed to delete snapshot on primary: " + result); @@ -1517,8 +1517,8 @@ public Answer createSnapshot(final CreateObjectCommand cmd) { } else { /* VM is not running, create a snapshot by ourself */ final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger); - command.add(MANAGE_SNAPSTHOT_CREATE, disk.getPath()); - command.add(NAME, snapshotName); + command.add(MANAGE_SNAPSTHOT_CREATE_OPTION, disk.getPath()); + command.add(NAME_OPTION, snapshotName); final String result = command.execute(); if (result != null) { s_logger.debug("Failed to manage snapshot: " + result); From 7121967892d5b0bb3ae9a68b41ad4505eadd49c2 Mon Sep 17 00:00:00 2001 From: GabrielBrascher Date: Mon, 16 Mar 2020 21:09:10 -0300 Subject: [PATCH 08/18] Fix typo on deleteSnapshotOnSecondaryStorage and enhance log message --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 4 ++-- .../com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index b2ff46c10064..219c2e820de5 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -270,7 +270,7 @@ public boolean deleteSnapshot(Long snapshotId) { } else { SnapshotObject obj = (SnapshotObject)snapshotOnImage; try { - deletedOnSecondary = deleteSnapshotOnSecundaryStorage(snapshotId, snapshotOnImage, obj); + deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj); if (!deletedOnSecondary) { s_logger.debug( String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.", @@ -297,7 +297,7 @@ public boolean deleteSnapshot(Long snapshotId) { * Deletes the snapshot on secondary storage. * It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false. */ - private boolean deleteSnapshotOnSecundaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException { + private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException { obj.processEvent(Snapshot.Event.DestroyRequested); List volumesFromSnapshot; volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null); 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 9eee58746442..3d7586af27e5 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 @@ -1656,7 +1656,7 @@ public Answer deleteSnapshot(final DeleteCommand cmd) { r.ioCtxDestroy(io); } } else if (primaryPool.getType() == StoragePoolType.NetworkFilesystem) { - s_logger.info(String.format("Attempting to remove snapshot on primary storage (id=%s, snapshot=%s, storage type=%s)", snapshotTO.getId(), snap_full_name, primaryPool.getType())); + s_logger.info(String.format("Attempting to remove snapshot (id=%s, name=%s, path=%s, storage type=%s) on primary storage", snapshotTO.getId(), snapshotTO.getName(), snapshotTO.getPath(), primaryPool.getType())); deleteSnapshotViaManageSnapshotScript(snapshotName, disk); } else { s_logger.warn("Operation not implemented for storage pool type of " + primaryPool.getType().toString()); From 3eafab4221543496dd905de643bab67494b248e9 Mon Sep 17 00:00:00 2001 From: Andrija Panic <45762285+andrijapanicsb@users.noreply.github.com> Date: Wed, 1 Apr 2020 14:27:27 +0200 Subject: [PATCH 09/18] Move the snapshotDao.remove(snapshotId); (#4006) --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 219c2e820de5..fba2674d79c9 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -267,6 +267,7 @@ public boolean deleteSnapshot(Long snapshotId) { boolean deletedOnSecondary = false; if (snapshotOnImage == null) { s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); + snapshotDao.remove(snapshotId); } else { SnapshotObject obj = (SnapshotObject)snapshotOnImage; try { @@ -326,7 +327,6 @@ private boolean deleteSnapshotOnPrimary(Long snapshotId) { if (isSnapshotOnPrimaryStorage(snapshotId) && snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { snapshotOnPrimary.setState(State.Destroyed); snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); - snapshotDao.remove(snapshotId); return true; } return false; From 9847b841049154ee9124dbbed5354eadcaca301d Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 3 Apr 2020 15:14:00 +0530 Subject: [PATCH 10/18] Fix deletesnapshot worflow to handle both snapshots created in primary storage and snapshots backed up to secondary storage --- .../snapshot/DefaultSnapshotStrategy.java | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index fba2674d79c9..291f12a26dde 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -267,7 +267,6 @@ public boolean deleteSnapshot(Long snapshotId) { boolean deletedOnSecondary = false; if (snapshotOnImage == null) { s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); - snapshotDao.remove(snapshotId); } else { SnapshotObject obj = (SnapshotObject)snapshotOnImage; try { @@ -285,7 +284,27 @@ public boolean deleteSnapshot(Long snapshotId) { } } - boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId); + boolean deletedOnPrimary = false; + snapshotVO = snapshotDao.findById(snapshotId); + SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); + if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) { + deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); + } else { + // This is to handle snapshots which are created only on primary when snapshot.backup.to.secondary is set to false. + SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; + try { + obj.processEvent(Snapshot.Event.DestroyRequested); + deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); + if (!deletedOnPrimary) { + obj.processEvent(Snapshot.Event.OperationFailed); + } else { + obj.processEvent(Snapshot.Event.OperationSucceeded); + } + } catch (NoTransitionException e) { + s_logger.debug("Failed to set the state to destroying: ", e); + return false; + } + } if (deletedOnPrimary) { s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId)); } else if (deletedOnSecondary) { @@ -321,9 +340,8 @@ private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo s * Deletes the snapshot on primary storage. It can return false when the snapshot was not stored on primary storage; however this does not means that it failed to delete the snapshot.
* In case of failure, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException.
*/ - private boolean deleteSnapshotOnPrimary(Long snapshotId) { + private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) { SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); - SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); if (isSnapshotOnPrimaryStorage(snapshotId) && snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { snapshotOnPrimary.setState(State.Destroyed); snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); From e4bef7b6434cd35b52ec0fb12a8729397465054a Mon Sep 17 00:00:00 2001 From: nvazquez Date: Sat, 4 Apr 2020 11:38:12 -0300 Subject: [PATCH 11/18] Fix extra space --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 291f12a26dde..f24a7dc22350 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -290,7 +290,7 @@ public boolean deleteSnapshot(Long snapshotId) { if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) { deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); } else { - // This is to handle snapshots which are created only on primary when snapshot.backup.to.secondary is set to false. + // This is to handle snapshots which are created only on primary when snapshot.backup.to.secondary is set to false. SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; try { obj.processEvent(Snapshot.Event.DestroyRequested); From ad26c8fa3bda01d906dda1ce331cfd82eb506995 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 6 Apr 2020 12:50:14 +0200 Subject: [PATCH 12/18] refactor out separate handling methods for secondary and primary (reducing returns) --- .../snapshot/DefaultSnapshotStrategy.java | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index f24a7dc22350..8b643b0258e5 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -262,35 +262,29 @@ public boolean deleteSnapshot(Long snapshotId) { throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status"); } - SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); + boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId); + boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId); - boolean deletedOnSecondary = false; - if (snapshotOnImage == null) { - s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); - } else { - SnapshotObject obj = (SnapshotObject)snapshotOnImage; - try { - deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj); - if (!deletedOnSecondary) { - s_logger.debug( - String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.", - snapshotId)); - } else { - s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId)); - } - } catch (NoTransitionException e) { - s_logger.debug("Failed to set the state to destroying: ", e); - return false; - } + if (deletedOnPrimary) { + s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId)); + } else if (deletedOnSecondary) { + s_logger.debug(String.format("The snapshot was deleted on secondary storage. Could not find/delete snapshot (id: %d) on primary storage.", snapshotId)); } + return deletedOnSecondary || deletedOnPrimary; + } + + private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { + SnapshotVO snapshotVO; boolean deletedOnPrimary = false; snapshotVO = snapshotDao.findById(snapshotId); SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary); if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) { deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); } else { - // This is to handle snapshots which are created only on primary when snapshot.backup.to.secondary is set to false. + // Here we handle snapshots which are to be deleted but are not marked as destroyed yet. + // This may occur for instance when they are created only on primary because + // snapshot.backup.to.secondary was set to false. SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; try { obj.processEvent(Snapshot.Event.DestroyRequested); @@ -301,16 +295,35 @@ public boolean deleteSnapshot(Long snapshotId) { obj.processEvent(Snapshot.Event.OperationSucceeded); } } catch (NoTransitionException e) { - s_logger.debug("Failed to set the state to destroying: ", e); - return false; + s_logger.debug("Failed to set the state to destroying: ", e); + deletedOnPrimary = false; } } - if (deletedOnPrimary) { - s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId)); - } else if (deletedOnSecondary) { - s_logger.debug(String.format("The snapshot was deleted on secondary storage. Could not find/delete snapshot (id: %d) on primary storage.", snapshotId)); + return deletedOnPrimary; + } + + private boolean deleteOnSecondaryIfNeeded(Long snapshotId) { + boolean deletedOnSecondary = false; + SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); + if (snapshotOnImage == null) { + s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); + } else { + SnapshotObject obj = (SnapshotObject)snapshotOnImage; + try { + deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj); + if (!deletedOnSecondary) { + s_logger.debug( + String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.", + snapshotId)); + } else { + s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId)); + } + } catch (NoTransitionException e) { + s_logger.debug("Failed to set the state to destroying: ", e); + deletedOnSecondary = false; + } } - return deletedOnSecondary || deletedOnPrimary; + return deletedOnSecondary; } /** From 1be6b999f2339a0b5665f4ff1dfc14265a156c30 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 6 Apr 2020 20:29:01 +0200 Subject: [PATCH 13/18] return false on unexpected error or log when expected --- .../snapshot/DefaultSnapshotStrategy.java | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 8b643b0258e5..5ec82de1909a 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -257,21 +257,23 @@ public boolean deleteSnapshot(Long snapshotId) { return true; } - if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) && !Snapshot.State.Error.equals(snapshotVO.getState()) && + if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) && !Snapshot.State.Destroying.equals(snapshotVO.getState())) { throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status"); } - boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId); + Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId); boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId); if (deletedOnPrimary) { s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId)); - } else if (deletedOnSecondary) { - s_logger.debug(String.format("The snapshot was deleted on secondary storage. Could not find/delete snapshot (id: %d) on primary storage.", snapshotId)); + } else { + s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId)); } - - return deletedOnSecondary || deletedOnPrimary; + if (null == deletedOnSecondary && deletedOnSecondary) { + s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId)); + } + return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary; } private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { @@ -302,8 +304,8 @@ private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { return deletedOnPrimary; } - private boolean deleteOnSecondaryIfNeeded(Long snapshotId) { - boolean deletedOnSecondary = false; + private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) { + Boolean deletedOnSecondary = false; SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); if (snapshotOnImage == null) { s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); @@ -320,7 +322,7 @@ private boolean deleteOnSecondaryIfNeeded(Long snapshotId) { } } catch (NoTransitionException e) { s_logger.debug("Failed to set the state to destroying: ", e); - deletedOnSecondary = false; +// deletedOnSecondary remain null } } return deletedOnSecondary; From 20cb0b23b2b818a7b7e6117fdb538f3a40b26a35 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 6 Apr 2020 21:44:57 +0200 Subject: [PATCH 14/18] != instead of == --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 5ec82de1909a..06aaa2c973b0 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -270,7 +270,7 @@ public boolean deleteSnapshot(Long snapshotId) { } else { s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId)); } - if (null == deletedOnSecondary && deletedOnSecondary) { + if (null != deletedOnSecondary && deletedOnSecondary) { s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId)); } return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary; From 0aa1190990d851deb15b9863d38d3d35f7fb8c6b Mon Sep 17 00:00:00 2001 From: Andrija Panic <45762285+andrijapanicsb@users.noreply.github.com> Date: Mon, 6 Apr 2020 22:28:03 +0200 Subject: [PATCH 15/18] secondary instead of backup storage --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index 06aaa2c973b0..f43a95b91224 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -308,7 +308,7 @@ private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) { Boolean deletedOnSecondary = false; SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); if (snapshotOnImage == null) { - s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId)); + s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId)); } else { SnapshotObject obj = (SnapshotObject)snapshotOnImage; try { From ae1dad5a1f66e3ee8bc42cbd67713fafadfd37ff Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 7 Apr 2020 09:23:40 +0200 Subject: [PATCH 16/18] init to null --- .../cloudstack/storage/snapshot/DefaultSnapshotStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index f43a95b91224..bc81fd0e5410 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -305,7 +305,7 @@ private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { } private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) { - Boolean deletedOnSecondary = false; + Boolean deletedOnSecondary = null; SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image); if (snapshotOnImage == null) { s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId)); From d62b54b7cafd7e2c399843a4245bff475a00504b Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 9 Apr 2020 11:40:15 +0530 Subject: [PATCH 17/18] Handle snapshot deletion on primary storage. When primary store ref not found for snapshot do not fail the operation. --- .../snapshot/DefaultSnapshotStrategy.java | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index bc81fd0e5410..daca43e591e4 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -287,18 +287,25 @@ private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { // Here we handle snapshots which are to be deleted but are not marked as destroyed yet. // This may occur for instance when they are created only on primary because // snapshot.backup.to.secondary was set to false. - SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; - try { - obj.processEvent(Snapshot.Event.DestroyRequested); - deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); - if (!deletedOnPrimary) { - obj.processEvent(Snapshot.Event.OperationFailed); - } else { - obj.processEvent(Snapshot.Event.OperationSucceeded); + if (snapshotOnPrimaryInfo == null) { + s_logger.debug("Snapshot id:" + snapshotId + " not found on primary storage, skipping snapshot deletion on primary and marking it destroyed"); + snapshotVO.setState(Snapshot.State.Destroyed); + snapshotDao.update(snapshotId, snapshotVO); + return true; + } else { + SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; + try { + obj.processEvent(Snapshot.Event.DestroyRequested); + deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo); + if (!deletedOnPrimary) { + obj.processEvent(Snapshot.Event.OperationFailed); + } else { + obj.processEvent(Snapshot.Event.OperationSucceeded); + } + } catch (NoTransitionException e) { + s_logger.debug("Failed to set the state to destroying: ", e); + deletedOnPrimary = false; } - } catch (NoTransitionException e) { - s_logger.debug("Failed to set the state to destroying: ", e); - deletedOnPrimary = false; } } return deletedOnPrimary; @@ -352,14 +359,21 @@ private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo s } /** - * Deletes the snapshot on primary storage. It can return false when the snapshot was not stored on primary storage; however this does not means that it failed to delete the snapshot.
- * In case of failure, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException.
+ * Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage;
+ * In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException.
*/ private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) { SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); - if (isSnapshotOnPrimaryStorage(snapshotId) && snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { - snapshotOnPrimary.setState(State.Destroyed); - snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + if (isSnapshotOnPrimaryStorage(snapshotId)) { + s_logger.debug("Snapshot reference is found on primary storage for snapshot id:" + snapshotId + ", performing snapshot deletion on primary"); + if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { + snapshotOnPrimary.setState(State.Destroyed); + snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + s_logger.debug("Successfully deleted snapshot id:" + snapshotId + " on primary storage"); + return true; + } + } else { + s_logger.debug("Snapshot reference is not found on primary storage for snapshot id:" + snapshotId + ", skipping snapshot deletion on primary"); return true; } return false; From ca94242a91276decda0ccb1f81dd621288d9c385 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Thu, 9 Apr 2020 14:47:22 +0530 Subject: [PATCH 18/18] Fix debug levels on log messages --- .../snapshot/DefaultSnapshotStrategy.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java index daca43e591e4..c0a2395aa34e 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java @@ -288,10 +288,12 @@ private boolean deleteOnPrimaryIfNeeded(Long snapshotId) { // This may occur for instance when they are created only on primary because // snapshot.backup.to.secondary was set to false. if (snapshotOnPrimaryInfo == null) { - s_logger.debug("Snapshot id:" + snapshotId + " not found on primary storage, skipping snapshot deletion on primary and marking it destroyed"); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId)); + } snapshotVO.setState(Snapshot.State.Destroyed); snapshotDao.update(snapshotId, snapshotVO); - return true; + deletedOnPrimary = true; } else { SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo; try { @@ -365,15 +367,21 @@ private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo s private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) { SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary); if (isSnapshotOnPrimaryStorage(snapshotId)) { - s_logger.debug("Snapshot reference is found on primary storage for snapshot id:" + snapshotId + ", performing snapshot deletion on primary"); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId)); + } if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) { snapshotOnPrimary.setState(State.Destroyed); snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); - s_logger.debug("Successfully deleted snapshot id:" + snapshotId + " on primary storage"); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId)); + } return true; } } else { - s_logger.debug("Snapshot reference is not found on primary storage for snapshot id:" + snapshotId + ", skipping snapshot deletion on primary"); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId)); + } return true; } return false;