address reviews

Author: JoaoJandre

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

@@ -332,6 +332,9 @@ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo, Long zon
         } else {
             snapshotZoneDao.removeSnapshotFromZones(snapshotVo.getId());
         }
+
+        updateEndOfChainIfNeeded(snapshotVo);
+
         if (CollectionUtils.isNotEmpty(retrieveSnapshotEntries(snapshotVo.getId(), null))) {
             return true;
         }
@@ -340,13 +343,9 @@ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo, Long zon
     }
 
     /**
-     * Updates the snapshot to {@link Snapshot.State#Destroyed}. If using the KVM hypervisor and the snapshot was the end of a chain,
-     * will mark their parents as end of chain as well.
-     */
-    protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
-        snapshotVo.setState(Snapshot.State.Destroyed);
-        snapshotDao.update(snapshotVo.getId(), snapshotVo);
-
+     * If using the KVM hypervisor and the snapshot was the end of a chain, will mark their parents as end of chain.
+     * */
+    protected void updateEndOfChainIfNeeded(SnapshotVO snapshotVo) {
         if (!HypervisorType.KVM.equals(snapshotVo.getHypervisorType())) {
             return;
         }
@@ -367,7 +366,14 @@ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
             parentSnapshotDatastoreVo.setEndOfChain(true);
             snapshotStoreDao.update(parentSnapshotDatastoreVo.getId(), parentSnapshotDatastoreVo);
         }
+    }
 
+    /**
+     * Updates the snapshot to {@link Snapshot.State#Destroyed}.
+     */
+    protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+        snapshotVo.setState(Snapshot.State.Destroyed);
+        snapshotDao.update(snapshotVo.getId(), snapshotVo);
     }
 
     protected List<SnapshotDataStoreVO> findLastAliveAncestors(long snapshotId) {

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

@@ -111,32 +111,30 @@ public void validateRetrieveSnapshotEntries() {
     }
 
     @Test
-    public void updateSnapshotToDestroyedTestNotKvm() {
+    public void updateEndOfChainIfNeededTestNotKvm() {
         Mockito.doReturn(Hypervisor.HypervisorType.VMware).when(snapshotVoMock).getHypervisorType();
 
-        defaultSnapshotStrategySpy.updateSnapshotToDestroyed(snapshotVoMock);
+        defaultSnapshotStrategySpy.updateEndOfChainIfNeeded(snapshotVoMock);
 
-        Mockito.verify(snapshotVoMock).setState(Snapshot.State.Destroyed);
-        Mockito.verify(snapshotVoMock, Mockito.never()).getSnapshotId();
+        Mockito.verify(snapshotDataStoreDao, Mockito.never()).findBySnapshotIdAndDataStoreRoleAndState(Mockito.anyLong(), Mockito.any(), Mockito.any());
     }
 
     @Test
-    public void updateSnapshotToDestroyedTestKvmAndIsNotEndOfChain() {
+    public void updateEndOfChainIfNeededTestKvmAndIsNotEndOfChain() {
         Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(snapshotVoMock).getHypervisorType();
         Mockito.doReturn(2L).when(snapshotVoMock).getSnapshotId();
 
         SnapshotDataStoreVO snapshotDataStoreVO = new SnapshotDataStoreVO();
         snapshotDataStoreVO.setEndOfChain(false);
         Mockito.doReturn(snapshotDataStoreVO).when(snapshotDataStoreDao).findBySnapshotIdAndDataStoreRoleAndState(2, DataStoreRole.Image, ObjectInDataStoreStateMachine.State.Destroyed);
 
-        defaultSnapshotStrategySpy.updateSnapshotToDestroyed(snapshotVoMock);
+        defaultSnapshotStrategySpy.updateEndOfChainIfNeeded(snapshotVoMock);
 
-        Mockito.verify(snapshotVoMock).setState(Snapshot.State.Destroyed);
         Mockito.verify(snapshotDataStoreDao, Mockito.never()).update(Mockito.anyLong(), Mockito.any());
     }
 
     @Test
-    public void updateSnapshotToDestroyedTestKvmAndIsEndOfChain() {
+    public void updateEndOfChainIfNeededTestKvmAndIsEndOfChain() {
         Mockito.doReturn(Hypervisor.HypervisorType.KVM).when(snapshotVoMock).getHypervisorType();
         Mockito.doReturn(2L).when(snapshotVoMock).getSnapshotId();
 
@@ -148,9 +146,8 @@ public void updateSnapshotToDestroyedTestKvmAndIsEndOfChain() {
         Mockito.doReturn(ObjectInDataStoreStateMachine.State.Ready).when(snapshotDataStoreVOMock).getState();
         Mockito.doReturn(List.of(snapshotDataStoreVOMock)).when(snapshotDataStoreDao).listBySnapshotId(8);
 
-        defaultSnapshotStrategySpy.updateSnapshotToDestroyed(snapshotVoMock);
+        defaultSnapshotStrategySpy.updateEndOfChainIfNeeded(snapshotVoMock);
 
-        Mockito.verify(snapshotVoMock).setState(Snapshot.State.Destroyed);
         Mockito.verify(snapshotDataStoreDao, Mockito.times(1)).listBySnapshotId(Mockito.anyLong());
         Mockito.verify(snapshotDataStoreVOMock).setEndOfChain(true);
         Mockito.verify(snapshotDataStoreDao, Mockito.times(1)).update(Mockito.anyLong(), Mockito.any());

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

@@ -1545,7 +1545,11 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
         boolean backupSnapToSecondary = isBackupSnapshotToSecondaryForZone(volume.getDataCenterId());
 
         if (isKvmAndFileBasedStorage && backupSnapToSecondary) {
-            snapshot.setImageStore(snapshotSrv.findSnapshotImageStore(snapshot));
+            DataStore imageStore = snapshotSrv.findSnapshotImageStore(snapshot);
+            if (imageStore == null) {
+                throw new CloudRuntimeException(String.format("Could not find any secondary storage to allocate snapshot [%s].", snapshot));
+            }
+            snapshot.setImageStore(imageStore);
         }
 
         updateSnapshotPayload(volume.getPoolId(), payload, isKvmAndFileBasedStorage, clusterId);
@@ -1918,6 +1922,15 @@ private boolean checkAndProcessSnapshotAlreadyExistInStore(long snapshotId, Data
             }
             return true; // already downloaded on this image store
         }
+
+        if (ObjectInDataStoreStateMachine.State.Hidden.equals(dstSnapshotStore.getState())) {
+            logger.debug("Snapshot [{}] was hidden in secondary storage [{}]. Since the user asked to copy it back. We will put it as ready.", dstSnapshotStore, dstSecStore);
+            dstSnapshotStore.setState(ObjectInDataStoreStateMachine.State.Ready);
+            dstSnapshotStore.setDisplay(true);
+            _snapshotStoreDao.update(dstSnapshotStore.getId(), dstSnapshotStore);
+            return true;
+        }
+
         if ((dstSnapshotStore.getDownloadState() != null && List.of(VMTemplateStorageResourceAssoc.Status.ABANDONED,
                 VMTemplateStorageResourceAssoc.Status.DOWNLOAD_ERROR,
                 VMTemplateStorageResourceAssoc.Status.NOT_DOWNLOADED,