Ala Hino has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 1: (19 comments) https://gerrit.ovirt.org/#/c/37664/1//COMMIT_MSG Commit Message: Line 6: Line 7: Core: Prevent VM migration when SCSI reservation is used Line 8: Line 9: Added an option to enable admin mark whether SCSI reservation is used when creating Direct Lun. Line 10: When migrating a VM, if SCSI reservation is marked, operation fails. > Please keep lines shorter then 80 characters in commit message. It is hard Done Line 11: Line 12: Change-Id: Ia38c03dae04c9dbb30c882941391b1909f5af416 Line 13: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1159420 https://gerrit.ovirt.org/#/c/37664/1/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 171: } Line 172: Line 173: // verify that scsi reservation is enabled only when scsi passthrough is enabled Line 174: LunDisk lunDisk = ((LunDisk) getParameters().getDiskInfo()); Line 175: if (lunDisk.isScsiReservationUsed() && !lunDisk.isScsiPassthrough()) { > How about isUsingScsiReseervation()? Done Line 176: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ILLEGAL_SCSI_RESERVATION_USE); Line 177: } Line 178: Line 179: return true; Line 172: Line 173: // verify that scsi reservation is enabled only when scsi passthrough is enabled Line 174: LunDisk lunDisk = ((LunDisk) getParameters().getDiskInfo()); Line 175: if (lunDisk.isScsiReservationUsed() && !lunDisk.isScsiPassthrough()) { Line 176: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ILLEGAL_SCSI_RESERVATION_USE); > How about ACTION_TYPE_FAILED_DISK_USES_SCSI_RESERVATION? Will be consistent Actually, here we verify that scsi reservation cannot be set unless scsi passthrough is enabled. Constant changed to: ACTION_TYPE_FAILED_PASSTHROUGH_IS_DISABLED Line 177: } Line 178: Line 179: return true; Line 180: } Line 376: createDiskBasedOnLun(((LunDisk) getParameters().getDiskInfo()).isScsiReservationUsed()); Line 377: } Line 378: } Line 379: Line 380: private void createDiskBasedOnLun(final boolean isScsiReservationUsed) { > Again, will be nicer as isUsingScsiReservation. Done Line 381: final LUNs lun = ((LunDisk) getParameters().getDiskInfo()).getLun(); Line 382: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Line 383: @Override Line 384: public Void runInTransaction() { https://gerrit.ovirt.org/#/c/37664/1/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 527: // nothing to do Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { Line 531: List<VmDevice> vmManagedDevices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); > I this really list of devices managed by the vm, or list of devices used by Do we really want to avoid null checking? Line 532: if (vmManagedDevices != null && !vmManagedDevices.isEmpty()) { Line 533: for (VmDevice device : vmManagedDevices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { Line 531: List<VmDevice> vmManagedDevices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (vmManagedDevices != null && !vmManagedDevices.isEmpty()) { > I think it is better to exit early in this case, and avoid the isEmpty() wh Done Line 533: for (VmDevice device : vmManagedDevices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 536: } https://gerrit.ovirt.org/#/c/37664/1/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 122: } Line 123: Line 124: @Override Line 125: protected void executeVmCommand() { Line 126: ImagesHandler.setDiskAlias(getParameters().getDiskInfo(), getVm()); > Not related and seems unwanted change. Done Line 127: Line 128: if (resizeDiskImageRequested()) { Line 129: extendDiskImageSize(); Line 130: } else { Line 383: } Line 384: getImageDao().update(diskImage.getImage()); Line 385: updateQuota(diskImage); Line 386: updateDiskProfile(); Line 387: } else { > Do we have only two types of disks? Even if we have, this code will fail wh Good catch; done Line 388: updateLunProperties(); Line 389: } Line 390: Line 391: reloadDisks(); Line 406: } Line 407: } Line 408: Line 409: private void updateLunProperties() { Line 410: vmDeviceForVm.setScsiReservationUsed(((LunDisk)getNewDisk()).isScsiReservationUsed()); > I guess that this should be called only if getNewDisk returns a LunDisk? Wh Done Line 411: getVmDeviceDao().update(vmDeviceForVm); Line 412: } Line 413: }); Line 414: } https://gerrit.ovirt.org/#/c/37664/1/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 626: Map<String, Object> specParams, Line 627: boolean is_plugged, Line 628: Boolean isReadOnly, Line 629: Map<String, String> customProp, Line 630: boolean isScsiReservationUsed) { > isUsingScsiReservation/usingScsiReservation Done Line 631: VmDevice managedDevice = Line 632: new VmDevice(id, Line 633: type, Line 634: device.getName(), Line 1241: Line 1242: return result; Line 1243: } Line 1244: Line 1245: private static VmDevice addManagedDeviceImpl(VmDevice managedDevice) { > We need a better name; this name does not mean anything useful. Done 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/1/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 43: return isScsiReservationUsed; Line 44: } Line 45: Line 46: public void setScsiReservationUsed(boolean isScsiReservationUsed) { Line 47: this.isScsiReservationUsed = isScsiReservationUsed; > We have here 2 lines where isScsiReservationUsed is mentioned 4 times for s Done Line 48: } Line 49: Line 50: @Override Line 51: public int hashCode() { https://gerrit.ovirt.org/#/c/37664/1/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 129: Guid snapshotId, Line 130: String logicalName, Line 131: boolean isScsiReservationUsed) { Line 132: this(id, type, device, address, bootOrder, specParams, isManaged, isPlugged, isReadOnly, alias, customProperties, snapshotId, logicalName); Line 133: this.setScsiReservationUsed(isScsiReservationUsed); > Why not add another parameter to the existing constructor, and call this(.. Adding another param to existing ctor results in having this new ctor that I added. My idea here is not to change existing ctor in order not to break existing code. Line 134: } Line 135: Line 136: @Override Line 137: public Object getQueryableId() { Line 351: return isScsiReservationUsed; Line 352: } Line 353: Line 354: public void setScsiReservationUsed(boolean isScsiReservationUsed) { Line 355: this.isScsiReservationUsed = isScsiReservationUsed; > Same as in LunDisk Done Line 356: } https://gerrit.ovirt.org/#/c/37664/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 728: Line 729: USER_FAILED_TO_AUTHENTICATION_WRONG_AUTHENTICATION_METHOD(ErrorType.NO_AUTHENTICATION), Line 730: ACTION_TYPE_FAILED_VM_IS_PINNED_TO_HOST(ErrorType.CONFLICT), Line 731: ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION(ErrorType.CONFLICT), Line 732: ACTION_TYPE_FAILED_ILLEGAL_SCSI_RESERVATION_USE(ErrorType.CONFLICT), > These names do are very different for what seems to be the same conflict - The names are for two different things: 1. For indicating that VM uses a disk with scsi reservation, hence cannot be migrated 2. For indicating that scsi reservation cannot be set if scsi passthrough is disabled. As mentioned in a different comment, this message now is: ACTION_TYPE_FAILED_PASSTHROUGH_IS_DISABLED Line 733: ACTION_TYPE_FAILED_VM_IS_NON_MIGRTABLE(ErrorType.CONFLICT), Line 734: ACTION_TYPE_FAILED_VM_IS_NON_MIGRTABLE_AND_IS_NOT_FORCED_BY_USER_TO_MIGRATE(ErrorType.CONFLICT), Line 735: VDS_CANNOT_MAINTENANCE_IT_INCLUDES_NON_MIGRATABLE_VM(ErrorType.CONFLICT), Line 736: VDS_CANNOT_MAINTENANCE_VM_HAS_PLUGGED_DISK_SNAPSHOT(ErrorType.CONFLICT), https://gerrit.ovirt.org/#/c/37664/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DefaultGenericDaoDbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DefaultGenericDaoDbFacade.java: Line 94: } Line 95: Line 96: @Override Line 97: public void update(T entity) { Line 98: > Not related and unwanted change. Done Line 99: update(entity, getProcedureNameForUpdate()); Line 100: } Line 101: Line 102: protected void update(T entity, String procedureName) { https://gerrit.ovirt.org/#/c/37664/1/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 746: disk.propagate_errors: xs:boolean Line 747: disk.wipe_after_delete: xs:boolean Line 748: disk.quota.id: xs:string Line 749: disk.sgio: xs:string Line 750: disk.uses_scsi_reservation: xs:boolean > Here you chose nice wording, please use it for the accessors in LunDisk and Done Line 751: disk.lun_storage.host: xs:string Line 752: description: add a new direct lun disk to the virtual machine, this operation does not require size but needs lun connection details Line 753: - mandatoryArguments: {disk.id: 'xs:string'} Line 754: optionalArguments: https://gerrit.ovirt.org/#/c/37664/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationConstants.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationConstants.java: Line 1168: Line 1169: @DefaultStringValue("Read Only") Line 1170: String isReadOnlyVmDiskPopup(); Line 1171: Line 1172: @DefaultStringValue("SCSI Reservation Used") > Using SCSI Reservation? Done Line 1173: String isScsiReservationUsedEditor(); Line 1174: Line 1175: @DefaultStringValue("Enable SCSI Pass-Through") Line 1176: String isScsiPassthroughEditor(); https://gerrit.ovirt.org/#/c/37664/1/packaging/dbscripts/create_views.sql File packaging/dbscripts/create_views.sql: Line 314: Line 315: Line 316: CREATE OR REPLACE VIEW all_disks_for_vms Line 317: AS Line 318: SELECT all_disks_including_snapshots.*, vm_device.is_plugged, vm_device.is_readonly, vm_device.logical_name, vm_device.vm_id, vm_device.is_scsi_reservation_used > Lets be consistent and use the same terms in the ui, database and accessors Done Line 319: FROM all_disks_including_snapshots Line 320: JOIN vm_device ON vm_device.device_id = all_disks_including_snapshots.image_group_id Line 321: WHERE ((vm_device.snapshot_id IS NULL AND all_disks_including_snapshots.active IS NOT FALSE) Line 322: OR vm_device.snapshot_id = all_disks_including_snapshots.vm_snapshot_id); -- 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: 1 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: 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
