Liron Ar has posted comments on this change.
Change subject: core: save memory state on live snapshot with memory
......................................................................
Patch Set 21: (4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 137: return cachedStorageDomainId;
Line 138: }
Line 139:
Line 140: private MemoryImageBuilder createMemoryImageBuilder() {
Line 141: return
FeatureSupported.memorySnapshot(getVm().getVdsGroupCompatibilityVersion())
isn't this operation done by the spm?
how can you guarantee that the spm host is in the vds group of the vm and that
the operation won't fail because it's undefined in vdsm side? I see the fact
that we rely on the vds group version as error prone in that case..
I think that we should use here the pool compatibility version
Line 142: && getParameters().isSaveMemory() &&
isLiveSnapshotApplicable() ?
Line 143: new DefaultMemoryImageBuilder(getVm(),
getStorageDomainIdForVmMemory(), getStoragePool(), this)
Line 144: : new NullableMemoryImageBuilder();
Line 145: }
Line 290:
Line 291: private SnapshotVDSCommandParameters
buildLiveSnapshotParameters(Snapshot snapshot) {
Line 292: List<Disk> pluggedDisks =
getDiskDao().getAllForVm(getVm().getId(), true);
Line 293: List<DiskImage> filteredPluggedDisks =
ImagesHandler.filterImageDisks(pluggedDisks, false, true);
Line 294: if
(FeatureSupported.memorySnapshot(getVm().getVdsGroupCompatibilityVersion())) {
same comment as before..
Line 295: return new
SnapshotVDSCommandParameters(getVm().getRunOnVds().getValue(),
Line 296: getVm().getId(),
Line 297: filteredPluggedDisks,
Line 298: snapshot.getMemoryVolume());
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/DefaultMemoryImageBuilder.java
Line 32:
Line 33: public DefaultMemoryImageBuilder(VM vm, Guid storageDomainId,
Line 34: StoragePool storagePool, TaskHandlerCommand<?>
enclosingCommand) {
Line 35: this.vm = vm;
Line 36: this.enclosingCommand = enclosingCommand;
I'd prefer that you'd remove the enclosing command reference from here.
Line 37: this.storageDomainId = storageDomainId;
Line 38: this.storagePoolId = storagePool.getId();
Line 39: this.memoryDumpImageGroupId = Guid.NewGuid();
Line 40: this.memoryDumpVolumeId = Guid.NewGuid();
Line 71: }
Line 72:
Line 73: Guid guid =
enclosingCommand.createTask(retVal.getCreationInfo(),
Line 74: VdcActionType.CreateAllSnapshotsFromVm);
Line 75: enclosingCommand.getTaskIdList().add(guid);
i'm in favour of just returning the task, it's the caller responsibility to
decide on what to do with it..the memory builder should be aware that it was
called by command and not to have it's instance/be able to change it.
Line 76: }
Line 77:
Line 78: private void createImageForMemoryDump() {
Line 79: VDSReturnValue retVal =
--
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: 21
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
