feat: Allow request control plugins to return ext_proc dynamic metadata#2156
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @fcfort. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
|
5f93208 to
efea3a8
Compare
| github.com/cespare/xxhash/v2 v2.3.0 | ||
| github.com/elastic/crd-ref-docs v0.2.0 | ||
| github.com/envoyproxy/go-control-plane/envoy v1.36.0 | ||
| github.com/envoyproxy/go-control-plane/envoy v1.35.0 |
There was a problem hiding this comment.
it seems lots of pkgs are being downgraded, why?
There was a problem hiding this comment.
I don't actually know how these files get changed. Should I revert the changes to go.mod / go.sum?
There was a problem hiding this comment.
It should be automatic, can you revert the changes and then run go build again?
efea3a8 to
af17094
Compare
| // Currently, this is only used by conformance test. | ||
| ReqMetadata map[string]any | ||
| // Token usage counts parsed from the response body. | ||
| Usage handlerstypes.Usage |
There was a problem hiding this comment.
This creates a dependency on the handlers pkg for the plugins, it doesn't seem ideal.
@kfswain @nirrozenbaum I feel the interfaces and types for the framework should be grouped in one place under epp/framework (renaming epp/plugins) with a separate pkg for each subsystem under it. With this, a plugin should only have a dependency on this epp/framework pkg. wdyt?
There was a problem hiding this comment.
generally I agree.
I have tried that a while ago and got to a circular dependency, cause scheduler needs to reference plugins, and profile handler plugin references scheduler profiles (its return value).
we can reorganize the scheduler package to get it fixed
There was a problem hiding this comment.
I love that idea. If we can make it work in practice that would be great.
There was a problem hiding this comment.
yes, we can.
that would still include some awkward references but it would work.
e.g.,
epp/framework would reference epp/scheduler/profile
and
epp/scheduler would reference epp/framework.
There was a problem hiding this comment.
that’s not ideal but doable.
LMKWYT.
There was a problem hiding this comment.
One way to avoid the circular dependency is to move pkg/epp/requestcontrol/types.go to pkg/epp/requestcontrol/types/types.go so that it is in a separate package. This does end up touching another dozen or so files. Is that what you'd like?
There was a problem hiding this comment.
It will also impact out of tree plugins if we move the existing types. I plan to re-organize all of that as discussed above, and so I don't want to force people to change twice.
Can we define two types and copy? once we do the refactor, we will converge on one.
There was a problem hiding this comment.
First PR focused on the scheduling layer: #2192
af17094 to
484ffc0
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, fcfort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Here we create a mechanism to pass in the usage struct to the
ResponseBodyCompletecallback and also a mechanism for returning dynamic_metadata out of the callback and into the ext_proc'sProcessingResponseobject. This will be used by the new plugin introduced in #2114.See #2019 for more details.
Which issue(s) this PR fixes:
This contributes to #2019, but does not fix it.
Does this PR introduce a user-facing change?:
No user-facing change.