generated from kubernetes/kubernetes-template-project
-
Notifications
You must be signed in to change notification settings - Fork 182
Enable pluggable datalayer as experimental feature #1391
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
Merged
k8s-ci-robot
merged 11 commits into
kubernetes-sigs:main
from
elevran:enable_datalayer_v2
Aug 20, 2025
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
3bcb68a
enable global metrics logging
elevran b986fef
enable v2 data layer
elevran 7418925
move feature flag to cmd
elevran d687ae6
remove logger from collector
elevran daca615
return a typed data source using generics
elevran d09a6c4
document client configuration constants
elevran 624992f
retrieve a typed data source
elevran 056a5c6
use structured logging
elevran e6053bc
commandline specified port value has precedence
elevran f964c89
pass data sources in ctor
elevran 48347d4
address remaining review comments
elevran File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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.
Doesnt have to be done in this PR, but I'm wondering if we extend:
gateway-api-inference-extension/apix/config/v1alpha1/endpointpickerconfig_types.go
Line 29 in 0eaf592
To include a field for features to be enabled, we will soon have: Flow Control, SLO prediction, & now the pluggable data layer as experimental, opt-in features. So rather than having various env vars, we keep all feature gating in one place
Uh oh!
There was an error while loading. Please reload this page.
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 is to have everything configured through the config file eventually.
do you suggest adding a free form of key value pairs as variables (kinda similar to env vars) for the transitionary state when a feature is experimental?
I was proposing this change recently, which I think aligns with your intention - #1288
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.
Maybe not everything but would be good to discuss the cutline. Def out of scope for this 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.
right. maybe not everything.
I meant the extension points of those parts :)
anyway, the parameters section the was suggested in #1288 can be leveraged as an solution for experimental features, so instead of having env vars we can just configure a parameter - e.g.,
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, I definitely agree there
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.
And we doc ^ features similar to K8s feature gates. We should also drop any "experimental" references and simply call them by feature name under
featureGates
. The associated docs will detail feature level, e.g.,Alpha
, and a short description of the feature. For example: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.
cc @shmuelk another useful use case for adding parameters section (key value pairs) to the config API.
to answer the above, we should have those parameters consumable not necessarily though the plugins but also by runner.go (as an example) to enable/disable features. generally speaking it could replace the usage of env vars (e.g., today we have env vars in saturation detector).
Uh oh!
There was an error while loading. Please reload this page.
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 like @danehans suggestion of a featureGates section. From a config point of view it is simply a map[string]bool.
The code that consumes the feature gates should validate the set and apply the values.
Overloading a shared parameters with so-called well known parameter names is bad idea and will lead to conflicts and confusion.