You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/08/27 07:58:55 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5297: KVM disk-only based snapshot of volumes instead of taking VM's full snapshot and extracting disks
DaanHoogland commented on a change in pull request #5297:
URL: https://github.com/apache/cloudstack/pull/5297#discussion_r697225748
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
##########
@@ -42,37 +49,41 @@
import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager;
import com.cloud.resource.CommandWrapper;
import com.cloud.resource.ResourceWrapper;
+import com.cloud.storage.DataStoreRole;
import com.cloud.storage.Storage.StoragePoolType;
+import com.cloud.utils.Pair;
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.utils.script.Script;
@ResourceWrapper(handles = RevertSnapshotCommand.class)
-public final class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
+public class LibvirtRevertSnapshotCommandWrapper extends CommandWrapper<RevertSnapshotCommand, Answer, LibvirtComputingResource> {
private static final Logger s_logger = Logger.getLogger(LibvirtRevertSnapshotCommandWrapper.class);
private static final String MON_HOST = "mon_host";
private static final String KEY = "key";
private static final String CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
private static final String RADOS_CONNECTION_TIMEOUT = "30";
+ protected Set<StoragePoolType> storagePoolTypesThatSupportRevertSnapshot = new HashSet<>(Arrays.asList(StoragePoolType.RBD, StoragePoolType.Filesystem,
+ StoragePoolType.NetworkFilesystem, StoragePoolType.SharedMountPoint));
+
@Override
public Answer execute(final RevertSnapshotCommand command, final LibvirtComputingResource libvirtComputingResource) {
+ SnapshotObjectTO snapshotOnPrimaryStorage = command.getDataOnPrimaryStorage();
SnapshotObjectTO snapshot = command.getData();
VolumeObjectTO volume = snapshot.getVolume();
PrimaryDataStoreTO primaryStore = (PrimaryDataStoreTO)volume.getDataStore();
DataStoreTO snapshotImageStore = snapshot.getDataStore();
- if (!(snapshotImageStore instanceof NfsTO) && primaryStore.getPoolType() != StoragePoolType.RBD) {
+ if (!(snapshotImageStore instanceof NfsTO) && !storagePoolTypesThatSupportRevertSnapshot.contains(primaryStore.getPoolType())) {
Review comment:
can primaryStore be `null` here?
##########
File path: engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/DefaultSnapshotStrategy.java
##########
@@ -262,142 +274,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;
+ }
Review comment:
Why is this needed? It only casts.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org