Liron Ar has posted comments on this change. Change subject: core: Add read-only disk functionality ......................................................................
Patch Set 6: Code-Review-1 (15 comments) 1. missing DAO changes? 2. marking -1 just so my comments would be seen before proceeding further. 3. please rebase on top http://gerrit.ovirt.org/#/c/17679/ related changes could be found there. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java Line 70: private AuditLogType attemptToAttachDisksToImportedVm(Collection<Disk> disks){ Line 71: List<String> failedDisks = new LinkedList<>(); Line 72: for (Disk disk : disks) { Line 73: AttachDettachVmDiskParameters params = new AttachDettachVmDiskParameters(getVm().getId(), Line 74: disk.getId(), disk.getPlugged(), disk.getReadOnly()); that won't work correctly with the current implementation..please add support for the readOnly attribute in OvfReader and OvfWriter otherwise that information would be lost when reading/writing the ovf. Line 75: VdcReturnValueBase returnVal = getBackend().runInternalAction(VdcActionType.AttachDiskToVm, params); Line 76: if (!returnVal.getSucceeded()) { Line 77: failedDisks.add(disk.getDiskAlias()); Line 78: } .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java Line 472: getOldDisk().getPropagateErrors() != getNewDisk().getPropagateErrors() || Line 473: getOldDisk().isWipeAfterDelete() != getNewDisk().isWipeAfterDelete() || Line 474: getOldDisk().isShareable() != getNewDisk().isShareable() || Line 475: getOldDisk().getSgio() != getNewDisk().getSgio() || Line 476: !ObjectUtils.objectsEqual(getOldDisk().getReadOnly(), getNewDisk().getReadOnly()) || That update of readonly won't work- 1. the "OldDisk" is being queried from DiskDao, which means that it will always have the default value in getOldDisk().getReadOnly() (as we don't load it "per vm", thus the information will be missing) therefore that method will return wrong results. 2. UpdateVmDisk currently doesn't update vm device which stores that information. please rebase on top of http://gerrit.ovirt.org/#/c/17679/ as i've added things that would be useful for the implementation here to avoid duplicate work :). Line 477: !StringUtils.equals(getOldDisk().getDiskDescription(), getNewDisk().getDiskDescription()) || Line 478: !StringUtils.equals(getOldDisk().getDiskAlias(), getNewDisk().getDiskAlias()); Line 479: } Line 480: .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQueryTest.java Line 112 Line 113 Line 114 Line 115 Line 116 this change is would cause to wrong data, all the created snapshots would be active instead of inactive. Line 86: thenReturn(Arrays.asList(pluggedDevice, unpluggedDevice)); Line 87: Line 88: // Snapshots Line 89: doReturn(new ArrayList<DiskImage>(Collections.nCopies(NUM_DISKS_OF_EACH_KIND, Line 90: createDiskImage(pluggedDisk.getId())))).when(getQuery()).getAllImageSnapshots(pluggedDisk); this change is would cause to wrong data, all the created snapshots of the disk would be active instead of inactive. Line 91: doReturn(Collections.nCopies(NUM_DISKS_OF_EACH_KIND, createDiskImage(unpluggedDisk.getId()))).when(getQuery()) Line 92: .getAllImageSnapshots(unpluggedDisk); Line 93: } Line 94: .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java Line 401: */ Line 402: private DiskImage createDiskImage() { Line 403: DiskImage disk = new DiskImage(); Line 404: disk.setId(diskImageGuid); Line 405: disk.setReadOnly(false); please see related comments in the command. Line 406: disk.setSize(100000L); Line 407: return disk; Line 408: } Line 409: .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AttachDettachVmDiskParameters.java Line 20: this.isReadOnly = isReadOnly; Line 21: } Line 22: Line 23: public AttachDettachVmDiskParameters(Guid vmId, Guid diskId) { Line 24: this(vmId, diskId, true, false); previously the default passed value of isPlugUnPlug was false, now it's changed to true. any reason for doing that in this patch? Line 25: } Line 26: Line 27: public void setPlugUnPlug(boolean isPlugUnPlug) { Line 28: this.isPlugUnPlug = isPlugUnPlug; .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Disk.java Line 72: final int prime = 31; Line 73: int result = super.hashCode(); Line 74: result = prime * result + ((plugged == null) ? 0 : plugged.hashCode()); Line 75: result = prime * result + ((readOnly == null) ? 0 : readOnly.hashCode()); Line 76: result = prime * result + ((vmEntityType == null) ? 0 : vmEntityType.hashCode()); vmEntityType is already present on line 78 Line 77: result = prime * result + ((vmNames == null) ? 0 : vmNames.hashCode()); Line 78: result = prime * result + ((vmEntityType == null) ? 0 : vmEntityType.hashCode()); Line 79: result = prime * result + numberOfVms; Line 80: return result; Line 92: return false; Line 93: } Line 94: Disk other = (Disk) obj; Line 95: return (ObjectUtils.objectsEqual(plugged, other.plugged) Line 96: && ObjectUtils.objectsEqual(readOnly, other.readOnly) General - I don't like it, when comparing the same disk loaded for different vms we will get false which is annoying (Yeah, plugged was here before :) ) Line 97: && ObjectUtils.objectsEqual(vmNames, other.vmNames) Line 98: && vmEntityType == other.vmEntityType Line 99: && numberOfVms == other.numberOfVms); Line 100: } .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java Line 188: this.isPlugged = isPlugged; Line 189: } Line 190: Line 191: public boolean getIsReadOnly() { Line 192: return isReadOnly == null ? Boolean.FALSE : isReadOnly; why is this needed? Line 193: } Line 194: Line 195: public void setIsReadOnly(boolean isReadOnly) { Line 196: this.isReadOnly = isReadOnly; .................................................... File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml Line 516: disk.propagate_errors: xs:boolean Line 517: disk.wipe_after_delete: xs:boolean Line 518: disk.sgio: xs:string Line 519: description: update the size, boot flag and other parameters of the disk attached to the virtual machine Line 520: disk.read_only: xs:boolean please move this one line up (before the description) Line 521: urlparams: {} Line 522: headers: Line 523: Content-Type: {value: application/xml|json, required: true} Line 524: Correlation-Id: {value: 'any string', required: false} Line 553: disk.sgio: xs:string Line 554: disk.storage_domains.storage_domain--COLLECTION: {storage_domain.id|name: 'xs:string'} Line 555: description: add a new direct lun disk to the virtual machine, this operation does not require size but needs lun connection details Line 556: - mandatoryArguments: {disk.id: 'xs:string'} Line 557: optionalArguments: please return the bracelets Line 558: disk.active: xs:boolean Line 559: disk.read_only: xs:boolean Line 560: description: attach a disk to the virtual machine Line 561: urlparams: {} .................................................... File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDiskResource.java Line 81: @Override Line 82: public Response deactivate(Action action) { Line 83: HotPlugDiskToVmParameters params = Line 84: new HotPlugDiskToVmParameters(((BackendVmDisksResource) collection).parentId, Line 85: guid); // deactivate doesn't care about readOnly value. I agree with Maor Line 86: return doAction(VdcActionType.HotUnPlugDiskFromVm, params, action); Line 87: } Line 88: Line 89: @Override .................................................... File backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmDiskResourceTest.java Line 133: new Object[] { PARENT_ID, GUIDS[1], Boolean.FALSE }, Line 134: true, Line 135: true)); Line 136: Disk disk = resource.update(getUpdate()); Line 137: assertNotNull(disk); General - I'd add a test for that scenario. Line 138: } Line 139: Line 140: @Test Line 141: public void testActivate() throws Exception { Line 141: public void testActivate() throws Exception { Line 142: setUriInfo(setUpActionExpectations(VdcActionType.HotPlugDiskToVm, Line 143: HotPlugDiskToVmParameters.class, Line 144: new String[] { "VmId", "DiskId" }, Line 145: new Object[] { PARENT_ID, DISK_ID }, please fix the identation Line 146: true, Line 147: true)); Line 148: Line 149: Response response = ((VmDiskResource) resource).activate(new Action()); Line 153: @Test Line 154: public void testDeactivate() throws Exception { Line 155: setUriInfo(setUpActionExpectations(VdcActionType.HotUnPlugDiskFromVm, Line 156: HotPlugDiskToVmParameters.class, Line 157: new String[] { "VmId", "DiskId" }, please fix the identation Line 158: new Object[] { PARENT_ID, DISK_ID }, Line 159: true, Line 160: true)); Line 161: -- 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: 6 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: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
