Liron Ar has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 15:
(10 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()) {
there is a bug here,
if the disk is plugged to few vms and one of them is running, we won't be able
to set it as read only for other vm which isn't running.
when setting the disk to be read only for vm, only this vm should be checked.
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();
suggestion: perhaps rename this method, now we have here two methods with
"update vm device"..
Line 314:
Line 315: setSucceeded(true);
Line 316: return null;
Line 317: }
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()));
why is it loaded again? we already have vmDeviceForVm
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);
why is it needed?
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);
why is it needed in this test?
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);
why is the interface relevant here?
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);
why is it needed?
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);
false and null shouldn't return the same, why not (isReadOnly == null) ? 0 :
isReadOnly.hashCode());
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())
same, if we have null and false it shouldn't return as equal.
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'm debating over this test method, anyway - please add the field also to the
verify() method..and let's to Michael opinion on that.
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 <vvola...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@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
