Dudi Maroshi has posted comments on this change. Change subject: core: enable pinning to multiple hosts ......................................................................
Patch Set 8: (12 comments) https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 1030: protected void addVmNumaNodes() { Line 1031: List<VmNumaNode> numaNodes = getParameters().getVmStaticData().getvNumaNodeList(); Line 1032: VmNumaNodeOperationParameters params = new VmNumaNodeOperationParameters(getVmId(), numaNodes); Line 1033: params.setNumaTuneMode(getParameters().getVmStaticData().getNumaTuneMode()); Line 1034: // TODO maroshi - redesign NUMAfor multiple hosts pinning > no names in comment Done Line 1035: params.setDedicatedHostList(getParameters().getVmStaticData().getDedicatedVmForVdsList()); Line 1036: params.setMigrationSupport(getParameters().getVmStaticData().getMigrationSupport()); Line 1037: if (numaNodes == null || numaNodes.isEmpty()) { Line 1038: return; https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVMClusterCommand.java: Line 79: } Line 80: } Line 81: } Line 82: Line 83: if (vm.getDedicatedVmForVdsList().isEmpty() == false) { > if we went for a rename of that getter, lets just call it some other sane n I agree, it is a good idea to rethink the property name. I still want the name to reflect the multiplicity. Adding an (s) is not good enough and error prone (for reading and writing). Line 84: vm.setDedicatedVmForVdsList(new LinkedList<Guid>()); Line 85: dedicatedHostWasCleared = true; Line 86: } Line 87: https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommandBase.java: Line 270: getVm().getStaticData().setQuotaId(getParameters().getQuotaId()); Line 271: Line 272: // if "run on host" field points to a non existent vds (in the current cluster) -> remove field and continue Line 273: if (!VmHandler.validateDedicatedVdsExistOnSameCluster(getVm().getStaticData(), null)) { Line 274: getVm().setDedicatedVmForVdsList(new LinkedList<Guid>()); > why LinkedList and not ArrayList? 1. ArryList by default initially allocate 10 objects. 2. We do not access list items by index and it is not ordered (always iterator). The comment is valid, we want to be consistent with LinkedList or ArrayList. Will fix that. We also want to insert static empty list. Will fix that as well. Line 275: } Line 276: Line 277: if (getVm().getOriginalTemplateGuid() != null && !VmTemplateHandler.BLANK_VM_TEMPLATE_ID.equals(getVm().getOriginalTemplateGuid())) { Line 278: // no need to check this for blank https://gerrit.ovirt.org/#/c/41962/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 115: setVdsIdRef(getVm().getRunOnVds()); Line 116: List<Guid> destinationHostGuidList = new LinkedList<>(); Line 117: if (getDestinationVdsId() != null){ Line 118: destinationHostGuidList.add(getDestinationVdsId()); Line 119: } > that code is duplicated in line 440. why can't it be a part of the old getD Done Line 120: Guid vdsToRunOn = Line 121: SchedulingManager.getInstance().schedule(getVdsGroup(), Line 122: getVm(), Line 123: getVdsBlackList(), https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java: Line 361: if (getVmTemplate() != null && !isInstanceType() && !isBlankTemplate()) { Line 362: // host-specific parameters can be changed by administration role only Line 363: List<Guid> tmpltDdctHostsLst = getVmTemplate().getDedicatedVmForVdsList(); Line 364: List<Guid> prmTmpltDdctHostsLst = getParameters().getVmTemplateData().getDedicatedVmForVdsList(); Line 365: // tmpltDdctHostsLst.equals(prmTmpltDdctHostsLs is not good enough, lists order may change > this check is probably viable if we have only one host in the list and it w Need to discuss this Line 366: if (CollectionUtils.isEqualCollection(tmpltDdctHostsLst, prmTmpltDdctHostsLst) == false) { Line 367: permissionList.add( Line 368: new PermissionSubject(getParameters().getVmTemplateId(), Line 369: VdcObjectType.VmTemplate, https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java: Line 108: if (vmStatic.getDedicatedVmForVdsList().isEmpty()){ Line 109: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_CANNOT_BE_PINNED_TO_CPU_WITH_UNDEFINED_HOST); Line 110: } Line 111: // TODO maroshi - validate, need to check each dedicated host? Maybe vmStatic.getRunOnVds() ? Line 112: for (Guid dedicatedHostGuid : vmStatic.getDedicatedVmForVdsList()){ > suggest refactoring this out, same as VmHandler.validateDedicatedVdsExistOn Done Line 113: dedicatedVds = getVds(dedicatedHostGuid); Line 114: Line 115: Collection<Integer> onlinePcpus = new HashSet<>(); Line 116: https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/AbstractVmHostDevicesCommand.java: Line 12: import org.ovirt.engine.core.dao.HostDeviceDao; Line 13: import org.ovirt.engine.core.dao.VmDeviceDAO; Line 14: Line 15: import javax.inject.Inject; Line 16: > remove Done Line 17: import java.util.ArrayList; Line 18: import java.util.Collection; Line 19: import java.util.Collections; Line 20: import java.util.HashSet; Line 105: } Line 106: return null; Line 107: } Line 108: Line 109: private HostDevice fetchHostDevice(String deviceName) { > again proposing this is valid when a single host Done Line 110: // TODO maroshi - redesign query, return from all pinned hosts, Line 111: // or from getVm().getRunOnVds(), or other hosts? Line 112: for (Guid hostId : getVm().getDedicatedVmForVdsList()){ Line 113: HostDevice hostDevice = hostDeviceDao.getHostDeviceByHostIdAndDeviceName(hostId, deviceName); https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java: Line 69: * @param vm Line 70: * @return true if the specified VM is pinned to a host and has host devices directly attached to it Line 71: */ Line 72: public boolean checkVmNeedsDirectPassthrough(VM vm) { Line 73: return vm.getDedicatedVmForVdsList().isEmpty() == false && supportsHostDevicePassthrough(vm) && > that should be changed as well Done Line 74: checkVmNeedsDirectPassthrough(vm.getId()); Line 75: } Line 76: Line 77: private boolean checkVmNeedsDirectPassthrough(Guid vmId) { https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/numa/vm/AbstractVmNumaNodeCommand.java: Line 69: // if VM do not contain any NUMA node, skip checking Line 70: return true; Line 71: } Line 72: Line 73: boolean pinHost = !Config.<Boolean> getValue(ConfigValues.SupportNUMAMigration); > off topic but not completly - Done Line 74: List<VdsNumaNode> hostNumaNodes = new ArrayList<>(); Line 75: for (Guid vdsId : getDedicatedHostList()) { Line 76: if (pinHost && vdsId == null && getMigrationSupport() != MigrationSupport.PINNED_TO_HOST) { Line 77: return failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NOT_EXIST); Line 71: } Line 72: Line 73: boolean pinHost = !Config.<Boolean> getValue(ConfigValues.SupportNUMAMigration); Line 74: List<VdsNumaNode> hostNumaNodes = new ArrayList<>(); Line 75: for (Guid vdsId : getDedicatedHostList()) { > same thing with Numa - we should allow numa-pinning only when there is one We need to define a clear policy for multiple pinning to NUMA. This requires a deeper discussion. Line 76: if (pinHost && vdsId == null && getMigrationSupport() != MigrationSupport.PINNED_TO_HOST) { Line 77: return failCanDoAction(VdcBllMessages.VM_NUMA_PINNED_VDS_NOT_EXIST); Line 78: } Line 79: https://gerrit.ovirt.org/#/c/41962/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/PinToHostPolicyUnit.java: Line 24: public List<VDS> filter(List<VDS> hosts, VM vm, Map<String, String> parameters, PerHostMessages messages) { Line 25: if (vm.getMigrationSupport() == MigrationSupport.PINNED_TO_HOST) { Line 26: // host has been specified for pin to host. Line 27: if(vm.getDedicatedVmForVdsList().isEmpty() == false) { Line 28: for (VDS host : hosts) { > I guess your next patches should take this into account and return all host Done Line 29: if (vm.getDedicatedVmForVdsList().contains(host.getId())) { Line 30: return Arrays.asList(host); Line 31: } Line 32: } -- To view, visit https://gerrit.ovirt.org/41962 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I710bd8d3505552a2a8d6060194d94c68c05445db Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dudi Maroshi <d...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Tomer Saban <tsa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
