feat: added status fields for API #650
Conversation
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Summary of ChangesHello @yashisrani, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces crucial functionality for reporting the operational status of Gateway API resources (Gateways and HTTPRoutes) and custom InferencePool resources within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 updates for Gateway API resources (Gateway, HTTPRoute, and InferencePool). The changes correctly pass the required clients to the controllers and add the logic to update the status subresource after processing an object. The status update logic for Gateway and HTTPRoute looks good. However, I've found several issues in the InferencePool status update implementation, which appears to be incomplete and contains dead code. I've also made minor suggestions to replace context.TODO() with context.Background() for better code clarity.
| func (c *InferencePoolController) updateInferencePoolStatus(inferencePool *inferencev1.InferencePool) error { | ||
| inferencePool = inferencePool.DeepCopy() | ||
|
|
||
| // In version 1.2.0, InferencePool status is per-parent. | ||
| // For now, we'll maintain a generic parent status if it's referenced by any HTTPRoute. | ||
| // This is a simplified implementation. | ||
|
|
||
| acceptedCond := metav1.Condition{ | ||
| Type: "Accepted", | ||
| Status: metav1.ConditionTrue, | ||
| Reason: "Accepted", | ||
| Message: "InferencePool has been accepted by kthena-router", | ||
| LastTransitionTime: metav1.Now(), | ||
| ObservedGeneration: inferencePool.Generation, | ||
| } | ||
|
|
||
| // We don't easily have the parent Gateway here, so we skip adding parents for now | ||
| // but we could implement a logic to find them from the store. | ||
| // To satisfy the lint and provide some status, we'll just ensure the object is updated. | ||
|
|
||
| // If the user wants specific fields, we can add a dummy parent or find real ones. | ||
| // For now, let's just update the object to trigger any observers. | ||
|
|
||
| _ = acceptedCond | ||
|
|
||
| // Convert back to unstructured to update status | ||
| content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(inferencePool) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert InferencePool to unstructured: %w", err) | ||
| } | ||
|
|
||
| unstructuredObj := &unstructured.Unstructured{Object: content} | ||
| gvr := inferencev1.SchemeGroupVersion.WithResource("inferencepools") | ||
| _, err = c.dynamicClient.Resource(gvr).Namespace(inferencePool.Namespace).UpdateStatus(context.TODO(), unstructuredObj, metav1.UpdateOptions{}) | ||
| return err | ||
| } | ||
|
|
||
| func (c *InferencePoolController) setInferencePoolCondition(inferencePool *inferencev1.InferencePool, parentRef inferencev1.ParentReference, newCond metav1.Condition) { | ||
| // Find or create parent status | ||
| var parentStatus *inferencev1.ParentStatus | ||
| for i := range inferencePool.Status.Parents { | ||
| if inferencePool.Status.Parents[i].ParentRef.Name == parentRef.Name { | ||
| parentStatus = &inferencePool.Status.Parents[i] | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if parentStatus == nil { | ||
| inferencePool.Status.Parents = append(inferencePool.Status.Parents, inferencev1.ParentStatus{ | ||
| ParentRef: parentRef, | ||
| ControllerName: inferencev1.ControllerName(ControllerName), | ||
| }) | ||
| parentStatus = &inferencePool.Status.Parents[len(inferencePool.Status.Parents)-1] | ||
| } | ||
|
|
||
| // Update conditions in parent status | ||
| for i, cond := range parentStatus.Conditions { | ||
| if cond.Type == newCond.Type { | ||
| if cond.Status == newCond.Status && cond.Reason == newCond.Reason { | ||
| newCond.LastTransitionTime = cond.LastTransitionTime | ||
| } | ||
| parentStatus.Conditions[i] = newCond | ||
| return | ||
| } | ||
| } | ||
| parentStatus.Conditions = append(parentStatus.Conditions, newCond) | ||
| } |
There was a problem hiding this comment.
The implementation for updating InferencePool status has several issues:
- The
updateInferencePoolStatusfunction is incomplete. It definesacceptedCondbut doesn't use it, resulting in a call toUpdateStatuswith an unmodified object. This is inefficient and confusing. - The
setInferencePoolConditionfunction is never called, making it dead code. - The parent reference comparison in
setInferencePoolCondition(line 209) is incorrect. It only compares theName, but should also compareSectionNameandPortto uniquely identify the parent, similar toisSameParentRefinhttproute_controller.go.
As the comments note, finding the parent Gateway is the main challenge. Given these issues, I recommend either fully implementing the status update logic (including finding parents and fixing the comparison) or removing this partial implementation for now to avoid confusion. If you choose to remove it, a // TODO comment would be appropriate.
| } | ||
| } | ||
|
|
||
| _, err := c.gatewayClient.GatewayV1().Gateways(gateway.Namespace).UpdateStatus(context.TODO(), gateway, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged as it's a placeholder for when the correct context is unknown. For background tasks like this that are not tied to a specific incoming request, context.Background() is a more appropriate choice. While a larger refactoring to pass a cancellable context down from the Run method would be ideal, switching to context.Background() is a good immediate improvement.
| _, err := c.gatewayClient.GatewayV1().Gateways(gateway.Namespace).UpdateStatus(context.TODO(), gateway, metav1.UpdateOptions{}) | |
| _, err := c.gatewayClient.GatewayV1().Gateways(gateway.Namespace).UpdateStatus(context.Background(), gateway, metav1.UpdateOptions{}) |
| c.setHTTPRouteParentStatus(httpRoute, parentStatus) | ||
| } | ||
|
|
||
| _, err := c.gatewayClient.GatewayV1().HTTPRoutes(httpRoute.Namespace).UpdateStatus(context.TODO(), httpRoute, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged as it's a placeholder for when the correct context is unknown. For background tasks like this that are not tied to a specific incoming request, context.Background() is a more appropriate choice. While a larger refactoring to pass a cancellable context down from the Run method would be ideal, switching to context.Background() is a good immediate improvement.
| _, err := c.gatewayClient.GatewayV1().HTTPRoutes(httpRoute.Namespace).UpdateStatus(context.TODO(), httpRoute, metav1.UpdateOptions{}) | |
| _, err := c.gatewayClient.GatewayV1().HTTPRoutes(httpRoute.Namespace).UpdateStatus(context.Background(), httpRoute, metav1.UpdateOptions{}) |
|
|
||
| unstructuredObj := &unstructured.Unstructured{Object: content} | ||
| gvr := inferencev1.SchemeGroupVersion.WithResource("inferencepools") | ||
| _, err = c.dynamicClient.Resource(gvr).Namespace(inferencePool.Namespace).UpdateStatus(context.TODO(), unstructuredObj, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Using context.TODO() is generally discouraged as it's a placeholder for when the correct context is unknown. For background tasks like this that are not tied to a specific incoming request, context.Background() is a more appropriate choice. While a larger refactoring to pass a cancellable context down from the Run method would be ideal, switching to context.Background() is a good immediate improvement.
| _, err = c.dynamicClient.Resource(gvr).Namespace(inferencePool.Namespace).UpdateStatus(context.TODO(), unstructuredObj, metav1.UpdateOptions{}) | |
| _, err = c.dynamicClient.Resource(gvr).Namespace(inferencePool.Namespace).UpdateStatus(context.Background(), unstructuredObj, metav1.UpdateOptions{}) |
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
| c.setGatewayCondition(gateway, programmedCond) | ||
|
|
||
| // Update listener status | ||
| gateway.Status.Listeners = make([]gatewayv1.ListenerStatus, len(gateway.Spec.Listeners)) |
There was a problem hiding this comment.
Why not write a setGatewayListenerConditions function?
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
I have read gateway status setting, some abnormal case like: no error is reported even we cannot startup a specific listener.
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
| portInfo.ShutdownFunc = cancel | ||
|
|
||
| // Start the server | ||
| lm.store.SetListenerStatus(config.GatewayKey, config.ListenerName, nil) |
There was a problem hiding this comment.
explain why set nil(// Clear listener status in store)
cmd/kthena-router/app/router.go
Outdated
| // Update all listeners on this port with the error | ||
| lm.mu.RLock() | ||
| if pi, ok := lm.portListeners[p]; ok { | ||
| pi.mu.Lock() | ||
| pi.LastError = err | ||
| for _, l := range pi.Listeners { | ||
| lm.store.SetListenerStatus(l.GatewayKey, l.ListenerName, err) | ||
| } | ||
| pi.mu.Unlock() | ||
| } | ||
| lm.mu.RUnlock() |
There was a problem hiding this comment.
we can only have one listener here, because L418. BTW, the lock in lock looks super risky.
portInfo, exists := lm.portListeners[port]
if !exists {
|
|
||
| // Listener status tracking | ||
| listenerStatusMutex sync.RWMutex | ||
| listenerStatuses map[string]map[string]error // gatewayKey -> listenerName -> error |
There was a problem hiding this comment.
Seems you donot actually cleanup when gw or listener deleted.
| return c.updateGatewayStatus(gateway) | ||
| } | ||
|
|
||
| func (c *GatewayController) updateGatewayStatus(gateway *gatewayv1.Gateway) error { |
There was a problem hiding this comment.
Please add corresponding E2E to make sure the status setting for Gateway/HTTPRoute/InferencePool all work properly.
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
|
[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 |
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
5 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #647
Special notes for your reviewer:
Does this PR introduce a user-facing change?: