Ala Hino has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 8: (12 comments) https://gerrit.ovirt.org/#/c/37664/8/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 172: } Line 173: Line 174: ValidationResult validationResult = diskValidator.isUsingScsiReservationValid(getVm(), lunDisk); Line 175: if (!validate(validationResult)) { Line 176: return false; > please change to validate(diskValidator.isUsingScsiReservationValid(getVm() Done Line 177: } Line 178: Line 179: return true; Line 180: } Line 374: createDiskBasedOnImage(); Line 375: } Line 376: Line 377: if (DiskStorageType.LUN == getParameters().getDiskInfo().getDiskStorageType()) { Line 378: createDiskBasedOnLun(); > this change is irrelevant for this patch Done Line 379: } Line 380: } Line 381: Line 382: private void createDiskBasedOnLun() { https://gerrit.ovirt.org/#/c/37664/8/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 378: } Line 379: Line 380: VmValidator vmValidator = new VmValidator(vm); Line 381: Line 382: if (isVmPluggedDiskUsingScsiReservation(vmValidator)) { > so was it decided to ignore the force migration case? Yes. After talking to Yaniv Dary, we decided that this option overrides other options. In addition, when VM uses lun with scsi reservation, migration policy cannot be set, it is graded-out with vm-pinned-to host selected and a tool explains that VM is implicitly pinned to host because it uses scsi reservation. Line 383: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_USES_SCSI_RESERVATION); Line 384: } Line 385: Line 386: switch (vm.getStatus()) { https://gerrit.ovirt.org/#/c/37664/8/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 642: Line 643: VmValidator vmValidator = createVmValidator(vmFromParams); Line 644: // check that is vm uses scsi reservatin, then migration support should be null Line 645: if (vmUsesScsiReservation(vmValidator) && vmFromParams.getMigrationSupport() != null) { Line 646: return failCanDoAction(VdcBllMessages.MIGRATION_SUPPORT_NOT_VALID_WHEN_VM_USES_SCSI_RESERVATION); > what about the other way around? update the vm migrate support and then mar As mentioned before, uses scsi reservation override other config options. Hence, in this specific case, if uses scsi reservation is set, migration policy cannot be set. In addition, migration policy has nothing to do with scsi reservation and no value of it can affect scsi reservation Line 647: } Line 648: Line 649: if (!validatePinningAndMigration(getReturnValue().getCanDoActionMessages(), Line 650: getParameters().getVm().getStaticData(), getParameters().getVm().getCpuPinning())) { https://gerrit.ovirt.org/#/c/37664/8/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 680: return readOnlyNewValue != null && !getVmDeviceForVm().getIsReadOnly().equals(readOnlyNewValue); Line 681: } Line 682: Line 683: private boolean updateIsUsingScsiReservationRequested(LunDisk lunDisk) { Line 684: return getVmDeviceForVm().isUsingScsiReservation() != lunDisk.isUsingScsiReservation(); > if lunDisk.isUsingScsiReservation() is null we'll have a NPE here Good catch. Fixed Line 685: } Line 686: Line 687: protected boolean updateWipeAfterDeleteRequested() { Line 688: return getNewDisk().isWipeAfterDelete() != getOldDisk().isWipeAfterDelete(); https://gerrit.ovirt.org/#/c/37664/8/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 227: /** Line 228: * Checks whether VM uses lun with scsi reservation true. Line 229: * @return If scsi lun with scsi reservation is plugged to VM Line 230: */ Line 231: public boolean isVmPluggedDiskUsingScsiReservation() { > sorry if i missed that before, but all the other methods here are retuning Done Line 232: // for unit tests Line 233: if (getDbFacade().getVmDeviceDao() == null) { Line 234: return false; Line 235: } Line 229: * @return If scsi lun with scsi reservation is plugged to VM Line 230: */ Line 231: public boolean isVmPluggedDiskUsingScsiReservation() { Line 232: // for unit tests Line 233: if (getDbFacade().getVmDeviceDao() == null) { > can you remove it? it can never be null and if there's a test for that - we This added after a test fail. I'd rather prefer to leave this instead of removing the test Line 234: return false; Line 235: } Line 236: Line 237: VM vm = vms.iterator().next(); Line 233: if (getDbFacade().getVmDeviceDao() == null) { Line 234: return false; Line 235: } Line 236: Line 237: VM vm = vms.iterator().next(); > you should iterate over all the vms as the rest of the validations here. Fixed though I am not sure why the validator works with a list of VMs. In this case, id a VM uses scsi reservation, validator caller cannot know which one it is because it only gets back a validation result. Line 238: List<VmDevice> devices = getDbFacade().getVmDeviceDao().getVmDeviceByVmIdAndType(vm.getId(), VmDeviceGeneralType.DISK); Line 239: for (VmDevice device : devices) { Line 240: if (device.getIsPlugged() && device.isUsingScsiReservation()) { Line 241: return true; https://gerrit.ovirt.org/#/c/37664/8/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() != null && lunDisk.isUsingScsiReservation()) { > you can replace lunDisk.isUsingScsiReservation() != null && lunDisk.isUsing Good catch. Fixed 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() != null && lunDisk.isUsingScsiReservation() && lunDisk.getSgio() == ScsiGenericIO.FILTERED) { https://gerrit.ovirt.org/#/c/37664/8/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 236: Line 237: assertThat(lunValidator.isLunDiskVisible(lunDisk.getLun(), vds), Line 238: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_DISK_LUN_INVALID)); Line 239: } Line 240: > consider to add a positive test(s) Done Line 241: @Test Line 242: public void testIsUsingScsiReservationValidWhenSgioIsFiltered() { Line 243: setupForLun(); Line 244: https://gerrit.ovirt.org/#/c/37664/8/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 50: @Override Line 51: public int hashCode() { Line 52: final int prime = 31; Line 53: int result = super.hashCode(); Line 54: result = prime * result + (usingScsiReservation ? 1231 : 1237); > if it's null npe will occur Good catch. Fixed Line 55: result = prime * result + ((lun == null) ? 0 : lun.hashCode()); Line 56: return result; Line 57: } Line 58: https://gerrit.ovirt.org/#/c/37664/8/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 80: * The device logical name. Line 81: */ Line 82: private String logicalName; Line 83: Line 84: private boolean usingScsiReservation; > it seems fine to me, what i don't really like is that this is added as bool Good catch. Discussed this with derez and decided to keep this variable declared as boolean Line 85: Line 86: /** Line 87: * Map of custom properties Line 88: */ -- 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: 8 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
