Skip to content

Stop reinstalling rootfs and initramfs on every sdk install - #185

Open
mobileoverlord wants to merge 1 commit into
mainfrom
jschneck/fix-sysroot-install-idempotence
Open

Stop reinstalling rootfs and initramfs on every sdk install#185
mobileoverlord wants to merge 1 commit into
mainfrom
jschneck/fix-sysroot-install-idempotence

Conversation

@mobileoverlord

Copy link
Copy Markdown
Contributor

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.

## 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.
Copilot AI review requested due to automatic review settings July 27, 2026 18:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 clean to 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.

Comment on lines +1162 to +1167
// 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)?;
}
Comment on lines +175 to +179
// 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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants