fix(jetson): use python3 to patch daemon.json instead of sed - #1913
Conversation
The previous sed command removed the trailing comma from
'"default-runtime": "nvidia",' which produced malformed JSON
when '"runtimes"' was the next key:
{
"default-runtime": "nvidia" <- missing comma
"runtimes": { ... }
}
This caused Docker to fail to start on Jetson devices after running
the setup script.
Replace the sed one-liner with a Python snippet that parses the JSON,
removes the 'iptables' and 'bridge' keys cleanly, and writes back
valid JSON. Python3 is available on all supported Jetson/Ubuntu setups.
Fixes NVIDIA#1875
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughFor JetPack Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
![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 `@scripts/setup-jetson.sh`:
- Around line 67-69: The trailing f.write('\n') is executed after the with
open(path, 'w') as f: block (closing the file) which causes a ValueError; move
the write into the same with block so the file handle f is still open — i.e.,
ensure the sequence inside the with uses json.dump(cfg, f, indent=4) followed by
f.write('\n') (referencing the with open(path, 'w') as f, json.dump, cfg, and
f.write calls).
🪄 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: Pro Plus
Run ID: cd5d14a0-c48f-4c00-b469-95f8f613ee68
📒 Files selected for processing (1)
scripts/setup-jetson.sh
There was a problem hiding this comment.
Pull request overview
Updates Jetson JP6 host setup to patch Docker’s /etc/docker/daemon.json using Python JSON parsing (instead of sed) to avoid generating malformed JSON that prevents Docker from starting.
Changes:
- Replaces a
sedone-liner with an embeddedpython3snippet to edit/etc/docker/daemon.json. - Removes
iptablesandbridgekeys via JSON manipulation and writes the updated config back.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with open(path, 'w') as f: | ||
| json.dump(cfg, f, indent=4) |
There was a problem hiding this comment.
Writing directly to /etc/docker/daemon.json is not atomic; if the process is interrupted the file can be left truncated and Docker may fail to start. Consider writing to a temporary file in the same directory and os.replace() it into place after a successful json.dump.
| cfg.pop('bridge', None) | ||
| with open(path, 'w') as f: | ||
| json.dump(cfg, f, indent=4) | ||
| f.write('\n') |
There was a problem hiding this comment.
In the embedded Python, f.write('\n') is outside the with open(path, 'w') as f: block, so the file handle is already closed and this will raise ValueError: I/O operation on closed file (and abort the script due to set -e). Move the newline write inside the with block (or use print(..., file=f)).
| f.write('\n') | |
| f.write('\n') |
| # The previous sed approach stripped the trailing comma from | ||
| # "default-runtime": "nvidia", which produced malformed JSON when | ||
| # "runtimes" was the next key. See: https://github.com/NVIDIA/NemoClaw/issues/1875 | ||
| "${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF' | ||
| import json, sys | ||
| path = sys.argv[1] | ||
| try: | ||
| with open(path) as f: | ||
| cfg = json.load(f) | ||
| except (FileNotFoundError, json.JSONDecodeError): | ||
| cfg = {} | ||
| cfg.pop('iptables', None) | ||
| cfg.pop('bridge', None) | ||
| with open(path, 'w') as f: | ||
| json.dump(cfg, f, indent=4) | ||
| f.write('\n') |
There was a problem hiding this comment.
On json.JSONDecodeError this code sets cfg = {} and then writes it back, which will overwrite /etc/docker/daemon.json and drop any existing settings (including default-runtime/runtimes). This seems especially likely when the file is already malformed from a previous installer run. Instead, avoid overwriting on decode errors (e.g., back up and exit non-zero with a clear message) or implement a targeted repair for the known missing-comma pattern before re-parsing.
| # The previous sed approach stripped the trailing comma from | |
| # "default-runtime": "nvidia", which produced malformed JSON when | |
| # "runtimes" was the next key. See: https://github.com/NVIDIA/NemoClaw/issues/1875 | |
| "${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF' | |
| import json, sys | |
| path = sys.argv[1] | |
| try: | |
| with open(path) as f: | |
| cfg = json.load(f) | |
| except (FileNotFoundError, json.JSONDecodeError): | |
| cfg = {} | |
| cfg.pop('iptables', None) | |
| cfg.pop('bridge', None) | |
| with open(path, 'w') as f: | |
| json.dump(cfg, f, indent=4) | |
| f.write('\n') | |
| # If the file is malformed, do not overwrite it: preserve a backup and | |
| # stop with a clear error so existing Docker settings are not lost. | |
| # The previous sed approach stripped the trailing comma from | |
| # "default-runtime": "nvidia", which produced malformed JSON when | |
| # "runtimes" was the next key. See: https://github.com/NVIDIA/NemoClaw/issues/1875 | |
| "${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF' | |
| import json, shutil, sys | |
| path = sys.argv[1] | |
| try: | |
| with open(path) as f: | |
| cfg = json.load(f) | |
| except FileNotFoundError: | |
| cfg = {} | |
| except json.JSONDecodeError as exc: | |
| backup_path = path + '.bak' | |
| shutil.copy2(path, backup_path) | |
| print( | |
| f"Refusing to overwrite malformed Docker daemon config at {path}. " | |
| f"A backup was saved to {backup_path}. " | |
| f"Please repair the JSON and rerun setup. Parse error: {exc}", | |
| file=sys.stderr, | |
| ) | |
| sys.exit(1) | |
| cfg.pop('iptables', None) | |
| cfg.pop('bridge', None) | |
| with open(path, 'w') as f: | |
| json.dump(cfg, f, indent=4) | |
| f.write('\n') |
…ON repair
Three issues found in code review:
1. f.write('\n') was outside the with block — ValueError on closed file.
Moved inside the with block.
2. Direct write to daemon.json is not atomic: if interrupted, Docker gets
a truncated/empty config. Now writes to a tempfile in the same directory
and os.replace()s it into place after a successful json.dump.
3. JSONDecodeError path silently wiped the file (cfg = {}).
Now attempts to repair the known missing-comma pattern from the previous
sed approach before re-parsing. Aborts with a clear error if the file
cannot be repaired automatically.
Per CodeRabbit + Copilot review on NVIDIA#1913.
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Addressed all three review points:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup-jetson.sh (1)
86-94: File permissions not preserved after atomic replace.
tempfile.mkstempcreates files with mode0600. Afteros.replace, the newdaemon.jsonwill have restrictive permissions instead of the typical0644. While Docker (running as root) can still read it, other diagnostic tools or scripts expecting world-readable config may fail.Consider setting permissions explicitly after the replace:
🔧 Proposed fix to preserve standard permissions
os.replace(tmp, path) + os.chmod(path, 0o644) except Exception: os.unlink(tmp) raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-jetson.sh` around lines 86 - 94, The temp-file write uses tempfile.mkstemp and then os.replace(tmp, path), but mkstemp creates 0600 permissions so the final daemon.json ends up too restrictive; after the atomic replace (os.replace) explicitly set the desired permissions on the final file (e.g., os.chmod(path, 0o644)) so the deployed file is world-readable, and keep the existing exception cleanup (os.unlink(tmp)) behavior intact; locate the block using tempfile.mkstemp, os.fdopen, os.replace, tmp and path and add a post-replace os.chmod(path, 0o644).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup-jetson.sh`:
- Around line 86-94: The temp-file write uses tempfile.mkstemp and then
os.replace(tmp, path), but mkstemp creates 0600 permissions so the final
daemon.json ends up too restrictive; after the atomic replace (os.replace)
explicitly set the desired permissions on the final file (e.g., os.chmod(path,
0o644)) so the deployed file is world-readable, and keep the existing exception
cleanup (os.unlink(tmp)) behavior intact; locate the block using
tempfile.mkstemp, os.fdopen, os.replace, tmp and path and add a post-replace
os.chmod(path, 0o644).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7595b2e-8345-408f-9cc5-52a74c5e699a
📒 Files selected for processing (1)
scripts/setup-jetson.sh
tempfile.mkstemp creates files with mode 0600. After os.replace the new daemon.json had restrictive permissions instead of the typical 0644, breaking diagnostic tools expecting world-readable config. Now copies permissions from the original file (or falls back to 0644). Per CodeRabbit review on NVIDIA#1913. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
2b136c6 to
f2c102b
Compare
|
Fixed — now preserves the original file's permissions ( |
After os.replace(), the new daemon.json should keep the original file mode (typically 0644). Restore the mode explicitly so the file remains readable for diagnostics and matches the previous behavior. Per CodeRabbit review on NVIDIA#1913. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Fixed — restored the original file mode after the atomic replace so |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/setup-jetson.sh (1)
57-57: Add an explicitpython3preflight before invoking the inline patcher.Line 57 executes
python3directly in the jp6 path. Ifpython3is unavailable in the sudo execution context, setup aborts with an unclear error. An explicit pre-check improves debuggability and failure clarity.♻️ Suggested patch
jp6) + "${SUDO[@]}" python3 --version >/dev/null 2>&1 || \ + error "python3 is required to patch /etc/docker/daemon.json" "${SUDO[@]}" update-alternatives --set iptables /usr/sbin/iptables-legacy # Patch /etc/docker/daemon.json using Python to avoid generating invalid JSON. # The previous sed approach stripped the trailing comma from🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-jetson.sh` at line 57, The inline Python patch invocation using "${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF' can fail silently if python3 is not available in the sudo execution context; add an explicit preflight check that verifies python3 is present and executable under the same sudo context before running the here-doc. Concretely, before the existing python3 here-doc (the line containing "${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF'), run a sudo-context check like invoking "${SUDO[@]}" to test command -v or which for python3 and fail with a clear error message and non-zero exit if missing so the setup aborts with a useful diagnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-jetson.sh`:
- Around line 82-83: The script assumes cfg is a dict when calling
cfg.pop('iptables', None) and cfg.pop('bridge', None); first check that cfg is a
mapping (e.g., isinstance(cfg, dict) or using a shell/json tool check) and if it
is not, emit a clear error and exit (or replace with an empty object) before
attempting to mutate keys; update the code around the cfg variable and the two
pop calls to validate the parsed JSON root type and handle non-object roots
gracefully (log a concise error and exit 1 or initialize cfg = {}), referencing
the cfg variable and the two pop usages to locate the change.
---
Nitpick comments:
In `@scripts/setup-jetson.sh`:
- Line 57: The inline Python patch invocation using "${SUDO[@]}" python3 -
/etc/docker/daemon.json <<'PYEOF' can fail silently if python3 is not available
in the sudo execution context; add an explicit preflight check that verifies
python3 is present and executable under the same sudo context before running the
here-doc. Concretely, before the existing python3 here-doc (the line containing
"${SUDO[@]}" python3 - /etc/docker/daemon.json <<'PYEOF'), run a sudo-context
check like invoking "${SUDO[@]}" to test command -v or which for python3 and
fail with a clear error message and non-zero exit if missing so the setup aborts
with a useful diagnostic.
🪄 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: Pro Plus
Run ID: 38672a67-2d0b-4958-bd5c-fd0760b6adbb
📒 Files selected for processing (1)
scripts/setup-jetson.sh
Two defensive improvements: - Add explicit python3 preflight before invoking the inline patcher, so missing python3 produces a clear error message instead of an opaque 'command not found' abort - Guard against non-dict JSON root in daemon.json with a clear sys.exit message before calling .pop() Per CodeRabbit review on NVIDIA#1913. Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
6175d8a to
75022f3
Compare
|
Addressed both new review points:
|
|
@BenediktSchackenberg CI failing on formatting — the pre-commit hooks (shfmt) modified the file. Run: npx prek run --all-files
git add -A && git commit -m "style: apply shfmt formatting"
git pushCode itself looks good — tested the auto-repair logic locally and it handles broken, valid, and garbage daemon.json files correctly. Ready to merge once CI is green. |
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
a2bd628 to
1d5f8dd
Compare
|
The |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Final gate pass on #1913 looks good. I reviewed the final installer diff and the new regression tests: the Python patcher now repairs the known missing-comma daemon.json case without silently clobbering unrecoverable files, preserves file mode across the atomic replace, and covers the malformed-input, non-object-root, and missing-file paths. CI is green and the branch is mergeable, so this is approved.
Align setup-jetson.sh with main's atomic python3 patcher so that test/setup-jetson.test.ts can extract and validate the inline script.
Bring feat/jetson-orin-nano-support up to date with origin/main after 217 commits of upstream work. Conflict resolutions: - scripts/setup-jetson.sh: adopt main's L4T-version-aware structure (NVIDIA#1910, NVIDIA#1913, NVIDIA#2419) — JP6 / JP7-R38 / JP7-R39 case dispatch with idempotent br_netfilter persistence. Drops the PR's preflight wrapper in favor of the simpler main rewrite that already covers Orin Nano via the JP6 path. - src/lib/onboard.ts: keep main's verifyWebSearchInsideSandbox; preserve the Jetson GPU-detection branch on top of main's refined NVIDIA detection format; restore the gpu parameter on startGatewayWithOptions so the patchGatewayImageForJetson() call still type-checks. - src/lib/onboard-providers.ts: re-add the ollama-local / vllm-local branches in getSandboxInferenceConfig that override inferenceBaseUrl via getLocalProviderBaseUrl. - docs/get-started/quickstart.md and the autogenerated skills commands.md: take main (Jetson now lives in ci/platform-matrix.json). Port setup-jetson into main's oclif command-registry architecture: - Add SetupJetsonCliCommand thin oclif adapter and runSetupJetsonAction that shells out to scripts/setup-jetson.sh under sudo on Linux. - Register setup-jetson in command-registry.ts (Getting Started), legacy-oclif-dispatch.ts, and oclif-commands.ts. Switch patchGatewayImageForJetson to docker.dockerInspectFormat / docker.dockerRun so it stays inside main's docker-abstraction guard, and update the existing jetson tests to match the argv-based runner contract. Fix the Jetson note in ci/platform-matrix.json — the relative ../reference/commands.md#nemoclaw-setup-jetson link broke the markdown-links CI check when the platform-matrix generator copied it into root README.md. Replaced with the docs.nvidia.com absolute URL. Allowlist two pre-existing slack-app-token false positives surfaced by the local pre-commit gitleaks pass. Verified: npm run build:cli, npm run lint, full vitest suite (3160 pass, 13 skip), and check-docs.sh markdown-link check all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wscurran
added
area: packaging
Problem
The
setup-jetson.shscript usedsedto patch/etc/docker/daemon.jsonon JP6 devices. The sed command stripped the trailing comma from"default-runtime": "nvidia",:This produced invalid JSON when
"runtimes"was the next key:{ "default-runtime": "nvidia" ← missing comma here "runtimes": { "nvidia": { ... } } }Docker rejects malformed JSON and fails to start, causing
Job for docker.service failederrors.Fix
Replace the sed one-liner with a Python3 snippet that parses the JSON properly, removes the
iptablesandbridgekeys cleanly, and writes back valid JSON. Python3 is available on all supported Jetson/Ubuntu setups.Fixes #1875
Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com
Summary by CodeRabbit