-
Notifications
You must be signed in to change notification settings - Fork 85
Scheduler config refactor for simplifying plugins registration #835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
16c2a41
to
c4749cc
Compare
c4749cc
to
b121c67
Compare
/retest |
} | ||
schedConfigOpts := []scheduling.ConfigOption{} | ||
|
||
schedulerConfig := scheduling.NewSchedulerConfig(). |
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 with
options?
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.
Another approach is to use reflection
yes. the two valid approaches we have here are type assertion and reflection, each has pros/cons.
main pros for type assertion are:
- Type safety: Checked at compile-time; you're guaranteed not to misuse types.
- Performance: Much faster than reflection; no runtime metadata lookup needed.
- Readability: Straightforward and idiomatic in Go.
- No imports: No need to bring in the reflect package, keeping dependencies minimal.
main pros for reflection:
- Dynamic inspection: You can iterate over available methods, fields, or types dynamically — useful for very generic tooling or plugin systems.
- No prior knowledge required: Useful if you don’t know all possible interfaces ahead of time.
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.
oh, you already have AddPlugin, why not use it here instead of the extension specific with options?
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 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.
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. the two valid approaches we have here are type assertion and reflection, each has pros/cons.
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.
right. the intention was to allow maximum flexibility with the plugins registration.
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 SchedulerConfig
will become SchedulerProfile
, and SchedulerConfig
will 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.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, nirrozenbaum 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:
Approvers can indicate their approval by writing |
} | ||
schedConfigOpts := []scheduling.ConfigOption{} | ||
|
||
schedulerConfig := scheduling.NewSchedulerConfig(). |
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.
if prefixCacheScheduling == "true" { | ||
prefixScorerWeight := envutil.GetEnvInt("PREFIX_CACHE_SCORE_WEIGHT", prefix.DefaultScorerWeight, setupLog) | ||
schedConfigOpts = append(schedConfigOpts, scheduling.AddPrefixPlugin(loadPrefixCacheConfig(), prefixScorerWeight)) | ||
if err := schedulerConfig.AddPlugins(scorer.NewWeightedScorer(prefix.New(loadPrefixCacheConfig()), prefixScorerWeight)); err != nil { |
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 think the fact you need to wrap a plugin with WeightedScorer is error prone. It's easy to miss.
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.
it doesn't make sense to allow scorer registration without a weight. we should allow only weighted scorer.
on the other hand, I didn't want to add a weight field inside the scorer itself, cause the same scorer can be used in different scheduling cycles with different weights (and it may be required to use the same scorer instance).
This was a requirement I got from internal IBM teams.
therefore, I introduced the weighted scorer struct which from one hand adds the weight, and on the other hand allows reusing the same scorer instance but with a different weight in a different scheduling cycle.
to address your concern (error prone), I added a check in the AddPlugins function and if a scorer is passed (instead of weighted scorer) an appropriate error is returned, so a developer can easily understand and fix it.
also added a comment on the AddPlugins function godoc.
handles how to register a plugin that implements multiple scheduler plugins interfaces with a single registration command Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
b121c67
to
a333ca4
Compare
Signed-off-by: Nir Rozenbaum <[email protected]>
@liu-cong addressed the code review comments |
This PR adds a generic way for registering plugins that implement multiple plugin interfaces with a single registration command -
AddPlugins
(can be called with one or more plugins).Additionally, it simplifies the scheduler config creation such that one doesn't need to create and pass an empty slice for plugins that are not in use (e.g., if no PreSchedule plugins are used, no need to create an empty slice).
example can be seen in main.go where only one filter, two scorers and one picker are used:
fix #813