sched: ensure --leader-elect* CLI args are honored #105712

Huang-Wei · 2021-10-16T03:34:54Z

What type of PR is this?

/kind bug
/sig scheduling

What this PR does / why we need it:

--leader-elect* consists of a collection of CLI arguments. They were handled as individuals which ends up with buggy behavior like what #105704 described - CLI arg gets overwritten by internal defaulted ComponentConfig.LeaderElection. Moreover, some other potential bugs can also occur.

Basically, there are 3 scenarios to specify leader elect related options:

by CLI arguments only
by ComponentConfig only
jointly by CLI arguments and ComponentConfig

This PR unifies the behavior to always honor CLI arguments over ComponentConfig.

Which issue(s) this PR fixes:

Fixes #105704

Special notes for your reviewer:

The fix should be backported to v1.22.

Does this PR introduce a user-facing change?

The --leader-elect* CLI args are now honored in scheduler.

k8s-ci-robot · 2021-10-16T03:35:00Z

@Huang-Wei: 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-10-16T03:35:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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~~ [Huang-Wei]

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

chendave · 2021-10-18T09:50:32Z

jointly by CLI arguments and ComponentConfig

sound like the issue is only occurred with this case, so is that better to compare the CLI args with the ComponentConfig and throw some warning if it it not consistent?

alculquicondor · 2021-10-18T13:42:47Z

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

+	if leaderelection.Changed("leader-elect") {
+		// Only honor other leader elect arguments when --leader-elect is set to true.
+		// Otherwise, invalidate the entire CC object.
+		if o.ComponentConfig.LeaderElection.LeaderElect {


why isn't this handled by the function that processes LeaderElection? Wouldn't that function break early when LeaderElect is false?

I think we shouldn't have "business-logic" in the arguments parsing.

Good question.

The old logic checks if leaderlection is nil, and compose a leader election obj as the final step to scheduler:

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

Lines 199 to 200 in 4c9c761

// If leader election is enabled, runCommand via LeaderElector until done and exit.

if cc.LeaderElection != nil {

But this is problematic if the passed-in leaderlection is already incorrect, which is exactly the case of this issue, triggered by providing a CC config file:

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

Lines 181 to 191 in a78e313

} else {

cfg, err := loadConfigFromFile(o.ConfigFile)

if err != nil {

return err

}

if err := validation.ValidateKubeSchedulerConfiguration(cfg); err != nil {

return err

}

c.ComponentConfig = *cfg

}

The above logic discarded all pre-parsed leaderlection object, just use the loaded leaderlection from the CC config file.

So in the PR, I delayed the "correction" step (Complete()) after loadConfigFromFile(), to build up an accurate leaderelection obj.

I think we shouldn't have "business-logic" in the arguments parsing.

Is that a practice commonly adopted? If yes, I can adapt to it. In that case, the tests (we'd expect semi-correct obj) need to be updated as well. Let me know your thought.

What if we make makeLeaderElectionConfig in options.go just check the boolean?

What if we make makeLeaderElectionConfig in options.go just check the boolean?

That doesn't make a difference. With this PR, the boolean (LeaderElection.LeaderElect) value is already set correctly. It's just we should compose other portions strickly following the user's input even if it's legally inaccurate, or do some tailoring.

I see.
If you have delayed the call to Complete, isn't that all we need?

What I'm saying is that we can keep this function simple, always override all the flags if they are present.

Although, I thought we had other flags that took precedence over component config. If we don't anymore, perhaps this is working as expected and we just need to clarify in the flags that they are ignored if component config is specified?

If you have delayed the call to Complete, isn't that all we need?

yes.

Also, isn't this change also making the "deprecated" flags take precedence over component config?

You're right, deprecated flags should be ignored if --config is provided. I will make some changes.

BTW: the doc says "This parameter is ignored if a config file is specified in --config.". It's more accurate to be worded as "This parameter is ignored if a config file is specified" because a bare config file would carry default values and those values will be favored over deprecated values, correct?

Yes, it will carry default values. I don't see a significant difference between the wordings. It talks about the config file, not necessarily that the field is explicitly enabled.

I'm wondering: is this a behavior that actually changed in 1.22? Or were leader elect flags already ignored if the config file was specified? If that's the case, the correct action would be to update the help texts.

I'm wondering: is this a behavior that actually changed in 1.22?

I can confirm leader elect was honored in 1.21.

Updated the code. PTAL.

Now we only need to particularly honor the CLI args that may conflict with CC args - i.e., just leader election args. For deprecated args, if --config file is not specified, they should be already filled automatically in cobra command parsing, which happens after instantiating a defaulting object. The current args evaluation flow is like this:

step 1: instantiate a latest default CC obj

step 2: (auto) override both deprecated and non-deprecated args to the aforementioned CC obj

step 3.1: if --config is not specified, do nothing but keep the CC obj

step 3.2: if --config is specified, compose a temp CC' obj by loading the config file. After that, apply the non-deprecated args and assign CC' to CC.

step 4: use the CC obj to build scheduler instance

alculquicondor · 2021-10-18T13:45:10Z

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

-	if leaderelection.Changed("leader-elect-resource-name") {
-		cfg.LeaderElection.ResourceName = o.ComponentConfig.LeaderElection.ResourceName
+	obtainLeaderElectionFn := func() {
+		if leaderelection.Changed("leader-elect-lease-duration") {


Can you remind me why we need to use Changed?

It's b/c when doing CLI parsing, without Changed, we don't know if a false leaderlection value is specified by the user, or a CLI defaulted value:

kubernetes/staging/src/k8s.io/component-base/config/options/leaderelectionconfig.go

Lines 24 to 29 in a78e313

// BindLeaderElectionFlags binds the LeaderElectionConfiguration struct fields to a flagset

func BindLeaderElectionFlags(l *config.LeaderElectionConfiguration, fs *pflag.FlagSet) {

fs.BoolVar(&l.LeaderElect, "leader-elect", l.LeaderElect, ""+

"Start a leader election client and gain leadership before "+

"executing the main loop. Enable this when running replicated "+

"components for high availability.")

Huang-Wei · 2021-10-18T18:51:27Z

sound like the issue is only occurred with this case, so is that better to compare the CLI args with the ComponentConfig and throw some warning if it it not consistent?

Not quite. Some potential issues (like by specifying --leader-elect=false only) are just not reported yet. I can add a sub-test without specifying --config.

chendave · 2021-10-19T09:39:27Z

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

-	// Obtain CLI args related with leaderelection. Set them to cfg if specified in command line.
-	leaderelection := nfs.FlagSet("leader election")
+func (o *Options) Complete(cfg *kubeschedulerconfig.KubeSchedulerConfiguration) {
+	// Obtain non-deprecated CLI args that may conflict with ComponentConfig fields.


more precisely, the non-deprecated CLI here just means leaderelection, the original comments looks more accurate.

If only the leaderelection is the special one, it should be able to check is there any conflict between the CLI and config file, and show us something in the log or the console, not sure if it's worth it as we should also consider the defaults.

more precisely, the non-deprecated CLI here just means leaderelection, the original comments looks more accurate.

I can use the original comment.

If only the leaderelection is the special one, it should be able to check is there any conflict between the CLI and config file, and show us something in the log or the console, not sure if it's worth it as we should also consider the defaults.

I don't see a big value here. We can evaluate whether the introduced complicity (you'll have to do comparings) outweighs the benefits later. But in any way, we shouldn't include it in this PR which is a bug fix and will be back-ported.

I agree. Can you confirm if LogOrWriteConfig includes the changes overridden by the flags?

@alculquicondor Yes, I can confirm that.

alculquicondor · 2021-10-19T13:48:15Z

cmd/kube-scheduler/app/server.go

@@ -80,9 +79,6 @@ kube-scheduler is the reference implementation.
 See [scheduling](https://kubernetes.io/docs/concepts/scheduling-eviction/)
 for more information about scheduling and the kube-scheduler component.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if err := opts.Complete(&namedFlagSets); err != nil {


So we don't need to call this for runs without component config?

Not any more. Now NewOptions() will obtain a latest defaulting obj, and the logic of honoring CLI args get placed after loading --config.

Huang-Wei · 2021-10-19T18:42:36Z

/retest

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

alculquicondor · 2021-10-19T19:02:09Z

cmd/kube-scheduler/app/server_test.go

@@ -193,6 +222,124 @@ profiles:
 				},
 			},
 		},
+		{
+			name: "leader election arg set to false, along with --config arg",


I think we can have a single test case will all the flags that should override values. Well, one with --config and one without.

SG. sub-tests are merged now.

alculquicondor · 2021-10-19T19:02:28Z

cmd/kube-scheduler/app/server_test.go

+			},
+		},
+		{
+			name: "leader election settings specified by ComponentConfig only",


This case and the one below are good. They should stay

alculquicondor

lgtm, you can squash

chendave · 2021-10-20T02:22:18Z

/lgtm

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 16, 2021

k8s-ci-robot requested review from alculquicondor and chendave October 16, 2021 03:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2021

alculquicondor reviewed Oct 18, 2021

View reviewed changes

chendave reviewed Oct 19, 2021

View reviewed changes

alculquicondor reviewed Oct 19, 2021

View reviewed changes

Huang-Wei force-pushed the honor-leader-elect branch from 4ce2e17 to 56d098c Compare October 19, 2021 16:51

alculquicondor reviewed Oct 19, 2021

View reviewed changes

sched: ensure --leader-elect* CLI args are honored

3c230af

Huang-Wei force-pushed the honor-leader-elect branch from 936a9fa to 3c230af Compare October 19, 2021 20:56

k8s-ci-robot assigned chendave Oct 20, 2021

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

k8s-ci-robot merged commit 4bb31b5 into kubernetes:master Oct 20, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 20, 2021

Huang-Wei deleted the honor-leader-elect branch October 20, 2021 16:16

Huang-Wei mentioned this pull request Oct 20, 2021

Automated cherry pick of #105712: sched: ensure --leader-elect* CLI args are honored #105792

Closed

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

This was referenced Oct 25, 2021

[Failing Job] gce-cos-master-alpha-features #105789

Closed

sched: ensure feature gate is honored when instantiating scheduler #105915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sched: ensure --leader-elect* CLI args are honored #105712

sched: ensure --leader-elect* CLI args are honored #105712

Huang-Wei commented Oct 16, 2021 •

edited

k8s-ci-robot commented Oct 16, 2021

k8s-ci-robot commented Oct 16, 2021

chendave commented Oct 18, 2021

alculquicondor Oct 18, 2021

Huang-Wei Oct 18, 2021

alculquicondor Oct 18, 2021

Huang-Wei Oct 18, 2021 •

edited

alculquicondor Oct 18, 2021

Huang-Wei Oct 18, 2021

Huang-Wei Oct 18, 2021

alculquicondor Oct 18, 2021

Huang-Wei Oct 18, 2021

Huang-Wei Oct 18, 2021

alculquicondor Oct 18, 2021

Huang-Wei Oct 18, 2021

Huang-Wei commented Oct 18, 2021 •

edited

chendave Oct 19, 2021

Huang-Wei Oct 19, 2021

alculquicondor Oct 19, 2021

Huang-Wei Oct 19, 2021

alculquicondor Oct 19, 2021

Huang-Wei Oct 19, 2021

Huang-Wei commented Oct 19, 2021

alculquicondor Oct 19, 2021

Huang-Wei Oct 19, 2021

alculquicondor Oct 19, 2021

alculquicondor left a comment

chendave commented Oct 20, 2021

	// If leader election is enabled, runCommand via LeaderElector until done and exit.
	if cc.LeaderElection != nil {

	} else {
	cfg, err := loadConfigFromFile(o.ConfigFile)
	if err != nil {
	return err
	}
	if err := validation.ValidateKubeSchedulerConfiguration(cfg); err != nil {
	return err
	}

	c.ComponentConfig = *cfg
	}

	// BindLeaderElectionFlags binds the LeaderElectionConfiguration struct fields to a flagset
	func BindLeaderElectionFlags(l config.LeaderElectionConfiguration, fs pflag.FlagSet) {
	fs.BoolVar(&l.LeaderElect, "leader-elect", l.LeaderElect, ""+
	"Start a leader election client and gain leadership before "+
	"executing the main loop. Enable this when running replicated "+
	"components for high availability.")

sched: ensure --leader-elect* CLI args are honored #105712

sched: ensure --leader-elect* CLI args are honored #105712

Conversation

Huang-Wei commented Oct 16, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 16, 2021

k8s-ci-robot commented Oct 16, 2021

chendave commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei Oct 18, 2021 • edited

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

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

Choose a reason for hiding this comment

Huang-Wei commented Oct 18, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

chendave commented Oct 20, 2021

Huang-Wei commented Oct 16, 2021 •

edited

Huang-Wei Oct 18, 2021 •

edited

Huang-Wei commented Oct 18, 2021 •

edited