test(e2e): validate native GPU sandbox topology - #7040
Conversation
Co-authored-by: hulynn <lynnh@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
Recommended Blocker
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe GPU E2E test validates direct sandbox GPU enablement, confirms one running and healthy native GPU container remains, and verifies the sandbox returns to the Ready phase after inference. ChangesGPU E2E validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/live/gpu-e2e.test.ts`:
- Around line 197-223: The sandbox container assertion currently checks only
that one labeled container exists. Update the docker query and parsing around
sandboxContainerNames to retrieve container status and health, then assert the
retained container is running and healthy while preserving the
exactly-one-container requirement.
- Line 195: Update the negative assertion in the GPU end-to-end test to reject
the complete “Docker GPU mode selected” marker, including variants without a
trailing colon. Ensure the native-route gate checks the same substring used by
the shared helper rather than only the colon-suffixed form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4d8a998-ebd6-472c-b3c9-a05296e9a5ff
📒 Files selected for processing (1)
test/e2e/live/gpu-e2e.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Blocking exact head bebde17 on two valid exact-head CodeRabbit findings before authorizing the live GPU plan. Align the negative oracle with the shared helper by rejecting the complete Docker GPU mode selected substring (not only the colon-suffixed variant). Also make the topology inventory prove the sole retained labeled container is running and healthy, not merely present/name-matched; preserve the exactly-one and no-backup assertions. Then run the focused mismatch/unit coverage and request trusted GPU E2E authorization/rereview.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Addendum at bebde17: exact-head Advisor Nemotron also identifies a valid acceptance gap from #5662. nemoclaw status can exit 0 while rendering a non-Ready phase, so GPU/CUDA/topology checks can mask Error/Starting. Add an explicit OpenShell sandbox Phase: Ready assertion—preferably after routed inference—alongside the complete legacy-marker rejection and running/healthy exactly-one-container proof already requested.
Co-authored-by: hulynn <lynnh@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6cdf24f. The marker and running/healthy-container blockers are fixed. One acceptance blocker remains: #5662 defines the closure signal as OpenShell Phase: Ready plus CUDA proof and no legacy swap, but this test still relies on nemoclaw status exit 0/CUDA text and never asserts the OpenShell sandbox phase. nemoclaw status can exit zero while rendering a non-Ready phase. Please add a fail-closed exact openshell sandbox get/list assertion for Phase: Ready (preferably after routed inference), then run the authorized cloud-onboard, credential-sanitization, security-posture, and gpu-e2e plan on the refreshed head.
Co-authored-by: hulynn <lynnh@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the remaining review gate at exact head |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Co-authored-by: Charan Jagwani <cjagwani@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact head |
|
Exact-head gate update for
Approval is intentionally held until #7024 lands: the current |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
4 participants
Summary
Strengthen the existing Linux GPU onboarding acceptance so release sign-off proves the ordinary native OpenShell GPU route leaves one healthy sandbox container. The live test now fails if legacy container-swap behavior or a stopped backup sibling remains, even when the patched container itself appears healthy.
Related Issue
Fixes #5662
Changes
docker ps -a, including stopped siblings.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablegit diff --checkpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit