Allon Mureinik has posted comments on this change.
Change subject: core: [WIP] save memory state on live snapshot with memory
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(8 inline comments)
I think there's a bug in the calculation of the needed memory - see inline.
This is the reason for my -1 review.
The rest of the inline comments are minor issues
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 130:
Line 131: @Override
Line 132: public NGuid getStorageDomainId() {
Line 133: if (cachedStorageDomainId.equals(Guid.Empty) && getVm() !=
null) {
Line 134: long sizeNeeded = getVm().getTotalMemorySizeInBytes() /
BYTES_IN_GB;
This will truncate to the closest size in GB. I'd do the division in doubles
and then round UP to a long
Line 135: StorageDomain storageDomain =
VmHandler.findStorageDomainForMemory(getVm().getStoragePoolId(), sizeNeeded);
Line 136: if (storageDomain != null) {
Line 137: cachedStorageDomainId = storageDomain.getId();
Line 138: }
Line 225: new
DeleteImageGroupVDSCommandParameters(guids[1], guids[0], guids[2],
Line 226: false, false,
getVm().getVdsGroupCompatibilityVersion().toString()));
Line 227:
Line 228: if (!vdsRetValue1.getSucceeded()) {
Line 229: // log..
WIP, right?
Line 230: }
Line 231: }
Line 232: } else {
Line 233: revertToActiveSnapshot(createdSnapshot.getId());
Line 230: }
Line 231: }
Line 232: } else {
Line 233: revertToActiveSnapshot(createdSnapshot.getId());
Line 234: // TODO: remove the memory image if exists
Just don't forget to take care of it :-)
Line 235: }
Line 236:
Line 237: incrementVmGeneration();
Line 238:
Line 477:
Line 478: return list;
Line 479: }
Line 480:
Line 481: private interface MemoryImageBuilder {
should be static
Line 482: void build();
Line 483: String getVolumeStringRepresentation();
Line 484: }
Line 485:
Line 545: storagePoolId,
Line 546: storageDomainId,
Line 547: memoryDumpImageGroupId,
Line 548:
getVm().getTotalMemorySizeInBytes(),
Line 549: VolumeType.Preallocated,
why not sparse?
Line 550: VolumeFormat.RAW,
Line 551: memoryDumpVolumeId,
Line 552: "",
Line 553:
getStoragePool().getcompatibility_version().toString()));
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HibernateVmCommand.java
Line 374: * Returns the meta data that should be allocated when saving
state of image.
Line 375: *
Line 376: * @return - Meta data size for allocation in bytes.
Line 377: */
Line 378: static long getMetaDataSizeInBytes() {
why is this even a method and not a constant?
Line 379: return (long) 10 * 1024;
Line 380: }
Line 381:
Line 382: private static Log log =
LogFactory.getLog(HibernateVmCommand.class);
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/SnapshotDaoTest.java
Line 115: }
Line 116:
Line 117: @Test
Line 118: public void getSnaphsotByTypeAndStatusForExistingEntity() throws
Exception {
Line 119: assertNotNull(dao.get(EXISTING_VM_ID, SnapshotType.REGULAR,
SnapshotStatus.OK));
Consider using assertEquals on the snapshot's ID, so that you make sure you got
the /right/ snapshot
Line 120: }
Line 121:
Line 122: @Test
Line 123: public void getSnaphsotByTypeAndStatusForNonExistingEntity()
throws Exception {
Line 120: }
Line 121:
Line 122: @Test
Line 123: public void getSnaphsotByTypeAndStatusForNonExistingEntity()
throws Exception {
Line 124: assertNull(dao.get(EXISTING_VM_ID, SnapshotType.REGULAR,
SnapshotStatus.LOCKED));
Consider using assertEquals on the snapshot's ID, so that you make sure you got
the /right/ snapshot
Line 125: }
Line 126:
Line 127: @Test
Line 128: public void getIdByTypeReturnsIdForExistingByTypeAndStatus()
throws Exception {
--
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: 9
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>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
