Merge pull request #1924 from niteshsarda/CS-50213

CLOUDSTACK-9766 : Executing deleteSnapshot api with already deleted sIf we try to delete the snapshot which is already deleted, then no proper error appears in the log and it just try to delete the snapshot which is already deleted.

Steps to reproduce :
-------
1-create a snapshot
2-delete the snapshot
3-try to delete snapshot which is deleted in step 2

Expected Result
-------------
Result should show proper error message. Request for deleting already deleted snapshot should not be placed.

* pr/1924:
  CLOUDSTACK-9766 : Executing deleteSnapshot api with already deleted snapshot does not throw any exception or failure message

Signed-off-by: Rajani Karuturi <rajani.karuturi@accelerite.com>

engine/schema/src/com/cloud/storage/dao/SnapshotDao.java

@@ -37,6 +37,8 @@ public interface SnapshotDao extends GenericDao<SnapshotVO, Long>, StateDao<Snap
 
     List<SnapshotVO> listByVolumeIdType(long volumeId, Type type);
 
+    List<SnapshotVO> listByVolumeIdTypeNotDestroyed(long volumeId, Type type);
+
     List<SnapshotVO> listByVolumeIdIncludingRemoved(long volumeId);
 
     List<SnapshotVO> listByBackupUuid(long volumeId, String backupUuid);

engine/schema/src/com/cloud/storage/dao/SnapshotDaoImpl.java

@@ -62,6 +62,7 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
 
     private SearchBuilder<SnapshotVO> VolumeIdSearch;
     private SearchBuilder<SnapshotVO> VolumeIdTypeSearch;
+    private SearchBuilder<SnapshotVO> VolumeIdTypeNotDestroyedSearch;
     private SearchBuilder<SnapshotVO> ParentIdSearch;
     private SearchBuilder<SnapshotVO> backupUuidSearch;
     private SearchBuilder<SnapshotVO> VolumeIdVersionSearch;
@@ -95,6 +96,15 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
         return listByVolumeIdType(null, volumeId, type);
     }
 
+    @Override
+    public List<SnapshotVO> listByVolumeIdTypeNotDestroyed(long volumeId, Type type) {
+        SearchCriteria<SnapshotVO> sc = VolumeIdTypeNotDestroyedSearch.create();
+        sc.setParameters("volumeId", volumeId);
+        sc.setParameters("type", type.ordinal());
+        sc.setParameters("status", State.Destroyed);
+        return listBy(sc, null);
+    }
+
     @Override
     public List<SnapshotVO> listByVolumeIdVersion(long volumeId, String version) {
         return listByVolumeIdVersion(null, volumeId, version);
@@ -147,6 +157,12 @@ public class SnapshotDaoImpl extends GenericDaoBase<SnapshotVO, Long> implements
         VolumeIdTypeSearch.and("type", VolumeIdTypeSearch.entity().getsnapshotType(), SearchCriteria.Op.EQ);
         VolumeIdTypeSearch.done();
 
+        VolumeIdTypeNotDestroyedSearch = createSearchBuilder();
+        VolumeIdTypeNotDestroyedSearch.and("volumeId", VolumeIdTypeNotDestroyedSearch.entity().getVolumeId(), SearchCriteria.Op.EQ);
+        VolumeIdTypeNotDestroyedSearch.and("type", VolumeIdTypeNotDestroyedSearch.entity().getsnapshotType(), SearchCriteria.Op.EQ);
+        VolumeIdTypeNotDestroyedSearch.and("status", VolumeIdTypeNotDestroyedSearch.entity().getState(), SearchCriteria.Op.NEQ);
+        VolumeIdTypeNotDestroyedSearch.done();
+
         VolumeIdVersionSearch = createSearchBuilder();
         VolumeIdVersionSearch.and("volumeId", VolumeIdVersionSearch.entity().getVolumeId(), SearchCriteria.Op.EQ);
         VolumeIdVersionSearch.and("version", VolumeIdVersionSearch.entity().getVersion(), SearchCriteria.Op.EQ);

server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java

@@ -481,7 +481,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
         Type type = spstVO.getRecurringType();
         int maxSnaps = type.getMax();
 
-        List<SnapshotVO> snaps = listSnapsforVolumeType(volumeId, type);
+        List<SnapshotVO> snaps = listSnapsforVolumeTypeNotDestroyed(volumeId, type);
         SnapshotPolicyVO policy = _snapshotPolicyDao.findById(policyId);
         if (policy != null && policy.getMaxSnaps() < maxSnaps) {
             maxSnaps = policy.getMaxSnaps();
@@ -514,6 +514,10 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
             throw new InvalidParameterValueException("unable to find a snapshot with id " + snapshotId);
         }
 
+        if (snapshotCheck.getState() == Snapshot.State.Destroyed) {
+            throw new InvalidParameterValueException("Snapshot with id: " + snapshotId + " is already destroyed");
+        }
+
         _accountMgr.checkAccess(caller, null, true, snapshotCheck);
 
         SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshotCheck, SnapshotOperation.DELETE);
@@ -898,8 +902,8 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
         return _snapshotDao.listByVolumeId(volumeId);
     }
 
-    private List<SnapshotVO> listSnapsforVolumeType(long volumeId, Type type) {
-        return _snapshotDao.listByVolumeIdType(volumeId, type);
+    private List<SnapshotVO> listSnapsforVolumeTypeNotDestroyed(long volumeId, Type type) {
+        return _snapshotDao.listByVolumeIdTypeNotDestroyed(volumeId, type);
     }
 
     @Override

server/test/com/cloud/storage/snapshot/SnapshotManagerTest.java

@@ -248,14 +248,13 @@ public class SnapshotManagerTest {
         _snapshotMgr.allocSnapshot(TEST_VOLUME_ID, Snapshot.MANUAL_POLICY_ID, null, null);
     }
 
-    @Test
+    @Test(expected = InvalidParameterValueException.class)
     public void testDeleteSnapshotF1() {
         when(snapshotStrategy.deleteSnapshot(TEST_SNAPSHOT_ID)).thenReturn(true);
         when(snapshotMock.getState()).thenReturn(Snapshot.State.Destroyed);
         when(snapshotMock.getAccountId()).thenReturn(2L);
         when(snapshotMock.getDataCenterId()).thenReturn(2L);
-        boolean result =_snapshotMgr.deleteSnapshot(TEST_SNAPSHOT_ID);
-        Assert.assertTrue(result);
+        _snapshotMgr.deleteSnapshot(TEST_SNAPSHOT_ID);
     }
 
     // vm state not stopped