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

Scheduler extension #13580

Merged
merged 1 commit into from Nov 25, 2015
Merged

Scheduler extension #13580

merged 1 commit into from Nov 25, 2015

Conversation

ravigadde
Copy link
Contributor

Implementation of scheduler extension proposal as discussed in #11470

@k8s-bot
Copy link

k8s-bot commented Sep 4, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@ravigadde
Copy link
Contributor Author

cc @davidopp @bgrant0607

Please review when you get a chance.

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 4, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@mikedanese mikedanese assigned davidopp and unassigned karlkfi Sep 4, 2015
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2015
@ravigadde
Copy link
Contributor Author

@davidopp Any feedback? Can you please review after 1.1 freeze?

@davidopp
Copy link
Member

I briefly took a peek at this last night and am planning to review this by end of tomorrow. Very sorry for the delay.

@davidopp
Copy link
Member

Very sorry for the delay. I will review it this Friday.

@karlkfi
Copy link
Contributor

karlkfi commented Sep 23, 2015

I would like to see some kind of documentation included in this PR that describes the capability being added and how people might make use of it to externalize/extend scheduling logic.

@davidopp
Copy link
Member

+1

@@ -26,6 +29,8 @@ type Policy struct {
Predicates []PredicatePolicy `json:"predicates"`
// Holds the information to configure the priority functions
Priorities []PriorityPolicy `json:"priorities"`
// Holds the information to communicate with the extender
ExtenderConfig *ExtenderConfig `json:"extender"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow an array of these, so the user can configure more than one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will do.

@davidopp
Copy link
Member

Sorry it took me so long to take a look at this.

This looks OK to me, with all of the caveats @bgrant0607 mentioned here
#11470 (comment)

That is -- you are going to need to make significant changes to this when we start making planned scheduler changes that we just haven't had time for yet (e.g. unified extension mechanism, and changing the way we choose the "best" machine from summing up priority scores to using a decision tree). Since we also want to make improvements in scheduler performance and generally make the scheduler implementation a bit cleaner, probably we will just create a new scheduler for all of this, and allow people to continue using the old scheduler but won't do new work on it.

Of course, before submitting you need to write unit tests and either e2e or integration test.

@ravigadde
Copy link
Contributor Author

@davidopp
Thanks for the review. I am working on the feedback. Wrt test case comment, is it adequate if the test cases launch a http server which supports the extensions and shut it down after they are done?

@karlkfi
I will add some documentation as well when I update the PR.

@ravigadde
Copy link
Contributor Author

@davidopp Addressed review comments, PTAL. I have implemented unit test only at this point. Need to spend more time on figuring out e2e/integ test cases for this.


# Scheduler extender

Kubernetes scheduler schedules based on resources managed by Kubernetes. Scheduler extender is a way to extend the scheduling capability of k8s for resources that are not managed by k8s. A couple of example use cases are storage affinity and network availability.
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs a bit more introduction, and actually shouldn't mention "resources that are not managed by k8s" because there are other ways to handle that (e.g. write your own scheduler). I'd just replace this paragraph with:

There are three ways to add new scheduling rules (predicates and priority functions) to Kubernetes: (1) by adding these rules to the scheduler and recompiling (described here: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/scheduler.md), (2) implementing your own scheduler process that runs instead of, or alongside of, the standard Kubernetes scheduler, (3) implementing a "scheduler extender" process that the standard Kubernetes scheduler calls out to as a final pass when making scheduling decisions. This document describes the third approach. (Note that the three approaches are not mutually exclusive.)

@davidopp
Copy link
Member

LGTM

@davidopp davidopp added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Nov 25, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit 31450b691048532a2001a4b278cf0eb87e311ffb.

@davidopp
Copy link
Member

@ravigadde integration test is failing with

01:18:56 # k8s.io/kubernetes/test/integration
01:18:56 _output/local/go/src/k8s.io/kubernetes/test/integration/extender_test.go:228: unknown "k8s.io/kubernetes/pkg/client/unversioned".Config field 'Version' in struct literal
01:18:56 FAIL   k8s.io/kubernetes/test/integration [build failed]

Did you try running the integration tests locally? hack/test-integration.sh should do it.

@ravigadde
Copy link
Contributor Author

@davidopp Let me update to latest and fix it. Looks like this commit renamed the field recently - 94ad6aa

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit cadc24e.

@ravigadde
Copy link
Contributor Author

@davidopp Done, all tests pass now

@davidopp
Copy link
Member

LGTM

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 25, 2015

GCE e2e test build/test passed for commit cadc24e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 25, 2015
@k8s-github-robot k8s-github-robot merged commit 3ffc680 into kubernetes:master Nov 25, 2015
@davidopp
Copy link
Member

Yay! Thanks for your patience with this, @ravigadde

@ravigadde
Copy link
Contributor Author

@davidopp Thank you for your effort to make this happen

@khoi0107
Copy link

khoi0107 commented Nov 4, 2016

@ravigadde Hi, is there sample implementation of an extender somewhere?

@ravigadde
Copy link
Contributor Author

@khoi0107 I haven't seen an open implementation. @ping035627 submitted a patch sometime ago, not sure if their implementation is open. If you have any specific questions I can help with, ping me on slack - ravi.gadde

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet