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

Implement a priority function that considers pod's resource limits #55906

Merged
merged 1 commit into from Nov 29, 2017

Conversation

@aveshagarwal
Copy link
Member

aveshagarwal commented Nov 16, 2017

This PR implement a new priority function ResourceLimitsPriorityMap (disabled by default and behind alpha feature gate and not part of the scheduler's default priority functions list) that assigns a lowest possible score of 1 to a node that satisfies one or both of input pod's cpu and memory limits, mainly to break ties between nodes with same scores.

@kubernetes/sig-scheduling-pr-reviews @sjenning @derekwaynecarr

Release note:

A new priority function `ResourceLimitsPriorityMap` (disabled by default and behind alpha feature gate and not part of the scheduler's default priority functions list) that assigns a lowest possible score of 1 to a node that satisfies one or both of input pod's cpu and memory limits, mainly to break ties between nodes with same scores.
@guangxuli

This comment has been minimized.

Copy link
Contributor

guangxuli commented Nov 17, 2017

cc

// its cpu and memory limits both, the node score is not affected. If one or both of cpu and memory limits
// of the pod are satisfied, the node is assigned a score of 1.
// Rationale of choosing the lowest score of 1 is that this is mainly selected to break ties between nodes that have
// same scores assigned by one of least and most requested priority functions.

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Nov 20, 2017

Contributor

Shouldn't we give a better score to node which fits both CPU and memory?

This comment has been minimized.

@aveshagarwal

aveshagarwal Nov 21, 2017

Member

wanted to keep score as low as possible to avoid significant impact on scores assigned by least/most requested priority functions.

// The reason to create this new function is to be consistent with other
// priority functions because most or perhaps all priority functions work
// with schedulercache.Resource.
func getResourceLimits(pod *v1.Pod) *schedulercache.Resource {

This comment has been minimized.

@ravisantoshgudimetla

ravisantoshgudimetla Nov 20, 2017

Contributor

Wondering if this f(n) should reside in priorityMetadata(https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/scheduler/algorithm/priorities/metadata.go) as it might be useful later.

This comment has been minimized.

@aveshagarwal

aveshagarwal Nov 21, 2017

Member

I had thought about it, since its not part of default priorities yet, so having as part of the metadata would unnecessarily involve extra overhead.

@@ -107,6 +107,10 @@ func init() {
factory.RegisterPriorityFunction2("ImageLocalityPriority", priorities.ImageLocalityPriorityMap, nil, 1)
// Optional, cluster-autoscaler friendly priority function - give used nodes higher priority.
factory.RegisterPriorityFunction2("MostRequestedPriority", priorities.MostRequestedPriorityMap, nil, 1)
// Prioritizes nodes that satisfy pod's resource limits
if !utilfeature.DefaultFeatureGate.Enabled(features.ResourceLimitsPriorityFunction) {

This comment has been minimized.

@davidopp

davidopp Nov 20, 2017

Member

why is this !utilfeature... instead of utilfeature... ?

This comment has been minimized.

@aveshagarwal

aveshagarwal Nov 21, 2017

Member

I will fix it, seems i forgot to revert it after testing. thanks for catching this.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

/kind feature

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

/priority important-longterm

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

/status in-progres

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2017

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

/status in-progress

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2017

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

@aveshagarwal aveshagarwal force-pushed the aveshagarwal:master-scheduler-limits branch from ff4e7cc to c51b7f4 Nov 21, 2017

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

@davidopp fixed. PTAL.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 21, 2017

/test pull-kubernetes-verify
/test pull-kubernetes-e2e-gce

@jberkus

This comment has been minimized.

Copy link

jberkus commented Nov 22, 2017

/remove-priority important-longterm

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 23, 2017

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

4 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 23, 2017

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 23, 2017

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 23, 2017

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 23, 2017

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 27, 2017

@davidopp @bsalamat @wojtek-t rebased and fixed based on some comments. PTAL. and it needs lgtm tag again. thanks.

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Nov 27, 2017

@aveshagarwal hack/verify-bazel.sh is complaining in pull-kubernetes-verify. Seems this test is passing globally but has never passed for this PR.

Implement resource limit priority function. This function checks if t…
…he input pod's

resource limits are satisfied by the input node's allocatable resources or not.
If yes, the node is assigned a score of 1, otherwise the node's score is not changed.

@aveshagarwal aveshagarwal force-pushed the aveshagarwal:master-scheduler-limits branch from 273218d to b571001 Nov 27, 2017

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 27, 2017

@sjenning fixed bazel issue, hopefully should be fine now. thanks.

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 27, 2017

/test pull-kubernetes-unit

1 similar comment
@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 27, 2017

/test pull-kubernetes-unit

@aveshagarwal

This comment has been minimized.

Copy link
Member

aveshagarwal commented Nov 28, 2017

@davidopp @bsalamat it has passed all tests now, could you retag it with lgtm? thanks.

@bsalamat

This comment has been minimized.

Copy link
Contributor

bsalamat commented Nov 29, 2017

/lgtm

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Nov 29, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, bsalamat, davidopp, wojtek-t

Associated issue requirement bypassed by: wojtek-t

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Nov 29, 2017

[MILESTONENOTIFIER] Milestone Pull Request Current

@aveshagarwal @bsalamat @davidopp @resouer @wojtek-t

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/feature: New functionality.
Help
@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Nov 29, 2017

Automatic merge from submit-queue (batch tested with PRs 55893, 55906, 55026). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 8cc6729 into kubernetes:master Nov 29, 2017

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation aveshagarwal authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details

@aveshagarwal aveshagarwal deleted the aveshagarwal:master-scheduler-limits branch Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment