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

Made multi-scheduler graduated to Beta #38871

Merged
merged 2 commits into from Jan 19, 2017
Merged

Made multi-scheduler graduated to Beta #38871

merged 2 commits into from Jan 19, 2017

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Dec 16, 2016

fixes #25318

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 16, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Dec 16, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2016
@k82cn k82cn force-pushed the k8s_25318 branch 2 times, most recently from ecb4bda to c75c8d4 Compare December 17, 2016 12:43
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2016
@davidopp davidopp added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Dec 18, 2016
@davidopp
Copy link
Member

Please remove the code that handles the annotation in this PR.

@k82cn
Copy link
Member Author

k82cn commented Dec 19, 2016

@k8s-bot cvm gke e2e test this

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2016
@k82cn
Copy link
Member Author

k82cn commented Jan 6, 2017

I'm working on the rebase; but it update-all.sh will also update docs unexpected, traced at #39505 .

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2017
@k82cn
Copy link
Member Author

k82cn commented Jan 7, 2017

cc @davidopp , if any more comments, please let me know :).

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2017
@davidopp
Copy link
Member

Please remove all the code that handles the annotation for scheduler name, e.g
https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/scheduler/factory/factory.go#L496
should be reading from the field rather than the annotation.

Also make sure the tests are setting scheduler name using the new field you introduced, and not the annotation. (I don't understand how the tests passed without you changing factory.go)

@k82cn
Copy link
Member Author

k82cn commented Jan 16, 2017

@davidopp , the annotation was removed as you mentioned, e.g. https://github.com/kubernetes/kubernetes/pull/38871/files#diff-77bdee21550bc698bfa654de11c64324L496 .

@k82cn
Copy link
Member Author

k82cn commented Jan 16, 2017

In the PR, the scheduler name was set to default if empty.

@davidopp
Copy link
Member

Ah OK, sorry I missed that! I will review the PR in the next couple of days.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2017
@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins Cross Build failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot cross build this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit adf672f17dbc0b54fdce64faf1ea1d42278afd12. Full PR test history. cc @k82cn

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2017
@k82cn
Copy link
Member Author

k82cn commented Jan 19, 2017

@timothysc , the code was re-based. PTAL :).

@davidopp
Copy link
Member

We decided to defer e2e tests when this feature went to alpha because we thought we were going to do a "next step" for this feature (an admission controller that would take a high-level policy configuration, and based on that, add the scheduler name annotations to the incoming pods), and I thought the integration test was sufficient until then. Well, we ended up not doing the next step, and I who knows if/when it will ever be done, so I think now is a very good time to do one or more e2e tests. @k82cn do you think you can do it in a follow-up PR?

As for back-to-back bindings from different schedulers, which @timothysc mentioned: the kubelet does admission control that checks for sufficient resources etc., so we are relying on that to ensure the node doesn't become overcomitted with respect to request. Unfortunately non-node-local properties aren't checked by the kubelet, but I think the only one that matters right now is non-node TopologyKey for pod anti-affinity (e.g. "don't put this pod in the same zone as any pods that match label selector X"), and we said we were probably going to disable that when it goes to beta in 1.6...

@k82cn k82cn mentioned this pull request Jan 19, 2017
Closed
@k82cn
Copy link
Member Author

k82cn commented Jan 19, 2017

@davidopp , sure; I'd like to add more e2e for this feature.
The follow-up PR will trace at #40125; please feel free to assign to me :).

@timothysc timothysc added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2017
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 05c2a08 into kubernetes:master Jan 19, 2017
@k82cn k82cn deleted the k8s_25318 branch January 19, 2017 23:34
@k82cn k82cn mentioned this pull request Jan 20, 2017
Closed
@davidopp davidopp mentioned this pull request Feb 13, 2017
Open
3 tasks
smarterclayton
smarterclayton reviewed Feb 20, 2017
View reviewed changes
pkg/api/v1/types.go
// If specified, the pod will be dispatched by specified scheduler.
// If not specified, the pod will be dispatched by default scheduler.
// +optional
SchedulerName string `json:"schedulername,omitempty" protobuf:"bytes,19,opt,name=schedulername"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong - this should have been schedulerName

Please ensure this is fixed pre 1.6.0 - this is a release blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I'll create a PR today :).

@smarterclayton
Copy link
Contributor

This has an incorrect JSON serialization for schedulername - must be schedulerName. Please fix that ASAP, it may not go out in 1.6.

@davidopp davidopp mentioned this pull request Feb 22, 2017
Closed
@davidopp davidopp changed the title Made multi-scheduler graduated to Beta and then v1. Made multi-scheduler graduated to Beta Mar 22, 2017
@davidopp davidopp mentioned this pull request Mar 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@timothysc timothysc timothysc approved these changes

Assignees

@timothysc timothysc

@davidopp davidopp

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Requirements for multi-scheduler to graduate to Beta and then v1
7 participants
@k82cn @k8s-reviewable @davidopp @timothysc @k8s-ci-robot @k8s-github-robot @smarterclayton