Skip to content
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.

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

compression: register docker zstd stream processor #3968

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

sipsma
Copy link
Collaborator

@sipsma sipsma commented Jun 23, 2023

Before this, buildkit was able to create and push layers of type vnd.docker.image.rootfs.diff.tar.zstd but if you tried to then pull and unpack those layers in buildkit, you'd get an error from containerd: failed to get stream processor for application/vnd.docker.image.rootfs.diff.tar.zstd: no processor for media-type

While that media type is not official, support for exporting it was added to buildkit in the past anyways since it works in practice and is accepted by many registries. It thus seems logical that buildkit should also support pulling and unpacking those layers too.

The fix works by just registering a custom stream processor w/ containerd.

There is support for registering stream processors in containerd, but
those only work when using the OCI worker since it relies on the apply
implementation being in the same process as buildkitd.

As a fallback, we instead just implement a hack that swaps out the
docker zstd media type for the oci one before sending it to containerd.
This works in practice because the two types are compatible.


Ran into problems with this in the real world with dagger here

@sipsma
Copy link
Collaborator Author

sipsma commented Jun 23, 2023

I just saw that the test only passes when using the OCI worker, fails on containerd worker. I'm guessing that's because when we use the containerd worker the stream processor is not in the buildkitd process but instead in containerd...

I'll poke around for ways of fixing, but open to suggestions.


EDIT:

I pushed what appears to be a fix by just changing the media type of docker zstd layers to be the oci equivalent before passing to the applier. This feels like a pretty dumb hack but as far as I can tell it should work consistently since the two types are fully compatible to my knowledge.

It also feels hacky since the code is in the winlayers package, but it seems like that's where we are handling all of our Applier-wrapping since there's also some nydus stuff in there too?

cc @tonistiigi @AkihiroSuda @ktock in case there's any other ideas or if you think this approach is okay.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

We have existing zstd tests like testPullZstdImage, testZstdRegistryCacheImportExport. What's the difference?

@@ -37,6 +37,12 @@ type winApplier struct {
}

func (s *winApplier) Apply(ctx context.Context, desc ocispecs.Descriptor, mounts []mount.Mount, opts ...diff.ApplyOpt) (d ocispecs.Descriptor, err error) {
// HACK:, containerd doesn't know about vnd.docker.image.rootfs.diff.tar.zstd, but that
// media type is compatible w/ the oci type, so just lie and say it's the oci type
if desc.MediaType == images.MediaTypeDockerSchema2Layer+".zstd" {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look specific to winApplier?

Before this, buildkit was able to create and push layers of type
vnd.docker.image.rootfs.diff.tar.zstd but if you tried to then pull and
unpack those layers in buildkit, you'd get an error from containerd:
`failed to get stream processor for application/vnd.docker.image.rootfs.diff.tar.zstd: no processor for media-type`

While that media type is not official, support for exporting it was
added to buildkit in the past anyways since it works in practice and is
accepted by many registries. It thus seems logical that buildkit should
also support pulling and unpacking those layers too.

There is support for registering stream processors in containerd, but
those only work when using the OCI worker since it relies on the apply
implementation being in the same process as buildkitd.

As a fallback, we instead just implement a hack that swaps out the
docker zstd media type for the oci one before sending it to containerd.
This works in practice because the two types are compatible.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>
@tonistiigi
Copy link
Member

Squashed this as all the stream processor code has been reverted in a later commit.

@tonistiigi tonistiigi merged commit 31a9120 into moby:master Jul 10, 2023
55 checks passed
@sipsma
Copy link
Collaborator Author

sipsma commented Jul 11, 2023

Squashed this as all the stream processor code has been reverted in a later commit.

Thanks @tonistiigi! To answer your previous questions:

We have existing zstd tests like testPullZstdImage, testZstdRegistryCacheImportExport. What's the difference?

The previous testPullZstdImage test specifically forced oci types. I just updated it to cover both oci and docker types and also have more assertions on the correct media type being produced.

This doesn't look specific to winApplier?

I agree but I saw that we were also doing some nydus-specific handling here that didn't seem related to Windows, so I figured it was just our generic wrapper for any custom Applier logic.

Maybe it should just be renamed? Or I can do a follow up to split this into several different wrappers for windows, nydus and this zstd handling. Let me know if you think it matters.

@jonjohnsonjr
Copy link

Shouldn't this all be +zstd not .zstd?

@tonistiigi
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants