fix: add missing workloadName for CodeInterpreter sandboxes#310
fix: add missing workloadName for CodeInterpreter sandboxes#310HarshitPal25 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: HarshitPal25 <harshit13082006@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 |
There was a problem hiding this comment.
Pull request overview
Fixes missing runtime.agentcube.io/workload-name labeling for CodeInterpreter sandboxes generated by the Workload Manager, aligning CodeInterpreter sandbox construction with the existing AgentRuntime path.
Changes:
- Populate
buildSandboxParams.workloadNameinbuildSandboxByCodeInterpretersoWorkloadNameLabelKeyis set on created Sandbox objects.
| buildParams := &buildSandboxParams{ | ||
| sandboxName: sandboxName, | ||
| namespace: namespace, | ||
| workloadName: codeInterpreterName, | ||
| sessionID: sessionID, |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
+ 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:
|
There was a problem hiding this comment.
Code Review
This pull request updates the buildSandboxByCodeInterpreter function to include the workloadName in the sandbox build parameters. However, the feedback indicates that the implementation is incomplete as it misses applying the WorkloadNameLabelKey label to the warm pool path, specifically for the simpleSandbox and SandboxClaim objects.
| buildParams := &buildSandboxParams{ | ||
| sandboxName: sandboxName, | ||
| namespace: namespace, | ||
| workloadName: codeInterpreterName, |
There was a problem hiding this comment.
While this change correctly addresses the missing workloadName for the direct sandbox creation path, the fix appears incomplete based on the PR description. The WorkloadNameLabelKey label is still missing in the warm pool path (when WarmPoolSize > 0). Specifically, the simpleSandbox object created at line 384 and the SandboxClaim created at line 371 should also include the WorkloadNameLabelKey label to ensure consistency across all CodeInterpreter workloads.
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.