adding fairness-id header to be used in flow control #1282
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain 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 |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
k8s-ci-robot
added
size/L
| destinationEndpointHintKey = flag.String( | ||
| "destination-endpoint-hint-key", | ||
| runserver.DefaultDestinationEndpointHintKey, | ||
| "Header and response metadata key used by Envoy to route to the appropriate pod. This must match Envoy configuration.") | ||
| destinationEndpointHintMetadataNamespace = flag.String( | ||
| "destination-endpoint-hint-metadata-namespace", | ||
| runserver.DefaultDestinationEndpointHintMetadataNamespace, | ||
| "The key for the outer namespace struct in the metadata field of the extproc response that is used to wrap the"+ | ||
| "target endpoint. If not set, then an outer namespace struct should not be created.") | ||
| fairnessIDHeaderKey = flag.String( | ||
| "fairness-id-header-key", | ||
| runserver.DefaultFairnessIDHeaderKey, | ||
| "The header key used to pass the fairness ID to be used in Flow Control.") |
There was a problem hiding this comment.
we have an open issue #1267 which is about removing destination headers from cmd line args.
this looks more of the same.
the main point is that protocol specifics shouldn't be configurable. this is the protocol and whoever wants to use GIE should use the headers according to the protocol.. so IMO fairness id header shouldn't be configurable same as the other two.
There was a problem hiding this comment.
For sure. How did you want to handle the transition? My thinking was that until we do that transition, we want to keep the functionality similar.
I can remove this flag in this PR but then its special cased until we remove the others. LMKWYT
There was a problem hiding this comment.
I was thinking about the opposite direction.. instead of adding third flag that should be removed, we could remove in this PR the two other flags and mark #1267 as fixed.
but that's ok we can continue with current version and handle that issue separately.
There was a problem hiding this comment.
went over the PR.
IMO fairness ID header shouldn't be configurable, it's part of the protocol.
generally speaking, I think protocol specifics shouldn't be configurable.
if we revert this change, this PR becomes ~50 code lines.
with this PR size, I'm not sure it's really needed to split to "adding the header" PR and "using the header" PR.
I'd prefer to see them together to avoid places were we pass arguments that are not really used and would later need to be cleaned (e.g., admitRequest now gets fairnessID, will it be used there? maybe yes, maybe not.. depends on how the fairnessID handling will be done).
I dont think there's anything wrong with stubbing out a flag we intend to use. This was something that was suggested as a change to the API, the only reason it's not consumed is because another engineer is implementing flow control. Splitting up work allows us all to go faster? |
Sure. let's move forward with this PR and we can address the other parts in separate PRs. /lgtm |
k8s-ci-robot
added
the
lgtm
Fixes: #1245
Some other small fixes also included (config updates mostly)
There is not currently usage for the flag, it will be integrated with @LukeAVanDrie's flow control. Currently just stubbing via log in admissionControl