Ala Hino 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. Done 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 override Done 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. Done 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 ( Done 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 popula Done 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/ Done 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() Done 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 Done 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 Done Line 625: } Line 626: Line 627: @Test Line 628: public void testIscsiLunCanBeAddedIfScsiPassthroughEnabledAndScsiReservationEnabled() { Line 649: VdcBllMessages > replace with verifyCanDoActionMessagesContainMessage Done 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 Done 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 It is used in DiskValidator Line 761: ), > this one can be removed Done 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? Removed 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 scs Done 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 Done 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 This is needed when, for example, a floating disk is created using rest and scsi reservation provided 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: Done 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 Done 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. If default is false, can we still get null? What code of the engine this could break? -- 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
