This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new 7936eb04e9b server: Fix delete parent snapshot (#6630)
7936eb04e9b is described below
commit 7936eb04e9bbdd89bafdf7bdf60393ea00bde61c
Author: Daniel Augusto Veronezi Salvador
<[email protected]>
AuthorDate: Thu Oct 13 04:01:11 2022 -0300
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 [...]
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 [...]
Co-authored-by: GutoVeronezi <[email protected]>
---
.../storage/snapshot/DefaultSnapshotStrategy.java | 64 ++++++++++---
.../snapshot/DefaultSnapshotStrategyTest.java | 104 +++++++++++++++++++--
2 files changed, 148 insertions(+), 20 deletions(-)
diff --git
a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
index 68a9a2991ed..85e0a02c5f7 100644
---
a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
+++
b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
@@ -23,6 +23,10 @@ import java.util.Map;
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 @@ public class DefaultSnapshotStrategy extends
SnapshotStrategyBase {
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.
diff --git
a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java
b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java
index 2143311c3bf..a092f8f108e 100644
---
a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java
+++
b/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 @@ import com.cloud.utils.fsm.NoTransitionException;
@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 class DefaultSnapshotStrategyTest {
@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 class DefaultSnapshotStrategyTest {
}
@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 class DefaultSnapshotStrategyTest {
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));
+ }
}
