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	`487eef3`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/689a824a8f816c00096abacc
😎 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: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.

api/v1/shared_types.go

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

apix/v1alpha2/inferencepool_conversion.go

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

+1 on using map[string]string. I don't think this kubebuilder validation is required.
as a side note, service label selector also doesn't have any kubebuilder validation

Sadly we need the CRD validation @zetxqx added here to maintain compatibility with upstream validation. When defining built-in APIs, that validation can be written in Go, and that's what happened here. When reusing the same types in CRDs, it's critical for us to try to replicate that validation with CEL, as @zetxqx is doing in this PR.

I think there's some corresponding upstream efforts to move more validation to annotations in go types, and that would help in cases like this.

we can keep LabelKey/LabelValue if that is needed 👍

Yes, "Declarative Validation" (KEP-5073) is the upstream effort to replace validation in build-in APIs that is handwritten in Go with annotations.

One of the goals of that project is to make it so that if CRDs import native types (e.g. "PodSpec") into CRDs, either via Kubebuilder or via OpenAPI, that the validation of the native type is preserved.

It's unlikely that we will preserve 100% of validation, since there are some really complicated validation rules in the build-in types, but we believe we can preserve a lot of it.

To clarify, for kubebuilder, our intention is to have kubebuilder understand and support validation of the annotations we add (we call them tags, and they usually look like this: // +k8s:maximum=5). For openAPI we intent to publish the validation rules either using native OpenAPI value validation rules, or, if needed, by publishing the CEL expressions to perform the validation.

Thanks you all! I'll keep the LabelKey/LabelValue.

@lalitc375 @cheftako for visibility

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.

zetxqx · 2025-08-11T15:40:58Z

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.

@nirrozenbaum I see what's the confusing part. Here @robscott is talking about the implementation details on the gateway controller side. You are talking about the reconciler on the EPP side. In the gateway implementation, inference pool is mostly translated to a service. Specifically, the inferencePool.Selector is converted to service's selector under ServiceSpec. And its type is map[string]string. So if we upgrade the selector inferencePool to the new k8s labelSelector we don't have an equivalent label on the service.

LiorLieberman · 2025-08-11T15:41:38Z

/cc

nirrozenbaum · 2025-08-11T17:38:02Z

@nirrozenbaum I see what's the confusing part. Here @robscott is talking about the implementation details on the gateway controller side. You are talking about the reconciler on the EPP side. In the gateway implementation, inference pool is mostly translated to a service. Specifically, the inferencePool.Selector is converted to service's selector under ServiceSpec. And its type is map[string]string. So if we upgrade the selector inferencePool to the new k8s labelSelector we don't have an equivalent label on the service.

@zetxqx thanks for the explanation. I think I've never really got into the details of how gateways are implementing the translation to InferencePool from the GW side. this makes sense, it's always good to learn :).

now that it's all clearer to me:
I support the upgrade of the label selector to a struct instead of a direct map.
I also support dropping the use of LabelKey/LabelValue and go with map[string]string. I don't think the kubebuilder validation was really in use.

pkg/epp/controller/inferencepool_reconciler.go

nirrozenbaum · 2025-08-11T18:03:39Z

can you update also e2e tests manifest and the helm chart template along with its documentation?

apix/v1alpha2/inferencepool_conversion.go

zetxqx · 2025-08-11T18:58:02Z

can you update also e2e tests manifest and the helm chart template along with its documentation?

Good call, updated accordingly

zetxqx · 2025-08-11T20:04:54Z

/test pull-gateway-api-inference-extension-test-unit-main

robscott

Thanks @zetxqx!

robscott · 2025-08-11T22:01:34Z

api/v1/shared_types.go

+	// matchLabels contains a set of required {key,value} pairs.
+	// An object must match every label in this map to be selected.
+	// The matching logic is an AND operation on all entries.
+	// +optional


Since the previous field was required, I think this should be too. I'd also recommend adding +kubebuilder:validation:MaxItems to some relatively high value (maybe 32 or 64) for the sake of always having an upper bound on resource size.

Thanks @robscott

Added the following validation comment.

// +kubebuilder:validation:Required // +kubebuilder:validation:MaxItems=64

QQ: for the required validation, in the future if we add new field to this struct, it may become not required. Could that happen? In that situation, will it be easy to downgrade required to optional? I think at least we can have a new API version for that change as the last resort.

QQ: for the required validation, in the future if we add new field to this struct, it may become not required. Could that happen? In that situation, will it be easy to downgrade required to optional? I think at least we can have a new API version for that change as the last resort.

Yep, it's possible to loosen validation in the future, but impossible to retroactively tighten validation, that's why we usually try to start with validation that's as complete as possible.

k8s-ci-robot · 2025-08-11T22:02:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robscott, 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

robscott · 2025-08-11T22:58:46Z

Thanks @zetxqx!

LGTM once you rerun CRD generation.

zetxqx · 2025-08-11T23:55:02Z

Thanks @zetxqx!

LGTM once you rerun CRD generation.

My miss, re-generated. @robscott

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

api/v1/shared_types.go Show resolved Hide resolved

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

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

k8s-ci-robot requested a review from LiorLieberman August 11, 2025 15:41

nirrozenbaum reviewed Aug 11, 2025

View reviewed changes

pkg/epp/controller/inferencepool_reconciler.go Show resolved Hide resolved

nirrozenbaum reviewed Aug 11, 2025

View reviewed changes

apix/v1alpha2/inferencepool_conversion.go Show resolved Hide resolved

zetxqx force-pushed the issue1327 branch from 4cb6453 to 504ebf8 Compare August 11, 2025 18:54

Upgrade the inferencePool selector to a struct from a map.

d7ab27d

zetxqx force-pushed the issue1327 branch from 504ebf8 to d7ab27d Compare August 11, 2025 20:57

robscott approved these changes Aug 11, 2025

View reviewed changes

add validation comment.

0307186

capri-xiyue mentioned this pull request Aug 11, 2025

feat: TargetPortNumber int32 to become TargetPorts []Port #1354

Open

rerun make generate.

487eef3

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!

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!

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!

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!

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!

zetxqx commented Aug 11, 2025

Uh oh!

LiorLieberman commented Aug 11, 2025

Uh oh!

nirrozenbaum commented Aug 11, 2025

Uh oh!

Uh oh!

nirrozenbaum commented Aug 11, 2025

Uh oh!

Uh oh!

zetxqx commented Aug 11, 2025

Uh oh!

zetxqx commented Aug 11, 2025

Uh oh!

robscott left a comment

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!

k8s-ci-robot commented Aug 11, 2025

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