From 0f8ce4c363d227912a772ae0f9c11bcd58d92258 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 3 Nov 2021 17:23:05 -0400 Subject: [PATCH 1/4] containers: Expose a state struct with base commit and layering state This is actually quite analogous to what rpm-ostree does internally; a lot of the internals there distinguish "base" versus "layered" commits, and this is very similar. Here, when we import a non-layered image, we can mostly ignore the merge commit since all it has is the image manifest. The base image layer ref *is* the encapsulated ostree commit with all the metadata injected by (rpm-)ostree in the non-layered case. Add an API which exposes this as a struct, and also return it from the importer's `AlreadyPresent` case. --- lib/src/cli.rs | 2 +- lib/src/container/deploy.rs | 2 +- lib/src/container/store.rs | 98 ++++++++++++++++++++++++++++--------- lib/tests/it/main.rs | 6 +-- 4 files changed, 81 insertions(+), 27 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 57e7e084..6dd02432 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -334,7 +334,7 @@ async fn container_store(repo: &str, imgref: &OstreeImageReference) -> Result<() let mut imp = LayeredImageImporter::new(repo, &imgref).await?; let prep = match imp.prepare().await? { PrepareResult::AlreadyPresent(c) => { - println!("No changes in {} => {}", imgref, c); + println!("No changes in {} => {}", imgref, c.merge_commit); return Ok(()); } PrepareResult::Ready(r) => r, diff --git a/lib/src/container/deploy.rs b/lib/src/container/deploy.rs index d6de68cc..76b8b018 100644 --- a/lib/src/container/deploy.rs +++ b/lib/src/container/deploy.rs @@ -11,7 +11,7 @@ pub const ORIGIN_CONTAINER: &str = "container-image-reference"; async fn pull_idempotent(repo: &ostree::Repo, imgref: &OstreeImageReference) -> Result { let mut imp = super::store::LayeredImageImporter::new(repo, imgref).await?; match imp.prepare().await? { - PrepareResult::AlreadyPresent(r) => Ok(r), + PrepareResult::AlreadyPresent(r) => Ok(r.merge_commit), PrepareResult::Ready(prep) => Ok(imp.import(prep).await?.commit), } } diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index 72a70740..a648e076 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -10,7 +10,7 @@ use crate::refescape; use anyhow::{anyhow, Context}; use containers_image_proxy::{ImageProxy, OpenedImage}; use fn_error_context::context; -use oci_spec::image as oci_image; +use oci_spec::image::{self as oci_image, ImageManifest}; use ostree::prelude::{Cast, ToVariant}; use ostree::{gio, glib}; use std::collections::{BTreeMap, HashMap}; @@ -40,6 +40,19 @@ fn ref_for_image(l: &ImageReference) -> Result { refescape::prefix_escape_for_ref(IMAGE_PREFIX, &l.to_string()) } +/// State of an already pulled layered image. +#[derive(Debug, PartialEq, Eq)] +pub struct LayeredImageState { + /// The base ostree commit + pub base_commit: String, + /// The merge commit unions all layers + pub merge_commit: String, + /// Whether or not the image has multiple layers. + pub is_layered: bool, + /// The digest of the original manifest + pub manifest_digest: String, +} + /// Context for importing a container image. pub struct LayeredImageImporter { repo: ostree::Repo, @@ -52,7 +65,7 @@ pub struct LayeredImageImporter { /// Result of invoking [`LayeredImageImporter::prepare`]. pub enum PrepareResult { /// The image reference is already present; the contained string is the OSTree commit. - AlreadyPresent(String), + AlreadyPresent(LayeredImageState), /// The image needs to be downloaded Ready(Box), } @@ -179,26 +192,27 @@ impl LayeredImageImporter { let new_imageid = manifest.config().digest().as_str(); // Query for previous stored state - let (previous_manifest_digest, previous_imageid) = if let Some(merge_commit) = - self.repo.resolve_rev(&self.ostree_ref, true)? - { - let merge_commit_obj = &self.repo.load_commit(merge_commit.as_str())?.0; - let commit_meta = &merge_commit_obj.child_value(0); - let commit_meta = &ostree::glib::VariantDict::new(Some(commit_meta)); - let (previous_manifest, previous_digest) = manifest_data_from_commitmeta(commit_meta)?; - // If the manifest digests match, we're done. - if previous_digest == manifest_digest { - return Ok(PrepareResult::AlreadyPresent(merge_commit.to_string())); - } - // Failing that, if they have the same imageID, we're also done. - let previous_imageid = previous_manifest.config().digest().as_str(); - if previous_imageid == new_imageid { - return Ok(PrepareResult::AlreadyPresent(merge_commit.to_string())); - } - (Some(previous_digest), Some(previous_imageid.to_string())) - } else { - (None, None) - }; + + let (previous_manifest_digest, previous_imageid) = + if let Some((previous_manifest, previous_state)) = + query_image_impl(&self.repo, &self.imgref)? + { + // If the manifest digests match, we're done. + if previous_state.manifest_digest == manifest_digest { + return Ok(PrepareResult::AlreadyPresent(previous_state)); + } + // Failing that, if they have the same imageID, we're also done. + let previous_imageid = previous_manifest.config().digest().as_str(); + if previous_imageid == new_imageid { + return Ok(PrepareResult::AlreadyPresent(previous_state)); + } + ( + Some(previous_state.manifest_digest), + Some(previous_imageid.to_string()), + ) + } else { + (None, None) + }; let mut layers = manifest.layers().iter().cloned(); // We require a base layer. @@ -355,6 +369,46 @@ pub fn list_images(repo: &ostree::Repo) -> Result> { .collect() } +fn query_image_impl( + repo: &ostree::Repo, + imgref: &OstreeImageReference, +) -> Result> { + let ostree_ref = &ref_for_image(&imgref.imgref)?; + let merge_rev = repo.resolve_rev(&ostree_ref, true)?; + let (merge_commit, merge_commit_obj) = if let Some(r) = merge_rev { + (r.to_string(), repo.load_commit(r.as_str())?.0) + } else { + return Ok(None); + }; + let commit_meta = &merge_commit_obj.child_value(0); + let commit_meta = &ostree::glib::VariantDict::new(Some(commit_meta)); + let (manifest, manifest_digest) = manifest_data_from_commitmeta(commit_meta)?; + let mut layers = manifest.layers().iter().cloned(); + // We require a base layer. + let base_layer = layers.next().ok_or_else(|| anyhow!("No layers found"))?; + let base_layer = query_layer(repo, base_layer)?; + let base_commit = base_layer + .commit + .ok_or_else(|| anyhow!("Missing base image ref"))?; + // If there are more layers after the base, then we're layered. + let is_layered = layers.count() > 0; + let state = LayeredImageState { + base_commit, + merge_commit, + is_layered, + manifest_digest, + }; + Ok(Some((manifest, state))) +} + +/// Query metadata for a pulled image. +pub fn query_image( + repo: &ostree::Repo, + imgref: &OstreeImageReference, +) -> Result> { + Ok(query_image_impl(repo, imgref)?.map(|v| v.1)) +} + /// Copy a downloaded image from one repository to another. pub async fn copy( src_repo: &ostree::Repo, diff --git a/lib/tests/it/main.rs b/lib/tests/it/main.rs index ad82e961..72368c83 100644 --- a/lib/tests/it/main.rs +++ b/lib/tests/it/main.rs @@ -466,7 +466,7 @@ async fn test_container_write_derive() -> Result<()> { panic!("Should have already imported {}", import.ostree_ref) } }; - assert_eq!(import.commit, already_present); + assert_eq!(import.commit, already_present.merge_commit); // Test upgrades; replace the oci-archive with new content. std::fs::write(exampleos_path, EXAMPLEOS_DERIVED_V2_OCI)?; @@ -486,7 +486,7 @@ async fn test_container_write_derive() -> Result<()> { } let import = imp.import(prep).await?; // New commit. - assert_ne!(import.commit, already_present); + assert_ne!(import.commit, already_present.merge_commit); // We should still have exactly one image stored. let images = ostree_ext::container::store::list_images(&fixture.destrepo)?; assert_eq!(images.len(), 1); @@ -513,7 +513,7 @@ async fn test_container_write_derive() -> Result<()> { panic!("Should have already imported {}", import.ostree_ref) } }; - assert_eq!(import.commit, already_present); + assert_eq!(import.commit, already_present.merge_commit); // Create a new repo, and copy to it let destrepo2 = ostree::Repo::create_at( From bfaab1d6721ecd9ab7425947279cc78758924df3 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 3 Nov 2021 17:53:23 -0400 Subject: [PATCH 2/4] container: Change import result case to contain state struct Notably, this also stops exposing the ostree ref for the merge commit, which I think is a good idea in general since it should be thought of more as an implementation detail. In other words, this module speaks container image references and ostree commits. --- lib/src/cli.rs | 5 +---- lib/src/container/deploy.rs | 9 +++++---- lib/src/container/store.rs | 19 ++++++++++--------- lib/tests/it/main.rs | 19 +++++++++++-------- 4 files changed, 27 insertions(+), 25 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 6dd02432..31b84d5d 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -366,10 +366,7 @@ async fn container_store(repo: &str, imgref: &OstreeImageReference) -> Result<() } } } - println!( - "Wrote: {} => {} => {}", - imgref, import.ostree_ref, import.commit - ); + println!("Wrote: {} => {}", imgref, import.state.merge_commit); Ok(()) } diff --git a/lib/src/container/deploy.rs b/lib/src/container/deploy.rs index 76b8b018..684cfb0b 100644 --- a/lib/src/container/deploy.rs +++ b/lib/src/container/deploy.rs @@ -10,10 +10,11 @@ pub const ORIGIN_CONTAINER: &str = "container-image-reference"; async fn pull_idempotent(repo: &ostree::Repo, imgref: &OstreeImageReference) -> Result { let mut imp = super::store::LayeredImageImporter::new(repo, imgref).await?; - match imp.prepare().await? { - PrepareResult::AlreadyPresent(r) => Ok(r.merge_commit), - PrepareResult::Ready(prep) => Ok(imp.import(prep).await?.commit), - } + let state = match imp.prepare().await? { + PrepareResult::AlreadyPresent(r) => r, + PrepareResult::Ready(prep) => imp.import(prep).await?.state, + }; + Ok(state.merge_commit) } /// Options configuring deployment. diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index a648e076..f36e8e1f 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -112,10 +112,8 @@ pub struct PreparedImport { /// A successful import of a container image. #[derive(Debug, PartialEq, Eq)] pub struct CompletedImport { - /// The ostree ref used for the container image. - pub ostree_ref: String, - /// The current commit. - pub commit: String, + /// The completed layered image state + pub state: LayeredImageState, /// A mapping from layer blob IDs to a count of content filtered out /// by toplevel path. pub layer_filtered_content: BTreeMap>, @@ -312,8 +310,9 @@ impl LayeredImageImporter { // Destructure to transfer ownership to thread let repo = self.repo; let target_ref = self.ostree_ref; - let (ostree_ref, commit) = crate::tokio_util::spawn_blocking_cancellable( - move |cancellable| -> Result<(String, String)> { + let imgref = self.imgref; + let state = crate::tokio_util::spawn_blocking_cancellable( + move |cancellable| -> Result { let cancellable = Some(cancellable); let repo = &repo; let txn = repo.auto_transaction(cancellable)?; @@ -344,13 +343,15 @@ impl LayeredImageImporter { )?; repo.transaction_set_ref(None, &target_ref, Some(merged_commit.as_str())); txn.commit(cancellable)?; - Ok((target_ref, merged_commit.to_string())) + // Here we re-query state just to run through the same code path, + // though it'd be cheaper to synthesize it from the data we already have. + let state = query_image(&repo, &imgref)?.unwrap(); + Ok(state) }, ) .await??; Ok(CompletedImport { - ostree_ref, - commit, + state, layer_filtered_content, }) } diff --git a/lib/tests/it/main.rs b/lib/tests/it/main.rs index 72368c83..fb20b481 100644 --- a/lib/tests/it/main.rs +++ b/lib/tests/it/main.rs @@ -444,7 +444,10 @@ async fn test_container_write_derive() -> Result<()> { assert_eq!(images.len(), 1); assert_eq!(images[0], exampleos_ref.imgref.to_string()); - let imported_commit = &fixture.destrepo.load_commit(import.commit.as_str())?.0; + let imported_commit = &fixture + .destrepo + .load_commit(import.state.merge_commit.as_str())? + .0; let digest = ostree_ext::container::store::manifest_digest_from_commit(imported_commit)?; assert!(digest.starts_with("sha256:")); assert_eq!(digest, expected_digest); @@ -453,7 +456,7 @@ async fn test_container_write_derive() -> Result<()> { bash!( "ostree --repo={repo} ls {r} /usr/share/anewfile", repo = fixture.destrepo_path.as_str(), - r = import.ostree_ref.as_str() + r = import.state.merge_commit.as_str() )?; // Import again, but there should be no changes. @@ -463,10 +466,10 @@ async fn test_container_write_derive() -> Result<()> { let already_present = match imp.prepare().await? { PrepareResult::AlreadyPresent(c) => c, PrepareResult::Ready(_) => { - panic!("Should have already imported {}", import.ostree_ref) + panic!("Should have already imported {}", &exampleos_ref) } }; - assert_eq!(import.commit, already_present.merge_commit); + assert_eq!(import.state.merge_commit, already_present.merge_commit); // Test upgrades; replace the oci-archive with new content. std::fs::write(exampleos_path, EXAMPLEOS_DERIVED_V2_OCI)?; @@ -486,7 +489,7 @@ async fn test_container_write_derive() -> Result<()> { } let import = imp.import(prep).await?; // New commit. - assert_ne!(import.commit, already_present.merge_commit); + assert_ne!(import.state.merge_commit, already_present.merge_commit); // We should still have exactly one image stored. let images = ostree_ext::container::store::list_images(&fixture.destrepo)?; assert_eq!(images.len(), 1); @@ -500,7 +503,7 @@ async fn test_container_write_derive() -> Result<()> { fi ", repo = fixture.destrepo_path.as_str(), - r = import.ostree_ref.as_str() + r = import.state.merge_commit.as_str() )?; // And there should be no changes on upgrade again. @@ -510,10 +513,10 @@ async fn test_container_write_derive() -> Result<()> { let already_present = match imp.prepare().await? { PrepareResult::AlreadyPresent(c) => c, PrepareResult::Ready(_) => { - panic!("Should have already imported {}", import.ostree_ref) + panic!("Should have already imported {}", &exampleos_ref) } }; - assert_eq!(import.commit, already_present.merge_commit); + assert_eq!(import.state.merge_commit, already_present.merge_commit); // Create a new repo, and copy to it let destrepo2 = ostree::Repo::create_at( From c9bdb94d19235eda1161801acf05590a66823dbb Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 3 Nov 2021 20:43:06 -0400 Subject: [PATCH 3/4] container/deploy: Use base commit if we're not layered If we're not doing a layered image, then use the base commit for the deployment. Closes: https://github.com/ostreedev/ostree-rs-ext/issues/143 --- lib/src/container/deploy.rs | 16 ++++++---------- lib/src/container/store.rs | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/src/container/deploy.rs b/lib/src/container/deploy.rs index 684cfb0b..3b7e058d 100644 --- a/lib/src/container/deploy.rs +++ b/lib/src/container/deploy.rs @@ -8,15 +8,6 @@ use ostree::glib; /// The key in the OSTree origin which holds a serialized [`super::OstreeImageReference`]. pub const ORIGIN_CONTAINER: &str = "container-image-reference"; -async fn pull_idempotent(repo: &ostree::Repo, imgref: &OstreeImageReference) -> Result { - let mut imp = super::store::LayeredImageImporter::new(repo, imgref).await?; - let state = match imp.prepare().await? { - PrepareResult::AlreadyPresent(r) => r, - PrepareResult::Ready(prep) => imp.import(prep).await?.state, - }; - Ok(state.merge_commit) -} - /// Options configuring deployment. #[derive(Debug, Default)] pub struct DeployOpts<'a> { @@ -45,7 +36,12 @@ pub async fn deploy<'opts>( let cancellable = ostree::gio::NONE_CANCELLABLE; let options = options.unwrap_or_default(); let repo = &sysroot.repo().unwrap(); - let commit = &pull_idempotent(repo, imgref).await?; + let mut imp = super::store::LayeredImageImporter::new(repo, imgref).await?; + let state = match imp.prepare().await? { + PrepareResult::AlreadyPresent(r) => r, + PrepareResult::Ready(prep) => imp.import(prep).await?.state, + }; + let commit = state.get_commit(); let origin = glib::KeyFile::new(); let target_imgref = options.target_imgref.unwrap_or(imgref); origin.set_string("origin", ORIGIN_CONTAINER, &target_imgref.to_string()); diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index f36e8e1f..ba8631a9 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -53,6 +53,22 @@ pub struct LayeredImageState { pub manifest_digest: String, } +impl LayeredImageState { + /// Return the default ostree commit digest for this image. + /// + /// If this is a non-layered image, the merge commit will be + /// ignored, and the base commit returned. + /// + /// Otherwise, this returns the merge commit. + pub fn get_commit(&self) -> &str { + if self.is_layered { + self.merge_commit.as_str() + } else { + self.base_commit.as_str() + } + } +} + /// Context for importing a container image. pub struct LayeredImageImporter { repo: ostree::Repo, From 35d3528c5f4c65082c4398b0744e764904690b21 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 3 Nov 2021 20:59:03 -0400 Subject: [PATCH 4/4] container/deploy: Also write ref with target if provided With this new emphasis on "dual commit objects" for the container deployment, the higher level code queries via container image references and we don't expose the ostree ref (since there is no longer a single one). This makes it critical to write the internal ref matching the target container image, because it now needs to match the origin. --- lib/src/container/deploy.rs | 3 +++ lib/src/container/store.rs | 17 +++++++++++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/src/container/deploy.rs b/lib/src/container/deploy.rs index 3b7e058d..8d5d17e9 100644 --- a/lib/src/container/deploy.rs +++ b/lib/src/container/deploy.rs @@ -37,6 +37,9 @@ pub async fn deploy<'opts>( let options = options.unwrap_or_default(); let repo = &sysroot.repo().unwrap(); let mut imp = super::store::LayeredImageImporter::new(repo, imgref).await?; + if let Some(target) = options.target_imgref { + imp.set_target(target); + } let state = match imp.prepare().await? { PrepareResult::AlreadyPresent(r) => r, PrepareResult::Ready(prep) => imp.import(prep).await?.state, diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index ba8631a9..fea17384 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -74,8 +74,8 @@ pub struct LayeredImageImporter { repo: ostree::Repo, proxy: ImageProxy, imgref: OstreeImageReference, + target_imgref: Option, proxy_img: OpenedImage, - ostree_ref: String, } /// Result of invoking [`LayeredImageImporter::prepare`]. @@ -176,16 +176,20 @@ impl LayeredImageImporter { let proxy = ImageProxy::new().await?; let proxy_img = proxy.open_image(&imgref.imgref.to_string()).await?; let repo = repo.clone(); - let ostree_ref = ref_for_image(&imgref.imgref)?; Ok(LayeredImageImporter { repo, proxy, proxy_img, - ostree_ref, + target_imgref: None, imgref: imgref.clone(), }) } + /// Write cached data as if the image came from this source. + pub fn set_target(&mut self, target: &OstreeImageReference) { + self.target_imgref = Some(target.clone()) + } + /// Determine if there is a new manifest, and if so return its digest. #[context("Fetching manifest")] pub async fn prepare(&mut self) -> Result { @@ -252,6 +256,8 @@ impl LayeredImageImporter { /// Import a layered container image pub async fn import(self, import: Box) -> Result { let proxy = self.proxy; + let target_imgref = self.target_imgref.as_ref().unwrap_or(&self.imgref); + let ostree_ref = ref_for_image(&target_imgref.imgref)?; // First download the base image (if necessary) - we need the SELinux policy // there to label all following layers. let base_layer = import.base_layer; @@ -325,8 +331,7 @@ impl LayeredImageImporter { // Destructure to transfer ownership to thread let repo = self.repo; - let target_ref = self.ostree_ref; - let imgref = self.imgref; + let imgref = self.target_imgref.unwrap_or(self.imgref); let state = crate::tokio_util::spawn_blocking_cancellable( move |cancellable| -> Result { let cancellable = Some(cancellable); @@ -357,7 +362,7 @@ impl LayeredImageImporter { &merged_root, cancellable, )?; - repo.transaction_set_ref(None, &target_ref, Some(merged_commit.as_str())); + repo.transaction_set_ref(None, &ostree_ref, Some(merged_commit.as_str())); txn.commit(cancellable)?; // Here we re-query state just to run through the same code path, // though it'd be cheaper to synthesize it from the data we already have.