davidjumani commented on a change in pull request #3969: Snapshot deletion
issues
URL: https://github.com/apache/cloudstack/pull/3969#discussion_r400787736
##########
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -269,63 +262,87 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete
snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + "
Status");
}
- // first mark the snapshot as destroyed, so that ui can't see it, but
we
- // may not destroy the snapshot on the storage, as other snapshots may
- // depend on it.
SnapshotInfo snapshotOnImage =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug("Can't find snapshot on backup storage, delete it
in db");
- snapshotDao.remove(snapshotId);
- return true;
- }
-
- SnapshotObject obj = (SnapshotObject)snapshotOnImage;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- List<VolumeDetailVO> volumesFromSnapshot;
- volumesFromSnapshot =
_volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId),
null);
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " +
e1.toString());
+ boolean deletedOnSecondary = false;
+ if (snapshotOnImage == null) {
+ s_logger.debug(String.format("Can't find snapshot [snapshot id:
%d] on backup storage", snapshotId));
+ } else {
+ SnapshotObject obj = (SnapshotObject)snapshotOnImage;
+ try {
+ deletedOnSecondary =
deleteSnapshotOnSecondaryStorage(snapshotId, snapshotOnImage, obj);
+ if (!deletedOnSecondary) {
+ s_logger.debug(
+ String.format("Failed to find/delete snapshot (id:
%d) on secondary storage. Still necessary to check and delete snapshot on
primary storage.",
+ snapshotId));
+ } else {
+ s_logger.debug(String.format("Snapshot (id: %d) has been
deleted on secondary storage.", snapshotId));
}
- throw new InvalidParameterValueException("Unable to perform
delete operation, Snapshot with id: " + snapshotId + " is in use ");
+ } catch (NoTransitionException e) {
+ s_logger.debug("Failed to set the state to destroying: ", e);
+ return false;
}
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
- return false;
}
- try {
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- if (result) {
- //snapshot is deleted on backup storage, need to delete it on
primary storage
- SnapshotDataStoreVO snapshotOnPrimary =
snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- if
(((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() ==
StoragePoolType.RBD && volumeVO != null) {
- snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo);
- }
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(),
snapshotOnPrimary);
- }
- }
- } catch (Exception e) {
- s_logger.debug("Failed to delete snapshot: ", e);
+ boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
+ if (deletedOnPrimary) {
+ s_logger.debug(String.format("Successfully deleted snapshot (id:
%d) on primary storage.", snapshotId));
+ } else if (deletedOnSecondary) {
+ s_logger.debug(String.format("The snapshot was deleted on
secondary storage. Could not find/delete snapshot (id: %d) on primary
storage.", snapshotId));
+ }
+ return deletedOnSecondary || deletedOnPrimary;
+ }
+
+ /**
+ * Deletes the snapshot on secondary storage.
+ * 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.
+ */
+ private boolean deleteSnapshotOnSecondaryStorage(Long snapshotId,
SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
+ obj.processEvent(Snapshot.Event.DestroyRequested);
+ List<VolumeDetailVO> volumesFromSnapshot;
+ volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID",
String.valueOf(snapshotId), null);
+
+ if (volumesFromSnapshot.size() > 0) {
try {
obj.processEvent(Snapshot.Event.OperationFailed);
} catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " +
e.toString());
+ s_logger.debug("Failed to change snapshot state: " +
e1.toString());
}
- return false;
+ throw new InvalidParameterValueException("Unable to perform delete
operation, Snapshot with id: " + snapshotId + " is in use ");
}
- return true;
+ boolean result = deleteSnapshotChain(snapshotOnImage);
+ obj.processEvent(Snapshot.Event.OperationSucceeded);
+ return result;
+ }
+
+ /**
+ * Deletes the snapshot on primary storage. It can return false when the
snapshot was not stored on primary storage; however this does not means that it
failed to delete the snapshot. </br>
+ * In case of failure, it will throw one of the following exceptions:
CloudRuntimeException, InterruptedException, or ExecutionException. </br>
+ */
+ private boolean deleteSnapshotOnPrimary(Long snapshotId) {
+ SnapshotDataStoreVO snapshotOnPrimary =
snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
+ SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
+ if (isSnapshotOnPrimaryStorage(snapshotId) &&
snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
+ snapshotOnPrimary.setState(State.Destroyed);
+ snapshotStoreDao.update(snapshotOnPrimary.getId(),
snapshotOnPrimary);
+ snapshotDao.remove(snapshotId);
Review comment:
Perhaps this is leading to the record having a removed date on the snapshots
table which later on causes issues during GC ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
