fix(rpm): keep importing after an archive containerd cannot read (MK8S-418) by ezekiel-alexrod · Pull Request #27 · scality/image-cache

ezekiel-alexrod · 2026-09-24T15:10:30Z

Component

rpm

Problem

containerd-image-preload.sh runs under set -euo pipefail and called ctr images import bare inside its loop. The first archive ctr refused ended the run, so every archive the glob listed after it stayed out of containerd and the log only named that first one. Which images survived depended on file names.

That's worst on a node with no registry to fall back on: one torn archive keeps the rest of the static pod images out, and the ten minute timer replays the same failure without ever getting further.

Fix

Every archive is tried. Each one ctr refuses is named on stderr as it happens, and the run exits 1 at the end with a summary: Failed to import N of the M cached archives (X imported, Y no longer there). The unit still fails, but the node gets every image the cache could still give it.
An archive that's already gone when its turn comes is skipped, not reported. The glob is expanded before the first import, and the agent removes a resource's directory both to collect it and to swap a fresh extraction in. Without the skip, a garbage collection during a run would have the unit name every archive under the dropped directory.
An archive that fails and is gone afterwards still counts as a failure. So does one removed between the check and ctr opening it. A stat taken after ctr refused it says the file is absent now, not that absence is why the import failed, and I'd rather report a rare race than hide a containerd outage that happened to line up with a cleanup.
A run with nothing left to import says so on stderr and exits 0. A whole cache going away at once is legitimate, and this script isn't the one to decide it's wrong.
Each line carries its syslog priority under systemd (err for failures, warning for an empty run, info otherwise), so journalctl -p err finds the failed imports. Without the prefix, stderr lands at info like stdout. It's only written when the stream really is the journal: JOURNAL_STREAM is inherited by shells started from a logged session, so the descriptor is compared to it. A successful run ends with Imported N of the M cached archives.

Two changes go beyond the ticket:

package.bats now runs ShellCheck over the bats files themselves. In bats, ! cmd that isn't the last line of a test asserts nothing (SC2314): a test with ! grep -q notes.txt on a log that contains notes.txt, followed by true, passes. script.bats:71 on main was one of those, and the new tests would have added more. ShellCheck parses bats from 0.9.0 only and Rocky 8 ships 0.6.0, so that case skips on EL8 and gates on EL9. It fails, not skips, when ShellCheck is missing.
The three linter cases print the captured output before checking the status. run was swallowing it, so a lint failure in CI showed the test name and nothing else.

.claude/REVIEW.md said "partial cache means skip, not fail" for the import script, which no longer matches. The row now describes the skip, the failure, and what keeps an unfinished agent directory out of the import (the glob).

Test

make -C rpm test EL=8 and EL=9: 33/33 on both, with the bats ShellCheck case skipped on EL8 as expected.
The new script.bats cases run against the script from main: 8 fail (the unreadable archive, every failure named, the summaries, the skip), the 7 existing ones pass.
The script under systemd-run --user: journalctl -o json shows priority 3 on the failures and 6 on the rest. The same run from a terminal that inherited JOURNAL_STREAM prints no prefix.
A stubbed ctr refuses archives by path and can delete a directory on its first call, which is how the tests cover a failure in the flat layout and in a subdirectory, and an agent collecting a resource mid-run.

Out of scope

Deleting or repairing an archive the script can't read. Whoever wrote it owns that.
The provisioning side, MK8S-157 and MK8S-394.
Shipping the fix to MetalK8s needs an image-cache release, then the version bump MK8S-408 describes.

The docs describing this behaviour are updated in the same pull request: rpm/README.md owns it.
A change to the cache directory layout (subdirectory scheme, archive names, sentinel, permissions) lands in both halves and in agent/DESIGN.md. Not applicable: the layout doesn't change.

Relates-to: MK8S-418

The loop ran under `set -e` with nothing catching a failed import, so the first archive `ctr` refused ended the run. Every archive the glob listed after it stayed out of containerd, and the log named only that first one. It hurts most on a node with no registry to fall back on. One torn archive keeps the static pod images out of the content store, and the ten minute timer replays the same failure without ever getting further. Every archive is tried now. The run still exits non-zero when at least one of them failed, and it names all of them, so the unit reports the failure without hiding the images it did import. The summary says what became of every archive the cache held, since one torn file out of forty reads nothing like a containerd that is down. Trying everything means the loop outlives the glob that listed it, and the agent collects a resource by taking its directory away. An archive that is already gone when its turn comes is therefore skipped rather than reported: without that, a garbage collection mid-run would have the unit name every archive under the directory the agent dropped. One that `ctr` refuses stays a reported failure even if it is gone afterwards, which is also what happens to one removed between the check and `ctr` opening it. A stat taken after the fact proves the file is absent now, not that absence is why the import failed, and swallowing a containerd outage that happened to coincide with a cleanup would be worse than reporting a rare race. A run that ends up importing nothing says so on stderr and still exits zero: a whole cache being collected at once is legitimate, and this script is not the one to decide otherwise. The tests that came with this needed a stubbed `ctr` that can refuse a named archive and delete one mid-run, stdout and stderr kept apart, and assertions that hold on bats 1.5.0, which is what Rocky 8 ships. Refs: MK8S-418

@test

The suite ran ShellCheck over sources/*.sh and build.sh and left its own test files out, which is where the trickier shell now lives: a stubbed `ctr` written through a heredoc, positional slicing, a grep whose pattern comes from a path. ShellCheck has a bats mode and it pays for itself immediately. It reports SC2314 on `! grep -q ...` used as anything but the last command of a test, because bash exempts a `!`-prefixed command from errexit and the assertion then passes whatever grep finds. One such assertion was already on main, and writing the tests for the import fix would have added another. It only runs on one leg. ShellCheck learned to parse bats in 0.9.0, Rocky 8 ships 0.6.0, and there the file stops parsing at the first @test, so the case skips on EL8 and gates on EL9. The version probe fails the test rather than skipping when ShellCheck is missing altogether, which is the failure mode that would quietly delete the gate. All three linter gates now go through one assertion that echoes the captured output before checking the status. `run` was swallowing the only thing a lint failure has to say, so CI named the test and nothing else: no rule, no line, no message. Refs: MK8S-418

anthony-treuillier-scality · 2026-10-01T06:30:16Z

    echo "Importing ${tar}"
-    ctr -n k8s.io images import --platform "${IMAGE_PLATFORM}" "${tar}"
+    if ! ctr -n k8s.io images import --platform "${IMAGE_PLATFORM}" "${tar}"; then
+        echo "Failed to import ${tar}" >&2


It would be interesting to add timestamp

The journal already timestamps each line, so I'd rather not add one in the message: it would show twice in journalctl. But you're right about the level, stderr was landing at info like stdout. Failures are err now, so journalctl -u containerd-image-preload -p err shows only them.

anthony-treuillier-scality · 2026-10-01T06:36:59Z

+    echo "Failed to import ${failures} of the ${scanned} cached archives" \
+         "(${imported} imported, ${skipped} no longer there)" >&2


same comment about timestamp

Ditto, this one is err now.

anthony-treuillier-scality · 2026-10-01T06:37:05Z

+    # Nothing failed, yet nothing made it in either. A whole cache going away
+    # under the run is legitimate (every resource collected at once) and is not
+    # this script's call to make, but it must not pass for a quiet success.
+    echo "None of the ${scanned} cached archives were still there to import" >&2


same comment about timestamp

Ditto, warning there.

anthony-treuillier-scality · 2026-10-01T06:40:31Z

Maybe add introduction and conclusion messages "images import is starting/finished", with timestamp and INFO level on stdout

systemd already logs when the unit starts and ends (Starting ..., then Finished ... or Failed with result 'exit-code'). What was missing is what the run did, so it now ends with Imported N of the M cached archives at info. Is it ok for you?

Under systemd, stdout and stderr both land in the journal at info, so a failed import did not show with `journalctl -p err`. Lines are now prefixed with their syslog priority, which journald reads and strips. The prefix is only written when the stream really is the journal. JOURNAL_STREAM is inherited by shells started from a logged session, so the descriptor is compared to it, and a run by hand stays plain. A run now also ends with a line saying what it imported. systemd already logs when the unit starts and ends, and the journal adds the timestamps. Refs: MK8S-418

ezekiel-alexrod added 2 commits September 24, 2026 14:14

ezekiel-alexrod requested a review from a team as a code owner September 24, 2026 15:10

ezekiel-alexrod requested review from TeddyAndrieux and anthony-treuillier-scality and removed request for a team September 24, 2026 17:14

ezekiel-alexrod self-assigned this Sep 24, 2026

ezekiel-alexrod added P1 High priority rpm The containerd-image-preload package labels Sep 24, 2026

anthony-treuillier-scality reviewed Oct 1, 2026

View reviewed changes

ezekiel-alexrod requested a review from anthony-treuillier-scality October 2, 2026 08:03

Navigation Menu

fix(rpm): keep importing after an archive containerd cannot read (MK8S-418) - #27

fix(rpm): keep importing after an archive containerd cannot read (MK8S-418)#27
ezekiel-alexrod wants to merge 3 commits into
mainfrom
bugfix/MK8S-418-import-every-cached-archive

ezekiel-alexrod commented Sep 24, 2026 •

edited

Loading

Uh oh!

anthony-treuillier-scality Oct 1, 2026

Uh oh!

ezekiel-alexrod Oct 1, 2026

Uh oh!

anthony-treuillier-scality Oct 1, 2026

Uh oh!

ezekiel-alexrod Oct 1, 2026

Uh oh!

anthony-treuillier-scality Oct 1, 2026

Uh oh!

ezekiel-alexrod Oct 1, 2026

Uh oh!

anthony-treuillier-scality Oct 1, 2026

Uh oh!

ezekiel-alexrod Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		echo "Failed to import ${failures} of the ${scanned} cached archives" \
		"(${imported} imported, ${skipped} no longer there)" >&2

Conversation

ezekiel-alexrod commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Component

Problem

Fix

Test

Out of scope

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ezekiel-alexrod commented Sep 24, 2026 •

edited

Loading