feat: Add ExternalProvider support to ModelServerSpec#1191
feat: Add ExternalProvider support to ModelServerSpec#1191shivansh-gohem wants to merge 1 commit into
Conversation
volcano-sh-bot
requested review from
LiZhenCheng9527,
YaoZengzeng,
git-malu and
hzxuzhonghu
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for routing ModelServer traffic to external cloud LLM providers by extending the CRD API and generated artifacts.
Changes:
- Make
workloadSelectoroptional inModelServerSpec. - Introduce
externalProviderconfiguration (provider type, endpoint, credentials secret ref). - Update generated CRD YAML (Helm chart) and reference docs to reflect the new API.
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/apis/networking/v1alpha1/modelserver_types.go | Adds ExternalProvider API types and makes WorkloadSelector optional. |
| docs/kthena/docs/reference/crd/networking.serving.volcano.sh.md | Documents ExternalProvider and updates ModelServerSpec field requirements. |
| charts/kthena/charts/networking/crds/networking.serving.volcano.sh_modelservers.yaml | Updates CRD schema to include externalProvider and removes workloadSelector from required. |
Files not reviewed (4)
- client-go/applyconfiguration/networking/v1alpha1/externalprovider.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverspec.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -47,6 +48,11 @@ type ModelServerSpec struct { | |||
| // KVConnector specifies the KV connector configuration for PD disaggregated routing | |||
| // +optional | |||
| KVConnector *KVConnectorSpec `json:"kvConnector,omitempty"` | |||
|
|
|||
| // ExternalProvider specifies an external cloud LLM provider to route requests to. | |||
| // When this is set, WorkloadSelector is ignored and requests are proxied to the external endpoint. | |||
| // +optional | |||
| ExternalProvider *ExternalProvider `json:"externalProvider,omitempty"` | |||
| // The full base URL for the external provider endpoint (e.g., https://api.openai.com/v1). | ||
| // +kubebuilder:validation:Required | ||
| Endpoint string `json:"endpoint"` |
| // Reference to a Kubernetes Secret containing the API credentials/keys for the external provider. | ||
| // +optional | ||
| CredentialsRef *corev1.LocalObjectReference `json:"credentialsRef,omitempty"` |
| required: | ||
| - inferenceEngine | ||
| - workloadSelector | ||
| type: object |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for external cloud LLM providers (such as OpenAI, Anthropic, and AzureOpenAI) in the ModelServer CRD by adding an ExternalProvider field to ModelServerSpec and making WorkloadSelector optional. Feedback on these changes suggests adding CEL validation to ensure either WorkloadSelector or ExternalProvider is specified, making InferenceEngine optional when an external provider is used, and documenting the expected key names within the referenced Kubernetes Secret.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // +optional | ||
| WorkloadSelector *WorkloadSelector `json:"workloadSelector,omitempty"` |
There was a problem hiding this comment.
Since WorkloadSelector is now optional (+optional and omitempty), it is possible to configure a ModelServer where neither WorkloadSelector nor ExternalProvider is specified. This would result in an invalid configuration with no routing destination.
To prevent this, we should enforce that at least one of these fields is set. Since the project uses Kubernetes 1.25+ compatible CRD generation, we can use CEL (Common Expression Language) validation on the ModelServerSpec struct to enforce this constraint at the API level.
For example, you can add the following kubebuilder marker to the ModelServerSpec struct:
// +kubebuilder:validation:XValidation:rule="has(self.workloadSelector) || has(self.externalProvider)",message="either workloadSelector or externalProvider must be specified"| // ExternalProvider specifies an external cloud LLM provider to route requests to. | ||
| // When this is set, WorkloadSelector is ignored and requests are proxied to the external endpoint. | ||
| // +optional | ||
| ExternalProvider *ExternalProvider `json:"externalProvider,omitempty"` |
There was a problem hiding this comment.
Currently, InferenceEngine is a required field in ModelServerSpec (with enum values vLLM and SGLang). However, when ExternalProvider is configured, requests are proxied to an external cloud LLM provider (like OpenAI or Anthropic). In this scenario, specifying a local inference engine like vLLM or SGLang is logically inconsistent and unnecessary.
To improve the API usability, InferenceEngine should be made optional when ExternalProvider is used. You can achieve this by changing InferenceEngine to be optional in the Go struct:
// The inference engine used to serve the model.
// +optional
InferenceEngine InferenceEngine `json:"inferenceEngine,omitempty"`(Note: Since InferenceEngine is outside the current diff hunk, you will need to update its definition manually in modelserver_types.go and regenerate the CRDs).
| // Reference to a Kubernetes Secret containing the API credentials/keys for the external provider. | ||
| // +optional | ||
| CredentialsRef *corev1.LocalObjectReference `json:"credentialsRef,omitempty"` |
There was a problem hiding this comment.
It would be highly beneficial to document the expected key name(s) inside the referenced Kubernetes Secret. For example, does the proxy/adapter expect a specific key like api-key, token, or does it vary by provider? Clarifying this in the field description will greatly improve the developer experience when configuring external providers.
// Reference to a Kubernetes Secret containing the API credentials/keys for the external provider.
// The Secret should contain the token/key under the expected key name (e.g., 'api-key').
// +optional
CredentialsRef *corev1.LocalObjectReference `json:"credentialsRef,omitempty"`
shivansh-gohem
force-pushed
the
feat/external-provider-crd
branch
from
50ad010 to
d183dd3
Compare
shivansh-gohem
force-pushed
the
feat/external-provider-crd
branch
from
d183dd3 to
05073b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 9 changed files in this pull request and generated 7 comments.
Files not reviewed (4)
- client-go/applyconfiguration/networking/v1alpha1/externalprovider.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverspec.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
| workload.serving.volcano.sh/model-name: test-model | ||
| workload.serving.volcano.sh/model-uid: randomUID | ||
| workload.serving.volcano.sh/revision: 587ff8c655 | ||
| workload.serving.volcano.sh/revision: 695b798d9 |
|
|
||
| // ModelServerSpec defines the desired state of ModelServer. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="has(self.workloadSelector) || has(self.externalProvider)",message="either workloadSelector or externalProvider must be specified" |
| // +optional | ||
| InferenceEngine InferenceEngine `json:"inferenceEngine,omitempty"` |
| // +optional | ||
| WorkloadSelector *WorkloadSelector `json:"workloadSelector,omitempty"` |
| // ExternalProvider specifies an external cloud LLM provider to route requests to. | ||
| // When this is set, WorkloadSelector is ignored and requests are proxied to the external endpoint. | ||
| // +optional | ||
| ExternalProvider *ExternalProvider `json:"externalProvider,omitempty"` |
| x-kubernetes-validations: | ||
| - message: either workloadSelector or externalProvider must be specified | ||
| rule: has(self.workloadSelector) || has(self.externalProvider) |
| // The full base URL for the external provider endpoint (e.g., https://api.openai.com/v1). | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Format=uri | ||
| Endpoint string `json:"endpoint"` |
Signed-off-by: Shivansh Sahu <sahushivansh142@gmail.com>
shivansh-gohem
force-pushed
the
feat/external-provider-crd
branch
from
05073b2 to
3064ab0
Compare
|
I've drafted a comprehensive design doc as requested! To keep the discussion focused before pushing it to the public repo, I've placed the draft in a private repository here: I'll invite you both as collaborators to the repository so you can review the design details. Let me know what you think of the adapter layer approach and the mutual exclusivity of |
|
/assign @YaoZengzeng |
This PR introduces the
ExternalProviderstruct to theModelServerSpecAPI as discussed in #1024.Changes:
ExternalProviderstruct andExternalProviderTypeenum.WorkloadSelectoroptional (omitempty).ExternalProviderfield toModelServerSpec.This is Part 1 of the implementation for routing to external cloud LLM providers. Part 2 will cover the proxying and adapter logic.
Fixes #1024