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

Huang-Wei · 2021-10-26T17:12:35Z

What type of PR is this?

/kind regression
/sig scheduling

What this PR does / why we need it:

The logic of binding CLI args to CC fields brings maintenance burden due to the following limitations:

instantiation of a CC obj should happen after CLI args parsing (instantiation of some plugins depend on feature gates)
some CLI args (leader election ones) are favored over CC fields
some CLI args (deprecated options) take precedence from CC fields if --config is specified; otherwise default values are adopted

This has caused bugs (#103440, #105704) and now regressions (#105789). So this PR tries to fix the problem thoroughly by breaking the binding from CLI args to CC field.

Which issue(s) this PR fixes:

Fixes #105704

Special notes for your reviewer:

This PR should be back-ported to 1.22.

Does this PR introduce a user-facing change?

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

k8s-ci-robot · 2021-10-26T17:12:42Z

@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.

Huang-Wei · 2021-10-26T17:12:49Z

/cc @ahg-g @alculquicondor

Huang-Wei · 2021-10-26T17:13:08Z

/priority critical-urgent

k8s-ci-robot · 2021-10-26T17:13:21Z

[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

Huang-Wei · 2021-10-26T17:52:44Z

/retest

alculquicondor · 2021-10-26T18:12:50Z

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

+	fs.BoolVar(&o.EnableContentionProfiling, "contention-profiling", false, "DEPRECATED: enable lock contention profiling, if profiling is enabled. This parameter is ignored if a config file is specified in --config.")
+	fs.StringVar(&o.Kubeconfig, "kubeconfig", "", "DEPRECATED: path to kubeconfig file with authorization and master location information. This parameter is ignored if a config file is specified in --config.")
+	fs.StringVar(&o.ContentType, "kube-api-content-type", "", "DEPRECATED: content type of requests sent to apiserver. This parameter is ignored if a config file is specified in --config.")
+	fs.Float32Var(&o.QPS, "kube-api-qps", 0, "DEPRECATED: QPS to use while talking with kubernetes apiserver. This parameter is ignored if a config file is specified in --config.")


The default value is not part of the output when doing --help?

Fair point. Updated the code to include default values.

alculquicondor · 2021-10-26T18:20:37Z

I wonder if we need a more targeted fix that we can more easily backport. And leave this PR just for 1.23

ahg-g · 2021-10-26T20:17:57Z

also, I think we should remove all deprecated flags.

Huang-Wei · 2021-10-26T20:31:30Z

also, I think we should remove all deprecated flags.

Can we remove them in 1.23? If so, I can come up with a followup PR. This one should still be needed, as 1.22 have those deprecated options.

Huang-Wei · 2021-10-26T20:37:37Z

I wonder if we need a more targeted fix that we can more easily backport. And leave this PR just for 1.23

The bug lay in 1.22 is that leader elect and deprecated options are not honored. A "targeted" fix would be also non-trivial IMO, so I'm not sure it is worth the extra efforts.

alculquicondor · 2021-10-26T20:48:36Z

Just checking IIUC: the reason for the regression is that the feature gates are not loaded by the time we create the component config, correct?

alculquicondor · 2021-10-26T20:51:38Z

can we first revert #105712 in master (as it's urgent) and then have this PR (to be bakported) have the 2 commits?

I think it would be useful to review the overall delta.

Huang-Wei · 2021-10-26T22:17:04Z

Just checking IIUC: the reason for the regression is that the feature gates are not loaded by the time we create the component config, correct?

Yes.

can we first revert #105712 in master (as it's urgent) and then have this PR (to be bakported) have the 2 commits?

Do you mean to submit a separate PR to revert #105712, so it resolves the urgent CI failure (#105789). And then update this PR to resolve the leader election issue (#105704) thoroughly?

alculquicondor · 2021-10-26T23:34:17Z

Yes, that's what I mean. Wdyt?

…

On Tue., Oct. 26, 2021, 6:17 p.m. Wei Huang, ***@***.***> wrote: Just checking IIUC: the reason for the regression is that the feature gates are not loaded by the time we create the component config, correct? Yes. can we first revert #105712 <#105712> in master (as it's urgent) and then have this PR (to be bakported) have the 2 commits? Do you mean to submit a separate PR to revert #105712 <#105712>, so it resolves the urgent CI failure (#105789 <#105789>). And then update this PR to resolve the leader election issue (#105704 <#105704>) thoroughly? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#105915 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ5E6BTU6ZZZJCHB4PNCWDUI4SGXANCNFSM5GYKIWFQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

Huang-Wei · 2021-10-26T23:36:57Z

Yes, that's what I mean. Wdyt?

Make sense to me.

Huang-Wei · 2021-10-26T23:37:12Z

/retest

ahg-g · 2021-10-27T14:38:09Z

Can we remove them in 1.23? If so, I can come up with a followup PR. This one should still be needed, as 1.22 have those deprecated options.

I think we can, they have been in deprecated status for a long time. We already removed a bunch earlier this cycle.

Huang-Wei · 2021-10-27T18:08:40Z

I think we can, they have been in deprecated status for a long time. We already removed a bunch earlier this cycle.

SG, I will raise a follow-up PR.

@alculquicondor The reverting PR has been merged. This PR will act as a pure fix and backporting candidate.

alculquicondor · 2021-10-28T14:39:44Z

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

-	// mapped into componentconfig.KubeSchedulerConfiguration.
+	componentbaseconfig.DebuggingConfiguration
+	componentbaseconfig.ClientConnectionConfiguration
+	componentbaseconfig.LeaderElectionConfiguration


So some leader election flags don't take precedence, right?

Can you add a comment?

It's a bit tricky here. Two deprecated flags (--lock-object-name and --lock-object-namespace) overlapped with the regular non-deprecated options, so here LeaderElectionConfiguration only populates the deprecated ones.

Will add a comment here.

alculquicondor · 2021-10-28T14:45:06Z

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

 	o := &Options{
 		SecureServing:  apiserveroptions.NewSecureServingOptions().WithLoopback(),
 		Authentication: apiserveroptions.NewDelegatingAuthenticationOptions(),
 		Authorization:  apiserveroptions.NewDelegatingAuthorizationOptions(),
 		Deprecated:     &DeprecatedOptions{},
-		Metrics:        metrics.NewOptions(),
-		Logs:           logs.NewOptions(),
+		LeaderElection: &componentbaseconfig.LeaderElectionConfiguration{


doesn't componentbaseconfig offer a defaulting function? let's use that.

I thought of it. There are a couple of shortcomings:

componentbaseconfigv1alpha1 (not componentbaseconfig) offers a defaulting function to build a versioned obj. We use unversioned obj here, so need extra code to do the conversion bits.

the defaulting logic in componentbaseconfigv1alpha1 still uses "endpoints" as the default lock name, however, it should be "leases" in latest k8s versions.

WDYT?

alculquicondor · 2021-10-28T14:54:20Z

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

 	"k8s.io/kubernetes/pkg/scheduler/apis/config/validation"
 	netutils "k8s.io/utils/net"
 )

 // Options has all the params needed to run a Scheduler
 type Options struct {
 	// The default values.
-	ComponentConfig kubeschedulerconfig.KubeSchedulerConfiguration
+	ComponentConfig *kubeschedulerconfig.KubeSchedulerConfiguration


was this change absolutely necessary?

We better have this as a defaulting CC obj is no longer offered in the very beginning, and hence if needed (like in some tests so far), we can use if cc == nil to compose it. On the other hand, it's aligned with other structs in Options.

alculquicondor · 2021-10-28T15:04:08Z

cmd/kube-scheduler/app/server.go

@@ -96,15 +89,17 @@ for more information about scheduling and the kube-scheduler component.`,
 		},
 	}

+	opts.InitFlags()


Can we just call this as part of New?

yup, good suggestion.

Huang-Wei · 2021-11-01T17:13:39Z

Comments addressed. PTAL.

alculquicondor · 2021-11-02T19:23:09Z

/lgtm

alculquicondor · 2021-11-02T19:23:40Z

/label tide/merge-method-squash

Huang-Wei · 2021-11-03T00:56:49Z

Oops, the unit tests in the reverted weren't added in this PR. I will raise a PR to add those UTs back.

…05915-#106105-upstream-release-1.22 Automated cherry pick of #105915: sched: ensure feature gate is honored when instantiating #106105: Add unit tests to cover scheduler's setup

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 26, 2021

k8s-ci-robot requested review from ahg-g and alculquicondor October 26, 2021 17:12

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 26, 2021

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

k8s-ci-robot requested review from chendave and damemi October 26, 2021 17:14

alculquicondor reviewed Oct 26, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021

Huang-Wei force-pushed the fix-feature-gate-dep branch from dd11c3a to f3c7cc7 Compare October 26, 2021 20:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021

Huang-Wei mentioned this pull request Oct 26, 2021

Revert "sched: ensure --leader-elect* CLI args are honored" #105924

Merged

Huang-Wei force-pushed the fix-feature-gate-dep branch from f3c7cc7 to 93e3c19 Compare October 26, 2021 22:34

Huang-Wei force-pushed the fix-feature-gate-dep branch from 93e3c19 to 3070c71 Compare October 27, 2021 18:07

alculquicondor reviewed Oct 28, 2021

View reviewed changes

Huang-Wei added 2 commits October 28, 2021 11:23

sched: ensure feature gate is honored when instantiating scheduler

7dcd3cd

fixup: address comments

16362c8

Huang-Wei force-pushed the fix-feature-gate-dep branch from 3070c71 to 16362c8 Compare October 28, 2021 18:23

k8s-ci-robot assigned alculquicondor Nov 2, 2021

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 2, 2021

k8s-ci-robot merged commit e30f964 into kubernetes:master Nov 2, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 2, 2021

Huang-Wei deleted the fix-feature-gate-dep branch November 2, 2021 21:35

Huang-Wei mentioned this pull request Nov 3, 2021

Add unit tests to cover scheduler's setup #106105

Merged

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 Nov 3, 2021

Huang-Wei mentioned this pull request Nov 3, 2021

Automated cherry pick of #105915: sched: ensure feature gate is honored when instantiating #106105: Add unit tests to cover scheduler's setup #106130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Huang-Wei commented Oct 26, 2021 •

edited

k8s-ci-robot commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

k8s-ci-robot commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

alculquicondor Oct 26, 2021

Huang-Wei Oct 26, 2021

alculquicondor commented Oct 26, 2021

ahg-g commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

alculquicondor commented Oct 26, 2021

alculquicondor commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

alculquicondor commented Oct 26, 2021 via email

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

ahg-g commented Oct 27, 2021

Huang-Wei commented Oct 27, 2021

alculquicondor Oct 28, 2021

Huang-Wei Oct 28, 2021 •

edited

alculquicondor Oct 28, 2021

Huang-Wei Oct 28, 2021

alculquicondor Oct 28, 2021

Huang-Wei Oct 28, 2021

alculquicondor Oct 28, 2021

Huang-Wei Oct 28, 2021

Huang-Wei commented Nov 1, 2021

alculquicondor commented Nov 2, 2021

alculquicondor commented Nov 2, 2021

Huang-Wei commented Nov 3, 2021

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

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

Conversation

Huang-Wei commented Oct 26, 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 26, 2021

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

k8s-ci-robot commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Oct 26, 2021

ahg-g commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

alculquicondor commented Oct 26, 2021

alculquicondor commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

alculquicondor commented Oct 26, 2021 via email

Huang-Wei commented Oct 26, 2021

Huang-Wei commented Oct 26, 2021

ahg-g commented Oct 27, 2021

Huang-Wei commented Oct 27, 2021

Choose a reason for hiding this comment

Huang-Wei Oct 28, 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

Huang-Wei commented Nov 1, 2021

alculquicondor commented Nov 2, 2021

alculquicondor commented Nov 2, 2021

Huang-Wei commented Nov 3, 2021

Huang-Wei commented Oct 26, 2021 •

edited

Huang-Wei Oct 28, 2021 •

edited