Upgrade the inferencePool selector to a struct from a map. #1330

zetxqx · 2025-08-07T22:07:34Z

Change the InferencePool.Selector from map[string]string to a struct for future flexibility in v1.

netlify · 2025-08-07T22:07:40Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`4cb6453`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/6896293851e73100080ec173
😎 Deploy Preview	https://deploy-preview-1330--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2025-08-07T22:07:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zetxqx
Once this PR has been reviewed and has the lgtm label, please assign sergeykanzhelev for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-08-07T22:07:44Z

Hi @zetxqx. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

robscott · 2025-08-07T22:39:43Z

api/v1/shared_types.go

+
+// LabelSelector defines a query for resources based on their labels.
+// This simplified version uses only the matchLabels field.
+type LabelSelector struct {


Maybe call this SimpleLabelSelector or something similar so it's clear that this is a subset of the OSS type of the same name.

do we foresee adding more fields to this struct? My impression is we want to leave the possibility to grow this struct to the real LabelSelector

I didn't change the name for now, because my understanding is that this struct could grow to be not "simple". I keep the name as-is for now.

I agree with @zetxqx. Otherwise, a)SimpleLabelSelector will no longer be a simple selector when MatchExpressions is added or b) we will have to create a new LabelSelector type that mirrors upstream and implementations will have to refactor to use the new type.

capri-xiyue · 2025-08-07T22:47:47Z

I think in this PR, we also need to change https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/epp/controller/inferencepool_reconciler.go#L98, as the v1a2 InferencePool and v1 InferencePool are not exactly the same. The existing conversion via unstructured won't work. Need to do manual conversion.

zetxqx · 2025-08-08T16:12:13Z

@kfswain could I get a ok-to-test first?

kfswain · 2025-08-08T16:14:50Z

/ok-to-test

capri-xiyue · 2025-08-08T16:16:56Z

apix/v1alpha2/inferencepool_conversion.go

+	return convert[EndpointPickerConfig](u)
+}
+
+func toUnstructured(obj any) (*unstructured.Unstructured, error) {


Can you use the common util in https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/common/convert.go? At the same time, I think you can remove the

var ToInferencePool = convert[v1.InferencePool] var ToXInferencePool = convert[v1alpha2.InferencePool]

in the common file.

I deleted the common/convert.go and moved it here.

I think if you still need to use func toUnstructured(obj any) (*unstructured.Unstructured, error) in the codebase, it's better to leave it in common package instead of specific apix package as they are common util. But we can wait until cong to make a call to see whether we need to do manual conversion for all fields.

capri-xiyue · 2025-08-08T16:20:30Z

apix/v1alpha2/inferencepool_conversion.go

+	return dst, nil
+}
+
+func converStatusToV1(src InferencePoolStatus) (*v1.InferencePoolStatus, error) {


As we use manual conversation for label selector, I'm wondering whether it makes more sense to use manual conversion for all fields now. I know the code maybe a little bit cumbersome. But maybe AI tool can help. I know such cumbersome manual conversion is widely used in OSS, see https://github.com/google/knative-gcp/blob/4a435faedc46726299800e1cdf2ad998c357b25b/pkg/apis/convert/conversion_helper.go

Let's leave it to go readability reviewer @liu-cong to make a call.

apix/v1alpha2/inferencepool_conversion_test.go

danehans · 2025-08-08T17:06:16Z

api/v1/shared_types.go

+	// An object must match every label in this map to be selected.
+	// The matching logic is an AND operation on all entries.
+	// +optional
+	MatchLabels map[LabelKey]LabelValue `json:"matchLabels,omitempty" protobuf:"bytes,1,rep,name=matchLabels"`


I thought we want MatchLabels to mirror upstream?

// matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels // map is equivalent to an element of matchExpressions, whose key field is "key", the // operator is "In", and the values array contains only "value". The requirements are ANDed. // +optional MatchLabels map[string]string `json:"matchLabels,omitempty" protobuf:"bytes,1,rep,name=matchLabels"`

Good point, I wanted to leverage the LabelKey and LabelValue because I saw they have some kubebuiler validation on it. Does that need to be kept? If not, I'll change to map[string]string for the simplicity.

// +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 // +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9]$` type LabelKey string

// +kubebuilder:validation:MinLength=0 // +kubebuilder:validation:MaxLength=63 // +kubebuilder:validation:Pattern=`^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$` type LabelValue string

nirrozenbaum · 2025-08-08T17:12:55Z

out of curiosity, I'm looking in this PR and thinking - why actually we don't want to use k8s label selector as is?
what's the problem of having MatchExpressions?
we can reuse the k8s upstream packages for labels matchers in code.

zetxqx · 2025-08-08T17:59:15Z

out of curiosity, I'm looking in this PR and thinking - why actually we don't want to use k8s label selector as is? what's the problem of have MatchExpressions? we can reuse the k8s upstream packages for labels matchers in code.

Great question @nirrozenbaum

Here's Rob's origional comment on this. #1173 (comment) cc: @robscott to give more insights.

Assuming this is a common implementation details, a lot of gateway implementations may not support the MatchExpressions very soon.

nirrozenbaum · 2025-08-10T17:33:09Z

I'm not sure I fully got the explanation in that comment.

This is an intentional omission as many implementations are stuck creating a Service behind the scenes to get EndpointSlices, so we want the selector that's specified here to be able to map to Service as long as that's a common implementation detail (I'd hope this would be temporary). It could be worth trying to find a way to structure this field in a way that allows for a more fully featured selector in the future though. I think that would mean copying the upstream LabelSelector type and temporarily omitting matchExpressions.

@robscott can you explain a bit more in detail?
InferencePool LabelSelector is used to select pods for the pool. the pods are currently reconciled using a pod reconciler (used to be endpoint slice reconciler in the past) and then stored in datastore.

I understand from the above comment that Gateways implementation are creating service to get EndpointSlices, but how is that related to the InferencePool LabelSelector? I lost you in the middle.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2025

k8s-ci-robot requested review from ahg-g and nirrozenbaum August 7, 2025 22:07

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 7, 2025

zetxqx marked this pull request as draft August 7, 2025 22:08

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2025

zetxqx mentioned this pull request Aug 7, 2025

v1.0 InferencePool API Review #1173

Open

robscott reviewed Aug 7, 2025

View reviewed changes

zetxqx force-pushed the issue1327 branch from 2349528 to 8bc537a Compare August 8, 2025 01:55

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 8, 2025

zetxqx force-pushed the issue1327 branch 2 times, most recently from daf936f to 886a0c4 Compare August 8, 2025 16:07

zetxqx marked this pull request as ready for review August 8, 2025 16:09

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 8, 2025

capri-xiyue suggested changes Aug 8, 2025

View reviewed changes

Upgrade the inferencePool selector to a struct from a map.

4cb6453

zetxqx force-pushed the issue1327 branch from 886a0c4 to 4cb6453 Compare August 8, 2025 16:43

danehans reviewed Aug 8, 2025

View reviewed changes

robscott mentioned this pull request Aug 8, 2025

InferencePool's pod selector cannot select multiple ports within a pod needed for data parallel attention #1336

Open

Upgrade the inferencePool selector to a struct from a map. #1330

Are you sure you want to change the base?

Upgrade the inferencePool selector to a struct from a map. #1330

Conversation

zetxqx commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

k8s-ci-robot commented Aug 7, 2025

Uh oh!

k8s-ci-robot commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

capri-xiyue commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zetxqx commented Aug 8, 2025

Uh oh!

kfswain commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zetxqx commented Aug 8, 2025

Uh oh!

nirrozenbaum commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zetxqx commented Aug 7, 2025 •

edited

Loading

netlify bot commented Aug 7, 2025 •

edited

Loading

capri-xiyue commented Aug 7, 2025 •

edited

Loading

nirrozenbaum commented Aug 8, 2025 •

edited

Loading

nirrozenbaum commented Aug 10, 2025 •

edited

Loading