Ala Hino 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? Done 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 tru 1. Config value for what? 2. Done 3. For now, I changed the code that forceMigration overrides scsiReservation 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 Done 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 fi It is either we call the clearance method twice, in updateDeviceProperties and in updateLunProperties, or once as it is defined now. Maybe I am missing your point here. 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 th 1. Done 2. updateDeviceProperties is called both for IMAGE and LUN while updateLunProperties is called only for LUN 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 What's irrelevant? 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 Basically, I'd still add a new API rather than changing existing API and change all API consumer (in this use case, 15 places). Like I said, I removed the new API and added new arg to existing method. 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 Please explain why 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. Done 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. Done 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. I thought that defining this property here, where it should be, may safe us some refactoring work, no? 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 Done 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. Done 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 fiel Removed. Using dao.update method instead 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. Done 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
