Liron Ar has posted comments on this change. Change subject: engine: Add action type for snapshot. ......................................................................
Patch Set 14: Code-Review-1 (9 comments) partially reviewed, please see the comments as they are crucial. 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 > no because we already know that this is the state when we called getSnapsho this should remain, otherwise you'll change the behavior for restoring from stateless snapshot (both when "restoring" from stateless and preview the target snapshot type is REGULAR) 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 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? if we chose to undo, the target snapshot is the snapshot with the type "Regular" because we want to "return" to it Line 129: case COMMIT: Line 130: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.REGULAR, SnapshotStatus.IN_PREVIEW); Line 131: break; Line 132: case RESTORE_STATELESS: Line 126: case UNDO: Line 127: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.PREVIEW); Line 128: break; Line 129: case COMMIT: Line 130: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.REGULAR, SnapshotStatus.IN_PREVIEW); this part seems wrong, if we chose to commit, the target snapshot is the snapshot with the type "Preview", because we want to commit it Line 131: break; Line 132: case RESTORE_STATELESS: Line 133: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.STATELESS); Line 134: break; Line 130: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.REGULAR, SnapshotStatus.IN_PREVIEW); Line 131: break; Line 132: case RESTORE_STATELESS: Line 133: snapshot = getSnapshotDao().get(getVmId(), SnapshotType.STATELESS); Line 134: break; this part seems to be wrong, if we chose to undo, the target snapshot is the snapshot with the type "Regular" Line 135: default: Line 136: log.errorFormat("The Snapshot Action {0} is not valid", getParameters().getSnapshotAction()); Line 137: } Line 138: 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 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" to an existing snapshot. Line 141: if (snapshot != null) { Line 142: getParameters().setSnapshotId(snapshot.getId()); Line 143: } Line 144: } 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? 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? 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
