custom enable or disable of scheduler plugins #2135

chaunceyjiang · 2022-07-05T12:08:11Z

Signed-off-by: chaunceyjiang chaunceyjiang@gmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
custom enable or disable of scheduler plugins

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-scheduler`: Introduced `--plugins` flag to enable or disable scheduler plugins

chaunceyjiang · 2022-07-05T12:14:16Z

Enable scheduler plugin
Disable scheduler plugin

XiShanYongYe-Chang · 2022-07-06T01:32:41Z

Thanks~

/assign

chaunceyjiang · 2022-07-07T02:17:41Z

Hi,how's the progress? @XiShanYongYe-Chang

XiShanYongYe-Chang · 2022-07-07T02:46:54Z

Hi,how's the progress? @XiShanYongYe-Chang

Sorry. I'm going to start reviewing now.

XiShanYongYe-Chang

Good job!

/cc @Poor12

XiShanYongYe-Chang · 2022-07-07T03:12:23Z

pkg/scheduler/framework/runtime/registry.go

+}
+
+// Filter out the disabled plugin
+func (r Registry) Filter(names []string) Registry {


Can we add a ut for this method?

I have a question: if the plugins is -foo,*, dose the result contain foo?

Can we add a ut for this method?

ok.

dose the result contain foo?

no, the result is *

If so, the behavior of --plugins will be different from controllers. I think that the writing habits should be consistent:

https://github.com/karmada-io/karmada/blob/master/docs/userguide/configure-controllers.md#configure-karmada-controllers

We have a similar logic at --controllers in karmada-controller-manager, which is used another way:

karmada/pkg/controllers/context/context.go

Lines 76 to 96 in 821a185

func (c Context) IsControllerEnabled(name string, disabledByDefaultControllers sets.String) bool {

hasStar := false

for _, ctrl := range c.Opts.Controllers {

if ctrl == name {

return true

}

if ctrl == "-"+name {

return false

}

if ctrl == "*" {

hasStar = true

}

}

// if we get here, there was no explicit choice

if !hasStar {

// nothing on by default

return false

}

return !disabledByDefaultControllers.Has(name)

}

.

I'm not sure if we can share some code here, will look at it later.

pkg/scheduler/scheduler.go

chaunceyjiang · 2022-07-08T01:36:16Z

/cc @XiShanYongYe-Chang @Poor12

Poor12 · 2022-07-08T02:22:08Z

/lgtm

XiShanYongYe-Chang · 2022-07-08T02:53:51Z

/lgtm

/cc @RainbowMango @likakuli

karmada-bot · 2022-07-08T02:53:54Z

@XiShanYongYe-Chang: GitHub didn't allow me to request PR reviews from the following users: likakuli.

Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lgtm

/cc @RainbowMango @likakuli

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.

RainbowMango

Generally looks good to me.

Some guys also requested this feature a long time ago but have been postponed as no clear use case at that time.

cc @likakuli
Who might need it?

@chaunceyjiang Can you explain why you need it in your case, and which plugin you want to disable?

cmd/scheduler/app/options/options.go

pkg/scheduler/framework/runtime/registry.go

RainbowMango · 2022-07-08T04:29:25Z

By the way, given the plugin name will be exposed to users, I'd like to request a final review of the plugin names.

karmada/pkg/scheduler/framework/plugins/registry.go

Lines 15 to 19 in 821a185

    
           apiinstalled.Name:     apiinstalled.New, 
        
           tainttoleration.Name:  tainttoleration.New, 
        
           clusteraffinity.Name:  clusteraffinity.New, 
        
           spreadconstraint.Name: spreadconstraint.New, 
        
           clusterlocality.Name:  clusterlocality.New,

Any idea would be welcome!

prodanlabs · 2022-07-08T11:55:22Z

I am thinking that we can provide a karmada-scheduler with only basic plugins, and users can build their own scheduler based on this karmada-scheduler according to their own needs.

We allow karmada to have multiple schedulers. When distributing workloads, a field is used to declare which scheduler to use, such as schedulerName: demo-scheduler

what do you think @RainbowMango @Garrybest @kerthcet

RainbowMango · 2022-07-09T01:40:05Z

yeah, this is one of what we need, another one is to use taint filter instead of directly filtering unready clusters, thx

I remember that(#1854 ), it still on my TODO list :)

RainbowMango · 2022-07-09T01:46:11Z

I am thinking that we can provide a karmada-scheduler with only basic plugins, and users can build their own scheduler based on this karmada-scheduler according to their own needs.

Given not all users need to extend the scheduler on their own, I prefer to enhance scalability(like this PR) and make it easy for users to choose.

Garrybest · 2022-07-09T08:08:15Z

Where the field schedulerName exist? In PropagationPolicy?

RainbowMango · 2022-07-09T08:38:26Z

Where the field schedulerName exist? In PropagationPolicy?

karmada/pkg/apis/policy/v1alpha1/propagation_types.go

Line 85 in 821a185

SchedulerName string `json:"schedulerName,omitempty"`

But the scheduler now does not use it yet. I guess @prodanlabs did it by himself.

Garrybest · 2022-07-09T09:30:44Z

Cool, I like this feature.

kerthcet · 2022-07-10T03:16:16Z

From a high level perspective, managing configurations via flags poses certain problems like they are unversioned, the maintenance burden grows as the number of flags grows, flags exist in a flat namespace which is hard to organize structuring configurations.

Considering we have quite a number of configations in karmada scheduler, and we like to have more in the future, I think we should migrate to Conponent Config gradually. Scheduler plugins can also be part of this.

I think this is where we want to forward, but now, back to the reality, we have rarely plugins in karmada, what we settled in this PR can be a transition plan, since many users are eager for this IIRC.

Regarding to @prodanlabs proposal, different scheduler for different scenario makes sense to me, and we also have a entrance already. If we're planning to move forward, I think Component Config would be a better choice.

prodanlabs · 2022-07-10T03:44:31Z

Regarding the issue of multiple schedulers, we can open another issues discussion.

chaunceyjiang · 2022-07-11T02:10:37Z

So, for this PR, is there anything else I need to update?

@RainbowMango

kerthcet · 2022-07-11T03:09:31Z

I have several questions:

If we don't set --plugins, what's the result, it seems like no plugins is registered?
If not all plugins defined in karmada enabled by default, like we have plugin sets A, B, C, D1, D2, we enable A, B, C, D1 by default, but now, we want A, B, C, D2 enabled, D1 disabled, how to cover this, --plugisn="A,B,C,D2,-D1"?

chaunceyjiang · 2022-07-11T03:18:42Z

If we don't set --plugins, what's the result, it seems like no plugins is registered?

All plugins are enabled by default

RainbowMango · 2022-07-11T10:16:15Z

@chaunceyjiang Just opened #2170 for the naming thing.

So, for this PR, is there anything else I need to update?

I'll get back to this PR after #2170. I'll look at if we have another way to implement it just like I mentioned at #2135 (comment).

RainbowMango · 2022-07-19T02:32:07Z

/assign

RainbowMango

Generally looks good to me.

      --plugins strings                                                                                                                                                                                                          
                A list of plugins to enable. '*' enables all on-by-default plugins, 'foo' enables the plugin named 'foo', '*,-foo' disables the plugin named 'foo'.  
                All build-in plugins: APIEnablement,ClusterAffinity,ClusterLocality,SpreadConstraint,TaintToleration. (default [*])

Just a question, since this flag also applies to customized plugins, should we mention it in usage?

'*' enables all on-by-default plugins -->
'*' enables all build-in and customized plugins

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>

RainbowMango

/lgtm
/approve
Thanks for your quick response.

karmada-bot · 2022-07-20T04:00:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~OWNERS~~ [RainbowMango]

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

chaunceyjiang · 2022-07-20T04:34:52Z

There are resource constraints on GitHub actions in each repo. Now there are too many E2E kind clusters, and contributors are relatively active during the day. The probability of E2E error is too high. It is suggested to reduce the number of E2E kind clusters. Can we cancel the E2E test (v1.21.10)

RainbowMango · 2022-07-20T06:26:00Z

There are resource constraints on GitHub actions in each repo.

Really? Where did you see that?

The probability of E2E error is too high. It is suggested to reduce the number of E2E kind clusters. Can we cancel the E2E test (v1.21.10)

I know @XiShanYongYe-Chang and @mrlihanbo are trying to figure out the reason, but don't really have a clue yet.
Yeah, we can do that if it is the root cause.

chaunceyjiang · 2022-07-20T06:36:33Z

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources

https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#usage-limits

@RainbowMango

RainbowMango · 2022-07-20T07:23:11Z

Yeah, I know the resource limit, but we haven't reached the limit, right?

The e2e tests(v1.21,v1.22,v1.23,v1.24) are run in different virtual machines, they don't affect each other in theory.

XiShanYongYe-Chang · 2022-07-20T07:31:16Z

Hi @chaunceyjiang, our CI is far from reaching the limit of github action. Currently, the failed CIs are mainly in the v1.24 k8s cluster, the cause of the failure is that karmada-apiserver restarts multiple times. Removing the v1.21 k8s cluster may not solve the problem.

We have not figured out the root cause of the CI failure of v1.24. k8s cluster. Can you help figure it out together?

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 5, 2022

karmada-bot requested review from lonelyCZ and XiShanYongYe-Chang July 5, 2022 12:08

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 5, 2022

karmada-bot assigned XiShanYongYe-Chang Jul 6, 2022

XiShanYongYe-Chang reviewed Jul 7, 2022

View reviewed changes

karmada-bot requested a review from Poor12 July 7, 2022 03:15

chaunceyjiang force-pushed the scheduler_plugins branch from 520f2af to 87a42a1 Compare July 7, 2022 10:16

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2022

chaunceyjiang force-pushed the scheduler_plugins branch 4 times, most recently from e2a8c19 to a3eeda3 Compare July 7, 2022 15:30

karmada-bot requested a review from XiShanYongYe-Chang July 8, 2022 01:36

karmada-bot assigned Poor12 Jul 8, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2022

karmada-bot requested a review from RainbowMango July 8, 2022 02:53

RainbowMango reviewed Jul 8, 2022

View reviewed changes

cmd/scheduler/app/options/options.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/runtime/registry.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/runtime/registry.go Outdated Show resolved Hide resolved

chaunceyjiang force-pushed the scheduler_plugins branch from a3eeda3 to 59023d0 Compare July 8, 2022 09:31

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2022

RainbowMango mentioned this pull request Jul 11, 2022

Rename scheduler plugin name #2170

Merged

karmada-bot assigned RainbowMango Jul 19, 2022

RainbowMango reviewed Jul 20, 2022

View reviewed changes

custom scheduler plugin

fc600ae

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>

chaunceyjiang force-pushed the scheduler_plugins branch from 59023d0 to fc600ae Compare July 20, 2022 03:16

RainbowMango approved these changes Jul 20, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2022

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2022

karmada-bot merged commit 37ffc7d into karmada-io:master Jul 20, 2022

chaunceyjiang deleted the scheduler_plugins branch September 1, 2022 01:20

Poor12 mentioned this pull request Jan 6, 2023

[umbrella]Support multiple schedulers #3024

Closed

6 tasks

	func (c Context) IsControllerEnabled(name string, disabledByDefaultControllers sets.String) bool {
	hasStar := false
	for _, ctrl := range c.Opts.Controllers {
	if ctrl == name {
	return true
	}
	if ctrl == "-"+name {
	return false
	}
	if ctrl == "*" {
	hasStar = true
	}
	}
	// if we get here, there was no explicit choice
	if !hasStar {
	// nothing on by default
	return false
	}

	return !disabledByDefaultControllers.Has(name)
	}

custom enable or disable of scheduler plugins #2135

custom enable or disable of scheduler plugins #2135

Conversation

chaunceyjiang commented Jul 5, 2022 • edited by RainbowMango

chaunceyjiang commented Jul 5, 2022

XiShanYongYe-Chang commented Jul 6, 2022

chaunceyjiang commented Jul 7, 2022 • edited

XiShanYongYe-Chang commented Jul 7, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang Jul 7, 2022

Choose a reason for hiding this comment

chaunceyjiang Jul 7, 2022

Choose a reason for hiding this comment

XiShanYongYe-Chang Jul 7, 2022

Choose a reason for hiding this comment

chaunceyjiang Jul 7, 2022

Choose a reason for hiding this comment

RainbowMango Jul 8, 2022

Choose a reason for hiding this comment

chaunceyjiang commented Jul 8, 2022

Poor12 commented Jul 8, 2022

XiShanYongYe-Chang commented Jul 8, 2022

karmada-bot commented Jul 8, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango commented Jul 8, 2022

prodanlabs commented Jul 8, 2022

RainbowMango commented Jul 9, 2022 • edited

RainbowMango commented Jul 9, 2022

Garrybest commented Jul 9, 2022

RainbowMango commented Jul 9, 2022

Garrybest commented Jul 9, 2022

kerthcet commented Jul 10, 2022

prodanlabs commented Jul 10, 2022

chaunceyjiang commented Jul 11, 2022

kerthcet commented Jul 11, 2022

chaunceyjiang commented Jul 11, 2022

RainbowMango commented Jul 11, 2022

RainbowMango commented Jul 19, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jul 20, 2022

chaunceyjiang commented Jul 20, 2022

RainbowMango commented Jul 20, 2022

chaunceyjiang commented Jul 20, 2022 • edited

RainbowMango commented Jul 20, 2022

XiShanYongYe-Chang commented Jul 20, 2022

chaunceyjiang commented Jul 5, 2022 •

edited by RainbowMango

chaunceyjiang commented Jul 7, 2022 •

edited

RainbowMango commented Jul 9, 2022 •

edited

chaunceyjiang commented Jul 20, 2022 •

edited