From 1a0734f07a6370dd6c5a8a72e736e3472184df56 Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Mon, 26 Oct 2020 09:44:01 +0200 Subject: [PATCH 1/7] Setting snapshot state to error on timeout --- .../storage/snapshot/SnapshotApiService.java | 7 +++++++ .../motion/AncientDataMotionStrategy.java | 4 ++++ .../storage/snapshot/SnapshotManagerImpl.java | 17 +++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java index a80391b35252..c50f6aaf09b6 100644 --- a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java +++ b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java @@ -42,6 +42,13 @@ public interface SnapshotApiService { */ Pair, Integer> listSnapshots(ListSnapshotsCmd cmd); + /** + * Place a snapshot into a state of error; + * + * @param snapshotId + */ + void markFailedSnapshot(long snapshotId); + /** * Delete specified snapshot from the specified. If no other policies are assigned it calls destroy snapshot. This * will be diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java index 7c930fb34c9c..f911bb52f3d0 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java @@ -23,6 +23,7 @@ import javax.inject.Inject; +import com.cloud.storage.snapshot.SnapshotApiService; import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult; import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionStrategy; @@ -85,6 +86,8 @@ public class AncientDataMotionStrategy implements DataMotionStrategy { DataStoreManager dataStoreMgr; @Inject StorageCacheManager cacheMgr; + @Inject + public SnapshotApiService _snapshotService; @Override public StrategyPriority canHandle(DataObject srcData, DataObject destData) { @@ -587,6 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) { if (cacheData != null) { cacheMgr.deleteCacheObject(cacheData); } + _snapshotService.markFailedSnapshot(destData.getId()); throw new CloudRuntimeException(e.toString()); } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 35ec665b97d3..ac5850631ca0 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -553,6 +553,23 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo } } + public void markFailedSnapshot(long snapshotId) { + Account caller = CallContext.current().getCallingAccount(); + + // Verify parameters + SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId); + + if (snapshotCheck == null) { + throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId); + } + + _accountMgr.checkAccess(caller, null, true, snapshotCheck); + + snapshotCheck.setState(Snapshot.State.Error); + _snapshotDao.update(snapshotId, snapshotCheck); + + } + @Override @DB @ActionEvent(eventType = EventTypes.EVENT_SNAPSHOT_DELETE, eventDescription = "deleting snapshot", async = true) From 9650051a295e1e3b383eda56bca27851a4644bcf Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Fri, 6 Nov 2020 12:36:24 +0200 Subject: [PATCH 2/7] Setting removed field so snapshot record is ignored by garbage collection --- .../java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index ac5850631ca0..616809b72d5c 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -567,6 +567,8 @@ public void markFailedSnapshot(long snapshotId) { snapshotCheck.setState(Snapshot.State.Error); _snapshotDao.update(snapshotId, snapshotCheck); + // Setting removed to prevent record from being deleted by garbage collection. + _snapshotDao.remove(snapshotId); } From cd8955e6c8faad04693e64996e36c751f3e0aee9 Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Tue, 17 Nov 2020 15:47:47 +0200 Subject: [PATCH 3/7] Removed explicitly setting error status, renamed method from markFailed to markRemoved --- .../java/com/cloud/storage/snapshot/SnapshotApiService.java | 4 ++-- .../cloudstack/storage/motion/AncientDataMotionStrategy.java | 2 +- .../java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | 5 +---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java index c50f6aaf09b6..393af648eabf 100644 --- a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java +++ b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java @@ -43,11 +43,11 @@ public interface SnapshotApiService { Pair, Integer> listSnapshots(ListSnapshotsCmd cmd); /** - * Place a snapshot into a state of error; + * Set the removed flag on a snapshot; * * @param snapshotId */ - void markFailedSnapshot(long snapshotId); + void markRemovedSnapshot(long snapshotId); /** * Delete specified snapshot from the specified. If no other policies are assigned it calls destroy snapshot. This diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java index f911bb52f3d0..af719b89e9c8 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java @@ -590,7 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) { if (cacheData != null) { cacheMgr.deleteCacheObject(cacheData); } - _snapshotService.markFailedSnapshot(destData.getId()); + _snapshotService.markRemovedSnapshot(destData.getId()); throw new CloudRuntimeException(e.toString()); } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 616809b72d5c..ad39512809b3 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -553,7 +553,7 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo } } - public void markFailedSnapshot(long snapshotId) { + public void markRemovedSnapshot(long snapshotId) { Account caller = CallContext.current().getCallingAccount(); // Verify parameters @@ -564,9 +564,6 @@ public void markFailedSnapshot(long snapshotId) { } _accountMgr.checkAccess(caller, null, true, snapshotCheck); - - snapshotCheck.setState(Snapshot.State.Error); - _snapshotDao.update(snapshotId, snapshotCheck); // Setting removed to prevent record from being deleted by garbage collection. _snapshotDao.remove(snapshotId); From a7574ba3323268eaede28df804bd24874cd79b6e Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Wed, 18 Nov 2020 16:05:58 +0200 Subject: [PATCH 4/7] Renamed method, moved code a few lines down --- .../java/com/cloud/storage/snapshot/SnapshotApiService.java | 2 +- .../cloudstack/storage/motion/AncientDataMotionStrategy.java | 2 +- .../java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java index 393af648eabf..0ce16bb10623 100644 --- a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java +++ b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java @@ -47,7 +47,7 @@ public interface SnapshotApiService { * * @param snapshotId */ - void markRemovedSnapshot(long snapshotId); + void markSnapshotAsRemoved(long snapshotId); /** * Delete specified snapshot from the specified. If no other policies are assigned it calls destroy snapshot. This diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java index af719b89e9c8..a75f7e8b78d8 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java @@ -590,7 +590,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) { if (cacheData != null) { cacheMgr.deleteCacheObject(cacheData); } - _snapshotService.markRemovedSnapshot(destData.getId()); + _snapshotService.markSnapshotAsRemoved(destData.getId()); throw new CloudRuntimeException(e.toString()); } diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index ad39512809b3..16a4c07db58d 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -553,9 +553,7 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo } } - public void markRemovedSnapshot(long snapshotId) { - Account caller = CallContext.current().getCallingAccount(); - + public void markSnapshotAsRemoved(long snapshotId) { // Verify parameters SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId); @@ -563,6 +561,7 @@ public void markRemovedSnapshot(long snapshotId) { throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId); } + Account caller = CallContext.current().getCallingAccount(); _accountMgr.checkAccess(caller, null, true, snapshotCheck); // Setting removed to prevent record from being deleted by garbage collection. _snapshotDao.remove(snapshotId); From 49d6ff065bbc6cfe853d32332aafaf81d4455839 Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Thu, 19 Nov 2020 11:01:31 +0200 Subject: [PATCH 5/7] Moved remove logic --- .../storage/snapshot/SnapshotApiService.java | 7 ------- .../storage/motion/AncientDataMotionStrategy.java | 1 - .../datastore/ObjectInDataStoreManagerImpl.java | 2 ++ .../storage/snapshot/SnapshotManagerImpl.java | 15 --------------- 4 files changed, 2 insertions(+), 23 deletions(-) diff --git a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java index 0ce16bb10623..a80391b35252 100644 --- a/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java +++ b/api/src/main/java/com/cloud/storage/snapshot/SnapshotApiService.java @@ -42,13 +42,6 @@ public interface SnapshotApiService { */ Pair, Integer> listSnapshots(ListSnapshotsCmd cmd); - /** - * Set the removed flag on a snapshot; - * - * @param snapshotId - */ - void markSnapshotAsRemoved(long snapshotId); - /** * Delete specified snapshot from the specified. If no other policies are assigned it calls destroy snapshot. This * will be diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java index a75f7e8b78d8..225c4c60862c 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java @@ -590,7 +590,6 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) { if (cacheData != null) { cacheMgr.deleteCacheObject(cacheData); } - _snapshotService.markSnapshotAsRemoved(destData.getId()); throw new CloudRuntimeException(e.toString()); } diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java index ff8112cceff9..27ba49a66cc3 100644 --- a/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java +++ b/engine/storage/src/main/java/org/apache/cloudstack/storage/datastore/ObjectInDataStoreManagerImpl.java @@ -264,6 +264,7 @@ public boolean deleteIfNotReady(DataObject dataObj) { } else if (dataObj.getType() == DataObjectType.SNAPSHOT) { SnapshotDataStoreVO destSnapshotStore = snapshotDataStoreDao.findByStoreSnapshot(dataStore.getRole(), dataStore.getId(), objId); if (destSnapshotStore != null && destSnapshotStore.getState() != ObjectInDataStoreStateMachine.State.Ready) { + snapshotDao.remove(objId); snapshotDataStoreDao.remove(destSnapshotStore.getId()); } return true; @@ -276,6 +277,7 @@ public boolean deleteIfNotReady(DataObject dataObj) { case SNAPSHOT: SnapshotDataStoreVO destSnapshotStore = snapshotDataStoreDao.findByStoreSnapshot(dataStore.getRole(), dataStore.getId(), objId); if (destSnapshotStore != null && destSnapshotStore.getState() != ObjectInDataStoreStateMachine.State.Ready) { + snapshotDao.remove(objId); return snapshotDataStoreDao.remove(destSnapshotStore.getId()); } else { s_logger.warn("Snapshot " + objId + " is not found on image store " + dataStore.getId() + ", so no need to delete"); diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 16a4c07db58d..35ec665b97d3 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -553,21 +553,6 @@ private void postCreateRecurringSnapshotForPolicy(long userId, long volumeId, lo } } - public void markSnapshotAsRemoved(long snapshotId) { - // Verify parameters - SnapshotVO snapshotCheck = _snapshotDao.findById(snapshotId); - - if (snapshotCheck == null) { - throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId); - } - - Account caller = CallContext.current().getCallingAccount(); - _accountMgr.checkAccess(caller, null, true, snapshotCheck); - // Setting removed to prevent record from being deleted by garbage collection. - _snapshotDao.remove(snapshotId); - - } - @Override @DB @ActionEvent(eventType = EventTypes.EVENT_SNAPSHOT_DELETE, eventDescription = "deleting snapshot", async = true) From 24033b0fa9460ac9c91f559a9c31a5e7316832b7 Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Thu, 19 Nov 2020 11:12:40 +0200 Subject: [PATCH 6/7] Removed unused service --- .../cloudstack/storage/motion/AncientDataMotionStrategy.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java index 225c4c60862c..7c930fb34c9c 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java @@ -23,7 +23,6 @@ import javax.inject.Inject; -import com.cloud.storage.snapshot.SnapshotApiService; import org.apache.cloudstack.engine.subsystem.api.storage.ClusterScope; import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult; import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionStrategy; @@ -86,8 +85,6 @@ public class AncientDataMotionStrategy implements DataMotionStrategy { DataStoreManager dataStoreMgr; @Inject StorageCacheManager cacheMgr; - @Inject - public SnapshotApiService _snapshotService; @Override public StrategyPriority canHandle(DataObject srcData, DataObject destData) { From 0b168eabe76db3aa13061d3c158a49ea0cbec5a8 Mon Sep 17 00:00:00 2001 From: Darrin Husselmann Date: Thu, 19 Nov 2020 12:28:57 +0200 Subject: [PATCH 7/7] Moved removed logic - last time, promise --- .../apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java | 1 + .../storage/datastore/ObjectInDataStoreManagerImpl.java | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java index 51a2741dddb5..b8788fbef988 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java @@ -323,6 +323,7 @@ protected Void copySnapshotAsyncCallback(AsyncCallbackDispatcher