Ala Hino 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? Fixed 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, Done 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 Done 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 Done 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 Done 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 Done 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 Done 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 Actually, here it doesn't matter because it is floating disk. However, I changed it so it clearer that scsi reservation is set when sgio is unfiltered. 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 " Done 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 Done 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' Done 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
