-
Notifications
You must be signed in to change notification settings - Fork 176
API: Adds default status condition to InferencePool #830
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
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
/cc @robscott Tested in my dev cluster: $ kubectl get inferencepool/test -o yaml
...
status:
parent:
- conditions:
- lastTransitionTime: "1970-01-01T00:00:00Z"
message: Waiting for controller
reason: Pending
status: Unknown
type: Accepted
parentRef: {} |
/approve Will leave final stamp to @robscott |
Since the default parent status contains an empty $ kubectl get inferencepool/vllm-llama3-8b-instruct -o yaml
...
status:
parent:
- conditions:
- lastTransitionTime: "1970-01-01T00:00:00Z"
message: Waiting for controller
reason: Pending
status: Unknown
type: Accepted
parentRef: {}
- conditions:
- lastTransitionTime: "2025-05-15T00:04:05Z"
message: InferencePool is accepted by controller kgateway.dev/kgateway
reason: Accepted
status: "True"
type: Accepted
- lastTransitionTime: "2025-05-15T00:04:05Z"
message: All InferencePool references are resolved
reason: ResolvedRefs
status: "True"
type: ResolvedRefs
parentRef:
kind: Gateway
name: inference-gateway We should update the InferencePool API to define the expected behavior of the default PoolStatus. For example:
Thoughts @robscott |
Thanks for doing this @danehans! We should update the InferencePool API to define the expected behavior of the default PoolStatus. For example:
Completely agree. I think 1) is a MUST, while 2) is a SHOULD. If you can add this to API Spec, I'll LGTM. |
api/v1alpha2/inferencepool_types.go
Outdated
|
||
// Status defines the observed state of InferencePool. | ||
// | ||
// +kubebuilder:default={parent: {{parentRef: {}, conditions: {{type: "Accepted", status: "Unknown", reason: "Pending", message: "Waiting for controller", lastTransitionTime: "1970-01-01T00:00:00Z"}}}}} |
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.
What if instead of an empty parentRef, we filled it with something like {kind: Status, name: default}
, or some kind of similar distinction so it's easier for controllers to know that they can safely remove this.
cc @SinaChavoshi related to conformance tests |
/test pull-gateway-api-inference-extension-test-e2e-main |
Bump on this PR, the conversation makes sense to me. I think implementation of that will put this PR in a good state. |
Signed-off-by: Daneyon Hansen <[email protected]>
Signed-off-by: Daneyon Hansen <[email protected]>
@robscott PTAL when you have a moment. |
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.
Thanks @danehans!
/lgtm
|
||
// Status defines the observed state of InferencePool. | ||
// | ||
// +kubebuilder:default={parent: {{parentRef: {kind: "Status", name: "default"}, conditions: {{type: "Accepted", status: "Unknown", reason: "Pending", message: "Waiting for controller", lastTransitionTime: "1970-01-01T00:00:00Z"}}}}} |
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.
@zetxqx I think we'll likely need some conformance test coverage here to ensure that controllers drop this condition from status. It's possible that our existing conformance tests will already cover this though.
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.
ack!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, kfswain, robscott 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:
Approvers can indicate their approval by writing |
) * API: Adds default status condition to InferencePool Signed-off-by: Daneyon Hansen <[email protected]> * Adds default parent to pool status Signed-off-by: Daneyon Hansen <[email protected]> --------- Signed-off-by: Daneyon Hansen <[email protected]>
) * API: Adds default status condition to InferencePool Signed-off-by: Daneyon Hansen <[email protected]> * Adds default parent to pool status Signed-off-by: Daneyon Hansen <[email protected]> --------- Signed-off-by: Daneyon Hansen <[email protected]>
) * API: Adds default status condition to InferencePool Signed-off-by: Daneyon Hansen <[email protected]> * Adds default parent to pool status Signed-off-by: Daneyon Hansen <[email protected]> --------- Signed-off-by: Daneyon Hansen <[email protected]>
Adds default status condition to the InferencePool API.
Fixes #825