Sergey Gotliv has posted comments on this change.
Change subject: core: Add read-only disk functionality
......................................................................
Patch Set 6:
(14 comments)
I didn't answer comments in Tests, because I still have to do a few changes
before dealing with tests.
The major issue is resolve device update in UpdateVmDiskCommand.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 483: }
Line 484: }
Line 485:
Line 486: private boolean isDiskReadOnly() {
Line 487: return
BooleanUtils.toBooleanDefaultIfNull(getParameters().getDiskInfo().getReadOnly(),
false);
I removed this code. Actually, I don't want care about NULL all over the code,
if its NULL do be it.
Line 488: }
Line 489:
Line 490: @Override
Line 491: protected VdcActionType getChildActionType() {
....................................................
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()) ||
1. If we duplicate disk property, I am expecting from property on Disk be the
same as property on matching VmDevice (in scope of concrete VM).
So oldDisk should contain property as it in DB and newDisk contains property
from UI. This method just check if property should be updated, it doesn't try
to update it.
2. Agree, I will rebase it after you merge your patch.
Line 477:
!StringUtils.equals(getOldDisk().getDiskDescription(),
getNewDisk().getDiskDescription()) ||
Line 478: !StringUtils.equals(getOldDisk().getDiskAlias(),
getNewDisk().getDiskAlias());
Line 479: }
Line 480:
....................................................
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);
I removed this constructor, its not in use.
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 25: private ArrayList<String> vmNames;
Line 26:
Line 27: /**
Line 28: * plugged and readOnly are of type Boolean (as opposed to
boolean) since they are optional
Line 29: * in case the disk is not in a vm context. In that case, null
will ensure they's invisible.
Done
Line 30: */
Line 31: private Boolean plugged;
Line 32: private Boolean readOnly;
Line 33:
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());
Rows 76 and 78 are identical. Done
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 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());
I know :-). Done.
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)
I actually agree with you...
Line 97: && ObjectUtils.objectsEqual(vmNames, other.vmNames)
Line 98: && vmEntityType == other.vmEntityType
Line 99: && numberOfVms == other.numberOfVms);
Line 100: }
....................................................
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
Done
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:
Why? Please, see another optionalArguments with multiple parameters, for
example line 49 or arounf your other comment.
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 removed this class from that patch.
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);
I need to think about it.
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 },
Done
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" },
Done
Line 158: new Object[] { PARENT_ID, DISK_ID },
Line 159: true,
Line 160: true));
Line 161:
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java
Line 1202:
Line 1203: private boolean getDiskReadOnly(Map device) {
Line 1204: String readOnly = StringUtils.defaultIfEmpty(
Line 1205: (String) device.get(VdsProperties.Type),
Boolean.FALSE.toString());
Line 1206: return Boolean.getBoolean(readOnly);
Done
Line 1207: }
Line 1208:
Line 1209: /**
Line 1210: * gets the device id from the structure returned by VDSM device
ids are stored in specParams map
--
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
