Conversation
Summary of ChangesHello @VanderChen, 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 dedicated router component to efficiently manage and direct incoming HTTP requests to the appropriate sandbox instances. It establishes a clear separation of concerns, offloading routing logic from other services and integrating with Redis for session state management. The changes include a new Go application for the router, comprehensive design documentation, and updates to build and deployment scripts to support this new service. This enhancement aims to improve the system's scalability, performance, and maintainability by centralizing request routing and session handling. 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 router component responsible for routing requests to sandboxes. The implementation includes a new binary, configuration, HTTP handlers, and a session manager for interacting with the workload manager and Redis. The changes also involve renaming the old agentcube-apiserver to agentcube-router in Makefiles and Kubernetes manifests. My review identifies several areas for improvement, focusing on performance, reliability, and maintainability. I've found a critical performance issue related to http.Transport creation, high-severity issues with configuration management and lack of timeouts in HTTP clients, and incorrect graceful shutdown logic. I have also provided suggestions for refactoring to reduce code duplication and improve test coverage.
VanderChen
force-pushed
the
frontend
branch
2 times, most recently
from
893f0bc to
6fc973a
Compare
|
ping @hzxuzhonghu |
VanderChen
force-pushed
the
frontend
branch
4 times, most recently
from
60be8ae to
2e97df2
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
This PR implements a router submodule that handles HTTP request routing and forwarding to sandbox instances. The router acts as an entry point for agent runtime and code interpreter invocations, managing sessions through a SessionManager interface and forwarding requests to appropriate sandbox endpoints.
Key changes:
- Implements SessionManager for sandbox session management and creation
- Adds router server with HTTP handlers for health checks and invocation forwarding
- Updates Kubernetes deployment configurations to support the router component
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/router/session_manager.go | Implements SessionManager interface for retrieving/creating sandbox sessions |
| pkg/router/session_manager_test.go | Comprehensive test coverage for session management functionality |
| pkg/router/server.go | Router server implementation with concurrency control and HTTP transport pooling |
| pkg/router/server_test.go | Tests for server initialization, configuration, and lifecycle |
| pkg/router/handlers.go | HTTP request handlers for health checks and request forwarding to sandboxes |
| pkg/router/handlers_test.go | Tests for HTTP handlers including health checks and invocation routing |
| pkg/router/config.go | Configuration structure for router server |
| cmd/router/main.go | Router server entry point with command-line flag handling |
| k8s/agentcube-apiserver.yaml | Updated Kubernetes deployment to reflect router component naming and RBAC |
| Dockerfile.router | Multi-stage Docker build configuration for router |
| Makefile | Updated build targets to include router binary |
| docs/proposal/router-design.md | Comprehensive design document for router architecture |
| pkg/workloadmanager/server.go | Import statement formatting adjustment |
| pkg/workloadmanager/handlers.go | Import statement reordering |
| pkg/workloadmanager/garbage_collection.go | Import statement reordering |
| go.mod | Moved k8s.io/utils from indirect to direct dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
|
@hzxuzhonghu @tjucoder all comments fixed. |
Signed-off-by: VanderChen <vanderchen@outlook.com>
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
docs/proposal/router-design.md
Outdated
|
|
||
| 1. **High-Performance Routing**: Fast routing to corresponding sandbox based on session-id | ||
| 2. **Session Integration**: Seamless collaboration with SessionManager, supporting dynamic sandbox creation | ||
| 3. **Long Connection Support**: Support for long-running requests such as code execution and file operations |
There was a problem hiding this comment.
Is it more appropriate to translate ‘Long Connection Support’ as ‘Persistent Connection Support’?
pkg/router/handlers.go
Outdated
| namespace := c.Param("namespace") | ||
| name := c.Param("name") | ||
| path := c.Param("path") | ||
| s.handleInvoke(c, namespace, name, path, "AgentRuntime") |
There was a problem hiding this comment.
It is preferable to define "AgentRuntime" and "CodeInterpreter" as type-representing constants.
| log.Printf("%s invoke request: namespace=%s, name=%s, path=%s", kind, namespace, name, path) | ||
|
|
||
| // Extract session ID from header | ||
| sessionID := c.GetHeader("x-agentcube-session-id") |
There was a problem hiding this comment.
Check whether the sessionID field is null.
|
|
||
| // Get sandbox info from session manager | ||
| sandbox, err := s.sessionManager.GetSandboxBySession(c.Request.Context(), sessionID, namespace, name, kind) | ||
| if err != nil { |
There was a problem hiding this comment.
Also check that the sandbox variable is returned as expected.
| // GetSandboxBySession returns the sandbox associated with the given sessionID. | ||
| // When sessionID is empty, it creates a new sandbox by calling the external API. | ||
| // When sessionID is not empty, it queries Redis for the sandbox. | ||
| func (m *manager) GetSandboxBySession(ctx context.Context, sessionID string, namespace string, name string, kind string) (*types.SandboxRedis, error) { |
There was a problem hiding this comment.
Is it more appropriate to modify the function name from GetSandboxBySession to GetorCreateSandboxBySessionID?
| default: | ||
| return nil, fmt.Errorf("unsupported kind: %s", kind) | ||
| } | ||
|
|
There was a problem hiding this comment.
var kindToEndpointPath = map[string]string{
types.AgentRuntimeKind: "/v1/agent-runtime",
types.CodeInterpreterKind: "/v1/code-interpreter",
}
path, ok := kindToEndpointPath[kind]
if !ok {
return "", fmt.Errorf("unsupported kind: %s", kind)
}
endpoint := m.workloadMgrURL + pathSuggest adjusting the code style to the above format to improve extensibility
docs/proposal/router-design.md
Outdated
|
|
||
| ## 1. Overview | ||
|
|
||
| Router apiserver is responsible for receiving user HTTP requests and forwarding them to the corresponding Sandbox. Router focuses on high-performance request routing, while session and sandbox management is handled by SessionManager. |
There was a problem hiding this comment.
Router APIServer is responsible for receiving user HTTP requests and forwarding them to the corresponding Sandbox. As core modules embedded within the Router APIServer, Router focuses on high-performance request routing, while session and sandbox management are handled by Sandbox Manager.
| // Extract endpoint from sandbox - find matching entry point by path | ||
| var endpoint string | ||
| for _, ep := range sandbox.EntryPoints { | ||
| if strings.HasPrefix(path, ep.Path) { |
There was a problem hiding this comment.
The current path resolution using a simple Prefix Match can lead to incorrect routing when less specific paths appear before more specific ones. We must switch to a Longest Prefix Match (LPM) approach to guarantee the correct and most specific endpoint is selected for request forwarding.
| // Only add protocol if not already present | ||
| if ep.Protocol != "" && !strings.Contains(ep.Endpoint, "://") { | ||
| endpoint = strings.ToLower(ep.Protocol) + "://" + ep.Endpoint | ||
| } else { |
There was a problem hiding this comment.
A potential logic error could arise when ep.protocol has an empty value.
No description provided.