fix: simplify sandbox pod lookup#314
Conversation
![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 Go version and several Kubernetes-related dependencies, while simplifying the sandbox pod IP retrieval logic to strictly use the pod name annotation instead of falling back to label selectors or owner references. Corresponding updates were made to the test suites to reflect these logic changes. Feedback identifies that the specified Go and Kubernetes versions appear invalid or future-dated, which will cause build failures. Additionally, a redundant nil check was noted in the Kubernetes client implementation.
| go 1.24.4 | ||
|
|
||
| toolchain go1.24.9 | ||
| go 1.26.1 |
There was a problem hiding this comment.
The Go version 1.26.1 and several dependency versions (e.g., k8s.io/api v0.35.0, sigs.k8s.io/structured-merge-diff/v6 with a 2026 timestamp) appear to be invalid or future-dated. Go 1.24 is the current stable release, and K8s v0.35.0 is not yet available. These versions will cause build failures as they cannot be resolved by the Go proxy. Please use valid, released versions.
| if pod == nil { | ||
| return "", fmt.Errorf("sandbox pod %s/%s not found", namespace, podName) | ||
| } | ||
|
|
||
| return "", fmt.Errorf("no pod found for sandbox %s", sandboxName) | ||
| return validateAndGetPodIP(pod) |
There was a problem hiding this comment.
This nil check is redundant. In the standard Kubernetes podLister, the Get method returns a non-nil error if the object is not found in the cache. If err == nil, the pod object is guaranteed to be non-nil. Removing this check simplifies the logic.
return validateAndGetPodIP(pod)There was a problem hiding this comment.
Pull request overview
This PR updates the Workload Manager’s sandbox pod IP resolution to rely on the SandboxPodNameAnnotation provided by sigs.k8s.io/agent-sandbox (v0.3.10), eliminating the previous label-selector + ownerReference lookup.
Changes:
- Simplifies
GetSandboxPodIPto do a direct pod-lister lookup by annotated pod name only (no fallback). - Updates sandbox creation flow to require
SandboxPodNameAnnotationand adds/adjusts unit tests for the new behavior. - Bumps
agent-sandbox(and related Kubernetes/controller-runtime deps), with accompanyinggo.mod/go.sumupdates.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workloadmanager/k8s_client.go | Removes label-based fallback; pod IP lookup is now direct by pod name from lister. |
| pkg/workloadmanager/k8s_client_test.go | Updates mocks/tests to cover direct lookup, missing pod name, and no-fallback behavior. |
| pkg/workloadmanager/informers_test.go | Minor test updates for informer factory construction and formatting. |
| pkg/workloadmanager/handlers.go | Uses SandboxPodNameAnnotation as the only source of truth; errors if missing. |
| pkg/workloadmanager/handlers_test.go | Adjusts tests to use the new annotation constant and adds missing-annotation rollback case. |
| go.mod | Updates dependencies (agent-sandbox, k8s libs, controller-runtime) and changes Go version directive. |
| go.sum | Updates module sums due to dependency changes. |
| module github.com/volcano-sh/agentcube | ||
|
|
||
| go 1.24.4 | ||
|
|
||
| toolchain go1.24.9 | ||
| go 1.26.1 | ||
|
|
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
2d6dfa0 to
28f3b75
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
==========================================
- Coverage 47.57% 47.34% -0.23%
==========================================
Files 30 30
Lines 2819 2915 +96
==========================================
+ Hits 1341 1380 +39
- Misses 1338 1387 +49
- Partials 140 148 +8
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:
|
|
@krrishrastogi05 I think this rely on the agent-sandbox version, maybe the e2e failure related |
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
28f3b75 to
6470c8f
Compare
| E2E_CLUSTER_NAME=${E2E_CLUSTER_NAME:-agentcube-e2e} | ||
| E2E_CLEAN_CLUSTER=${E2E_CLEAN_CLUSTER:-true} | ||
| E2E_SKIP_SETUP=${E2E_SKIP_SETUP:-false} | ||
| AGENT_SANDBOX_VERSION=${AGENT_SANDBOX_VERSION:-v0.1.1} | ||
| AGENT_SANDBOX_VERSION=${AGENT_SANDBOX_VERSION:-v0.3.10} | ||
| WORKLOAD_MANAGER_IMAGE=${WORKLOAD_MANAGER_IMAGE:-workloadmanager:latest} | ||
| ROUTER_IMAGE=${ROUTER_IMAGE:-agentcube-router:latest} | ||
| PICOD_IMAGE=${PICOD_IMAGE:-picod:latest} |
| sandboxPodName = podName | ||
| sandboxPodName := createdSandbox.Annotations[controllers.SandboxPodNameAnnotation] | ||
| if sandboxPodName == "" { | ||
| return nil, api.NewInternalError(fmt.Errorf("sandbox %s/%s missing pod name annotation %s", sandbox.Namespace, sandbox.Name, controllers.SandboxPodNameAnnotation)) |
| if pod == nil { | ||
| return "", fmt.Errorf("sandbox pod %s/%s not found", namespace, podName) | ||
| } |
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
6470c8f to
351cd62
Compare
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
351cd62 to
b525ae0
Compare
|
[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 |
| go 1.26.1 | ||
|
|
| @@ -13,14 +11,14 @@ require ( | |||
| github.com/redis/go-redis/v9 v9.17.1 | |||
| github.com/stretchr/testify v1.11.1 | |||
| github.com/valkey-io/valkey-go v1.0.69 | |||
| golang.org/x/net v0.47.0 | |||
| k8s.io/api v0.34.1 | |||
| k8s.io/apimachinery v0.34.1 | |||
| k8s.io/client-go v0.34.1 | |||
| golang.org/x/net v0.48.0 | |||
| k8s.io/api v0.35.0 | |||
| k8s.io/apimachinery v0.35.0 | |||
| k8s.io/client-go v0.35.0 | |||
| k8s.io/klog/v2 v2.130.1 | |||
| k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 | |||
| sigs.k8s.io/agent-sandbox v0.1.1 | |||
| sigs.k8s.io/controller-runtime v0.22.2 | |||
| sigs.k8s.io/agent-sandbox v0.3.10 | |||
| sigs.k8s.io/controller-runtime v0.23.3 | |||
| ) | |||
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
b525ae0 to
ba0d334
Compare
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
ba0d334 to
7b1b2a7
Compare
| go 1.24.4 | ||
|
|
||
| toolchain go1.24.9 | ||
| go 1.26.1 |
| @@ -13,21 +11,20 @@ require ( | |||
| github.com/redis/go-redis/v9 v9.17.1 | |||
| github.com/stretchr/testify v1.11.1 | |||
| github.com/valkey-io/valkey-go v1.0.69 | |||
| golang.org/x/net v0.47.0 | |||
| k8s.io/api v0.34.1 | |||
| k8s.io/apimachinery v0.34.1 | |||
| k8s.io/client-go v0.34.1 | |||
| golang.org/x/net v0.48.0 | |||
| k8s.io/api v0.35.0 | |||
| k8s.io/apimachinery v0.35.0 | |||
| k8s.io/client-go v0.35.0 | |||
| k8s.io/klog/v2 v2.130.1 | |||
| k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 | |||
| sigs.k8s.io/agent-sandbox v0.1.1 | |||
| sigs.k8s.io/controller-runtime v0.22.2 | |||
| sigs.k8s.io/agent-sandbox v0.3.10 | |||
| sigs.k8s.io/controller-runtime v0.23.3 | |||
| @@ -1,5 +1,5 @@ | |||
| # Multi-stage build for agentcube-router | |||
| FROM golang:1.24.9-alpine AS builder | |||
| FROM golang:1.26.1-alpine AS builder | |||
Signed-off-by: Krrish <krrishrastogi00@gmail.com>
krrishrastogi05
force-pushed
the
krrish/simplify-sandbox-pod-match
branch
from
7b1b2a7 to
8a205a2
Compare
|
After your comment I tried to rework the PR properly around So far I have: The CI exposed changes step by step, so I had to force-push a few times. First it was dependency/toolchain mismatch, then Docker/CI Go version mismatch, then warm-pool claim name resolution. Could you please point me in the right direction here? |
/kind bug
What this PR does / why we need it:
Updates
sigs.k8s.io/agent-sandboxtov0.3.10and usesSandboxPodNameAnnotationas the source of truth for the backing pod name.After kubernetes-sigs/agent-sandbox#272, Sandboxes are annotated with their pod name, so the workload manager no longer needs to list pods by label and then verify ownerReferences. This keeps sandbox pod lookup simpler and direct through the pod lister/cache.
Which issue(s) this PR fixes:
Fixes #277
Special notes for your reviewer:
This removes the label selector + ownerReference fallback from
GetSandboxPodIP. Unit tests cover direct pod-name lookup, missing annotations, invalid pod status, and the no-fallback behavior when a label-matched pod exists but the annotated pod is missing.Testing done:
go test ./pkg/workloadmanagergolangci-lint run ./pkg/workloadmanagergit diff --checkDoes this PR introduce a user-facing change?: