Refactors InferencePool v1 Status and FailureModes #1427

danehans · 2025-08-21T17:00:21Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Refactors InferencePool v1 status.
Expands godocs for InferencePool v1 status.
Makes the Namespace and Kind fields of a ParentRef optional to match Gateway API ref semantics.
Adds the correct typeMeta version in InferencePool v1 <> v1alpha2 conversion.
Commit 77831c7 renames the FailureMode types from ExtensionXX to EndpointPickerXXX.
Commit 4abcaab aligns v1 InferencePool API types with K8s built-in API and Gateway API conventions, e.g., optional fields are pointer types.
Commit 88990b0 runs the API docs generators.

Which issue(s) this PR fixes:

Supports: #1173

Does this PR introduce a user-facing change?:

Make v1 Pool status conditions required.

k8s-ci-robot · 2025-08-21T17:00:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans

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:

~~OWNERS~~ [danehans]

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

netlify · 2025-08-21T17:01:24Z

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

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

robscott · 2025-08-21T17:16:50Z

/cc @robscott @capri-xiyue @zetxqx

capri-xiyue · 2025-08-21T17:48:44Z

apix/v1alpha2/inferencepool_conversion.go

+
+	meta := metav1.TypeMeta{
+		Kind:       src.Kind,
+		APIVersion: v1.GroupVersion.Version, // Ensure the API version is set correctly.


api version is not just version. API version is a combination of Group and Version. You can see how the UT fails now. See an example here https://kubernetes.io/docs/concepts/overview/working-with-objects/#describing-a-kubernetes-object

Fixed in 574e13e

capri-xiyue · 2025-08-21T17:50:13Z

apix/v1alpha2/inferencepool_conversion.go

@@ -39,11 +41,20 @@ func (src *InferencePool) ConvertTo(dst *v1.InferencePool) error {
 	if err != nil {
 		return err
 	}
-	dst.TypeMeta = src.TypeMeta


Why not use this. TypeMeta is all built-in string type, see https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#TypeMeta?

See #1427 (comment)

Based on https://kubernetes.slack.com/archives/C08E3RZMT2P/p1755801705394689, consensus is for the conversion functions to convert APIVersion instead of keeping the source APIVersion.

apix/v1alpha2/inferencepool_conversion.go

apix/v1alpha2/inferencepool_conversion_test.go

apix/v1alpha2/inferencepool_conversion.go

api/v1/inferencepool_types.go

apix/v1alpha2/inferencepool_conversion.go

apix/v1alpha2/shared_types.go

api/v1/shared_types.go

danehans · 2025-08-21T19:33:27Z

@capri-xiyue @capri-xiyue based on https://kubernetes.slack.com/archives/C08E3RZMT2P/p1755801705394689, consensus is for the conversion functions to convert APIVersion instead of keeping the source APIVersion.

danehans · 2025-08-21T19:37:04Z

api/v1/inferencepool_types.go

 	// Defaults to failClose.
 	//
 	// +optional
 	// +kubebuilder:default="FailClose"
 	FailureMode ExtensionFailureMode `json:"failureMode,omitempty"`
 }

-// ExtensionFailureMode defines the options for how the gateway handles the case when the extension is not
+// ExtensionFailureMode defines the options for how the parent handles the case when the extension is not


@robscott since we renamed ExtensionRef to EndpointPickerRef, it seems like FailureModes should be EPP-specific, e.g., ExtensionFailureMode -> PickerFailureMode

Commit 77831c7 renames the FailureMode types from ExtensionXX to EndpointPickerXXX.

danehans · 2025-08-21T19:40:11Z

api/v1/inferencepool_types.go

 	FailOpen ExtensionFailureMode = "FailOpen"
 	// FailClose specifies that the proxy should drop the request when the Endpoint Picker fails.
 	FailClose ExtensionFailureMode = "FailClose"
 )

-// InferencePoolStatus defines the observed state of InferencePool.
-// +kubebuilder:validation:MinProperties=1


I removed +kubebuilder:validation:MinProperties=1 since the struct contains only one field, and the field is optional.

api/v1/inferencepool_types.go

capri-xiyue · 2025-08-21T23:30:49Z

apix/v1alpha2/inferencepool_conversion.go

-	return convert[v1.InferencePoolStatus](u)
+
+	// v1alpha2 used json:"parent" (singular) v1 uses json:"parents"
+	if v, ok := u.Object["parent"]; ok {


I'm wondering instead of leverage unstructured and then change it, can we just use manual conversion without u, err := toUnstructured(src), will it be more clear? For ExtensionRef conversion, it is all manual conversion instead of using toUnstructured. Manual conversion could be cumbersome, but I think ai coding tool can help write it.

capri-xiyue · 2025-08-21T23:30:58Z

apix/v1alpha2/inferencepool_conversion.go

-	return convert[InferencePoolStatus](u)
+
+	// v1 uses json:"parents"; v1alpha2 used json:"parent" (singular)
+	if v, ok := u.Object["parents"]; ok {


robscott

Thanks @danehans! A few nits but mostly LGTM

api/v1/inferencepool_types.go

robscott · 2025-08-22T00:49:28Z

api/v1/inferencepool_types.go

@@ -65,19 +67,24 @@ type InferencePoolSpec struct {
 	// this configuration into a Service resource.
 	//
 	// +required
-	Selector LabelSelector `json:"selector,omitempty,omitzero"`
+	//nolint:kubeapilinter // ignore kubeapilinter here as we don't want to use pointer here.


Why would kubeapilinter want us to use a pointer here?

robscott · 2025-08-22T00:52:27Z

api/v1/inferencepool_types.go

-	// +kubebuilder:default={{type: "Accepted", status: "Unknown", reason:"Pending", message:"Waiting for controller", lastTransitionTime: "1970-01-01T00:00:00Z"}}
-	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	//nolint:kubeapilinter // ignore kubeapilinter here as we want conditions to be required.
+	Conditions []metav1.Condition `json:"conditions"`


My understanding is that omitempty should still be used everywhere, even for required fields. It likely doesn't matter here, but probably worth being consistent with the rest of the API.

My understanding is that omitempty should still be used everywhere, even for required fields.

I reviewed several K8s API and GW API types, and I do not see the omitempty JSON tag for any required fields. However, I do see Conditions set as +optional and the K8s API conventions state Conditions should be +optional, so I'm going to revert this portion of the PR. Since the field will be +optional, it will also include omitempty.

robscott · 2025-08-22T00:54:05Z

api/v1/inferencepool_types.go

 	//
 	// Possible reasons for this condition to be True are:
 	//
-	// * "Accepted"
+	// * "SupportedByParent"


We've always used Accepted as the only positive polarity reason for a resource to be Accepted in Gateway API, recommend keeping that here.

I have always found the Accepted reason to be unusual. The condition type is Accepted, and when being set to False, the reason is "NotSupportedByParent". The opposite should be set when Accepted: True, e.g., "SupportedByParent".

@robscott While on the topic of status condition reasons, PTAL at the following:

// This reason is used with the "Accepted" when a controller has not yet // reconciled the InferencePool. InferencePoolReasonPending InferencePoolReason = "Pending"

Now that the default status condition has been removed, I question whether this reason is still applicable. If a controller has not yet reconciled the resource, something else is required to set Accepted: Unknown with Reason: Pending.

Now that the default status condition has been removed, I question whether this reason is still applicable.

+1, let's remove it.

I have always found the Accepted reason to be unusual. The condition type is Accepted, and when being set to False, the reason is "NotSupportedByParent".

The problem is when you have multiple reasons that a condition could be false. Take HTTPRoute for an example. To follow your example, the positive condition would be something like "AllowedByListenersAndHasMatchingHostnameAndParentAndNoUnsupportedValues".

I think the odds that we add more reasons to be false in the future are quite here, so I'd rather stick with a simpler reason for the positive state here and follow what GW API has done.

robscott · 2025-08-22T00:55:48Z

api/v1/inferencepool_types.go

-	// This reason is used with the "Accepted" condition when the InferencePool is
-	// referenced by an HTTPRoute that has been rejected by the Gateway. The user
-	// should inspect the status of the referring HTTPRoute for the specific reason.
+	// InferencePoolReasonHTTPRouteNotAccepted is a reason used with the "Accepted"


Not a blocker, but it could be worth clarifying that this is optional. I don't think we want to require Gateways to jump through to HTTPRoute backends if the HTTPRoute is invalid. It's certainly nice to have, but seems rather onerous to require.

Will do. This reason would be set with Accepted: False if the referent HTTPRoute has a negative condition, e.g. Accepted: False, correct?

robscott · 2025-08-22T00:57:57Z

config/crd/bases/inference.networking.k8s.io_inferencepools.yaml

            properties:
-              parent:
+              parents:


This is a great catch, no idea how I'd missed this in review, thanks!

robscott · 2025-08-22T00:59:22Z

api/v1/inferencepool_types.go

 	//
 	// +optional
 	// +kubebuilder:default="FailClose"
-	FailureMode ExtensionFailureMode `json:"failureMode,omitempty"`
+	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer since empty means default value.


I don't think a pointer is required to set a default value, just "omitempty". My understanding is that this is only ambiguous for structs.

I'm trying to provide consistency across the API as follows:

Optional Fields

Use a pointer type (FailureMode *ExtensionFailureMode) or have a built-in nil value (maps and slices). The only caveat to this rule is InferencePoolStatus due to K8s API conventions for the status subresource.

Have the +optional kubebuilder marker.

Are marked with the omitempty JSON tag.

Required Fields

Are explicitly marked as required with the +required kubebuilder marker.

Are NOT marked with the omitempty JSON tag since the field should not be empty when being serialized.

Do not use pointer types (Name ObjectName).

Use the omitzero JSON tag to avoid marshalling the zero value.

K8s API conventions states "required fields where the zero value is a valid value must use pointer types, paired with an omitempty struct tag to avoid spurious null serializations.". Selector is a good example of this rule, but the v1 InferencePool API does not support an empty selector, e.g. select all Pods.

^ seems to be consistent with K8s API and GW API conventions.

This approach SGTM. While I think this likely undoes some of the less controversial kube-api-linter changes, it is consistent with how all previous releases of GW API have worked, so I'm fine with the idea of following some other project for bigger changes instead of being first here.

robscott · 2025-08-22T00:59:49Z

api/v1/inferencepool_types.go

 	//
 	// +required
-	Name ObjectName `json:"name,omitempty"`
+	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer since empty means default value.


We're not using pointer? (I don't think we want to here, I'm just guessing this is copied from another nolint line). In this case I think we can likely remove the nolint

robscott · 2025-08-22T01:00:06Z

api/v1/inferencepool_types.go

 	//
 	// +required
-	Name ObjectName `json:"name,omitempty"`
+	//nolint:kubeapilinter // ignore kubeapilinter here as we want to use pointer since empty means default value.
+	Name ObjectName `json:"name"`


I think all fields should still have omitempty

robscott · 2025-08-22T01:01:48Z

api/v1/inferencepool_types.go

+	//nolint:kubeapilinter // ignore kubeapilinter here since the field is required and we don't want omitempty tag.
+	TargetPorts []Port `json:"targetPorts"`


I think omitempty is fine here and all other fields which would allow us to remove nolint line.

danehans · 2025-08-22T17:11:01Z

@robscott here is how LabelSelector is defined in this PR:

type LabelSelector struct {
	...
	// +required
	// +kubebuilder:validation:MinItems=1
	// +kubebuilder:validation:MaxItems=64
	MatchLabels map[LabelKey]LabelValue `json:"matchLabels"`
}

We will need to transition MatchLabels to +optional when additional fields are added to LabelSelector, so I'm going to change this field to optional unless I hear otherwise.

robscott · 2025-08-22T17:30:39Z

We will need to transition MatchLabels to +optional when additional fields are added to LabelSelector, so I'm going to change this field to optional unless I hear otherwise.

We can always move from required to optional in the future, but we can't go the other way. My vote would be to keep this required until we add MatchExpressions, at which point we'd require that at least one of those fields was set.

Signed-off-by: Daneyon Hansen <[email protected]>

k8s-ci-robot · 2025-08-22T19:58:29Z

@danehans: 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	`6f89efd`	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.

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

I forked it and made it pass the linter #1438
But I need to change the the InferencePoolStatus to become an pointer as zero value is valid now. I think either it's a pointer or non-pointer, both works?

k8s-ci-robot · 2025-08-23T12:09:07Z

PR needs rebase.

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.

danehans · 2025-08-25T15:21:31Z

Closing in favor of #1441

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 21, 2025

k8s-ci-robot requested review from ahg-g and liu-cong August 21, 2025 17:00

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2025

danehans force-pushed the remove_inf_pool_def_condition branch from 20b8deb to d11b1b8 Compare August 21, 2025 17:15

k8s-ci-robot requested review from capri-xiyue, robscott and zetxqx August 21, 2025 17:16

danehans force-pushed the remove_inf_pool_def_condition branch from d11b1b8 to 06a15b7 Compare August 21, 2025 17:25

capri-xiyue reviewed Aug 21, 2025

View reviewed changes

danehans force-pushed the remove_inf_pool_def_condition branch from 06a15b7 to 574e13e Compare August 21, 2025 17:53

capri-xiyue reviewed Aug 21, 2025

View reviewed changes

apix/v1alpha2/inferencepool_conversion.go Outdated Show resolved Hide resolved

zetxqx reviewed Aug 21, 2025

View reviewed changes

capri-xiyue reviewed Aug 21, 2025

View reviewed changes

apix/v1alpha2/shared_types.go Outdated Show resolved Hide resolved

api/v1/shared_types.go Outdated Show resolved Hide resolved

danehans force-pushed the remove_inf_pool_def_condition branch from 574e13e to 78c9e01 Compare August 21, 2025 19:26

danehans commented Aug 21, 2025

View reviewed changes

danehans force-pushed the remove_inf_pool_def_condition branch 2 times, most recently from 8e50ce9 to 77831c7 Compare August 21, 2025 21:35

danehans changed the title ~~Refactors InferencePool v1 Status~~ Refactors InferencePool v1 Status and FailureModes Aug 21, 2025

capri-xiyue mentioned this pull request Aug 21, 2025

v1.0 InferencePool API Review #1173

Open

capri-xiyue reviewed Aug 21, 2025

View reviewed changes

api/v1/inferencepool_types.go Outdated Show resolved Hide resolved

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 21, 2025

danehans requested review from capri-xiyue and zetxqx August 21, 2025 23:15

capri-xiyue mentioned this pull request Aug 21, 2025

fix(cleanup) added pointer usage for failure mode #1428

Closed

capri-xiyue reviewed Aug 21, 2025

View reviewed changes

robscott reviewed Aug 22, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2025

danehans added 5 commits August 22, 2025 12:29

Refactors InferencePool v1 Status

6bf0bc4

Signed-off-by: Daneyon Hansen <[email protected]>

Renames Pool v1 FailMode Types

c377456

Signed-off-by: Daneyon Hansen <[email protected]>

Runs API docs generators

7357a5c

Signed-off-by: Daneyon Hansen <[email protected]>

Resolves latest kubeapilinter failures

69c946d

Signed-off-by: Daneyon Hansen <[email protected]>

Resolve robscott feedback

6f89efd

Signed-off-by: Daneyon Hansen <[email protected]>

danehans force-pushed the remove_inf_pool_def_condition branch from 8550ca8 to 6f89efd Compare August 22, 2025 19:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2025

danehans closed this Aug 25, 2025

		//nolint:kubeapilinter // ignore kubeapilinter here since the field is required and we don't want omitempty tag.
		TargetPorts []Port `json:"targetPorts"`

Refactors InferencePool v1 Status and FailureModes #1427

Refactors InferencePool v1 Status and FailureModes #1427

Uh oh!

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 21, 2025

Uh oh!

netlify bot commented Aug 21, 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!

robscott commented Aug 21, 2025

Uh oh!

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

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danehans commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

danehans commented Aug 21, 2025 •

edited

Loading

netlify bot commented Aug 21, 2025 •

edited

Loading

capri-xiyue Aug 21, 2025 •

edited

Loading

danehans Aug 21, 2025 •

edited

Loading

capri-xiyue Aug 21, 2025 •

edited

Loading

danehans Aug 22, 2025 •

edited

Loading