Arik Hadas has posted comments on this change. Change subject: core: save memory state on live snapshot with memory ......................................................................
Patch Set 17: (4 inline comments) partial responds, I'll respond to the rest of the comments later .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java Line 222: Line 223: VDSReturnValue vdsRetValue1 = runVdsCommand( Line 224: VDSCommandType.DeleteImageGroup, Line 225: new DeleteImageGroupVDSCommandParameters(guids[1], guids[0], guids[2], Line 226: false, false, getVm().getVdsGroupCompatibilityVersion().toString())); yes, that's leftover from the time I was naive and thought I wouldn't need the vm configuration image.. anyway, it's replaced in a later patch, see: http://gerrit.ovirt.org/#/c/15119 Line 227: Line 228: if (!vdsRetValue1.getSucceeded()) { Line 229: // log.. Line 230: } Line 307: Line 308: private SnapshotVDSCommandParameters buildLiveSnapshotParameters(Snapshot snapshot) { Line 309: List<Disk> pluggedDisks = getDiskDao().getAllForVm(getVm().getId(), true); Line 310: List<DiskImage> filteredPluggedDisks = ImagesHandler.filterImageDisks(pluggedDisks, false, true); Line 311: if (FeatureSupported.memorySnapshot(ClusterUtils.getCompatilibilyVersion(getVm()))) { yes, see the comment I've added in http://gerrit.ovirt.org/#/c/15072/5/vdsm/BindingXMLRPC.py which explains the protocol Line 312: return new SnapshotVDSCommandParameters(getVm().getRunOnVds().getValue(), Line 313: getVm().getId(), Line 314: filteredPluggedDisks, Line 315: snapshot.getMemoryVolume()); Line 524: VDSReturnValue retVal = Line 525: Backend Line 526: .getInstance() Line 527: .getResourceManager() Line 528: .RunVdsCommand( @Maor - I think those classes should move to separate files later (as Liron also mentioned before) and that kind of change will just couple them to the command (as Omer said), so I prefer not to change it, even though it's a small change, so that the coupling won't increase Line 529: VDSCommandType.CreateImage, Line 530: new CreateImageVDSCommandParameters(getVm().getStoragePoolId(), Line 531: getStorageDomainId() Line 532: .getValue(), Line 567: getStoragePool().getcompatibility_version().toString())); Line 568: Line 569: if (!retVal.getSucceeded()) { Line 570: throw new VdcBLLException(VdcBllErrors.VolumeCreationError, Line 571: "CreateAllSnapshotsFromVmCommand::executeVmCommand: Failed to create image for memory!"); I've been told that "In Rome act as the Romans", so I saw the error "CreateAllSnapshotsFromVmCommand::executeVmCommand: Failed to create snapshot!" elsewhere in the file and I thought that it's some kind of standard in storage commands so I used it - I don't mind to change it, I agree that the prefix is redundant. Line 572: } Line 573: Line 574: Guid guid = Line 575: createTask(retVal.getCreationInfo(), -- To view, visit http://gerrit.ovirt.org/14291 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifbb28efda1b63e506233a88399488a256e6ab1c8 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
