Skip to content

fix(router,workloadmanager): populate sandbox Kind and Namespace on create#338

Open
Abhinav-kodes wants to merge 1 commit into
volcano-sh:mainfrom
Abhinav-kodes:fix-router-sandbox-kind
Open

fix(router,workloadmanager): populate sandbox Kind and Namespace on create#338
Abhinav-kodes wants to merge 1 commit into
volcano-sh:mainfrom
Abhinav-kodes:fix-router-sandbox-kind

Conversation

@Abhinav-kodes
Copy link
Copy Markdown
Contributor

@Abhinav-kodes Abhinav-kodes commented May 14, 2026

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

When the router creates a new sandbox via the workload manager, the returned SandboxInfo had empty Kind and SandboxNamespace fields. This causes generateSandboxJWT to skip JWT signing for freshly-created sandboxes, since it checks sandbox.Kind against SandboxKind/SandboxClaimsKind.

This PR:

  1. Adds Kind to CreateSandboxResponse and populates it from the workload manager with the real resource kind (Sandbox or SandboxClaim)
  2. Populates SandboxNamespace in the router for consistency with store-fetched sandboxes

Special notes for your reviewer:

  • The Kind uses the resource kind (Sandbox/SandboxClaim), not the invoke kind (AgentRuntime/CodeInterpreter), to stay consistent with the store path and the workload manager's deletion logic.

Does this PR introduce a user-facing change?:

NONE

Copilot AI review requested due to automatic review settings May 14, 2026 13:32
@volcano-sh-bot volcano-sh-bot added the kind/bug Something isn't working label May 14, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the createSandbox function in pkg/router/session_manager.go to populate the Kind field in the SandboxInfo struct. Feedback indicates that using the 'invoke kind' for this field may cause inconsistencies and break deletion logic in the workload manager. It was also recommended to populate the SandboxNamespace field to maintain consistency with data retrieved from the store.

Comment thread pkg/router/session_manager.go Outdated
Comment thread pkg/router/session_manager.go Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 14, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.17%. Comparing base (524e55e) to head (c4e0bd5).
⚠️ Report is 54 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   47.57%   49.17%   +1.60%     
==========================================
  Files          30       30              
  Lines        2819     2861      +42     
==========================================
+ Hits         1341     1407      +66     
+ Misses       1338     1301      -37     
- Partials      140      153      +13     
Flag Coverage Δ
unittests 49.17% <100.00%> (+1.60%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…reate

Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
@Abhinav-kodes Abhinav-kodes force-pushed the fix-router-sandbox-kind branch from 888d6cb to c4e0bd5 Compare May 14, 2026 13:46
@volcano-sh-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Abhinav-kodes Abhinav-kodes changed the title fix(router): populate Kind on newly created sandbox fix(router,workloadmanager): populate sandbox Kind and Namespace on create May 14, 2026
@Abhinav-kodes
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a Kind field to the CreateSandboxResponse struct and ensures it is correctly populated and propagated within the session_manager and workloadmanager. Additionally, the SandboxNamespace is now included in the SandboxInfo object. The reviewer suggests further improving consistency by populating SandboxInfo with additional metadata from the response, such as timestamps and status, to ensure the in-memory object matches the persisted state.

Comment on lines 187 to 194
sandbox := &types.SandboxInfo{
SandboxID: res.SandboxID,
Name: res.SandboxName,
SessionID: res.SessionID,
EntryPoints: res.EntryPoints,
Kind: res.Kind,
SandboxNamespace: namespace,
SandboxID: res.SandboxID,
Name: res.SandboxName,
SessionID: res.SessionID,
EntryPoints: res.EntryPoints,
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Populate the SandboxInfo object using the actual namespace and metadata returned by the workload manager. This ensures that the in-memory object is fully consistent with what was persisted in the store, including timestamps and status.

	sandbox := &types.SandboxInfo{
		Kind:             res.Kind,
		SandboxNamespace: res.SandboxNamespace,
		SandboxID:        res.SandboxID,
		Name:             res.SandboxName,
		SessionID:        res.SessionID,
		EntryPoints:      res.EntryPoints,
		CreatedAt:        res.CreatedAt,
		ExpiresAt:        res.ExpiresAt,
		Status:           res.Status,
	}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The router only uses this SandboxInfo to resolve the upstream target and decide whether to sign with JWT, it doesn't need lifecycle metadata like CreatedAt, ExpiresAt, or Status. Those fields are populated when the sandbox is fetched from the store (existing session path). Keeping the response minimal avoids coupling the router to internal workload manager state.

For SandboxNamespace, the router already has it from the request parameter, no need to round-trip it through the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants