Skip to content

deploy: short-circuit pull if digest pullspec already exists #1380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 23, 2025
Merged

deploy: short-circuit pull if digest pullspec already exists #1380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94f82b4
store: Lazily open target image
cgwalters Jun 23, 2025
fff106e
store: Load previous state earlier
cgwalters Jun 23, 2025
8da5557
store: short-circuit pull if digest pullspec already exists
jlebon Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 72 additions & 29 deletions ostree-ext/src/container/store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use glib::prelude::*;
use oci_spec::image::{
self as oci_image, Arch, Descriptor, Digest, History, ImageConfiguration, ImageManifest,
};
use ocidir::oci_spec::distribution::Reference;
use ostree::prelude::{Cast, FileEnumeratorExt, FileExt, ToVariant};
use ostree::{gio, glib};
use std::collections::{BTreeMap, BTreeSet, HashMap};
Expand Down Expand Up @@ -181,9 +182,10 @@ pub struct ImageImporter {
disable_gc: bool, // If true, don't prune unused image layers
/// If true, require the image has the bootable flag
require_bootable: bool,
/// Do not attempt to contact the network
offline: bool,
/// If true, we have ostree v2024.3 or newer.
ostree_v2024_3: bool,
pub(crate) proxy_img: OpenedImage,

layer_progress: Option<Sender<ImportProgress>>,
layer_byte_progress: Option<tokio::sync::watch::Sender<Option<LayerProgress>>>,
Expand Down Expand Up @@ -241,6 +243,8 @@ pub struct PreparedImport {
pub layers: Vec<ManifestLayerState>,
/// OSTree remote signature verification text, if enabled.
pub verify_text: Option<String>,
/// Our open image reference
proxy_img: OpenedImage,
}

impl PreparedImport {
Expand Down Expand Up @@ -509,17 +513,16 @@ impl ImageImporter {
&format!("Fetching {}", imgref),
);

let proxy_img = proxy.open_image(&imgref.imgref.to_string()).await?;
let repo = repo.clone();
Ok(ImageImporter {
repo,
proxy,
proxy_img,
target_imgref: None,
no_imgref: false,
ostree_v2024_3: ostree::check_version(2024, 3),
disable_gc: false,
require_bootable: false,
offline: false,
imgref: imgref.clone(),
layer_progress: None,
layer_byte_progress: None,
Expand All @@ -538,6 +541,11 @@ impl ImageImporter {
self.no_imgref = true;
}

/// Do not attempt to contact the network
pub fn set_offline(&mut self) {
self.offline = true;
}

/// Require that the image has the bootable metadata field
pub fn require_bootable(&mut self) {
self.require_bootable = true;
Expand Down Expand Up @@ -619,6 +627,7 @@ impl ImageImporter {
config: ImageConfiguration,
previous_state: Option<Box<LayeredImageState>>,
previous_imageid: Option<String>,
proxy_img: OpenedImage,
) -> Result<Box<PreparedImport>> {
let config_labels = super::labels_of(&config);
if self.require_bootable {
Expand Down Expand Up @@ -662,6 +671,7 @@ impl ImageImporter {
ostree_commit_layer: commit_layer,
layers: remaining_layers,
verify_text: None,
proxy_img,
};
Ok(Box::new(imp))
}
Expand All @@ -681,30 +691,63 @@ impl ImageImporter {
_ => {}
}

let (manifest_digest, manifest) = self.proxy.fetch_manifest(&self.proxy_img).await?;
// Check if we have an image already pulled
let previous_state = try_query_image(&self.repo, &self.imgref.imgref)?;

// Parse the target reference to see if it's a digested pull
let target_reference = self.imgref.imgref.name.parse::<Reference>().ok();
let previous_state = if let Some(target_digest) = target_reference
Copy link
Contributor Author

@jlebon jlebon Jun 23, 2025

Choose a reason for hiding this comment

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

Yeah... I know why we do this (to not lose ownership of previous_state). What I had done in my first attempt which was in this same spot was:

let oci_ref = self.imgref.imgref.name.parse::<OciReference>().ok();
if let Some(digest) = oci_ref.as_ref().and_then(|oci_ref| oci_ref.digest()) {
    let digest = Digest::from_str(digest)?;
    if previous_state.as_ref()
        .map(|state| state.manifest_digest == digest)
        .unwrap_or_default()
    {
        // SAFETY: It can't be None if the map().unwrap() above gave
        // true. We do it this way so that we only consume the Option if
        // we return here. That way it can be reused below if not.
        return Ok(PrepareResult::AlreadyPresent(previous_state.unwrap()));
    }
}

which yes, does an unwrap (though is guaranteed safe), but has a cleaner flow.

Copy link
Collaborator

@cgwalters cgwalters Jun 23, 2025

Choose a reason for hiding this comment

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

That version is definitely a lot cleaner to read, but I usually try pretty hard to avoid unwrap().

.as_ref()
.and_then(|v| v.digest())
.map(Digest::from_str)
.transpose()?
{
if let Some(previous_state) = previous_state {
// A digested pull spec, and our existing state matches.
if previous_state.manifest_digest == target_digest {
tracing::debug!("Digest-based pullspec {:?} already present", self.imgref);
return Ok(PrepareResult::AlreadyPresent(previous_state));
}
Some(previous_state)
} else {
None
Copy link
Contributor Author

@jlebon jlebon Jun 23, 2025

Choose a reason for hiding this comment

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

Shouldn't this be previous_state?

Copy link
Collaborator

@cgwalters cgwalters Jun 23, 2025

Choose a reason for hiding this comment

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

No, because we destructured above - here we know previous_state is None.

}
} else {
previous_state
};

if self.offline {
anyhow::bail!("Manifest fetch required in offline mode");
}

let proxy_img = self
.proxy
.open_image(&self.imgref.imgref.to_string())
.await?;

let (manifest_digest, manifest) = self.proxy.fetch_manifest(&proxy_img).await?;
let manifest_digest = Digest::from_str(&manifest_digest)?;
let new_imageid = manifest.config().digest();

// Query for previous stored state

let (previous_state, previous_imageid) =
if let Some(previous_state) = try_query_image(&self.repo, &self.imgref.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_state.manifest.config().digest();
if previous_imageid == new_imageid {
return Ok(PrepareResult::AlreadyPresent(previous_state));
}
let previous_imageid = previous_imageid.to_string();
(Some(previous_state), Some(previous_imageid))
} else {
(None, None)
};
let (previous_state, previous_imageid) = if let Some(previous_state) = previous_state {
// 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_state.manifest.config().digest();
if previous_imageid == new_imageid {
return Ok(PrepareResult::AlreadyPresent(previous_state));
}
let previous_imageid = previous_imageid.to_string();
(Some(previous_state), Some(previous_imageid))
} else {
(None, None)
};

let config = self.proxy.fetch_config(&self.proxy_img).await?;
let config = self.proxy.fetch_config(&proxy_img).await?;

// If there is a currently fetched image, cache the new pending manifest+config
// as detached commit metadata, so that future fetches can query it offline.
Expand All @@ -724,6 +767,7 @@ impl ImageImporter {
config,
previous_state,
previous_imageid,
proxy_img,
)?;
Ok(PrepareResult::Ready(imp))
}
Expand Down Expand Up @@ -756,7 +800,7 @@ impl ImageImporter {
}
return Ok(());
};
let des_layers = self.proxy.get_layer_info(&self.proxy_img).await?;
let des_layers = self.proxy.get_layer_info(&import.proxy_img).await?;
for layer in import.ostree_layers.iter_mut() {
if layer.commit.is_some() {
continue;
Expand All @@ -767,7 +811,7 @@ impl ImageImporter {
}
let (blob, driver, media_type) = fetch_layer(
&self.proxy,
&self.proxy_img,
&import.proxy_img,
&import.manifest,
&layer.layer,
self.layer_byte_progress.as_ref(),
Expand Down Expand Up @@ -814,7 +858,7 @@ impl ImageImporter {
}
let (blob, driver, media_type) = fetch_layer(
&self.proxy,
&self.proxy_img,
&import.proxy_img,
&import.manifest,
&commit_layer.layer,
self.layer_byte_progress.as_ref(),
Expand Down Expand Up @@ -874,7 +918,7 @@ impl ImageImporter {
self.unencapsulate_base(&mut prep, true, false).await?;
// TODO change the imageproxy API to ensure this happens automatically when
// the image reference is dropped
self.proxy.close_image(&self.proxy_img).await?;
self.proxy.close_image(&prep.proxy_img).await?;
// SAFETY: We know we have a commit
let ostree_commit = prep.ostree_commit_layer.unwrap().commit.unwrap();
let image_digest = prep.manifest_digest;
Expand All @@ -899,9 +943,8 @@ impl ImageImporter {
// First download all layers for the base image (if necessary) - we need the SELinux policy
// there to label all following layers.
self.unencapsulate_base(&mut import, false, true).await?;
let des_layers = self.proxy.get_layer_info(&self.proxy_img).await?;
let des_layers = self.proxy.get_layer_info(&import.proxy_img).await?;
let proxy = self.proxy;
let proxy_img = self.proxy_img;
let target_imgref = self.target_imgref.as_ref().unwrap_or(&self.imgref);
let base_commit = import
.ostree_commit_layer
Expand Down Expand Up @@ -935,7 +978,7 @@ impl ImageImporter {
}
let (blob, driver, media_type) = super::unencapsulate::fetch_layer(
&proxy,
&proxy_img,
&import.proxy_img,
&import.manifest,
&layer.layer,
self.layer_byte_progress.as_ref(),
Expand Down Expand Up @@ -989,7 +1032,7 @@ impl ImageImporter {

// TODO change the imageproxy API to ensure this happens automatically when
// the image reference is dropped
proxy.close_image(&proxy_img).await?;
proxy.close_image(&import.proxy_img).await?;

// We're done with the proxy, make sure it didn't have any errors.
proxy.finalize().await?;
Expand Down
54 changes: 54 additions & 0 deletions ostree-ext/tests/it/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use gvariant::aligned_bytes::TryAsAligned;
use gvariant::{Marker, Structure};
use oci_image::ImageManifest;
use oci_spec::image as oci_image;
use ocidir::oci_spec::distribution::Reference;
use ocidir::oci_spec::image::{Arch, DigestAlgorithm};
use ostree_ext::chunking::ObjectMetaSized;
use ostree_ext::container::{store, ManifestDiff};
Expand Down Expand Up @@ -712,6 +713,59 @@ async fn test_export_as_container_nonderived() -> Result<()> {
Ok(())
}

/// Verify that fetches of a digested pull spec don't do networking
#[tokio::test]
async fn test_no_fetch_digested() -> Result<()> {
if !check_skopeo() {
return Ok(());
}
let fixture = Fixture::new_v1()?;
let (src_imgref_oci, expected_digest) = fixture.export_container().await.unwrap();
let mut imp = store::ImageImporter::new(
fixture.destrepo(),
&OstreeImageReference {
sigverify: SignatureSource::ContainerPolicyAllowInsecure,
imgref: src_imgref_oci.clone(),
},
Default::default(),
)
.await
.unwrap();
// Because oci: transport doesn't allow digested pull specs, we pull from OCI, but set the target
// to a registry.
let target_imgref_name = Reference::with_digest(
"quay.io/exampleos".into(),
"example".into(),
expected_digest.to_string(),
);
let target_imgref = ImageReference {
transport: Transport::Registry,
name: target_imgref_name.to_string(),
};
let target_imgref = OstreeImageReference {
sigverify: SignatureSource::ContainerPolicyAllowInsecure,
imgref: target_imgref,
};
imp.set_target(&target_imgref);
let prep = match imp.prepare().await? {
store::PrepareResult::AlreadyPresent(_) => unreachable!(),
store::PrepareResult::Ready(prep) => prep,
};
let r = imp.import(prep).await.unwrap();
assert_eq!(r.manifest_digest, expected_digest);
let mut imp = store::ImageImporter::new(fixture.destrepo(), &target_imgref, Default::default())
.await
.unwrap();
// And the key test, we shouldn't reach out to the registry here
imp.set_offline();
match imp.prepare().await.context("Init prep derived").unwrap() {
store::PrepareResult::AlreadyPresent(_) => {}
store::PrepareResult::Ready(_) => panic!("Should have image already"),
};

Ok(())
}

#[tokio::test]
async fn test_export_as_container_derived() -> Result<()> {
if !check_skopeo() {
Expand Down