Maor Lipchuk has uploaded a new change for review. Change subject: engine: Add action type for snapshot. ......................................................................
engine: Add action type for snapshot. The following patch changes the API between the client and the engine when preview/commit or undo a snapshot. The patch introduces the following changes: * Adding an enum which distinguishes between several operations on a snapshot: Undo Commit Restore From Stateless status * Use of snapshot instead snapshotId * Changing the client to call to the engine with the operation on snapshot * Removing snapshotId from the constructor of the parameters, since the snapshot is only on a VM, and there is no reason to transfer it, but better that the engine will fetch it, it self. The reason for this change, is for REST operations to be implemented in the following patches Change-Id:If877befc5058c3412ae3d3731d5beacbc09e5c97 Related to Bug-Url: https://bugzilla.redhat.com/867339 Signed-off-by: Maor Lipchuk <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java M backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java A backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SnapshotActionEnum.java M backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java M backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java M frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml M frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java 9 files changed, 99 insertions(+), 72 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/90/25690/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java index 5aaaf84..67239f1 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java @@ -58,7 +58,7 @@ public class RestoreAllSnapshotsCommand<T extends RestoreAllSnapshotsParameters> extends VmCommand<T> implements QuotaStorageDependent { private final Set<Guid> snapshotsToRemove = new HashSet<Guid>(); - private Snapshot targetSnapshot; + private Snapshot snapshot; List<DiskImage> imagesToRestore = new ArrayList<>(); /** @@ -90,21 +90,13 @@ } } - // The snapshot being restored to - Snapshot targetSnapshot = getSnapshotDao().get(getSnapshotId()); - - if (targetSnapshot == null) { - throw new VdcBLLException(VdcBllErrors.ENGINE, "Can't find target snapshot by id: " - + getSnapshotId()); - } - - restoreSnapshotAndRemoveObsoleteSnapshots(targetSnapshot); + restoreSnapshotAndRemoveObsoleteSnapshots(getSnapshot()); boolean succeeded = true; for (DiskImage image : imagesToRestore) { if (image.getImageStatus() != ImageStatus.ILLEGAL) { ImagesContainterParametersBase params = new RestoreFromSnapshotParameters(image.getImageId(), - getVmId(), targetSnapshot, removedSnapshotId); + getVmId(), getSnapshot(), removedSnapshotId); VdcReturnValueBase returnValue = runAsyncTask(VdcActionType.RestoreFromSnapshot, params); // Save the first fault if (succeeded && !returnValue.getSucceeded()) { @@ -121,15 +113,36 @@ deleteOrphanedImages(); } else { getVmStaticDAO().incrementDbGeneration(getVm().getId()); - getSnapshotDao().updateStatus(getSnapshotId(), SnapshotStatus.OK); + getSnapshotDao().updateStatus(getSnapshot().getId(), SnapshotStatus.OK); unlockVm(); } setSucceeded(succeeded); } - private Guid getSnapshotId() { - return getParameters().getDstSnapshotId(); + private Snapshot getSnapshot() { + if (snapshot == null) { + switch (getParameters().getSnapshotAction()) { + case UNDO: + snapshot = getSnapshotDao().get(getVmId(), SnapshotType.PREVIEW); + break; + case COMMIT: + snapshot = getSnapshotDao().get(getVmId(), SnapshotType.REGULAR, SnapshotStatus.IN_PREVIEW); + break; + case RESTORE_STATELESS: + snapshot = getSnapshotDao().get(getVmId(), SnapshotType.STATELESS); + break; + default: + log.errorFormat("The Snapshot Action {0} is not valid", getParameters().getSnapshotAction()); + } + + // We initialize the snapshotId in the parameters so we can use it in the endVmCommand + // to unlock the snapshot, after the task that creates the snapshot finishes. + if (snapshot != null) { + getParameters().setSnapshotId(snapshot.getId()); + } + } + return snapshot; } protected void removeSnapshotsFromDB() { @@ -257,18 +270,15 @@ restoreConfiguration(targetSnapshot); break; - // Currently UI sends the "in preview" snapshot to restore when "Commit" is pressed. case REGULAR: - if (SnapshotStatus.IN_PREVIEW == targetSnapshot.getStatus()) { - prepareToDeletePreviewBranch(); + prepareToDeletePreviewBranch(); - // Set the active snapshot's images as target images for restore, because they are what we keep. - getParameters().setImages(imagesFromActiveSnapshot); - imagesToRestore = ImagesHandler.imagesIntersection(imagesFromActiveSnapshot, imagesFromPreviewSnapshot); - updateSnapshotIdForSkipRestoreImages( - ImagesHandler.imagesSubtract(imagesFromActiveSnapshot, imagesToRestore), activeSnapshotId); - break; - } + // Set the active snapshot's images as target images for restore, because they are what we keep. + getParameters().setImages(imagesFromActiveSnapshot); + imagesToRestore = ImagesHandler.imagesIntersection(imagesFromActiveSnapshot, imagesFromPreviewSnapshot); + updateSnapshotIdForSkipRestoreImages( + ImagesHandler.imagesSubtract(imagesFromActiveSnapshot, imagesToRestore), activeSnapshotId); + break; default: throw new VdcBLLException(VdcBllErrors.ENGINE, "No support for restoring to snapshot type: " + targetSnapshot.getType()); @@ -344,8 +354,8 @@ } private List<DiskImage> getImagesList() { - if (getParameters().getImages() == null && !getSnapshotId().equals(Guid.Empty)) { - getParameters().setImages(getDiskImageDao().getAllSnapshotsForVmSnapshot(getSnapshotId())); + if (getParameters().getImages() == null && !getSnapshot().getId().equals(Guid.Empty)) { + getParameters().setImages(getDiskImageDao().getAllSnapshotsForVmSnapshot(getSnapshot().getId())); } return getParameters().getImages(); } @@ -365,8 +375,7 @@ public Map<String, String> getJobMessageProperties() { if (jobProperties == null) { jobProperties = super.getJobMessageProperties(); - Snapshot snapshot = getSnapshotDao().get(getSnapshotId()); - if (snapshot != null) { + if (getSnapshot() != null) { jobProperties.put(VdcObjectType.Snapshot.name().toLowerCase(), snapshot.getDescription()); } } @@ -383,18 +392,16 @@ return false; } - if (Guid.Empty.equals(getSnapshotId())) { - return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID); - } - SnapshotsValidator snapshotValidator = createSnapshotValidator(); - VmValidator vmValidator = createVmValidator(getVm()); - if (!validate(snapshotValidator.snapshotExists(getVmId(), getSnapshotId())) || + if (!validate(snapshotValidator.snapshotExists(getSnapshot())) + || !validate(snapshotValidator.snapshotExists(getVmId(), getSnapshot().getId())) || !validate(new StoragePoolValidator(getStoragePool()).isUp())) { return false; } - - targetSnapshot = getSnapshotDao().get(getParameters().getDstSnapshotId()); + if (Guid.Empty.equals(getSnapshot().getId())) { + return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID); + } + VmValidator vmValidator = createVmValidator(getVm()); MultipleStorageDomainsValidator storageValidator = createStorageDomainValidator(); if (!validate(storageValidator.allDomainsExistAndActive()) || @@ -402,13 +409,12 @@ !performImagesChecks() || !validate(vmValidator.vmDown()) || // if the user choose to commit a snapshot - the vm cant have disk snapshots attached to other vms. - (targetSnapshot.getType() == SnapshotType.REGULAR && !validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms(false)))) { + (getSnapshot()).getType() == SnapshotType.REGULAR && !validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms(false))) { return false; } - Snapshot snapshot = getSnapshotDao().get(getSnapshotId()); - if (snapshot.getType() == SnapshotType.REGULAR - && snapshot.getStatus() != SnapshotStatus.IN_PREVIEW) { + if (getSnapshot().getType() == SnapshotType.REGULAR + && getSnapshot().getStatus() != SnapshotStatus.IN_PREVIEW) { return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_SNAPSHOT_NOT_IN_PREVIEW); } @@ -478,7 +484,7 @@ @Override protected void endVmCommand() { // if we got here, the target snapshot exists for sure - getSnapshotDao().updateStatus(getSnapshotId(), SnapshotStatus.OK); + getSnapshotDao().updateStatus(getParameters().getSnapshotId(), SnapshotStatus.OK); super.endVmCommand(); } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java index 5ba6e92..a3527e9 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreStatelessVmCommand.java @@ -9,6 +9,7 @@ import org.ovirt.engine.core.common.action.VdcReturnValueBase; import org.ovirt.engine.core.common.action.VmOperationParameterBase; import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType; import org.ovirt.engine.core.compat.Guid; import org.ovirt.engine.core.dal.dbbroker.DbFacade; @@ -55,12 +56,12 @@ /** * restore all snapshots */ - RestoreAllSnapshotsParameters tempVar = new RestoreAllSnapshotsParameters(getVm().getId(), snapshotId); - tempVar.setShouldBeLogged(false); - tempVar.setImages(imagesList); + RestoreAllSnapshotsParameters restoreParameters = new RestoreAllSnapshotsParameters(getVm().getId(), SnapshotActionEnum.RESTORE_STATELESS); + restoreParameters.setShouldBeLogged(false); + restoreParameters.setImages(imagesList); VdcReturnValueBase vdcReturn = Backend.getInstance().runInternalAction(VdcActionType.RestoreAllSnapshots, - tempVar, + restoreParameters, ExecutionHandler.createDefaultContexForTasks(getExecutionContext(), getLock())); returnVal = vdcReturn.getSucceeded(); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java index 2391712..98f80ea 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RestoreAllSnapshotCommandTest.java @@ -26,6 +26,7 @@ import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.Snapshot; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType; import org.ovirt.engine.core.common.businessentities.StorageDomain; @@ -120,6 +121,8 @@ @Test public void canDoActionFailsOnSnapshotTypeRegularNotInPreview() { mockSnapshotExists(); + mockSnapshot = new Snapshot(); + when(snapshotDao.exists(any(Guid.class), any(Guid.class))).thenReturn(true); mockSnapshotFromDb(); mockSnapshot.setType(SnapshotType.REGULAR); mockSnapshot.setStatus(SnapshotStatus.OK); @@ -145,11 +148,12 @@ private void mockSnapshotFromDb() { mockSnapshot = new Snapshot(); mockSnapshot.setType(SnapshotType.STATELESS); - when(snapshotDao.get(any(Guid.class))).thenReturn(mockSnapshot); + when(snapshotDao.get(any(Guid.class), any(SnapshotType.class), any(SnapshotStatus.class))).thenReturn(mockSnapshot); + when(snapshotDao.get(any(Guid.class), any(SnapshotType.class))).thenReturn(mockSnapshot); } private void initSpyCommand() { - RestoreAllSnapshotsParameters parameters = new RestoreAllSnapshotsParameters(vmId, dstSnapshotId); + RestoreAllSnapshotsParameters parameters = new RestoreAllSnapshotsParameters(vmId, SnapshotActionEnum.COMMIT); List<DiskImage> diskImageList = createDiskImageList(); parameters.setImages(diskImageList); doReturn(ValidationResult.VALID).when(storageValidator).allDomainsExistAndActive(); diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java index 04868b0..2b0b4e9 100644 --- a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RestoreAllSnapshotsParameters.java @@ -3,20 +3,22 @@ import java.util.List; import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.compat.Guid; -public class RestoreAllSnapshotsParameters extends TryBackToAllSnapshotsOfVmParameters implements java.io.Serializable { +public class RestoreAllSnapshotsParameters extends VmOperationParameterBase implements java.io.Serializable { private static final long serialVersionUID = -8756081739745132849L; private List<DiskImage> images; + private SnapshotActionEnum snapshotAction; - public RestoreAllSnapshotsParameters(Guid vmId, Guid dstSnapshotId) { - super(vmId, dstSnapshotId); - } + // We need to keep the snapshot id in the parameters for the tasks, + // so we can unlock it when the restore finishes. + private Guid snapshotId; - public RestoreAllSnapshotsParameters(Guid vmId, Guid dstSnapshotId, List<DiskImage> images) { - this(vmId, dstSnapshotId); - this.images = images; + public RestoreAllSnapshotsParameters(Guid vmId, SnapshotActionEnum snapshotAction) { + super(vmId); + this.snapshotAction = snapshotAction; } public List<DiskImage> getImages() { @@ -29,4 +31,20 @@ public RestoreAllSnapshotsParameters() { } + + public SnapshotActionEnum getSnapshotAction() { + return snapshotAction; + } + + public void setSnapshotAction(SnapshotActionEnum snapshotAction) { + this.snapshotAction = snapshotAction; + } + + public Guid getSnapshotId() { + return snapshotId; + } + + public void setSnapshotId(Guid snapshotId) { + this.snapshotId = snapshotId; + } } diff --git a/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SnapshotActionEnum.java b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SnapshotActionEnum.java new file mode 100644 index 0000000..583b889 --- /dev/null +++ b/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SnapshotActionEnum.java @@ -0,0 +1,7 @@ +package org.ovirt.engine.core.common.businessentities; + +public enum SnapshotActionEnum { + UNDO, + COMMIT, + RESTORE_STATELESS; +} diff --git a/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java b/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java index 391bcf4..66662ad 100644 --- a/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java +++ b/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResource.java @@ -14,6 +14,7 @@ import org.ovirt.engine.api.resource.SnapshotResource; import org.ovirt.engine.core.common.action.RestoreAllSnapshotsParameters; import org.ovirt.engine.core.common.action.TryBackToAllSnapshotsOfVmParameters; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.common.action.VdcActionType; import org.ovirt.engine.core.compat.Guid; @@ -62,7 +63,7 @@ action, PollingType.JOB); if (response.getStatus()==Response.Status.OK.getStatusCode()) { - RestoreAllSnapshotsParameters restoreParams = new RestoreAllSnapshotsParameters(parentId, guid); + RestoreAllSnapshotsParameters restoreParams = new RestoreAllSnapshotsParameters(parentId, SnapshotActionEnum.COMMIT); restoreParams.setCorrelationId(RESTORE_SNAPSHOT_CORRELATION_ID); Response response2 = doAction(VdcActionType.RestoreAllSnapshots, restoreParams, diff --git a/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java b/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java index c1a10e3..876e108 100644 --- a/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java +++ b/backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java @@ -15,6 +15,7 @@ import org.ovirt.engine.core.common.action.RestoreAllSnapshotsParameters; import org.ovirt.engine.core.common.action.TryBackToAllSnapshotsOfVmParameters; import org.ovirt.engine.core.common.action.VdcActionType; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.common.job.JobExecutionStatus; import org.ovirt.engine.core.common.queries.IdQueryParameters; import org.ovirt.engine.core.common.queries.VdcQueryType; @@ -135,8 +136,8 @@ return setUpActionExpectations( VdcActionType.RestoreAllSnapshots, RestoreAllSnapshotsParameters.class, - new String[] { "VmId", "DstSnapshotId" }, - new Object[] { VM_ID, SNAPSHOT_ID }, + new String[] { "VmId", "SnapshotAction" }, + new Object[] { VM_ID, SnapshotActionEnum.COMMIT }, true, true, null, diff --git a/frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml b/frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml index fcac20c..76a5c4d 100644 --- a/frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml +++ b/frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml @@ -175,6 +175,7 @@ <include name="common/businessentities/VmWatchdogType.java" /> <include name="common/businessentities/InstanceType.java" /> <include name="common/businessentities/ImageType.java" /> + <include name="common/businessentities/SnapshotActionEnum.java"/> <include name="common/job/*.java" /> <!-- Quota --> diff --git a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java index 58a4541..5df0001 100644 --- a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java +++ b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java @@ -14,6 +14,7 @@ import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.DisplayType; import org.ovirt.engine.core.common.businessentities.Snapshot; +import org.ovirt.engine.core.common.businessentities.SnapshotActionEnum; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus; import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotType; import org.ovirt.engine.core.common.businessentities.StoragePool; @@ -381,10 +382,8 @@ VM vm = (VM) getEntity(); if (vm != null) { - Snapshot snapshot = getPreview(); - Frontend.getInstance().runAction(VdcActionType.RestoreAllSnapshots, - new RestoreAllSnapshotsParameters(vm.getId(), snapshot.getId()), + new RestoreAllSnapshotsParameters(vm.getId(), SnapshotActionEnum.UNDO), null, null); } @@ -395,10 +394,8 @@ VM vm = (VM) getEntity(); if (vm != null) { - Snapshot snapshot = getInPreview(); - Frontend.getInstance().runAction(VdcActionType.RestoreAllSnapshots, - new RestoreAllSnapshotsParameters(vm.getId(), snapshot.getId()), + new RestoreAllSnapshotsParameters(vm.getId(), SnapshotActionEnum.COMMIT), null, null); } @@ -749,15 +746,6 @@ public Snapshot getInPreview() { for (Snapshot snapshot : (ArrayList<Snapshot>) getItems()) { if (snapshot.getStatus() == SnapshotStatus.IN_PREVIEW) { - return snapshot; - } - } - return null; - } - - public Snapshot getPreview() { - for (Snapshot snapshot : (ArrayList<Snapshot>) getItems()) { - if (snapshot.getType() == SnapshotType.PREVIEW) { return snapshot; } } -- To view, visit http://gerrit.ovirt.org/25690 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If877befc5058c3412ae3d3731d5beacbc09e5c97 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: ovirt-engine-3.4 Gerrit-Owner: Maor Lipchuk <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
