KEP of even pods spreading

[Huang-Wei]

As a follow-up of discussion kubernetes/kubernetes#72479 (comment)

Tracking issue: #895

/sig scheduling
/kind design
/priority important-longterm

[Huang-Wei]

cc/ @wojtek-t @bsalamat @k82cn @ravisantoshgudimetla @misterikkit @resouer @wgliang

[Fei-Guo]

"Even spreading is a predicate (hard requirement) instead of a priority"
For feature like this, a typical concern is what if spreading causes App performance issue. i.e., Do you want to place Pod to a high load node for the sake of spreading? Difference people may have difference option, but a hard rule seems to be very restrictive here. In practice, I feel AZ level spreading can be a hard rule but within a AZ, a soft rule may be more suitable for spreading.

[Huang-Wei]

@Fei-Guo I agree that hard requirement (predicate) could probably place a pod onto a high load node. So the "maxSkew" option is brought up - setting it to a reasonable value instead of 1 (default value, which can be thought of "double hard") can give you higher odds to pick up a low load node. It will firstly tolerate more candidate nodes in predicate phase, and then proceed to rely on priority strategies to choose which node is best fit.

[Huang-Wei]

A new commit is pushed which contains details of Design B. PTAL.

[bsalamat]

Thanks, @Huang-Wei. I think Design B makes more sense. I have a few minor comments.

[bsalamat]

Not sure if Wojtek will have the time to review this, but please add @MaciekPytel to ensure that this will not cause any issues for the autoscaler.

[bsalamat]

IIUC, the "Preferred" version will be the same as our current preferred inter-pod anti-affinity. Existence of this item is only justified if we plan to remove preferred anti-affinity.

[Huang-Wei]

the "Preferred" version will be the same as our current preferred inter-pod anti-affinity

It's more similar as podAffinity, for both "required" and "preferred" versions. It's just that additionally we need to implement MaxSkew.

The whole Design B works independently with any other predicate/priority.

[Huang-Wei]

Oh wait, I missed a top level TopologyKey along with MaxSkew. Without it, we can't identify which specific topology we're gonna place the matching pods evenly. (b/c we support []PodAffinityTerms, and each term can has different topology key)

[Huang-Wei]

BTW: the top level TopologyKey can be renamed to DistributedBy (or DistributionKey), so as not to cause confusion with TopologyKey which works inside each affinityTerm.

[bsalamat]

IMO we should remove PodAffinityTerm and WeightedPodAffinityTerm and add a NodeSelector. NodeSelector allows users to specify where pods should be scheduled and what subset of nodes even spreading is applied. For example, a user should be able to specify that their pods should be scheduled in 3 out of 4 available zones and pods should be spread evenly among those 3 zones.
We don't need to specify pod affinity here at all. It complicates the design and I think the API becomes hard to use. Besides, if pod affinity is needed, users can add inter-pod affinity rules to their pod spec. Instead, we should add a pod selector which is a label selector and a namespace only. Unlike PodAffinityTerm, this one does not have a topology key.

[krmayankk]

omg, i think i just now understood the presence of two different topology keys, one in the podAffinityTerms and one outside. I think its getting too complicated to use. I think we need to think harder here.

[bsalamat]

@Huang-Wei Sorry if I was not clear. I think the API should specify all the following pieces of information:

1. Which pods should be spread (needs a pod selector).
2. What topology pods should be spread to (e.g., node, zone, region, etc.)
3. What set of nodes should be considered. This is needed to filter out some nodes. For example, topology may be "zone", but a user may want to filter out one of the zones.
4. MaxSkew as described in the KEP.
5. Whether this is preferred or required.

This is what I have in mind:

type PodSelectorTerm struct {
	// A label query over a set of resources, in this case pods.
	// +optional
	LabelSelector *metav1.LabelSelector
	// namespaces specifies which namespaces the labelSelector applies to (matches against);
	// null or empty list means "this pod's namespace"
	// +optional
	Namespaces []string
}

type EvenSpreading struct {
    MaxSkew        int32
    TopologyKey    string
    PodSelector    []PodSelectorTerm
    Required       bool  // vs. preffered
    // +optional
    NodeSelector   v1.NodeSelector
}

[bsalamat]

After talking with @Huang-Wei offline, I think we can drop the NodeSelector from EvenSpreading and let the predicate process an external NodeAffinity if there is one in the pod spec.

[krmayankk]

@bsalamat do you mean the NodeAffinity specified in Affinity *Affinity ?

[bsalamat]

@krmayankk Yes. NodeAffinity and NodeSelector are two other predicates that the scheduler has. This feature will take those into account.

[Huang-Wei]

/cc @wojtek-t for reviewing.

[krmayankk]

why is this not a bool like required ?

[bsalamat]

+1 to Mayank's question. Do you think you want to support other choices in the future? Do you have any examples in mind?

[bsalamat]

+1 to Mayank's question. Do you think you want to support other choices in the future? Do you have any examples in mind?

[Huang-Wei]

Right now, nope :)

(I was thinking of a third option to respect NodeSelector/NodeAffinity, but it turns out to be more a reasonable implicit assumption)

@lavalamp are you OK with changing it back to bool: ScheduleWhenUnsatisfiable bool.

[lavalamp]

I think things like this are more understandable if the consequences of the action are in the enum value rather than true/false.

E.g., as an outsider, reading "ScheduleWhenUnsatisifiable" I have a lot of questions, like, "does that mean it doesn't schedule unless it's unsatisfiable?"

And I do think that you might need an additional parameter in the future, which is how strong the preference is.

[Huang-Wei]

And I do think that you might need an additional parameter in the future, which is how strong the preference is.

That reminds me of a possible scenario called "FallBackToAvailableNodes". In between "ScheduleAnyway" and "DoNotSchedule", maybe it's possible to compute the internal ActualySkew and MinimalSkew among the nodes which passed previous Predicates. This is related with earlier discussion with @bsalamat.

This is just an immature thought though, we definitely won't consider it in initial implementation.

[bsalamat]

I agree with the readability part.
Regarding another parameter to indicate the strength of preference, we already have a weight parameter for all preference (priority) functions.

[lavalamp]

For this particular case, I feel fairly strongly that the strings in the current draft communicate the purpose and implications of the decision much more clearly than the bool.

[Huang-Wei]

/hold
before squashing commits.

[lavalamp]

@Huang-Wei

Bobby suggested earlier to focus on the KEP itself... As the design is a standalone predicate/priority, users can do whatever kind of combination as they wish.

I can appreciate that, but I do think part of the point of a KEP is to show how a feature fits into Kubernetes holistically. Showing how it is complimentary to existing features, or how the use cases are redistributed between features seems very relevant to that end.

Basically I think you answered the question, it might be nice to say what you wrote in the KEP somewhere so future readers understand more exactly what is changing.

[lavalamp]

This is a little too broad of a name. "UnsatisfiableConstraintResponse"?

[Huang-Wei]

Done.

[Huang-Wei]

I can appreciate that, but I do think part of the point of a KEP is to show how a feature fits into Kubernetes holistically. Showing how it is complimentary to existing features, or how the use cases are redistributed between features seems very relevant to that end.

That makes sense. I will add a paragraph addressing this.

Updated: done.

[lavalamp]

I still don't see a user story or something explaining why you would ever want a different namespace than the pod's own namespace?

[Huang-Wei]

I used to put it as a derivation from PodAffinity. I agree we can remove it and default to "this pod's namespace".

cc @bsalamat in case Bobby happen to know some histories or particular user story.

[lavalamp]

As long as you go for option 2, I think this is good enough from an API review perspective. I'll likely nitpick the exact comment phrasing more when the actual API change PR happens. :)

[bsalamat]

Bobby suggested earlier to focus on the KEP itself... As the design is a standalone predicate/priority, users can do whatever kind of combination as they wish.

To be clear, I said we should postpone making the final decision on anti-affinity after we analyzed our code-base and after we have even pod spreading, but at this point we know that we will either limit the topology of anti-affinity to node only or make "anti-affinity on node" a fast path and any other topology in anti-affinity a slow path. I think we need to point this out in the KEP so that readers know that inter-pod anti-affinity in any topology other than node will either be dropped or will cause scheduling slow down.

[bsalamat]

/lgtm
/approve

Thanks, @Huang-Wei!
Obviously, we may change some names of types when you send the actual PRs to implement the API. When those names are finalized we can come back and update this KEP.

[Huang-Wei]

Commits have been squashed.

Thanks all!

Obviously, we may change some names of types when you send the actual PRs to implement the API. When those names are finalized we can come back and update this KEP.

@bsalamat Absolutely. PS: need another /lgtm.

[Huang-Wei]

/hold cancel

[wgliang]

/lgtm
Thanks, @Huang-Wei

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, Huang-Wei, wgliang

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:

• keps/sig-scheduling/OWNERS [bsalamat]

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