docker: add automatic image pull cronjob and cleanup system #9265
Conversation
📝 WalkthroughWalkthroughThe changes introduce automated Docker image management functionality, including periodic cleanup of dangling images, image pulling with timestamp tracking, and a cron-based auto-pull system that refreshes specified Docker images every 12 hours with hash-based change detection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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
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
size/medium
igorpecovnik
added
Work in progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@lib/functions/host/docker.sh`:
- Around line 795-801: The header in cron_content includes a dynamic $(date)
which invalidates the content hash every run; update the here-doc assigned to
cron_content (and references to wrapper_script inside it) to remove the $(date)
substitution and use a static, deterministic header (or a build/version
constant) instead so the generated cron file is stable and hash checks can
succeed—i.e., edit the cron_content here-doc to omit $(date) and keep the rest
of the lines intact.
- Around line 738-771: The script sets errexit but not pipefail, so failures
inside the pipeline in pull_with_marker (the `docker pull ... | logger -t
armbian-docker-pull` pipeline) can be masked; to fix, add "set -o pipefail"
immediately after the existing "set -e" near the top of the script (so the
script uses both errexit and pipefail), ensuring pull_with_marker and other
pipeline commands correctly propagate docker pull failures and prevent updating
the MARKER_DIR/last-pull marker on failed pulls.
- Around line 655-679: The cleanup commands in the Docker cleanup loop (the
docker image prune invocation near display_alert "Pruning dangling images" and
the docker rmi calls in the loop that removes images for each image_tag using
array image_ids) can fail and abort the script when set -e is enabled; make
these removal commands best‑effort by appending a non-fatal fallback (e.g., ||
true) to both the docker image prune invocation and the docker rmi calls invoked
for each image_ids entry so failures do not stop the build while retaining the
existing display_alert/debug logging and the image_tags/image_ids logic.
- Around line 718-724: The docker_setup_auto_pull_cronjob function currently
assumes it can write to cron and bin locations; change it to fail fast by
validating prerequisites before attempting writes: check that /etc/cron.d is
writable (and exists), /usr/local/bin is writable (and exists), and
/var/lib/armbian (or the directory for hash_file) is writable, and verify that a
cron executor (crontab/cron service) is available; if any check fails, exit
immediately with a clear error message via stderr (include which path or
dependency failed) instead of attempting the operation. Use the existing symbols
cron_file, wrapper_script, and hash_file in the validation and error messages so
the failure context is obvious, and perform these checks at the start of
docker_setup_auto_pull_cronjob before creating or writing any files.
148f276 to
ba3f94e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/functions/host/docker.sh`:
- Around line 821-843: The sudo-backed file operations (echo | sudo tee, sudo
chmod, sudo mkdir -p) performed when needs_update == "yes" can abort if sudo is
unavailable or prompts for a password; update the block to first validate
non-interactive sudo access (e.g., test sudo -n true) and if it fails, either
perform non-fatal fallbacks (attempt writes without sudo when permissions allow)
or skip those specific steps while calling display_alert with a clear "warn"
message; ensure wrappers referenced (wrapper_script, cron_file, hash_file) and
inputs (wrapper_content, cron_content, current_hash) are handled idempotently
and that failures of sudo commands do not cause the script to exit but instead
log warnings and continue.
♻️ Duplicate comments (3)
lib/functions/host/docker.sh (3)
654-684: Make cleanup commands best-effort to avoid aborting builds.With
set -eenabled globally,docker image pruneordocker rmifailures will abort the build despite cleanup being non-critical. Images may be in use by running containers or have other locks preventing removal.Proposed fix
# Remove dangling images (layers with no tags) display_alert "Pruning dangling images" "docker image prune -f" "debug" - docker image prune -f > /dev/null 2>&1 + docker image prune -f > /dev/null 2>&1 || true # For each armbian image tag, keep only the 2 most recent ... for ((i=2; i<${`#image_ids`[@]}; i++)); do display_alert "Removing old image" "${image_tag}:${image_ids[$i]}" "debug" - docker rmi "${image_ids[$i]}" > /dev/null 2>&1 + docker rmi "${image_ids[$i]}" > /dev/null 2>&1 || true done
718-727: Add prerequisite check for cron availability.The function attempts to write to
/etc/cron.dwithout validating the directory exists. Per Armbian's fail-fast philosophy for host dependencies, add an explicit check that provides a clear error message rather than potentially confusing sudo permission errors.Proposed fix
function docker_setup_auto_pull_cronjob() { + # Validate prerequisites - fail fast with clear error + if [[ ! -d /etc/cron.d ]]; then + display_alert "Docker auto-pull cronjob" "cron not available (/etc/cron.d missing), skipping" "debug" + return 0 + fi + declare cron_file="/etc/cron.d/armbian-docker-pull" declare wrapper_script="/usr/local/bin/armbian-docker-pull"Also note that the hardcoded
images_to_pullarray may not match the user's configuredDOCKER_ARMBIAN_BASE_IMAGE. Consider using the actual base image variable or documenting this intentional disconnect.
737-738: Addset -o pipefailto prevent masking docker pull failures.The wrapper script pipes
docker pulloutput tologger(line 760), but withoutpipefail, the pipeline returns logger's exit status rather than docker's. This causes failed pulls to appear successful and incorrectly update the marker file.Proposed fix
set -e +set -o pipefail SRC="__SRC_PLACEHOLDER__"
🧹 Nitpick comments (3)
lib/functions/host/docker.sh (3)
157-163: Consider making these calls non-fatal to avoid interrupting builds.Both
docker_cleanup_old_imagesanddocker_ensure_auto_pull_cronjobperform non-critical housekeeping. Withset -eactive globally, any failure (permission issues, missing cron, etc.) will abort the entire build. Since image cleanup and cron setup are convenience features, consider wrapping these calls to be best-effort:############################################################################################################# # Cleanup old Docker images to free disk space - docker_cleanup_old_images + docker_cleanup_old_images || display_alert "Docker cleanup" "skipped (non-fatal)" "debug" ############################################################################################################# # Ensure auto-pull cronjob is installed to keep Docker images fresh - docker_ensure_auto_pull_cronjob + docker_ensure_auto_pull_cronjob || display_alert "Docker auto-pull cronjob" "skipped (non-fatal)" "debug"
760-772: Pipeline may mask docker pull failures even with pipefail.The
docker pull ... 2>&1 | loggerpipeline has an additional issue: the if-check tests the pipeline exit status, but even withpipefail, the logic flow needs adjustment. The current code assumes the pipeline exit reflects docker's status, but the success/failure paths should be verified.Consider restructuring to capture docker pull's exit status explicitly:
Alternative approach
pull_with_marker() { local image_name="$1" log "Pulling Docker image: ${image_name}" - if docker pull "${image_name}" 2>&1 | logger -t armbian-docker-pull; then + local pull_output pull_status=0 + pull_output="$(docker pull "${image_name}" 2>&1)" || pull_status=$? + echo "${pull_output}" | logger -t armbian-docker-pull + + if [[ ${pull_status} -eq 0 ]]; then # Update marker file after successful pull
846-859: Simplify redundant conditional logic.The if/else block always calls
docker_setup_auto_pull_cronjobregardless of which branch is taken. The only difference is the display_alert message. Consider simplifying:Proposed simplification
function docker_ensure_auto_pull_cronjob() { declare wrapper_script="/usr/local/bin/armbian-docker-pull" declare hash_file="/var/lib/armbian/docker-pull.hash" - # Always call docker_setup_auto_pull_cronjob - it will check hashes and only update if needed - if [[ ! -f "${wrapper_script}" ]] || [[ ! -f "${hash_file}" ]]; then + if [[ ! -f "${wrapper_script}" ]] || [[ ! -f "${hash_file}" ]]; then display_alert "Docker auto-pull cronjob" "wrapper or hash file missing, installing now" "info" - docker_setup_auto_pull_cronjob - else - # Still call setup to check for updates via hash comparison - docker_setup_auto_pull_cronjob fi + + # Always call setup - it will check hashes and only update if needed + docker_setup_auto_pull_cronjob }
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
3 times, most recently
from
8f8ac10 to
bdfc4c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/functions/host/docker.sh`:
- Around line 668-673: The current pipeline into image_ids uses sort -k2 -r
which only sorts by the first token of the CreatedAt string; change the sort to
key on the full CreatedAt field range so the most-recent image is reliably
first. Update the pipeline that builds image_ids (the docker images --format
'{{.ID}} {{.CreatedAt}}' ... | sort -k2 -r | awk '{print $1}') to use a
range-based sort such as sort -r -k2,5 (or equivalent range covering the entire
CreatedAt tokens) before awk so the full timestamp is considered when selecting
newest images.
♻️ Duplicate comments (1)
lib/functions/host/docker.sh (1)
718-836: Fail fast when cron/sudo prerequisites are missing.
This path assumes writable/etc/cron.d,/usr/local/bin, and non-interactive sudo. Without preflight checks, failures surface late and with unclear errors. Based on learnings, fail-fast dependency checks are preferred in this codebase.Suggested preflight checks
function docker_setup_auto_pull_cronjob() { declare cron_file="/etc/cron.d/armbian-docker-pull" declare wrapper_script="/usr/local/bin/armbian-docker-pull" declare hash_file="/var/lib/armbian/docker-pull.hash" + + if [[ ! -d /etc/cron.d ]]; then + exit_with_error "Docker auto-pull cronjob" "cron not available; /etc/cron.d is missing" + fi + if ! sudo -n true 2>/dev/null; then + exit_with_error "Docker auto-pull cronjob" "non-interactive sudo required to write ${cron_file}, ${wrapper_script}, and ${hash_file}" + fi
🧹 Nitpick comments (2)
lib/functions/host/docker.sh (2)
701-707: Avoid unbounded growth of the marker file.
You only need the latest pull metadata for mtime checks; appending will grow indefinitely.Suggested update
- echo "${image_name}|${local_image_sha}|$(date +%s)" >> "${docker_marker_dir}"/last-pull + echo "${image_name}|${local_image_sha}|$(date +%s)" > "${docker_marker_dir}"/last-pull
847-859: Simplify the redundant branch.
Both branches calldocker_setup_auto_pull_cronjob; keep the log but avoid duplication.Suggested update
# Always call docker_setup_auto_pull_cronjob - it will check hashes and only update if needed if [[ ! -f "${wrapper_script}" ]] || [[ ! -f "${hash_file}" ]]; then display_alert "Docker auto-pull cronjob" "wrapper or hash file missing, installing now" "info" - docker_setup_auto_pull_cronjob - else - # Still call setup to check for updates via hash comparison - docker_setup_auto_pull_cronjob fi + docker_setup_auto_pull_cronjob
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
from
bdfc4c0 to
3722cea
Compare
|
@coderabbitai Review |
✅ Actions performedReview triggered.
|
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
2 times, most recently
from
0f424d7 to
c03c354
Compare
igorpecovnik
added
Ready to merge
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
from
c03c354 to
e0d5358
Compare
github-actions
bot
added
Needs review
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
from
e0d5358 to
9279fbc
Compare
|
This is interesting. I'd avoid touching regular (human) users crontabs, they might be surprised. In that sense maybe only setup the cron if running under GHA or explicitly asked for? Another aspect of the "slow pulling" is the Docker image itself: it has like some 10 layers, but one is about 2gb (the one that has the big |
|
Oh wow, didn't even realize that this would setup a cronjob. I oppose. This should be an extension or opt-in. |
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
from
9279fbc to
6ff54d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/functions/cli/cli-requirements.sh`:
- Around line 49-55: The script currently calls docker_ensure_auto_pull_cronjob
whenever docker exists, which unconditionally installs system cronjobs; change
this to require an explicit opt-in by checking an environment flag (e.g.,
ARMBIAN_DOCKER_AUTO_PULL=yes) before calling docker_ensure_auto_pull_cronjob,
following the established pattern used for INSTALL_ARMBIAN_FIRMWARE; update the
conditional that wraps the docker_ensure_auto_pull_cronjob invocation to also
test the ARMBIAN_DOCKER_AUTO_PULL variable (and document the flag) so the
function only runs when the user has explicitly enabled auto-pull.
In `@lib/functions/host/docker.sh`:
- Around line 792-802: The cron entry is using $(whoami) which becomes root when
run under sudo; update the code that builds cron_content (variable cron_content
and the referenced wrapper_script) to use the SUDO_USER environment variable
(falling back to $(whoami) only if SUDO_USER is empty) so the cron job runs as
the original invoking user rather than root; locate the block that appends "0
*/12 * * * $(whoami) ${wrapper_script} ..." and replace the user portion with
something like "${SUDO_USER:-$(whoami)}" to preserve the invoking user context.
|
Fair points. Changed to middle ground and adding under: Next option is going completly stealth under a new variable. Edit: switched to out-in. |
- Add docker_cleanup_old_images() to remove dangling images and keep only 2 most recent per tag - Add docker_pull_with_marker() to pull images and update marker files tracking last pull time - Add docker_setup_auto_pull_cronjob() to create/update system cronjob and wrapper script via hash-based detection - Add docker_ensure_auto_pull_cronjob() to ensure cronjob is installed and up-to-date - Create self-contained wrapper script at /usr/local/bin/armbian-docker-pull for cron execution - Store configuration hash in /var/lib/armbian/docker-pull.hash for smart update detection - Install cronjob at /etc/cron.d/armbian-docker-pull to pull images every 12 hours - Move cronjob setup from docker_cli_prepare() to requirements command - Cronjob is now only installed when users explicitly run ./compile.sh requirements - Prevents "12 hours since last pull, pulling again" delay during builds Signed-off-by: Igor Pecovnik <[email protected]>
igorpecovnik
force-pushed
the
feature/docker-auto-pull-cronjob
branch
from
6ff54d6 to
04b4d80
Compare
- Add ARMBIAN_DOCKER_AUTO_PULL environment variable (opt-in, must be explicitly set to "yes") - Move auto-pull cronjob setup from requirements to docker CLI - Add automatic cleanup of cronjob files when flag is disabled/removed - Remove verbose "unchanged" messages for cleaner output - Simplify control flow in docker_ensure_auto_pull_cronjob()
github-actions
bot
added
size/large
Motivation
Tired of waiting on pull to complete. This is especially annoying on not very good network connection.
Summary
ARMBIAN_DOCKER_AUTO_PULLenvironment variable for opt-in controlrequirementscommand to Docker CLIChanges
docker_cleanup_old_images()- Removes dangling images and keeps only 2 most recent per tagdocker_pull_with_marker()- Pulls images and updates marker files tracking last pull timedocker_setup_auto_pull_cronjob()- Creates/updates system cronjob and wrapper script via hash-based detectiondocker_ensure_auto_pull_cronjob()- Ensures cronjob is installed/up-to-date OR removed if disableddocker CLI- Now handles cronjob setup instead of requirements commandARMBIAN_DOCKER_AUTO_PULL=yes- Must be explicitly set to enable cronjobSystem files created/managed:
/usr/local/bin/armbian-docker-pull- Self-contained wrapper script for cron execution/etc/cron.d/armbian-docker-pull- Cronjob (runs every 12 hours at 00:00 and 12:00)/var/lib/armbian/docker-pull.hash- Configuration hash for smart update detectionEnvironment Variables
ARMBIAN_DOCKER_AUTO_PULL=yes- Enable auto-pull cronjob (default: disabled)ARMBIAN_DOCKER_AUTO_PULL=no- Disable and remove any existing cronjobBenefits
ARMBIAN_DOCKER_AUTO_PULL=yesis explicitly setUsage Examples
Test plan
ARMBIAN_DOCKER_AUTO_PULL=yesis setARMBIAN_DOCKER_AUTO_PULL=noSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.