Add KEP for default Even Pods Spreading

[alculquicondor]

Done: Summary, Motivation, Proposal, Design Details (Alpha Graduation Criteria), Alternatives.

Feature: #1258
Main repo issue: kubernetes/kubernetes#80639

[alculquicondor]

/cc @Huang-Wei @ahg-g

[draveness]

It would be easy to implement this with runtime class scheduling, we could add another field to Scheduling struct like this:

type Scheduling struct {
    // nodeSelector lists labels that must be present on nodes that support this
    // RuntimeClass. Pods using this RuntimeClass can only be scheduled to a
    // node matched by this selector. The RuntimeClass nodeSelector is merged
    // with a pod's existing nodeSelector. Any conflicts will cause the pod to
    // be rejected in admission.
    // +optional
    NodeSelector map[string]string

    // tolerations adds tolerations to pods running with this RuntimeClass.
    // +optional
    Tolerations []corev1.Toleration

    // TopologySpreadConstraints describes how a group of pods are spread
    // If specified, scheduler will enforce the constraints
    // +optional
    TopologySpreadConstraints []TopologySpreadConstraint // new field

}

The pod with specified runtime class would use the topology spread constraints in the Scheduling.

cc/ @tallclair

[alculquicondor]

RuntimeClass solves a totally different problem. And it doesn't solve the portability problem. The workload authors now need to be aware of what the runtime classes are.

[Huang-Wei]

Thanks @alculquicondor ! LGTM generally, some comments below.

[Huang-Wei]

I think we should reword this section to highlight that EvenPodsSpreading enables users to control pod spreading on a PodSpec basis, while this feature will bring a cluster-level configurable pod spreading policy, and then mention a bit on the differences.

[alculquicondor]

Reworded the 2 paragraphs.

[Huang-Wei]

"nodes and zones", or just "zones"?

[alculquicondor]

both. It gives a weight of 1/3 to nodes and 2/3 to zones.

[ahg-g]

I think we want to present this KEP as the replacement for the SelectorSpreadingPriority, they shouldn't exist together. To make the transition frictionless, we can set a default value for what we are proposing here such that the scheduler's default behavior isn't changed (i.e., we set a default configuration for "default even pods spreading" to spread across zones).

[alculquicondor]

This is what this paragraph is trying to say.

[ahg-g]

What I meant is that we shouldn't allow configuring both at the same time at all. The paragraph is still talking about the possibility of that being the case.

Currently, SelectorSpreadingPriority is used as one of the priority functions when no Policy is specified, and it is a value that we accept in the Policy file. Those two aspects are our guarantees to the users. We can solve them by doing the following if this feature is enabled:

1. SelectorSpreadingPriority is removed from the default set of priorities. The default configuration for cluster-wide EvenPodsSpreading is set such that we maintain the same current scheduler default behavior that SelectorSpreadingPriority offers (or as close as possible).

2. During the deprecation period of Policy (and by association SelectorSpreadingPriority), we say when set, SelectorSpreadingPriority overrides cluster-wide EvenPodsSpreading.

WDYT?

[alculquicondor]

I agree that they are conflicting. See updated section.

[ahg-g]

I would drop the prefix Default, I think it is not necessary. It will be confusing when we start talking about a default value for DefaultTopologySpreadConstraints.

[alculquicondor]

I agree that it's confusing. But we need to convey the idea that it's still a cluster default that is applied if the pods don't specify any rules.

[ahg-g]

Component config is a cluster-wide configuration by definition, adding a "Default" prefix does not really clarify the semantics of this configuration variable as you state it, at the end of the day it has to be explained in the docs/comments that this value can be overridden at the pod level.

[alculquicondor]

See updated name and comment.

[ahg-g]

As current TopologySpreadConstraint.LabelSelector is optional, you can reuse that structure. (validation logic can help detect illegal presence of labelSelector)

@Huang-Wei What happens currently in EvenPodsSpreading when the label selector is not specified?

[ahg-g]

What I meant is that we shouldn't allow configuring both at the same time at all. The paragraph is still talking about the possibility of that being the case.

Currently, SelectorSpreadingPriority is used as one of the priority functions when no Policy is specified, and it is a value that we accept in the Policy file. Those two aspects are our guarantees to the users. We can solve them by doing the following if this feature is enabled:

1. SelectorSpreadingPriority is removed from the default set of priorities. The default configuration for cluster-wide EvenPodsSpreading is set such that we maintain the same current scheduler default behavior that SelectorSpreadingPriority offers (or as close as possible).

2. During the deprecation period of Policy (and by association SelectorSpreadingPriority), we say when set, SelectorSpreadingPriority overrides cluster-wide EvenPodsSpreading.

WDYT?

[tallclair]

I agree with @alculquicondor, the RuntimeClass scheduling constraints are only intended to address compatibility.

A more relevant proposal would the SchedulingPolicy, which unfortunately hasn't gotten off the ground yet.
/cc @yastij

[alculquicondor]

I updated the PR with the provided feedback.

However, I'm taking a look at the progress of SchedulingPolicy. If there a no major blockers, I might merge the efforts.

[ahg-g]

However, I'm taking a look at the progress of SchedulingPolicy. If there a no major blockers, I might merge the efforts.

+1 to considering SchedulingPolicy again, it could allow us to deprecate most scheduler's "custom predicates/priorities".

[draveness]

+1 to considering SchedulingPolicy again, it could allow us to deprecate most scheduler's "custom predicates/priorities".

I talked to Yassine and took the job, and I'll send the KEP soon.

[ahg-g]

+1 to considering SchedulingPolicy again, it could allow us to deprecate most scheduler's "custom predicates/priorities".

I talked to Yassine and took the job, and I'll send the KEP soon.

note that I miss understood the current proposal for SchedulingPolicy, in its current form it will not help with deprecating custom predicates/priorities.

In any case, we discussed this during the sig meeting today and Aldo will be following up with few people who initially proposed it to understand if this is still worth pursuing especially that what SchedulingPolicy is providing can be implemented as a user admission webhook.

[alculquicondor]

After looking at what the Scheduling Policy proposes and the older discussions, I suggest that we proceed with this KEP as is.

The rational being:

• Scheduler Policy focuses on admission, not necessarily setting defaults.
• An API that would give us such flexibility for setting defaults is probably a better candidate for a separate mutating webhook, rather than the core scheduler.

I updated Goals and Alternatives sections to reflect this suggestion.

[ahg-g]

I don't think we should run both priorities, I think we should clarify that if both are specified, then one overrides the other.

[alculquicondor]

So your take would be that EvenPodsSpreadPriority overrides and disables SelectorSpreadingPriority?

[ahg-g]

We should have a single implementation, a plugin. This plugin will either be configured using the priority-to-plugin translation logic we have if SelectorSpreadingPriority is specified, otherwise it is configured using the new ComponentConfig parameters you will be adding.

[alculquicondor]

See updated clarifications

[ahg-g]

the constraint should be k8s default always, never provided in this case because SelectorSpreading takes precedence.

[ahg-g]

or you can consider specifying both (topologySpreadConstraint and SelectorSpreading) an invalid configuration, which I think is better since we don't have to make a call which one takes precedence.

[alculquicondor]

I went with the second option

[ahg-g]

we should say that this feature is tired to even pods spread, so they will graduate together.

[ahg-g]

/lgtm

This is great, thanks Aldo for the detailed explanation of how this will work. I will approve one @Huang-Wei LGTMs

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [ahg-g]

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

[ahg-g]

/hold

[ahg-g]

sorry for the premature merge :(

Aldo, can you please clarify the graduation dependency between this and EvenPodsSpreading in a followup PR?

[alculquicondor]

Sure thing. There is a new PR in place.

[wojtek-t]

Sorry for jumping late into this discussion - but do we believe we can do that efficiently?
I'm asking in the context of large clusters - i.e. 5k nodes.

I want to ensure that we won't introduce a visible regression, because this may become a blocker.

[ahg-g]

I think we should be able to do this as efficiently as the current logic, I don't see why we can't. I agree that it is a blocker if this will be causing performance regressions though.

[alculquicondor]

SelectorSpreadingPriority is currently part of the default algorithms that run. Theoretically, we are talking about the same problem, and then it should be possible to devise an algorithm for EvenPodsSpreadPriority with the same complexity.
We will be backing it with performance measurements, as seen in the Test Plan.

[ahg-g]

@alculquicondor perhaps we should explicitly call that out in the KEP?

[alculquicondor]

The Test Plan is part of the graduation criteria.