-
Notifications
You must be signed in to change notification settings - Fork 193
Enable EPP to support endpoint discovery using pod selector #1833
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
base: main
Are you sure you want to change the base?
Enable EPP to support endpoint discovery using pod selector #1833
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: capri-xiyue The full list of commands accepted by this bot can be found 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. |
|
No need to review it right now. I just made the CUJ of standalone epp work without inferencepool. Still need to fix the e2e and ut |
elevran
left a comment
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.
cursory review to provide initial feedback (realizing this is work in progress)
The main question I have (and might be worth mentioning in the PR description) is the need for a new abstraction/type (EndPointsPool). A naive/simple solution (which perhaps does not work...) would be to copy the selector and port array into a Go InferencePool object and use datastore.PoolSet() along with disabling the Pool notification/reconciliation so it does not overwrite with nil.
Hopefully the rest of the code should not care or depend on the pool's origin (from command line or the API server)
|
assign @ahg-g for early review. |
|
assign @kfswain for early review |
Signed-off-by: Xiyue Yu <[email protected]>
| selector = flag.String("selector", "", "selector to filter pods on, only key value paris is supported. Format: a comma-separated list of key value paris, e.g., 'app:vllm-llama3-8b-instruct,env=prod'.") | ||
| targetPorts = flag.String("target-ports", "", "target ports of model server pods. Format: a comma-separated list of numbers, e.g., '3000,3001,3002'") |
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.
too generic, perhaps prefix each flag with "endpoint"? endpoint-selector and endpoint-target-ports
| return nil | ||
| } | ||
|
|
||
| func strToUniqueIntSlice(s string) ([]int, 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.
there is no existing library for that in k8s?
| return intList, nil | ||
| } | ||
|
|
||
| func strToMap(s string) (map[string]string, 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.
ditto, no existing library for this tokenization?
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| - name: EPP_NAME |
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.
why do we need this?
| // 4. Convert the fetched object to the canonical v1.InferencePool. | ||
| v1infPool := &v1.InferencePool{} | ||
|
|
||
| var endPointsPool *datalayer.EndpointPool |
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.
endpointPool
| t.Errorf("Unexpected InferencePool reconcile error: %v", err) | ||
| } | ||
| if diff := diffStore(ds, diffStoreParams{wantPool: pool1, wantPods: []string{"pod1-rank-0", "pod2-rank-0"}}); diff != "" { | ||
| endPointsPool1 := pool.InferencePoolToEndPointsPool(pool1) |
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.
ditto, endpointPool1, and all other similar instances in this file.
| limitations under the License. | ||
| */ | ||
|
|
||
| package datalayer |
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.
file name should be endpoint_pool.go
| // /// InferencePool APIs /// | ||
| func (ds *datastore) PoolSet(ctx context.Context, reader client.Reader, pool *v1.InferencePool) error { | ||
| if pool == nil { | ||
| // /// EndPoints APIs /// |
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.
EndpointPool
| "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/datalayer" | ||
| ) | ||
|
|
||
| func InferencePoolToEndPointsPool(inferencePool *v1.InferencePool) *datalayer.EndpointPool { |
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.
InferencePoolToEndpointPool
| return endPointsPool | ||
| } | ||
|
|
||
| func AlphaInferencePoolToEndPointsPool(inferencePool *v1alpha2.InferencePool) *datalayer.EndpointPool { |
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.
ditto
| return endPointsPool | ||
| } | ||
|
|
||
| func EndPointsPoolToInferencePool(endPointsPool *datalayer.EndpointPool) *v1.InferencePool { |
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.
ditto
| GrpcPort int | ||
| PoolNamespacedName types.NamespacedName | ||
| PoolGKNN common.GKNN | ||
| EndPointsPool *datalayer.EndpointPool |
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.
EndpointPool
|
|
||
| // defaultManagerOptions returns the default options used to create the manager. | ||
| func defaultManagerOptions(gknn common.GKNN, metricsServerOptions metricsserver.Options) (ctrl.Options, error) { | ||
| func defaultManagerOptions(endPointsPool *datalayer.EndpointPool, metricsServerOptions metricsserver.Options) (ctrl.Options, 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.
ditto, endpointPool
What type of PR is this?
/kind feature
What this PR does / why we need it:
See #1779
Which issue(s) this PR fixes:
Part of #1779
Does this PR introduce a user-facing change?: