Skip to content
This repository has been archived by the owner on Jan 15, 2025. It is now read-only.

Fetch via container-image-proxy #98

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Sep 22, 2021

https://github.com/cgwalters/container-image-proxy
is prototype code to expose containers/image via a HTTP API
suitable for use in non-Go programs.

This has many advantages over us forking skopeo; the key one being
on-demand layer fetching.

Closes: #18

@cgwalters cgwalters changed the title lib/container: Split off helper to find all layers into oci WIP: Fetch via container-image-proxy Sep 22, 2021
/// The first return value is an `AsyncRead` of that tar file.
/// The second return value is a background worker task that
/// owns stream processing.
pub async fn find_layer_tar(
Copy link
Member Author

Choose a reason for hiding this comment

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

Good riddance to this code!

@cgwalters
Copy link
Member Author

cgwalters commented Sep 22, 2021

So what I am currently having a huge hellish pain of a time debugging is why after exactly 32768 bytes reading from the child process, the stream gets corrupted.

We get exactly the correct length. I haven't figured out yet whether it's the child process outputting the wrong thing, or we're reading the wrong thing. But I think it's the latter.

My instinct was to blame something going wrong in this weird manual "hyper over socketpair" that we have going on - I tried extracting that into a standalone thing https://github.com/cgwalters/hyper-self-socketpair but it works there.

@cgwalters
Copy link
Member Author

It was cgwalters/container-image-proxy@b64f3a0
😢 😢 😢

@cgwalters
Copy link
Member Author

OK, this depends on #95 so keeping draft, but this should otherwise be in a merge-able state.

Actually shipping this in e.g. rpm-ostree is probably going to depend on cgwalters/container-image-proxy#1
but short term, I think development can proceed by just building/copying a build of container-image-proxy into custom FCOS.

@cgwalters cgwalters changed the title WIP: Fetch via container-image-proxy Fetch via container-image-proxy Sep 23, 2021
@cgwalters cgwalters marked this pull request as ready for review September 23, 2021 15:11
@cgwalters
Copy link
Member Author

OK lifting draft!

@cgwalters
Copy link
Member Author

Oops, had forgotten we had two manifest fetch paths - changed the other one to stop using skopeo too.

(Now that I look there's more cleanup to do here - right now we're tearing down and recreating the proxy, which is inefficient)

https://github.com/cgwalters/container-image-proxy
is prototype code to expose containers/image via a HTTP API
suitable for use in non-Go programs.

This has many advantages over us forking skopeo; the key one being
on-demand layer fetching.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Some minor comments, but overall looks sane to me!

#!/bin/bash
set -xeuo pipefail

yum -y install skopeo
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be able to drop this now that we no longer fork it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now our CI only runs in the buildroot container which doesn't have skopeo.

At some point though we clearly need to go to the next level and split buildroot and runtime CI flow, whether that's Prow or CoreOS Jenkins (or both), and that leads to things like reusing ostree/rpm-ostree CI etc.

set -xeuo pipefail

yum -y install skopeo
yum -y --enablerepo=updates-testing update ostree-devel
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to drop this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm true but OTOH I think let's keep it because I've actually had a new desire to add new API to ostree again, and while this still requires an ostree release, we don't need to block on bodhi at least.

}

impl ImageProxy {
pub(crate) async fn new(imgref: &ImageReference) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why spawn separate instances per image ref, vs just making the image ref part of the protocol?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is more an issue for https://github.com/cgwalters/container-image-proxy/ right? So it actually did originally work that way, but when I generalized it to support all containers/image backends (e.g. supporting oci-archive: and containers-storage: and not just a registry) it became much nicer to have it do only one image at a time.

And the reason for that is HTTP is inherently stateless, but interacting with a containers/image reference is somewhat stateful (e.g. it will transparently convert an oci-archive: to an oci: directory in the background in a tempdir).

Copy link
Member Author

Choose a reason for hiding this comment

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

@cgwalters cgwalters merged commit ae58e16 into ostreedev:main Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

container: Work on new skopeo backend
2 participants