Vered Volansky has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 14:
(8 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 355: VmDeviceGeneralType.DISK,
Line 356: VmDeviceType.DISK,
Line 357: null,
Line 358: getVm().getStatus() == VMStatus.Down,
Line 359:
getParameters().getDiskInfo().getReadOnly(),
getReadOnly() returns Boolean, and as such can potentially (and AFAIR actually
did in at least one scenario) return null, while addManagedDevice expects
boolean.
Line 360: null);
Line 361: }
Line 362: return null;
Line 363: }
Line 391: VmDeviceGeneralType.DISK,
Line 392: VmDeviceType.DISK,
Line 393: null,
Line 394: getVm().getStatus() == VMStatus.Down,
Line 395: getParameters().getDiskInfo().getReadOnly(),
Same as above
Line 396: null));
Line 397: getCompensationContext().stateChanged();
Line 398: }
Line 399: VdcReturnValueBase tmpRetValue =
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java
Line 25: List<Disk> allDisks =
Line 26: getDbFacade().getDiskDao().getAllForVm
Line 27: (getParameters().getId(), getUserID(),
getParameters().isFiltered());
Line 28: Map<Guid, VmDevice> disksVmDevices = getDisksVmDeviceMap();
Line 29: List<Disk> disks = new ArrayList<Disk>(allDisks);
disks and alldisks duality is redundant and was removed in previous patches.
Line 30: for (Disk disk : allDisks) {
Line 31:
disk.setPlugged(disksVmDevices.get(disk.getId()).getIsPlugged());
Line 32:
disk.setReadOnly(disksVmDevices.get(disk.getId()).getIsReadOnly());
Line 33: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 28: Map<Guid, VmDevice> disksVmDevices = getDisksVmDeviceMap();
Line 29: List<Disk> disks = new ArrayList<Disk>(allDisks);
Line 30: for (Disk disk : allDisks) {
Line 31:
disk.setPlugged(disksVmDevices.get(disk.getId()).getIsPlugged());
Line 32:
disk.setReadOnly(disksVmDevices.get(disk.getId()).getIsReadOnly());
I'd extract (disksVmDevices.get(disk.getId()) for clarity, especially since
it's being used twice and might be used again in the future.
Line 33: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
Line 34: DiskImage diskImage = (DiskImage) disk;
Line 35:
diskImage.getSnapshots().addAll(getAllImageSnapshots(diskImage));
Line 36: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 504: }
Line 505:
Line 506: private boolean shouldUpdateDeviceProperties() {
Line 507: return !ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly()) ||
Line 508: getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface();
There were talks about this fact being buggy and should be changed in the
future. But AFAIK at this point Liron is correct.
Line 509: }
Line 510:
Line 511: private boolean isAtLeastOneVmIsNotDown(List<VM>
vmsDiskPluggedTo) {
Line 512: for (VM vm : vmsDiskPluggedTo) {
Line 576: if (vm.getId().equals(getParameters().getVmId())) {
Line 577: vmDeviceForVm = pair.getSecond();
Line 578: // disk's 'readOnly' property is transient, more
than that disk can be read only in the scope of
Line 579: // one VM and read write in the scope of another,
therefore 'readOnly' should be updated with
Line 580: // the value from device.
I second that.
Line 581:
getOldDisk().setReadOnly(vmDeviceForVm.getIsReadOnly());
Line 582: }
Line 583: }
Line 584: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java
Line 62:
Line 63: /**
Line 64: * The device read-only flag
Line 65: */
Line 66: private Boolean isReadOnly;
This has to be Boolean in order to comply with rest, as is isPlugged.
Line 67:
Line 68: /**
Line 69: * The device flag indicating whether the device
Line 70: * is a device from a taken snapshot
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDisksResource.java
Line 74: public Response remove(String id, Action action) {
Line 75: getEntity(id); //verifies that entity exists, returns 404
otherwise.
Line 76: if (action.isSetDetach() && action.isDetach()) {
Line 77: return performAction(VdcActionType.DetachDiskFromVm,
Line 78: new AttachDettachVmDiskParameters(parentId,
Guid.createGuidFromStringDefaultEmpty(id), true, false));
You can use the previous c'tor, no need to explicitly mention it here since it
has no relevance in remove.
Line 79: } else {
Line 80: return remove(id);
Line 81: }
Line 82: }
--
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: 14
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
