Liron Ar has posted comments on this change.
Change subject: backend, restapi: Add read-only disk functionality
......................................................................
Patch Set 14:
(14 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllDisksByVmIdQuery.java
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());
please instead of getting from the map twice, please get it to a variable first
and use it on both calls
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());
that won't work correctly, take a look @ OvfVmReader - line 107, this property
isn't being set there to the disk, so you'll never have here the correct value.
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 319: if
(!ObjectUtils.objectsEqual(getOldDisk().getReadOnly(),
getNewDisk().getReadOnly())) {
Line 320: device.setIsReadOnly(getNewDisk().getReadOnly());
Line 321: }
Line 322: if (getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface()) {
Line 323: device.setAddress(StringUtils.EMPTY);
"" is shorter..i don't see a reason to have/use a constant for empty string :)
but that's your choice
Line 324: }
Line 325: getVmDeviceDao().update(device);
Line 326: }
Line 327: });
Line 321: }
Line 322: if (getOldDisk().getDiskInterface() !=
getNewDisk().getDiskInterface()) {
Line 323: device.setAddress(StringUtils.EMPTY);
Line 324: }
Line 325: getVmDeviceDao().update(device);
there is a bug here, previously the update cleared the address on all VMS the
disk is attached to, now it will clear it only for one (the loaded) vm device.
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();
1. disk interface is not a device property, therefore it doesn't belong to that
function.
2. please read related comment down below.
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.
this method only load data from the db
i think that it's confusing to set this info to the disk within this scope,
hard to debug and to track. please move it from here and preferably use the vm
device instead of setting it to the disks.
Having those properties in the disks are only because of the lack of UI support
for vm device..there's no need to persist the use of them - IMO, just use
VmDevice.
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)
what about test to read only?
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);
why is it needed?
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);
why is it needed?
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;
this is relevant for attach only..i'd sepeare the parameters class. there's no
meaning to send it for detach and it just add confusion IMO.
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;
1. if you do change it, the equals,hashcode ,methods needs to be updated as well
2. why was it changed?
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() {
if you do change it to Boolean (please explain why), the getter return type
should be changed as well
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"/>
please add also maxOccurs="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/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: }
please add also to DiskMapperTest
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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
