Liron Aravot has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 15: (20 comments) https://gerrit.ovirt.org/#/c/37664/15/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 417: ()); it can be null, which will lead to a npe. you can perform the same as was done for ReadOnly. https://gerrit.ovirt.org/#/c/37664/15/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 380: ); I'd move this check to be before the other migration checks, as it overrides them..so in terms of ux it's clearer. https://gerrit.ovirt.org/#/c/37664/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 670: this change is unneeded..can be restored. https://gerrit.ovirt.org/#/c/37664/15/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 407: if (unlockImage && cinderDisk.getImageStatus() == ImageStatus.LOCKED) { Line 408: cinderDisk.setImageStatus(ImageStatus.OK); Line 409: } Line 410: getImageDao().update(cinderDisk.getImage()); Line 411: } else if (disk.getDiskStorageType() == DiskStorageType.LUN) { as this patch adds another elseif..i'd consider to replace to switch case (can be done in a further patch) Line 412: updateLunProperties((LunDisk)getNewDisk()); Line 413: } Line 414: Line 415: reloadDisks(); https://gerrit.ovirt.org/#/c/37664/15/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java: Line 237: ())); perhaps it'll be better to aggregate the list of problematic vms and populate the message with all of them. https://gerrit.ovirt.org/#/c/37664/15/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 199: // this operation is valid only when attaching disk to VMs Line 200: if (vm == null && Boolean.TRUE.equals(lunDisk.isUsingScsiReservation())) { Line 201: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_SCSI_RESERVATION_NOT_VALID_FOR_FLOATING_DISK); Line 202: } Line 203: // reservation is can be enabled only when sgio is unfiltered /s/is/ Line 204: if (lunDisk.isUsingScsiReservation() != null && lunDisk.isUsingScsiReservation() && lunDisk.getSgio() == ScsiGenericIO.FILTERED) { Line 205: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_SGIO_IS_FILTERED); Line 206: } Line 207: Line 204: you can use here also Boolean.TRUE.equals() https://gerrit.ovirt.org/#/c/37664/15/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 605: LunDisk disk = createISCSILunDisk(); Line 606: disk.setSgio(sgio); Line 607: disk.setUsingScsiReservation(isUsingScsiReservation); Line 608: disk.setDiskInterface(diskInterface); Line 609: return disk; remove one space Line 610: } Line 611: Line 612: @Test Line 613: public void testIscsiLunCannotBeAddedIfSgioIsFilteredAndScsiReservationEnabled() { Line 620: assertFalse("Lun disk added successfully WHILE sgio is filtered and scsi reservation is enabled", Line 621: command.checkIfLunDiskCanBeAdded(spyDiskValidator(disk))); Line 622: assertTrue(command.getReturnValue() Line 623: .getCanDoActionMessages() Line 624: .contains(VdcBllMessages.ACTION_TYPE_FAILED_SGIO_IS_FILTERED.toString())); replace with verifyCanDoActionMessagesContainMessage Line 625: } Line 626: Line 627: @Test Line 628: public void testIscsiLunCanBeAddedIfScsiPassthroughEnabledAndScsiReservationEnabled() { Line 649: VdcBllMessages replace with verifyCanDoActionMessagesContainMessage https://gerrit.ovirt.org/#/c/37664/15/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 316: && alias.equals(other.alias) Line 317: && ObjectUtils.objectsEqual(customProperties, other.customProperties) Line 318: && ObjectUtils.objectsEqual(snapshotId, other.snapshotId) Line 319: && ObjectUtils.objectsEqual(logicalName, other.logicalName) Line 320: && ObjectUtils.objectsEqual(usingScsiReservation, other.usingScsiReservation)); it's a primitive, you can avoid using objectEqual Line 321: } Line 322: Line 323: @Override Line 324: public String toString() { https://gerrit.ovirt.org/#/c/37664/15/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 748: ), is this one used? if it does, its missing from https://gerrit.ovirt.org/#/c/37664/15/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties Line 761: ), this one can be removed https://gerrit.ovirt.org/#/c/37664/15/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 207: @Test Line 208: public void testCreateVmDeviceUsingScsiReservationProperty() { Line 209: VmDevice vmDevice = createVmDevice(Guid.newGuid()); Line 210: vmDevice.setUsingScsiReservation(true); Line 211: assertEquals(vmDevice.isUsingScsiReservation(), true); what's tested here? Line 212: } Line 213: Line 214: @Test Line 215: public void testUpdateVmDeviceUsingScsiReservationProperty() { Line 218: ())); seems like the address assertion isn't related, we are testing here the scsi reservation property. Line 223: } Line 224: Line 225: private VmDevice createVmDevice(Guid vmGuid) { Line 226: Map<String, String> customProp = new LinkedHashMap<>(); Line 227: customProp.put("prop1", "value1"); you can remove customProp and use Collections.singletonMap Line 228: return new VmDevice(new VmDeviceId(Guid.newGuid(), vmGuid), Line 229: VmDeviceGeneralType.DISK, Line 230: "floppy", Line 231: "type:'drive', controller:'0', bus:'0', unit:'1'", https://gerrit.ovirt.org/#/c/37664/15/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 518: (); see comment regards the messages on previous files https://gerrit.ovirt.org/#/c/37664/15/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 1806: .") I'd suggest to change this a bit: "When at least one of the activated VM disks uses iSCSI reservation the VM can't be migrated" https://gerrit.ovirt.org/#/c/37664/15/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationMessages.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationMessages.java: Line 141: , this is a bit confusing, one can look at the vm settings and look for scsi reservation. I'd clarify that it's settings on a disk that is plugged to the vm. https://gerrit.ovirt.org/#/c/37664/15/packaging/dbscripts/upgrade/03_06_1170_add_uses_scsi_reservation.sql File packaging/dbscripts/upgrade/03_06_1170_add_uses_scsi_reservation.sql: Line 1: '); Imo this should "not null", null on that column will break the engine code. -- 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: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Candace Sheremeta <csher...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
