Maor Lipchuk has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 3: (6 comments) 1) Please also handle the attach flow 2) add the messagees which you added to appErrors.java to all the other properties files (described in my last comment) https://gerrit.ovirt.org/#/c/37664/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java: Line 169: if (getVds() != null && !validate(diskValidator.isLunDiskVisible(lun, getVds()))) { Line 170: return false; Line 171: } Line 172: Line 173: LunDisk lunDisk = ((LunDisk) getParameters().getDiskInfo()); I would add this line before line 134, so we first fetch the LunDisk and from it we will fetch the lun. Line 174: ValidationResult validationResult = diskValidator.isUsingScsiReservationValid(lunDisk); Line 175: if (!validate(validationResult)) { Line 176: return failCanDoAction(validationResult.getMessage()); Line 177: } https://gerrit.ovirt.org/#/c/37664/3/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 531: = use getVmDeviceDao() instead of getDbFacade().getVmDeviceDao() Line 533: if (device.getIsPlugged() && device.isUsingScsiReservation()) { Line 534: return true; Line 535: } Line 536: } Line 537: Please remove redundant empty line Line 538: return false; Line 539: } https://gerrit.ovirt.org/#/c/37664/3/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: error I don't really see any reason to add this else, but if you already decided to add it, it should not be in the execute phase but in the CDA phase and prompt a proper message to the user. https://gerrit.ovirt.org/#/c/37664/3/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 605: * Line 606: * @param id Line 607: * @param type Line 608: * @param device Line 609: * @param customProp device custom properties we are missing the parameter isUsingScsiReservation in the javadoc though I would prefer just changing all references for this API Line 610: * @return New created VmDevice instance Line 611: */ Line 612: public static VmDevice addManagedDevice(VmDeviceId id, Line 613: VmDeviceGeneralType type, https://gerrit.ovirt.org/#/c/37664/3/packaging/dbscripts/upgrade/03_06_0940_add_uses_scsi_reservation.sql File packaging/dbscripts/upgrade/03_06_0940_add_uses_scsi_reservation.sql: Line 1: select fn_db_add_column('vm_device', 'is_using_scsi_reservation', 'BOOLEAN DEFAULT NULL'); I assume that the default value should be false (same as is_managed, is_plugged and is_readonly) -- 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: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ala Hino <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[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: Eli Mesika <[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
