nvazquez commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r694175284
##########
File path:
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
##########
@@ -575,6 +577,7 @@ protected Answer copySnapshot(DataObject srcData,
DataObject destData) {
}
Map<String, String> options = new HashMap<String, String>();
options.put("fullSnapshot", fullSnapshot.toString());
+ options.put(BackupSnapshotAfterTakingSnapshot.key(),
String.valueOf(BackupSnapshotAfterTakingSnapshot.value()));
Review comment:
Also here
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -100,20 +111,23 @@ public Answer execute(final RevertSnapshotCommand
command, final LibvirtComputin
rbd.close(image);
rados.ioCtxDestroy(io);
} else {
- NfsTO nfsImageStore = (NfsTO)snapshotImageStore;
- String secondaryStoragePoolUrl = nfsImageStore.getUrl();
- secondaryStoragePool =
storagePoolMgr.getStoragePoolByURI(secondaryStoragePoolUrl);
- String ssPmountPath = secondaryStoragePool.getLocalPath();
- snapshotPath = ssPmountPath + File.separator + snapshotRelPath;
-
- Script cmd = new
Script(libvirtComputingResource.manageSnapshotPath(),
libvirtComputingResource.getCmdsTimeout(), s_logger);
- cmd.add("-v", snapshotPath);
- cmd.add("-n", snapshotDisk.getName());
- cmd.add("-p", snapshotDisk.getPath());
- String result = cmd.execute();
- if (result != null) {
- s_logger.debug("Failed to revert snaptshot: " + result);
- return new Answer(command, false, result);
+ KVMStoragePool secondaryStoragePool = null;
+ if (snapshotImageStore != null && DataStoreRole.Primary !=
snapshotImageStore.getRole()) {
+
storagePoolMgr.getStoragePoolByURI(snapshotImageStore.getUrl());
+ }
+
+ if (primaryPool.getType() == StoragePoolType.CLVM) {
Review comment:
Does it need to be part of `storagePoolTypesThatSupportRevertSnapshot`?
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java
##########
@@ -231,9 +229,14 @@
@Inject
VolumeApiService _volumeApiService;
+ @Inject
+ protected SnapshotHelper snapshotHelper;
+
private final StateMachine2<Volume.State, Volume.Event, Volume>
_volStateMachine;
protected List<StoragePoolAllocator> _storagePoolAllocators;
+ protected boolean backupSnapshotAfterTakingSnapshot =
BackupSnapshotAfterTakingSnapshot.value();
Review comment:
I think a null check would be good here
##########
File path:
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +271,93 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete
snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + "
Status");
}
- Boolean deletedOnSecondary = deleteOnSecondaryIfNeeded(snapshotId);
- boolean deletedOnPrimary = deleteOnPrimaryIfNeeded(snapshotId);
-
- if (deletedOnPrimary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id:
%d) on primary storage.", snapshotId));
- } else {
- s_logger.debug(String.format("The snapshot (id: %d) could not be
found/deleted on primary storage.", snapshotId));
- }
- if (null != deletedOnSecondary && deletedOnSecondary) {
- s_logger.debug(String.format("Successfully deleted snapshot (id:
%d) on secondary storage.", snapshotId));
- }
- return (deletedOnSecondary != null) && deletedOnSecondary ||
deletedOnPrimary;
+ return destroySnapshotEntriesAndFiles(snapshotVO);
}
- private boolean deleteOnPrimaryIfNeeded(Long snapshotId) {
- SnapshotVO snapshotVO;
- boolean deletedOnPrimary = false;
- snapshotVO = snapshotDao.findById(snapshotId);
- SnapshotInfo snapshotOnPrimaryInfo =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotVO != null && snapshotVO.getState() ==
Snapshot.State.Destroyed) {
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
- } else {
- // Here we handle snapshots which are to be deleted but are not
marked as destroyed yet.
- // This may occur for instance when they are created only on
primary because
- // snapshot.backup.to.secondary was set to false.
- if (snapshotOnPrimaryInfo == null) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot (id: %d) not found
on primary storage, skipping snapshot deletion on primary and marking it
destroyed", snapshotId));
- }
- snapshotVO.setState(Snapshot.State.Destroyed);
- snapshotDao.update(snapshotId, snapshotVO);
- deletedOnPrimary = true;
- } else {
- SnapshotObject obj = (SnapshotObject) snapshotOnPrimaryInfo;
- try {
- obj.processEvent(Snapshot.Event.DestroyRequested);
- deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId,
snapshotOnPrimaryInfo);
- if (!deletedOnPrimary) {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } else {
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ",
e);
- deletedOnPrimary = false;
- }
- }
+ /**
+ * Destroys the snapshot entries and files on both primary and secondary
storage (if it exists).
+ * @return true if destroy successfully, else false.
+ */
+ protected boolean destroySnapshotEntriesAndFiles(SnapshotVO snapshotVo) {
+ if (!deleteSnapshotInfos(snapshotVo)) {
+ return false;
}
- return deletedOnPrimary;
- }
- private Boolean deleteOnSecondaryIfNeeded(Long snapshotId) {
- Boolean deletedOnSecondary = null;
- SnapshotInfo snapshotOnImage =
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
- if (snapshotOnImage == null) {
- s_logger.debug(String.format("Can't find snapshot [snapshot id:
%d] on secondary 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));
- }
- } catch (NoTransitionException e) {
- s_logger.debug("Failed to set the state to destroying: ", e);
-// deletedOnSecondary remain null
- }
- }
- return deletedOnSecondary;
+ updateSnapshotToDestroyed(snapshotVo);
+
+ return true;
}
/**
- * 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.
+ * Updates the snapshot to {@link Snapshot.State#Destroyed}.
*/
- 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);
+ protected void updateSnapshotToDestroyed(SnapshotVO snapshotVo) {
+ snapshotVo.setState(Snapshot.State.Destroyed);
+ snapshotDao.update(snapshotVo.getId(), snapshotVo);
+ }
- if (volumesFromSnapshot.size() > 0) {
- try {
- obj.processEvent(Snapshot.Event.OperationFailed);
- } catch (NoTransitionException e1) {
- s_logger.debug("Failed to change snapshot state: " +
e1.toString());
+ protected boolean deleteSnapshotInfos(SnapshotVO snapshotVo) {
+ Map<String, SnapshotInfo> snapshotInfos =
retrieveSnapshotEntries(snapshotVo.getId());
+
+ for (var infoEntry : snapshotInfos.entrySet()) {
+ if (!deleteSnapshotInfo(infoEntry.getValue(), infoEntry.getKey(),
snapshotVo)) {
+ return false;
}
- throw new InvalidParameterValueException("Unable to perform delete
operation, Snapshot with id: " + snapshotId + " is in use ");
}
- boolean result = deleteSnapshotChain(snapshotOnImage);
- obj.processEvent(Snapshot.Event.OperationSucceeded);
- return result;
+ return true;
}
/**
- * Deletes the snapshot on primary storage. It returns true when the
snapshot was not found on primary storage; </br>
- * In case of failure while deleting the snapshot, it will throw one of
the following exceptions: CloudRuntimeException, InterruptedException, or
ExecutionException. </br>
+ * Destroys the snapshot entry and file.
+ * @return true if destroy successfully, else false.
*/
- private boolean deleteSnapshotOnPrimary(Long snapshotId, SnapshotInfo
snapshotOnPrimaryInfo) {
- SnapshotDataStoreVO snapshotOnPrimary =
snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (isSnapshotOnPrimaryStorage(snapshotId)) {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is found on
primary storage for snapshot id: %d, performing snapshot deletion on primary",
snapshotId));
- }
- if (snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
- snapshotOnPrimary.setState(State.Destroyed);
- snapshotStoreDao.update(snapshotOnPrimary.getId(),
snapshotOnPrimary);
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Successfully deleted
snapshot id: %d on primary storage", snapshotId));
- }
+ 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;
+ }
+
+ DataStore dataStore = snapshotInfo.getDataStore();
+ storage = 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)) {
+ snapshotObject.processEvent(Snapshot.Event.OperationSucceeded);
+ s_logger.debug(String.format("%s was deleted on %s.",
snapshotVo, storage));
return true;
}
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(String.format("Snapshot reference is not found
on primary storage for snapshot id: %d, skipping snapshot deletion on primary",
snapshotId));
- }
- return true;
+
+ 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);
}
+
return false;
}
/**
- * Returns true if the snapshot volume is on primary storage.
+ * Cast SnapshotInfo to SnapshotObject.
+ * @return SnapshotInfo cast to SnapshotObject.
*/
- private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
- SnapshotDataStoreVO snapshotOnPrimary =
snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
- if (snapshotOnPrimary != null) {
- long volumeId = snapshotOnPrimary.getVolumeId();
- VolumeVO volumeVO = volumeDao.findById(volumeId);
- return volumeVO != null && volumeVO.getRemoved() == null;
- }
- return false;
+ protected SnapshotObject castSnapshotInfoToSnapshotObject(SnapshotInfo
snapshotInfo) {
+ return (SnapshotObject) snapshotInfo;
+ }
+
+ /**
+ * Retrieves the snapshot infos on primary and secondary storage.
+ * @param snapshotId The snapshot to retrieve the infos.
+ * @return A map of snapshot infos.
+ */
+ protected Map<String, SnapshotInfo> retrieveSnapshotEntries(long
snapshotId) {
+ Map<String, SnapshotInfo> snapshotInfos = new LinkedHashMap<>();
+ snapshotInfos.put("secondary storage",
snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image, false));
Review comment:
Can we make them constant values?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
