Fix the kube-scheduler binary's description of the --config parameter… #98254

changshuchao · 2021-01-21T06:23:38Z

… is inaccurate

Signed-off-by: changshuchao chang.shuchao1@zte.com.cn

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #
#98211
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-01-21T06:23:45Z

@changshuchao: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2021-01-21T06:23:46Z

Hi @changshuchao. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

changshuchao · 2021-01-21T06:25:26Z

/assign @damemi

changshuchao · 2021-01-21T06:25:36Z

@damemi sorry, I accidentally closed the previous pr, please review the new modification.

alculquicondor · 2021-01-21T14:48:23Z

/ok-to-test

alculquicondor · 2021-01-21T14:49:45Z

cmd/kube-scheduler/app/options/options.go

+	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, `The path to the configuration file. The affected parameters are as follows.
+The following flags can overwrite fields in this file:
+  --algorithm-provider
+  --policy-config-file


IRRC policy flags cause an error when used with config

I enabled policy-config to be set with componentconfig (to ease the deprecation transition) in #92531

alculquicondor · 2021-01-21T14:50:30Z

cmd/kube-scheduler/app/options/options.go

+  --policy-configmap
+  --policy-configmap-namespace
+  --use-legacy-policy-config
+The following flags will be not effective when set --config , we must move into this file:


TMI. Instead, we can put a note in each of the flags description

Oh, I missed this line from my first comment. Agree this is not necessary

damemi

/ok-to-test
@changshuchao I am still not clear on the intent of this change, see my question in the original PR (#98211 (review))

This might be a misinterpretation though. You said:

when the --config parameter is set, the following parameters are invalid in the command line...

But the flag description says

The following flags can overwrite fields in this file..

This means that for example, passing --kubeconfig with --config should work (with --kubeconfig overwriting the value in the --config file). But --use-legacy-policy-config should not work, because it shouldn't overwrite any values

Is it the case that these flags don't work when passing --config or that they do work?

Do these additional fields actually overwrite values in the config file? If so, that is a bigger bug than just updating the description (they should not work at all when a config file is passed)
/hold

changshuchao · 2021-01-22T03:51:44Z

@damemi Sorry,my previous description is not very accurate.
After my test, when the --config parameter is set, the following command line parameters will be not effected in the command line,it means that no matter what value you set the parameter, it will only use the default value or not take effect. (For example --profiling, its default value is true, if you set --profiling to false while setting the --config parameter, the last effective value is still true),the parameters that are not effective like this parameter are as follows:
--scheduler-name
--hard-pod-affinity-symmetric-weight
--address
--port
--leader-elect
--leader-elect-lease-duration
--leader-elect-renew-deadline
--leader-elect-retry-period
--leader-elect-resource-lock
--leader-elect-resource-name
--leader-elect-resource-namespace
--profiling
--contention-profiling
--kubeconfig
--kube-api-content-type
--kube-api-qps
--kube-api-burst
--lock-object-namespace
--lock-object-name

damemi

@changshuchao no problem, it was my confusion from the original description :)

however I have one more comment, I think what we should do is:

list the flags that will work with --config in the config flag description
for the flags that don't work, add the comment to those separate flag's description (which you did here)

damemi · 2021-01-22T13:34:59Z

cmd/kube-scheduler/app/options/options.go

@@ -150,13 +150,26 @@ func newDefaultComponentConfig() (*kubeschedulerconfig.KubeSchedulerConfiguratio
 // Flags returns flags for a specific scheduler by section name
 func (o *Options) Flags() (nfs cliflag.NamedFlagSets) {
 	fs := nfs.FlagSet("misc")
-	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, `The path to the configuration file. The following flags can overwrite fields in this file:
+	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, `The path to the configuration file. The following flags will be not effective when set --config , we must move into this file:


I think @alculquicondor's intent was to keep the original (shorter) list of flags that do work with --config, and only note in the description of those that don't work.

This change doesn't provide any info to the user about what they can set with the config flag, and having the list of ones that don't work is redundant with having that info added to the flag description, as you have done

ok, your suggestion is really pertinent. We need to care about the parameters that are in effect, and remind users to pay attention to those that are not. : )

The command line parameters that do work when --config is set, need to remove --port and --address. The code before modification is as follows( in cmd/kube-scheduler/app/options/options.go):

kubernetes/cmd/kube-scheduler/app/options/options.go

Lines 153 to 159 in b557633

fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, `The path to the configuration file. The following flags can overwrite fields in this file:

--address

--port

--use-legacy-policy-config

--policy-configmap

--policy-config-file

--algorithm-provider`)

2.According to your suggestion--'only note in the description of those that don't work',I can note most of the parameters, but some parameters for leader elect cannot be done, Because the code is in the component-base dependency package(vendor/k8s.io/component-base/config/options/leaderelectionconfig.go),as follows:
--leader-elect
--leader-elect-lease-duration
--leader-elect-renew-deadline
--leader-elect-retry-period
--leader-elect-resource-lock
--leader-elect-resource-name
--leader-elect-resource-namespace
For this reason, I keep the following notes:
https://github.com/kubernetes/kubernetes/blob/078cfa486e0b9390eb8e96d29076bc9a7fee7370/cmd/kube-scheduler/app/options/options.go#L158-L177

@changshuchao I see, then in my opinion it makes sense to note here which flags will not be effective for the flags which we import from other packages (and therefore can't update the description of).

Are all of the flags you listed in your comment imported? some like scheduler-name we should have the ability to update the description right? Though, it may be less confusing to have all of the affected flags in one spot. In other words, as a user I would expect this list to be complete if it's here at all.

So, since we don't have control over every flag's description that isn't effective with the config file I think it does make sense to list them all here. Sorry to flip flop on that, I wasn't originally thinking about the ones we can't update the description for (@alculquicondor may have missed that too, do you agree Aldo?)

@damemi
'Are all of the flags you listed in your comment imported?' --Yes,i think it's all included.
'some like scheduler-name we should have the ability to update the description right? ' --Yes, I have modified everything that can be modified.

damemi · 2021-01-28T14:13:52Z

At this point this lgtm. I originally agreed that we should keep the list of affected flags short in --config's description, but after it was pointed out that some of them are imported it makes sense to have a complete list in one spot. The deprecation notices on the individual flags we do control are good too.

/approve
/hold cancel

I'll leave lgtm to Aldo if there's anything else :)

k8s-ci-robot · 2021-01-28T14:14:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: changshuchao, damemi

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:

~~cmd/kube-scheduler/OWNERS~~ [damemi]

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

… is inaccurate Signed-off-by: changshuchao <chang.shuchao1@zte.com.cn>

alculquicondor

/lgtm

changshuchao · 2021-02-01T01:15:34Z

@alculquicondor Sorry, can this pr be merged?

damemi · 2021-02-01T14:08:40Z

@changshuchao the release-note block in your main comment needs a description, since this is a user-facing change

changshuchao · 2021-02-02T01:58:55Z

@damemi I'm sorry, I didn't understand what you mean, can you give me more detailed instructions?

damemi · 2021-02-02T13:58:50Z

@damemi I'm sorry, I didn't understand what you mean, can you give me more detailed instructions?

@changshuchao this has the do-not-merge/release-note-label-needed flag, which means in your main PR description you need to update the "User facing change?" section to include a note in the release-note code block (this is a user facing change). For example:

For non-user-facing changes, you need to write NONE in that block

changshuchao · 2021-02-03T02:25:36Z

@damemi Sorry, I am ignorant. I just added some descriptions in release-note block. Is it OK to write like this? : )

damemi · 2021-02-03T13:46:11Z

@changshuchao not to worry, the release-note block is a bit unclear for many people :)

alculquicondor · 2021-02-03T15:19:20Z

Should we cherrypick to 1.20 or earlier?

alculquicondor · 2021-02-03T15:21:35Z

FYI, this release note is too detailed. You could say something like:

Fix the description of command line flags that can override --config

You can still change the release note, I think. They are all put together when the release happens.

changshuchao · 2021-02-04T01:29:20Z

@alculquicondor I have modified it, thanks for the guidance.

alculquicondor · 2021-02-04T14:23:57Z

Can you do a cherry-pick to 1.20 at least? https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md

changshuchao · 2021-02-05T01:21:24Z

@alculquicondor ok,i will give a try :) #98786

…iption of the --config parameter is inaccurate Signed-off-by: changshuchao <chang.shuchao1@zte.com.cn>

…ry's description of the --config parameter is inaccurate Signed-off-by: changshuchao <chang.shuchao1@zte.com.cn>

…1-19 Cherry pick of #98254 for 1.19: Fix the kube-scheduler binary's descr…

Cherry pick of #98254:Fix the kube-scheduler binary's description of …

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 21, 2021

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2021

k8s-ci-robot requested review from draveness and liu-cong January 21, 2021 06:24

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 21, 2021

k8s-ci-robot assigned damemi Jan 21, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 21, 2021

alculquicondor reviewed Jan 21, 2021

View reviewed changes

damemi reviewed Jan 21, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2021

changshuchao force-pushed the scheduler-config-help-line branch from 721b029 to b7df393 Compare January 22, 2021 06:34

damemi reviewed Jan 22, 2021

View reviewed changes

changshuchao force-pushed the scheduler-config-help-line branch from b7df393 to 078cfa4 Compare January 23, 2021 09:25

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021

Fix the kube-scheduler binary's description of the --config parameter…

7d8e07b

… is inaccurate Signed-off-by: changshuchao <chang.shuchao1@zte.com.cn>

alculquicondor reviewed Jan 29, 2021

View reviewed changes

k8s-ci-robot assigned alculquicondor Jan 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 3, 2021

k8s-ci-robot merged commit 356ee4c into kubernetes:master Feb 3, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 3, 2021

changshuchao deleted the scheduler-config-help-line branch February 3, 2021 06:02

changshuchao added a commit to changshuchao/kubernetes that referenced this pull request Feb 5, 2021

Cherry pick of kubernetes#98254:Fix the kube-scheduler binary's descr…

5d9910a

…iption of the --config parameter is inaccurate Signed-off-by: changshuchao <chang.shuchao1@zte.com.cn>

changshuchao mentioned this pull request Feb 5, 2021

Cherry pick of #98254:Fix the kube-scheduler binary's description of … #98786

Merged

changshuchao mentioned this pull request Feb 8, 2021

Cherry pick of #98254 for 1.19: Fix the kube-scheduler binary's description of the --config parameter is inaccurate #98872

Closed

changshuchao mentioned this pull request Feb 8, 2021

Cherry pick of #98254 for 1.19: Fix the kube-scheduler binary's descr… #98873

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2021

Merge pull request #98873 from changshuchao/cherry-pick-of-scheduler-…

66f4806

…1-19 Cherry pick of #98254 for 1.19: Fix the kube-scheduler binary's descr…

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2021

Merge pull request #98786 from changshuchao/cherry-pick-of-scheduler

aaf4c5c

Cherry pick of #98254:Fix the kube-scheduler binary's description of …

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the kube-scheduler binary's description of the --config parameter… #98254

Fix the kube-scheduler binary's description of the --config parameter… #98254

changshuchao commented Jan 21, 2021 •

edited by liggitt

Loading

k8s-ci-robot commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

changshuchao commented Jan 21, 2021

changshuchao commented Jan 21, 2021

alculquicondor commented Jan 21, 2021

alculquicondor Jan 21, 2021

damemi Jan 25, 2021

alculquicondor Jan 21, 2021

damemi Jan 21, 2021

damemi left a comment

changshuchao commented Jan 22, 2021

damemi left a comment

damemi Jan 22, 2021

alculquicondor Jan 22, 2021

changshuchao Jan 23, 2021

changshuchao Jan 23, 2021 •

edited

Loading

damemi Jan 25, 2021

changshuchao Jan 26, 2021

damemi commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

alculquicondor left a comment

changshuchao commented Feb 1, 2021

damemi commented Feb 1, 2021

changshuchao commented Feb 2, 2021 •

edited

Loading

damemi commented Feb 2, 2021

changshuchao commented Feb 3, 2021

damemi commented Feb 3, 2021

alculquicondor commented Feb 3, 2021

alculquicondor commented Feb 3, 2021

changshuchao commented Feb 4, 2021

alculquicondor commented Feb 4, 2021

changshuchao commented Feb 5, 2021 •

edited

Loading

	fs.StringVar(&o.ConfigFile, "config", o.ConfigFile, `The path to the configuration file. The following flags can overwrite fields in this file:
	--address
	--port
	--use-legacy-policy-config
	--policy-configmap
	--policy-config-file
	--algorithm-provider`)

Fix the kube-scheduler binary's description of the --config parameter… #98254

Fix the kube-scheduler binary's description of the --config parameter… #98254

Conversation

changshuchao commented Jan 21, 2021 • edited by liggitt Loading

k8s-ci-robot commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

changshuchao commented Jan 21, 2021

changshuchao commented Jan 21, 2021

alculquicondor commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi left a comment

Choose a reason for hiding this comment

changshuchao commented Jan 22, 2021

damemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changshuchao Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

changshuchao commented Feb 1, 2021

damemi commented Feb 1, 2021

changshuchao commented Feb 2, 2021 • edited Loading

damemi commented Feb 2, 2021

changshuchao commented Feb 3, 2021

damemi commented Feb 3, 2021

alculquicondor commented Feb 3, 2021

alculquicondor commented Feb 3, 2021

changshuchao commented Feb 4, 2021

alculquicondor commented Feb 4, 2021

changshuchao commented Feb 5, 2021 • edited Loading

changshuchao commented Jan 21, 2021 •

edited by liggitt

Loading

changshuchao Jan 23, 2021 •

edited

Loading

changshuchao commented Feb 2, 2021 •

edited

Loading

changshuchao commented Feb 5, 2021 •

edited

Loading