Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A single scoring plugin for node resources #98746

Closed
ahg-g opened this issue Feb 4, 2021 · 26 comments
Closed

A single scoring plugin for node resources #98746

ahg-g opened this issue Feb 4, 2021 · 26 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@ahg-g
Copy link
Member

ahg-g commented Feb 4, 2021

We have four score plugins that implement different strategies for preferred resource allocation. Those plugins should not be enabled together, only one.

I propose we deprecate those and combine them under one Score plugin, the same one used for filtering (NodeResourcesFit), and add a FitStrategy parameter to NodeResourcesFit that allows users to select which exact scoring strategy to run.

I also suggest that we go ahead and implement four other strategies: LeastAllocatable, MostAllocatable, BestFit and WorstFit. The first two are basically a graduation of https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/pkg/noderesources, the latter two are common scheduling placement strategies that k8s currently don't support. BestFit for example is important to achieve higher utilization in clusters that have different VM shapes.

In the future, I think we can explore adding FitStrategy to the pod spec, and make it a workload parameter rather than a static scheduler configuration. This can be useful when we have a mix of serving and batch workloads running on the same cluster. Although scheduling profiles may be an alternative for this, albeit a less flexible one.

Previous discussion on this topic: #93547

/sig scheduling

@ahg-g ahg-g added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 4, 2021
@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Feb 4, 2021
@k8s-ci-robot
Copy link
Contributor

@ahg-g: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 4, 2021
@ahg-g
Copy link
Member Author

ahg-g commented Feb 4, 2021

/cc @Huang-Wei @damemi @alculquicondor

@Huang-Wei
Copy link
Member

ref: kubernetes/enhancements#2144

@yuzhiquan
Copy link
Member

It's likely already has an sample plugin in scheduler-plugins/#36, it;s reimplement for that plugin?

@alculquicondor
Copy link
Member

alculquicondor commented Feb 4, 2021

SGTM.

Should we hold v1beta2 component config to next release to account for this?

#95308 is not merged yet.

@ahg-g
Copy link
Member Author

ahg-g commented Feb 4, 2021

Should we hold v1beta2 component config to next release to account for this?

I would hold it, any downsides to delaying it?

@alculquicondor
Copy link
Member

not really, we are not under pressure of API deprecation like REST APIs are

@ahg-g
Copy link
Member Author

ahg-g commented Feb 4, 2021

ref: kubernetes/enhancements#2144

Ah, I missed this one. The above proposal tries to support every single combination, which I think is an overkill.

We can make reasonable assumptions that we bake into each strategy to avoid the over-proliferation of options. For example, LeastAllocatable is always associated with LeastAllocated as a second scoring key (as in nodes with the same least allocatable are differentiated by least allocated)

@alculquicondor
Copy link
Member

Should we have a KEP for this?

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

Yes, I was thinking we could target 1.22

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

I can put a KEP together in an hour, do we have time to review and merge before the deadline tomorrow? I don't think this needs PRR if we focus this cycle on the config API change to support existing plugins through NodeResourcesFit, wdyt?

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

this will help us deprecate the existing plugins in v1beta2 in 1.22 if all goes to plan and the community is happy with the new api.

@alculquicondor
Copy link
Member

Fine with me to do a review last minute, but PRR is a hard requirement now.

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

ok, lets leave it to 1.22 then..

@damemi
Copy link
Contributor

damemi commented Feb 8, 2021

In the future, I think we can explore adding FitStrategy to the pod spec, and make it a workload parameter rather than a static scheduler configuration.

+1 this as a longer-term goal, I like exposing scheduler config options in the pod spec because it gives users better control over their app placement.

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

I opened kubernetes/enhancements#2458

@alculquicondor
Copy link
Member

So that't the placeholder for the 1.22 work, right?

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

I will send a PR in 30min, if it gets reviewed by tomorrow, then good, if not then we will delay to 1.22.

@ahg-g
Copy link
Member Author

ahg-g commented Feb 8, 2021

I sent out kubernetes/enhancements#2461

@Huang-Wei @alculquicondor @damemi timing is too tight, so please feel free to push back... we may not get a PRR too, so...

@wangyanghack
Copy link

wangyanghack commented Apr 27, 2021

sry,I don't understand what is difference in (LeastAllocatable and BestFit) or (MostAllocatable and WorstFit)?
Can you explain this for me?Thanks very much

@yuzhiquan
Copy link
Member

/assign

@ahg-g
Copy link
Member Author

ahg-g commented May 19, 2021

Here is the plan:

  1. Add Score extension point for Fit plugin. This needs to be added to both v1beta1 and v1beta2 (Add score func for NodeResourcesFit plugin #101822)
  2. Add validation in v1beta1 such that Fit Score plugin is not used with LeastAllocated, MostAllocated or RequestedToCapacityRatio.
  3. Deprecate leastAllocated, mostAllocated or requestedToCapacityRatio in v1beta2
  4. One of the following two:
    • Make algorithmProvider.Registry versioned (pass in the component version to NewRegistry). In v1beta2, Fit replaces LeastAllocated score plugin.
    • Remove algorithmProvider flag (it has been marked as deprecated since 1.17), and move the set of default plugins to be set in the CC defaulting logic

@ahg-g
Copy link
Member Author

ahg-g commented May 19, 2021

I created #102151 to remove algorithmProvider pkg.

@chendave
Copy link
Member

chendave commented Jun 3, 2021

/cc

@ahg-g
Copy link
Member Author

ahg-g commented Aug 20, 2021

/close

This is now done.

@k8s-ci-robot
Copy link
Contributor

@ahg-g: Closing this issue.

In response to this:

/close

This is now done.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
Development

No branches or pull requests

8 participants