[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #5130: Fix of delete of Ceph's snapshots from secondary storage

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

GabrielBrascher commented on a change in pull request #5130:
URL: https://github.com/apache/cloudstack/pull/5130#discussion_r655317878



##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/CephSnapshotStrategy.java
##########
@@ -64,10 +64,6 @@ public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
             return StrategyPriority.HIGHEST;
         }
 
-        if (SnapshotOperation.DELETE.equals(op)) {
-            return StrategyPriority.HIGHEST;
-        }
-

Review comment:
       @sureshanaparti thanks for the code review. I would like to just add a note to avoid misunderstandings.
   
   Delete operation for Ceph has always been supported by CephSnapshotStrategy due to the fact that the _parent_ class of CephSnapshotStrategy is StorageSystemSnapshotStrategy, which handles the deletion of Ceph/RBD snapshots.
   
   However, StorageSystemSnapshotStrategy does not handle backed snapshots via NFS in this context and when Ceph had features introduced the backup on secondary storage was not taken into account.
   
   With the fix proposed by @slavkap, it was possible to check that **nowadays** DefaultSnapshotStrategy supports deletions of Snapshots on Ceph (with or without backup on NFS).




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