v1.0 InferencePool API Review #1173

capri-xiyue · 2025-07-16T19:06:41Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR is a diff of /apis from alpha (main branch) to v1.0 (release-1.0 branch).

Note: This PR is purely to facilitate review, it is not intended to merge.

Changes:

group change from "inference.networking.x-k8s.io" to "inference.networking.k8s.io"
Change the InferencePool.Selector from map[string]string to a struct for future flexibility in v1. See Upgrade the inferencePool selector to a struct from a map. #1330
Simplify EndpointPickerConfig(remove the whole inline struct)
TargetPortNumber int32 to become TargetPorts []Port see feat: TargetPortNumber int32 to become TargetPorts []Port #1354

/assign @robscott

k8s-ci-robot · 2025-07-16T19:06:50Z

Hi @capri-xiyue. 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.

capri-xiyue · 2025-07-16T19:07:01Z

/hold
Don't merge as it is just for api review.

netlify · 2025-07-16T19:07:12Z

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

Name	Link
🔨 Latest commit	`6c02159`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/687811ca8fb1b50008233e1f
😎 Deploy Preview	https://deploy-preview-1173--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.

capri-xiyue · 2025-07-16T20:53:02Z

/hold, this should not get merged

robscott · 2025-07-17T06:11:54Z

Note: This PR is NOT intended to merge, it is entirely for the purpose of API review.

/cc @aojea @danwinship @thockin

JoelSpeed · 2025-08-22T15:12:44Z

I lost the comment thread, but I believe I saw something along the lines of:

A decision has been made to stick to pointers for optional structs, but pointers only when required for scalars

If that's the case, you can set KALs config for optionalfields.omitzero.policy: Forbid and it will behave as you intend.

Omitzero support is only for structs in KAL right now, and with it set to forbid, it will force the omitempty route with a pointer for all optional structs

danehans · 2025-08-22T17:35:50Z

FYI #1427 is a PR under review that implements several API changes based on feedback from API reviewers here and discussions among the community, maintainers, etc.

danehans · 2025-08-22T17:47:33Z

Given it is consistent, I am satisified. Implementations MUST use RV preconditions. If there is a v2, I would suggest to inline the Ref here and use those fields as the map key.

@thockin the challenge I see with inlining is that no field by itself can be used as the map key:

type ParentReference struct {
	// Group is the group of the referent API object. When unspecified, the referent is assumed
	// to be in the "gateway.networking.k8s.io" API group.
	//
	// +optional
	// +kubebuilder:default="gateway.networking.k8s.io"
	Group *Group `json:"group,omitempty"`

	// Kind is the kind of the referent API object. When unspecified, the referent is assumed
	// to be a "Gateway" kind.
	//
	// +optional
	// +kubebuilder:default=Gateway
	Kind *Kind `json:"kind,omitempty"`

	// Name is the name of the referent API object.
	//
	// +required
	Name ObjectName `json:"name,omitempty"`

	// Namespace is the namespace of the referenced object. When unspecified, the local
	// namespace is inferred.
	...
	// +optional
	Namespace *Namespace `json:"namespace,omitempty"`
}

All fields are needed to ensure the uniqueness of a parent.

capri-xiyue · 2025-08-22T22:49:38Z

I lost the comment thread, but I believe I saw something along the lines of:
A decision has been made to stick to pointers for optional structs, but pointers only when required for scalars
If that's the case, you can set KALs config for optionalfields.omitzero.policy: Forbid and it will behave as you intend.

Omitzero support is only for structs in KAL right now, and with it set to forbid, it will force the omitempty route with a pointer for all optional structs

I tried it, with omitzero forbid, https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1438/files#diff-8d7db842a7673f66c7db001de9fe07ce67b67e21d8cbce12394fde46eea1e5b7R36 I still need ignore kubeapi linter otherwise it will also show optionalfields: field Kind does not allow the zero value. The field does not need to be a pointer. (kubeapilinter) Kind *Kind `json:"kind,omitempty" for optional pointer struct where zero value is not allowed

robscott · 2025-08-22T23:42:26Z

@thockin @aojea I believe we've addressed all your feedback, thanks! PTAL

JoelSpeed · 2025-08-23T07:39:10Z

I still need ignore kubeapi linter otherwise it will also show optionalfields: field Kind does not allow the zero value. The field does not need to be a pointer. (kubeapilinter) Kind *Kind `json:"kind,omitempty" for optional pointer struct where zero value is not allowed

Kind is not a struct, it is a string. You have a non-zero minimum length marker on kind so why do we need it to be a pointer? The string being empty implies to the go client that it wasn't present when admitted

JoelSpeed · 2025-08-23T07:40:53Z

apix/v1/inferencepool_types.go

+	// Status defines the observed state of the InferencePool.
+	//
+	// +optional
+	//nolint:kubeapilinter // status should not be a pointer.


Why not? It's the same as any other struct

We followed the existing k8s convention. See https://github.com/kubernetes/api/blob/master/batch/v1/types.go#L84

@JoelSpeed I agree that Status should be a pointer type, but we could not find other K8s APIs that follow this convention. We prefer to follow established patterns.

In theory every optional struct-field should be a pointer. In practice, we do not do that (in part because we have had no linter).

I am OK with this because it is "not worse than" everything else in the system. We should refine the rules (the doc which emerged from this discussion)

Joel, are you OK to resolve this one?

I think we should preserve the right to the projects to decide to switch to the new conventions meanwhile they adhere to the long established patterns that we know are safe

I'm ok with adding an exception, but can we please use the more specific golangci-lint exceptions in the config file that ignore specific messages rather than blanket ignoring the whole KAL suite for this field?

@JoelSpeed can you share a pointer for how we could do that?

Apologies for getting to this so late, but, an example would be how CAPI has done so, https://github.com/kubernetes-sigs/cluster-api/blob/0db4bf4fd9ec5cea39d1853fd4b2e877788b46df/.golangci-kal.yml#L86-L91

It's directly in the golangci config with a path regex and text regex to match on.

apix/v1/inferencepool_types.go

JoelSpeed · 2025-08-23T07:49:51Z

Further to my comment above, did we decide all optional fields should be pointers, or, only optional structs? Cc @robscott

I thought the latter, but the exceptions I'm seeing imply the former

Either decision can be configured in KAL so we shouldn't need any exceptions apart from the port number where a specific decision was made to ignore the guidance about the zero value

danehans · 2025-08-25T15:25:09Z

Note that when the API is approved, we need to resolve #1424.

danehans · 2025-08-25T16:48:50Z

Kind is not a struct, it is a string. You have a non-zero minimum length marker on kind so why do we need it to be a pointer? The string being empty implies to the go client that it wasn't present when admitted

We are trying to provide consistency with Gateway API, where Kind is a pointer (xref). @robscott it's my understanding that kubeapilinter is being used in the Gateway API repo. If so, is //nolint:kubeapilinter not needed for the Kind *Kind field because the linter is disabled for v1 APIs?

thockin · 2025-08-25T18:14:15Z

the challenge I see with inlining is that no field by itself can be used as the map key:
...
All fields are needed to ensure the uniqueness of a parent.

Map keys can be a set of fields, they just can't be deeper nested.

capri-xiyue · 2025-08-25T22:44:37Z

cc @aojea @thockin The API is ready for final review. Can you help review it and grant approval? Thank you very much!

thockin · 2025-08-26T16:38:11Z

apix/v1/inferencepool_types.go

+	// Status defines the observed state of the InferencePool.
+	//
+	// +optional
+	//nolint:kubeapilinter // status should not be a pointer.


Joel, are you OK to resolve this one?

thockin · 2025-08-26T16:47:24Z

apix/v1/inferencepool_types.go

+	Name ObjectName `json:"name,omitempty"`
+
+	// PortNumber is the port number of the Endpoint Picker extension service. When unspecified,
+	// implementations SHOULD infer a default value of 9002 when the kind field is "Service" or


Rob, we discussed this, but I forget what we came to. Do we have good precedent for "should infer" as opposed to just setting a default?

To clarify, the default value of 9002 only applies when the kind filed is a service or unspecified (defaults to "Service").

My point is that telling clients to infer a value is generally not what we do. We either give it an explicit default or we require the user to set it.

You can bundle an MAP which looks at Group and Kind, and assigns 9002, but the validation for it would be "required". Or you can assign a static default of 9002 and leave it optional.

cc @robscott for the context of inferring here.

Sorry I'd forgotten about this one! Fundamentally we're trying to leave room open for a type of backend where port defined at this level would either be meaningless or repetitive. IE imagine that we want to point to a different kind of backend that only ever has one port, and that port is inferred by the type. Why would we require port here?

We could follow the model that Gateway API uses here which would be to require port in cases where it's ambiguous, such as multi-port Services. With that said, unlike Gateway API, we already know that the most common case will be a Service listening on port 9002, so it seems like a shame to require that input from users in the common case.

Unfortunately "only require a field when the target has multiple ports, and one of them is not 9002" is not something that I can find a good way to express with any of the validations we have available to us today.

You can bundle an MAP which looks at Group and Kind, and assigns 9002
I think that makes a lot of sense as MAP becomes more broadly available, but it will still be in alpha when this release happens. We can file a follow up issue to bundle a MAP when it reaches beta.

So that's a long way of saying that I think the current state is the least bad way to achieve the following combination:

Sensible defaults that cover the common case and avoid unnecessary inputs - this default can be translated from controller-inferred to MAP in the future as it's available

Room for targeting other backend types where port would not be meaningful

Consistency with existing APIs (alpha version of this API + Gateway API BackendRef)

I have seen no other APIs do this

Obviously very biased here, but IMO this same pattern has worked reasonably well for Gateway API. Some examples of where this has been valuable:

GKE, Envoy Gateway, and others support targeting ServiceImport from Routes to effectively provide "Multi-Cluster Gateways"

AgentGateway uses Gateway API Routes and targets a custom MCP Backend type

Gateway for Mesh is based on a creative use of parentRefs on Routes. See Istio's docs for an example, but this pattern has become widespread across meshes

Carrying this a fuller normalization, why is it not even more abstract, like

type EndpointPicker struct { // One of the following must be specified // A reference to a kubernetes Service Service *ServiceRef // An arbitrary thing on the network Network *NetworkRef // A reference to an arbitrary kubernetes object Object *ObjectRef

Doesn't the presence of ObjectRef in that example invalidate the idea of ServiceRef? It would provide more than one way to point to a Service which feels suboptimal. If on the other hand we exclude ObjectRef, we're missing out on some very real opportunity for innovation and extension from the implementations of this API. Honestly I think one of the strongest parts of Gateway API is that there's a well defined core with plenty of extension points that allow you to plug in your own custom type of Route or Backend while still retaining the rest of the API. I'd hate to give that up here.

Some examples of where this has been valuable

What is "this". To me "this" is defining an implcit default value in the documentation, and imposing that work on clients and/or implementations.

Doesn't the presence of ObjectRef in that example invalidate the idea of ServiceRef?

No - a ServiceRef can a) be simpler and b) carry more assumptions. You don't need group or kind, and you can safely assume that "port" is meaningful. Or you can just assert a fixed port number (maybe?). You can, of course, represent a Service as an ObjectRef, but the API is less precise and more generic.

I'm not saying to necessarily do exactly this, I am saying that an implicit, conditional default is more than a little bit smelly.

Chatted offline with @thockin and we agreed that although this is not perfect, mirroring Gateway API behavior here is a reasonable path. That means two things:

We should remove the inferred default of 9002

We should require port to be specified when the target kind is Service (this will require some CEL validation)

Change needed is included in #1484

I've updated PR to reflect the change. Let me know whether it works now or not.

thockin · 2025-08-26T16:49:38Z

apix/v1/inferencepool_types.go

+	//
+	// +optional
+	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer as zero means all ports in convention, we don't make to use 0 to indicate not set.
+	PortNumber *PortNumber `json:"portNumber,omitempty"`


why "PortNumber" vs "Port" ?

Will we need protocol here eventually? Did we discuss this one?

Why not use the same Port struct?

@robscott Do you have any context about this?

Good point. I think the ideal would be to mirror BackendObjectReference in GW API where we have a port field with the same validation instead of portNumber. I think this started as portNumber to avoid any confusion over portName, but given that we've used port successfully for years in GW API without any issues or need for struct/other fields, I'd expect that to work well here as well.

I'm a bit split on whether or not we should make the change from portNumber to port at this late stage. Will defer to @danehans, @kfswain, and @nirrozenbaum.

I’m fine with this change. portNumber was introduced only in v1 (different field exists in v1alpha2) and as long as the api has not been finalized, that’s ok to change.
we do need to keep in mind that v1 rc1 was already cut, but if we feel strongly that this is an improvement let’s go for it.

This is the PR of approach 3 to make two different ports symmetric #1484

why "PortNumber" vs "Port" ?

To reduce the nesting within EndpointPickerRef. To your point, if named ports and app protocol are anticipated future requirements, then +1 to use type Port instead of PortNumber. Currently, this spec defines the Gateway <> EPP protocol as ext-proc (gRPC), so I'm unsure if any additional app protocols are needed. Thoughts @robscott @kfswain @nirrozenbaum

I've updated PR to reflect the change. Let me know whether it works now or not.

Yeah, +1. I think further app protocols may be useful at some point in the future, but it's not necessary now.

As mentioned by @capri-xiyue, this has been addressed by #1484.

apix/v1/inferencepool_types.go

k8s-ci-robot · 2025-08-27T22:33:53Z

@capri-xiyue: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gateway-api-inference-extension-test-unit-main	`03ca5d9`	link	true	`/test pull-gateway-api-inference-extension-test-unit-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2025-08-28T00:38:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: capri-xiyue, thockin
Once this PR has been reviewed and has the lgtm label, please assign danehans 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

nirrozenbaum · 2025-09-08T19:22:38Z

may we close this as completed?

robscott · 2025-09-08T20:09:12Z

Yep, thanks @nirrozenbaum!

/close

k8s-ci-robot · 2025-09-08T20:09:18Z

@robscott: Closed this PR.

In response to this:

Yep, thanks @nirrozenbaum!

/close

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 assigned robscott Jul 16, 2025

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 16, 2025

k8s-ci-robot requested review from kfswain and nirrozenbaum July 16, 2025 19:06

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 Jul 16, 2025

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 16, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2025

capri-xiyue closed this Jul 16, 2025

capri-xiyue force-pushed the capri-xiyue/capri-xiyue-v1-api-review branch from 5317094 to 4ffb5f6 Compare July 16, 2025 20:36

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 16, 2025

capri-xiyue reopened this Jul 16, 2025

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2025

capri-xiyue closed this Jul 16, 2025

capri-xiyue force-pushed the capri-xiyue/capri-xiyue-v1-api-review branch from 77371fe to 4ffb5f6 Compare July 16, 2025 20:53

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 16, 2025

capri-xiyue reopened this Jul 16, 2025

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2025

capri-xiyue mentioned this pull request Aug 22, 2025

fork daneyon's change - Refactors InferencePool v1 Status and FailureModes #1438

Closed

robscott mentioned this pull request Aug 22, 2025

Final bits of v1.0 API cleanup #1441

Merged

final clean up

33c8e89

JoelSpeed reviewed Aug 23, 2025

View reviewed changes

clean up pointer

5033512

thockin reviewed Aug 26, 2025

View reviewed changes

capri-xiyue mentioned this pull request Aug 27, 2025

cleanup: make port definitions symmetric and clean endpointPickerRef.port #1484

Merged

capri-xiyue added 2 commits August 27, 2025 13:55

clean up ports

8c582e3

updated message

03ca5d9

thockin reviewed Aug 28, 2025

View reviewed changes

k8s-ci-robot assigned thockin Aug 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2025

k8s-ci-robot closed this Sep 8, 2025

mathetake mentioned this pull request Sep 8, 2025

Support Gateway Inference Extension's V1 InferencePool envoyproxy/ai-gateway#1032

Open

v1.0 InferencePool API Review #1173

v1.0 InferencePool API Review #1173

Uh oh!

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 16, 2025

Uh oh!

capri-xiyue commented Jul 16, 2025

Uh oh!

netlify bot commented Jul 16, 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!

capri-xiyue commented Jul 16, 2025

Uh oh!

robscott commented Jul 17, 2025

Uh oh!

JoelSpeed commented Aug 22, 2025

Uh oh!

danehans commented Aug 22, 2025

Uh oh!

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

Uh oh!

Uh oh!

capri-xiyue commented Aug 22, 2025

Uh oh!

robscott commented Aug 22, 2025

Uh oh!

JoelSpeed commented Aug 23, 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!

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!

Uh oh!

Uh oh!

JoelSpeed commented Aug 23, 2025

Uh oh!

danehans commented Aug 25, 2025

Uh oh!

danehans commented Aug 25, 2025

Uh oh!

thockin commented Aug 25, 2025

Uh oh!

capri-xiyue commented Aug 25, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

capri-xiyue commented Jul 16, 2025 •

edited

Loading

netlify bot commented Jul 16, 2025 •

edited

Loading

danehans commented Aug 22, 2025 •

edited

Loading

robscott Aug 26, 2025 •

edited

Loading

nirrozenbaum Aug 26, 2025 •

edited

Loading

capri-xiyue Aug 27, 2025 •

edited

Loading