-
Notifications
You must be signed in to change notification settings - Fork 179
feat: Load the SchedulerConfig from a configuration file/text and make it easier to add plugins #881
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
feat: Load the SchedulerConfig from a configuration file/text and make it easier to add plugins #881
Changes from all commits
174be49
84cde29
a1494f3
4b2e8ba
3153f57
3fb0945
07406b0
d11f45b
eb94355
a9bd54d
bce595b
70b9b74
75e8fb6
8792e58
f44163b
32b53fd
6bf9bd7
b8e0b4d
58cd2fa
b91488d
2bcd718
b4980ef
b7443b2
3eb408f
0ac2718
8ebec80
2072d23
511a5c2
6386cd6
d108223
17d6993
642b9e5
9dbb2d5
3d37de1
189c5fe
a15557e
e1fc8c9
c5f78f9
ba530cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
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 v1alpha1 | ||
|
||
// SetDefaults_EndpointPickerConfig sets default values in a | ||
// EndpointPickerConfig struct. | ||
// | ||
// This naming convension is required by the defalter-gen code. | ||
func SetDefaults_EndpointPickerConfig(cfg *EndpointPickerConfig) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: is this an autogenerated function name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function name is required by the generated code. See api/config/v1alpha1/zz_generated_defaulys.go I agree its not a cool name |
||
for idx, pluginConfig := range cfg.Plugins { | ||
if pluginConfig.Name == "" { | ||
cfg.Plugins[idx].Name = pluginConfig.PluginName | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/* | ||
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 v1alpha1 contains API Schema definitions for the | ||
// inference.networking.x-k8s.io API group. | ||
// | ||
// +kubebuilder:object:generate=true | ||
// +groupName=inference.networking.x-k8s.io | ||
package v1alpha1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
/* | ||
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 v1alpha1 | ||
|
||
import ( | ||
"encoding/json" | ||
|
||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
// +k8s:defaulter-gen=true | ||
// +kubebuilder:object:root=true | ||
|
||
// EndpointPickerConfig is the Schema for the endpointpickerconfigs API | ||
type EndpointPickerConfig struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about the general plugin registration instead of being explicit and have leaving that aside and commenting on the existing approach, I was expecting to see the structure here a bit different (and the configuration file structure as well). plugins:
- name: lowQueue
pluginName: low-queue
parameters:
threshold: 10
- name: prefixCache
pluginName: prefix-cache
parameters:
hashBlockSize: 32
- name: maxScore
pluginName: max-score
- name: singleProfileHandler
pluginName: single-profile
requestcontrol:
postResponse:
- pluginRef: prefixCache
scheduling:
profileHandler: singleProfileHandler
profiles:
- name: default
plugins:
- pluginRef: lowQueue
- pluginRef: prefixCache
weight: 50
- pluginRef: maxScore and then I expect to see an appropriate structure here. additionally, in current example config file and in this struct I don't see clarification - I do NOT expect to see all layers atm. I expect to see here only the layers that have extension points today which is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to answer several questions:
This subject has been discussed before, either here in the PR or on the proposal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t think there’s a ”right” thing. IMO it’s expected from whoever configure GIE to understand what he is configuring. if he doesn’t, he should read the documentation and learn. I was deeply involved in scheduler and I can’t understand from this file what’s the setup of the scheduler, so how can we expect someone who never touched the scheduler code to understand which plugins are going to be configured? would be happy to hear more thoughts about this - @kfswain @elevran @ahg-g There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I shared my view at #839 (comment) In summary:
In the above, I am trying to shift the complexity to the plugin developer and simplify it for the plugin admin/user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shmuelk I recommend to document the decision that we will reach in the doc, and also publish the doc as a proposal under https://github.com/kubernetes-sigs/gateway-api-inference-extension/tree/main/docs/proposals, perhaps under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hard disagree here. hard disagree also on the point that plugin should implement a single extension. and I wasn’t even mentioning the readability point. IMO when reading a configuration file one should be able to understand what is the configuration that is going to be used. this looks to me as if we’re trying to get inspiration from the kube-scheduler and make use of the same principles, although some of them don’t represent GIE layers. an inspiration example is good until the point it doesn’t. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm okay with that. User should be reading documentation anyhow to know what a plugin does.
+1. This means that Plugins are defined in their own section, not under scheduling specific sections, right?
If this refers only to the scheduling layer - this is fine. However, this would be an issue for plugins that span subsystems (e.g., prefix/kv-cache implement a scheduling interface as well as a post response handler). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Scheduling profiles are a special case that we need to clearly document what plugins can be listed there. We can also add validation
I didn't say a single extension, I said a single feature. A feature can be composed of more than one extension, in which case disabling an extension breaks the feature (e.g., prefix cache can't function without both the post-response and the score extensions). To give an example where I think breaking things into two features would be recommended: In kube-scheduler, we have preferred affinity and required affinity, they are both affinity features, but from user perspective they can be enabled/disable independently, so I would not implement them under as a single plugin, but as two.
I view this as a configuration of features, not extensions. Having to explicitly configure each extension point a feature requires is not good UX and error prone. What if I forget to configure the postResponse for the prefix cache feature? what if in a future release I add another optimization to prefix-cache that required implementing another extension point?
See response above, it is not related to a specific layer, more about the idea that a plugin should not implement more than one user facing feature that could be disable/enabled independently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree with this in spirit. But I worry that 'defining a feature' defines too many opinions. If we want to include in-tree plugins & have the feature approach, that means that we have to have in-tree plugins implement exactly 1 extension point. As we do not know if a user will want to make different decisions than the feature creator did. But, I do understand the idea of setting a minimum atomic unit: i.e. if you implement a Scorer plugin that relies on a data layer extension to work, full stop. Then yes, I absolutely could see those being considered an atomic unit. So perhaps it's a matter of best practices, plugin development should only implement what is strictly necessary. So even if there is a KV-cache util scorer, & a KV-cache util filter. They should not be implemented together, b/c a user does not necessarily want to implement them both. However that seems very easy to footgun.
++ I think with the existence of multiple profile runs per request (some of which might just be dark-launches; aka doesnt actually impact scheduling decisions). We may want to make sure we aren't relying principles for a different set of requirements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
None of the three points I mentioned above referenced kube-scheduler or argued that we should copy exactly what we did there. But I understand this impression since I referenced kube-scheduler many times throughout the whole process of designing extensibility. Speaking of requirements, I am arguing that the primary persona for the config api is the platform admin, and my thinking is that platform admins think of features, not implementation details of the component (EPP in this case). The idea is to shift defining what a feature is to the feature developer since they understand its dependencies the best, rather than pushing that to the platform admin. |
||
metav1.TypeMeta `json:",inline"` | ||
|
||
// +required | ||
// +kubebuilder:validation:Required | ||
// Plugins is the list of plugins that will be instantiated. | ||
Plugins []PluginSpec `json:"plugins"` | ||
|
||
// +required | ||
// +kubebuilder:validation:Required | ||
// SchedulingProfiles is the list of named SchedulingProfiles | ||
// that will be created. | ||
SchedulingProfiles []SchedulingProfile `json:"schedulingProfiles"` | ||
} | ||
|
||
// PluginSpec contains the information that describes a plugin that | ||
// will be instantiated. | ||
type PluginSpec struct { | ||
// +optional | ||
// Name provides a name for plugin entries to reference. If | ||
// omitted, the value of the PluginName field will be used. | ||
Name string `json:"name"` | ||
ahg-g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// +required | ||
// +kubebuilder:validation:Required | ||
// PluginName specifies the plugin to be instantiated. | ||
PluginName string `json:"pluginName"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another follow up is to probably change this to "type" @elevran |
||
|
||
// +optional | ||
// Parameters are the set of parameters to be passed to the plugin's | ||
// factory function. The factory function is responsible | ||
// to parse the parameters. | ||
Parameters json.RawMessage `json:"parameters"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in kube-scheduler, we used runtime.Object, probably allows us to use k8s api-machinery functions for decoding https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/apis/config/types.go#L207 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code in the K8S scheduler, runtime.Object is simply cast to the appropriate type used by the plugin. Those types seem to include a standard K8S TypeMeta stanza. You want every plugin's parameters object to start with apiVersion and kind? You were trying to same extra name tags, isn't here even more acute? Also if we go with runtime.Object, every plugin with parameters would need to register it's schema in the global schema for the config... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use a secondary schema that this not used/known to the k8s client and only used in configuration? I do think that the use of Object makes sense as it would natively separate the plugin type from its name, which is currently unclear, IMO. This is a side note - I'll open an issue to discuss to avoid sidetracking the feedback. |
||
} | ||
|
||
// SchedulingProfile contains the information to create a SchedulingProfile | ||
// entry to be used by the scheduler. | ||
type SchedulingProfile struct { | ||
// +kubebuilder:validation:Required | ||
// Name specifies the name of this SchedulingProfile | ||
Name string `json:"name"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we verify name uniqueness somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. In the function validateConfiguration in pkg/epp/common/config/configloader.go |
||
|
||
// +required | ||
// +kubebuilder:validation:Required | ||
// Plugins is the list of plugins for this SchedulingProfile. They are assigned | ||
// to the appropriate "slots" based on their type. | ||
Plugins []SchedulingPlugin `json:"plugins"` | ||
} | ||
|
||
// SchedulingPlugin describes a plugin that will be associated with a | ||
// SchedulingProfile entry. | ||
type SchedulingPlugin struct { | ||
// +required | ||
// +kubebuilder:validation:Required | ||
// PluginRef specifies a partiular Plugin instance to be associated with | ||
// this SchedulingProfile. The reference is to the name of an | ||
// entry of the Plugins defined in the configuration's Plugins | ||
// section | ||
PluginRef string `json:"pluginRef"` | ||
|
||
// +optional | ||
// Weight is the weight fo be used if this plugin is a Scorer. | ||
Weight *int `json:"weight"` | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
We should use this function to default the parameters of the various in-tree plugins because defaulting is associated with the api version. I believe this function will be automatically invoked the file is decoded.
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.
In general this "extension point" is used to set default values in the CR in question. Look at the examples you pointed me to. I don't think it should be used to specify default values for parameters of plugins. The plugins should handle that themselves.
This could be used, if we wanted to create default configurations, albeit in a round about way.
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 in-tree plugins config types should be part of the versioned api, their defaulting should be as well.
See as an example: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/apis/config/v1/defaults.go
We can handle that as a followup though to keep this PR focused on defining the api.