[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #3969: Snapshot deletion issues

[GitBox <gi...@apache.org>]

GabrielBrascher commented on a change in pull request #3969: Snapshot deletion issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r401680996
 
 

 ##########
 File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
 ##########
 @@ -269,63 +262,87 @@ 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<VolumeDetailVO> 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", snapshotId));
 
 Review comment:
   @davidjumani @harikrishna-patnala it might work placing `snapshotDao.remove(snapshotId)` here, indeed. 
   
   However, I see a potential problem in the case thre is no snapshot backed up on secondary and deletion fails at the primary storage. In such a case it would remove the snapshot entry before removing the snapshot on primary storage. Not sure if this could lead to a snapshot garbage issue on primary storage (instead of on secondary).
   
   What do you think? Does this observation make sense?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services