Liron Aravot has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 4: (15 comments) https://gerrit.ovirt.org/#/c/37664/4/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 378: createDiskBasedOnLun(((LunDisk) getParameters().getDiskInfo()).isUsingScsiReservation()); Line 379: } Line 380: } Line 381: Line 382: private void createDiskBasedOnLun(final boolean isUsingScsiReservation) { why do we need to get this boolean? we cast in line 383 and get the Lun, so we can get that info from the disk. Line 383: final LUNs lun = ((LunDisk) getParameters().getDiskInfo()).getLun(); Line 384: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Line 385: @Override Line 386: public Void runInTransaction() { https://gerrit.ovirt.org/#/c/37664/4/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 375: } Line 376: Line 377: if (isVmUsingScsiReservation()) { Line 378: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION); Line 379: } 1. I think that this should be a config value right now with default of true. 2. this check should go down, if the vm is non migrateable it's irrelevant to let the user know about iscsi reservation. 3. what about getParameters().isForceMigrationForNonMigratableVm(), if we force while using scsi reservation it'll still fail. Line 380: Line 381: if (vm.getMigrationSupport() == MigrationSupport.IMPLICITLY_NON_MIGRATABLE Line 382: && !getParameters().isForceMigrationForNonMigratableVm()) { Line 383: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NON_MIGRTABLE_AND_IS_NOT_FORCED_BY_USER_TO_MIGRATE); Line 526: public void onPowerringUp() { Line 527: // nothing to do Line 528: } Line 529: Line 530: private boolean isVmUsingScsiReservation() { /s/isVmUsingScsiReservation/isVmPluggedDisksUsingScsiReservation Line 531: List<VmDevice> devices = getVmDeviceDao().getVmDeviceByVmIdAndType(getVmId(), VmDeviceGeneralType.DISK); Line 532: for (VmDevice device : devices) { Line 533: if (device.getIsPlugged() && device.isUsingScsiReservation()) { Line 534: return true; https://gerrit.ovirt.org/#/c/37664/4/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 386: } else if (disk.getDiskStorageType() == DiskStorageType.LUN) { Line 387: updateLunProperties((LunDisk)getNewDisk()); Line 388: } Line 389: Line 390: clearDeviceAddressIfNeeded(); instead of moving this call to here, the better approach imo would be to fix that method (so the clearance of the address will be reflected on the object) - otherwise next time that someone will want to add something we'll be in the same situation. Line 391: Line 392: reloadDisks(); Line 393: updateBootOrder(); Line 394: Line 404: } Line 405: Line 406: private void updateLunProperties(LunDisk lunDisk) { Line 407: vmDeviceForVm.setUsingScsiReservation(lunDisk.isUsingScsiReservation()); Line 408: getVmDeviceDao().update(vmDeviceForVm); 1.why do we perform it always? see how it's determined whether update of the read only property is needed on updateDeviceProperties() 2. i'd put it for now in updateDeviceProperties Line 409: } Line 410: Line 411: private void clearDeviceAddressIfNeeded() { Line 412: if (getOldDisk().getDiskInterface() != getNewDisk().getDiskInterface()) { https://gerrit.ovirt.org/#/c/37664/4/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 587: * @param type device type Line 588: * @param device the device Line 589: * @param specParams device spec params Line 590: * @param is_plugged is device plugged-in Line 591: * @param isReadOnly is device read-only irrelevant for this change Line 592: * @param customProp device custom properties Line 593: * @return New created VmDevice instance Line 594: */ Line 595: public static VmDevice addManagedDevice(VmDeviceId id, Line 622: Map<String, Object> specParams, Line 623: boolean is_plugged, Line 624: Boolean isReadOnly, Line 625: Map<String, String> customProp, Line 626: boolean isUsingScsiReservation) { I'd prefer to just add it to the old method siganature, it's bit irritating to add new overload each time a new member is added to the class. Line 627: VmDevice managedDevice = Line 628: new VmDevice(id, Line 629: type, Line 630: device.getName(), Line 1241: Line 1242: return result; Line 1243: } Line 1244: Line 1245: private static VmDevice saveManagedDevice(VmDevice managedDevice) { exporting this logic to a method shouldn't be done on this patch Line 1246: dao.save(managedDevice); Line 1247: VmDeviceGeneralType type = managedDevice.getType(); Line 1248: // If we add Disk/Interface/CD/Floppy, we have to recalculate boot order Line 1249: if (type == VmDeviceGeneralType.DISK || type == VmDeviceGeneralType.INTERFACE) { https://gerrit.ovirt.org/#/c/37664/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/DiskValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/DiskValidator.java: Line 198: public ValidationResult isUsingScsiReservationValid(LunDisk lunDisk) { Line 199: // verify that scsi reservation is enabled only when scsi passthrough is enabled Line 200: // and sgio is enabled. Otherwise, no meaning for scsi reservation Line 201: if (lunDisk.isUsingScsiReservation() && (!lunDisk.isScsiPassthrough() || Line 202: lunDisk.getSgio() == ScsiGenericIO.FILTERED)) { please try to rewrite this condition and the comment in a clearer way. Line 203: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_SGIO_IS_FILTERED); Line 204: } Line 205: Line 206: return ValidationResult.VALID; https://gerrit.ovirt.org/#/c/37664/4/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskCommandTest.java: Line 599: when(diskLunMapDAO.getDiskIdByLunId(disk.getLun().getLUN_id())).thenReturn(null); Line 600: assertTrue("checkIfLunDiskCanBeAdded() failed for valid iscsi lun", Line 601: command.checkIfLunDiskCanBeAdded(spyDiskValidator(disk))); Line 602: } Line 603: please share code between the different methods. Line 604: @Test Line 605: public void testIscsiLunCannotBeAddedIfScsiPassthroughDisabledAndScsiReservationEnabled() { Line 606: LunDisk disk = createISCSILunDisk(); Line 607: disk.setSgio(null); https://gerrit.ovirt.org/#/c/37664/4/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 13: * The LUN details. Line 14: */ Line 15: private LUNs lun; Line 16: Line 17: private boolean usingScsiReservation; seems to me like this should move higher in the hierarchy. Yes, it is relevant only for lun disks - but we can say the same on other properties as well that are relevant for only one type of the disks, I'd consider to continue with the unified solution for now and when we'll refactor the whole chain - this property would be refactored with the rest of the properties. Line 18: Line 19: @Override Line 20: public DiskStorageType getDiskStorageType() { Line 21: return DiskStorageType.LUN; https://gerrit.ovirt.org/#/c/37664/4/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 352: } Line 353: Line 354: public boolean isUsingScsiReservation() { Line 355: return usingScsiReservation; Line 356: } please move the getter/setter to be with the rest of the getters/setters Line 357: Line 358: public void setUsingScsiReservation(boolean flag) { Line 359: this.usingScsiReservation = flag; Line 360: } https://gerrit.ovirt.org/#/c/37664/4/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAO.java: Line 75: /** Line 76: * Updates device usingScsiReservation field. Line 77: * @param vmDevice Line 78: */ Line 79: void updateUsingScsiReservation(VmDevice vmDevice); this is never used. https://gerrit.ovirt.org/#/c/37664/4/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java: Line 239: updateAllInBatch("UpdateVmDeviceBootOrder", vmDevices, getBootOrderBatchMapper()); Line 240: } Line 241: Line 242: @Override Line 243: public void updateUsingScsiReservation(VmDevice vmDevice) { this is misleading, one might think that this method updated only this field while it updates all of the fields. Line 244: MapSqlParameterSource paramsForUpdate = createFullParametersMapper(vmDevice); Line 245: getCallsHandler().executeModification("UpdateVmDevice", paramsForUpdate); Line 246: } https://gerrit.ovirt.org/#/c/37664/4/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VmDeviceDAOTest.java: Line 232: new HashMap<String, Object>(), Line 233: true, false, false, "alias", customProp, null, null); Line 234: } Line 235: Line 236: private VmDevice createVmDevice(boolean usingScsiReservation) { i'd remove this method, i don't see the benefit in having it. Line 237: VmDevice vmDevice = createVmDevice(Guid.newGuid()); Line 238: vmDevice.setUsingScsiReservation(usingScsiReservation); Line 239: return vmDevice; Line 240: } -- 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: 4 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
