Roy Golan has posted comments on this change. Change subject: core: enable pinning to multiple hosts ......................................................................
Patch Set 8: (12 comments) partial 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 1034: maroshi no names in comment lets discuss with mbetak that subjects and see where is this going 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 83: getDedicatedVmForVdsList() if we went for a rename of that getter, lets just call it some other sane name getHosts() seems perfectly fine as it depends on the migrationPolicy if its preffered or pinned. 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 274: new LinkedList<Guid> why LinkedList and not ArrayList? 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 116: List<Guid> destinationHostGuidList = new LinkedList<>(); : if (getDestinationVdsId() != null){ : destinationHostGuidList.add(getDestinationVdsId()); : } that code is duplicated in line 440. why can't it be a part of the old getDestinationVdsId? 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 was changed 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.validateDedicatedVdsExistOnSameCluster 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 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 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 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 73: ); off topic but not completly - we should fetch that config value with version - Config.<Boolean> getValue(SupportNUMAMigration, vm.getCompatiblityVersion()) 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 host in the list, otherwise we can't determine how to use the UI of numa pinning 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 hosts which match 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: 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
