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

Added KEP for Node Resource Score Strategy #2461

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

ahg-g
Copy link
Member

@ahg-g ahg-g commented Feb 8, 2021

#2458

/sig scheduling

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Feb 8, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 8, 2021
@ahg-g ahg-g force-pushed the ahg-fit-strategy branch 2 times, most recently from cd991b3 to dc8d69a Compare February 8, 2021 20:02
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
- `NodeResourcesLeastAllocated`
- `NodeResourcesMostAllocated`
- `RequestedToCapacityRatio`
- `NodeResourcesBalancedAllocation`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the default profile, this plugin is enabled along with NodeResourcesLeastAllocated. It's optimization objective is complementary to the other plugins, it should remain separate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am hoping we can express everything using the plugin config, the idea is to have a path to mirror this into the pod spec. I guess we need to answer the following questions:

  1. Is it useful to use balanced allocation on its own
  2. What other plugins benefit from having balanced allocation enabled?
  3. For plugins identified in (2), is it always better to have balanced allocation enabled?

answering those questions will help us understand if its scoring behavior can be expressed in the unified configuration (e.g., either bake it into existing strategies always or as a flag that can be enabled on demand with other strategies)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the balanced strategy for now, we can come back to it later if we find a good API to integrate it.

keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
keps/sig-scheduling/2458-preferred-fit-strategy/README.md Outdated Show resolved Hide resolved
@ahg-g ahg-g changed the title Added KEP for Preferred Fit Strategy Added KEP for Node Resource Score Strategy Feb 9, 2021
@ahg-g ahg-g force-pushed the ahg-fit-strategy branch 2 times, most recently from c5f00cc to 0eb1920 Compare February 9, 2021 14:16
@alculquicondor
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
@ahg-g
Copy link
Member Author

ahg-g commented Feb 9, 2021

Thanks, squashed for now.

@alculquicondor
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021

## Design Details

Define a new `ScoringStrategy` type as follows:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better as ResourceScoringStrategy? Or is this planned to be used for other scoring plugins besides resource allocation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be, lets leave that to the api review.

allows preferring nodes with the least/most amount of available absolute resources that can host the
pod; also, Least/MostAllocatable which allows preferring nodes with the least/most amount of
allocatable resources that can host the pod. Adding those strategies as separate plugins will
make the scheduler configuration problem even worse.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this plugin flexible to support internal combination of sub-fit strategy? Like a user may want to combine 2 strategies: least (absolute) available resource plus most allocated resource (ratio).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For new ones, we need to understand if they should be baked into existing scoring strategies, or add knobs to enable them on demand if they are cross cutting (or have keep them in separate plugin).

@Huang-Wei
Copy link
Member

/approve

@ahg-g
Copy link
Member Author

ahg-g commented Feb 9, 2021

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 9, 2021
@wojtek-t
Copy link
Member

wojtek-t commented Feb 9, 2021

/approve PRR

Please squash commits.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Huang-Wei, wojtek-t

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021
@ahg-g
Copy link
Member Author

ahg-g commented Feb 9, 2021

/milestone v1.21

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 9, 2021

### Test Plan

- Unit and integration tests covering the new configuration path.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ahg-g, would you be able to expand this a bit more? A little more detail here would really help. You can take a look at the comment of what test plan should consider in the KEP template as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is nothing particularly challenging to test here, we are proposing a new config parameter to an existing logic, so this doesn't really include complex interactions; I added some clarification.

@Huang-Wei
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
@ahg-g
Copy link
Member Author

ahg-g commented Feb 9, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2021
Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 83a1b1f into kubernetes:master Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants