Fix fresh Linux channel provisioning - #37
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe release is bumped to 0.0.28 across package metadata, installers, channel image defaults, documentation, and tests. LXC provisioning now uses the shared ChangesRelease 0.0.28 and LXC runtime alignment
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant LXC_Runtime
participant LXC_Guest
participant Ubuntu_DNS
LXC_Runtime->>LXC_Guest: Start guest and poll default route
LXC_Guest->>Ubuntu_DNS: Resolve Ubuntu archive domains
Ubuntu_DNS-->>LXC_Guest: Return DNS resolution
LXC_Runtime->>LXC_Guest: Run apt-get update after readiness
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/site.mjs`:
- Around line 175-177: Extend the regression assertions around lxcHelper to also
reject the obsolete $LXC_ROOT/containers path, alongside the existing
lxcInstaller check. Keep the current positive LXC_PATH assertions unchanged and
use the same does-not-match behavior and descriptive message for both runtime
and installer content.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d54f5b7-11a9-48ca-95e3-597292ac51d1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdpackage.jsonsite/public/install-lxc-runtime.shsite/public/install.shsrc/server/channel-computers.tssrc/server/db.tstest/channel-computers.mjstest/site.mjs
Comment on lines
+175
to
+177
| assert.match(lxcInstaller, /LXC_PATH="\$LXC_ROOT\/machines"/, "installation and the runtime use the same LXC state directory"); | ||
| assert.match(lxcHelper, /LXC_PATH="\$LXC_ROOT\/machines"/, "the runtime reads the installer-created LXC state directory"); | ||
| assert.doesNotMatch(lxcInstaller, /LXC_ROOT\/containers/, "the obsolete mismatched LXC state directory is not installed"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Also reject obsolete paths in the runtime helper.
The test verifies that lxcHelper defines the new LXC_PATH, but only checks for $LXC_ROOT/containers in lxcInstaller. A separate hardcoded obsolete reference in the helper could therefore pass this regression test.
Suggested assertion
assert.doesNotMatch(lxcInstaller, /LXC_ROOT\/containers/, "the obsolete mismatched LXC state directory is not installed");
+ assert.doesNotMatch(lxcHelper, /LXC_ROOT\/containers/, "the runtime helper does not use the obsolete LXC state directory");📝 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
| assert.match(lxcInstaller, /LXC_PATH="\$LXC_ROOT\/machines"/, "installation and the runtime use the same LXC state directory"); | |
| assert.match(lxcHelper, /LXC_PATH="\$LXC_ROOT\/machines"/, "the runtime reads the installer-created LXC state directory"); | |
| assert.doesNotMatch(lxcInstaller, /LXC_ROOT\/containers/, "the obsolete mismatched LXC state directory is not installed"); | |
| assert.match(lxcInstaller, /LXC_PATH="\$LXC_ROOT\/machines"/, "installation and the runtime use the same LXC state directory"); | |
| assert.match(lxcHelper, /LXC_PATH="\$LXC_ROOT\/machines"/, "the runtime reads the installer-created LXC state directory"); | |
| assert.doesNotMatch(lxcInstaller, /LXC_ROOT\/containers/, "the obsolete mismatched LXC state directory is not installed"); | |
| assert.doesNotMatch(lxcHelper, /LXC_ROOT\/containers/, "the runtime helper does not use the obsolete LXC state directory"); |
🤖 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/site.mjs` around lines 175 - 177, Extend the regression assertions
around lxcHelper to also reject the obsolete $LXC_ROOT/containers path,
alongside the existing lxcInstaller check. Keep the current positive LXC_PATH
assertions unchanged and use the same does-not-match behavior and descriptive
message for both runtime and installer content.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/1helm-lxc-runtime`:
- Around line 79-100: The wait_for_guest_network function only bounds retries,
not individual lxc-attach or DNS operations. Add a per-probe timeout to each
lxc-attach invocation, including the getent DNS check, so stalled commands fail
and the loop continues toward its existing final die path without changing the
retry behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65600af4-3d78-4f22-be3e-514a0e1e2d50
📒 Files selected for processing (3)
CHANGELOG.mdscripts/1helm-lxc-runtimetest/site.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Comment on lines
+79
to
+100
| wait_for_guest_network() { | ||
| local name="$1" attempt | ||
| for attempt in {1..300}; do | ||
| if lxc-attach -P "$LXC_PATH" -n "$name" --clear-env -- /bin/sh -c ' | ||
| ip -4 -o addr show dev eth0 scope global | grep -q " inet 10\.0\.3\." \ | ||
| && ip -4 route show default | grep -q " via 10\.0\.3\.1 dev eth0" | ||
| ' >/dev/null 2>&1; then | ||
| lxc-attach -P "$LXC_PATH" -n "$name" --clear-env -- /bin/sh -c ' | ||
| rm -f /etc/resolv.conf | ||
| printf "nameserver 10.0.3.1\n" >/etc/resolv.conf | ||
| ' | ||
| if lxc-attach -P "$LXC_PATH" -n "$name" --clear-env -- /bin/sh -c ' | ||
| getent ahostsv4 archive.ubuntu.com >/dev/null \ | ||
| && getent ahostsv4 security.ubuntu.com >/dev/null | ||
| ' >/dev/null 2>&1; then | ||
| return | ||
| fi | ||
| fi | ||
| sleep 0.2 | ||
| done | ||
| die "container started but its private DHCP route and DNS did not become ready" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the lxc-attach and DNS probes, not only the loop.
The 300-iteration loop is not a hard timeout: a stuck lxc-attach or getent resolver query can block an iteration, causing installation to exceed the intended ~60-second retry window by potentially many minutes. Add a per-probe timeout and treat timeout failures as retries/final failure.
🤖 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 `@scripts/1helm-lxc-runtime` around lines 79 - 100, The wait_for_guest_network
function only bounds retries, not individual lxc-attach or DNS operations. Add a
per-probe timeout to each lxc-attach invocation, including the getent DNS check,
so stalled commands fail and the loop continues toward its existing final die
path without changing the retry behavior.
gitcommit90
changed the title
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.
Outcome
Fix fresh Linux channel creation in the superseding
0.0.28installer hotfix./var/lib/1helm-lxc/machines.FORWARD=DROPwhile retaining 1Helm-owned NAT.Verification
npm run typechecknpm run buildnpm run test:sitenode --test test/channel-computers.mjsnpm test(running locally on latest commit)RUNNING, executes asagentin/workspace, has the exact ownership marker, and reaches the Ubuntu archive over outbound HTTP.This is intentionally a Linux-only prerelease hotfix. Stable macOS and Windows feeds remain on
0.0.26.