Support modelroute and modelserver status#1292
Conversation
Signed-off-by: Leo Xie <stleox@qq.com>
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 |
![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 status tracking for ModelRoute and ModelServer resources by adding fields like observedGeneration, conditions, readyReplicas, and matchedReplicas to their CRDs, API definitions, and generated client-go apply configurations. The ModelRouteController and ModelServerController are updated to reconcile and write these statuses, backed by new unit tests. The review feedback highlights critical improvements to controller reliability: first, status updates should be aborted if the resource's generation changes during the API fetch to prevent writing stale status; second, the ModelServer status update should be deferred to the end of the sync handler so that non-critical API write failures do not block critical datastore and pod binding updates.
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.
| return retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| // Get latest version from API server to avoid conflicts | ||
| ctx := context.Background() | ||
| latest, err := c.kthenaClient.NetworkingV1alpha1().ModelRoutes(mr.Namespace).Get(ctx, mr.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| mrCopy := latest.DeepCopy() | ||
| mrCopy.Status.ObservedGeneration = latest.Generation |
There was a problem hiding this comment.
If the ModelRoute spec generation has changed since we fetched it, we should abort the status update. Otherwise, we might write a stale status or set an incorrect ObservedGeneration for the new spec. Since a new sync event is guaranteed to be queued for the updated spec, returning nil here is safe and prevents race conditions.
return retry.RetryOnConflict(retry.DefaultRetry, func() error {
// Get latest version from API server to avoid conflicts
ctx := context.Background()
latest, err := c.kthenaClient.NetworkingV1alpha1().ModelRoutes(mr.Namespace).Get(ctx, mr.Name, metav1.GetOptions{})
if err != nil {
return err
}
if latest.Generation != mr.Generation {
return nil
}
mrCopy := latest.DeepCopy()
mrCopy.Status.ObservedGeneration = latest.Generation| if err := c.store.AddOrUpdateModelServer(ms, pods); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := c.updateModelServerStatus(ms, podList, pods); err != nil { | ||
| klog.Errorf("Failed to update ModelServer status for %s/%s: %v", ms.Namespace, ms.Name, err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Updating the ModelServer status is a non-critical API server write, whereas updating the datastore and pod-to-server bindings (lines 227-267) is critical for the router's traffic routing functionality. If updateModelServerStatus fails here, the entire sync handler returns early, skipping the critical pod binding updates. We should perform the status update at the very end of syncModelServerHandler to ensure all datastore updates are completed first.
| if err := c.store.AddOrUpdateModelServer(ms, pods); err != nil { | |
| return err | |
| } | |
| if err := c.updateModelServerStatus(ms, podList, pods); err != nil { | |
| klog.Errorf("Failed to update ModelServer status for %s/%s: %v", ms.Namespace, ms.Name, err) | |
| return err | |
| } | |
| if err := c.store.AddOrUpdateModelServer(ms, pods); err != nil { | |
| return err | |
| } |
| @@ -253,6 +267,59 @@ func (c *ModelServerController) syncModelServerHandler(key string) error { | |||
| return nil | |||
There was a problem hiding this comment.
Perform the ModelServer status update at the end of the sync handler, after all datastore and pod binding updates have been successfully completed.
| return nil | |
| return c.updateModelServerStatus(ms, podList, pods) |
| return retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| // Get latest version from API server to avoid conflicts | ||
| ctx := context.Background() | ||
| latest, err := c.kthenaClient.NetworkingV1alpha1().ModelServers(ms.Namespace).Get(ctx, ms.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| msCopy := latest.DeepCopy() | ||
| msCopy.Status.ObservedGeneration = latest.Generation |
There was a problem hiding this comment.
If the ModelServer spec generation has changed since we fetched it, we should abort the status update. Otherwise, we might write stale status metrics (like MatchedReplicas and ReadyReplicas computed for the old spec) and mark them as observing the new generation. Since a new sync event is guaranteed to be queued for the updated spec, returning nil here is safe and prevents race conditions.
return retry.RetryOnConflict(retry.DefaultRetry, func() error {
// Get latest version from API server to avoid conflicts
ctx := context.Background()
latest, err := c.kthenaClient.NetworkingV1alpha1().ModelServers(ms.Namespace).Get(ctx, ms.Name, metav1.GetOptions{})
if err != nil {
return err
}
if latest.Generation != ms.Generation {
return nil
}
msCopy := latest.DeepCopy()
msCopy.Status.ObservedGeneration = latest.GenerationThere was a problem hiding this comment.
Pull request overview
This PR adds observable status fields to the ModelServer and ModelRoute CRDs and updates the kthena-router controllers to populate those status fields, enabling readiness/observability workflows like kubectl wait and status-driven automation.
Changes:
- Add
ObservedGenerationandConditionstoModelRouteStatus, andObservedGeneration/replica counts/ConditionstoModelServerStatus. - Update router controllers to write CR
statusvia the typed kthena client (with conflict retries) and to re-enqueue ModelServers on relevant Pod lifecycle events. - Regenerate CRDs, docs, deepcopy, and client-go applyconfiguration outputs to reflect the new status fields.
Reviewed changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/kthena-router/controller/modelserver_controller.go | Update ModelServer controller to compute pod readiness and write ModelServer status/conditions; enqueue ModelServers on pod events. |
| pkg/kthena-router/controller/modelserver_controller_test.go | Update controller constructor usage; add tests for pod→modelserver enqueue behavior and selector matching helper. |
| pkg/kthena-router/controller/modelroute_controller.go | Update ModelRoute controller to write Ready condition and observedGeneration via UpdateStatus with retry-on-conflict. |
| pkg/kthena-router/controller/modelroute_controller_test.go | Update constructor usage and add status update tests for ModelRoute. |
| pkg/apis/networking/v1alpha1/modelserver_types.go | Define new ModelServerStatus fields (observedGeneration/replica counts/conditions). |
| pkg/apis/networking/v1alpha1/modelroute_types.go | Define new ModelRouteStatus fields (observedGeneration/conditions). |
| pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go | Regenerate deepcopy logic for new status fields (conditions deep copy). |
| cmd/kthena-router/app/controller.go | Wire kthena client into router controllers. |
| docs/kthena/docs/reference/crd/networking.serving.volcano.sh.md | Regenerated CRD reference docs reflecting new status fields. |
| charts/kthena/charts/networking/crds/networking.serving.volcano.sh_modelservers.yaml | Regenerated ModelServer CRD schema to include status fields (conditions/replicas/observedGeneration). |
| charts/kthena/charts/networking/crds/networking.serving.volcano.sh_modelroutes.yaml | Regenerated ModelRoute CRD schema to include status fields (conditions/observedGeneration). |
| client-go/applyconfiguration/utils.go | Regenerated applyconfiguration kind mapping to include new *Status apply configs. |
| client-go/applyconfiguration/networking/v1alpha1/modelserverstatus.go | Generated applyconfiguration type for ModelServerStatus. |
| client-go/applyconfiguration/networking/v1alpha1/modelserver.go | Regenerated ModelServer applyconfiguration to use ModelServerStatusApplyConfiguration. |
| client-go/applyconfiguration/networking/v1alpha1/modelroutestatus.go | Generated applyconfiguration type for ModelRouteStatus. |
| client-go/applyconfiguration/networking/v1alpha1/modelroute.go | Regenerated ModelRoute applyconfiguration to use ModelRouteStatusApplyConfiguration. |
Files not reviewed (6)
- client-go/applyconfiguration/networking/v1alpha1/modelroute.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelroutestatus.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserver.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverstatus.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (1)
pkg/kthena-router/controller/modelserver_controller.go:85
- NewModelServerController now requires a kthenaClient parameter. There is existing controller documentation under pkg/kthena-router/controller/README.md that still shows the old constructor signature (missing kthenaClient), which will be confusing for contributors.
func NewModelServerController(
kthenaClient kthenaclient.Interface,
kthenaInformerFactory informersv1alpha1.SharedInformerFactory,
kubeInformerFactory informers.SharedInformerFactory,
store datastore.Store,
) *ModelServerController {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| msCopy := latest.DeepCopy() | ||
| msCopy.Status.ObservedGeneration = latest.Generation | ||
| msCopy.Status.MatchedReplicas = int32(len(podList)) | ||
| msCopy.Status.ReadyReplicas = int32(readyPods.Len()) | ||
|
|
||
| if readyPods.Len() > 0 { | ||
| meta.SetStatusCondition(&msCopy.Status.Conditions, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "PodsReady", | ||
| Message: fmt.Sprintf("%d ready pods matched", readyPods.Len()), | ||
| }) | ||
| } else { | ||
| meta.SetStatusCondition(&msCopy.Status.Conditions, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "NoReadyPods", | ||
| Message: "No ready pods match workloadSelector", | ||
| }) | ||
| } |
| mrCopy := latest.DeepCopy() | ||
| mrCopy.Status.ObservedGeneration = latest.Generation | ||
|
|
||
| meta.SetStatusCondition(&mrCopy.Status.Conditions, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "RouteRegistered", | ||
| Message: "ModelRoute registered in router store", | ||
| }) |
| // TestModelServerController_PodEnqueuesModelServer verifies that syncPodHandler | ||
| // enqueues matching ModelServers after pod state changes. | ||
| func TestModelServerController_PodEnqueuesModelServer(t *testing.T) { | ||
| kubeClient := kubefake.NewSimpleClientset() | ||
| kthenaClient := kthenafake.NewSimpleClientset() | ||
|
|
||
| ms := &aiv1alpha1.ModelServer{ | ||
| ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "test-ms-enq"}, |
Signed-off-by: Leo Xie <stleox@qq.com>
…or handling Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
StLeoX
force-pushed
the
feat/modelroute-status
branch
from
7125d78 to
e739430
Compare
| ReadyReplicas int32 `json:"readyReplicas,omitempty"` | ||
|
|
||
| // matchedReplicas is the total number of pods (ready or not) that match the | ||
| // workloadSelector. | ||
| // +optional | ||
| MatchedReplicas int32 `json:"matchedReplicas,omitempty"` |
There was a problem hiding this comment.
Seems not appropriate to represent pd disaggregated case
There was a problem hiding this comment.
Now I want to define two conditions in ModelServerStatus.Condition: 1. At least one pod is ready, 2. All pods are ready. That's enough for steady-state determination during testing. If we want to support PD disaggregated semantics, i can create another issue.
Signed-off-by: Leo Xie <stleox@qq.com>
Signed-off-by: Leo Xie <stleox@qq.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 16 changed files in this pull request and generated 2 comments.
Files not reviewed (6)
- client-go/applyconfiguration/networking/v1alpha1/modelroute.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelroutestatus.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserver.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverstatus.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
| if err := c.store.AddOrUpdateModelServer(ms, pods); err != nil { | ||
| return err | ||
| } |
| if errors.IsNotFound(err) { | ||
| _ = c.store.DeletePod(types.NamespacedName{Namespace: namespace, Name: name}) | ||
| // When a pod is deleted, enqueue all ModelServers in the namespace | ||
| // to refresh their status (we no longer have the pod to check labels). | ||
| c.enqueueModelServersInNamespace(namespace) | ||
| return nil | ||
| } |
# Conflicts: # cmd/kthena-router/app/controller.go # pkg/kthena-router/controller/modelroute_controller_test.go # pkg/kthena-router/controller/modelserver_controller_test.go
|
Adding label DetailsInstructions 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/test-infra repository. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 16 changed files in this pull request and generated 12 comments.
Files not reviewed (6)
- client-go/applyconfiguration/networking/v1alpha1/modelroute.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelroutestatus.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserver.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverstatus.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
| msCopy := latest.DeepCopy() | ||
| msCopy.Status.ObservedGeneration = latest.Generation | ||
| msCopy.Status.MatchedReplicas = matchedReplicas | ||
| msCopy.Status.ReadyReplicas = readyReplicas |
| meta.SetStatusCondition(&msCopy.Status.Conditions, metav1.Condition{ | ||
| Type: string(aiv1alpha1.ModelServerConditionReady), | ||
| Status: readyStatus, | ||
| Reason: readyReason, | ||
| Message: readyMessage, | ||
| }) | ||
|
|
||
| meta.SetStatusCondition(&msCopy.Status.Conditions, metav1.Condition{ | ||
| Type: string(aiv1alpha1.ModelServerConditionAllPodsReady), | ||
| Status: allPodsReadyStatus, | ||
| Reason: allPodsReadyReason, | ||
| Message: allPodsReadyMessage, | ||
| }) |
| mrCopy := latest.DeepCopy() | ||
| mrCopy.Status.ObservedGeneration = latest.Generation | ||
|
|
| meta.SetStatusCondition(&mrCopy.Status.Conditions, metav1.Condition{ | ||
| Type: string(aiv1alpha1.ModelRouteConditionReady), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "RouteRegistered", | ||
| Message: "ModelRoute registered in router store", | ||
| }) |
| if errors.IsNotFound(err) { | ||
| _ = c.store.DeletePod(types.NamespacedName{Namespace: namespace, Name: name}) | ||
| // When a pod is deleted, enqueue all ModelServers in the namespace | ||
| // to refresh their status (we no longer have the pod to check labels). | ||
| c.enqueueModelServersInNamespace(namespace) | ||
| return nil |
| kubeInformerFactory := informers.NewSharedInformerFactory(kubeClient, 0) | ||
| kthenaInformerFactory := informersv1alpha1.NewSharedInformerFactory(kthenaClient, 0) | ||
| store := newStoreWithMockBackend() | ||
| controller := NewModelServerController(kthenaClient, kthenaInformerFactory, kubeInformerFactory, store) |
| kubeInformerFactory := informers.NewSharedInformerFactory(kubeClient, 0) | ||
| kthenaInformerFactory := informersv1alpha1.NewSharedInformerFactory(kthenaClient, 0) | ||
| store := newStoreWithMockBackend() | ||
| controller := NewModelServerController(kthenaClient, kthenaInformerFactory, kubeInformerFactory, store) |
| kthenaInformerFactory := informersv1alpha1.NewSharedInformerFactory(kthenaClient, 0) | ||
| store := datastore.New() | ||
|
|
||
| controller := NewModelRouteController(kthenaClient, kthenaInformerFactory, store) |
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `observedGeneration` _integer_ | observedGeneration is the most recent generation observed for this ModelRoute. | | | |
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `observedGeneration` _integer_ | observedGeneration is the most recent generation observed for this ModelServer. | | | | ||
| | `readyReplicas` _integer_ | readyReplicas is the number of ready pods that are currently matched by the<br />workloadSelector and have been successfully registered in the router's store. | | | | ||
| | `matchedReplicas` _integer_ | matchedReplicas is the total number of pods (ready or not) that match the<br />workloadSelector. | | | |
Signed-off-by: Leo Xie <stleox@qq.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 16 changed files in this pull request and generated 4 comments.
Files not reviewed (6)
- client-go/applyconfiguration/networking/v1alpha1/modelroute.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelroutestatus.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserver.go: Generated file
- client-go/applyconfiguration/networking/v1alpha1/modelserverstatus.go: Generated file
- client-go/applyconfiguration/utils.go: Generated file
- pkg/apis/networking/v1alpha1/zz_generated.deepcopy.go: Generated file
| meta.SetStatusCondition(&mrCopy.Status.Conditions, metav1.Condition{ | ||
| Type: string(aiv1alpha1.ModelRouteConditionReady), | ||
| Status: metav1.ConditionTrue, | ||
| Reason: aiv1alpha1.ReasonRouteRegistered, | ||
| Message: "ModelRoute registered in router store", | ||
| }) |
| if err := c.addOrUpdatePod(pod); err != nil { | ||
| return err | ||
| } | ||
| // Pod became ready: enqueue matching ModelServers to refresh status | ||
| c.enqueueMatchingModelServers(pod) | ||
| return nil |
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `observedGeneration` _integer_ | observedGeneration is the most recent generation observed for this ModelRoute. | | | |
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `observedGeneration` _integer_ | observedGeneration is the most recent generation observed for this ModelServer. | | | | ||
| | `readyReplicas` _integer_ | readyReplicas is the number of ready pods that are currently matched by the<br />workloadSelector and have been successfully registered in the router's store. | | | | ||
| | `matchedReplicas` _integer_ | matchedReplicas is the total number of pods (ready or not) that match the<br />workloadSelector. | | | |
Summary
close #1257
Result
I manually tested
For reviewers: charts/, client-go/ are auto generated, no need to review.