Maor Lipchuk has posted comments on this change. Change subject: Core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 2: (12 comments) looks like you did some work here :). general comments: 1) Please add tests to the DAO changes 2) Please separate the REST and the GUI to separate patches 3) Please add unit tests to the backend actions https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MigrateVmCommand.java: Line 526: public void onPowerringUp() { Line 527: // nothing to do Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { /s/vmUsesScsiReservation/isVmUsesScsiReservation Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (devices != null) { Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (devices != null) { devices can not be null, the API will return empty list in the worst case Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 536: } https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 389: ( You should use if (!isDiskImage()) Line 387: } Line 388: Line 389: if (disk.getDiskStorageType() == DiskStorageType.LUN) { Line 390: updateLunProperties((LunDisk)getNewDisk()); Line 391: } > else? It should not be an error since this command updates image and Lun disks Line 392: Line 393: reloadDisks(); Line 394: updateBootOrder(); Line 395: Line 411: void Why not doing this as part of updateDeviceProperties https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/VmDeviceUtils.java: Line 623: static why can't we use the original addManagedDevice instead adding a new API? https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/LunDisk.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/LunDisk.java: Line 51: What about hashcode and equals, should they need to be regenerated with the new boolean https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDevice.java: Line 84: isScsiReservationUsed What about equals and hashcode, should they need to be regenerated? https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java: Line 147: } Please add this to LunDiskRowMapper https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3665: <xs:element name="read_only" type="xs:boolean" minOccurs="0"/> Line 3666: <xs:element ref="quota" minOccurs="0" maxOccurs="1"/> Line 3667: <xs:element name="lun_storage" type="Storage" minOccurs="0"/> Line 3668: <xs:element name="sgio" type="xs:string" minOccurs="0"/> Line 3669: <xs:element name="uses_scsi_reservation" type="xs:boolean" minOccurs="0"/> I would separate the REST part into a new seperate patch Line 3670: <xs:element ref="snapshot" minOccurs="0" maxOccurs="1"/> Line 3671: <xs:element ref="disk_profile" minOccurs="0" maxOccurs="1"/> Line 3672: <xs:element name="logical_name" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3673: </xs:sequence> https://gerrit.ovirt.org/#/c/37664/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.java: Line 156: EntityModelCheckBoxEditor Please separate the GUI part to a separate patch https://gerrit.ovirt.org/#/c/37664/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.ui.xml: Line 91: please align this with the formatter -- To view, visit https://gerrit.ovirt.org/37664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia38c03dae04c9dbb30c882941391b1909f5af416 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ala Hino <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Candace Sheremeta <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
