Maor Lipchuk has posted comments on this change. Change subject: engine: Add action type for snapshot. ......................................................................
Patch Set 14: (8 comments) http://gerrit.ovirt.org/#/c/20420/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java: Line 258 Line 259 Line 260 Line 261 Line 262 > this should remain, otherwise you'll change the behavior for restoring from There should not be a problem. stateless has a specific case for it Line 57: @LockIdNameAttribute Line 58: public class RestoreAllSnapshotsCommand<T extends RestoreAllSnapshotsParameters> extends VmCommand<T> implements QuotaStorageDependent { Line 59: Line 60: private final Set<Guid> snapshotsToRemove = new HashSet<Guid>(); Line 61: private Snapshot snapshot = null; > no need to = null done Line 62: List<DiskImage> imagesToRestore = new ArrayList<>(); Line 63: Line 64: /** Line 65: * The snapshot id which will be removed (the stateless/preview/active image). Line 124: if (snapshot == null) { Line 125: switch (getParameters().getSnapshotAction()) { Line 126: case UNDO: Line 127: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.PREVIEW); Line 128: break; > isn't that wrong? Daniel is right, this is the same as the previous behaviour Line 129: case COMMIT: Line 130: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.REGULAR, SnapshotStatus.IN_PREVIEW); Line 131: break; Line 132: case RESTORE_STATELESS: Line 132: case RESTORE_STATELESS: Line 133: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.STATELESS); Line 134: break; Line 135: default: Line 136: log.errorFormat("The Snapshot Action {0} is not valid", getParameters().getSnapshotAction()); > seems like this should be an exception, why would we contue the flow I have added a default due to a warning. there will be no benefit to throw an exception, so I only added a log Line 137: } Line 138: Line 139: // We initialize the snapshotId in the parameters so we can use it in the endVmCommand Line 140: // to unlock the snapshot, after the task that creates the snapshot finishes. Line 136: log.errorFormat("The Snapshot Action {0} is not valid", getParameters().getSnapshotAction()); Line 137: } Line 138: Line 139: // We initialize the snapshotId in the parameters so we can use it in the endVmCommand Line 140: // to unlock the snapshot, after the task that creates the snapshot finishes. > please fix the comment - what snapshot are we creating here ? we "restore" I'm not following? I added this comment so we will know why we initialize the snapshot id. What do you suggest? Line 141: if (snapshot != null) { Line 142: getParameters().setSnapshotId(snapshot.getId()); Line 143: } Line 144: } Line 269: imagesToRestore = getParameters().getImages(); Line 270: restoreConfiguration(targetSnapshot); Line 271: break; Line 272: Line 273: // Currently UI sends the "in preview" snapshot to restore when "Commit" is pressed. > the comment can be removed now done Line 274: case REGULAR: Line 275: prepareToDeletePreviewBranch(); Line 276: Line 277: // Set the active snapshot's images as target images for restore, because they are what we keep. Line 355: } Line 356: Line 357: private List<DiskImage> getImagesList() { Line 358: if (getParameters().getImages() == null && !getSnapshot().getId().equals(Guid.Empty)) { Line 359: getParameters().setImages(getDiskImageDao().getAllSnapshotsForVmSnapshot(getSnapshot().getId())); > why not use getSnapshotId() to return that? getSnapshotId will take the snapshot Id from the parameters and we want the snapshot from the fetch. I removed this function, since it is not relevant any more. Line 360: } Line 361: return getParameters().getImages(); Line 362: } Line 363: Line 399: !validate(new StoragePoolValidator(getStoragePool()).isUp())) { Line 400: return false; Line 401: } Line 402: if (Guid.Empty.equals(getSnapshot().getId())) { Line 403: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CORRUPTED_VM_SNAPSHOT_ID); > when we'll get here? we already checked for existence in line 398? no, this is not the same check. here we check if the GUID is empty, this could happen from V2V bugs, and it is necessery to keep it Line 404: } Line 405: VmValidator vmValidator = createVmValidator(getVm()); Line 406: Line 407: MultipleStorageDomainsValidator storageValidator = createStorageDomainValidator(); -- To view, visit http://gerrit.ovirt.org/20420 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If877befc5058c3412ae3d3731d5beacbc09e5c97 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michael Pasternak <mishka8...@yahoo.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
