Liron Aravot has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 6: (11 comments) https://gerrit.ovirt.org/#/c/37664/6/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 627: dao.save(managedDevice); Line 628: // If we add Disk/Interface/CD/Floppy, we have to recalculate boot order Line 629: if (type == VmDeviceGeneralType.DISK || type == VmDeviceGeneralType.INTERFACE) { Line 630: // recalculate boot sequence Line 631: VmBase vmBase = DbFacade.getInstance().getVmStaticDao().get(managedDevice.getId().getVmId()); why was the first part changed? Line 632: updateBootOrderInVmDeviceAndStoreToDB(vmBase); Line 633: } Line 634: return managedDevice; Line 635: } https://gerrit.ovirt.org/#/c/37664/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidationUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidationUtils.java: Line 94: * @param devices List of devices Line 95: * @return If scsi lun with scsi reservation is plugged to VM Line 96: */ Line 97: public static boolean isVmPluggedDiskUsingScsiReservation(List<VmDevice> devices) { Line 98: if (devices == null) { Please move this code to VmValidator, also - this method should load the devices it needs to check rather then get them from the caller, what if the caller loaded wrong devices? what if we need to change the checked devices? that way we won't have to fix all the callers and this method will encapsulate the check logic. Line 99: return false; Line 100: } Line 101: for (VmDevice device : devices) { Line 102: if (device.getIsPlugged() && device.isUsingScsiReservation()) { https://gerrit.ovirt.org/#/c/37664/6/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 196: } Line 197: Line 198: public ValidationResult isUsingScsiReservationValid(VM vm, LunDisk lunDisk) { Line 199: // this operation is valid only when attaching disk to VMs Line 200: if (vm == null && lunDisk.isUsingScsiReservation()) { see related comment in LunDisk 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 Line 204: if (lunDisk.isUsingScsiReservation() && lunDisk.getSgio() == ScsiGenericIO.FILTERED) { https://gerrit.ovirt.org/#/c/37664/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmCommandTest.java: Line 353: prepareVmToPassCanDoAction(); Line 354: VmDevice device = createVmDevice(); Line 355: Line 356: doReturn(vmDeviceDAO).when(command).getVmDeviceDao(); Line 357: when(vmDeviceDAO.getVmDeviceByVmIdAndType(vm.getId(), VmDeviceGeneralType.DISK)).thenReturn(Arrays.asList(device)); 1. you can share some code with mockGraphicsDevice 2. in terms of ordering, i'd seperate between entities preperations and mocks- makes reading it easier. Line 358: Line 359: vm.setMigrationSupport(MigrationSupport.MIGRATABLE); Line 360: CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, Line 361: VdcBllMessages.MIGRATION_SUPPORT_NOT_VALID_WHEN_VM_USES_SCSI_RESERVATION); Line 360: CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, Line 361: VdcBllMessages.MIGRATION_SUPPORT_NOT_VALID_WHEN_VM_USES_SCSI_RESERVATION); Line 362: } Line 363: Line 364: private VmDevice createVmDevice() { you can use Guid.emptyGuid Line 365: return new VmDevice(new VmDeviceId(Guid.newGuid(), vm.getId()), Line 366: VmDeviceGeneralType.DISK, Line 367: "device", Line 368: "address", https://gerrit.ovirt.org/#/c/37664/6/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/DiskValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/DiskValidatorTest.java: Line 49: private VmDAO vmDAO; Line 50: private DiskValidator validator; Line 51: private DiskImage disk; Line 52: private LunDisk lunDisk; Line 53: private DiskValidator lunValidator; removing the static modifiers isn't related to this patch Line 54: Line 55: private DiskImage createDiskImage() { Line 56: DiskImage disk = new DiskImage(); Line 57: disk.setId(Guid.newGuid()); Line 250: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_SGIO_IS_FILTERED)); Line 251: } Line 252: Line 253: @Test Line 254: public void testIsUsingScsiReservationValidWhenAddingFloatingDisk() { Please share code between the methods public void isUsingScsiReservationFails(ScsiGenericIO scsiGenericIo, VdcBllMessages failureMessage) { setupForLun(); LunDisk lunDisk = createLunDisk(); lunDisk.setSgio(scsiGenericIo); lunDisk.setUsingScsiReservation(true); assertThat(lunValidator.isUsingScsiReservationValid(createVM(), lunDisk), failsWith(failureMessage)) } Line 255: setupForLun(); Line 256: Line 257: LunDisk lunDisk1 = createLunDisk(); Line 258: lunDisk1.setSgio(ScsiGenericIO.FILTERED); Line 254: public void testIsUsingScsiReservationValidWhenAddingFloatingDisk() { Line 255: setupForLun(); Line 256: Line 257: LunDisk lunDisk1 = createLunDisk(); Line 258: lunDisk1.setSgio(ScsiGenericIO.FILTERED); here it shouldn't be filtered Line 259: lunDisk1.setUsingScsiReservation(true); Line 260: Line 261: assertThat(lunValidator.isUsingScsiReservationValid(null, lunDisk1), Line 262: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_SCSI_RESERVATION_NOT_VALID_FOR_FLOATING_DISK)); https://gerrit.ovirt.org/#/c/37664/6/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 38: public boolean isAllowSnapshot() { Line 39: return false; Line 40: } Line 41: Line 42: public boolean isUsingScsiReservation() { this should be a Boolean, it can be null (when you don't load the disk as "disk for vm") Line 43: return usingScsiReservation; Line 44: } Line 45: Line 46: public void setUsingScsiReservation(boolean value) { https://gerrit.ovirt.org/#/c/37664/6/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 259: public boolean isUsingScsiReservation() { Line 260: return usingScsiReservation; Line 261: } Line 262: Line 263: public void setUsingScsiReservation(boolean flag) { please rename flag to usingScsiReservation Line 264: this.usingScsiReservation = flag; Line 265: } Line 266: Line 267: public void setCustomProperties(Map<String, String> customProperties) { https://gerrit.ovirt.org/#/c/37664/6/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/DiskMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/DiskMapper.java: Line 171: } else { Line 172: model.setLunStorage(StorageLogicalUnitMapper.map(((LunDisk) entity).getLun(), new Storage())); Line 173: if (entity.getSgio() != null && entity.getDiskInterface() == map(DiskInterface.VIRTIO_SCSI, null)) { Line 174: model.setSgio(map(entity.getSgio(), null)); Line 175: model.setUsesScsiReservation(((LunDisk)entity).isUsingScsiReservation()); why is this within the if? if the entity is lun it should be set. if there's something wrong it should fail in the command. please test it on DiskMapperTest Line 176: } Line 177: } Line 178: return model; Line 179: } -- 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: 6 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: Eli Mesika <[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: Tomas Jelinek <[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
