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.

Sign up for GitHub

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

Jump to bottom

Change scheduler logic from random to round-robin #22430

Merged
merged 1 commit into from
Mar 8, 2016
Merged

Change scheduler logic from random to round-robin #22430

merged 1 commit into from
Mar 8, 2016

Conversation

gmarek
Copy link
Contributor

@gmarek gmarek commented Mar 3, 2016

To reduce the probability of multiple Pods being schedule on the same node.

cc @hongchaodeng @wojtek-t

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 3, 2016
@gmarek gmarek force-pushed the round-robin branch 2 times, most recently from 0389ead to 38e5c7b Compare March 3, 2016 12:43
@eparis eparis mentioned this pull request Mar 3, 2016
Closed
@gmarek
Copy link
Contributor Author

gmarek commented Mar 3, 2016

I'm thinking if we shouldn't sort the priority list alphabetically.

@wojtek-t
Copy link
Member

wojtek-t commented Mar 3, 2016

We definitely should do some sorting, because for maps which we are processing in the meantime, we have exaclty no guarantees...

@davidopp
Copy link
Member

davidopp commented Mar 3, 2016

Yeah I think this only works if priorityList and idx both remain stable across calls. Is that actually true in practice?

(BTW I didn't understand @wojtek-t 's comment about "maps we are processing in the meantime")

@wojtek-t
Copy link
Member

wojtek-t commented Mar 4, 2016

(BTW I didn't understand @wojtek-t 's comment about "maps we are processing in the meantime")

Sorry - I meant that in the meantime we have maps, and iterating over a map (to generate a slice) is random, so the order is also random.

But yeah - the underlying comment is what @davidopp wrote right above, which is currently not true, but adding sorting will solve this issue.

@gmarek
Copy link
Contributor Author

gmarek commented Mar 4, 2016

@wojtek-t @davidopp PTAL

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 4, 2016
wojtek-t
wojtek-t reviewed Mar 4, 2016
View reviewed changes
plugin/pkg/scheduler/generic_scheduler.go
idx = append(idx, i)
}
}
lastMaxScoreIndes := sort.Search(len(priorityList), func(i int) bool { return priorityList[i].Score < maxScore })
Copy link
Member

Choose a reason for hiding this comment

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

to be clear, it's not "lastMaxScoreIndex", but "firstNonMaxScoreIndex"

we should be explicit about this

@wojtek-t
Copy link
Member

wojtek-t commented Mar 4, 2016

One comment from me - other than that LGTM

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 052886161e095dbb63358c96f9aca5fcdf37f016.

@k8s-bot
Copy link

k8s-bot commented Mar 4, 2016

GCE e2e build/test passed for commit 413673f6213b579f9fbc0660f0b51cfa2f59adbf.

@wojtek-t
Copy link
Member

wojtek-t commented Mar 4, 2016

LGTM

@davidopp - can you please take a look?

davidopp
davidopp reviewed Mar 6, 2016
View reviewed changes
plugin/pkg/scheduler/api/types.go
@@ -165,7 +165,7 @@ func (h HostPriorityList) Less(i, j int) bool {
if h[i].Score == h[j].Score {
return h[i].Host < h[j].Host
}
return h[i].Score < h[j].Score
return h[i].Score > h[j].Score
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the more standard way to do this is to leave it as < and then do the sort using
sort.Sort(sort.Reverse(priorityList))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gmarek
Copy link
Contributor Author

gmarek commented Mar 7, 2016

@davidopp PTAL

@k8s-bot
Copy link

k8s-bot commented Mar 7, 2016

GCE e2e build/test passed for commit 2c52e62.

@davidopp
Copy link
Member

davidopp commented Mar 7, 2016

LGTM

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 8, 2016

GCE e2e build/test passed for commit 2c52e62.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 8, 2016
Merge pull request #22430 from gmarek/round-robin
a6aaf81 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit a6aaf81 into kubernetes:master Mar 8, 2016
@gmarek
Copy link
Contributor Author

gmarek commented Mar 8, 2016

It seems that it prevents spikes in e2e startup latencies in 100 pod/node kubemark runs. We have only 3 runs with this patch now, but if it'll be passing until tomorrow I think we should cherry-pick this change. @wojtek-t @davidopp

@wojtek-t
Copy link
Member

wojtek-t commented Mar 8, 2016

@gmarek - ok I will check it tomorrow and apply the label if that's the case.

@eparis eparis mentioned this pull request Mar 8, 2016
Merged
@timothysc
Copy link
Member

@gmarek could you please /cc the scheduling sigs on these changes.. this effects us too.

@hongchaodeng
Copy link
Contributor

/cc @kubernetes/sig-scheduling @kubernetes/sig-scalability

@wojtek-t
Copy link
Member

wojtek-t commented Mar 9, 2016

I've just check our test - and the high density test didn't failed since this PR was merged (which is last 20 runs). We should definitely cherrypick it to 1.2.

@wojtek-t wojtek-t added sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cherrypick-candidate labels Mar 9, 2016
@gmarek
Copy link
Contributor Author

gmarek commented Mar 9, 2016

@timothysc - sorry, I thought that this is cosmetic enough change, that I don't need to inform anyone about it.

@jeremyeder
Copy link

Honest question: is there a way in github to set up certain fields that are required before you can open an issue?

For example, a drop-down picker for "groups to cc" or "tags to add". Github as a issue reporting/ticketing system is just brutal.

@eparis
Copy link
Contributor

eparis commented Mar 9, 2016

Add the v1.2 milestone. Starting later today cherrypick-candidate will be removed from any PR that doesn't set the milestone.

@eparis eparis added this to the v1.2 milestone Mar 9, 2016
@bgrant0607
Copy link
Member

@jeremyeder Github just recently added issue and PR templates, if you'd like to investigate how to help us set it up.

@bgrant0607 bgrant0607 added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 11, 2016
@bgrant0607
Copy link
Member

@gmarek Please write a good release note for this and put it in the PR description.

@gmarek
Copy link
Contributor Author

gmarek commented Mar 11, 2016

@bgrant0607 - I'll do this, but this is really just a workaround for broken go 1.4.2 random number generator.

eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 11, 2016
@eparis
Merge pull request kubernetes#22430 from gmarek/round-robin
7d07d2c 
Auto commit by PR queue bot
@eparis eparis mentioned this pull request Mar 11, 2016
Merged
@eparis
Copy link
Contributor

eparis commented Mar 11, 2016

This PR was sucessfully cherry picked in PR #22855
please verify that the release-1.2 branch contains these changes as you would expect and contact @eparis if there appear to be problems.

@eparis eparis removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
@gmarek gmarek deleted the round-robin branch August 30, 2016 09:47
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#22430 from gmarek/round-robin
71738dc 
Auto commit by PR queue bot
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#22430 from gmarek/round-robin
2cb3b88 
Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidopp davidopp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.