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

Listing existing algorithm providers in scheduler #6967

Merged
merged 1 commit into from
Apr 22, 2015
Merged

Listing existing algorithm providers in scheduler #6967

merged 1 commit into from
Apr 22, 2015

Conversation

HaiyangDING
Copy link

This PR implements issue #6612

Allow 'kube-scheduler --help' to show available algorithm providers under field '--algorithm_provider'. All available providers are listed and separated by ' | '.

If implemented a custom algorithm provider (MyProvider), --algorithm_provider would be like:

--algorithm_provider="DefaultProvider": The scheduling algorithm provider to use, one of: DefaultProvider | MyProvider

@HaiyangDING
Copy link
Author

Updated 2015.04.21.

Confirmed, not a bug.

@lavalamp , Hi,

I might have found a bug in kubernetes.

  1. Implemente a custom algorithm provider and name it "MyProvider".

  2. Start kube-scheduler with DefaultProvider, use kube-scheduler --help and it prints

    algorithm_provider="DefaultProvider"

    which is quite OK.

  3. Stop kube-scheduler and start a new one with

    kube-scheduler --logtostderr=true --master=127.0.0.1:8080 --algorithm_Provider=MyProvider

    which successfully launches a kube-scheduler process.

  4. use kube-scheduler --help again, and it prints

    algorithm_provider="DefaultProvider"

    and there is the problem: intuitively, field "algorithm_provider" should be "MyProvider", not "DefaultProvider"

This suggests either or both of the following:

  1. algorithm_provider can not reflect current running algorithm_provider
  2. the scheduler fails to switch the algorithm_provider

According to my understanding, I think it is due to both of the above reasons.

Am I right? If so, I would like file a new issue and try to fix this.

@bgrant0607
Copy link
Member

cc @abhgupta @mikedanese

@mikedanese
Copy link
Member

@HaiyangDING regarding your question to @lavalamp, any parameters displayed in --help refer to the default parameters of the app, not the parameters selected for the currently running instance of the app. That doesn't look like a bug.

mikedanese
mikedanese reviewed Apr 17, 2015
View reviewed changes
plugin/pkg/scheduler/factory/plugins.go
@@ -61,6 +61,11 @@ const (
DefaultProvider = "DefaultProvider"
)

var (
AlgorithmProviders = ""
providerCounter = 1
Copy link
Member

Choose a reason for hiding this comment

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

Mutable global state! @lavalamp just did a PR to remove all this from the scheduler. See #5351

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely cannot do it this way. :)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mikedanese and @lavalamp

Thanks for all the advice. I have learned much:) And I need a little more time to fix all the changes you have proposed.

@HaiyangDING
Copy link
Author

@mikedanese @lavalamp

Aside from improving this PR, your suggestions also help me deepen my understanding of both go programming and kubernetes design. I cannot start working on it until Monday so new patch would need a little more time to be submitted. Many thanks to you again.

Have a nice weekend!

@HaiyangDING HaiyangDING force-pushed the ListAlgos branch 3 times, most recently from 8a0f6c8 to de157bc Compare April 21, 2015 06:45
@HaiyangDING
Copy link
Author

@mikedanese @lavalamp

New patch submitted thanks to your suggestions, please have a look.

@mikedanese
Copy link
Member

LGTM besides nit.

mikedanese
mikedanese reviewed Apr 21, 2015
View reviewed changes
plugin/pkg/scheduler/factory/plugins.go
@@ -300,3 +301,12 @@ func validatePriorityOrDie(priority schedulerapi.PriorityPolicy) {
}
}
}

// This function is called when listing all availabe algortihm providers in `kube-scheduler --help`
Copy link
Member

Choose a reason for hiding this comment

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

Replace 'This function' with 'ListAlgorithmProviders' to conform with the godoc spec. From http://blog.golang.org/godoc-documenting-go-code a 'comment is a complete sentence that begins with the name of the element it describes'.

Listing available algorithm providers in scheduler
71b9884 
List the available algorithm providers with 'kube-scheduler --help' under field `algorithm_provider`
@HaiyangDING
Copy link
Author

PTAL. @mikedanese @lavalamp

@davidopp
Copy link
Member

LGTM

bgrant0607 added a commit that referenced this pull request Apr 22, 2015
@bgrant0607
Merge pull request #6967 from HaiyangDING/ListAlgos
740d2a5 
Listing existing algorithm providers in scheduler
@bgrant0607 bgrant0607 merged commit 740d2a5 into kubernetes:master Apr 22, 2015
@HaiyangDING HaiyangDING mentioned this pull request Jun 26, 2015
Closed
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@HaiyangDING @bgrant0607 @mikedanese @davidopp @lavalamp @googlebot