improve sandbox access resilience through ServiceFQDN#245
improve sandbox access resilience through ServiceFQDN#245FAUST-BENCHOU wants to merge 2 commits intovolcano-sh:mainfrom
Conversation
Signed-off-by: zhoujinyu <2319109590@qq.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, 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 significantly improves the resilience of sandbox access by prioritizing the use of Kubernetes ServiceFQDN for connecting to sandboxes. This change ensures that ongoing sessions can gracefully recover from underlying pod evictions or recreations, as the ServiceFQDN remains stable while pod IPs might change. A fallback to direct pod IP is maintained for compatibility, and comprehensive testing has been added to validate this enhanced stability. 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. 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 improves the resilience of sandbox access by preferring the Kubernetes Service FQDN over the pod IP. This is a solid improvement that makes the system more robust against pod restarts and evictions. The fallback to using the pod IP ensures backward compatibility. The changes are well-implemented and are accompanied by thorough unit tests and a new end-to-end test that effectively validates the resilience feature. I have one suggestion regarding a duplicated constant to improve maintainability.
| // sessionIDLabelKey is the label key for session ID on Sandbox (matches workloadmanager.SessionIdLabelKey) | ||
| sessionIDLabelKey = "runtime.agentcube.io/session-id" |
There was a problem hiding this comment.
To improve maintainability and avoid potential drift, it's better to use a single source of truth for this label key. You've correctly noted in the comment that this matches workloadmanager.SessionIdLabelKey.
Instead of duplicating the constant, consider moving workloadmanager.SessionIdLabelKey to a shared package (e.g., pkg/common/types) and using it here and in the workloadmanager package. This ensures consistency across the codebase.
For example, you could add it to pkg/common/types/types.go:
const (
// ... other constants
SessionIDLabelKey = "runtime.agentcube.io/session-id"
)
Then, you can import github.com/volcano-sh/agentcube/pkg/common/types and use types.SessionIDLabelKey here, removing the local constant definition.
|
@YaoZengzeng @hzxuzhonghu I'm not really sure if the introduction of ServiceFQDN affected the design intent of agentcube. |
There was a problem hiding this comment.
Pull request overview
This PR improves sandbox access resilience in the Workload Manager by preferring a stable ServiceFQDN (instead of a one-time pod IP lookup) when building sandbox entrypoints, and adds/updates tests to validate the behavior—including an e2e scenario that deletes the sandbox pod and re-invokes with the same session.
Changes:
- Prefer
Sandbox.Status.ServiceFQDNas the sandbox endpoint host, with a fallback to pod IP lookup whenServiceFQDNis empty. - Update unit tests around sandbox info/handler behavior to cover
ServiceFQDNand fallback behavior. - Add an e2e test that deletes the sandbox pod and verifies the same session still works after recreation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/workloadmanager/handlers.go |
Prefer ServiceFQDN for sandbox access; fallback to pod IP only when needed. |
pkg/workloadmanager/sandbox_helper.go |
Build entrypoint endpoints from a generalized host (FQDN or IP). |
pkg/workloadmanager/sandbox_helper_test.go |
Extend sandbox info tests to validate ServiceFQDN endpoint formatting. |
pkg/workloadmanager/handlers_test.go |
Add coverage for ServiceFQDN vs pod-IP fallback behavior in createSandbox. |
test/e2e/e2e_test.go |
Add e2e test verifying sandbox access after pod deletion/recreation; add helper to find sandbox by session label. |
| host, err = s.k8sClient.GetSandboxPodIP(ctx, sandbox.Namespace, sandbox.Name, sandboxPodName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get sandbox %s/%s pod IP: %v", sandbox.Namespace, sandbox.Name, err) | ||
| } |
There was a problem hiding this comment.
The error returned from GetSandboxPodIP is formatted with %v, which drops wrapping and prevents callers/tests from using errors.Is / errors.As on the underlying error. Use %w when embedding the underlying error (and keep the rest of the message the same).
| // 3. Wait for new pod to be ready (agent-sandbox reconciles and recreates) | ||
| require.Eventually(t, func() bool { | ||
| newPod, err := ctx.getPodByOwner(namespace, "Sandbox", sandbox.Name) | ||
| if err != nil || newPod == nil { | ||
| return false | ||
| } | ||
| for _, c := range newPod.Status.Conditions { | ||
| if c.Type == corev1.PodReady && c.Status == corev1.ConditionTrue { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }, 3*time.Minute, 5*time.Second, "New pod should become ready after recreation") |
There was a problem hiding this comment.
The wait for the "new pod" becoming ready can succeed while the deleted pod is still terminating (it may still be returned by getPodByOwner and may still have PodReady=true), which makes this test a potential false positive/flaky. Capture the original pod UID (or deletion timestamp) before deleting, and in the Eventually predicate require that the pod UID differs from the original and that DeletionTimestamp is nil in addition to PodReady=true.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This e2e can still pass without ever observing the replacement pod. After deletion, |
| return nil, fmt.Errorf("failed to get sandbox %s/%s pod IP: %v", sandbox.Namespace, sandbox.Name, err) | ||
| // Prefer ServiceFQDN: K8s Endpoints controller keeps DNS in sync when pod is evicted/recreated. | ||
| // Fallback to pod IP for agent-sandbox without ServiceFQDN. | ||
| host := createdSandbox.Status.ServiceFQDN |
There was a problem hiding this comment.
createdSandbox.Status.ServiceFQDN is preferred here, but upstream agent-sandbox currently hardcodes that field to .svc.cluster.local. If a cluster uses a different DNS domain, this may regress cases that previously worked via pod IP fallback.
| Path: port.PathPrefix, | ||
| Protocol: string(port.Protocol), | ||
| Endpoint: net.JoinHostPort(podIP, strconv.Itoa(int(port.Port))), | ||
| Endpoint: net.JoinHostPort(host, strconv.Itoa(int(port.Port))), |
There was a problem hiding this comment.
Changing Endpoint: net.JoinHostPort(host, ...) means entryPoints[].endpoint now effectively becomes a service DNS name instead of a pod IP-style address. Router usage looks fine, but it would be good to confirm there are no external consumers depending on the old behavior and to update the API docs if this is intentional.
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #205
Special notes for your reviewer:
Does this PR introduce a user-facing change?: