[GitHub] [cloudstack] weizhouapache commented on a change in pull request #3649: Garbage snapshots are left on Primary and Secondary

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

weizhouapache commented on a change in pull request #3649: Garbage snapshots are left on Primary and Secondary
URL: https://github.com/apache/cloudstack/pull/3649#discussion_r376426479
 
 

 ##########
 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));
+        } 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 {
 
 Review comment:
   @GabrielBrascher typo here. it should be secondary, not secundary :)
   
   

----------------------------------------------------------------
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