removepodsviolatingtopologyspreadconstraint: implement explicit constraints

[a7i]

closes #1123 and #1138

1. Currently we cannot have a "just ScheduleAnyway" definition. This PR allows users to configure them separately. In particular, my use-case is not do a nodeFit when balancing domains for DoNotSchedule in order to force a scale-up with Cluster Autoscaler. However, for ScheduleAnyway, I want nodeFit, because otherwise Scheduler will put it back on the same node it was on.
2. I added a new arg called nodeFit. I admit, it's a bit confusing given the Default Evictor's nodeFit. I mainly wanted to preserve existing behavior, hence why it's true by default.

[knelasevero]

Maybe we should call this internal nodeFit something different? Like we did for EvictableNamespaces in the nodeutilization stratedies?

[a7i]

Maybe we should call this internal nodeFit something different? Like we did for EvictableNamespaces in the nodeutilization stratedies?

@knelasevero Yes, I like that idea. Would topologyBalanceNodeFit be ok?

[knelasevero]

Maybe we should call this internal nodeFit something different? Like we did for EvictableNamespaces in the nodeutilization stratedies?

@knelasevero Yes, I like that idea. Would topologyBalanceNodeFit be ok?

Sounds good to me. @damemi @ingvagabund , WDYT?

[ingvagabund]

Sounds good to me. @damemi @ingvagabund , WDYT?

Making it configurable is reasonable. The semantics of nodeFit before eviction and right before eviction is not clear here. The original intent of the nodeFit here was to evict pods that can be scheduled to a different node to avoid evicting a pod that lands back on the same node. Thus, helping to improve the skew. The case where a new node can be provisioned as a reaction to eviction was not considered during the implementation. For that, we should not always assume a node pool is static.

Would topologyBalanceNodeFit be ok?

Both RemovePodsViolatingTopologySpreadConstraint and DefaultEvictor invoke the same PodFitsAnyOtherNode function. Nevertheless, that can change in the future as one can enable a different implementation of PreEvictionFilter extension point once the descheduling framework is offcially out. The conditions for when a pod is not suitable for evicting wrt. TopologySpreadConstraint can diverge from what it means for the default evictor to fail to fit another node. E.g. a pod can be evicted to re-balance when an autoscaler is enabled (RemovePodsViolatingTopologySpreadConstraint). Vs. a pod can not be evicted since there's a company policy to reduce eviction between 10pm and 6am (custom implementation of the DefaultEvictor plugin).

We can probably completely omit the nodeFit keyword part. On the other hand when we expose the possibility to ignore nodeFit conditions, do we know which specific condition is actually causing limitation? Based on #1138 there's no existing node because:

• pods does not match a node selector?
• taints are not tolerated?
• insufficient resources?
• no schedulable node?

Q is whether we wanna expose this level of detail. Or create groups of conditions that can be enabled/disabled as a whole unit. Providing an argument to completely disable NodeFit seems reasonable. On the other hand what if we get a request to disable/enable a specific condition mentioned in the list? One might want to test for pods does not match a node selector and taints are not tolerated and ignore insufficient resources so the autoscaler can actually kick in only when insufficient resources scenario is on instead of evicting a pod and poke autoscaler just because a taint is not tolerated (for a good reason when it's preferable to pick a different pod for eviction which e.g. still can fit into one of the existing nodes).

[ingvagabund]

@a7i also better to split this PR into two. One commit for each change (explicit constraints, new nodeFit alias argument). E.g. in case we need to revert.

[a7i]

@ingvagabund thanks for your review.

The semantics of nodeFit before eviction and right before eviction is not clear here. The original intent of the nodeFit here was to evict pods that can be scheduled to a different node to avoid evicting a pod that lands back on the same node.

The intention of nodeFit when balancing domains is to calculate the spread/skew given the "fit" constraints. This is particularly helpful for a best-effort constraint such as "ScheduleAnyway".

Both RemovePodsViolatingTopologySpreadConstraint and DefaultEvictor invoke the same PodFitsAnyOtherNode function

RemovePodsViolatingTopologySpreadConstraint only considers nodesBelowAverage for NodeFit because when balancing domains, it should only consider nodes that can help the skew. Default Evictor considers all available nodes.

Q is whether we wanna expose this level of detail.

I had created an Issue for NodeFit Specs to be able to provide that level of detail. We can move the discussion there: #1149

also better to split this PR into two.

Generally, I would agree. However, for my particular use-case, it only helps if both args are available. If it's rolled back, then might as well roll it all back because just one wouldn't satisfy my use cases.

As described in the PR, my use-cases are:

• I need to split ScheduleAnyway and DoNotSchedule as we require different behavior for balancing them out.

• I need a "Just ScheduleAnyway" that takes nodeFit into account. For this, if nodeFit is not taken into account when balancing domains, then Scheduler will just put it back on the same node.

• I need a just "DoNotSchedule". While this is already possible, nodeFit when balancing domains is not configurable. Hence why I need the nodeFit arg for when balancing domains. i.e. do not perform a nodeFit to force Cluster-Autoscaler to do a scale-up.

[knelasevero]

I think we can have a single PR, however, with 2 commits. Not saying we would revert a single commit, and leave it at that. We might revert a single one and still pursue this use case in a different approach taking advantage of the first commit. Or revert both. Still makes sense to separate it.

Would #1149 discussion make this PR obsolete? Maybe we should have a proper design proposal for all our nodeFit "problems"? There are a few issues on that topic

[a7i]

@ingvagabund @knelasevero Thank you for your feedback ❤️

• Split to two commits
• Changed nodeFit to topologyBalanceNodeFit
• Removed includeSoftConstraints from v1alpha2 api

Look forward to your feedback

[ingvagabund]

How about?

RemovePodsViolatingTopologySpreadConstraintArgs:
- TopologyBalanceNodeFit: [] // empty field -> fall back to defaults which is a subject to change
- TopologyBalanceNodeFit: [""] // empty string -> disable the node fit checks
- TopologyBalanceNodeFit: ["Cond1", ..., "CondN"] // arbitrary list of enabled conditions (implemented later)

type NodeFitConditions string

var (
	NoCondition NodeFitConditions = ""
)

type RemovePodsViolatingTopologySpreadConstraintArgs struct {
	...
	TopologyBalanceNodeFit []NodeFitConditions
}

Alternatively, "" can be replaced with "NoCheck" or similar string which if specified will be the only valid item in the list.

[a7i]

I was under the impression that we will handle NodeFit Spec in a subsequent design/PR. But given that, perhaps we should design/implement that first.

[ingvagabund]

This is more like a middle step so once the NodeFit is refined the API of TopologyBalanceNodeFit does not need to change. It will "just" get extended with additional enums.

[ingvagabund]

As discussed on the community meeting, let's keep the new TopologyBalanceNodeFit argument for v1alpha2 and deprecate it in v1alpha3.
/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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:

• OWNERS [ingvagabund]

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

[a7i]

/hold

[MarcPow]

@a7i @damemi - Why is this change being held? My teams also have the auto-scaler scenario in #1138. I'd dearly love to get a fix in place for this.

[a7i]

@a7i @damemi - Why is this change being held? My teams also have the auto-scaler scenario in #1138. I'd dearly love to get a fix in place for this.

My apologies, let me address the casing issue real quick:

	TopologyBalanceNodeFit *bool                 `json:"TopologyBalanceNodeFit"`

to:

	TopologyBalanceNodeFit *bool                 `json:"topologyBalanceNodeFit"`

[a7i]

fyi, we just tested this in our cluster, and this is the config that worked for us:

  profiles:
    - name: fit # nodefit enabled plugins
      pluginConfig:
        - name: DefaultEvictor
          args:
            nodeFit: true
        - name: RemovePodsViolatingTopologySpreadConstraint
          args:
            constraints:
            - ScheduleAnyway
      plugins:
        balance:
          enabled:
            - RemovePodsViolatingTopologySpreadConstraint

    - name: nofit # plugins without nodefit (e.g. force scale-up with cluster-autoscaler)
      pluginConfig:
        - name: "DefaultEvictor"
          args:
            nodeFit: false
        - name: RemovePodsViolatingTopologySpreadConstraint
          args:
            constraints:
            - DoNotSchedule
            topologyBalanceNodeFit: false
      plugins:
        balance:
          enabled:
            - RemovePodsViolatingTopologySpreadConstraint

[ingvagabund]

@a7i file conflict :(

[ingvagabund]

/lgtm

[a7i]

/remove-hold