First pod with affinity can schedule only on nodes with matching topology keys

[ahg-g]

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:
Currently the first pod with required affinity can schedule on any node, this is not correct as no future pod with matching selector/namespace can be placed on that node. This PR fixes this by mandating that a node can accept a first pod only if it has all the topology keys.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

First pod with required affinity terms can schedule only on nodes with matching topology keys.

/assign @Huang-Wei

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/scheduler/OWNERS [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Huang-Wei]

Thanks @ahg-g. This makes the Filter() logic pretty neat!

Some comments below.

[Huang-Wei]

nit: we can evaluate that, in a common case, which one is more expensive: satisfiesExistingPodsAntiAffinity or satisfiesPodsAntiAffinity? and then put the cheaper one at the first place.

[ahg-g]

Non of them are "expensive" per se, but probably satisfiesExistingPodsAntiAffinity will lead to the most iterations since it iterates over all the node's labels, so I moved it to last.

[Huang-Wei]

I'd prefer to refactor the whole function to return early when there is a simple negative path:

func satisfiesPodsAffinity(state *preFilterState, nodeInfo *framework.NodeInfo) bool {
	podsExist := true
	for _, term := range state.podInfo.RequiredAffinityTerms {
		topologyValue, ok := nodeInfo.Node().Labels[term.TopologyKey]
		if !ok {
			return false
		}
		pair := topologyPair{key: term.TopologyKey, value: topologyValue}
		if state.topologyToMatchedAffinityTerms[pair] <= 0 {
			podsExist = false
			break
		}
	}

	if podsExist {
		return true
	}

	// This pod may be the first pod in a series that have affinity to themselves. In order
	// to not leave such pods in pending state forever, we check that if no other pod
	// in the cluster matches the namespace and selector of this pod, the pod matches
	// its own terms, and the node has all the requested topologies, then we allow the pod
	// to pass the affinity check.
	podInfo := state.podInfo
	if len(state.topologyToMatchedAffinityTerms) == 0 && podMatchesAllAffinityTerms(podInfo.Pod, podInfo.RequiredAffinityTerms) {
		return true
	}
	return false
}

[ahg-g]

there should be one or two affinity terms and so exiting early is not really beneficial and makes the code more complicated, I prefer simplicity in this case.

[Huang-Wei]

Or, we can just eliminate the variable toposExist - directly return false in L367, and the logic afterwards would be only checking podsExist.

[ahg-g]

done.

[ahg-g]

/hold
for squash.

[Huang-Wei]

Some nits. Good to squash if you resolve them.

[Huang-Wei]

s/Pods/Pod/ to be consistent with others?

[ahg-g]

this one should stay because we are checking existing pods (multiple pods), in the other two we are checking the incoming pod (single pod).

[Huang-Wei]

Or, we can just eliminate the variable toposExist - directly return false in L367, and the logic afterwards would be only checking podsExist.

[ahg-g]

Thanks @Huang-Wei I squashed.

[Huang-Wei]

/lgtm
/retest

[Huang-Wei]

/hold cancel