Repository navigation
Conversation
ideaship
force-pushed
the
mirror-automation
branch
from
September 11, 2026 13:46
4da8b42 to
297a60f
Compare
ideaship
force-pushed
the
mirror-automation
branch
2 times, most recently
from
October 3, 2026 09:43
99563a4 to
ce4e635
Compare
mirror_version() extracts a compressed source into tmp/ and never removes what it extracted, so every compressed version processed leaves its full unpacked image on disk. Nine of 39 in-scope versions are compressed (gardenlinux 7, opnsense 1, talos 1). The existing suite documents the leak instead of catching it: test_failed_extraction_leaves_no_downloaded_archive filters tmp out of its assertion. This change uses tempfile.mkdtemp() to create a per-version extraction directory and removes it in a finally block, guaranteeing cleanup on all paths out. shutil.move replaces shutil.copy so a multi-gigabyte image is renamed rather than duplicated—halving both peak disk and IO for every compressed version. Three new tests verify the cleanup: one for partial extraction with written bytes, one for archive members besides the image, and one for missing expected images. Assisted-by: Claude:claude-haiku-4-5-20251001 Signed-off-by: Roger Luethi <luethi@osism.tech>
The mirror.py main() function decides inline which versions to mirror based on an allow-listed set of shortname prefixes. A new read-only checker (check_mirror.py) must apply the identical predicate or it would report findings for images the uploader never touches (fedora, kubernetes, clearlinux). Extract the predicate into one importable place so the two cannot drift. This extraction defines: - MIRRORED_SHORTNAME_PREFIXES: the ten allow-listed prefixes - read_image_definitions(images_dir): load images from YAML files - iter_mirrorable(images_dir, name_filter=None): yield (image, version) tuples for mirrored versions only, optionally narrowed by --filter The main() loop is simplified to use iter_mirrorable(), eliminating inline filtering logic. --filter now narrows that scope, so a filter that matches only unmirrored images (Fedora, say) warns that no mirrored image matches instead of passing silently. The new functions are tested comprehensively: they handle disabled images (deliberately in scope), missing shortnames, versions without mirror_url, non-YAML files, the allow-list filtering, and --filter. Assisted-by: Claude:claude-haiku-4-5-20251001 Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
etc/images/*.yml points every image version at a mirror_url in the OSISM object store, and main.py hands that URL to Glance without falling back upstream. Nothing verifies the object is actually there: six versions the catalog references today cannot be served, which has already surfaced as a 403 an operator read as a transient store failure (#1268) and as a PR whose integration test cannot pass (#1273). Add contrib/check_mirror.py, a credential-free checker that probes every mirrored object the catalog references and reports what the store cannot serve. It consumes contrib.mirror.iter_mirrorable(), the same predicate the uploader itself applies, so the two cannot drift on scope. The probe is anonymous because that is how the system operates: main.py hands Glance a plain mirror_url with no credentials, and nothing in mirror.py verifies an object after upload. Anonymous HTTP is therefore the only check that reflects what the manager will see. A mirrored object must satisfy two independent conditions, because neither implies the other: - accepted_by_manager() reproduces main.py's own pre-import check, a non-following HEAD that accepts only 200 or 302. A 301 the manager refuses can still serve bytes perfectly well over GET. - fetchable() performs a ranged GET following redirects, requires the chain to end in 200 or 206, and reads one byte. Gardenlinux upstream URLs answer 302 (GitHub redirects its release downloads), so a check trusting the first status alone would pass a redirect whose target is gone. Reading the byte matters too: with stream=True the body is untouched until something consumes it, so a status-only check would call a zero-byte or truncated object healthy. An empty body is a verdict, reported as "206 (empty body)". A body that dies mid-transfer is not: it raises inside the retried callable and, if every attempt fails, ends the run with no verdict. Probes retry three times with a short backoff on transport failures only; an HTTP status is never retried, because it is an answer. When a mirror object is not retrievable, upstream is re-probed with fetchable() -- mirror.py downloads upstream with allow_redirects=True, so any chain ending in bytes is fine there -- to tell a gap the next mirror run can fill from one it cannot. The report never claims an object is missing or absent. The store answers 403 both for a key that was never uploaded and for one that exists but lost public read permission, and the fix differs, so "not retrievable" is the strongest claim available. Findings name the Glance image name alongside the shortname, version and status codes. Exit codes are the contract the Zuul playbook reads: - 0 clean -- every mirrored object the catalog references is retrievable. - 1 unmirrorable -- at least one version has neither a working mirror nor a working upstream, so no mirror run can fix it. - 2 operational -- the run reached no verdict (transport failure, bad usage). This intentionally matches typer's own usage-error exit code, so a mistyped CLI option can never be mistaken for a verdict. - 3 pending mirror only -- not yet mirrored but upstream is still reachable. Pending is 3, not 2, precisely so it is never confused with the operational code above. Adds the check-mirror tox environment, run as `python -m contrib.check_mirror` rather than a file path: the module imports contrib.mirror as a package, which only resolves with the repository root on sys.path. Assisted-by: Claude:claude-sonnet-5 Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Wire the mirror check into CI with two credential-free jobs: - openstack-image-manager-check-upstream runs in check on changes to the image definitions. It fails while any in-scope version has neither a retrievable mirror nor a retrievable upstream; it walks the whole catalog, so a red result is not necessarily caused by the change under review. A version that is merely not mirrored yet is fine here, since the upload happens after the merge. - openstack-image-manager-check-mirror runs in periodic-daily and accepts only a clean result. It is the only signal that survives the upload job being broken, expired, or not yet wired. The files matcher is an anchored regular expression, '^etc/images/.*\.yml$', not a glob. Zuul treats files entries as regexes, and 'etc/images/*.yml' as a regex means "etc/images, zero or more slashes, any character, then yml", which matches nothing here: the job would never run, and Zuul reports nothing for a job whose matcher never fires, so a change would look green with this gate silently disabled. Accepted exit codes are set per job -- check tolerates a pending mirror, the daily job does not -- and the playbook deliberately carries no default for them. Zuul passes job vars into the inventory as variables of the all group, and Ansible ranks play vars above inventory vars, so a default in the playbook would override every job and quietly make the daily check tolerant. An assert task fails a job that forgets to set them. playbooks/mirror-images.yml and playbooks/pre-mirror-images.yml land unreferenced: the upload job they belong to needs object-store credentials nobody holds yet. Zuul only permits a job defined in an untrusted project to use a secret in a post-review pipeline (post, periodic-*, tag); check is not post-review, so a check-pipeline job structurally cannot carry those credentials. The upload playbook keeps no_log: true on the credentialed task, matching the credential handling of the job removed in 3c21cc5, and asserts both secret data keys are defined beforehand -- no_log would otherwise censor Ansible's own "undefined variable" message and turn a first-run misconfiguration into an unexplained failure on a job nobody has ever run. It then re-runs the check strictly, because mirror.py verifies an upload through the authenticated client while Glance fetches anonymously. Both playbooks invoke tox through the path ensure-tox exports rather than a bare `tox`: the role installs into ~/.local/tox and only symlinks /usr/local/bin/tox when ensure_global_symlinks is set, which it is not here, so a bare call fails with "No such file or directory: b'tox'". Assisted-by: Claude:claude-haiku-4-5-20251001 Assisted-by: Claude:claude-sonnet-5 Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
Nothing has run the mirror step automatically since 2024, and an operator facing a red check or a 403 from the object store had no written path from symptom to action. Describe how an object reaches the store, how to read each of the two classes the check reports, how to run the mirror by hand with the keys, how to trigger the Zuul job manually, and what to add once the object-store credentials exist. Two points the prose is careful about. "Not retrievable" is the strongest claim available, because the store answers 403 both for an object that was never uploaded and for one that exists but lost public read permission, and the fix differs -- an upload versus a permissions change. And an unmirrorable version anywhere in the catalog blocks check for every subsequent image-definition PR, including automated ones, until that pin is bumped; the PR that bumps it is what clears the block. The credentialed job's YAML is included as a snippet only, with a warning that a placeholder encrypted value is a Zuul configuration syntax error for the whole project rather than for that job alone. The secret name, data keys and binding deliberately match the job removed in 3c21cc5, so an earlier runbook still applies. Assisted-by: Claude:claude-sonnet-5 Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
The README tells an operator holding the object-store keys to encrypt them with zuul-client and paste the result into .zuul.yaml, but leaves the invocation to them. Two details are easy to get wrong and neither fails loudly: zuul-client emits one complete "- secret:" block per field, so the two fields have to be merged into a single block by hand, and --infile keeps surrounding whitespace, so a credentials file with a trailing newline yields a key that authenticates nowhere while looking correct. Take the two values from the table "openstack ec2 credentials create" prints, or from MINIO_ACCESS_KEY and MINIO_SECRET_KEY, feed each to zuul-client over stdin -- which strips whitespace, and keeps the plaintext out of any argv, since printf is a shell builtin -- and emit the merged block on stdout. The run aborts unless exactly two encrypted fields come back, so a truncated block is never offered for pasting. Deliberately no "set -x", unlike build.sh and push.sh in this directory: every value handled here is a credential. The script wraps zuul-client rather than reimplementing RSA-OAEP against the project key, because a malformed !encrypted/pkcs1-oaep value is a configuration syntax error for the whole project, not just for the job that reads the secret. Verified against a dummy credentials file: the emitted block passes yamllint under .yamllint.yml when spliced into .zuul.yaml alongside the semaphore and job stanzas, and shellcheck reports no findings. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
mirror-automation
branch
from
October 5, 2026 05:21
ce4e635 to
f5e6451
Compare
This branch has not been deployed
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
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.
Nothing has run
contrib/mirror.pyautomatically since 2024 — the GitHubworkflow went in
855780e(2021-08-05), the Zuul play in3c21cc5(2024-03-05) — so the catalog can reference objects the object store cannot
serve, with no signal when that happens. Six versions are in that state right
now:
almalinux-10,centos-stream-10,opensuse-leap-16.0,rocky-10,ubuntu-26.04,ubuntu-26.04-minimal, all added by #1260 and all answering403.That gap has already cost time twice, in both cases diagnosed as something
else:
What this does
c39da07mirror_version()extracted compressed sources intotmp/and never removed what it extracted (9 of 39 in-scope versions are compressed)8261b89iter_mirrorable(), so a checker cannot drift from the uploaderaeade6econtrib/check_mirror.py, a credential-free checker, plus thecheck-mirrortox env13da9344da8b42README.mdThe checker probes anonymously, because that is how the system actually
operates:
main.pyhands Glance a plainmirror_urlwith no credentials, andnothing in
mirror.pyverifies an object after upload. A mirrored object mustsatisfy two independent conditions — the non-following
HEADgatemain.pyitself applies before importing (
200/302only), and a rangedGETfollowing redirects that ends in bytes actually read. Neither implies the
other: a
301the manager refuses still serves bytes fine overGET, thegardenlinux upstream URLs answer
302so trusting the first status alone wouldpass a redirect to a target that is gone, and with
stream=Truea status-onlycheck would call a zero-byte object healthy.
Gaps are reported in two classes — pending mirror (an upload is owed) and
unmirrorable (the pin must be bumped first) — and never as "missing": the
store answers
403both for a key that was never uploaded and for one thatlost public read permission, and the fix differs.
Exit codes are the contract the jobs read:
0clean,1unmirrorable,2noverdict reached,
3pending only.check-upstreamtolerates3, so a changeadding a version passes;
periodic-dailydoes not.What is deliberately NOT here
The encrypted secret and the credentialed upload job. Nobody currently holds
the object-store keys, and a placeholder
!encrypted/pkcs1-oaepvalue is aZuul configuration syntax error for the whole project, not just for that
job.
README.mdcarries the exact block to add — secret name, data keys andbinding deliberately identical to the job removed in
3c21cc5, so an earlierrunbook still applies — behind that warning.
Consequence, stated plainly:
periodic-dailygoes red on merge and staysred until someone with the keys runs an upload. That is the designed signal,
not a defect.
checkstays green.Things a reviewer may want to poke at
files:oncheck-upstreamis an anchored regex,'^etc/images/.*\.yml$',not a glob. Zuul treats
files:as regular expressions, andetc/images/*.ymlas a regex matches nothing here — the job would never run,silently. (The repo's pre-existing
irrelevant-files: - etc/images/*worksby accident, since
etc/imagesplus zero-or-more slashes matches as aprefix.)
playbook. Zuul passes job vars into the inventory as
all-group variablesand Ansible ranks play vars above those, so a default in the playbook would
override both jobs and quietly make the daily check tolerant. An
asserttask fails a job that forgets it.
no_log: trueon the credentialed task and assertsboth secret keys are defined first, because
no_logwould otherwise censorAnsible's own "undefined variable" message on the first run of a job nobody
has ever run.
Testing
tox -e test -- test/unit: 140 tests, and green at every commit in theseries (107 / 113 / 140 / 140 / 140), not just at the tip.
flake8,black,mypy,yamllintclean. The checker was also run against the live store: exit3, listing exactly the six versions above.The Zuul job definitions themselves were not executed — there is no local Zuul
— so those are review-by-reading.
Merge order
This conflicts textually with one open PR, which adds
"cirros"to the inlineallow-list inside
main()that8261b89replaces with a module-levelconstant:
Whichever lands second moves that one line into
MIRRORED_SHORTNAME_PREFIXES.Landing this one first makes that a trivial one-line diff. This PR does not
unblock it — its integration test imports Cirros from a
mirror_urlwhoseobject does not exist, and only an actual upload fixes that (filed separately
as an integration-test design issue).