From 446a5358c2532cedd87432a44f9c4c003540147e Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 8 Nov 2024 12:52:05 +0200 Subject: [PATCH] mgmt: Fix delete snapshot The snapshots were deleted from CS but the main record in the `snapshots` DB table was not marked as deleted --- .../storage/datastore/util/StorPoolUtil.java | 3 + .../snapshot/StorPoolSnapshotStrategy.java | 110 +++++++++++------- 2 files changed, 72 insertions(+), 41 deletions(-) diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java index 8f92b52c755b..ed432f31a6e8 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java @@ -134,6 +134,9 @@ public static void spLog(String fmt, Object... args) { public static final String DELAY_DELETE = "delayDelete"; public static final String SP_TIER = "SP_QOSCLASS"; + + public static final String OBJECT_DOES_NOT_EXIST = "objectDoesNotExist"; + public static enum StorpoolRights { RO("ro"), RW("rw"), DETACH("detach"); diff --git a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java index 7e4bb437d8ea..83c492b1129b 100644 --- a/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java +++ b/plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java @@ -16,10 +16,21 @@ // under the License. package org.apache.cloudstack.storage.snapshot; -import java.util.ArrayList; -import java.util.List; - -import javax.inject.Inject; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.hypervisor.kvm.storage.StorPoolStorageAdaptor; +import com.cloud.storage.DataStoreRole; +import com.cloud.storage.Snapshot; +import com.cloud.storage.SnapshotVO; +import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.SnapshotDao; +import com.cloud.storage.dao.SnapshotDetailsDao; +import com.cloud.storage.dao.SnapshotDetailsVO; +import com.cloud.storage.dao.SnapshotZoneDao; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.utils.fsm.NoTransitionException; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; @@ -40,27 +51,19 @@ import org.apache.cloudstack.storage.datastore.util.StorPoolUtil.SpApiResponse; import org.apache.cloudstack.storage.datastore.util.StorPoolUtil.SpConnectionDesc; import org.apache.commons.collections.CollectionUtils; +import org.apache.log4j.LogManager; import org.apache.log4j.Logger; + import org.springframework.stereotype.Component; -import com.cloud.exception.InvalidParameterValueException; -import com.cloud.hypervisor.kvm.storage.StorPoolStorageAdaptor; -import com.cloud.storage.DataStoreRole; -import com.cloud.storage.Snapshot; -import com.cloud.storage.SnapshotVO; -import com.cloud.storage.VolumeVO; -import com.cloud.storage.dao.SnapshotDao; -import com.cloud.storage.dao.SnapshotDetailsDao; -import com.cloud.storage.dao.SnapshotDetailsVO; -import com.cloud.storage.dao.SnapshotZoneDao; -import com.cloud.storage.dao.VolumeDao; -import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.utils.fsm.NoTransitionException; +import javax.inject.Inject; +import java.util.ArrayList; +import java.util.List; @Component public class StorPoolSnapshotStrategy implements SnapshotStrategy { - private static final Logger log = Logger.getLogger(StorPoolSnapshotStrategy.class); + protected Logger logger = LogManager.getLogger(getClass()); @Inject private SnapshotDao _snapshotDao; @@ -90,11 +93,11 @@ public SnapshotInfo backupSnapshot(SnapshotInfo snapshotInfo) { snapshotObj.processEvent(Snapshot.Event.BackupToSecondary); snapshotObj.processEvent(Snapshot.Event.OperationSucceeded); } catch (NoTransitionException ex) { - log.debug("Failed to change state: " + ex.toString()); + logger.debug("Failed to change state: " + ex.toString()); try { snapshotObj.processEvent(Snapshot.Event.OperationFailed); } catch (NoTransitionException ex2) { - log.debug("Failed to change state: " + ex2.toString()); + logger.debug("Failed to change state: " + ex2.toString()); } } return snapshotInfo; @@ -115,12 +118,11 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { SpApiResponse resp = StorPoolUtil.snapshotDelete(name, conn); if (resp.getError() != null) { final String err = String.format("Failed to clean-up Storpool snapshot %s. Error: %s", name, resp.getError()); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp); StorPoolUtil.spLog(err); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp); - throw new CloudRuntimeException(err); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp.getError().getName().equals(StorPoolUtil.OBJECT_DOES_NOT_EXIST)); } else { res = deleteSnapshotFromDbIfNeeded(snapshotVO, zoneId); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId,true); StorPoolUtil.spLog("StorpoolSnapshotStrategy.deleteSnapshot: executed successfully=%s, snapshot uuid=%s, name=%s", res, snapshotVO.getUuid(), name); } } catch (Exception e) { @@ -129,28 +131,41 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { } } + List snapshots = _snapshotStoreDao.listBySnapshotIdAndState(snapshotId, State.Ready); + if (res || CollectionUtils.isEmpty(snapshots)) { + updateSnapshotToDestroyed(snapshotVO); + return true; + } return res; } - private void markSnapshotAsDestroyedIfAlreadyRemoved(Long snapshotId, SpApiResponse resp) { - if (resp.getError().getName().equals("objectDoesNotExist")) { - SnapshotDataStoreVO snapshotOnPrimary = _snapshotStoreDao.findBySourceSnapshot(snapshotId, DataStoreRole.Primary); - if (snapshotOnPrimary != null) { - snapshotOnPrimary.setState(State.Destroyed); - _snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary); + private void markSnapshotAsDestroyedIfAlreadyRemoved(Long snapshotId, boolean isSnapshotDeleted) { + if (!isSnapshotDeleted) { + return; + } + List snapshotsOnStore = _snapshotStoreDao.listBySnapshotIdAndState(snapshotId, State.Ready); + for (SnapshotDataStoreVO snapshot : snapshotsOnStore) { + if (snapshot.getInstallPath() != null && snapshot.getInstallPath().contains(StorPoolUtil.SP_DEV_PATH)) { + snapshot.setState(State.Destroyed); + _snapshotStoreDao.update(snapshot.getId(), snapshot); } } } @Override public StrategyPriority canHandle(Snapshot snapshot, Long zoneId, SnapshotOperation op) { - log.debug(String.format("StorpoolSnapshotStrategy.canHandle: snapshot=%s, uuid=%s, op=%s", snapshot.getName(), snapshot.getUuid(), op)); + logger.debug(String.format("StorpoolSnapshotStrategy.canHandle: snapshot=%s, uuid=%s, op=%s", snapshot.getName(), snapshot.getUuid(), op)); if (op != SnapshotOperation.DELETE) { return StrategyPriority.CANT_HANDLE; } + SnapshotDataStoreVO snapshotOnPrimary = _snapshotStoreDao.findOneBySnapshotAndDatastoreRole(snapshot.getId(), DataStoreRole.Primary); if (snapshotOnPrimary == null) { + List snapshotsOnStorPool = listSnapshotsByIdAndPath(snapshot.getId(), StorPoolUtil.SP_DEV_PATH, DataStoreRole.Image); + if (CollectionUtils.isNotEmpty(snapshotsOnStorPool)) { + return StrategyPriority.HIGHEST; + } return StrategyPriority.CANT_HANDLE; } if (zoneId != null) { // If zoneId is present, then it should be same as the zoneId of primary store @@ -173,25 +188,25 @@ public StrategyPriority canHandle(Snapshot snapshot, Long zoneId, SnapshotOperat } private boolean deleteSnapshotChain(SnapshotInfo snapshot) { - log.debug("delete snapshot chain for snapshot: " + snapshot.getId()); + logger.debug("delete snapshot chain for snapshot: " + snapshot.getId()); final SnapshotInfo snapOnImage = snapshot; boolean result = false; boolean resultIsSet = false; try { while (snapshot != null && - (snapshot.getState() == Snapshot.State.Destroying || snapshot.getState() == Snapshot.State.Destroyed || snapshot.getState() == Snapshot.State.Error || snapshot.getState() == Snapshot.State.BackedUp)) { + (snapshot.getState() == Snapshot.State.Destroying || snapshot.getState() == Snapshot.State.Destroyed || snapshot.getState() == Snapshot.State.Error || snapshot.getState() == Snapshot.State.BackedUp)) { SnapshotInfo child = snapshot.getChild(); if (child != null) { - log.debug("the snapshot has child, can't delete it on the storage"); + logger.debug("the snapshot has child, can't delete it on the storage"); break; } - log.debug("Snapshot: " + snapshot.getId() + " doesn't have children, so it's ok to delete it and its parents"); + logger.debug("Snapshot: " + snapshot.getId() + " doesn't have children, so it's ok to delete it and its parents"); SnapshotInfo parent = snapshot.getParent(); boolean deleted = false; if (parent != null) { if (parent.getPath() != null && parent.getPath().equalsIgnoreCase(snapshot.getPath())) { - log.debug("for empty delta snapshot, only mark it as destroyed in db"); + logger.debug("for empty delta snapshot, only mark it as destroyed in db"); snapshot.processEvent(Event.DestroyRequested); snapshot.processEvent(Event.OperationSuccessed); deleted = true; @@ -208,7 +223,7 @@ private boolean deleteSnapshotChain(SnapshotInfo snapshot) { if (r) { List cacheSnaps = snapshotDataFactory.listSnapshotOnCache(snapshot.getId()); for (SnapshotInfo cacheSnap : cacheSnaps) { - log.debug("Delete snapshot " + snapshot.getId() + " from image cache store: " + cacheSnap.getDataStore().getName()); + logger.debug("Delete snapshot " + snapshot.getId() + " from image cache store: " + cacheSnap.getDataStore().getName()); cacheSnap.delete(); } } @@ -217,7 +232,7 @@ private boolean deleteSnapshotChain(SnapshotInfo snapshot) { resultIsSet = true; } } catch (Exception e) { - log.debug("Failed to delete snapshot on storage. ", e); + logger.debug("Failed to delete snapshot on storage. ", e); } } } else { @@ -226,7 +241,7 @@ private boolean deleteSnapshotChain(SnapshotInfo snapshot) { snapshot = parent; } } catch (Exception e) { - log.debug("delete snapshot failed: ", e); + logger.debug("delete snapshot failed: ", e); } return result; } @@ -248,7 +263,7 @@ protected boolean deleteSnapshotOnImageAndPrimary(long snapshotId, DataStore sto obj.processEvent(Snapshot.Event.DestroyRequested); } } catch (NoTransitionException e) { - log.debug("Failed to set the state to destroying: ", e); + logger.debug("Failed to set the state to destroying: ", e); return false; } @@ -266,13 +281,13 @@ protected boolean deleteSnapshotOnImageAndPrimary(long snapshotId, DataStore sto } } } catch (Exception e) { - log.debug("Failed to delete snapshot: ", e); + logger.debug("Failed to delete snapshot: ", e); try { if (areLastSnapshotRef) { obj.processEvent(Snapshot.Event.OperationFailed); } } catch (NoTransitionException e1) { - log.debug("Failed to change snapshot state: " + e.toString()); + logger.debug("Failed to change snapshot state: " + e.toString()); } return false; } @@ -372,4 +387,17 @@ public boolean revertSnapshot(SnapshotInfo snapshot) { @Override public void postSnapshotCreation(SnapshotInfo snapshot) { } + + private List listSnapshotsByIdAndPath (Long id, String pathList, DataStoreRole role) { + SearchBuilder dataStoreAndInstallPathSearch = _snapshotStoreDao.createSearchBuilder(); + dataStoreAndInstallPathSearch.and("id", dataStoreAndInstallPathSearch.entity().getSnapshotId(), SearchCriteria.Op.EQ); + dataStoreAndInstallPathSearch.and("install_pathIN", dataStoreAndInstallPathSearch.entity().getInstallPath(), SearchCriteria.Op.LIKE); + dataStoreAndInstallPathSearch.and("role", dataStoreAndInstallPathSearch.entity().getRole(), SearchCriteria.Op.EQ); + dataStoreAndInstallPathSearch.done(); + SearchCriteria sc = dataStoreAndInstallPathSearch.create(); + sc.setParameters("id", id); + sc.setParameters("install_pathIN", pathList + "%"); + sc.setParameters("role", role); + return _snapshotStoreDao.search(sc, null); + } }