server: Fix delete parent snapshot (#6630)

ACS + Xenserver works with differential snapshots. ACS takes a volume full snapshot and the next ones are referenced as a child of the previous snapshot until the chain reaches the limit defined in the global setting snapshot.delta.max; then, a new full snapshot is taken. PR #5297 introduced disk-only snapshots for KVM volumes. Among the changes, the delete process was also refactored. Before the changes, when one was removing a snapshot with children, ACS was marking it as Destroyed and it was keeping the Image entry on the table cloud.snapshot_store_ref as Ready. When ACS was rotating the snapshots (the max delta was reached) and all the children were already marked as removed; then, ACS would start removing the whole hierarchy, completing the differential snapshot cycle. After the changes, the snapshots with children stopped being marked as removed and the differential snapshot cycle was not being completed.

This PR intends to honor again the differential snapshot cycle for XenServer, making the snapshots to be marked as removed when deleted while having children and following the differential snapshot cycle.

Also, when one takes a volume snapshot and ACS backs it up to the secondary storage, ACS inserts 2 entries on table cloud.snapshot_store_ref (Primary and Image). When one deletes a volume snapshot, ACS first tries to remove the snapshot from the secondary storage and mark the entry Image as removed; then, it tries to remove the snapshot from the primary storage and mark the entry Primary as removed. If ACS cannot remove the snapshot from the primary storage, it will keep the snapshot as BackedUp; however, If it does not exist in the secondary storage and without the entry SNAPSHOT.DELETE on cloud.usage_event. In the end, after the garbage collector flow, the snapshot will be marked as BackedUp, with a value in the field removed and still being rated. This PR also addresses the correction for this situation.

Co-authored-by: GutoVeronezi <[email protected]>

Author: GutoVeronezi

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java

@@ -23,6 +23,10 @@
 
 import javax.inject.Inject;
 
+import com.cloud.storage.VolumeDetailVO;
+import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang3.BooleanUtils;
 import org.apache.log4j.Logger;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
@@ -302,47 +306,85 @@ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
     protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
         Map<String, SnapshotInfo> snapshotInfos = retrieveSnapshotEntries(snapshotVo.getId());
 
+        boolean result = false;
         for (var infoEntry : snapshotInfos.entrySet()) {
-            if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo)) {
-                return false;
+            if (BooleanUtils.toBooleanDefaultIfNull(deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(), snapshotVo), false)) {
+                result = true;
             }
         }
 
-        return true;
+        return result;
     }
 
     /**
      * Destroys the snapshot entry and file.
      * @return true if destroy successfully, else false.
      */
-    protected boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
+    protected Boolean deleteSnapshotInfo(SnapshotInfo snapshotInfo, String storage, SnapshotVO snapshotVo) {
         if (snapshotInfo == null) {
-            s_logger.debug(String.format("Could not find %s entry on a %s. Skipping deletion on %s.", snapshotVo, storage, storage));
-            return true;
+            s_logger.debug(String.format("Could not find %s entry on %s. Skipping deletion on %s.", snapshotVo, storage, storage));
+            return SECONDARY_STORAGE_SNAPSHOT_ENTRY_IDENTIFIER.equals(storage) ? null : true;
         }
 
         DataStore dataStore = snapshotInfo.getDataStore();
-        storage = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
+        String storageToString = String.format("%s {uuid: \"%s\", name: \"%s\"}", storage, dataStore.getUuid(), dataStore.getName());
 
         try {
             SnapshotObject snapshotObject = castSnapshotInfoToSnapshotObject(snapshotInfo);
             snapshotObject.processEvent(Snapshot.Event.DestroyRequested);
 
-            if (deleteSnapshotChain(snapshotInfo, storage)) {
+            if (SECONDARY_STORAGE_SNAPSHOT_ENTRY_IDENTIFIER.equals(storage)) {
+
+                verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObject);
+
+                if (deleteSnapshotChain(snapshotInfo, storageToString)) {
+                    s_logger.debug(String.format("%s was deleted on %s. We will mark the snapshot as destroyed.", snapshotVo, storageToString));
+                } else {
+                    s_logger.debug(String.format("%s was not deleted on %s; however, we will mark the snapshot as destroyed for future garbage collecting.", snapshotVo,
+                        storageToString));
+                }
+
                 snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
-                s_logger.debug(String.format("%s was deleted on %s.", snapshotVo, storage));
+                return true;
+            } else if (deleteSnapshotInPrimaryStorage(snapshotInfo, snapshotVo, storageToString, snapshotObject)) {
                 return true;
             }
 
+            s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storageToString));
             snapshotObject.processEvent(Snapshot.Event.OperationFailed);
-            s_logger.debug(String.format("Failed to delete %s on %s.", snapshotVo, storage));
         } catch (NoTransitionException ex) {
-            s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storage, ex.getMessage()), ex);
+            s_logger.warn(String.format("Failed to delete %s on %s due to %s.", snapshotVo, storageToString, ex.getMessage()), ex);
         }
 
         return false;
     }
 
+    protected boolean deleteSnapshotInPrimaryStorage(SnapshotInfo snapshotInfo, SnapshotVO snapshotVo, String storageToString, SnapshotObject snapshotObject) throws NoTransitionException {
+        try {
+            if (snapshotSvr.deleteSnapshot(snapshotInfo)) {
+                snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+                s_logger.debug(String.format("%s was deleted on %s. We will mark the snapshot as destroyed.", snapshotVo, storageToString));
+                return true;
+            }
+        } catch (CloudRuntimeException ex) {
+            s_logger.warn(String.format("Unable do delete snapshot %s on %s due to [%s]. The reference will be marked as 'Destroying' for future garbage collecting.",
+                    snapshotVo, storageToString, ex.getMessage()), ex);
+        }
+        return false;
+    }
+
+    protected void verifyIfTheSnapshotIsBeingUsedByAnyVolume(SnapshotObject snapshotObject) throws NoTransitionException {
+        List<VolumeDetailVO> volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotObject.getSnapshotId()), null);
+        if (CollectionUtils.isEmpty(volumesFromSnapshot)) {
+            return;
+        }
+
+        snapshotObject.processEvent(Snapshot.Event.OperationFailed);
+        throw new CloudRuntimeException(String.format("Unable to delete snapshot [%s] because it is being used by the following volumes: %s.",
+            ReflectionToStringBuilderUtils.reflectOnlySelectedFields(snapshotObject.getSnapshotVO(), "id", "uuid", "volumeId", "name"),
+            ReflectionToStringBuilderUtils.reflectOnlySelectedFields(volumesFromSnapshot, "resourceId")));
+    }
+
     /**
      * Cast SnapshotInfo to SnapshotObject.
      * @return SnapshotInfo cast to SnapshotObject.

engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java

@@ -17,16 +17,23 @@
 
 package org.apache.cloudstack.storage.snapshot;
 
+import java.util.ArrayList;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 
+import com.cloud.storage.VolumeDetailVO;
+import com.cloud.storage.dao.VolumeDetailsDao;
+import com.cloud.utils.exception.CloudRuntimeException;
 import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
@@ -40,6 +47,7 @@
 @RunWith(MockitoJUnitRunner.class)
 public class DefaultSnapshotStrategyTest {
 
+    @InjectMocks
     DefaultSnapshotStrategy defaultSnapshotStrategySpy = Mockito.spy(DefaultSnapshotStrategy.class);
 
     @Mock
@@ -60,15 +68,18 @@ public class DefaultSnapshotStrategyTest {
     @Mock
     DataStore dataStoreMock;
 
+    @Mock
+    VolumeDetailsDao volumeDetailsDaoMock;
+
+    @Mock
+    SnapshotService snapshotServiceMock;
+
     Map<String, SnapshotInfo> mapStringSnapshotInfoInstance = new LinkedHashMap<>();
 
     @Before
     public void setup() {
-        defaultSnapshotStrategySpy.snapshotDataFactory = snapshotDataFactoryMock;
-        defaultSnapshotStrategySpy.snapshotDao = snapshotDaoMock;
-
         mapStringSnapshotInfoInstance.put("secondary storage", snapshotInfo1Mock);
-        mapStringSnapshotInfoInstance.put("priamry storage", snapshotInfo1Mock);
+        mapStringSnapshotInfoInstance.put("primary storage", snapshotInfo1Mock);
     }
 
     @Test
@@ -122,7 +133,7 @@ public void validateDeleteSnapshotInfosDeletesSuccessfullyReturnsTrue() {
 
     @Test
     public void validateDeleteSnapshotInfoSnapshotInfoIsNullOnSecondaryStorageReturnsTrue() {
-        Assert.assertTrue(defaultSnapshotStrategySpy.deleteSnapshotInfo(null, "secondary storage", snapshotVoMock));
+        Assert.assertNull(defaultSnapshotStrategySpy.deleteSnapshotInfo(null, "secondary storage", snapshotVoMock));
     }
 
     @Test
@@ -131,24 +142,60 @@ public void validateDeleteSnapshotInfoSnapshotInfoIsNullOnPrimaryStorageReturnsF
     }
 
     @Test
-    public void validateDeleteSnapshotInfoSnapshotDeleteSnapshotChainSuccessfullyReturnsTrue() throws NoTransitionException {
+    public void deleteSnapshotInfoTestReturnTrueIfCanDeleteTheSnapshotOnPrimaryStorage() throws NoTransitionException {
+        Mockito.doReturn(dataStoreMock).when(snapshotInfo1Mock).getDataStore();
+        Mockito.doReturn(snapshotObjectMock).when(defaultSnapshotStrategySpy).castSnapshotInfoToSnapshotObject(snapshotInfo1Mock);
+        Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
+        Mockito.doReturn(true).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+
+        boolean result = defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "primary storage", snapshotVoMock);
+        Assert.assertTrue(result);
+    }
+
+    @Test
+    public void deleteSnapshotInfoTestReturnFalseIfCannotDeleteTheSnapshotOnPrimaryStorage() throws NoTransitionException {
+        Mockito.doReturn(dataStoreMock).when(snapshotInfo1Mock).getDataStore();
+        Mockito.doReturn(snapshotObjectMock).when(defaultSnapshotStrategySpy).castSnapshotInfoToSnapshotObject(snapshotInfo1Mock);
+        Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
+        Mockito.doReturn(false).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+
+        boolean result = defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "primary storage", snapshotVoMock);
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void deleteSnapshotInfoTestReturnFalseIfDeleteSnapshotOnPrimaryStorageThrowsACloudRuntimeException() throws NoTransitionException {
+        Mockito.doReturn(dataStoreMock).when(snapshotInfo1Mock).getDataStore();
+        Mockito.doReturn(snapshotObjectMock).when(defaultSnapshotStrategySpy).castSnapshotInfoToSnapshotObject(snapshotInfo1Mock);
+        Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
+        Mockito.doThrow(CloudRuntimeException.class).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+
+        boolean result = defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "primary storage", snapshotVoMock);
+        Assert.assertFalse(result);
+    }
+
+    @Test
+    public void deleteSnapshotInfoTestReturnTrueIfCanDeleteTheSnapshotChainForSecondaryStorage() throws NoTransitionException {
         Mockito.doReturn(dataStoreMock).when(snapshotInfo1Mock).getDataStore();
         Mockito.doReturn(snapshotObjectMock).when(defaultSnapshotStrategySpy).castSnapshotInfoToSnapshotObject(snapshotInfo1Mock);
+        Mockito.doNothing().when(defaultSnapshotStrategySpy).verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
         Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
         Mockito.doReturn(true).when(defaultSnapshotStrategySpy).deleteSnapshotChain(Mockito.any(), Mockito.anyString());
 
-        Assert.assertTrue(defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "secondary storage", snapshotVoMock));
+        boolean result = defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "secondary storage", snapshotVoMock);
+        Assert.assertTrue(result);
     }
 
     @Test
-    public void validateDeleteSnapshotInfoSnapshotDeleteSnapshotChainFails() throws NoTransitionException {
+    public void deleteSnapshotInfoTestReturnTrueIfCannotDeleteTheSnapshotChainForSecondaryStorage() throws NoTransitionException {
         Mockito.doReturn(dataStoreMock).when(snapshotInfo1Mock).getDataStore();
         Mockito.doReturn(snapshotObjectMock).when(defaultSnapshotStrategySpy).castSnapshotInfoToSnapshotObject(snapshotInfo1Mock);
+        Mockito.doNothing().when(defaultSnapshotStrategySpy).verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
         Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
         Mockito.doReturn(false).when(defaultSnapshotStrategySpy).deleteSnapshotChain(Mockito.any(), Mockito.anyString());
 
         boolean result = defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "secondary storage", snapshotVoMock);
-        Assert.assertFalse(result);
+        Assert.assertTrue(result);
     }
 
     @Test
@@ -159,4 +206,43 @@ public void validateDeleteSnapshotInfoSnapshotProcessSnapshotEventThrowsNoTransi
 
         Assert.assertFalse(defaultSnapshotStrategySpy.deleteSnapshotInfo(snapshotInfo1Mock, "secondary storage", snapshotVoMock));
     }
+
+    @Test
+    public void verifyIfTheSnapshotIsBeingUsedByAnyVolumeTestDetailsIsEmptyDoNothing() throws NoTransitionException {
+        Mockito.doReturn(new ArrayList<>()).when(volumeDetailsDaoMock).findDetails(Mockito.any(), Mockito.any(), Mockito.any());
+        defaultSnapshotStrategySpy.verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
+        Mockito.verify(snapshotObjectMock, Mockito.never()).processEvent(Mockito.any(Snapshot.Event.class));
+    }
+
+    @Test
+    public void verifyIfTheSnapshotIsBeingUsedByAnyVolumeTestDetailsIsNullDoNothing() throws NoTransitionException {
+        Mockito.doReturn(null).when(volumeDetailsDaoMock).findDetails(Mockito.any(), Mockito.any(), Mockito.any());
+        defaultSnapshotStrategySpy.verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
+        Mockito.verify(snapshotObjectMock, Mockito.never()).processEvent(Mockito.any(Snapshot.Event.class));
+    }
+
+    @Test(expected = CloudRuntimeException.class)
+    public void verifyIfTheSnapshotIsBeingUsedByAnyVolumeTestDetailsIsNotEmptyThrowCloudRuntimeException() throws NoTransitionException {
+        Mockito.doReturn(List.of(new VolumeDetailVO())).when(volumeDetailsDaoMock).findDetails(Mockito.any(), Mockito.any(), Mockito.any());
+        defaultSnapshotStrategySpy.verifyIfTheSnapshotIsBeingUsedByAnyVolume(snapshotObjectMock);
+    }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnTrueIfDeleteReturnsTrue() throws NoTransitionException {
+        Mockito.doReturn(true).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Mockito.doNothing().when(snapshotObjectMock).processEvent(Mockito.any(Snapshot.Event.class));
+        Assert.assertTrue(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, snapshotObjectMock));
+    }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnFalseIfDeleteReturnsFalse() throws NoTransitionException {
+        Mockito.doReturn(false).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Assert.assertFalse(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, null));
+    }
+
+    @Test
+    public void deleteSnapshotInPrimaryStorageTestReturnFalseIfDeleteThrowsException() throws NoTransitionException {
+        Mockito.doThrow(CloudRuntimeException.class).when(snapshotServiceMock).deleteSnapshot(Mockito.any());
+        Assert.assertFalse(defaultSnapshotStrategySpy.deleteSnapshotInPrimaryStorage(null, null, null, null));
+    }
 }