compression: register docker zstd stream processor#3968
compression: register docker zstd stream processor#3968tonistiigi merged 1 commit intomoby:masterfrom
Conversation
a9f9283 to
dda0bd9
Compare
|
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 cc @tonistiigi @AkihiroSuda @ktock in case there's any other ideas or if you think this approach is okay. |
dda0bd9 to
d73e29d
Compare
tonistiigi
left a comment
There was a problem hiding this comment.
We have existing zstd tests like testPullZstdImage, testZstdRegistryCacheImportExport. What's the difference?
| 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" { |
There was a problem hiding this comment.
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>
d73e29d to
6ce499c
Compare
|
Squashed this as all the stream processor code has been reverted in a later commit. |
Thanks @tonistiigi! To answer your previous questions:
The previous
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. |
|
Shouldn't this all be |
|
@jonjohnsonjr No, this is based on Docker mediatypes distribution/distribution@2ff77c0#diff-dcad444e0ff28a727a091ec0e27cf747e488d4433277c7c86d792fe8296bd84eR22 containerd/containerd@5a3151e#diff-054c8a3671a5a631a918aa1b64f25fe3ad640f0c55d554945df8775dcb51f64fR17 . This is different from OCI mediatypes that use |
Before this, buildkit was able to create and push layers of type
vnd.docker.image.rootfs.diff.tar.zstdbut 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-typeWhile 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