-
Notifications
You must be signed in to change notification settings - Fork 200
Implement EPP Plugins by datalayer objects #1901
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
Implement EPP Plugins by datalayer objects #1901
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/cc |
|
@elevran: GitHub didn't allow me to request PR reviews from the following users: elevran. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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-sigs/prow repository. |
|
Please change DataSource and Extractor to be more like the other plugins. They all have a field TypedName, that gets set in the factory function. The TypedNAme function, should simply return that field. |
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
e8d4564 to
c05299a
Compare
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
|
@shmuelk all comments have been address and changes integrated. PTAL. |
- use Name as the unique key to allow future extension to support multiple instances of the same type. - remove GetSourceByName/Type. Signed-off-by: Etai Lev Ran <[email protected]>
|
/lgtm |
| // DataSource is a Model Server Protocol (MSP) compliant metrics data source, | ||
| // returning Prometheus formatted metrics for an endpoint. | ||
| type DataSource struct { | ||
| tn plugins.TypedName |
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.
nit: consistency with other plugins?
| tn plugins.TypedName | |
| typedName plugins.TypedName |
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.
current plugins use both typedName (most) and tn (e.g., lora affinity, slo scorer, some mocks in test files).
The scope (i.e., distance between where defined and used) is small enough to use tn here.
If you think it's important, can open another PR to standardize on on typedName across the entire code base.
| } | ||
| if _, loaded := dsr.sources.LoadOrStore(src.Name(), src); loaded { | ||
| return fmt.Errorf("unable to register duplicate data source: %s", src.Name()) | ||
| if _, loaded := dsr.sources.LoadOrStore(src.TypedName().Name, src); loaded { |
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.
This is a bit confusing.
generally in the codebase, plugins have TypeName, where Type is mandatory and Name is optional.
when using ConfigFile Name defaults to Type is not mentioned otherwise, but that is not the case if configured through code (e.g., in unit tests name remains empty).
Additionally, in other plugins - Type is guaranteed to be unique, while Name is not.
maybe we can use TypedName.String() as the key of the map?
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.
Was originally using Type as the key, went back and forth on this given @shmuelk feedback on usage for configuration file. You can see some of the discussion context in resolved conversations.
The plugin references in the configuration file use the plugin name as the reference key, hence the same is applied here.
At least in theory, you could have multiple sources or extractors of the same type registered (same as plugins).
If we want to change to use TypedName.String() should probaly do so across the entire config handling code.
@shmuelk ^^
|
|
||
| type mockDataSource struct { | ||
| name string | ||
| tn plugins.TypedName |
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.
typedName?
|
|
||
| // Config defines the configuration of EPP data layer, as the set of DataSources and | ||
| // Extractors defined on them. | ||
| type Config struct { |
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.
is this struct a prep for next PR?
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.
Correct!
Mainly to confirm the interface between the datalayer and the configuration setting code so they can proceed in parallel.
nirrozenbaum
left a comment
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.
left few minor non blocking comments.
I think it would be good if we standardize all plugins are keyed by TypedName in all places (in current PR, most of the places use Name but in other plugins TypedName as a whole is the unique identifier).
let's proceed with a "fix it forward" approach.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elevran, nirrozenbaum, shmuelk 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 |
#1910 created for tracking. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implement Plugin interface by data layer objects (DataSources and Extractors) to enable configuration from file.
plugins.HandleobjectPodListmethod to avoid circular import dependency (datalayer importsplugins.TypedNameand at the same timeplugins.Handle.ListPodimportsdatalayer.Endpoint/backendmetrics.PodMetrics). Replaced with a "narrower" function that only returns the pod names so prefix scorer (the only current user) can evict entries for terminated Pods.TypedName()(replacingName()) to datalayer objects so they're "proper" plugins and can be instantiated by the configuration loader in a future PR. Note that we expect a unique type for each DataSource/Extractor so Name is effectively ignore and left empty.datalayer.metrics.ClientfromNewDataSource(nil was passed in all cases)Which issue(s) this PR fixes:
Ref #1883
Does this PR introduce a user-facing change?:
Not yet. This lays the ground work for enabling data layer section in the config file but does not implement it yet.