fix(e2e): allow Brev GPU bridge gateway traffic#3960
Open
ericksoa wants to merge 10 commits into
Open
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
the
E2E
Contributor
|
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:
📝 WalkthroughWalkthroughExtracts GPU Docker runtime command assembly into gpuDockerRuntimeSetupCommands(), adds ChangesGPU Runtime Setup for Brev
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ericksoa
added
Platform: Brev
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Review used read-only repository/diff inspection and deterministic PR context; no PR scripts, package-manager commands, or live E2E jobs were executed by this review.; Linked issue #3959 body/comments were not available in the deterministic context, so issue-specific clauses could not be extracted literally beyond the PR body reference.; CI is still pending and mergeability is blocked for the requested head SHA; final gate status may change after checks complete.; Review thread state is unknown despite empty GraphQL reviewThreads.nodes; CodeRabbit auto-review was paused in the provided issue comment.; Brev E2E failure comments were available, but logs were not inspected, so the exact failure cause is unknown.; The behavioral safety and effectiveness of the UFW rules cannot be fully proven without a successful live Brev GPU VM run. Full advisor summaryPR Review AdvisorBase: Patch is localized to Brev/GPU E2E harness files and appears directionally scoped, but it is not merge-ready because CI is pending, mergeability is blocked/review-required, and repeated Brev GPU validation failures have no passing head-SHA evidence. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/brev-e2e.test.ts (1)
1057-1061: ⚡ Quick winBind the
|| truecheck to the UFW rule assertion.The standalone
expect(setup).toContain("|| true")can pass if any unrelated command includes that token. Assert the full UFW rule line with its tolerant suffix to avoid false positives.Proposed test tightening
- expect(setup).toContain( - `ufw allow from ${DOCKER_BRIDGE_CIDR} to any port ${OPENSHELL_GATEWAY_PORT} proto tcp`, - ); - expect(setup).toContain("|| true"); + expect(setup).toContain( + `ufw allow from ${DOCKER_BRIDGE_CIDR} to any port ${OPENSHELL_GATEWAY_PORT} proto tcp >/dev/null || true`, + );🤖 Prompt for 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. In `@test/e2e/brev-e2e.test.ts` around lines 1057 - 1061, The test currently asserts expect(setup).toContain(...) for the UFW rule and separately expect(setup).toContain("|| true"), which can yield false positives; update the assertion so the UFW rule expectation includes the tolerant suffix by checking for the full combined string `ufw allow from ${DOCKER_BRIDGE_CIDR} to any port ${OPENSHELL_GATEWAY_PORT} proto tcp || true` (i.e. modify the expect that references setup, DOCKER_BRIDGE_CIDR and OPENSHELL_GATEWAY_PORT to include the trailing "|| true" in the same toContain call).
🤖 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.
Nitpick comments:
In `@test/e2e/brev-e2e.test.ts`:
- Around line 1057-1061: The test currently asserts expect(setup).toContain(...)
for the UFW rule and separately expect(setup).toContain("|| true"), which can
yield false positives; update the assertion so the UFW rule expectation includes
the tolerant suffix by checking for the full combined string `ufw allow from
${DOCKER_BRIDGE_CIDR} to any port ${OPENSHELL_GATEWAY_PORT} proto tcp || true`
(i.e. modify the expect that references setup, DOCKER_BRIDGE_CIDR and
OPENSHELL_GATEWAY_PORT to include the trailing "|| true" in the same toContain
call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: faffbc97-2373-4796-8f64-727cb2271e37
📒 Files selected for processing (1)
test/e2e/brev-e2e.test.ts
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
|
❌ Brev E2E (gpu): FAILED on branch |
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| gpu-e2e | ⏭️ skipped |
Contributor
|
✅ Brev E2E (gpu): PASSED on branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refs #3959.
Testing
npx vitest run test/e2e/brev-e2e.test.ts -t "Brev GPU runtime setup|Brev deploy input validation"git diff --checkFollow-up
[2/8] Starting OpenShell gatewayand reaches sandbox GPU proof/inference.Summary by CodeRabbit
Tests
Bug Fixes / Reliability