Skip to content

Commit f18fe5e

Browse files
DaanHooglandGabrielBrascherandrijapanicsbharikrishna-patnalanvazquez
authored
Snapshot deletion issues (apache#3969)
* Fixes snapshot deletion * Remove legacy '@component', it is not necessary in this bean/class. * Fix log message missing %d and remove snapshot on DB * Remove "dummy" boolean return statement * Manage snapshot deletion for KVM + NFS (primary storage) * checkstyle trailing spaces * rename options strings to *_OPTION * Fix typo on deleteSnapshotOnSecondaryStorage and enhance log message * Move the snapshotDao.remove(snapshotId); (apache#4006) * Fix deletesnapshot worflow to handle both snapshots created in primary storage and snapshots backed up to secondary storage * Fix extra space * refactor out separate handling methods for secondary and primary (reducing returns) * return false on unexpected error or log when expected * != instead of == * secondary instead of backup storage * init to null * Handle snapshot deletion on primary storage. When primary store ref not found for snapshot do not fail the operation. * Fix debug levels on log messages Co-authored-by: GabrielBrascher <gabriel@apache.org> Co-authored-by: Andrija Panic <45762285+andrijapanicsb@users.noreply.github.com> Co-authored-by: Harikrishna Patnala <harikrishna.patnala@gmail.com> Co-authored-by: nvazquez <nicovazquez90@gmail.com>
1 parent e0b67a4 commit f18fe5e

5 files changed

Lines changed: 166 additions & 80 deletions

File tree

‎engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
<bean id="dataStoreProviderManagerImpl" class="org.apache.cloudstack.storage.datastore.provider.DataStoreProviderManagerImpl" />
5050
<bean id="storageCacheManagerImpl" class="org.apache.cloudstack.storage.cache.manager.StorageCacheManagerImpl" />
5151
<bean id="storageCacheRandomAllocator" class="org.apache.cloudstack.storage.cache.allocator.StorageCacheRandomAllocator" />
52-
<bean id="xenserverSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
52+
<bean id="defaultSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />
5353
<bean id="bAREMETAL" class="org.apache.cloudstack.storage.image.format.BAREMETAL" />
5454
<bean id="dataMotionServiceImpl" class="org.apache.cloudstack.storage.motion.DataMotionServiceImpl" />
5555
<bean id="dataObjectManagerImpl" class="org.apache.cloudstack.storage.datastore.DataObjectManagerImpl" />

‎engine/storage/integration-test/src/test/resources/storageContext.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
<bean id="ancientDataMotionStrategy" class="org.apache.cloudstack.storage.motion.AncientDataMotionStrategy" />
5151
<bean id="storageCacheManagerImpl" class="org.apache.cloudstack.storage.cache.manager.StorageCacheManagerImpl" />
5252
<bean id="storageCacheRandomAllocator" class="org.apache.cloudstack.storage.cache.allocator.StorageCacheRandomAllocator" />
53-
<bean id="xenserverSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
53+
<bean id="defaultSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />
5454
<bean id="bAREMETAL" class="org.apache.cloudstack.storage.image.format.BAREMETAL" />
5555
<bean id="dataMotionServiceImpl" class="org.apache.cloudstack.storage.motion.DataMotionServiceImpl" />
5656
<bean id="dataObjectManagerImpl" class="org.apache.cloudstack.storage.datastore.DataObjectManagerImpl" />

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

Lines changed: 124 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import javax.inject.Inject;
2222

2323
import org.apache.log4j.Logger;
24-
import org.springframework.stereotype.Component;
2524
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
2625
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
2726
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event;
@@ -34,7 +33,6 @@
3433
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
3534
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
3635
import org.apache.cloudstack.framework.jobs.AsyncJob;
37-
import org.apache.cloudstack.framework.jobs.dao.SyncQueueItemDao;
3836
import org.apache.cloudstack.storage.command.CreateObjectAnswer;
3937
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
4038
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
@@ -71,9 +69,8 @@
7169
import com.cloud.utils.exception.CloudRuntimeException;
7270
import com.cloud.utils.fsm.NoTransitionException;
7371

74-
@Component
75-
public class XenserverSnapshotStrategy extends SnapshotStrategyBase {
76-
private static final Logger s_logger = Logger.getLogger(XenserverSnapshotStrategy.class);
72+
public class DefaultSnapshotStrategy extends SnapshotStrategyBase {
73+
private static final Logger s_logger = Logger.getLogger(DefaultSnapshotStrategy.class);
7774

7875
@Inject
7976
SnapshotService snapshotSvr;
@@ -90,12 +87,8 @@ public class XenserverSnapshotStrategy extends SnapshotStrategyBase {
9087
@Inject
9188
SnapshotDataFactory snapshotDataFactory;
9289
@Inject
93-
private SnapshotDao _snapshotDao;
94-
@Inject
9590
private SnapshotDetailsDao _snapshotDetailsDao;
9691
@Inject
97-
private SyncQueueItemDao _syncQueueItemDao;
98-
@Inject
9992
VolumeDetailsDao _volumeDetailsDaoImpl;
10093

10194
@Override
@@ -264,68 +257,147 @@ public boolean deleteSnapshot(Long snapshotId) {
264257
return true;
265258
}
266259

267-
if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) && !Snapshot.State.Error.equals(snapshotVO.getState()) &&
260+
if (!Snapshot.State.BackedUp.equals(snapshotVO.getState()) &&
268261
!Snapshot.State.Destroying.equals(snapshotVO.getState())) {
269262
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
270263
}
271264

272-
// first mark the snapshot as destroyed, so that ui can't see it, but we
273-
// may not destroy the snapshot on the storage, as other snapshots may
274-
// depend on it.
275-
SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
276-
if (snapshotOnImage == null) {
277-
s_logger.debug("Can't find snapshot on backup storage, delete it in db");
278-
snapshotDao.remove(snapshotId);
279-
return true;
280-
}
265+
Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
266+
boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
281267

282-
SnapshotObject obj = (SnapshotObject)snapshotOnImage;
283-
try {
284-
obj.processEvent(Snapshot.Event.DestroyRequested);
285-
List<VolumeDetailVO> volumesFromSnapshot;
286-
volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
268+
if (deletedOnPrimary) {
269+
s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
270+
} else {
271+
s_logger.debug(String.format("The snapshot (id: %d) could not be found/deleted on primary storage.", snapshotId));
272+
}
273+
if (null != deletedOnSecondary && deletedOnSecondary) {
274+
s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on secondary storage.", snapshotId));
275+
}
276+
return (deletedOnSecondary != null) && deletedOnSecondary || deletedOnPrimary;
277+
}
287278

288-
if (volumesFromSnapshot.size() > 0) {
279+
private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
280+
SnapshotVO snapshotVO;
281+
boolean deletedOnPrimary = false;
282+
snapshotVO = snapshotDao.findById(snapshotId);
283+
SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
284+
if (snapshotVO != null && snapshotVO.getState() == Snapshot.State.Destroyed) {
285+
deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
286+
} else {
287+
// Here we handle snapshots which are to be deleted but are not marked as destroyed yet.
288+
// This may occur for instance when they are created only on primary because
289+
// snapshot.backup.to.secondary was set to false.
290+
if (snapshotOnPrimaryInfo == null) {
291+
if (s_logger.isDebugEnabled()) {
292+
s_logger.debug(String.format("Snapshot (id: %d) not found on primary storage, skipping snapshot deletion on primary and marking it destroyed", snapshotId));
293+
}
294+
snapshotVO.setState(Snapshot.State.Destroyed);
295+
snapshotDao.update(snapshotId, snapshotVO);
296+
deletedOnPrimary = true;
297+
} else {
298+
SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
289299
try {
290-
obj.processEvent(Snapshot.Event.OperationFailed);
291-
} catch (NoTransitionException e1) {
292-
s_logger.debug("Failed to change snapshot state: " + e1.toString());
300+
obj.processEvent(Snapshot.Event.DestroyRequested);
301+
deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId, snapshotOnPrimaryInfo);
302+
if (!deletedOnPrimary) {
303+
obj.processEvent(Snapshot.Event.OperationFailed);
304+
} else {
305+
obj.processEvent(Snapshot.Event.OperationSucceeded);
306+
}
307+
} catch (NoTransitionException e) {
308+
s_logger.debug("Failed to set the state to destroying: ", e);
309+
deletedOnPrimary = false;
293310
}
294-
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
295311
}
296-
} catch (NoTransitionException e) {
297-
s_logger.debug("Failed to set the state to destroying: ", e);
298-
return false;
299312
}
313+
return deletedOnPrimary;
314+
}
300315

301-
try {
302-
boolean result = deleteSnapshotChain(snapshotOnImage);
303-
obj.processEvent(Snapshot.Event.OperationSucceeded);
304-
if (result) {
305-
//snapshot is deleted on backup storage, need to delete it on primary storage
306-
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
307-
if (snapshotOnPrimary != null) {
308-
SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
309-
long volumeId = snapshotOnPrimary.getVolumeId();
310-
VolumeVO volumeVO = volumeDao.findById(volumeId);
311-
if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD && volumeVO != null) {
312-
snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo);
313-
}
314-
snapshotOnPrimary.setState(State.Destroyed);
315-
snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
316+
private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
317+
Boolean deletedOnSecondary = null;
318+
SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
319+
if (snapshotOnImage == null) {
320+
s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on secondary storage", snapshotId));
321+
} else {
322+
SnapshotObject obj = (SnapshotObject)snapshotOnImage;
323+
try {
324+
deletedOnSecondary = deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
325+
if (!deletedOnSecondary) {
326+
s_logger.debug(
327+
String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
328+
snapshotId));
329+
} else {
330+
s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
316331
}
332+
} catch (NoTransitionException e) {
333+
s_logger.debug("Failed to set the state to destroying: ", e);
334+
// deletedOnSecondary remain null
317335
}
318-
} catch (Exception e) {
319-
s_logger.debug("Failed to delete snapshot: ", e);
336+
}
337+
return deletedOnSecondary;
338+
}
339+
340+
/**
341+
* Deletes the snapshot on secondary storage.
342+
* It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
343+
*/
344+
private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
345+
obj.processEvent(Snapshot.Event.DestroyRequested);
346+
List<VolumeDetailVO> volumesFromSnapshot;
347+
volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);
348+
349+
if (volumesFromSnapshot.size() > 0) {
320350
try {
321351
obj.processEvent(Snapshot.Event.OperationFailed);
322352
} catch (NoTransitionException e1) {
323-
s_logger.debug("Failed to change snapshot state: " + e.toString());
353+
s_logger.debug("Failed to change snapshot state: " + e1.toString());
354+
}
355+
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
356+
}
357+
358+
boolean result = deleteSnapshotChain(snapshotOnImage);
359+
obj.processEvent(Snapshot.Event.OperationSucceeded);
360+
return result;
361+
}
362+
363+
/**
364+
* Deletes the snapshot on primary storage. It returns true when the snapshot was not found on primary storage; </br>
365+
* In case of failure while deleting the snapshot, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
366+
*/
367+
private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo snapshotOnPrimaryInfo) {
368+
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
369+
if (isSnapshotOnPrimaryStorage(snapshotId)) {
370+
if (s_logger.isDebugEnabled()) {
371+
s_logger.debug(String.format("Snapshot reference is found on primary storage for snapshot id: %d, performing snapshot deletion on primary", snapshotId));
372+
}
373+
if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
374+
snapshotOnPrimary.setState(State.Destroyed);
375+
snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
376+
if (s_logger.isDebugEnabled()) {
377+
s_logger.debug(String.format("Successfully deleted snapshot id: %d on primary storage", snapshotId));
378+
}
379+
return true;
324380
}
325-
return false;
381+
} else {
382+
if (s_logger.isDebugEnabled()) {
383+
s_logger.debug(String.format("Snapshot reference is not found on primary storage for snapshot id: %d, skipping snapshot deletion on primary", snapshotId));
384+
}
385+
return true;
326386
}
387+
return false;
388+
}
327389

328-
return true;
390+
/**
391+
* Returns true if the snapshot volume is on primary storage.
392+
*/
393+
private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
394+
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
395+
if (snapshotOnPrimary != null) {
396+
long volumeId = snapshotOnPrimary.getVolumeId();
397+
VolumeVO volumeVO = volumeDao.findById(volumeId);
398+
return volumeVO != null && volumeVO.getRemoved() == null;
399+
}
400+
return false;
329401
}
330402

331403
@Override

‎engine/storage/snapshot/src/main/resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@
2727
http://www.springframework.org/schema/context/spring-context.xsd"
2828
>
2929

30-
<bean id="xenserverSnapshotStrategy"
31-
class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
30+
<bean id="defaultSnapshotStrategy"
31+
class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />
3232

3333
<bean id="storageSystemSnapshotStrategy"
3434
class="org.apache.cloudstack.storage.snapshot.StorageSystemSnapshotStrategy" />

0 commit comments

Comments
 (0)