From 0eddd1ba6412595aa437e606480cb6802ac7bf36 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Tue, 9 Sep 2025 17:51:29 +0200 Subject: [PATCH] container: refactor cleanup on instance removal This commit reverts 477f7ae and bb19d06, which intended to fix an issue with lingering old images, see #1098. However, as detailed in #1147, this caused severe side effects while working with multiple larger containers. Basically, the prune operation of one container removed images of other containers that are just being created in parallel. Instead of using the podman prune command we can use the meta datain the start script to pinpoint exactly which image(s) to remove, including any downloaded OCI archives when the container instance is removed. Fixes #1147 Signed-off-by: Joachim Wiberg --- .../etc/finit.d/available/container@.conf | 9 +- board/common/rootfs/usr/sbin/container | 125 ++++++++++++++++-- src/confd/src/core.c | 6 +- src/confd/src/infix-containers.c | 26 ++++ 4 files changed, 149 insertions(+), 17 deletions(-) diff --git a/board/common/rootfs/etc/finit.d/available/container@.conf b/board/common/rootfs/etc/finit.d/available/container@.conf index 7a494c60..4e6b813c 100644 --- a/board/common/rootfs/etc/finit.d/available/container@.conf +++ b/board/common/rootfs/etc/finit.d/available/container@.conf @@ -1,7 +1,8 @@ # Start a container instance (%i) and redirect logs to /log/container -# Give podman enough time to properly shut down the container, kill:30 -# The pre:script, which is responsibe for fetching a remote image, must -# not have a timeout. The cleanup should take no longer than a minute. +# Give podman enough time to properly shut down the container, kill:30, +# which is also matched in the container script (podman default is 10!) +# The pre:script, responsibe for fetching a remote image and calling on +# 'podman load', must not have a timeout. sysv log:prio:local1,tag:%i kill:30 pid:!/run/container:%i.pid \ - pre:0,/usr/sbin/container cleanup:60,/usr/sbin/container \ + pre:0,/usr/sbin/container cleanup:0,/usr/sbin/container \ [2345] :%i container -n %i -- container %i diff --git a/board/common/rootfs/usr/sbin/container b/board/common/rootfs/usr/sbin/container index c3a5bda4..bbebb43b 100755 --- a/board/common/rootfs/usr/sbin/container +++ b/board/common/rootfs/usr/sbin/container @@ -6,19 +6,24 @@ # NOTE: when creating/deleting containers, remember 'initctl reload' to # activate the changes! In confd this is already handled. # +CLEANUP=/var/lib/containers/cleanup DOWNLOADS=/var/lib/containers/oci BUILTIN=/lib/oci BASEDIR=/var/tmp container=$0 checksum="" extracted= -timeout=30 +timeout=30 # NOTE: matched with container@.conf dir="" all="" env="" port="" force= +# Variable shared across subshells +export meta_sha="" + + log() { logger -I $PPID -t container -p local1.notice -- "$*" @@ -118,6 +123,7 @@ load_archive() { uri=$1 tag=$2 + tmp=$3 img=$(basename "$uri") # Supported transports for load and create @@ -230,7 +236,8 @@ load_archive() err 1 "failed tagging image as $tag" fi - # Save archive SHA256 to sidecar file for local archives to enable optimization + # Save archive SHA256 to sidecar file to enable optimization + # This applies to both local archives and downloaded remote archives if [ -f "$file" ]; then img_sha256=$(sha256sum "$file" | cut -f1 -d' ') archive_basename=$(basename "$file") @@ -239,6 +246,10 @@ load_archive() mkdir -p "$DOWNLOADS" echo "$img_sha256" > "$sha_file" + if [ -n "$tmp" ]; then + echo "$img_sha256" > "$tmp" + fi + log "Saved archive checksum to $sha_file" fi @@ -273,10 +284,16 @@ create() # Unpack and load docker-archive/oci/oci-archive, returning image # name, or return docker:// URL for download. - if ! image=$(load_archive "$image"); then + sha_file=$(mktemp -u) + if ! image=$(load_archive "$image" "" "$sha_file"); then exit 1 fi + if [ -f "$sha_file" ]; then + meta_sha=$(cat "$sha_file" 2>/dev/null || echo "") + rm "$sha_file" + fi + if [ -z "$logging" ]; then logging="--log-driver syslog" fi @@ -310,8 +327,10 @@ create() # Add optimization labels for meta-sha256 and config checksum script="/run/containers/${name}.sh" if [ -f "$script" ]; then - # Extract meta-sha256 from script if present - meta_sha=$(awk '/^# meta-sha256:/ {print $3}' "$script" 2>/dev/null) + if [ -z "$meta_sha" ]; then + # Extract meta-sha256 from script if present + meta_sha=$(awk '/^# meta-sha256:/ {print $3}' "$script" 2>/dev/null) + fi if [ -n "$meta_sha" ]; then args="$args --label meta-sha256=$meta_sha" fi @@ -357,11 +376,90 @@ delete() sleep 1 done + # NOTE: -v does not remove *named volumes* podman rm -vif "$name" >/dev/null 2>&1 [ -n "$quiet" ] || log "Container $name has been removed." +} - cnt=$(podman image prune -af | wc -l) - log "Pruned $cnt image(s)" +# Called by Finit cleanup:script when a container service is removed. +# Processes all container instance IDs found in $CLEANUP// directory +# and removes them. This handles the case where a container with the same +# name but different configuration replaces an old one. +# +# Note: Volumes are NOT automatically removed to prevent accidental data loss. +# Use 'admin-exec container prune' to clean up unused volumes manually. +cleanup() +{ + if [ -z "$name" ]; then + log "cleanup: missing container name" + return 1 + fi + + cleanup_dir="$CLEANUP/$name" + if [ ! -d "$cleanup_dir" ]; then + log "cleanup: no cleanup directory for $name, nothing to do" + return 0 + fi + + log "Cleaning up container instances for: $name" + + # Remove all container instances by ID from the cleanup directory + for id_file in "$cleanup_dir"/*; do + [ -f "$id_file" ] || continue + cid=$(basename "$id_file") + + # Extract image name and meta-sha256 from the container instance labels + img=$(podman inspect "$cid" 2>/dev/null | jq -r '.[].ImageName' 2>/dev/null || echo "") + sha=$(podman inspect "$cid" --format '{{index .Config.Labels "meta-sha256"}}' 2>/dev/null || echo "") + + log "Removing container instance with ID: ${cid:0:12}..." + podman rm -vif "$cid" >/dev/null 2>&1 + rm -f "$id_file" + + # Clean up the image if it exists and is not used by other containers + if [ -n "$img" ] && [ "$img" != "null" ]; then + if ! podman ps -a --format "{{.Image}}" | grep -q "^${img}$"; then + log "Removing unused image: $img" + podman rmi "$img" 2>/dev/null || true + + # Also remove archive and sidecar file from $DOWNLOADS for remote images + if [ -n "$sha" ] && [ -d "$DOWNLOADS" ]; then + # Search for matching sidecar file containing this SHA + for sha_file in "$DOWNLOADS"/*.sha256; do + [ -f "$sha_file" ] || continue + stored_sha=$(cat "$sha_file" 2>/dev/null || echo "") + if [ "$stored_sha" = "$sha" ]; then + # Found matching sidecar - derive archive filename + archive="${sha_file%.sha256}" + + if [ -f "$archive" ]; then + # Safety check: verify archive SHA matches before removing + archive_sha=$(sha256sum "$archive" 2>/dev/null | awk '{print $1}') + if [ "$archive_sha" = "$sha" ]; then + log "Removing archive: $archive" + rm -f "$archive" + else + log "Warning: archive SHA mismatch for $archive, not removing" + fi + fi + + log "Removing sidecar file: $sha_file" + rm -f "$sha_file" + break + fi + done + fi + else + log "Image $img still in use by other containers, not removing" + fi + fi + done + + # Remove the cleanup directory for this container, and parent if empty + rmdir "$cleanup_dir" 2>/dev/null + rmdir "$CLEANUP" 2>/dev/null + + exit 0 } waitfor() @@ -466,7 +564,7 @@ netrestart() done } -cleanup() +atexit() { pidfile=$(pidfn "$name") @@ -674,7 +772,7 @@ if [ -n "$cmd" ]; then shift fi -trap cleanup INT HUP TERM +trap atexit INT HUP TERM case $cmd in # Does not work atm., cannot attach to TTY because @@ -682,6 +780,9 @@ case $cmd in # attach) # podman attach "$1" # ;; + cleanup) # Hidden from public view, use remove or flush commands instead + cleanup "$@" + ;; create) [ -n "$quiet" ] || log "Got create args: $*" create "$@" @@ -920,7 +1021,7 @@ case $cmd in rm -f "$pidfile" cnt=$(podman image prune -f | wc -l) - log "setup: pruned $cnt image(s)" + log "Pruned $cnt image(s)" ;; shell) if [ -z "$name" ]; then @@ -1060,8 +1161,8 @@ case $cmd in ;; cleanup) # Called as cleanup-script from Finit service - log "Calling $container -n $SERVICE_ID delete" - exec $container -q -n "$SERVICE_ID" delete + log "Calling $container -n $SERVICE_ID cleanup" + exec $container -q -n "$SERVICE_ID" cleanup ;; *) false diff --git a/src/confd/src/core.c b/src/confd/src/core.c index 49fb59fe..719d6228 100644 --- a/src/confd/src/core.c +++ b/src/confd/src/core.c @@ -82,8 +82,12 @@ int core_post_hook(sr_session_ctx_t *session, uint32_t sub_id, const char *modul return SR_ERR_OK; } - if (systemf("initctl -b reload")) + if (systemf("initctl -b reload")) { + EMERG("initctl reload: failed applying new configuration!"); return SR_ERR_SYS; + } + + AUDIT("The new configuration has been applied."); return SR_ERR_OK; } diff --git a/src/confd/src/infix-containers.c b/src/confd/src/infix-containers.c index b4e37a89..61d777ea 100644 --- a/src/confd/src/infix-containers.c +++ b/src/confd/src/infix-containers.c @@ -21,6 +21,7 @@ #define CFG_XPATH "/infix-containers:containers" #define _PATH_CONT "/run/containers" +#define _PATH_CLEAN "/var/lib/containers/cleanup" /* * Check if image is a local archive and return the offset to the file path. @@ -302,9 +303,34 @@ static int add(const char *name, struct lyd_node *cif) */ static int del(const char *name) { + char prune_dir[sizeof(_PATH_CLEAN) + strlen(name) + 3]; + char buf[256]; + FILE *pp; + erasef("%s/%s.sh", _PATH_CONT, name); systemf("initctl -bnq disable container@%s.conf", name); + /* Schedule a cleanup job for this container as soon as it has stopped */ + snprintf(prune_dir, sizeof(prune_dir), "%s/%s", _PATH_CLEAN, name); + systemf("mkdir -p %s", prune_dir); + + /* Finit cleanup:script runs when container is deleted, it will remove any image by-ID */ + pp = popenf("r", "podman inspect %s 2>/dev/null | jq -r '.[].Id' 2>/dev/null", name); + if (!pp) { + /* Nothing to do, if we can't get the Id we cannot safely remove anything */ + ERROR("Cannot find any container instance named '%s' to delete", name); + rmdir(prune_dir); + return SR_ERR_OK; + } + + if (fgets(buf, sizeof(buf), pp)) { + chomp(buf); + if (strlen(buf) > 2) + touchf("%s/%s", prune_dir, buf); + } + + pclose(pp); + return SR_ERR_OK; }