[WIP] validation for PR #387 with review fixes#390
Conversation
Signed-off-by: ranxi2001 <ranxi169@163.com>
Signed-off-by: ranxi2001 <ranxi169@163.com>
volcano-sh-bot
requested review from
LiZhenCheng9527,
VanderChen,
YaoZengzeng and
acsoto
|
[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 upgrades Go to version 1.26.2, updates Kubernetes dependencies to v0.35.4, and adapts the codebase to support the new SandboxClaim and SandboxWarmPool models from agent-sandbox. Key changes include updating Dockerfiles, CRD manifests, and the CodeInterpreter controller, as well as refactoring sandbox creation handlers to wait for adopted sandboxes. The review feedback highlights a critical case-sensitivity typo in the fake clientset's watchlist-disabling method, a potential nil pointer dereference panic when reading from the sandbox readiness channel, a recommendation to return early on context cancellation during sandbox claim checks, and a suggestion to make the JSON parsing in the codegen script more robust.
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.
| // IsWatchListSemanticsSupported informs the reflector that this client | ||
| // doesn't support WatchList semantics. | ||
| // | ||
| // This is a synthetic method whose sole purpose is to satisfy the optional | ||
| // interface check performed by the reflector. | ||
| // Returning true signals that WatchList can NOT be used. | ||
| // No additional logic is implemented here. | ||
| func (c *Clientset) IsWatchListSemanticsUnSupported() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The method name is misspelled as IsWatchListSemanticsUnSupported (with a capital S). In standard Go and Kubernetes client-go, the optional interface watchListUnsupportedInterface expects the method name to be IsWatchListSemanticsUnsupported (with a lowercase s). Because Go interface matching is strictly case-sensitive, the reflector will fail to detect this method, and watchlist semantics will not be disabled as intended. This will cause unit tests using the fake client to fail or panic when watchlist is attempted.
| // IsWatchListSemanticsSupported informs the reflector that this client | |
| // doesn't support WatchList semantics. | |
| // | |
| // This is a synthetic method whose sole purpose is to satisfy the optional | |
| // interface check performed by the reflector. | |
| // Returning true signals that WatchList can NOT be used. | |
| // No additional logic is implemented here. | |
| func (c *Clientset) IsWatchListSemanticsUnSupported() bool { | |
| return true | |
| } | |
| // IsWatchListSemanticsUnsupported informs the reflector that this client | |
| // doesn't support WatchList semantics. | |
| // | |
| // This is a synthetic method whose sole purpose is to satisfy the optional | |
| // interface check performed by the reflector. | |
| // Returning true signals that WatchList can NOT be used. | |
| // No additional logic is implemented here. | |
| func (c *Clientset) IsWatchListSemanticsUnsupported() bool { | |
| return true | |
| } |
| case result := <-resultChan: | ||
| createdSandbox := result.Sandbox | ||
| klog.V(2).Infof("sandbox %s/%s reported ready, verifying entrypoints", createdSandbox.Namespace, createdSandbox.Name) | ||
| return createdSandbox, nil |
There was a problem hiding this comment.
When reading from resultChan, if the channel is closed, result will receive its zero value, and result.Sandbox will be nil. Attempting to access createdSandbox.Namespace or createdSandbox.Name on the next line will cause a nil pointer dereference panic. We should check the ok boolean from the channel read to ensure the channel wasn't closed, and also verify that createdSandbox is not nil before dereferencing it.
| case result := <-resultChan: | |
| createdSandbox := result.Sandbox | |
| klog.V(2).Infof("sandbox %s/%s reported ready, verifying entrypoints", createdSandbox.Namespace, createdSandbox.Name) | |
| return createdSandbox, nil | |
| case result, ok := <-resultChan: | |
| if !ok { | |
| return nil, fmt.Errorf("sandbox ready channel closed unexpectedly") | |
| } | |
| createdSandbox := result.Sandbox | |
| if createdSandbox == nil { | |
| return nil, fmt.Errorf("received nil sandbox from ready channel") | |
| } | |
| klog.V(2).Infof("sandbox %s/%s reported ready, verifying entrypoints", createdSandbox.Namespace, createdSandbox.Name) | |
| return createdSandbox, nil |
| for { | ||
| claim, err := getSandboxClaim(ctx, dynamicClient, sandboxClaim.Namespace, sandboxClaim.Name) | ||
| if err != nil && !isContextError(err) { | ||
| klog.V(2).Infof("waiting for sandbox claim %s/%s status: %v", sandboxClaim.Namespace, sandboxClaim.Name, err) | ||
| } |
There was a problem hiding this comment.
If getSandboxClaim fails due to context cancellation, we should return immediately instead of continuing the loop and relying on the select block. This prevents unnecessary processing and potential extra API calls when the context is already canceled.
for {
claim, err := getSandboxClaim(ctx, dynamicClient, sandboxClaim.Namespace, sandboxClaim.Name)
if err != nil {
if isContextError(err) {
return nil, err
}
klog.V(2).Infof("waiting for sandbox claim %s/%s status: %v", sandboxClaim.Namespace, sandboxClaim.Name, err)
}|
|
||
| # Find code-generator in module cache | ||
| CODEGEN_PKG=$(go list -m -f '{{.Dir}}' "k8s.io/code-generator@${CODEGEN_VERSION}" 2>/dev/null || echo "") | ||
| CODEGEN_PKG=$(printf '%s\n' "${CODEGEN_JSON}" | sed -n 's/^[[:space:]]*"Dir": "\(.*\)",$/\1/p') |
There was a problem hiding this comment.
The sed pattern expects a trailing comma: ,". If "Dir" happens to be the last field in the JSON output of go mod download -json (or if the formatting changes), there might not be a trailing comma, causing the parsing to fail. A more robust and portable pattern that doesn't depend on the trailing comma or spaces is sed -n 's/^[[:space:]]*"Dir": "\([^"]*\)".*/\1/p'.
| CODEGEN_PKG=$(printf '%s\n' "${CODEGEN_JSON}" | sed -n 's/^[[:space:]]*"Dir": "\(.*\)",$/\1/p') | |
| CODEGEN_PKG=$(printf '%s\n' "${CODEGEN_JSON}" | sed -n 's/^[[:space:]]*"Dir": "\([^"]*\)".*/\1/p') |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
===========================================
+ Coverage 47.57% 58.72% +11.15%
===========================================
Files 30 34 +4
Lines 2819 3254 +435
===========================================
+ Hits 1341 1911 +570
+ Misses 1338 1146 -192
- Partials 140 197 +57
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:
|
There was a problem hiding this comment.
Pull request overview
Temporary validation PR to exercise CI for the upstream agent-sandbox v0.4.6 adaptation (#387) plus follow-up fixes (lint/coverage alignment, sandbox readiness handling, warm-pool e2e ownership matching, and test/store fixes) in AgentCube’s Workload Manager + generated artifacts.
Changes:
- Align build/CI/tooling to Go
1.26.2and updated Kubernetes/client-go stack (v0.35.4) required bysigs.k8s.io/agent-sandbox v0.4.6. - Improve sandbox creation flow: add direct-sandbox readiness watcher guard, refactor error handling, and correctly handle adopted Sandbox (claim → sandbox) for probing while storing the claim identity.
- Update warm-pool e2e helpers to support both legacy direct Pod ownership and new
SandboxWarmPool -> Sandbox -> Podownership via Sandbox UID.
Reviewed changes
Copilot reviewed 15 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/e2e_test.go | Warm-pool pod discovery updated to handle sandbox-owned pods (UID-based) in newer agent-sandbox controller shapes. |
| pkg/workloadmanager/sandbox_helper.go | Include CreatedAt in store placeholder to avoid zero timestamps during creation. |
| pkg/workloadmanager/k8s_client.go | Add dynamic-client getters for Sandbox and SandboxClaim used by claim adoption polling. |
| pkg/workloadmanager/handlers.go | Refactor sandbox create error handling; add direct-ready watcher guard; poll claim→adopted sandbox readiness; switch to public SandboxPodNameAnnotation. |
| pkg/workloadmanager/handlers_test.go | Extend tests to validate claim storage vs adopted sandbox probing and nil-ready-watcher guard behavior. |
| pkg/workloadmanager/codeinterpreter_controller.go | Force NetworkPolicyManagement: Unmanaged on SandboxTemplate and ensure existing templates are updated accordingly. |
| pkg/workloadmanager/codeinterpreter_controller_test.go | Add coverage ensuring network policy management is set/updated to Unmanaged. |
| manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml | Regenerated CRD schema changes from dependency/Kubernetes generator updates. |
| hack/update-codegen.sh | Update code-generator version and use go mod download -json to locate it without go get -d. |
| go.mod | Bump Go version and key deps (agent-sandbox, k8s.io/, controller-runtime, golang.org/x/). |
| go.sum | Dependency checksum updates consistent with module bumps and regeneration. |
| docker/Dockerfile.router | Builder image updated to Go 1.26.2. |
| docker/Dockerfile.picod | Builder image updated to Go 1.26.2; tighten apt install (no recommends) and clean apt lists. |
| docker/Dockerfile | Builder image updated to Go 1.26.2. |
| client-go/informers/externalversions/runtime/v1alpha1/codeinterpreter.go | Regenerated informer to use WatchList semantics wrapper. |
| client-go/informers/externalversions/runtime/v1alpha1/agentruntime.go | Regenerated informer to use WatchList semantics wrapper. |
| client-go/informers/externalversions/factory.go | Minor generated comment tweak (context cancel example) and doc formatting. |
| client-go/clientset/versioned/fake/clientset_generated.go | Generated fake clientset updated; adds WatchList semantics capability hook (with a doc-comment mismatch noted). |
| .github/workflows/test-coverage.yml | CI coverage job Go version aligned to 1.26.2. |
| .github/workflows/lint.yml | CI lint job Go version aligned to 1.26.2. |
Files not reviewed (4)
- client-go/clientset/versioned/fake/clientset_generated.go: Generated file
- client-go/informers/externalversions/factory.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/agentruntime.go: Generated file
- client-go/informers/externalversions/runtime/v1alpha1/codeinterpreter.go: Generated file
| // IsWatchListSemanticsSupported informs the reflector that this client | ||
| // doesn't support WatchList semantics. | ||
| // | ||
| // This is a synthetic method whose sole purpose is to satisfy the optional | ||
| // interface check performed by the reflector. | ||
| // Returning true signals that WatchList can NOT be used. | ||
| // No additional logic is implemented here. |
|
/retitle [WIP] validation for PR #387 with review fixes |
volcano-sh-bot
changed the title
This is a temporary validation PR for upstream PR #387.
Please do not review or merge this PR. It exists only to run the upstream bot / CI on the final expected patch set:
golangci-lint/coverageCI failures.If this validation PR passes CI, I will port the same fix commit back into #387 with a clean history and close this PR.
Included follow-up fixes:
lintandtest-coverageworkflow Go versions withgo.modtarget Go1.26.2.handleSandboxCreatecomplexity by extracting sandbox create error handling.Sandbox -> PodownerRefs.EntryPoints.Local validation before opening this PR:
go test ./pkg/workloadmanager -count=1make lintgit diff --checkAdditional validation already run on the follow-up fix branch:
go test ./test/e2e -run TestNonExistent -count=1go test -race -v -coverprofile=coverage.out -coverpkg=./pkg/... ./pkg/...go test -race ./pkg/workloadmanager -count=1go test ./pkg/... -count=1Known local limitation:
go test ./... -count=1fails only intest/e2ewhen local Router / WorkloadManager are not running and no kubeconfig is configured. The non-e2e package set passes.AI assistance: Used Codex to inspect CI logs, implement the fix, and run local validation. I reviewed the changes and test output.
/kind bug