Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
61d751d to
c8ebad0
Compare
|
🌿 Preview your docs: https://nvidia-preview-pr-2265.docs.buildwithfern.com/openshell |
d7f06ff to
5cd240a
Compare
|
Label |
|
Label |
5cd240a to
c8b3044
Compare
c8b3044 to
c040b8d
Compare
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements roadmap issue #1606 for Docker GPU/CDI sandbox policy derivation, includes user-facing documentation, and has the required E2E/GPU E2E labels.
Head SHA: c040b8dd5d7adcb126d1c5199e6203ba5dfdc93b
Thanks @drew, I checked the CDI context trust-boundary concern you raised and confirmed it is still a blocking issue on this head. The independent review also found related supervisor-side hardening gaps and a Docker token cleanup regression.
Review findings:
- Blocking findings remain on CDI context trust, CDI device-node validation, and failed-provisioning token cleanup.
Docs: Fern docs were updated under docs/; no navigation change appears necessary for these existing pages.
Checks: Required checks are currently green, including DCO, Branch Checks, Helm Lint, Core E2E, and GPU E2E.
E2E: test:e2e and test:e2e-gpu are applied and passing.
Next state: gator:in-review
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements roadmap issue #1606 for Docker GPU/CDI sandbox policy derivation, includes architecture and Fern documentation updates, and has the required test:e2e and test:e2e-gpu labels.
Head SHA: cf6a99519f85611da0558468ce42f84c6940976d
Thanks @drew, I re-checked the CDI context trust-boundary concern you raised against the latest head. The Docker env cleanup and token cleanup paths are improved, but the independent review found the supervisor still needs a stronger driver-attestation boundary before CDI-derived policy is trusted.
Review findings:
- Blocking finding remains on CDI context trust/attestation.
- Warning remains on explicit CDI device ID namespace validation.
Docs: Fern docs were updated under docs/; no navigation change appears necessary for these existing pages.
Checks: Current head has DCO and Helm Lint passing, with Branch Checks, E2E, and GPU E2E still pending/queued.
E2E: test:e2e and test:e2e-gpu are applied; results are still pending for this head.
Next state: gator:in-review
| let cdi_context = self | ||
| .config | ||
| .gpu | ||
| .cdi_context(gpu_devices.as_deref()) |
There was a problem hiding this comment.
gator-agent
Warning: The selected IDs passed into the CDI context can come from explicit driver_config.cdi_devices, and current validation only checks count/duplicates, not that each ID is in the NVIDIA GPU CDI namespace. Since this PR now derives device, mount, and group policy from those specs, arbitrary CDI vendors can broaden sandbox permissions behind a GPU request. Please require nvidia.com/gpu= for explicit IDs, matching the default inventory filter, and add a negative unit test.
There was a problem hiding this comment.
I don't agree that these should be NVIDIA-SPECIFIC. CDI is vendor agnostic, and we should be able to use the same mechanism to inject CDI devices from other vendors. I agree that we could check for valid CDI device IDs though.
|
I looked at rebasing this and found two issues:
DetailsThe conflict is understandable: main added OCI image identity and working-directory handling in the same code that PR #2265 changes for CDI. It is not safe to choose either side wholesale. There are four conflicted files, with the conflict originating in feat(gpu): derive sandbox policy from CDI specs and then propagating through the two descendant commits.
The supervisor merge deserves extra care because main now uses supplementary groups during workspace authorization, before privilege dropping. Ideally CDI groups should be incorporated into the common resolved identity so workspace validation and the eventual setgroups() call see the same group set. Simply pasting the old CDI block after main’s new logic could reject a valid workspace or overwrite OCI-derived groups. Cargo.lock:1166 has four textual regions caused by main’s dependency updates colliding with CDI’s new packages. It should be regenerated from the merged manifests, retaining main’s newer dependency selections rather than selecting either lockfile side manually. |
cf6a995 to
6079eb8
Compare
9f1a3ef to
e73162e
Compare
e73162e to
9a23111
Compare
9a23111 to
51875a7
Compare
51875a7 to
93efb9f
Compare
93efb9f to
770c455
Compare
770c455 to
41e019c
Compare
41e019c to
38b5531
Compare
38b5531 to
8f977b5
Compare
8f977b5 to
ab5268c
Compare
Blocker Follow-Up NudgeThis item is still blocked because GitHub reports PR #2265 as conflicting with its stacked base after more than 48 business hours. The current head is a direct child of PR #2776’s current head, so the reported mergeability state may be stale. Next action: @elezar, please refresh the stacked branches so GitHub reports this PR as mergeable, or ask a maintainer to investigate the stale mergeability state. |
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Summary
Provide the Docker-side CDI producer for GPU sandboxes: discover CDI support, select devices, bind-mount a gateway-owned CDI context and daemon-reported specs read-only, and clear CDI state for non-GPU containers. This is the final PR in the #1606 stack.
Related Issue
Part of #1606. Depends on #2776.
Changes
Testing
mise run pre-commitpasses.Checklist