-
Notifications
You must be signed in to change notification settings - Fork 122
handle picking multiple destinations in scheduling layer #1059
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum 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 |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
0668a03
to
bd6c559
Compare
bd6c559
to
58cd3c4
Compare
293bbec
to
61c36ab
Compare
61c36ab
to
73c9eaf
Compare
8762ad7
to
44a5172
Compare
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
44a5172
to
951b5af
Compare
@@ -238,7 +238,8 @@ func (d *Director) prepareRequest(ctx context.Context, reqCtx *handlers.RequestC | |||
return reqCtx, errutil.Error{Code: errutil.Internal, Msg: "results must be greater than zero"} | |||
} | |||
// primary profile is used to set destination | |||
targetPod := result.ProfileResults[result.PrimaryProfileName].TargetPod.GetPod() | |||
// TODO should use multiple destinations according to epp protocol. current code assumes a single target |
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.
Can we bind an issue to this TODO?
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.
we do have #414 which covers this point current PR implements first half (scheduling part) and next PR should mark the issue as completed (handle request control + hndlers).
a new issue will be a duplicate
// RandomPickerFactory defines the factory function for RandomPicker. | ||
func RandomPickerFactory(name string, _ json.RawMessage, _ plugins.Handle) (plugins.Plugin, error) { | ||
return NewRandomPicker().WithName(name), nil | ||
func RandomPickerFactory(name string, rawParameters json.RawMessage, _ plugins.Handle) (plugins.Plugin, error) { |
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'm wondering if we want to parse the json before here and just push everything into an unstructured map.
It's out of scope of this PR, definitely, and I do understand the issue that a complex object would still need its type inferred... But it feels strange to do json parsing in every factory. Is there a reason that I missed that it was done this way?
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.
same answer as in the other thread.. it was initially implemented this way and this is yet another point that potentially can be improved in config api.
/lgtm Holding if we want to spin up an issue to track the TODO work, thanks Nir! |
/unhold will take care of next part using issue #414 (current PR was focused on scheduling and left the behavior of other layers as is) |
SGTM! Thanks Nir! |
…sigs#1059) * implement multiple destination as the output of the scheduler Signed-off-by: Nir Rozenbaum <[email protected]> * updated max score picker unit tests to cover multiple pods Signed-off-by: Nir Rozenbaum <[email protected]> * imports Signed-off-by: Nir Rozenbaum <[email protected]> * unit-test fix Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
This PR implements the multiple destination handling in the scheduling layer, as defined in the EPP protocol:
https://github.com/kubernetes-sigs/gateway-api-inference-extension/tree/main/docs/proposals/004-endpoint-picker-protocol#destination-endpoint
please pay attention that this PR updates only the scheduling layer.
as a follow up, we should update the request-control layer and the handlers to use the multiple returned endpoints according to the protocol.
In order to keep this PR tightly scoped, the director keeps using a single targetPod to keep the current functionality.