fix: add missing workloadName for CodeInterpreter sandboxes#309
fix: add missing workloadName for CodeInterpreter sandboxes#309HarshitPal25 wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
In buildSandboxByCodeInterpreter, the buildSandboxParams never set the workloadName field, causing the WorkloadNameLabelKey label to be empty on all CodeInterpreter Sandbox objects. The AgentRuntime path (buildSandboxByAgentRuntime) correctly sets workloadName. This fix ensures CodeInterpreter sandboxes carry the same WorkloadNameLabelKey label as AgentRuntime sandboxes, which is used for label-based queries and debugging. Signed-off-by: Mahil Patel <mahil2040@gmail.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 |
|
Welcome @HarshitPal25! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Code Review
This pull request adds the workloadName field to the buildSandboxParams struct within the buildSandboxByCodeInterpreter function. The reviewer pointed out that the implementation is incomplete because it does not address the 'warm pool' path, which also requires the workloadName label to ensure all CodeInterpreter workloads are correctly labeled.
| buildParams := &buildSandboxParams{ | ||
| sandboxName: sandboxName, | ||
| namespace: namespace, | ||
| workloadName: codeInterpreterName, |
There was a problem hiding this comment.
The PR aims to add the missing workloadName to CodeInterpreter sandboxes, but the fix appears to be incomplete based on the PR description's goal of covering "all CodeInterpreter Sandbox objects".
The "warm pool" path (lines 370-399) also creates a Sandbox object (line 384) and a SandboxClaim (line 371) that are currently missing the WorkloadNameLabelKey label. To ensure all CodeInterpreter workloads are correctly labeled for queries and debugging, these objects should also be updated to include the workloadName label.
There was a problem hiding this comment.
Pull request overview
Fixes a labeling gap in the Workload Manager sandbox builder so CodeInterpreter-created Sandbox objects carry runtime.agentcube.io/workload-name, aligning them with the AgentRuntime path and improving label-based querying/debugging.
Changes:
- Set
workloadName: codeInterpreterNamewhen building Sandboxes from CodeInterpreter resources.
Comments suppressed due to low confidence (1)
pkg/workloadmanager/workload_builder.go:434
- No unit test currently asserts that CodeInterpreter-built Sandboxes include
WorkloadNameLabelKeyin.metadata.labels. Since this label is relied on for queries/debugging and was previously missing, please add coverage (e.g., a focused test that constructs a CodeInterpreter object in the informer store and verifies the built Sandbox has the expected workload-name label).
buildParams := &buildSandboxParams{
sandboxName: sandboxName,
namespace: namespace,
workloadName: codeInterpreterName,
sessionID: sessionID,
podSpec: podSpec,
podLabels: codeInterpreterObj.Spec.Template.Labels,
podAnnotations: codeInterpreterObj.Spec.Template.Annotations,
idleTimeout: idleTimeout,
}
| buildParams := &buildSandboxParams{ | ||
| sandboxName: sandboxName, | ||
| namespace: namespace, | ||
| workloadName: codeInterpreterName, | ||
| sessionID: sessionID, | ||
| podSpec: podSpec, | ||
| podLabels: codeInterpreterObj.Spec.Template.Labels, |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
==========================================
+ Coverage 47.57% 47.74% +0.17%
==========================================
Files 30 30
Lines 2819 2855 +36
==========================================
+ Hits 1341 1363 +22
- Misses 1338 1344 +6
- 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:
|
What this PR does
In
buildSandboxByCodeInterpreter, thebuildSandboxParamsnever sets theworkloadNamefield, causing theWorkloadNameLabelKeylabel to be emptyon all CodeInterpreter Sandbox objects.
The AgentRuntime path (
buildSandboxByAgentRuntime) correctly setsworkloadNameat line 274:But the CodeInterpreter path at line 425 does not:
Impact
CodeInterpreter sandboxes are missing the runtime.agentcube.io/workload-name label
This affects label-based queries and debugging for CodeInterpreter workloads
Fix
Added workloadName: codeInterpreterName to match the AgentRuntime pattern.