Add support for --from-delta for bootc switch and update - #2449
alexlarsson wants to merge 6 commits into
Conversation
b57b08a to
e99dc64
Compare
The diff_id appended for the generated layer was the bare hex encoding rather than the `sha256:...` form the spec requires. Our own importer does not look at it, so nothing noticed, but any other consumer of an image built by this helper trips over it. Assisted-by: AI Signed-off-by: Alexander Larsson <alexl@redhat.com>
e99dc64 to
033dc0d
Compare
|
New version that is not depending on the new composefs-rs, instead using the separated out delta parsing from the rust code in containers/oci-delta. |
This adds a `--from-delta PATH` flag to `bootc upgrade` and `bootc switch` that applies an oci-delta in one step. This commit add support only for the composefs backend, and uses the native delta pull support in composefs-rs. Later commits add ostree backend support. Basic parsing of the delta file is done by the oci-delta rust bindings. See https://github.com/containers/oci-delta for details of the format. Unified storage cannot apply a delta yet and will fail. Real support for this would require delta support in containers/storage. Generated-by: AI Signed-off-by: Alexander Larsson <alexl@redhat.com>
The importer has always fetched both the image metadata and the layer bytes over the same containers-image-proxy connection. To apply an OCI delta we need to split those apart: the manifest and config come out of the delta file, and each layer is either already in the repository or reconstructed locally from a binary patch. No network access at all. Introduce a `LayerSource` trait, which is just the existing `fetch_layer` signature behind a trait object, and a `ProxyLayerSource` implementing today's behaviour. `PreparedImport` now carries a LayerSource instead of a bare `OpenedImage`, and `ImageImporter::prepare_from_manifest` builds a `PreparedImport` from a manifest and config the caller already has, with the layers coming from whatever source it passes in. `ImageImporter::new_without_proxy` skips spawning skopeo entirely, so an importer that will never touch the network does not need it installed. Along the way, `prepare_internal` splits into `check_sigverify` and `diff_previous_state` so both prepare paths share the "is this already imported?" logic, and `cache_pending` moves into `create_prepared_import` where both paths reach it. `ProxyLayerSource` also fetches the proxy's layer info once and caches it, where before both `unencapsulate_base()` and `import()` asked for it separately. Assisted-by: AI Signed-off-by: Alexander Larsson <alexl@redhat.com>
We will need this later. We're reusing most of the old list_container_deployment_manifests() for this. Signed-off-by: Alexander Larsson <alexl@redhat.com>
`bootc upgrade --from-delta` and `bootc switch --from-delta` worked only with the composefs backend. Wire them up for ostree too. A tar-diff patch reads the source image by *path*, not from the original tar byte stream, so the source does not have to be a container layer: an ostree commit holding the same filesystem works just as well. `OstreeDataSource` serves file content out of a commit, and `DeltaLayerSource` plugs that into the container importer in place of the image proxy, so nothing is fetched. ostree does not store an image's root filesystem verbatim - the tar importer moves `/etc` to `/usr/etc`, and `/var` to `/usr/share/factory/var` on ostree older than v2024.3 - so a source path is tried at each of the locations that importer could have put it. Each reconstructed layer is streamed to the importer over a pipe, with the reconstruction itself running as the driver future that `join_fetch` already polls concurrently, so a multi-GB base layer never has to be spooled to disk. The source image is looked up by config digest among the images in the repository, and an absent source is fatal: there is no guarantee of a network connection at the point a delta is applied, so falling back to the registry would defeat the purpose. Byte-level progress is not reported for delta layers: the reconstructed bytes are uncompressed, so counting them against the descriptor's compressed size would overshoot. Per-layer start/completion still is. A layer's ref is written as the layer is unpacked, i.e. before the driver future has had the chance to report a diff_id mismatch, so a failed apply can leave a ref to unverified content that a retry would reuse. Prune the unreferenced layers after a failed delta apply. Also, `switch --from-delta` no longer short-circuits when the image specification is unchanged - a delta names one specific target digest, and switching to the reference you are already tracking is exactly how it is normally used. Assisted-by: AI Signed-off-by: Alexander Larsson <alexl@redhat.com>
Three tests covering the whole path from a delta on disk to an imported image, at increasing degrees of realism: `test_apply_delta_whole_layers` builds a delta whose patches are the target layers carried verbatim. The format allows that, and the layer reconstruction dispatches on media type, so this exercises parsing, validation, source lookup and the import while needing no external tooling - it runs everywhere. `test_apply_delta_chunked` and `test_apply_delta_derived` use the real `oci-delta` to build the delta, and skip themselves when it is not installed. The first covers an ostree-native image, whose layers are made of repo objects under `sysroot/ostree/` that do not exist in the commit's file tree - it passes only because oci-delta is told never to ask for one. The second covers a derived layer, an ordinary root filesystem tar, which is the case that actually drives `OstreeDataSource`; the file it patches lives at `/etc/bigconf`, i.e. at a path the importer relocates, and the test asserts both that it lands at `/usr/etc/bigconf` and that the patch is far smaller than the layer, which it can only be if the content really was read back out of the source commit. `oci-delta` goes in a new optional package list, because it is not available on every distribution we build on. Assisted-by: AI Signed-off-by: Alexander Larsson <alexl@redhat.com>
033dc0d to
eb615f0
Compare
cgwalters
left a comment
There was a problem hiding this comment.
Just an initial skim overall looks sane
| } | ||
|
|
||
| /// Reject `--from-delta` for an image that also has to be in containers-storage. | ||
| pub(crate) fn reject_unified_storage(delta: &Delta, use_unified: bool) -> Result<()> { |
There was a problem hiding this comment.
So...we are going to need a plan to make this work though...
I think the most viable thing will be extending podman-container-tools/container-libs#651 to support pushes?
I guess we can merge this as is and just add "fix deltas" to the unified storage tracker TODO (can you do that as part of this PR?)
| } | ||
|
|
||
| /// Check the delta's internal consistency, to fail early | ||
| fn validate(p: &ParsedDelta) -> Result<()> { |
There was a problem hiding this comment.
There was a problem hiding this comment.
(Also this should be part of impl ParsedDelta right)
| let config = ImageConfiguration::from_reader(&p.target_config_raw[..]) | ||
| .context("Parsing embedded target config")?; | ||
| let layers = p.target_manifest.layers(); | ||
| ensure!(!layers.is_empty(), "Delta target image has no layers"); |
There was a problem hiding this comment.
Like on this topic there is https://docs.rs/nonempty/0.12.0/nonempty/ for example (and arguably we should use that in oci-spec by default for images, or at least have a standard falliable accessor)
| } | ||
|
|
||
| /// Verify that `data` hashes to `expected`. | ||
| fn verify_digest(what: &str, data: &[u8], expected: &Digest) -> Result<()> { |
There was a problem hiding this comment.
I think what should be an impl Read in the general case for this, I'd be surprised if we don't have a variant of this somewhere
| delta.validate_image_reference(spec_imgref)?; | ||
| crate::delta::reject_unified_storage(delta, use_unified)?; | ||
| if !quiet { | ||
| println!("Applying delta {}", delta.describe()); |
There was a problem hiding this comment.
This needs to use the progress API.
We may need a blanket ban on (e)println! in crates/lib
println!() panics if stdout is closed (e.g. a broken pipe when piping into head), whereas writeln!() lets us propagate the error normally. These call sites are the actual output of a subcommand (digests, JSON, tables, GC stats), so an explicit stdout writer is the right thing anyway. Prep for denying clippy::print_stdout in crates/lib, see bootc-dev#2449 (comment) Generated-by: AI
fsck() already takes an explicit writer for its errors, but the "ok: <check>" lines bypassed it and went straight to stdout. The only caller passes stdout, so the output is unchanged; this just makes the function honor its own API. Prep for denying clippy::print_stdout in crates/lib, see bootc-dev#2449 (comment) Generated-by: AI
Subcommand implementations such as install, upgrade, switch and
rollback print status lines for a human at a terminal ("Installing bootloader
via bootupd", "Installation complete!", ...). We want to deny direct
println!/eprintln! in the library, but exempting each of these
functions with #[expect] would also exempt everything else in them,
and several are very large.
Instead, put the one exemption on a pair of small helpers and use them
at these call sites, so a new stray println!() anywhere else, including
inside those functions, still trips the lint. Output is unchanged:
cli_status!() writes a line to stdout and cli_warn!() to stderr, just
as the calls they replace did.
Prep for denying clippy::print_stdout in crates/lib, see
bootc-dev#2449 (comment)
Generated-by: AI
Library code writing directly to stdout/stderr is easy to add and hard to notice in review, and it corrupts output for callers that expect something machine-readable (or that want progress routed through ProgressWriter). This came up in bootc-dev#2449 (comment) where a new println!() slipped into the composefs pull path. Deny both lints at the crate root so new call sites fail clippy. They are restriction lints, so the -A clippy::all in the Makefile validate target doesn't touch them and CI enforces the deny as is. Human-facing status output already goes through cli_status!() and cli_warn!(). The few remaining direct prints get a targeted #[expect] with a reason: global_init() runs before tracing is set up, journal_send() reports that the journal itself is failing, and the spinner has a plain-text fallback when there is no tty. Using expect rather than allow means the attribute gets flagged once it is no longer needed. Tests are exempted via allow-print-in-tests in clippy.toml. Generated-by: AI
This adds native support for oci-delta files to bootc, for both the native composefs backend and the ostree backend.
The composefs backend just uses the existing support in composefs-rs, whereas the ostree backend has a bit more custom code. This code uses the generic delta code in composefs-rs to avoid having to reimplement delta stuff as we're applying deltas directly. This depends on composefs/composefs-rs#394 and means we have a WIP commit that points to this MR. The intent is to get some release with that out before merging this PR.
Note: I have not really tested the native composefs codepath, because I didn't manage to get such a setup going. I will try to do this.