Use sandbox pod annotation for GetSandboxPodIP#413
Conversation
|
[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 updates the GetSandboxPodIP method in k8s_client.go to retrieve the backing pod name from the Sandbox resource's annotations instead of using the previous label and ownerReference fallback. It also updates the corresponding unit tests to pass explicit pod names and removes unused imports. I have no feedback to provide.
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.
There was a problem hiding this comment.
Pull request overview
This PR updates Workload Manager’s GetSandboxPodIP lookup flow to stop discovering pods via labels/OwnerReferences and instead rely on an explicit pod name (caller-provided) or the authoritative SandboxPodNameAnnotation stored on the Sandbox resource (via dynamic client).
Changes:
- Prefer direct pod lookup by explicit
podNameusing the pod lister cache. - When
podNameis not provided, readcontrollers.SandboxPodNameAnnotationfrom the Sandbox resource and use it to resolve the pod. - Update unit tests to pass explicit pod names and adjust expectations when no dynamic client is available.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/workloadmanager/k8s_client.go | Reworks GetSandboxPodIP to use explicit pod name and Sandbox annotation fallback; removes label/OwnerReference discovery. |
| pkg/workloadmanager/k8s_client_test.go | Updates tests to pass explicit pod names and to expect “not found” when annotation lookup can’t run. |
| go.sum | Removes unused golang.org/x/oauth2 v0.36.0 sums (module tidying). |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
===========================================
+ Coverage 47.57% 58.66% +11.09%
===========================================
Files 30 37 +7
Lines 2819 3493 +674
===========================================
+ Hits 1341 2049 +708
+ Misses 1338 1235 -103
- Partials 140 209 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if podName != "" { | ||
| pod, err := c.podLister.Pods(namespace).Get(podName) | ||
| if err == nil && pod != nil { | ||
| return validateAndGetPodIP(pod) | ||
| } | ||
| klog.Infof("failed to get sandbox pod %s/%s: %v, try get pod by sandbox-name label", namespace, podName, err) | ||
| klog.Infof("failed to get sandbox pod %s/%s: %v", namespace, podName, err) | ||
| } |
| if ann := sb.GetAnnotations()[controllers.SandboxPodNameAnnotation]; ann != "" { | ||
| podName = ann | ||
| pod, err := c.podLister.Pods(namespace).Get(podName) | ||
| if err == nil && pod != nil { | ||
| return validateAndGetPodIP(pod) | ||
| } | ||
| klog.Infof("failed to get sandbox pod %s/%s from annotation: %v", namespace, podName, err) | ||
| } |
| } | ||
|
|
||
| phase, found, err := unstructured.NestedString(status, "phase") | ||
| // OPTIMIZATION: Flatten nested loop parameters using unstructured fields path syntax directly |
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: "agents.x-k8s.io/v1alpha1", | ||
| Kind: "Sandbox", | ||
| }, |
| if c.dynamicClient == nil { | ||
| return "", fmt.Errorf("no pod found for sandbox %s: dynamic client not configured", sandboxName) | ||
| } |
|
@hzxuzhonghu Hello, Please review this PR, this PR Simplify sandbox pod match #277 . |
Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
safiya2610
force-pushed
the
fix/sandbox-pod-lookup
branch
from
7613f05 to
65d38f5
Compare
Summary
This change makes GetSandboxPodIP prefer an explicit pod name. When no pod name is provided, it falls back to reading the SandboxPodNameAnnotation from the Sandbox resource instead of discovering the pod through labels or OwnerReferences.
Changes
Fixes #277