From 3c792aa83bc4aa7f2b9d301bbb39b9dd87331cb9 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 25 Oct 2024 12:10:28 +0300 Subject: [PATCH 1/4] StorPool: fix of delete snapshot Mark the DB record as destroyed when a snapshot is deleted --- .../snapshot/StorPoolSnapshotStrategy.java | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 deletions(-) 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 5cdb7b8cda1a..8b16e0761389 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,11 +16,19 @@ // 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.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; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event; @@ -40,23 +48,13 @@ 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.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.apache.logging.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 @@ -117,10 +115,11 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { if (resp.getError() != null) { final String err = String.format("Failed to clean-up Storpool snapshot %s. Error: %s", name, resp.getError()); StorPoolUtil.spLog(err); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp, resp.getError().getName().equals("objectDoesNotExist")); throw new CloudRuntimeException(err); } else { res = deleteSnapshotFromDbIfNeeded(snapshotVO, zoneId); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp, true); StorPoolUtil.spLog("StorpoolSnapshotStrategy.deleteSnapshot: executed successfully=%s, snapshot uuid=%s, name=%s", res, snapshotVO.getUuid(), name); } } catch (Exception e) { @@ -129,15 +128,22 @@ 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, SpApiResponse resp, boolean isSnapshotDeleted) { + if (isSnapshotDeleted) { + 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); + } } } } From 871409249672685fad2b7da6f046f1b1d6924200 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Fri, 25 Oct 2024 17:18:20 +0300 Subject: [PATCH 2/4] Addressed reviews --- .../snapshot/StorPoolSnapshotStrategy.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) 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 8b16e0761389..c4544a3aaf1b 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 @@ -115,11 +115,11 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { if (resp.getError() != null) { final String err = String.format("Failed to clean-up Storpool snapshot %s. Error: %s", name, resp.getError()); StorPoolUtil.spLog(err); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp, resp.getError().getName().equals("objectDoesNotExist")); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp.getError().getName().equals("objectDoesNotExist")); throw new CloudRuntimeException(err); } else { res = deleteSnapshotFromDbIfNeeded(snapshotVO, zoneId); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp, true); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId,true); StorPoolUtil.spLog("StorpoolSnapshotStrategy.deleteSnapshot: executed successfully=%s, snapshot uuid=%s, name=%s", res, snapshotVO.getUuid(), name); } } catch (Exception e) { @@ -136,14 +136,15 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { return res; } - private void markSnapshotAsDestroyedIfAlreadyRemoved(Long snapshotId, SpApiResponse resp, boolean isSnapshotDeleted) { - if (isSnapshotDeleted) { - 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); - } + 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); } } } From 7777af2c620e5a21630bf44b704e8d5dd36bcb5e Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Mon, 28 Oct 2024 18:28:13 +0200 Subject: [PATCH 3/4] addressed review --- .../cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java | 1 + 1 file changed, 1 insertion(+) 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 c4544a3aaf1b..be7fcca73d48 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 @@ -29,6 +29,7 @@ import com.cloud.storage.dao.VolumeDao; 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; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event; From 393eb6c38b70b114838ccdf37962b727b4629097 Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Wed, 30 Oct 2024 15:37:17 +0200 Subject: [PATCH 4/4] addressed review --- .../cloudstack/storage/datastore/util/StorPoolUtil.java | 4 +++- .../cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java | 2 +- 2 files changed, 4 insertions(+), 2 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 97f4e2fe155a..dcb190a573b0 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 @@ -138,6 +138,8 @@ public static void spLog(String fmt, Object... args) { 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"); @@ -458,7 +460,7 @@ public static JsonArray templatesStats(SpConnectionDesc conn) { } private static boolean objectExists(SpApiError err) { - if (!err.getName().equals("objectDoesNotExist")) { + if (!err.getName().equals(OBJECT_DOES_NOT_EXIST)) { throw new CloudRuntimeException(err.getDescr()); } return false; 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 be7fcca73d48..c7bcc8a46b7f 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 @@ -116,7 +116,7 @@ public boolean deleteSnapshot(Long snapshotId, Long zoneId) { if (resp.getError() != null) { final String err = String.format("Failed to clean-up Storpool snapshot %s. Error: %s", name, resp.getError()); StorPoolUtil.spLog(err); - markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp.getError().getName().equals("objectDoesNotExist")); + markSnapshotAsDestroyedIfAlreadyRemoved(snapshotId, resp.getError().getName().equals(StorPoolUtil.OBJECT_DOES_NOT_EXIST)); throw new CloudRuntimeException(err); } else { res = deleteSnapshotFromDbIfNeeded(snapshotVO, zoneId);