Liron Aravot has posted comments on this change. Change subject: core: Prevent VM migration when SCSI reservation is used ......................................................................
Patch Set 8: (3 comments) 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); > As mentioned before, uses scsi reservation override other config options. H This change blocks setting the migration support if scsi reservation is used, I don't understand why it's blocked if you can get to the same situation the other way around, have a vm with migration support set and then update the iscsi reservation flag. on both cases you end with a vm with migration support set and disk that is marked as using 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/validator/VmValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java: 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) { > This added after a test fail. I'd rather prefer to leave this instead of re I still don't understand why it's needed. You are guaranteed to not get null here. 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(); > Fixed though I am not sure why the validator works with a list of VMs. In general the messages here contains a replacement variable, so you should put the related vm name so it can be understood which vm is problematic. it's also relevant if you can mark few vms in the screen and choose to migrate and imo more correct. see for example vmHostCanLiveMerge(). 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; -- 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 <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 <rolla...@gmail.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
