fix: replace mount -o loop with mkfs.ext4 -d for rootfs creation#44
fix: replace mount -o loop with mkfs.ext4 -d for rootfs creation#44
Conversation
Use `mkfs.ext4 -d` to populate ext4 images directly from a directory instead of mount -o loop + cp/tar + umount. This removes the dependency on loop devices, fixing installation on containerized environments (Docker, LXC, many VPS providers) where /dev/loop* is unavailable. Should solve #42
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces loopback mount + tar-into-mounted-fs workflow with creating ext4 images seeded from an extracted directory via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/create/image-rootfs.ts`:
- Around line 166-174: The temp extraction directory tmpExtract is created and
removed inside the same try block so if execSync calls for mkfs.ext4 or tune2fs
throw, tmpExtract is left behind; refactor so tmpExtract is declared at function
scope and move the rm -rf "${tmpExtract}" cleanup into a finally block (or add a
try/finally around the mkdirSync, tar -xf, mkfs.ext4, tune2fs sequence) so that
regardless of exceptions from mkfs.ext4 or tune2fs (the execSync calls
referencing tmpTar, ext4Path, imageSizeMb) the temporary directory is always
removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dc7eae0-1b16-4241-a90b-38e9a7fda9c2
📒 Files selected for processing (2)
install.shsrc/commands/create/image-rootfs.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Line 771: Replace the current trap that only listens for RETURN with one that
runs on shell exit/errors so cleanup always occurs; specifically change the trap
invocation that references container_name and build_dir (the line with trap '...
RETURN') so it uses EXIT or ERR (or both) instead of RETURN, ensuring the same
cleanup body runs when error() (the function that calls exit 1) is invoked after
failing docker build/create/export; keep the existing commands (docker rm -f
"$container_name" ... || true; rm -rf "$build_dir") intact and ensure the trap
is set early enough to cover failures.
- Around line 454-463: Move Docker readiness checks into build_runtime() and
stop performing an unconditional ensure_docker() at script start: remove the
top-level ensure_docker() call and instead, after the runtime_metadata_matches()
short-circuit in build_runtime(), replace the existing need_cmd docker check
with a daemon probe using docker info >/dev/null 2>&1; if that probe fails log a
warning (use warn "Docker daemon is unavailable — skipping runtime ${name}") and
return to skip building that runtime rather than hard-failing. Keep
ensure_docker() only if other non-runtime code truly requires installing Docker,
otherwise remove or leave unused.
| ensure_docker() { | ||
| if command -v docker >/dev/null 2>&1; then | ||
| return | ||
| fi | ||
|
|
||
| info "Docker not found — installing Docker via get.docker.com..." | ||
| curl -fsSL https://get.docker.com | sh | ||
| command -v docker >/dev/null 2>&1 || error "Docker installation failed" | ||
| success "Docker $(docker --version | awk '{print $3}' | tr -d ',') installed" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'ensure_docker|runtime_metadata_matches|need_cmd docker|docker info' install.shRepository: angelorc/vmsan
Length of output: 946
🏁 Script executed:
# Find the need_cmd function definition
rg -n "^need_cmd" install.sh -A 5Repository: angelorc/vmsan
Length of output: 402
🏁 Script executed:
# Get full context around build_runtime function (lines 750-770)
sed -n '745,775p' install.shRepository: angelorc/vmsan
Length of output: 931
🏁 Script executed:
# Get context around the ensure_docker call site (lines 855-865)
sed -n '850,870p' install.shRepository: angelorc/vmsan
Length of output: 591
Move the Docker readiness check inside build_runtime() and verify daemon availability.
Line 858 calls ensure_docker unconditionally, so even a re-run with all three runtime images already cached will still install or require Docker. Additionally, ensure_docker() (lines 455-457) only verifies the docker CLI is present via command -v, not whether the daemon is reachable. A host with docker on PATH but no active daemon will pass this check, then fail when build_runtime() attempts to execute docker build or docker create.
Move the Docker readiness check inside build_runtime() after the runtime_metadata_matches() short-circuit at line 753, and verify daemon availability with docker info instead of command -v docker alone. If the daemon is unavailable, skip that runtime rather than hard-failing, since the script should aim to be resilient when Docker is absent.
Suggested approach
- Remove the unconditional
ensure_dockercall at line 858 - Replace
need_cmd dockerat line 763 with:docker info >/dev/null 2>&1 || { warn "Docker daemon is unavailable — skipping runtime ${name}" return }
- Consider keeping a top-level
ensure_dockercall only if non-runtime code depends on Docker, or remove it entirely if not needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 454 - 463, Move Docker readiness checks into
build_runtime() and stop performing an unconditional ensure_docker() at script
start: remove the top-level ensure_docker() call and instead, after the
runtime_metadata_matches() short-circuit in build_runtime(), replace the
existing need_cmd docker check with a daemon probe using docker info >/dev/null
2>&1; if that probe fails log a warning (use warn "Docker daemon is unavailable
— skipping runtime ${name}") and return to skip building that runtime rather
than hard-failing. Keep ensure_docker() only if other non-runtime code truly
requires installing Docker, otherwise remove or leave unused.
| local build_tag="vmsan-rootfs-${name}:latest" | ||
| local container_name="vmsan-export-${name}-$$" | ||
| trap 'trap - RETURN; grep -qs " $mnt_dir " /proc/mounts && umount "$mnt_dir" >/dev/null 2>&1 || true; docker rm -f "$container_name" >/dev/null 2>&1 || true; rm -rf "$build_dir"' RETURN | ||
| trap 'trap - RETURN; docker rm -f "$container_name" >/dev/null 2>&1 || true; rm -rf "$build_dir"' RETURN |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
logfile="$(mktemp)"
bash -c '
set -euo pipefail
f() {
local d="$1" log="$2"
trap '\''printf "RETURN fired\n" >> "$log"; rm -rf "$d"'\'' RETURN
exit 1
}
f "'"$tmpdir"'" "'"$logfile"'"
' || true
printf 'tmpdir_exists=%s\n' "$( [ -d "$tmpdir" ] && echo yes || echo no )"
printf 'trap_log=%s\n' "$(cat "$logfile" 2>/dev/null || true)"
rm -rf "$tmpdir" "$logfile"Repository: angelorc/vmsan
Length of output: 85
🏁 Script executed:
sed -n '25,35p' install.shRepository: angelorc/vmsan
Length of output: 420
🏁 Script executed:
sed -n '765,780p' install.shRepository: angelorc/vmsan
Length of output: 823
🏁 Script executed:
sed -n '710,750p' install.shRepository: angelorc/vmsan
Length of output: 1004
🏁 Script executed:
# Find the entire build_runtime function and check for error() calls
sed -n '742,900p' install.sh | head -150Repository: angelorc/vmsan
Length of output: 5612
🏁 Script executed:
# Search for error( calls in the build_runtime function area
grep -n 'error(' install.sh | grep -A5 -B5 '74[0-9]\|75[0-9]\|76[0-9]\|77[0-9]\|78[0-9]\|79[0-9]\|8[0-1][0-9]'Repository: angelorc/vmsan
Length of output: 40
Line 771's cleanup trap bypasses on error() paths.
This trap only fires on function return via RETURN. When docker build, docker create, or docker export fail and call error() (line 30, which does exit 1), the trap never runs, leaving both "$build_dir" and "$container_name" behind. Use EXIT or ERR trap to catch shell exits and error conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` at line 771, Replace the current trap that only listens for
RETURN with one that runs on shell exit/errors so cleanup always occurs;
specifically change the trap invocation that references container_name and
build_dir (the line with trap '... RETURN') so it uses EXIT or ERR (or both)
instead of RETURN, ensuring the same cleanup body runs when error() (the
function that calls exit 1) is invoked after failing docker build/create/export;
keep the existing commands (docker rm -f "$container_name" ... || true; rm -rf
"$build_dir") intact and ensure the trap is set early enough to cover failures.
1 participant
Summary
mount -o loop+cp/tar+umountwithmkfs.ext4 -dto populate ext4 images directly from a directory/dev/loop*), which are unavailable in containerized environments (Docker, LXC, many VPS providers)src/commands/create/image-rootfs.tsSolve #42
Test plan
vmsan create --from-image node:22and verify the ext4 rootfs is built correctlySummary by CodeRabbit