Sergey Gotliv has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 15:
(11 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 132: }
Line 133:
Line 134: if (!vmsDiskOrSnapshotPluggedTo.isEmpty()) {
Line 135: // only virtual drive size can be updated when VMs is
running
Line 136: if (isAtLeastOneVmIsNotDown(vmsDiskOrSnapshotPluggedTo)
&& shouldUpdatePropertiesOtherThanSize()) {
Done
Line 137: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN);
Line 138: }
Line 139:
Line 140: boolean isUpdatedAsBootable = !getOldDisk().isBoot() &&
getNewDisk().isBoot();
Line 309: diskImage.setImageStatus(ImageStatus.OK);
Line 310: }
Line 311: getImageDao().update(diskImage.getImage());
Line 312: }
Line 313: updateVmDisksAndDevice();
This method exists just to make stubbing easier. I will split it to 2 separate
methods.
Line 314:
Line 315: setSucceeded(true);
Line 316: return null;
Line 317: }
Line 316: return null;
Line 317: }
Line 318:
Line 319: private void updateDeviceProperties() {
Line 320: if
(!getOldDisk().getReadOnly().equals(getNewDisk().getReadOnly())) {
Its now shouldUpdateReadOnly()
Line 321: VmDevice device = getVmDeviceDao().get(new
VmDeviceId(getOldDisk().getId(), getVmId()));
Line 322: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 323: getVmDeviceDao().update(device);
Line 324: }
Line 317: }
Line 318:
Line 319: private void updateDeviceProperties() {
Line 320: if
(!getOldDisk().getReadOnly().equals(getNewDisk().getReadOnly())) {
Line 321: VmDevice device = getVmDeviceDao().get(new
VmDeviceId(getOldDisk().getId(), getVmId()));
Done
Line 322: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 323: getVmDeviceDao().update(device);
Line 324: }
Line 325:
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveImageCommandTest.java
Line 165: disk.setDiskInterface(DiskInterface.VirtIO);
Line 166: disk.setStoragePoolId(vm.getStoragePoolId());
Line 167: disk.setActive(Boolean.TRUE);
Line 168: disk.setPlugged(Boolean.TRUE);
Line 169: disk.setReadOnly(Boolean.FALSE);
OvfVmReader is populating readOnly property now. So test has to do the same
otherwise equals failes (same as isPlugged)
Line 170: disk.setVmSnapshotId(snapshotId);
Line 171: disk.setImageStatus(ImageStatus.OK);
Line 172: disk.setAppList("");
Line 173: disk.setDescription("");
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java
Line 251: @Override
Line 252: public Object answer(InvocationOnMock invocationOnMock)
throws Throwable {
Line 253: final DiskImage oldDisk = createDiskImage();
Line 254: oldDisk.setDiskInterface(DiskInterface.VirtIO);
Line 255: oldDisk.setReadOnly(false);
Indeed, can be removed.
Line 256: assert(oldDisk.getDiskInterface() !=
parameters.getDiskInfo().getDiskInterface());
Line 257: return oldDisk;
Line 258: }
Line 259: });
Line 284: when(diskDao.get(diskImageGuid)).thenAnswer(new Answer() {
Line 285: @Override
Line 286: public Object answer(InvocationOnMock invocationOnMock)
throws Throwable {
Line 287: final DiskImage oldDisk = createDiskImage();
Line 288: oldDisk.setDiskInterface(DiskInterface.VirtIO);
I will remove it.
Line 289: oldDisk.setReadOnly(false);
Line 290:
assert(!oldDisk.getReadOnly().equals(parameters.getDiskInfo().getReadOnly()));
Line 291: return oldDisk;
Line 292: }
Line 468: private DiskImage createDiskImage() {
Line 469: DiskImage disk = new DiskImage();
Line 470: disk.setId(diskImageGuid);
Line 471: disk.setSize(100000L);
Line 472: disk.setReadOnly(false);
I will remove it.
Line 473: return disk;
Line 474: }
Line 475:
Line 476: /**
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java
Line 239: result = prime * result + bootOrder;
Line 240: result = prime * result + ((specParams == null) ? 0 :
specParams.hashCode());
Line 241: result = prime * result + (isManaged ? 1231 : 1237);
Line 242: result = prime * result + (isPlugged ? 1231 : 1237);
Line 243: result = prime * result + (getIsReadOnly() ? 1231 : 1237);
I answered in equals.
Line 244: result = prime * result + alias.hashCode();
Line 245: result = prime * result + (customProperties == null ? 0 :
customProperties.hashCode());
Line 246: result = prime * result + (snapshotId == null ? 0 :
snapshotId.hashCode());
Line 247: return result;
Line 266: && bootOrder == other.bootOrder
Line 267: && ObjectUtils.objectsEqual(specParams,
other.specParams)
Line 268: && isManaged == other.isManaged
Line 269: && getIsPlugged().equals(other.getIsPlugged())
Line 270: && getIsReadOnly().equals(other.getIsReadOnly())
I believe that if 2 devices have same id, type, address... properties but one
of them has readOnly = false and second readOnly = null they are equals.
Line 271: && alias.equals(other.alias)
Line 272: && ObjectUtils.objectsEqual(customProperties,
other.customProperties)
Line 273: && ObjectUtils.objectsEqual(snapshotId,
other.snapshotId));
Line 274: }
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/DiskMapperTest.java
Line 79: assertEquals(entity.getSize(), 888888);
Line 80: }
Line 81:
Line 82: @Test
Line 83: public void testReadOnlyMapping() {
I will add it as a part of "verify" either.
I don't see any harm in keeping this test either, mostly because it validates
all possible values of read-only. It seems that verify validates only 1
particular value.
We can remove it later if needed.
Line 84: Disk model = new Disk();
Line 85: model.setReadOnly(true);
Line 86:
Line 87: org.ovirt.engine.core.common.businessentities.Disk entity =
DiskMapper.map(model, null);
--
To view, visit http://gerrit.ovirt.org/15409
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I124176e8feb91b601a71e76dd63051648ec4353a
Gerrit-PatchSet: 15
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
