Alona Kaplan has posted comments on this change. Change subject: engine: scheduling host- find free VF ......................................................................
Patch Set 6: (11 comments) https://gerrit.ovirt.org/#/c/37931/6//COMMIT_MSG Commit Message: Line 8: Line 9: A VF is marked as suitable for a passthrough vnic in case: Line 10: 1. It is free (reported by vdsCaps, not attached to vm, doesn't have vlan Line 11: device or network attached to it). Line 12: 2. It's PF has the vnic's network (or label) in it's vf's configuration. > s/It's/Its Done Line 13: Line 14: Change-Id: I58c7aaa7a5a7160132418c397840583209aa8371 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 111: Line 112: private Guid cachedActiveIsoDomainId; Line 113: private boolean needsHostDevices = false; Line 114: Line 115: private Map<Guid, String> passthroughVnicToVfMap = new HashMap<>(); > Can we encapsulate this thing in hostNicVfsConfigHelper instead of having i hostNicVfsConfigHelper is a utility class, it doesn't have state. Line 116: Line 117: public static final String ISO_PREFIX = "iso://"; Line 118: public static final String STATELESS_SNAPSHOT_DESCRIPTION = "stateless snapshot"; Line 119: Line 252: // re-throw it. otherwise, continue (the vm will be down and a re-run will be triggered) Line 253: switch (e.getErrorCode()) { Line 254: case Done: // should never get here with errorCode = 'Done' though Line 255: case exist: Line 256: cleanupPassthroughVnics(); > +1 1. I can't remove the cleanup from line- 256, since it doesn't invokes runningFailed. 2. I moved the cleanup from line 261 to runningFailed. The reasons for doing it are- 1. Although runningFailed invokes ProcessDownVmCommand (ProcessDownVmCommand is cleaning the passthrough vnics (https://gerrit.ovirt.org/#/c/38624/3)) I prefer to do the cleanup here, to avoid calling redundant VDSCommandType.CollectVdsNetworkData by ProcessDownVmCommand. 2. there are 4 use cases for invoking running failed. For now, just one is relevant. But it is more secure to do the cleanup in runningFailed. Line 257: reportCompleted(); Line 258: throw e; Line 259: case VDS_NETWORK_ERROR: // probably wrong xml format sent. Line 260: case PROVIDER_FAILURE: 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 ? Done 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 NetworkPolicyU I need it here because the SchedulingManager (see the next file in the patch- line 530) sets it while iterating over all the PolicyUnitImpls. The alternative is using instanceOf to determine if the PolicyUnitImpl is NetworkPolicyUnit. 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 Done 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 Done 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 c Yes, it will be null when it will be executed by the canDoAction of the runVmCommand. We don't need the map (because we don't need to update the db), but the check is still needed. 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. If the vnic is passthrough the network cannot be null. I can add the if->continue, but it is not necessary. 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 ;-) Done- now it has "just" 3 levels. Also found out a bug- I didn't take into account that more than one vnic can be 'passthrough', and the same vf can be scheduled to all/some of them. 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 1. Added caching for the host nics. 2. You're right! Fixed it. 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: 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
