Moti Asayag has posted comments on this change. Change subject: engine: scheduling host- find free VF ......................................................................
Patch Set 6: (8 comments) https://gerrit.ovirt.org/#/c/37931/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java: Line 310: releaseHostDevicesLock(); Line 311: } Line 312: Line 313: private void cleanupPassthroughVnics() { Line 314: HostNicVfsConfigHelper hostNicVfsConfigHelper = Injector.get(HostNicVfsConfigHelper.class); why do you need to explicitly inject the instance like that ? If you're in the command context, you can use @Inject. Line 315: hostNicVfsConfigHelper.setVmIdOnVfs(getVdsId(), null, new HashSet<String>(passthroughVnicToVfMap.values())); Line 316: } Line 317: Line 318: @Override https://gerrit.ovirt.org/#/c/37931/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/PolicyUnitImpl.java: Line 120: } Line 121: Line 122: private final PolicyUnit policyUnit; Line 123: protected VdsFreeMemoryChecker memoryChecker; Line 124: protected Map<Guid, Map<Guid, String>> hostToVnicToVfMap; is it relevant for any of the implementations other than the NetworkPolicyUnit ? Can't it be located on that class ? Line 125: Line 126: public PolicyUnitImpl(PolicyUnit policyUnit) { Line 127: this.policyUnit = policyUnit; Line 128: } https://gerrit.ovirt.org/#/c/37931/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/NetworkPolicyUnit.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/scheduling/policyunits/NetworkPolicyUnit.java: Line 94: /** Line 95: * Determine whether all required Networks are attached to the Host's Nics. A required Network, depending on Line 96: * ConfigValue.OnlyRequiredNetworksMandatoryForVdsSelection, is defined as: 1. false: any network that is defined on Line 97: * an Active vNic of the VM or the cluster's display network. 2. true: a Cluster-Required Network that is defined on Line 98: * an Active vNic of the VM. please document the new requirement, i.e. "Vnic configured for passthrough will skip the network existence check for the host and will be validated later by {@code validatePassthroughVnics(...)}" or something like that, since this class became more and more complex to understand each bit of it. Line 99: * Line 100: * @param vds Line 101: * the Host Line 102: * @param vm Line 123: boolean onlyRequiredNetworks = Line 124: Config.<Boolean> getValue(ConfigValues.OnlyRequiredNetworksMandatoryForVdsSelection); Line 125: for (final VmNetworkInterface vmIf : vmNICs) { Line 126: boolean found = false; Line 127: boolean isVnicPassthrough = vmIf.isPassthrough(); you can inline the call for vmIf.isPassthrough() or set the actual meaning of this field which basically says: skipNetworkExistenceCheckForVnicPassthrough Line 128: Line 129: if (vmIf.getNetworkName() == null) { Line 130: found = true; Line 131: } else { Line 222: List<HostNicVfsConfig> vfsConfigs = Line 223: getHostNicVfsConfigHelper().getHostNicVfsConfigsWithNumVfsDataByHostId(host.getId()); Line 224: Line 225: Map<Guid, String> vnicToVfMap = null; Line 226: if (hostToVnicToVfMap != null) { what if this wasn't set and it is null ? is there a point to continue the check ? Line 227: vnicToVfMap = new HashMap<>(); Line 228: hostToVnicToVfMap.put(host.getId(), vnicToVfMap); Line 229: } Line 230: Line 231: for (final VmNetworkInterface vnic : vnics) { Line 232: boolean found = false; Line 233: if (vnic.isPassthrough()) { Line 234: Network vnicNetwork = Line 235: vnic.getNetworkName() == null ? null : getNetworkDAO().getByName(vnic.getNetworkName()); if vnic.getNetworkName() is null, we can skip the rest of the loop. Line 236: for (HostNicVfsConfig vfsConfig : vfsConfigs) { Line 237: if (vfsConfig.getNumOfVfs() != 0 && isNetworkInVfsConfig(vnicNetwork, vfsConfig)) { Line 238: Line 239: HostDevice freeVf = getFreeVf(vfsConfig); Line 242: found = true; Line 243: Line 244: if (vnicToVfMap != null) { Line 245: vnicToVfMap.put(vnic.getId(), freeVf.getDeviceName()); Line 246: } http://memegenerator.net/instance/61068269 ;-) 6 levels is too much Line 247: } Line 248: } Line 249: } Line 250: } else { Line 279: Line 280: return isNetworkInConfig || isLabelInConfig; Line 281: } Line 282: Line 283: private HostDevice getFreeVf(HostNicVfsConfig hostNicVfsConfig) { 1. so for host with 2 nics, marked as "allowedAllNetworks", in order to run a vm with 2 vnics, we'll have at worse case to fetch the host nics twice for each host (total of 4). not sure about the 'performance' impact, but the number of queries for the same entity could be reduced. 2. How would it fit if there is only 1 free vf and 2 vnics ? the condition will succeed for each vnic, but overall we'd fail to run the vm, isn't ? Line 284: VdsNetworkInterface nic = getInterfaceDAO().get(hostNicVfsConfig.getNicId()); Line 285: return getHostNicVfsConfigHelper().getFreeVf(nic); Line 286: } Line 287: -- To view, visit https://gerrit.ovirt.org/37931 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I58c7aaa7a5a7160132418c397840583209aa8371 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
