Skip to content

fix(driver-vm): resolve lifecycle requests on sandbox_id alone - #3305

Open
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3254-make-sandbox-id-requirement-for-driver-vm/letv1nnn
Open

fix(driver-vm): resolve lifecycle requests on sandbox_id alone#3305
letv1nnn wants to merge 1 commit into
NVIDIA:mainfrom
letv1nnn:3254-make-sandbox-id-requirement-for-driver-vm/letv1nnn

Conversation

@letv1nnn

Copy link
Copy Markdown
Contributor

Summary

stop_sandbox, start_sandbox, and delete_sandbox on the VM driver preferred the supplied sandbox_id but fell back to matching on sandbox_name whenever that id was absent from the registry. Because sandbox names are unique per workspace rather than globally, that fallback could resolve to a same-named sandbox in a different workspace. Lifecycle resolution now treats a supplied id as authoritative.

Related Issue

Fixes #3254

Changes

  • stop_sandbox, start_sandbox, and delete_sandbox select the resolution branch on whether the caller supplied a sandbox_id, not on whether that id happened to be present. A supplied id resolves on its own or reports absence.
  • Resolution moves into a shared resolve_record_id helper, replacing the same logic copy-pasted across the three methods.
  • A name-only request matching more than one sandbox is rejected with FailedPrecondition instead of resolving by HashMap iteration order.
  • Test helper insert_named_record sets a record's id, name, and workspace independently.

get_sandbox is unchanged: it already gates its name fallback on sandbox_id.is_empty(), and it is read-only.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers

Copy link
Copy Markdown
Collaborator

Is this unique to the vm driver or are others already using ID? I thought we were standardizing on workspace+sandbox_name for sandbox management. @drew can you weigh in?

@letv1nnn

Copy link
Copy Markdown
Contributor Author

hey @johntmyers, this is not unique to vm driver, docker (#3240) and mxc (#3278) drivers had the same weakness and got the same fix: sandbox_id wins, and a name-only match backs off instead of guessing.
Workspace+sandbox_name isn't available here because StopSandboxRequest/StartSandboxRequest/DeleteSandboxRequest in compute_driver.proto don't carry a workspace field, that's why K8s/Podman can key on workspace+name (they resolve through platform labels), but the registry-based drivers (docker/vm/mxc) can't without a proto change.

@drew

drew commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

We have this ticket #3050 to standardize how we reference sandboxes across the product. Lets prefer that ticket and associated pr. We should try and prefer workspace+name rather than id I think.

@letv1nnn

letv1nnn commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @drew, I read through #3272. My understanding is that it standardizes the public gateway RPCs on workspace + name while keeping sandbox IDs internal at the compute-driver boundary, and it doesn't touch compute_driver.proto or the driver crates. This PR sits below that line, so I think it's complementary rather than competing, but please correct me if I've read the intent wrong.

Two options, happy with either:

  1. Land as-is. Same id-authoritative shape as fix(driver-docker): scope pending sandbox matching by id and workspace #3240 and fix(driver-mxc): resolve StopSandbox by sandbox_id #3278, so all three registry drivers behave consistently. Ambiguous name-only requests return FailedPrecondition.
  2. Extend to workspace + name at the driver boundary. Add workspace to StopSandboxRequest/StartSandboxRequest/DeleteSandboxRequest in compute_driver.proto and have the driver resolve on (workspace, name), with sandbox_id as a consistency check. That makes the driver mirror feat(api)!: unify sandbox references across gateway RPCs #3050's model instead of diverging from it, but it's an additive proto change touching five driver crates, so I'd suggest it as a follow-up to feat(api)!: unify sandbox references across gateway RPCs #3050 rather than folded in here.

I'd lean toward 1 now and 2 as the follow-up, mainly so docker/vm/mxc don't sit in three different states while #3050 is in flight.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(driver-vm): lifecycle requests fall back to sandbox_name when the supplied sandbox_id is absent

3 participants