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

container: Use docker-archive: to stream image #22

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

cgwalters
Copy link
Member

Depends: #20 (mostly to avoid conflicts)

container: Use docker-archive: to stream image

It turns out that for whatever reasons (presumably mostly historical)
in skopeo, while oci-archive: is backed by just writing everything
to a tempdir and then tarring it up, the docker-archive: format
is actually streaming.

And it's way nicer and more efficient to stream. Then the
last piece of this puzzle is that I realized we can use a named pipe
i.e. mkfifo to avoid having the containers/image stack crash
because it wants to invoke realpath() on the provided target.
By simply giving our pipe a name, it can do so (though it's
pointless) but this way we work with shipped versions of skopeo
and don't need to wait months for patches to propagate.

Also as a bonus, here skopeo ends up decompressing the layer for us,
so we don't need to e.g. handle gzip vs zstd in our code.

It turns out that for whatever reasons (presumably mostly historical)
in skopeo, while `oci-archive:` is backed by just writing everything
to a tempdir and then tarring it up, the `docker-archive:` format
is actually streaming.

And it's *way* nicer and more efficient to stream.  Then the
last piece of this puzzle is that I realized we can use a named pipe
i.e. `mkfifo` to avoid having the containers/image stack crash
because it wants to invoke `realpath()` on the provided target.
By simply giving our pipe a name, it can do so (though it's
pointless) but this way we work with shipped versions of skopeo
and don't need to wait months for patches to propagate.

Also as a bonus, here skopeo ends up decompressing the layer for us,
so we don't need to e.g. handle gzip vs zstd in our code.
@cgwalters cgwalters force-pushed the async-skopeo-fetch branch from c70c41d to 3ecb887 Compare April 28, 2021 16:18
@cgwalters cgwalters marked this pull request as ready for review April 28, 2021 16:18
@cgwalters
Copy link
Member Author

OK rebased 🏄 and lifting draft!

@cgwalters
Copy link
Member Author

(OK actually I was wrong it seems, docker-archive: still seems to spool the whole uncompressed layer to a file before sending it to us 😢 )

@cgwalters
Copy link
Member Author

But to be clear I think it's still worth merging this because we at least re-gain the "incrementally parse streamed tarfile" code which is definitely closer to the desired end state.

// Is there a better way to do this?
let worker = async move {
let (import, input_copydriver) = tokio::join!(import, input_copydriver);
let _: () = import?.context("Import worker")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering what the purpose of these unused variables are. Is it just for adding annotations for the compiler?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's confusing because there's Result<Result<T>> stuff involved here and I want to be careful we're fully unwrapping these and handling the chain.

It's too easy to do e.g. let _ = foo? when it should be let _ = foo??.

Copy link
Member Author

Choose a reason for hiding this comment

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

To elaborate further by doing let _: () and adding the explicit () typing we make it a compile error if the result of ? isn't ().

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, thanks!

Copy link
Contributor

@kelvinfan001 kelvinfan001 left a comment

Choose a reason for hiding this comment

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

Looks sane to me.
(Sorry for lack of any real feedback; this one was a bit harder than I thought 😅, maybe another pair of eyes would help to confirm sanity).

@cgwalters cgwalters merged commit fbdad14 into ostreedev:main Apr 30, 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.

None yet

2 participants