Stop reinstalling rootfs and initramfs on every sdk install - #185
Stop reinstalling rootfs and initramfs on every sdk install#185mobileoverlord wants to merge 1 commit into
Conversation
## Problem
`avocado sdk install` redid the rootfs and initramfs installs in full on
every invocation, even when nothing had changed. Two independent defects
caused it.
**1. False "packages removed" detection wiped both sysroots every run.**
`detect_sysroot_package_removals` compared *config-declared* package
names against *locked* names and returned true — meaning `rm -rf
$AVOCADO_PREFIX/<sysroot>` plus a from-scratch reinstall — whenever the
lock held names the config didn't.
But the install auto-appends two packages the config never declares, then
records them in the lock:
- `packagegroup-avocado-{rootfs,initramfs}-modules-<kver>`
- `kernel-image-<kver>` (rootfs only)
From the second run onward those always read as removed. Because
`get_rootfs_packages` / `get_initramfs_packages` default to the single
meta-package, this fired on *every* project, not only ones overriding
`rootfs.packages`. Confirmed against every real lockfile in the tree —
for example `references/jetson-trt/avocado.lock`:
| | locked names | config names |
|---|---|---|
| rootfs | `avocado-pkg-rootfs`, `kernel-image-6.8.12-…`, `packagegroup-avocado-rootfs-modules-6.8.12-…` | `avocado-pkg-rootfs` |
| initramfs | `avocado-pkg-initramfs`, `packagegroup-avocado-initramfs-modules-6.8.12-…` | `avocado-pkg-initramfs` |
**Second-order effect:** the same path called
`remove_packages_from_sysroot`, dropping those packages' version pins
from the in-memory lock. `build_package_spec_with_lock` then fell back to
a bare package name, so dnf resolved the kernel image and module
packagegroup to *newest available* instead of the pinned NVR.
`avocado.lock` looked pinned on disk (it is rewritten post-install) but
the pin never applied.
**2. The install stamps were written but never read.** `install_sysroot`
wrote `rootfs/install.stamp` and `initramfs/install.stamp`, but nothing
consulted them to skip work — across the whole CLI, stamps were only
prerequisite gates. So even with defect 1 fixed, each run still paid a
kernel repoquery, off-kernel exclude computation, a repoquery existence
probe, the dnf transaction, `query_installed_packages`, kernel-sysroot
staging, and the stamp write.
## Solution
An unchanged config now does zero container work for rootfs/initramfs; a
changed one reinstalls exactly the sysroot(s) affected; `avocado.lock`
pins actually bind.
## Key changes
- **Compare removals against the effective set.** `install_sysroot`
resolves the kernel and computes the auto-appended names *before* removal
detection, and `detect_sysroot_package_removals` now takes the effective
name set (config packages ∪ auto-appends) and returns the removed names
rather than a bool. That set is built once and reused as the post-install
lock query list, replacing a second, duplicate computation. A kver change
falls out of the same mechanism, so the dedicated kernel-pin-change block
became the *reason* for the clean rather than a second code path, and the
two verbatim inline `rm -rf` RunConfig blocks collapse into one
`clean_sysroot` helper built from the existing `clean_sysroot_command`.
- **Skip the install when the stamp is current.** New
`read_stamps_batch` / `StampBatch` in `utils/prerequisites.rs` wraps
`generate_batch_read_stamps_script` + one container run + parsing. `sdk
install` reads both sysroot stamps in a single invocation before the
parallel phase and passes them in via `SysrootInstallParams`; the
standalone `rootfs install` / `initramfs install` read their own through
the same helper. `install_sysroot` short-circuits before any container
call. Stamps live in the `/opt/_avocado` named volume rather than a host
bind mount, so a container run is unavoidable — hence batching.
`check_prerequisites` and five sites that hand-rolled the same RunConfig
(`build.rs`, `sdk/{clean,package}.rs`, `ext/{clean,checkout}.rs`) are
ported to the helper.
- **Make the hash trustworthy enough to skip on.** The two per-sysroot
hash functions collapse into one core that now hashes the *effective*
package map (so an absent `rootfs.packages` and an explicit default hash
identically), plus `sdk.repo_url`, `sdk.repo_release` — the hook that
makes a snapshot bump or `avocado update` land — and
`sdk.disable_weak_dependencies`. `StampInputs.package_list_hash` is
populated with a digest of that sysroot's lockfile pins, so a hand-edited
`avocado.lock` or an `avocado unlock` goes stale automatically. An empty
pin set hashes to its own value rather than to `None`, because
`Stamp::is_current` only compares that field when both sides carry one —
otherwise a cleared lock would compare equal by omission.
`STAMP_VERSION` is deliberately *not* bumped: that would invalidate every
SDK/ext/runtime stamp. Changing only these two hash bodies makes existing
rootfs/initramfs stamps mismatch naturally, costing one reinstall.
- **Make `clean` invalidate its stamp.** `clean_sysroot_command` also
removes `$AVOCADO_PREFIX/.stamps/<sysroot>`, mirroring `ext clean`.
Without it, `rootfs clean && sdk install` would skip against a current
stamp and leave an empty sysroot.
- **Drop the mid-flight lockfile saves.** `install_sysroot` and
`stage_kernel_sysroot_from_rootfs` saved their lockfile *clone* while the
rootfs and initramfs tasks ran concurrently on separate clones —
last-writer-wins, healed only by the caller's final save. Saving is now
the caller's job throughout, which removes the race.
Escape hatches are unchanged in spirit: `--no-stamps` disables the read
along with the write, so it always reinstalls, and `avocado rootfs clean`
remains the hard reset.
## Reviewer notes
`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings`, and the
full `cargo test` suite are green (1124 lib tests plus the integration
suites, no failures).
New unit tests:
- `detect_sysroot_package_removals` reports nothing for the exact
`references/jetson-trt/avocado.lock` shape — a default config with both
auto-appended kernel packages locked. This is the regression test for the
bug.
- It still reports a genuinely config-dropped package, and reports the
previous kver's auto-appends as stale after a repin.
- The rootfs hash is stable across absent-vs-explicit-default
`rootfs.packages`, and moves on an added package, on `repo_url`,
`repo_release` and `disable_weak_dependencies`, and on a lock re-pin.
- `package_list_hash` is order-independent (pins come out of a `HashMap`),
and an `avocado unlock` invalidates `is_current`.
- A rootfs-only config edit does not invalidate the initramfs hash.
End-to-end verification against a live feed is in progress separately.
There was a problem hiding this comment.
Pull request overview
This PR optimizes avocado sdk install by preventing unnecessary rootfs/initramfs reinstalls when inputs haven’t changed, and by fixing erroneous “packages removed” detection that caused sysroot wipes and ineffective lockfile pinning.
Changes:
- Fix sysroot removal detection to compare against the effective package set (config packages plus auto-appended kernel/module packages), returning concrete removed names.
- Add batched stamp reading (
read_stamps_batch/StampBatch) and use sysroot install stamps to short-circuit rootfs/initramfs installs when current. - Strengthen sysroot install stamp hashing to include effective packages, SDK feed identity/resolver flags, and a digest of lockfile pins; update
cleanto invalidate install stamps.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/stamps.rs | Adds sysroot stamp input plumbing (effective package hashing + lock pin digest) and extends hash coverage; updates/expands unit tests. |
| src/utils/prerequisites.rs | Introduces StampBatch + read_stamps_batch to run one container invocation for multiple stamp reads (local or --runs-on). |
| src/commands/sdk/package.rs | Switches SDK prerequisite stamp checking to read_stamps_batch. |
| src/commands/sdk/install.rs | Prefetches rootfs/initramfs install stamps in one batched read and passes them into sysroot install to enable skipping. |
| src/commands/sdk/clean.rs | Switches SDK prerequisite stamp checking to read_stamps_batch. |
| src/commands/rootfs/install.rs | Implements sysroot install skipping via prefetched stamp + stronger inputs; fixes removal detection; refactors clean; adjusts lock saving responsibility. |
| src/commands/rootfs/clean.rs | Ensures clean removes the sysroot’s install stamp directory as well as the sysroot. |
| src/commands/initramfs/install.rs | Adds prefetched install-stamp read and moves lock saving responsibility to the command. |
| src/commands/ext/clean.rs | Switches SDK prerequisite stamp checking to read_stamps_batch. |
| src/commands/ext/checkout.rs | Switches stamp checking to read_stamps_batch for prerequisites. |
| src/commands/build.rs | Switches prerequisite stamp checking to read_stamps_batch. |
| CHANGELOG.md | Documents the fixed reinstall behavior, stamp-based skipping, and lock pin correctness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist the lockfile the install updated. `install_sysroot` no | ||
| // longer saves for itself — under `avocado sdk install` it runs on a | ||
| // clone that the caller merges and saves once. | ||
| if result.is_ok() { | ||
| lock_file.save(src_dir)?; | ||
| } |
| // Persist the lockfile the install updated — `install_sysroot` leaves | ||
| // saving to its caller. | ||
| if result.is_ok() { | ||
| lock_file.save(src_dir)?; | ||
| } |
| RunConfig { | ||
| container_image: container_image.to_string(), | ||
| target: target.to_string(), | ||
| verbose: self.verbose, |
There was a problem hiding this comment.
Nit (consistency): this stamp-read RunConfig sets verbose: self.verbose, which flips AVOCADO_VERBOSE=1 for what is just an echo … | tr script. It's harmless — the output is captured (not echoed on success) and parse_batch_stamps_output ignores any non-::: lines — but it's inconsistent with the other reads now routed through read_stamps_batch (check_prerequisites, sdk clean, sdk package, ext/*), which leave verbose at its false default. Same applies to the read_sysroot_install_stamp config in the standalone rootfs/initramfs paths. Suggest dropping it here for consistency.
| // Escape hatches: `--no-stamps` skips the read (and the write) so it | ||
| // always reinstalls, and `avocado {rootfs,initramfs} clean` removes the | ||
| // stamp along with the sysroot, so a cleaned sysroot never skips. | ||
| if !params.no_stamps { |
There was a problem hiding this comment.
Nit (cosmetic): the skip short-circuit nests three if/if let. A let-chain (if !params.no_stamps && let Some(stamp) = params.prefetched_stamp.as_ref() && let Some(inputs) = compute_install_stamp_inputs(params, &packages)?) or a small extracted helper would read a touch flatter. Purely stylistic — feel free to ignore.
Problem
avocado sdk installredid the rootfs and initramfs installs in full onevery invocation, even when nothing had changed. Two independent defects
caused it.
1. False "packages removed" detection wiped both sysroots every run.
detect_sysroot_package_removalscompared config-declared packagenames against locked names and returned true — meaning
rm -rf $AVOCADO_PREFIX/<sysroot>plus a from-scratch reinstall — whenever thelock held names the config didn't.
But the install auto-appends two packages the config never declares, then
records them in the lock:
packagegroup-avocado-{rootfs,initramfs}-modules-<kver>kernel-image-<kver>(rootfs only)From the second run onward those always read as removed. Because
get_rootfs_packages/get_initramfs_packagesdefault to the singlemeta-package, this fired on every project, not only ones overriding
rootfs.packages. Confirmed against every real lockfile in the tree —for example
references/jetson-trt/avocado.lock:avocado-pkg-rootfs,kernel-image-6.8.12-…,packagegroup-avocado-rootfs-modules-6.8.12-…avocado-pkg-rootfsavocado-pkg-initramfs,packagegroup-avocado-initramfs-modules-6.8.12-…avocado-pkg-initramfsSecond-order effect: the same path called
remove_packages_from_sysroot, dropping those packages' version pinsfrom the in-memory lock.
build_package_spec_with_lockthen fell back toa bare package name, so dnf resolved the kernel image and module
packagegroup to newest available instead of the pinned NVR.
avocado.locklooked pinned on disk (it is rewritten post-install) butthe pin never applied.
2. The install stamps were written but never read.
install_sysrootwrote
rootfs/install.stampandinitramfs/install.stamp, but nothingconsulted them to skip work — across the whole CLI, stamps were only
prerequisite gates. So even with defect 1 fixed, each run still paid a
kernel repoquery, off-kernel exclude computation, a repoquery existence
probe, the dnf transaction,
query_installed_packages, kernel-sysrootstaging, and the stamp write.
Solution
An unchanged config now does zero container work for rootfs/initramfs; a
changed one reinstalls exactly the sysroot(s) affected;
avocado.lockpins actually bind.
Key changes
Compare removals against the effective set.
install_sysrootresolves the kernel and computes the auto-appended names before removal
detection, and
detect_sysroot_package_removalsnow takes the effectivename set (config packages ∪ auto-appends) and returns the removed names
rather than a bool. That set is built once and reused as the post-install
lock query list, replacing a second, duplicate computation. A kver change
falls out of the same mechanism, so the dedicated kernel-pin-change block
became the reason for the clean rather than a second code path, and the
two verbatim inline
rm -rfRunConfig blocks collapse into oneclean_sysroothelper built from the existingclean_sysroot_command.Skip the install when the stamp is current. New
read_stamps_batch/StampBatchinutils/prerequisites.rswrapsgenerate_batch_read_stamps_script+ one container run + parsing.sdk installreads both sysroot stamps in a single invocation before theparallel phase and passes them in via
SysrootInstallParams; thestandalone
rootfs install/initramfs installread their own throughthe same helper.
install_sysrootshort-circuits before any containercall. Stamps live in the
/opt/_avocadonamed volume rather than a hostbind mount, so a container run is unavoidable — hence batching.
check_prerequisitesand five sites that hand-rolled the same RunConfig(
build.rs,sdk/{clean,package}.rs,ext/{clean,checkout}.rs) areported to the helper.
Make the hash trustworthy enough to skip on. The two per-sysroot
hash functions collapse into one core that now hashes the effective
package map (so an absent
rootfs.packagesand an explicit default hashidentically), plus
sdk.repo_url,sdk.repo_release— the hook thatmakes a snapshot bump or
avocado updateland — andsdk.disable_weak_dependencies.StampInputs.package_list_hashispopulated with a digest of that sysroot's lockfile pins, so a hand-edited
avocado.lockor anavocado unlockgoes stale automatically. An emptypin set hashes to its own value rather than to
None, becauseStamp::is_currentonly compares that field when both sides carry one —otherwise a cleared lock would compare equal by omission.
STAMP_VERSIONis deliberately not bumped: that would invalidate everySDK/ext/runtime stamp. Changing only these two hash bodies makes existing
rootfs/initramfs stamps mismatch naturally, costing one reinstall.
Make
cleaninvalidate its stamp.clean_sysroot_commandalsoremoves
$AVOCADO_PREFIX/.stamps/<sysroot>, mirroringext clean.Without it,
rootfs clean && sdk installwould skip against a currentstamp and leave an empty sysroot.
Drop the mid-flight lockfile saves.
install_sysrootandstage_kernel_sysroot_from_rootfssaved their lockfile clone while therootfs and initramfs tasks ran concurrently on separate clones —
last-writer-wins, healed only by the caller's final save. Saving is now
the caller's job throughout, which removes the race.
Escape hatches are unchanged in spirit:
--no-stampsdisables the readalong with the write, so it always reinstalls, and
avocado rootfs cleanremains the hard reset.
Reviewer notes
cargo fmt --check,cargo clippy --all-targets -- -D warnings, and thefull
cargo testsuite are green (1124 lib tests plus the integrationsuites, no failures).
New unit tests:
detect_sysroot_package_removalsreports nothing for the exactreferences/jetson-trt/avocado.lockshape — a default config with bothauto-appended kernel packages locked. This is the regression test for the
bug.
previous kver's auto-appends as stale after a repin.
rootfs.packages, and moves on an added package, onrepo_url,repo_releaseanddisable_weak_dependencies, and on a lock re-pin.package_list_hashis order-independent (pins come out of aHashMap),and an
avocado unlockinvalidatesis_current.End-to-end verification against a live feed is in progress separately.