Add fallback logic to support multiple endpoints #1122
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @rlakhtakia. 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. 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. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
mostly already implemented in #1059 |
k8s-ci-robot
added
the
needs-rebase
|
I think there is still some work to be done to actually return multiple endpoints, can we please rebase and address what is left? |
k8s-ci-robot
added
size/M
|
/ok-to-test |
|
@rlakhtakia: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@ahg-g @nirrozenbaum updated conflicts to remove duplicated logic from #1059 The main change incorporated now is to add the multiple endpoints to the header response. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/epp/requestcontrol/director.go
Outdated
|
|
||
| endpoint := net.JoinHostPort(targetPod.Address, strconv.Itoa(targetPort)) | ||
| logger.V(logutil.DEFAULT).Info("Request handled", "model", reqCtx.Model, "targetModel", reqCtx.ResolvedTargetModel, "endpoint", targetPod) | ||
| combinedEndpointsString := strings.Join(targetEndpoints, ",") | ||
|
|
||
| reqCtx.TargetPod = targetPod |
There was a problem hiding this comment.
This is a bit problematic.
reqCtx.TargetPod is used later to call PostResponse plugins and we should call the plugins with the pod that served the request (the first pod did not necessarily serve it, if the request failed and used fallback to next endpoint).
up until today we used a single endpoint, so that assumption was safe (either the request succeeded and then this is the pod, or it failed). but this assumption is no longer true and reqCtx.TargetPod should be updated correctly to reflect the pod that served the request.
There was a problem hiding this comment.
I added some functionality to have multiple target pods. If we don't want to disrupt the current flow, I can add an extra field labelled 'FallbackPods' which will be the remaining list of pods
There was a problem hiding this comment.
last change is making changes to the scheduler interfaces.. that wasn't the intention :)
the interfaces shouldn't change (I'm referring to PostResponse interface).
in the last change the problem just moved from the director to PostResponse. in PostResponse we need to know which pod actually SERVED the request. there is only one such pod.
not the first one out of the candidate list (cause the first one didn't necessarily served the request, this is the whole point of supporting multiple endpoints).
There was a problem hiding this comment.
last change is making changes to the scheduler interfaces.. that wasn't the intention :)
the interfaces shouldn't change (I'm referring to PostResponse interface).
They can change if we need to change them, this is not an object api, but I understand that in this case we want to maintain the semantics in that we want to communicate the endpoint that the request was actually sent to.
in the last change the problem just moved from the director to PostResponse. in PostResponse we need to know which pod actually SERVED the request. there is only one such pod.
That is a good point. @AndresGuedez do we have the endpoint that the LB selected in the ext-proc response?
There was a problem hiding this comment.
adding also @kfswain to the thread
There was a problem hiding this comment.
So returning the exact endpoint that the LB actually sent the request to is not part of the fallback protocol, we should add that, I opened an issue to track alignment on this #1224; we will need to get agreement on this protocol with Gateway providers, and so this will take time.
However, I don't think we should block this PR on that, fallbacks is for resiliency, avoiding dropping the requests is a better outcome than suboptimal routing. We can keep the PostResponse interface as is and pass the first item in the mean time until we align on the protocol.
There was a problem hiding this comment.
PR has been updated to maintain the PostResponse interface as is.
There was a problem hiding this comment.
However, I don't think we should block this PR on that, fallbacks is for resiliency, avoiding dropping the requests is a better outcome than suboptimal routing. We can keep the PostResponse interface as is and pass the first item in the mean time until we align on the protocol.
sounds fair enough.
I do like us to try and push the finalization of this feature cause currently it feels incomplete.
I'll do another review cycle.
@rlakhtakia thanks for your patience
There was a problem hiding this comment.
Agreed on all accounts.
- We probably shouldnt knowingly break the interface contract (i.e. the endpoint we pass to PostResponse extension points should be the endpoint served)
- Fallback is opt-in and actually needing to fall back should have a low occurrence rate, so for the short term we can probably document this limitation.
I do agree we need to see this resolved rather quickly so we dont forget about this limitation and let it bite us later.
k8s-ci-robot
added
size/L
|
/retest |
|
one nit, just remove the TODO comment, otherwise this looks good to me. |
|
current PR actually is not addressing the issue. it changes PostResponse interface (which we should not) and does not give the required information about the selected endpoint which successfully served the request. This point has to be addressed before the PR can be merged (wrote also #1122 (comment)) /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
left few minor comments. overall LGTM! |
k8s-ci-robot
added
size/M
|
/unhold Thanks @rlakhtakia! |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum, rlakhtakia 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
approved
#414