deploy: short-circuit pull if digest pullspec already exists

[jlebon]

If we're pulling by digest and the pullspec already exists, then there's no need to reach out to the registry or even spawn skopeo.

Detect this case and exit early in the pull code.

This allows RHCOS to conform better to the PinnedImageSet API[1], where the expectation is that once an image is pulled, the registry will not be contacted again. In a future with unified storage, the MCO's pre-pull would work just the same for the RHCOS image as any other.

Framing this more generally: this patch allows one to pre-pull an image into the store, and making the later deployment operation be fully offline. E.g. this could be used to implement a bootc switch --download-only option.

[1] https://github.com/openshift/enhancements/blob/26ce3cd8a0c7ce650e73bc5393a3605022cb6847/enhancements/machine-config/pin-and-pre-load-images.md

[jlebon]

If we do this, I'll probably add the same to rpm-ostree (... will be nice once we get to coreos/rpm-ostree#4994).

Also this needs a test.

[gemini-code-assist]

Code Review

This pull request optimizes image pulls by short-circuiting the process if the image already exists locally, improving efficiency. The changes are well-structured and the logic is sound.

[cgwalters]

Thanks!

[cgwalters]

• I downgraded the println! to a tracing::debug! since I think silence-is-golden in this case
• Now that I look closer I think this would be better in the ostree-ext code

[jlebon]

I downgraded the println! to a tracing::debug! since I think silence-is-golden in this case

WFM. Just to note though, this was an attempt to match the "No changes" code lower down for the AlreadyPresent case. (But using a different string to make the cases distinguishable for e.g. testing/debugging.)

Now that I look closer I think this would be better in the ostree-ext code

Heh, that was actually what I did at first, trying to make it directly part of ImageImporter::prepare() since it's what already returns AlreadyPresent. The problem is that new_importer() already forks skopeo, and crucially does OpenImage which will reach out to the registry, even before we get there.

Or do you mean just shoving that code into a non-member utility function in ostree-ext?

[jlebon]

Alternatively I guess we switch proxy_img to be an Option<OpenedImage> and delay initialization until prepare().

[cgwalters]

Yep let me take this one

[cgwalters]

OK I redid this, can you review?

[jlebon]

Shouldn't this be previous_state?

[cgwalters]

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

[jlebon]

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.

[cgwalters]

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

[jlebon]

I can't officially LGTM, but LGTM. Thanks for picking this up! Having it in ostree-ext means rpm-ostree should also get this behaviour.

[jeckersb]

LGTM on a cursory glance, mostly trusting @jlebon's +1 and making it official 😄