Fix issue 64 #71
Fix issue 64 #71
Conversation
Signed-off-by: VanderChen <vanderchen@outlook.com>
Signed-off-by: LeslieKuo <676365950@qq.com>
Signed-off-by: VanderChen <vanderchen@outlook.com>
Signed-off-by: VanderChen <vanderchen@outlook.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 |
Summary of ChangesHello @LeslieKuo, 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 a new Highlights
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 a new agentcube-router component and adds support for ReadinessProbe when creating sandboxes. The changes are extensive, including a new Dockerfile, Makefile targets, Kubernetes manifests, and the core router implementation with handlers, server logic, and a session manager. Overall, the implementation is solid and well-tested. I've identified a few areas for improvement, including a race condition in the graceful shutdown logic, a redundant Redis call, and some minor inconsistencies in the Makefile and documentation. Addressing these points will enhance the robustness and maintainability of the new router component.
| select { | ||
| case <-ctx.Done(): | ||
| log.Println("Received shutdown signal, shutting down gracefully...") | ||
| // Cancel the context to trigger server shutdown | ||
| cancel() | ||
| // Wait for server goroutine to exit after graceful shutdown is complete | ||
| <-errCh | ||
| case err := <-errCh: | ||
| log.Fatalf("Server error: %v", err) | ||
| } |
There was a problem hiding this comment.
There's a race condition in the graceful shutdown logic. If the server fails to start (e.g., port already in use) and a shutdown signal is received at the same time, the select statement has non-deterministic behavior. If the <-ctx.Done() case is selected, the startup error from errCh will be consumed but ignored, causing the program to exit silently without reporting the fatal startup error. This can make debugging startup issues very difficult.
To fix this, you should ensure that any error from errCh is handled correctly, regardless of which select case is chosen. Here is a suggested change to at least log the error in the shutdown path, which mitigates the issue of silently ignoring it.
| select { | |
| case <-ctx.Done(): | |
| log.Println("Received shutdown signal, shutting down gracefully...") | |
| // Cancel the context to trigger server shutdown | |
| cancel() | |
| // Wait for server goroutine to exit after graceful shutdown is complete | |
| <-errCh | |
| case err := <-errCh: | |
| log.Fatalf("Server error: %v", err) | |
| } | |
| select { | |
| case <-ctx.Done(): | |
| log.Println("Received shutdown signal, shutting down gracefully...") | |
| // The server's own context handling will trigger shutdown. | |
| // We wait for it to complete and check for any unexpected errors. | |
| // A `http.ErrServerClosed` is expected on graceful shutdown, so we ignore it. | |
| if err := <-errCh; err != nil && err.Error() != "http: Server closed" { | |
| log.Printf("Server shutdown with error: %v", err) | |
| } | |
| case err := <-errCh: | |
| log.Fatalf("Server error: %v", err) | |
| } |
| @echo "Cleaning..." | ||
| rm -rf bin/ | ||
| rm -f agentcube-apiserver agentd | ||
| rm -f agentcube-router agentd |
There was a problem hiding this comment.
The clean target was modified to remove agentcube-router but it no longer removes agentcube-apiserver. Since the build target for agentcube-apiserver still exists, it should probably be included in the clean target to avoid leaving stale artifacts.
rm -f agentcube-apiserver agentcube-router agentd
| - **Liveness Check**: `/health/live` | ||
| - **Readiness Check**: `/health/ready` |
There was a problem hiding this comment.
The "Health Checks" section lists /health/live and /health/ready endpoints. However, the implementation in pkg/router/server.go also exposes a generic /health endpoint, which is used by the Kubernetes readiness and liveness probes in k8s/agentcube-router.yaml. For consistency and completeness, this endpoint should also be documented here.
| - **Liveness Check**: `/health/live` | |
| - **Readiness Check**: `/health/ready` | |
| - **Health Check**: `/health` | |
| - **Liveness Check**: `/health/live` | |
| - **Readiness Check**: `/health/ready` |
| if err := s.redisClient.UpdateSessionLastActivity(c.Request.Context(), sandbox.SessionID, time.Now()); err != nil { | ||
| log.Printf("Failed to update sandbox last activity for request: %v", err) | ||
| } |
There was a problem hiding this comment.
The call to s.redisClient.UpdateSessionLastActivity is duplicated. It's called once before s.forwardToSandbox (lines 98-102) and again immediately after this. Since forwardToSandbox blocks until the request is served, the second call is redundant and causes an unnecessary write to Redis. You should remove this second call.
Additionally, the log message on line 100 "Failed to update sandbox last activity" is slightly misleading, as the function UpdateSessionLastActivity updates the activity for a session, not a sandbox.
| if len(sandbox.Spec.PodTemplate.Spec.Containers) > 0 { | ||
| container := &sandbox.Spec.PodTemplate.Spec.Containers[0] |
There was a problem hiding this comment.
The logic for adding a readiness probe applies it to the first container of the pod spec (.Spec.PodTemplate.Spec.Containers[0]). This assumes that the pod has only one container or that the probe is always intended for the first one. For AgentRuntime workloads, which can have multiple containers in their pod template, this might lead to incorrect behavior if the probe is intended for a different container.
Consider making this more robust, for example by allowing the ReadinessProbe configuration to specify a container name, and falling back to the first container if no name is provided.
|
Issue #64 is just a quick draft idea for the readiness probe. You’d better add a fully designed proposal for it in this PR. |
| sandbox.Spec.PodTemplate.ObjectMeta.Labels[SessionIdLabelKey] = params.sessionID | ||
| sandbox.Spec.PodTemplate.ObjectMeta.Labels["sandbox-name"] = params.sandboxName | ||
|
|
||
| // Handle Readiness Probe |
There was a problem hiding this comment.
Is the readiness probe only being added to container[0]? If this follows the design logic and is a deliberate design limit, please add code comments to mark this point clearly.
|
-> #89 |
5 participants
add ReadinessProbe to fix issue #64