Nir Soffer has posted comments on this change. Change subject: Core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 1: (19 comments) http://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()? Why if using isScsiPassthrough(), we don't care about ScsiReservation? This must be documented here. 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 with the VM_USES... message. 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. Actually usingScsiReservation is even better but this code seems to suffer from "is" prefixism. Line 381: final LUNs lun = ((LunDisk) getParameters().getDiskInfo()).getLun(); Line 382: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Line 383: @Override Line 384: public Void runInTransaction() { http://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 the vm? Isn't "devices" good enough in the context of this method? Do we really get get null from getVmDeviceByVmId() when there are no devices? 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() which does nothing. Line 533: for (VmDevice device : vmManagedDevices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 536: } http://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 373 Line 374 Line 375 Line 376 Line 377 If you think that updating lun properties does not belong here, you must agree that updating image properties does not belong here and should be moved to updateImageProperties() :-) I would move this away in another patch, which will make your addition fit better. 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. 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 when we add another type. I think we should check for LUN type before trying to update one. 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? What if this cast fails? Why does this method does not receive the disk as parameter, after we checked its type? Line 411: getVmDeviceDao().update(vmDeviceForVm); Line 412: } Line 413: }); Line 414: } http://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 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. How about saveManageDevice()? 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) { http://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 storing one bit of information :-) How about: public void setScsiReservationUsed(boolean flag) { this.isScsiReservationUsed = flag; } Now we mention this only twice. Line 48: } Line 49: Line 50: @Override Line 51: public int hashCode() { http://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(..., isScsiReservationUsed) ? 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 Line 356: } http://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 - why? 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), http://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. Line 99: update(entity, getProcedureNameForUpdate()); Line 100: } Line 101: Line 102: protected void update(T entity, String procedureName) { http://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 VmDevice. 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: http://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? Line 1173: String isScsiReservationUsedEditor(); Line 1174: Line 1175: @DefaultStringValue("Enable SCSI Pass-Through") Line 1176: String isScsiPassthroughEditor(); http://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. 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 http://gerrit.ovirt.org/37664 To unsubscribe, visit http://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: 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
