Scheduler config refactor for simplifying plugins registration #835
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| Copyright 2025 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package scorer | ||
|
|
||
| import ( | ||
| "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/plugins" | ||
| ) | ||
|
|
||
| // NewWeightedScorer initializes a new WeightedScorer and returns its pointer. | ||
| func NewWeightedScorer(scorer plugins.Scorer, weight int) *WeightedScorer { | ||
nirrozenbaum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return &WeightedScorer{ | ||
| Scorer: scorer, | ||
| weight: weight, | ||
| } | ||
| } | ||
|
|
||
| // WeightedScorer is a struct that encapsulates a scorer with its weight. | ||
| type WeightedScorer struct { | ||
| plugins.Scorer | ||
| weight int | ||
| } | ||
|
|
||
| // Weight returns the weight of the scorer. | ||
| func (s *WeightedScorer) Weight() int { | ||
| return s.weight | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| /* | ||
| Copyright 2025 The Kubernetes Authors. | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package scheduling | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/plugins" | ||
| "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/plugins/scorer" | ||
| ) | ||
|
|
||
| // NewSchedulerConfig creates a new SchedulerConfig object and returns its pointer. | ||
| func NewSchedulerConfig() *SchedulerConfig { | ||
| return &SchedulerConfig{ | ||
| preSchedulePlugins: []plugins.PreSchedule{}, | ||
| filters: []plugins.Filter{}, | ||
| scorers: []*scorer.WeightedScorer{}, | ||
| postSchedulePlugins: []plugins.PostSchedule{}, | ||
| postResponsePlugins: []plugins.PostResponse{}, | ||
| // picker remains nil since config doesn't support multiple pickers | ||
| } | ||
| } | ||
|
|
||
| // SchedulerConfig provides a configuration for the scheduler which influence routing decisions. | ||
| type SchedulerConfig struct { | ||
| preSchedulePlugins []plugins.PreSchedule | ||
| filters []plugins.Filter | ||
| scorers []*scorer.WeightedScorer | ||
| picker plugins.Picker | ||
| postSchedulePlugins []plugins.PostSchedule | ||
| postResponsePlugins []plugins.PostResponse | ||
| } | ||
|
|
||
| // WithPreSchedulePlugins sets the given plugins as the PreSchedule plugins. | ||
| // If the SchedulerConfig has PreSchedule plugins, this call replaces the existing plugins with the given ones. | ||
| func (c *SchedulerConfig) WithPreSchedulePlugins(plugins ...plugins.PreSchedule) *SchedulerConfig { | ||
| c.preSchedulePlugins = plugins | ||
| return c | ||
| } | ||
|
|
||
| // WithFilters sets the given filter plugins as the Filter plugins. | ||
| // if the SchedulerConfig has Filter plugins, this call replaces the existing plugins with the given ones. | ||
| func (c *SchedulerConfig) WithFilters(filters ...plugins.Filter) *SchedulerConfig { | ||
| c.filters = filters | ||
| return c | ||
| } | ||
|
|
||
| // WithScorers sets the given scorer plugins as the Scorer plugins. | ||
| // if the SchedulerConfig has Scorer plugins, this call replaces the existing plugins with the given ones. | ||
| func (c *SchedulerConfig) WithScorers(scorers ...*scorer.WeightedScorer) *SchedulerConfig { | ||
| c.scorers = scorers | ||
| return c | ||
| } | ||
|
|
||
| // WithPicker sets the given picker plugins as the Picker plugin. | ||
| // if the SchedulerConfig has Picker plugin, this call replaces the existing plugin with the given one. | ||
| func (c *SchedulerConfig) WithPicker(picker plugins.Picker) *SchedulerConfig { | ||
| c.picker = picker | ||
| return c | ||
| } | ||
|
|
||
| // WithPostSchedulePlugins sets the given plugins as the PostSchedule plugins. | ||
| // If the SchedulerConfig has PostSchedule plugins, this call replaces the existing plugins with the given ones. | ||
| func (c *SchedulerConfig) WithPostSchedulePlugins(plugins ...plugins.PostSchedule) *SchedulerConfig { | ||
| c.postSchedulePlugins = plugins | ||
| return c | ||
| } | ||
|
|
||
| // WithPostResponsePlugins sets the given plugins as the PostResponse plugins. | ||
| // If the SchedulerConfig has PostResponse plugins, this call replaces the existing plugins with the given ones. | ||
| func (c *SchedulerConfig) WithPostResponsePlugins(plugins ...plugins.PostResponse) *SchedulerConfig { | ||
| c.postResponsePlugins = plugins | ||
| return c | ||
| } | ||
|
|
||
| // AddPlugins adds the given plugins to all scheduler plugins according to the interfaces each plugin implements. | ||
| // A plugin may implement more than one scheduler plugin interface. | ||
| // Special Case: In order to add a scorer, one must use the scorer.NewWeightedScorer function in order to provide a weight. | ||
| // if a scorer implements more than one interface, supplying a WeightedScorer is sufficient. The function will take the internal | ||
| // scorer object and register it to all interfaces it implements. | ||
| func (c *SchedulerConfig) AddPlugins(pluginObjects ...plugins.Plugin) error { | ||
| for _, plugin := range pluginObjects { | ||
| if weightedScorer, ok := plugin.(*scorer.WeightedScorer); ok { | ||
| c.scorers = append(c.scorers, weightedScorer) | ||
| plugin = weightedScorer.Scorer // if we got WeightedScorer, unwrap the plugin | ||
| } else if scorer, ok := plugin.(plugins.Scorer); ok { // if we got a Scorer instead of WeightedScorer that's an error. | ||
|
There was a problem hiding this comment. nit: consider adding a unit test |
||
| return fmt.Errorf("failed to register scorer '%s' without a weight. follow function documentation to register a scorer", scorer.Name()) | ||
| } | ||
| if preSchedulePlugin, ok := plugin.(plugins.PreSchedule); ok { | ||
| c.preSchedulePlugins = append(c.preSchedulePlugins, preSchedulePlugin) | ||
| } | ||
| if filter, ok := plugin.(plugins.Filter); ok { | ||
| c.filters = append(c.filters, filter) | ||
| } | ||
| if picker, ok := plugin.(plugins.Picker); ok { | ||
| if c.picker != nil { | ||
|
There was a problem hiding this comment. nit: Considering adding a unit test |
||
| return fmt.Errorf("failed to set '%s' as picker, already have a registered picker plugin '%s'", picker.Name(), c.picker.Name()) | ||
| } | ||
| c.picker = picker | ||
| } | ||
| if postSchedulePlugin, ok := plugin.(plugins.PostSchedule); ok { | ||
| c.postSchedulePlugins = append(c.postSchedulePlugins, postSchedulePlugin) | ||
| } | ||
| if postResponsePlugin, ok := plugin.(plugins.PostResponse); ok { | ||
| c.postResponsePlugins = append(c.postResponsePlugins, postResponsePlugin) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another approach is to use reflection, for each plugin we detect what extension points it implements and register them: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/runtime/framework.go#L522
This will make it easier to add out of tree plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, you already have AddPlugin, why not use it here instead of the extension specific
withoptions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are two different questions. I'll try to answer both.
yes. the two valid approaches we have here are type assertion and reflection, each has pros/cons.
main pros for type assertion are:
main pros for reflection:
in our case, we know the interfaces we're checking upfront and I see no benefit for using reflection which is a better fit when the interfaces are not known upfront. type assertions are much more readable & maintainable and much faster. Generally speaking, Go encourages using explicit, compile-time checked patterns like type assertions when the set of interfaces is known.
right. the intention was to allow maximum flexibility with the plugins registration.
for example, let's assume I have a multi extension plugin but I'm running some performance benchmarks and want to register it only as a filter and not as a scorer to compare results. AddPlugin will add to all matching interfaces, while using the
Withfunctions sets only the requested plugin type.This PR is trying to solve the multi extension plugin registration and make it clean, but not in the price of introducing other inconveniences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to suggest runtime discovery of what extensions to register vs having to explicitly code that at registration. Type assertion vs reflication is an implementation detail.
I think we are not there yet, but we want to distinguish between registration and creating profiles. We should have a central registry of plugins and the extensions they implement. Profiles reference specific set of extensions to execute by referencing the registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we're not there yet with the multi profiles and central registry. I agree that all of this is needed.
I believe that as we make progress, the current
SchedulerConfigwill becomeSchedulerProfile, andSchedulerConfigwill probably be a set of configured profiles and maybe more configuration values.but all of this is not really related to having the "With" functions which allows flexibility as long as the config (or profile) is defined by code and not by configuration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg, good we are aligned on the future direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer a single AddPlugins method, it's simple and I don't need to decide which one to use. It's just in the startup path so I don't think performance hit of reflection really matters. Yes we sacrifice some readability but we are already using it anyway. To me the benefit of not having to understand the different options overweighs the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the intention was to allow maximum flexibility with the plugins registration.
for example, let's assume I have a multi extension plugin but I'm running some performance benchmarks and want to register it only as a filter and not as a scorer to compare results. AddPlugin will add to all matching interfaces, while using the With functions sets only the requested plugin type.
This PR is trying to solve the multi extension plugin registration and make it clean, but not in the price of introducing other inconveniences.