[pluggable parser] add AppProtocol validation in the InferencePool Reconcile loop#2761
[pluggable parser] add AppProtocol validation in the InferencePool Reconcile loop#2761zetxqx wants to merge 4 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
kind/feature
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zetxqx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
| return ctrl.Result{}, fmt.Errorf("unsupported API group: %s", c.PoolGKNN.Group) | ||
| } | ||
|
|
||
| if c.Validator != nil { |
There was a problem hiding this comment.
I think we should do this in the epp health check: https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/cmd/epp/runner/health.go#L47; return not healthy if the configured parser doesn't support the appProtocol.
There was a problem hiding this comment.
great point, moved the check to health.go
| return p.typedName | ||
| } | ||
|
|
||
| func (p *VllmGRPCParser) ValidateAppProtocol(appProtocol v1.AppProtocol) error { |
There was a problem hiding this comment.
Instead of placing the burden of validation on the plugin, the plugin should just return the list of protocols it supports and then the caller validates if that matches the value declared on the infPool.
We should have a special match all value, perhaps an empty list, so that plugins like the passthrough one can return to signify that all protocols are supported.
There was a problem hiding this comment.
great suggestion, updated.
for the match all value, currently I made the AppProtocolSupporter a separate interface from the parser interface. So that parser can select not implementing AppProtocolSupporter, not implementing is also considered to be match all
|
@zetxqx can w please finalize this? |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add
AppProtocolValidatorinterface for parsers to implement optionally.The validation is done in the
health.goloop. If a mismatch is detected, the health server will return non_servingI didn't put the validation in startup because inferencePool may installed after the EPP start. Adding to the reconcile loop will prevent some early failure when inferencePool is installed later.
Which issue(s) this PR fixes:
Fixes #2750
Does this PR introduce a user-facing change?: