Sergey Gotliv has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 14:
(19 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(),
I am fixing it here because its quick.
addManagedDevice probably should receive Boolean because for some managed
devices readOnly is not applicable.
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 same :-)
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);
Agree.
Previous patches contained few not related changes therefore they were removed.
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());
Liron/Vered Done.
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/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());
Now OvfVmReader has new line 108 :-)
Done
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 313: setSucceeded(true);
Line 314: return null;
Line 315: }
Line 316:
Line 317: private void updateDeviceProperties() {
My previous comment is not relevant. I have to use clearVmDeviceAddress for
now, because it updates all devices not only in scope of the given VM.
Line 318: VmDevice device = getVmDeviceDao().get(new
VmDeviceId(getOldDisk().getId(), getVmId()));
Line 319: if
(!ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly())) {
Line 320: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 321: }
Line 321: }
Line 322: if (getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface()) {
Line 323: device.setAddress(StringUtils.EMPTY);
Line 324: }
Line 325: getVmDeviceDao().update(device);
Done
Line 326: }
Line 327: });
Line 328: }
Line 329:
Line 504: }
Line 505:
Line 506: private boolean shouldUpdateDeviceProperties() {
Line 507: return !ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly()) ||
Line 508: getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface();
Done
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.
1. Keeping duplicate data is bad, but keeping duplicate data which is out of
sync is even worse. Read only on disk and device should be consistent.
2. Confusing? Comparing between: vmDeviceForVm.getIsReadOnly() and
getNewDisk().getReadOnly is even more confusing. Again let's compare disk
properties with disk properties and device with device.
3. We already initialize members in CDA which is not exactly the place to do
that so I will move this one to CDA.
Line 581:
getOldDisk().setReadOnly(vmDeviceForVm.getIsReadOnly());
Line 582: }
Line 583: }
Line 584: }
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java
Line 64: import org.ovirt.engine.core.utils.MockEJBStrategyRule;
Line 65: import org.ovirt.engine.core.utils.ejb.ContainerManagedResourceType;
Line 66:
Line 67:
Line 68: @RunWith(MockitoJUnitRunner.class)
Done
Line 69: public class UpdateVmDiskCommandTest {
Line 70:
Line 71: private Guid diskImageGuid = Guid.newGuid();
Line 72: private Guid vmId = Guid.newGuid();
Line 438: */
Line 439: private DiskImage createDiskImage() {
Line 440: DiskImage disk = new DiskImage();
Line 441: disk.setId(diskImageGuid);
Line 442: disk.setReadOnly(false);
Done
Line 443: disk.setSize(100000L);
Line 444: return disk;
Line 445: }
Line 446:
Line 450: private DiskImage createShareableDisk(VolumeFormat volumeFormat) {
Line 451: DiskImage disk = createDiskImage();
Line 452: disk.setvolumeFormat(volumeFormat);
Line 453: disk.setShareable(true);
Line 454: disk.setReadOnly(false);
Done
Line 455: return disk;
Line 456: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AttachDettachVmDiskParameters.java
Line 7: public class AttachDettachVmDiskParameters extends
VmDiskOperationParameterBase {
Line 8:
Line 9: private static final long serialVersionUID = 5640716432695539552L;
Line 10: private boolean isPlugUnPlug;
Line 11: private boolean isReadOnly;
I agree with you 100% - AttachDettach is one of our best.
However this change will cause refactoring in related command and each caller
should be changed either.
I'll do all these changes in another patch.
Line 12:
Line 13: public AttachDettachVmDiskParameters() {
Line 14: }
Line 15:
....................................................
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;
Personally, I don't understand why bussiness entity compared by properties
other than id. But I will update equals and hashcode.
Line 67:
Line 68: /**
Line 69: * The device flag indicating whether the device
Line 70: * is a device from a taken snapshot
Line 62:
Line 63: /**
Line 64: * The device read-only flag
Line 65: */
Line 66: private Boolean isReadOnly;
I heard a lot of explanations about it. Like not for all device that property
is applicable - nic, console ...
The problem that this purism costs a lot.
Line 67:
Line 68: /**
Line 69: * The device flag indicating whether the device
Line 70: * is a device from a taken snapshot
Line 195: public void setIsPlugged(Boolean isPlugged) {
Line 196: this.isPlugged = isPlugged;
Line 197: }
Line 198:
Line 199: public boolean getIsReadOnly() {
Done
Line 200: return isReadOnly == null ? Boolean.FALSE : isReadOnly;
Line 201: }
Line 202:
Line 203: public void setIsReadOnly(Boolean isReadOnly) {
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 2743: <xs:element name="wipe_after_delete" type="xs:boolean"
minOccurs="0"/>
Line 2744: <xs:element name="propagate_errors" type="xs:boolean"
minOccurs="0"/>
Line 2745: <xs:element name="statistics" type="Statistics"
minOccurs="0" maxOccurs="1"/>
Line 2746: <xs:element name="active" type="xs:boolean" minOccurs="0"/>
Line 2747: <xs:element name="read_only" type="xs:boolean"
minOccurs="0"/>
Default value of minOccurs and maxOccurs is 1.
Line 2748: <xs:element ref="quota" minOccurs="0" maxOccurs="1"/>
Line 2749: <xs:element name="lun_storage" type="Storage"
minOccurs="0"/>
Line 2750: <xs:element name="sgio" type="xs:string" minOccurs="0"/>
Line 2751: <xs:element ref="snapshot" minOccurs="0" maxOccurs="1"/>
....................................................
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));
I am removing 3 parameters constructor in UI patch.
I didn't want change UI files as a part of this patch so you still see it.
Line 79: } else {
Line 80: return remove(id);
Line 81: }
Line 82: }
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/DiskMapper.java
Line 59: engineDisk.setPlugged(disk.isActive());
Line 60: }
Line 61: if (disk.isSetReadOnly()) {
Line 62: engineDisk.setReadOnly(disk.isReadOnly());
Line 63: }
Done
Line 64: if (disk.isSetInterface()) {
Line 65: DiskInterface diskInterface =
DiskInterface.fromValue(disk.getInterface());
Line 66: if (diskInterface != null) {
Line 67: engineDisk.setDiskInterface(map(diskInterface, 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: 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
