Liron Ar has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 17: Code-Review+1
(8 comments)
mostly minor issues that needs to be fixed, looks good other than that
....................................................
Commit Message
Line 9: All underlying components are already supporting RO disks
Line 10: functionlity. After that change Engine will support it either through
Line 11: the REST API.
Line 12:
Line 13: When adding/attaching a disk to a VM, this feature adds a read-only
please add also "editing" to the commit message.
Line 14: property to the VM-Disk relationship which is persisted through
Line 15: VMDevice (vm_device in the DB). Floating disks continue to be RW.
Line 16:
Line 17: Note that shareable disk could be attached to one VM as RO, and to
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 240: }
Line 241:
Line 242: private boolean validateCanUpdateReadOnly() {
Line 243: if (shouldUpdateReadOnly() && getVm().getStatus() !=
VMStatus.Down) {
Line 244: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN);
I think that more informative message is required here (can be done in other
patch)
Line 245: }
Line 246: return true;
Line 247: }
Line 248:
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java
Line 274: final UpdateVmDiskParameters parameters = createParameters();
Line 275: parameters.getDiskInfo().setReadOnly(true);
Line 276:
Line 277: // creating old disk with interface different than interface
of disk from parameters
Line 278: // have to return original disk on each request to dao,
the comment is wrong (interface isn't related)..
Line 279: // since the command updates retrieved instance of disk
Line 280: when(diskDao.get(diskImageGuid)).thenAnswer(new Answer() {
Line 281: @Override
Line 282: public Object answer(InvocationOnMock invocationOnMock)
throws Throwable {
Line 281: @Override
Line 282: public Object answer(InvocationOnMock invocationOnMock)
throws Throwable {
Line 283: final DiskImage oldDisk = createDiskImage();
Line 284: oldDisk.setReadOnly(false);
Line 285:
assert(!oldDisk.getReadOnly().equals(parameters.getDiskInfo().getReadOnly()));
this block isn't necessary,
in the command you compare the new disk with the vm device
the old disk isn't relevant so that answer mock could be removed.
Line 286: return oldDisk;
Line 287: }
Line 288: });
Line 289:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AttachDettachVmDiskParameters.java
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6:
Line 7: public class AttachDettachVmDiskParameters extends
VmDiskOperationParameterBase {
Line 8:
Line 9: private static final long serialVersionUID = 5640716432695539552L;
needs to be regenerated..
Line 10: private boolean isPlugUnPlug;
Line 11: private boolean isReadOnly;
Line 12:
Line 13: public AttachDettachVmDiskParameters() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java
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 still think that this should change, any reason that you are against it?
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/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 559: description: add a new direct lun disk to the virtual
machine, this operation does not require size but needs lun connection details
Line 560: - mandatoryArguments: {disk.id: 'xs:string'}
Line 561: optionalArguments:
Line 562: disk.active: xs:boolean
Line 563: disk.read_only: xs:boolean
why did you remove the brackets?
additionally, the parameters should be on the same line..that's the convention
in that file
Line 564: description: attach a disk to the virtual machine
Line 565: - mandatoryArguments: {disk.id: 'xs:string', disk.snapshot.id:
'xs:string'}
Line 566: optionalArguments: {disk.active: 'xs:boolean'}
Line 567: description: attach a disk snapshot to the virtual machine
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmDiskResourceTest.java
Line 251:
Line 252: protected Disk getUpdate() {
Line 253: Disk update = new Disk();
Line 254: update.setWipeAfterDelete(false);
Line 255: update.setReadOnly(false);
I'd suggest to add a test to verify that it's actually passed to the command,
like done in testUpdate()
Line 256: return update;
Line 257: }
Line 258:
Line 259: @Override
--
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: 17
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
