refactor(debos): pin snapshot builds via native APT config - #619
refactor(debos): pin snapshot builds via native APT config#619Knightkolla wants to merge 1 commit into
Conversation
|
Knightkolla thanks for the PR; did you use am LLM to craft the changes? If so, could you document this in the commit pseudo-headers? Robie Basak (@basak-qcom) would you mind handling the PR review? |
The snapshot implementation juggles a parallel set of snapshot_*.sources files, a hand-rolled apt-snapshot-toggle script, and a per-source table of live-mirror-to-archive-URL rewrites, to point package installs at a dated snapshot archive. APT has a native mechanism for this: setting APT::Snapshot and Acquire::Check-Valid-Until in a single apt.conf.d snippet makes APT resolve every configured source through the dated archive on its own, for any archive that supports snapshot metadata. The Debusine-hosted qli archive now supports this too, so this covers all three currently pinned sources (debian, debian-backports, qli) with no per-source rewriting. Both recipes now write /etc/apt/apt.conf.d/99snapshot instead of deriving snapshot_*.sources and toggling between live and snapshot sources; the live .sources files are never touched. The rootfs recipe deletes the pin at the end instead of restoring live mirrors (there is nothing to restore); the image recipe recreates it from the SNAPSHOT value in /etc/buildinfo rather than re-enabling a disabled copy. Acquire::Check-Valid-Until "false" remains required and is a permanent trade-off, not a stopgap: a snapshot's Release file is served exactly as originally published and is never re-signed, so its Valid-Until window is always in the past. HTTPS is the accepted mitigation for the archive's authenticity in transit. Verified locally against snapshot.debian.org and the qli archive with debos in a container: packages resolve correctly through the dated archive for all three sources for a valid snapshot timestamp. Fixes: qualcomm-linux#602 Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Knightkolla <kartikeyadhavala2468@gmail.com>
dbdcaf5 to
b5f1e17
Compare
|
Hi Loïc Minier (@lool), yes I used Claude Code to help draft these changes. I've amended the commit with an Assisted-by: Claude Code:claude-sonnet-5 trailer, per the guidelines in CONTRIBUTING.md. I reviewed and tested the changes myself before opening the PR (see the PR description for what I verified locally against snapshot.debian.org and the qli archive), and I'm happy to answer any questions or make adjustments during review. Thanks for the quick response, and thanks Robie Basak (@basak-qcom) for taking a look let me know if you'd like me to change anything or if you have any questions. |
Robie Basak (basak-qcom)
left a comment
There was a problem hiding this comment.
Hi! Sorry for the delay in reviewing. An AI review found a rabbithole and haven't had the time to dig into it. I wonder if you could take a look, please? I'm not sure if the issue is valid. It seems serious enough that it's worth investigating. I would next try to reproduce this behaviour in apt outside qcom-deb-images using mmdebstrap+systemd-nspawn. If it is valid, I wonder if the apt maintainer would consider this a bug in apt, and if so, the previous reproducer would be helpful in filing a bug report upstream in apt if there isn't one there already. If the issue is valid, I think it would be better for apt to detect the case and redirect itself to the snapshot Release file to continue, so that a single initial apt update does the right thing automatically.
Given that this is refactoring and not blocking a feature, if the issue in apt is valid then it's probably worth getting a fix for apt in the pipeline while landing this.
I think it's also worth adding some automatic verification that the snapshot selected was actually used, or perhaps that the non-snapshot was not used. IMHO this would be better done in a separate test suite, but I suppose an assertion during the operation also works if that's much easier.
AI report below. I'm marking this "Request changes" since I think more investigation is needed before landing.
1. (High, correctness) qli is silently not pinned on a fresh build — ordering bug
In the rootfs recipe the pin is written (qualcomm-linux-debian-rootfs.yaml:277-292) before any apt-get update has fetched qli's Release. The first and only guaranteed update runs with the pin already in place (:305), immediately followed by apt-get -y full-upgrade (:306).
At that first update, qli has no Release on disk and its host isn't preseeded, so APT can't read qli's Snapshots: field and falls back to live qli, silently (the APT::Snapshot apt.conf path carries a ? best-effort suffix — debmetaindex.cc:1331 — and does goto nosnapshot when no snapshot server is found, :1381). full-upgrade — and any qli-sourced package it pulls (e.g. the backport/qli-pinned fastrpc, alsa-ucm-conf, and the fastrpc-tests/userspace-resource-manager installs) — therefore comes from the live qli archive, defeating reproducibility for precisely the archive this commit claims newly supports.
Debian/backports are unaffected because deb.debian.org is in APT's built-in host preseed table (init.cc:208), which is likely why the "verified locally … all three sources resolve correctly" check passed: either a second apt-get update had happened, or qli's lists were already cached in the test environment. The failure mode is silent, so a green local run doesn't clear it.
Fix: follow APT's intended workflow ("run update without snapshot id first"). Create all .sources, run one plain apt-get update to populate Release files (incl. qli's, with its Snapshots: field), then write 99snapshot, then apt-get update again. Everything after that resolves through the dated archive for all three sources.
2. (Medium, correctness/robustness) No verification the pin took effect; the loud assert was removed
The old code hard-asserted each expected mirror URL and failed the build if absent, plus warned for any enabled source without snapshot support. The new code has no check that pinning actually happened, and the ?/apt.conf path fails open. So a broken qli pin (finding #1, or a future Release that drops the Snapshots: field) degrades to live with no signal.
Given reproducibility is the whole point, add a post-update assertion — e.g. confirm the fetched qli index resolves to a dated URL (grep /var/lib/apt/lists/ for the snapshot path, or parse apt-get indextargets) and fail loudly otherwise. This restores the "fail, don't silently drift" property the table+assert used to provide.
Title: refactor(debos/rootfs,debos/image): use native APT snapshot config
Fixes: #602
What I did
Refactored how the snapshot build option pins APT package installs to a dated archive, in both debos-recipes/qualcomm-linux-debian-rootfs.yaml and debos-recipes/qualcomm-linux-debian-image.yaml, plus the matching internals documentation in docs/snapshot.md.
Removed:
The apt-snapshot-toggle shell script written into the rootfs, whose only job was flipping Enabled: yes/no between live and snapshot .sources files.
The step that derived a parallel snapshot_*.sources file for each live .sources file, using a hand-maintained per-source table of mirror-URL-to-archive-URL rewrites (one entry each for debian, debian-backports, qli).
The runtime warning loop that scanned .sources files for ones with no snapshot equivalent.
Replaced with:
A single file, /etc/apt/apt.conf.d/99snapshot, containing two lines:
APT::Snapshot "";
Acquire::Check-Valid-Until "false";
This is APT's own native mechanism for resolving packages from a dated snapshot across every configured source automatically, with no per-source knowledge required.
A static warning, gated on the aptlocalrepo variable, since a local APT repo is the one source type that genuinely has no dated-archive equivalent.
In the image recipe, the pin file is recreated (from SNAPSHOT= in /etc/buildinfo, carried forward via rootfs.tar) rather than toggled back on, since the rootfs recipe now deletes it outright at the end instead of leaving it disabled.
Also rewrote the stale # TODO: remove Check-Valid-Until once Debusine-based snapshots are available comment. Per the issue's maintainer, this assumption was wrong — disabling Check-Valid-Until is a permanent, accepted trade-off (a snapshot's Release file is published once and never re-signed, so its validity window is always in the past; HTTPS is the mitigation for authenticity in transit), not a stopgap waiting on a future infra change.
Why the old approach existed and why this is better
The old mechanism worked, but required duplicating every .sources file and hand-maintaining a per-archive rewrite table, purely to work around not having a way to tell APT directly "resolve everything as of this date." That's exactly what APT::Snapshot is for. The refactor became possible because the maintainer confirmed (in the issue thread) that the Debusine-hosted qli archive now has snapshot metadata support too previously only Debian's own archive definitely supported it, so the original implementation had to special-case things.
How it solves the issue
It's a straight replacement of a manual mechanism with the native one, with the same externally-visible behavior (same snapshot variable, same requirement to pass it to both recipes). No .sources file rewriting, no duplication, no toggle script one config file, written before install and deleted after, is the entire mechanism.
What I tested, and what I didn't
No Linux build environment on my end (macOS, 8GB RAM). I ran the godebos/debos:latest container directly (bypassing the Makefile's tty requirement) against the rootfs recipe with real -t snapshot: runs.
Confirmed working:
debian, debian-security: every fetch was transparently redirected to snapshot.debian.org/...// by APT itself, and apt-get full-upgrade completed cleanly against the pinned archive.
qli: for a timestamp after Debusine's snapshot rollout, both Release/indexes and actual .deb packages were fetched successfully through https://deb.debusine.qualcomm.com/qualcomm/qli//.... An older timestamp (predating rollout) correctly 404s on qli verified directly with curl that this is an archive-side condition, not a bug in this change: the same URL shape resolves fine for a recent date.
Not tested:
A full rootfs.tar build, the disk-ufs.img build, or booting the result. The local build got wiped out past the qli install step the part this refactor actually touches because the host ran out of memory running debos through a software-emulation backend under Docker Desktop. This is a local hardware constraint, not something the code changes caused.
The image recipe's re-creation of the pin file was written by hand, matching the rootfs recipe's logic, but not exercised in an actual image build.
Nothing was boot-tested with run-qemu.py.
How it fits into the pipeline
Pure internals change to one build option. Doesn't touch the flash recipe (no snapshot option there), doesn't change the Makefile, doesn't change anything outside the {{- if $snapshot }} blocks in the two recipe files — EXTRA_DEBOS_OPTS="-t snapshot:" works identically from the outside.
What to review
Whether recreating 99snapshot in the image recipe (rather than toggling a disabled copy left in place) is the right call it's a small behavior change from the original design: deletes now instead of disabling.
Whether the bootstrap step's separate, unmodified snapshot-mirror-URL override (it runs host-side, before /etc/apt/apt.conf.d/ exists, so it can't use the new mechanism) is fine as-is, or whether reviewers want to explore folding it in too I deliberately didn't guess at how mmdebstrap's internal APT invocation handles APT::Snapshot.
Whether the aptlocalrepo warning replacement is adequate the old one checked actual .sources file state at runtime, and the new one is a static check against the aptlocalrepo variable.
A full local build couldn't complete —someone with more RAM, a real Linux box, or CI would need to run the full pipeline before merge.