Ala Hino has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 2: (28 comments) https://gerrit.ovirt.org/#/c/37664/2//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2015-02-10 14:56:12 +0200 Line 4: Commit: Ala Hino <[email protected]> Line 5: CommitDate: 2015-02-25 09:26:21 +0200 Line 6: Line 7: Core: Prevent VM migration when SCSI reservation is used > I think that "Core" should be "core" - check how others are formatting the Done Line 8: Line 9: Added an option to enable admin mark whether SCSI reservation is used when Line 10: creating Direct Lun. When migrating a VM, if SCSI reservation is marked, Line 11: operation fails. https://gerrit.ovirt.org/#/c/37664/2/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 175: { > please have this logic in the validator same as the other validations. Done Line 379: ()); > why do we need to pass it? we can just get that info in the createDiskBased Here, in this cond, we are confidence that we don't get class cat exception https://gerrit.ovirt.org/#/c/37664/2/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 377: { > We should think what happens when host goes non operational- Good point. Maybe worth using some config params. I'd prefer to have a separate patch for this one. Line 373: if (vm.getMigrationSupport() == MigrationSupport.PINNED_TO_HOST) { Line 374: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_PINNED_TO_HOST); Line 375: } Line 376: Line 377: if (vmUsesScsiReservation()) { > should be added to tests No UT for this command currently. Adding Omer Frenkel for the review. Line 378: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION); Line 379: } Line 380: Line 381: if (vm.getMigrationSupport() == MigrationSupport.IMPLICITLY_NON_MIGRATABLE Line 526: public void onPowerringUp() { Line 527: // nothing to do Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { > /s/vmUsesScsiReservation/isVmUsesScsiReservation Done Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (devices != null) { Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 526: public void onPowerringUp() { Line 527: // nothing to do Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { > formatting Done Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (devices != null) { Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 531: ()); > use getVmDeviceByVmIdAndType, Done Line 531: ()); > you should also check only plugged devices, if the disk is unplugged it's u Done Line 527: // nothing to do Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); > use getDbFacade() Done Line 532: if (devices != null) { Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 528: } Line 529: Line 530: private boolean vmUsesScsiReservation() { Line 531: List<VmDevice> devices = DbFacade.getInstance().getVmDeviceDao().getVmDeviceByVmId(getVmId()); Line 532: if (devices != null) { > devices can not be null, the API will return empty list in the worst case Done Line 533: for (VmDevice device : devices) { Line 534: if (device.isScsiReservationUsed()) { Line 535: return true; Line 536: } https://gerrit.ovirt.org/#/c/37664/2/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 385: updateQuota(diskImage); Line 386: updateDiskProfile(); Line 387: } Line 388: Line 389: if (disk.getDiskStorageType() == DiskStorageType.LUN) { > indeed :) Indeed for which part ... I am confused now ... are there only IMAGE and LUN disk storage types? Line 390: updateLunProperties((LunDisk)getNewDisk()); Line 391: } Line 392: Line 393: reloadDisks(); Line 389: ( > You should use if (!isDiskImage()) Original code uses: disk.getDiskStorageType() == DiskStorageType.IMAGE Line 385: updateQuota(diskImage); Line 386: updateDiskProfile(); Line 387: } Line 388: Line 389: if (disk.getDiskStorageType() == DiskStorageType.LUN) { > else if? Done Line 390: updateLunProperties((LunDisk)getNewDisk()); Line 391: } Line 392: Line 393: reloadDisks(); Line 387: } Line 388: Line 389: if (disk.getDiskStorageType() == DiskStorageType.LUN) { Line 390: updateLunProperties((LunDisk)getNewDisk()); Line 391: } > yeah, but shouldn't happen, maybe consider logging in another patch.. Added log message Line 392: Line 393: reloadDisks(); Line 394: updateBootOrder(); Line 395: Line 411: void > Why not doing this as part of updateDeviceProperties I think updateDeviceProperties is used for generic device properties and then we have separate logic for IMAGE and Lun Line 412: ()); > there's a problem here, if the device address was cleared in updateDevicePr Done https://gerrit.ovirt.org/#/c/37664/2/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 623: static > why can't we use the original addManagedDevice instead adding a new API? The consideration to add a new a API is not to break existing code Line 1044: private static void addEmptyCD(Guid dstId) { Line 1045: VmDeviceUtils.addManagedDevice(new VmDeviceId(Guid.newGuid(), dstId), Line 1046: VmDeviceGeneralType.DISK, Line 1047: VmDeviceType.CDROM, Line 1048: Collections.<String, Object>singletonMap(VdsProperties.Path, ""), > Is this change related? No ... Line 1049: true, Line 1050: true, Line 1051: null); Line 1052: } https://gerrit.ovirt.org/#/c/37664/2/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 isUsingScsiReservation; > s/isUsingScsiReservation/usingScsiReservation Done Line 18: Line 19: @Override Line 20: public DiskStorageType getDiskStorageType() { Line 21: return DiskStorageType.LUN; Line 42: public boolean isUsingScsiReservation() { Line 43: return isUsingScsiReservation; Line 44: } Line 45: Line 46: public void setUsingScsiReservation(boolean flag) { > s/flag/usingScsiReservation Global agreement was to use "value" instead of "flag" or "usingScsiReservation" Line 47: this.isUsingScsiReservation = flag; Line 48: } Line 49: Line 50: @Override Line 51: > What about hashcode and equals, should they need to be regenerated with the Done https://gerrit.ovirt.org/#/c/37664/2/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 84: isScsiReservationUsed > What about equals and hashcode, should they need to be regenerated? Done https://gerrit.ovirt.org/#/c/37664/2/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/DiskDaoDbFacadeImpl.java: Line 147: } > Please add this to LunDiskRowMapper Agreed with Maor to leave the this code for now. Consider changing it while doing the refactor work in this aea. https://gerrit.ovirt.org/#/c/37664/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java: Line 513: Line 514: @DefaultStringValue("Cannot ${action} ${type}. VM is pinned to Host.") Line 515: String ACTION_TYPE_FAILED_VM_IS_PINNED_TO_HOST(); Line 516: Line 517: @DefaultStringValue("Cannot ${action} ${type}. VM uses Scsi reservation.") > s/Scsi/SCSI Done Line 518: String ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION(); Line 519: Line 520: @DefaultStringValue("Cannot ${action} ${type}. Scsi reservation can be set only when SCSI passthrough is set.") Line 521: String ACTION_TYPE_FAILED_ILLEGAL_SCSI_RESERVATION_USE(); Line 516: Line 517: @DefaultStringValue("Cannot ${action} ${type}. VM uses Scsi reservation.") Line 518: String ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION(); Line 519: Line 520: @DefaultStringValue("Cannot ${action} ${type}. Scsi reservation can be set only when SCSI passthrough is set.") > same Done Line 521: String ACTION_TYPE_FAILED_ILLEGAL_SCSI_RESERVATION_USE(); Line 522: Line 523: @DefaultStringValue("Note: The VM is pinned to Host '${VdsName}' but cannot run on it.") Line 524: String VM_PINNED_TO_HOST_CANNOT_RUN_ON_THE_DEFAULT_VDS(); https://gerrit.ovirt.org/#/c/37664/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AbstractDiskModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AbstractDiskModel.java: Line 258: Line 259: setIsUsingScsiReservation(new EntityModel<Boolean>()); Line 260: getIsUsingScsiReservation().setEntity(false); Line 261: Line 262: > redundant empty line Done Line 263: setIsScsiPassthrough(new EntityModel<Boolean>()); Line 264: getIsScsiPassthrough().setIsAvailable(false); Line 265: getIsScsiPassthrough().setEntity(true); Line 266: getIsScsiPassthrough().getEntityChangedEvent().addListener(this); Line 342: DiskModel diskModel = (DiskModel) target; Line 343: ArrayList<StorageDomain> storageDomains = (ArrayList<StorageDomain>) returnValue; Line 344: Line 345: ArrayList<StorageDomain> filteredStorageDomains = new ArrayList<StorageDomain>(); Line 346: for (StorageDomain a : storageDomains) { > irrelevant to the patch Where do these spaces come from? I don't see them in my IntelliJ Line 347: if (!a.getStorageDomainType().isIsoOrImportExportDomain() && a.getStatus() == StorageDomainStatus.Active) { Line 348: filteredStorageDomains.add(a); Line 349: } Line 350: } -- 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: 2 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
