Skip to content

Fix delete parent snapshot #6630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rohityadavcloud merged 6 commits into from
Oct 13, 2022
Merged

Fix delete parent snapshot #6630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b4932ed
Always mark parent snapshot as destroyed for future garbage collecting
Jul 27, 2022
3a75488
Fix previous and add new unit tests
Jul 27, 2022
4d52598
Return null if there is no entry on secondary storage
Jul 27, 2022
ec2efcc
Fix unit test
Jul 27, 2022
8b210cb
Catch exception on deletion of snapshot on primary storage and improv…
Jul 27, 2022
5e268ce
Fix code smells
Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 53 additions & 11 deletions ...napshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
104 changes: 95 additions & 9 deletions ...hot/src/test/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -40,6 +47,7 @@
@RunWith(MockitoJUnitRunner.class)
public class DefaultSnapshotStrategyTest {

@InjectMocks
DefaultSnapshotStrategy defaultSnapshotStrategySpy = Mockito.spy(DefaultSnapshotStrategy.class);

@Mock
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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));
}
}