Add configurable cache bind for chroot #8973
Conversation
WalkthroughAdds conditional host-cache bind-mounting to chroot helpers: Changes
Sequence Diagram(s)sequenceDiagram
participant Host
participant Script as mount_chroot / umount_chroot
participant TargetFS as target (chroot)
Host->>Script: optional ARMBIAN_CACHE_DIR env
Script->>Host: check host cache exists
alt host cache exists
Script->>TargetFS: ensure target/armbian/cache directory exists
Script->>TargetFS: mount --bind host_cache -> target/armbian/cache
Note right of Script: record mount in stack for cleanup
else host cache missing
Script->>Host: log warning / skip bind
end
Script->>TargetFS: perform other guarded mounts (tmp,var,tmp,run/user/0,proc,sys,dev,dev/pts)
Note right of Script: on error -> unmount recorded mounts in reverse order
Script->>TargetFS: umount target/armbian/cache (if recorded/mounted)
Script->>TargetFS: continue existing unmount sequence
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-10-24T04:46:22.901ZApplied to files:
📚 Learning: 2025-06-12T21:03:39.686ZApplied to files:
📚 Learning: 2025-07-21T04:12:02.439ZApplied to files:
🔇 Additional comments (6)
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 |
github-actions
bot
added
11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/functions/general/chroot-helpers.sh (1)
1-41: Document ARMBIAN_CACHE_DIR and add error handling for filesystem mounts.The variable
ARMBIAN_CACHE_DIRis not documented anywhere in the codebase—it only appears in the implementation. The cache mount itself is well-implemented (checks directory existence, validates already-bound mount points, logs warnings), but the early mounts lack error handling: tmpfs, proc, sysfs, and/devbind mounts have no checks and will fail silently if permissions are denied or the target is read-only.
- Add ARMBIAN_CACHE_DIR to documentation (config/README.md or similar) with the default value and use case.
- Add error handling to the early mounts in
mount_chroot(lines 21–27), since they're prerequisite operations that determine success or failure of the entire chroot setup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/functions/general/chroot-helpers.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/general/chroot-helpers.sh (2)
lib/functions/image/initrd.sh (1)
update_initramfs(23-133)lib/functions/rootfs/rootfs-create.sh (1)
create_new_rootfs_cache_via_debootstrap(54-365)
🔇 Additional comments (1)
lib/functions/general/chroot-helpers.sh (1)
49-56: Cache unmount ordering is correct; no issues identified.Verification of call sites and the
umount_chrootfunction confirms:
- Cache unmount (lines 54–56) correctly executes before the proc/sys/dev/tmp/run cleanup loop (lines 58–69)
- All call sites properly pair
mount_chrootwithumount_chroot- Error handling with
|| trueallows partial failures without stopping cleanup- The implementation maintains proper mount/unmount semantics
| cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}" | ||
| if mountpoint -q "${target}/armbian/cache"; then | ||
| umount "${target}/armbian/cache" || true | ||
| fi |
There was a problem hiding this comment.
Remove unused variable assignment.
Line 53 declares and assigns cache_src, but it is never referenced. The unmount logic on lines 54–56 works correctly without it.
Apply this diff to remove the unused variable:
- cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}"
- if mountpoint -q "${target}/armbian/cache"; then
+ if mountpoint -q "${target}/armbian/cache"; then📝 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.
| cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}" | |
| if mountpoint -q "${target}/armbian/cache"; then | |
| umount "${target}/armbian/cache" || true | |
| fi | |
| if mountpoint -q "${target}/armbian/cache"; then | |
| umount "${target}/armbian/cache" || true | |
| fi |
🤖 Prompt for AI Agents
lib/functions/general/chroot-helpers.sh around lines 53 to 56: remove the unused
variable assignment "cache_src=\"${ARMBIAN_CACHE_DIR:-/armbian/cache}\"" on line
53 since it is never referenced; leave the subsequent mountpoint check and
umount logic intact and ensure no other code in the file expects that variable
before committing the change.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
igorpecovnik
added
02
benhoff
requested review from
EvilOlaf,
TRSx80,
igorpecovnik and
littlecxm
as code owners
github-actions
bot
added
size/medium
|
Tip For best results, initiate chat on the files or code changes. @benhoff Hello! How can I assist you with this PR? I can help with:
Just let me know what you'd like me to do! |
|
@coderabbitai Review the code changes in detail |
🧩 Analysis chain🏁 Script executed: Length of output: 5402 🏁 Script executed: Length of output: 135 @benhoff I'll provide a detailed review of the code changes. Overall AssessmentThe implementation adds useful mount tracking and error handling to the chroot mounting process, plus configurable cache binding. However, there are several issues that need to be addressed. Critical Issues1. Cache mount not tracked in cleanup logicLocation: Lines 90-98 in The cache bind mount is performed after all other mounts, but it's not added to the
Recommendation: Add the cache mount to the tracking array: if ! mount --bind "${cache_src}" "${target}/armbian/cache"; then
display_alert "Cache bind failed" "${cache_src} -> ${target}/armbian/cache" "warn"
else
mounted_points+=("${target}/armbian/cache")
fi2. Nested cleanup function scope issueLocation: Lines 20-26 The
Recommendation: Consider whether cleanup should be more symmetrical with unmount, or document why they differ. 3. No validation of ARMBIAN_CACHE_DIRLocation: Line 90 The code uses
Recommendation: Add validation: cache_src="${ARMBIAN_CACHE_DIR:-/armbian/cache}"
# Validate cache_src is an absolute path
if [[ -n "${cache_src}" && "${cache_src}" != /* ]]; then
display_alert "ARMBIAN_CACHE_DIR must be an absolute path" "${cache_src}" "err"
cache_src="" # Disable cache binding
fiModerate Issues4. Race condition with mountpoint checksLocation: Lines 33, 40, 47, 54, 61, 68, 75 The pattern Recommendation: Consider logging when skipping an already-mounted path: if ! mountpoint -q "${target}/tmp"; then
if ! mount -t tmpfs -o "size=99%" tmpfs "${target}/tmp"; then
display_alert "Failed to mount tmpfs inside chroot" "${target}/tmp" "err"
cleanup_mounted_points
return 1
fi
mounted_points+=("${target}/tmp")
else
display_alert "Already mounted — skipping" "${target}/tmp" "debug"
fi5. Inconsistent error handling for cache bindLocation: Lines 91-98 Cache directory creation and bind mount failures are logged as "warn" but don't cause function failure, while all other mount failures cause immediate return. This inconsistency should be intentional and documented. Recommendation: Add a comment explaining why cache bind failures are non-fatal: # Bind host cache into chroot if present (configurable via ARMBIAN_CACHE_DIR)
# Note: cache bind is optional — failures here are non-fatal6. umount_chroot doesn't respect mount trackingLocation: Lines 115-116 The unmount of the cache is hardcoded, while the original mounts use a while loop with grep. This could lead to:
Recommendation: Consider whether Minor Issues7. mkdir -p error handlingLocation: Lines 27-30, 93-94 The error message "Failed to prepare chroot runtime directory" could be more specific about whether it's a permissions issue, disk full, etc. Recommendation: Consider capturing and displaying the actual error: if ! mkdir -p "${target}/run/user/0" 2>&1 | display_alert "Failed to prepare chroot runtime directory" "${target}/run/user/0" "err"; then
return 1
fi8. Default cache path assumptionLocation: Line 90, config/README.md line 5 The default Current documentation is good but could add a note: "Ensure the directory exists and is writable before building." Positive Aspects
Testing RecommendationsGiven you mentioned not having bare-metal for testing:
Security Considerations
Would you like me to elaborate on any of these points or help with generating test cases? ✅ Actions performedReview triggered.
|
3 participants
Description
This reimplements this pull request, while fixing the edge case (hopefully). #8186
I don't have a bare metal device to test.
How Has This Been Tested?
I haven't checked this yet, I'll run it with a Qt buildout that I have
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.