[WIP] feat(minimald): --gvproxy-bin flag + just recipes for #478 networking/VM bring-up - #574
Closed
norrietaylor wants to merge 2 commits into
Closed
[WIP] feat(minimald): --gvproxy-bin flag + just recipes for #478 networking/VM bring-up#574norrietaylor wants to merge 2 commits into
norrietaylor wants to merge 2 commits into
Conversation
The daemon resolved the gvproxy ("gvisor-tap-vsock") switch binary only
from the fixed install path `/usr/lib/minimal/bin/gvproxy`, so exercising
the OwnIp data plane on a dev host required installing gvproxy
system-wide (sudo). Add `minimald run --gvproxy-bin <path>` to point the
per-host switch at an arbitrary binary — e.g. one fetched by
scripts/fetch-gvproxy.sh — with no system install. Unset preserves the
previous default path. The flag survives the `--detach` re-exec since
spawn_detached forwards argv verbatim.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Capture the Lane A (native netns proofs, mesh proof, networking feature suites) and Lane B (minvmd VM E2E) command sequences as just recipes that mirror ci-netns.yml and ci-linux-kvm.yml, so a dev reproduces each lane with one command instead of reassembling the env-var soup by hand. Also adds fetch-gvproxy, setup-minvmd, setup-kvm-group, and a run-minimald recipe that wires the new --gvproxy-bin flag to a fetched gvproxy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds a Changesgvproxy override and workflow recipes
Sequence Diagram(s)sequenceDiagram
participant User
participant MinimaldRun
participant ListenArgs
participant MinimaldServerConfig
participant OwnIpSwitch
User->>MinimaldRun: passes --gvproxy-bin <path>
MinimaldRun->>ListenArgs: stores gvproxy_bin
ListenArgs->>MinimaldServerConfig: clones gvproxy_bin
MinimaldServerConfig->>OwnIpSwitch: uses override
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
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 `@justfile`:
- Around line 41-47: The mesh_uc7 recipe can invoke sudo with an empty binary
path if the cargo/jq pipeline returns nothing, so add an explicit guard after
resolving bin in the justfile recipe. In the recipe that sets bin and runs
"$bin", check that mesh_uc7 binary resolution succeeded and fail early with a
clear message before the sudo execution if it did not.
- Around line 68-80: The test-minvmd-e2e Just recipe assumes .scratch artifacts
already exist, so it can fail on a clean checkout. Update the test-minvmd-e2e
target in the justfile to depend on setup-minvmd before running the two sg kvm
cargo test commands, so the generated boot and rootfs files are prepared first
and the recipe is runnable directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48a11be4-5423-41ed-a6ec-e6b3bf59472b
📒 Files selected for processing (3)
crates/minimald/src/main.rscrates/minimald/src/server.rsjustfile
Comment on lines
+41
to
+47
| bin=$(cargo test -p minimald --features networking-wg --test mesh_uc7 \ | ||
| --no-run --message-format=json \ | ||
| | jq -r 'select(.executable != null and (.target.name? == "mesh_uc7")) | .executable' \ | ||
| | tail -1) | ||
| echo "mesh_uc7 binary: $bin" | ||
| sudo -E MINIMALD_NETNS_TEST=1 GVPROXY_BIN="$PWD/.scratch/gvproxy" \ | ||
| "$bin" --include-ignored --nocapture |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty mesh_uc7 executable resolution before sudo execution.
If the JSON filter returns nothing, "$bin" is empty and the recipe fails with a non-actionable error. Add an explicit check with a clear message.
Suggested patch
bin=$(cargo test -p minimald --features networking-wg --test mesh_uc7 \
--no-run --message-format=json \
| jq -r 'select(.executable != null and (.target.name? == "mesh_uc7")) | .executable' \
| tail -1)
+ if [[ -z "${bin:-}" ]]; then
+ echo "mesh_uc7 executable not found; cargo test --no-run did not emit a binary path" >&2
+ exit 1
+ fi
echo "mesh_uc7 binary: $bin"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bin=$(cargo test -p minimald --features networking-wg --test mesh_uc7 \ | |
| --no-run --message-format=json \ | |
| | jq -r 'select(.executable != null and (.target.name? == "mesh_uc7")) | .executable' \ | |
| | tail -1) | |
| echo "mesh_uc7 binary: $bin" | |
| sudo -E MINIMALD_NETNS_TEST=1 GVPROXY_BIN="$PWD/.scratch/gvproxy" \ | |
| "$bin" --include-ignored --nocapture | |
| bin=$(cargo test -p minimald --features networking-wg --test mesh_uc7 \ | |
| --no-run --message-format=json \ | |
| | jq -r 'select(.executable != null and (.target.name? == "mesh_uc7")) | .executable' \ | |
| | tail -1) | |
| if [[ -z "${bin:-}" ]]; then | |
| echo "mesh_uc7 executable not found; cargo test --no-run did not emit a binary path" >&2 | |
| exit 1 | |
| fi | |
| echo "mesh_uc7 binary: $bin" | |
| sudo -E MINIMALD_NETNS_TEST=1 GVPROXY_BIN="$PWD/.scratch/gvproxy" \ | |
| "$bin" --include-ignored --nocapture |
🤖 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 `@justfile` around lines 41 - 47, The mesh_uc7 recipe can invoke sudo with an
empty binary path if the cargo/jq pipeline returns nothing, so add an explicit
guard after resolving bin in the justfile recipe. In the recipe that sets bin
and runs "$bin", check that mesh_uc7 binary resolution succeeded and fail early
with a clear message before the sudo execution if it did not.
Comment on lines
+68
to
+80
| test-minvmd-e2e: | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| krun="{{krun_prefix}}" | ||
| common="LIBKRUN_PREFIX=$krun LD_LIBRARY_PATH=$krun MINVMD_E2E=1 \ | ||
| MINVMD_KERNEL_PATH=$PWD/.scratch/vmlinuz \ | ||
| MINVMD_ROOTFS_PATH=$PWD/.scratch/rootfs.img \ | ||
| MINVMD_INITRAMFS=$PWD/.scratch/initramfs.cpio" | ||
| sg kvm -c "env $common MINVMD_BOOT_LOG=$PWD/.scratch/boot-e2e.log \ | ||
| cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture" | ||
| sg kvm -c "env $common \ | ||
| cargo test -p minvmd --test minimald_session_e2e -- --include-ignored --nocapture --exact minimald_exec_over_bridge" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
test-minvmd-e2e should depend on setup-minvmd for first-run reliability.
On a clean repo, this recipe can fail because .scratch/vmlinuz, .scratch/rootfs.img, and .scratch/initramfs.cpio may not exist yet. Add setup-minvmd as a prerequisite so Lane B is runnable directly.
Suggested patch
-test-minvmd-e2e:
+test-minvmd-e2e: setup-minvmd
#!/usr/bin/env bash
set -euo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test-minvmd-e2e: | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| krun="{{krun_prefix}}" | |
| common="LIBKRUN_PREFIX=$krun LD_LIBRARY_PATH=$krun MINVMD_E2E=1 \ | |
| MINVMD_KERNEL_PATH=$PWD/.scratch/vmlinuz \ | |
| MINVMD_ROOTFS_PATH=$PWD/.scratch/rootfs.img \ | |
| MINVMD_INITRAMFS=$PWD/.scratch/initramfs.cpio" | |
| sg kvm -c "env $common MINVMD_BOOT_LOG=$PWD/.scratch/boot-e2e.log \ | |
| cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture" | |
| sg kvm -c "env $common \ | |
| cargo test -p minvmd --test minimald_session_e2e -- --include-ignored --nocapture --exact minimald_exec_over_bridge" | |
| test-minvmd-e2e: setup-minvmd | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| krun="{{krun_prefix}}" | |
| common="LIBKRUN_PREFIX=$krun LD_LIBRARY_PATH=$krun MINVMD_E2E=1 \ | |
| MINVMD_KERNEL_PATH=$PWD/.scratch/vmlinuz \ | |
| MINVMD_ROOTFS_PATH=$PWD/.scratch/rootfs.img \ | |
| MINVMD_INITRAMFS=$PWD/.scratch/initramfs.cpio" | |
| sg kvm -c "env $common MINVMD_BOOT_LOG=$PWD/.scratch/boot-e2e.log \ | |
| cargo test -p minvmd --test boot_e2e -- --include-ignored --nocapture" | |
| sg kvm -c "env $common \ | |
| cargo test -p minvmd --test minimald_session_e2e -- --include-ignored --nocapture --exact minimald_exec_over_bridge" |
🤖 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 `@justfile` around lines 68 - 80, The test-minvmd-e2e Just recipe assumes
.scratch artifacts already exist, so it can fail on a clean checkout. Update the
test-minvmd-e2e target in the justfile to depend on setup-minvmd before running
the two sg kvm cargo test commands, so the generated boot and rootfs files are
prepared first and the recipe is runnable directly.
norrietaylor
marked this pull request as draft
June 25, 2026 15:47
Member
Author
|
closing as won't do |
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.
Dev-ergonomics for working the #478 networking epic locally. Two independent commits, no behavior change to existing flows.
Commits
feat(minimald): add --gvproxy-bin flag— the daemon resolved the gvproxy ("gvisor-tap-vsock") switch binary only from the fixed install path/usr/lib/minimal/bin/gvproxy, so exercising the OwnIp data plane on a dev host meant installing gvproxy system-wide (sudo). Addsminimald run --gvproxy-bin <path>to point the per-host switch at any binary — e.g. one fetched byscripts/fetch-gvproxy.sh— with no system install. Unset preserves the previous default path.build: add just recipes for #478 bring-up— captures the Lane A (native netns proofs, mesh proof, networking feature suites) and Lane B (minvmd VM E2E) command sequences asjustrecipes that mirrorci-netns.ymlandci-linux-kvm.yml, so a dev reproduces each lane with one command instead of reassembling the env-var soup. Addsfetch-gvproxy,setup-minvmd,setup-kvm-group,test-netns,test-mesh-netns,test-net-features,test-minvmd-e2e, and arun-minimaldrecipe that wires the new--gvproxy-binflag to a fetched gvproxy.Verification
cargo build -p minimald— cleancargo fmt -p minimald -- --check— cleancargo clippy -p minimald --all-targets -- -D warnings— cleanjust --listparses; recipes exercised locally (just test-netns/test-net-features/test-mesh-netnsgreen;--gvproxy-binused to run a privileged dev daemon with no system gvproxy install).🤖 Generated with Claude Code
Summary by CodeRabbit