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

Add write-config-to to scheduler #62515

Merged
merged 2 commits into from
Apr 19, 2018
Merged

Conversation

resouer
Copy link
Contributor

@resouer resouer commented Apr 13, 2018

What this PR does / why we need it:
Scheduler should be able to write its default configure to file. This actually applies to all components which claims options other than --config will be deprecated.

Otherwise, users will be super confused to find out how to write a proper config file to these components.

See: https://stackoverflow.com/questions/47966440/how-to-create-a-config-file-for-kube-scheduler-to-use-by-the-config-argument
ref: #52562

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 #58805

Usage:

./_output/bin/kube-scheduler --write-config-to /tmp/kube-scheduler.yaml

Special notes for your reviewer:
This should have been fixed several releases ago, so lets include it in 1.11

Release note:

Add write-config-to to scheduler

@resouer resouer added this to the v1.11 milestone Apr 13, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 13, 2018
@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 13, 2018
@resouer resouer added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 13, 2018
@resouer
Copy link
Contributor Author

resouer commented Apr 13, 2018

cc @corest , you should be interested in

Copy link
Contributor

@xiangpengzhao xiangpengzhao left a comment

Choose a reason for hiding this comment

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

I like this idea, but maybe we can just document an straightforward example somewhere rather than coding ?

@@ -167,8 +172,8 @@ func NewOptions() (*Options, error) {
}

func (o *Options) Complete() error {
if len(o.ConfigFile) == 0 {
glog.Warning("WARNING: all flags other than --config are deprecated. Please begin using a config file ASAP.")
if len(o.ConfigFile) == 0 && len(o.WriteConfigTo) == 0 {
Copy link
Contributor

@xiangpengzhao xiangpengzhao Apr 13, 2018

Choose a reason for hiding this comment

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

When len(o.ConfigFile) == 0 but len(o.WriteConfigTo) != 0, we still need to apply these deprecated stuff, but this condition skips this case. So I think we don't need to add len(o.WriteConfigTo) == 0 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. It's just a warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

But o.applyDeprecatedHealthzAddressToConfig() and friends aren't just warning. They apply healthzAddress (which is the flag --address), etc. So if users specify flags like this --write-config-to ./scheduler.yaml --address 1.2.3.4, the flag --address 1.2.3.4 won't take effect. Maybe I miss something here? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Mis-read again... When --write-config-to is specified, kube-scheduler will write the config and exit. So it's ok as-is.

@@ -106,6 +110,7 @@ type Options struct {
// AddFlags adds flags for a specific SchedulerServer to the specified FlagSet
func (o *Options) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, "The path to the configuration file.")
fs.StringVar(&o.WriteConfigTo, "write-config-to", o.WriteConfigTo, "If set, write the default configuration values to this file and exit.")
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, writeConfigFile writes both default and configured values which are loaded from --config if specified, so the description of this flag is inaccurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this patch always generate default values, which is expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you're right. I mis-read some codes.

@resouer
Copy link
Contributor Author

resouer commented Apr 13, 2018

I like this idea, but maybe we can just document an straightforward example somewhere rather than coding ?

Actually as we are migrating to ComponentConfig , --write-config-to is now a mandatory option and it has already been added to several components like kube-proxy I believe.

@@ -106,6 +110,7 @@ type Options struct {
// AddFlags adds flags for a specific SchedulerServer to the specified FlagSet
func (o *Options) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, "The path to the configuration file.")
fs.StringVar(&o.WriteConfigTo, "write-config-to", o.WriteConfigTo, "If set, write the default configuration values to this file and exit.")
Copy link
Member

Choose a reason for hiding this comment

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

If the config is not nil, the value is not default, right?

Copy link
Contributor Author

@resouer resouer Apr 13, 2018

Choose a reason for hiding this comment

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

Right now, it only write default values from schema, which is my intention.

@k82cn
Copy link
Member

k82cn commented Apr 13, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2018
@resouer
Copy link
Contributor Author

resouer commented Apr 13, 2018

@bsalamat @k82cn Currently, I intentionally make it write default values to file only, but please let me know if you think we should write out real values if --config is provided.

But it's wired in my opinion:

kube-scheduler --config=/tmp/scheduler.yaml --write-config-to /tmp/temp

So my 2c is let's keep current design and I can make those two flags exclusive. After all, it does make sense to use them together.

@k82cn
Copy link
Member

k82cn commented Apr 16, 2018

So my 2c is let's keep current design and I can make those two flags exclusive. After all, it does make sense to use them together.

That's ok to me. If writing out real values is NOT complex, it seems better as it cover more case (writting default value if no --config).

@resouer
Copy link
Contributor Author

resouer commented Apr 16, 2018

@k82cn There's no tech blocker. Just hope to make sure this consistent with users expectation.

@AshleyByeUK @tiukhtinvladimir I saw you are tracking this issue, do you expect --write-config-to generate user set values when used with --config, or it's better to stick it to always default. Or you just don't care too much?

@kennethredler
Copy link

@resouer IMO, do default first. Then iterate to include --config values on next round.

@k82cn
Copy link
Member

k82cn commented Apr 16, 2018

There's no tech blocker. Just hope to make sure this consistent with users expectation.

Both can cover the original case: writing the default value into a file. Working with --config provides more options, but don-t want to put much effort on a "maybe" case.

@junglie85
Copy link

@resouer I think it makes sense for --write-config-to to provide default values. If --config flag is applied, it would seem more appropriate to write out the current config - this latter flag is likely to only be an interim requirement to allow existing command line arguments to be converted to a config file, right?

Ultimately, I'm not overly fussed, but this seems like the sanest approach.

@resouer
Copy link
Contributor Author

resouer commented Apr 17, 2018

@AshleyByeUK Correct.

Thanks guys. According to current discussion, I will make -write-config-to write out default values, but if --config is provided at the same time, it will write out given values.

Change to write provided values if config is set
@resouer
Copy link
Contributor Author

resouer commented Apr 18, 2018

@k82cn Take a final look?

@k82cn
Copy link
Member

k82cn commented Apr 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@k82cn @resouer @kubernetes/sig-scheduling-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 2 days, the pull request will be moved out of the v1.11 milestone.

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, resouer

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 62481, 62643, 61877, 62515). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2c54e9c into kubernetes:master Apr 19, 2018
@resouer resouer deleted the fix-58805 branch April 19, 2018 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-approval priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kube-scheduler doesn not have --write-config-to option
7 participants