Promote resource limits priority function to beta #69437

ravisantoshgudimetla · 2018-10-04T19:34:27Z

What this PR does / why we need it:
We should promote resource limits priority function to beta as this feature is useful in scenarios where the nodes that satisfies limits of pods are given higher score.

/cc @aveshagarwal @bsalamat
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Promote resource limits priority function to beta

ravisantoshgudimetla · 2018-10-04T19:34:48Z

/sig scheduling

bsalamat

I am fine with the promotion, I just wonder if you have tested this feature in real clusters enough.

bsalamat · 2018-10-05T20:56:01Z

test/e2e/scheduling/priorities.go

Limits is smaller than Requests?

This was part of testing, I was doing, I forgot to revert it. Thanks.

bsalamat · 2018-10-05T20:58:34Z

test/e2e/scheduling/priorities.go

It makes more sense for the function to receive the new amount of memory as an argument.

Sure, I have added it.

bsalamat · 2018-10-05T21:10:20Z

test/e2e/scheduling/priorities.go

I think you need to revert the node resources back to avoid surprises for future test cases.

Yes, I missed it. I have updated the PR now.

bsalamat · 2018-10-05T21:11:30Z

test/e2e/scheduling/priorities.go

Any particular reason for deleting this?

Thanks for catching, you are right, I was thinking I was changing the test I am introducing as part of this PR. I was not intending to touch this.

k82cn · 2018-10-07T01:25:40Z

test/e2e/scheduling/priorities.go

hm... , seems node's memory is smaller than pod's limit :) The middle one of node list maybe better.

Including other priority test, we should check length of nodeList; if there's only one node, our test can not catch "issues".

Yes, I can change it to a middle node but tbh, none of the pull-verify-* jobs are testing these current e2e's, so actually this could have failed after the PR merged :(.

so actually this could have failed after the PR merged :(.

I mean set nodeName to middle of nodeList.Items:)
As I think you're setting a smaller memory (10^4) than pod's limits (3 * 10^4), it's better to use a const, e.g. node's memory is twice of big pod.

k82cn · 2018-10-07T01:34:03Z

/kind feature

bsalamat · 2018-10-08T22:43:36Z

test/e2e/scheduling/priorities.go

nit: s/value/memory/

Also, please add a comment for the function and perhaps make it private (change to lower case).

bsalamat · 2018-10-08T22:46:05Z

test/e2e/scheduling/priorities.go

nit: This if statement is not needed. At this point it returns 'err' regardless.

bsalamat · 2018-10-08T22:49:40Z

test/e2e/scheduling/priorities.go

Shouldn't you read the original amount of memory at the beginning of the function, before you update it?

yastij · 2018-10-09T20:47:31Z

cc @kubernetes/api-approvers

ravisantoshgudimetla · 2018-10-15T01:44:40Z

/retest

bsalamat

just a minor comment. Otherwise, LGTM.

bsalamat · 2018-10-15T21:41:41Z

test/e2e/scheduling/priorities.go

Looks like this var is used only in one of the tests. Please move it inside the test func.

ravisantoshgudimetla · 2018-10-16T19:13:13Z

/retest

bsalamat

/lgtm

Thanks, @ravisantoshgudimetla!

ravisantoshgudimetla · 2018-10-16T23:47:40Z

/retest

ravisantoshgudimetla · 2018-10-17T00:03:45Z

/retest

bsalamat · 2018-10-17T17:17:29Z

/approve

k8s-ci-robot · 2018-10-17T17:17:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, ravisantoshgudimetla

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/features/OWNERS~~ [bsalamat]
~~test/e2e/scheduling/OWNERS~~ [bsalamat,ravisantoshgudimetla]

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

AishSundar · 2018-10-18T04:08:08Z

@ravisantoshgudimetla I see the newly added Scheduler Priorities test added fail in the test run - https://k8s-testgrid.appspot.com/sig-release-master-upgrade#gke-gci-new-gci-master-upgrade-cluster-new

Can you please take a look?

@jberkus @mortent as FYI. We need to open a test issue to track the test failure if it repros in the next run too.

ravisantoshgudimetla · 2018-10-18T04:52:14Z

@AishSundar Sure, I will look into it. FWIW, the test is also failing on master(https://k8s-testgrid.appspot.com/sig-scheduling#gce-serial).

AishSundar · 2018-10-18T12:50:10Z

/milestone v1.13

Ok lets open a new test issue for this. thanks @ravisantoshgudimetla

bsalamat · 2018-10-22T21:16:18Z

@ravisantoshgudimetla Looks like the e2e test for this PR is flaky: https://k8s-testgrid.appspot.com/sig-release-master-blocking#gce-cos-master-serial

Could you please take a look?

k8s-ci-robot requested review from aveshagarwal and bsalamat October 4, 2018 19:34

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 4, 2018

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch from 9468a1c to 00ba87a Compare October 4, 2018 19:45

bsalamat reviewed Oct 4, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 5, 2018

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch from 5f07317 to 3df5963 Compare October 5, 2018 18:47

bsalamat reviewed Oct 5, 2018

View reviewed changes

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch 3 times, most recently from 1289007 to 44ff6dc Compare October 6, 2018 17:32

k82cn reviewed Oct 7, 2018

View reviewed changes

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 7, 2018

bsalamat reviewed Oct 8, 2018

View reviewed changes

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch 3 times, most recently from 5b8906f to 58c7768 Compare October 15, 2018 00:32

bsalamat reviewed Oct 15, 2018

View reviewed changes

test/e2e/scheduling/priorities.go Outdated

Copy link

Member

bsalamat Oct 15, 2018

Choose a reason for hiding this comment

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

Looks like this var is used only in one of the tests. Please move it inside the test func.

Promote resource limits priority functions to beta

e44f29a

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch from 58c7768 to 922ea66 Compare October 16, 2018 16:57

E2E and build file generated

ba71ae7

ravisantoshgudimetla force-pushed the promote-resource-limits-priority-function branch from 922ea66 to ba71ae7 Compare October 16, 2018 16:59

k8s-ci-robot assigned bsalamat Oct 16, 2018

bsalamat approved these changes Oct 16, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2018

k8s-ci-robot merged commit 897b3a9 into kubernetes:master Oct 17, 2018

k8s-ci-robot added this to the v1.13 milestone Oct 18, 2018

mortent mentioned this pull request Oct 18, 2018

Failing test: [sig-scheduling] SchedulerPriorities [Serial] Pod should be preferably scheduled to nodes which satisfy its limits #69989

Closed

Promote resource limits priority function to beta #69437

Promote resource limits priority function to beta #69437

Uh oh!

Conversation

ravisantoshgudimetla commented Oct 4, 2018

Uh oh!

ravisantoshgudimetla commented Oct 4, 2018

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn commented Oct 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yastij commented Oct 9, 2018

Uh oh!

ravisantoshgudimetla commented Oct 15, 2018

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Oct 16, 2018

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

ravisantoshgudimetla commented Oct 16, 2018

Uh oh!

ravisantoshgudimetla commented Oct 17, 2018

Uh oh!

bsalamat commented Oct 17, 2018

Uh oh!

k8s-ci-robot commented Oct 17, 2018

Uh oh!

AishSundar commented Oct 18, 2018

Uh oh!

ravisantoshgudimetla commented Oct 18, 2018

Uh oh!

AishSundar commented Oct 18, 2018

Uh oh!

bsalamat commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

k82cn Oct 10, 2018 •

edited

Loading