custom plugin config should take precedence over default plugin config #99582

chendave · 2021-03-01T10:32:53Z

Signed-off-by: Dave Chen dave.chen@arm.com

What type of PR is this?

Add one of the following kinds:
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #99580

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-scheduler: a plugin enabled in a v1beta2 configuration file takes precedence over the default configuration for that plugin; this simplifies enabling default plugins with custom configuration without needing to explicitly disable those default plugins.

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

k8s-ci-robot · 2021-03-01T10:33:00Z

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

chendave · 2021-03-01T10:33:15Z

/sig scheduling

chendave · 2021-03-01T10:45:03Z

/cc @Huang-Wei @ahg-g @alculquicondor

pkg/scheduler/apis/config/types.go

Huang-Wei · 2021-03-06T00:39:06Z

pkg/scheduler/apis/config/types.go


 	var enabledPlugins []Plugin
 	if !disabledPlugins.Has("*") {
 		for _, defaultEnabledPlugin := range defaultPluginSet.Enabled {
-			if disabledPlugins.Has(defaultEnabledPlugin.Name) {
+			// custom plugin config will take precedence over default plugin config
+			if disabledPlugins.Has(defaultEnabledPlugin.Name) || enabledCustomPlugin.Has(defaultEnabledPlugin.Name) {


(a bit off the topic)

@alculquicondor no matter with or w/o this PR, a lightweight customized plugin may be placed in the end, am I understanding right?

chendave · 2021-03-08T10:03:28Z

@alculquicondor @Huang-Wei , I managed to reserve both the order of default and custom plugins.

Current code implementation and testcase, "InsertBeforeAllPlugins" for example, show me that the overwrite of the default plugin config is allowed, so this PR intends to release us from explicitly disable the default plugins and then re-enable it again, which sound like a hack to me, this will help reduce the surprise to end user.

chendave · 2021-03-08T13:09:34Z

/retest

chendave · 2021-03-15T02:38:04Z

/remove-kind bug
/kind feature

fejta-bot · 2021-06-13T03:11:58Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

chendave · 2021-06-16T01:36:20Z

/remove-lifecycle stale

chendave · 2021-07-06T10:38:54Z

/retest

alculquicondor

Since you are changing the interpretation of PluginSet.Enabled, you need to update the documentation for the field in k8s.io/kube-scheduler/config/v1beta2/types.go

alculquicondor · 2021-07-06T13:29:05Z

pkg/scheduler/apis/config/v1beta2/default_plugins.go

 	for _, disabledPlugin := range customPluginSet.Disabled {
 		disabledPlugins.Insert(disabledPlugin.Name)
 	}
-
+	for index, enabledPlugin := range customPluginSet.Enabled {
+		enabledCustomPlugins[enabledPlugin.Name] = pluginIndex{index, enabledPlugin}


only the first repetition?

true, seems like we'd better to validate this config to avoid the case of repetition, will fix it in my next day!

recall that code freeze is on Thursday

because mergePlugins is done before the validation, so the repetition here is still possible, I'd suggest to just log something here or revert to the original version, only the last repetition will be valid for either way.
I am a little lost here, do you have any suggestion?

You can keep it like this, it doesn't really matter because more than one repetition will fail later.

Fix the types.go though

alculquicondor · 2021-07-06T13:29:44Z

pkg/scheduler/apis/config/v1beta2/default_plugins.go

-
+			// The default plugin is explicitly re-configured, update the default plugin accordingly.
+			if customPlugin, ok := enabledCustomPlugins[defaultEnabledPlugin.Name]; ok {
+				klog.InfoS("Defaut plugin is explicitly re-configured, will be overridden.", "plugin", defaultEnabledPlugin.Name)


use present tense: overriding

Signed-off-by: Dave Chen <dave.chen@arm.com>

alculquicondor · 2021-07-06T15:26:14Z

You probably need to run make update

chendave · 2021-07-06T15:27:42Z

You probably need to run make update

I did, nothing was changed.

chendave · 2021-07-06T15:28:28Z

only run make generated_files which I think should be enough.

alculquicondor · 2021-07-06T16:52:41Z

/approve
/lgtm

alculquicondor · 2021-07-06T16:52:53Z

/label api-review
/retest

liggitt · 2021-07-06T23:17:18Z

does this fix only apply to v1beta2 configs? if so, should that be noted in the release note?

liggitt · 2021-07-06T23:18:25Z

My understanding is that this makes config files work which would previously fail validation (because they didn't explicitly disable the default plugin they wanted to explicitly enable), and would have no impact on config files which were previously working. Is that correct?

chendave · 2021-07-06T23:41:21Z

does this fix only apply to v1beta2 configs? if so, should that be noted in the release note?

Just updated

chendave · 2021-07-06T23:45:37Z

My understanding is that this makes config files work which would previously fail validation (because they didn't explicitly disable the default plugin they wanted to explicitly enable), and would have no impact on config files which were previously working. Is that correct?

yes, no impact on the previous config file, this PR does some enhancement only if the default plugin is not disabled by *, and thus more flexible for end-user to config, and avoid unnecessary failure if there is no config confliction with default plugin (could be overridden).

liggitt · 2021-07-07T01:15:50Z

/approve

for API bits

k8s-ci-robot · 2021-07-07T01:16:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, chendave, liggitt

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:

~~pkg/scheduler/apis/config/OWNERS~~ [alculquicondor,liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]

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

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 1, 2021

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 Mar 1, 2021

k8s-ci-robot requested review from andrewsykim and mwielgus March 1, 2021 10:33

k8s-ci-robot requested review from ahg-g, alculquicondor and Huang-Wei March 1, 2021 10:45

Huang-Wei reviewed Mar 6, 2021

View reviewed changes

chendave force-pushed the fix_config branch from 8ce8542 to 274f6a8 Compare March 8, 2021 09:56

chendave force-pushed the fix_config branch from 274f6a8 to 0da7065 Compare March 8, 2021 10:07

Huang-Wei mentioned this pull request Mar 8, 2021

scheduler failed to start due to the custom plugin config in scheduler config file #99580

Closed

chendave mentioned this pull request Mar 11, 2021

Enable scheduler_perf to support scheduler config file #99638

Merged

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 15, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 13, 2021

chendave force-pushed the fix_config branch from 7f5bda6 to d959d82 Compare July 6, 2021 09:56

alculquicondor reviewed Jul 6, 2021

View reviewed changes

Custom plugin config should take precedence over default plugin config

9a5237c

Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the fix_config branch from d959d82 to 9a5237c Compare July 6, 2021 15:20

k8s-ci-robot assigned alculquicondor Jul 6, 2021

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

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 6, 2021

liggitt added this to In progress in API Reviews Jul 6, 2021

liggitt self-assigned this Jul 6, 2021

liggitt added this to the v1.22 milestone Jul 6, 2021

liggitt moved this from In progress to API review completed, 1.22 in API Reviews Jul 7, 2021

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

k8s-ci-robot merged commit 7df432f into kubernetes:master Jul 7, 2021

alculquicondor mentioned this pull request Jul 7, 2021

kube-scheduler: ensure the default config output of --write-to-config is usable #103327

Merged

chendave mentioned this pull request Jul 8, 2021

scheduler: index out of range #103572

Closed

sanposhiho mentioned this pull request Jul 28, 2021

Scheduler: make KubeSchedulerConfiguration override the default plugins configuration #103956

Closed

chendave deleted the fix_config branch September 3, 2021 02:30

alculquicondor mentioned this pull request Oct 12, 2021

Migration guide between KubeSchedulerConfig versions kubernetes/website#30063

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom plugin config should take precedence over default plugin config #99582

custom plugin config should take precedence over default plugin config #99582

chendave commented Mar 1, 2021 •

edited by liggitt

Loading

k8s-ci-robot commented Mar 1, 2021

chendave commented Mar 1, 2021

chendave commented Mar 1, 2021

Huang-Wei Mar 6, 2021

chendave commented Mar 8, 2021

chendave commented Mar 8, 2021

chendave commented Mar 15, 2021

fejta-bot commented Jun 13, 2021

chendave commented Jun 16, 2021

chendave commented Jul 6, 2021

alculquicondor left a comment

alculquicondor Jul 6, 2021

chendave Jul 6, 2021

alculquicondor Jul 6, 2021

chendave Jul 6, 2021

alculquicondor Jul 6, 2021

alculquicondor Jul 6, 2021

alculquicondor commented Jul 6, 2021

chendave commented Jul 6, 2021

chendave commented Jul 6, 2021

alculquicondor commented Jul 6, 2021

alculquicondor commented Jul 6, 2021

liggitt commented Jul 6, 2021

liggitt commented Jul 6, 2021

chendave commented Jul 6, 2021

chendave commented Jul 6, 2021 •

edited

Loading

liggitt commented Jul 7, 2021 •

edited

Loading

k8s-ci-robot commented Jul 7, 2021

custom plugin config should take precedence over default plugin config #99582

custom plugin config should take precedence over default plugin config #99582

Conversation

chendave commented Mar 1, 2021 • edited by liggitt Loading

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?

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

k8s-ci-robot commented Mar 1, 2021

chendave commented Mar 1, 2021

chendave commented Mar 1, 2021

Huang-Wei Mar 6, 2021

Choose a reason for hiding this comment

chendave commented Mar 8, 2021

chendave commented Mar 8, 2021

chendave commented Mar 15, 2021

fejta-bot commented Jun 13, 2021

chendave commented Jun 16, 2021

chendave commented Jul 6, 2021

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Jul 6, 2021

Choose a reason for hiding this comment

chendave Jul 6, 2021

Choose a reason for hiding this comment

alculquicondor Jul 6, 2021

Choose a reason for hiding this comment

chendave Jul 6, 2021

Choose a reason for hiding this comment

alculquicondor Jul 6, 2021

Choose a reason for hiding this comment

alculquicondor Jul 6, 2021

Choose a reason for hiding this comment

alculquicondor commented Jul 6, 2021

chendave commented Jul 6, 2021

chendave commented Jul 6, 2021

alculquicondor commented Jul 6, 2021

alculquicondor commented Jul 6, 2021

liggitt commented Jul 6, 2021

liggitt commented Jul 6, 2021

chendave commented Jul 6, 2021

chendave commented Jul 6, 2021 • edited Loading

liggitt commented Jul 7, 2021 • edited Loading

k8s-ci-robot commented Jul 7, 2021

chendave commented Mar 1, 2021 •

edited by liggitt

Loading

chendave commented Jul 6, 2021 •

edited

Loading

liggitt commented Jul 7, 2021 •

edited

Loading