[RFC] oci: allow zstd for Docker images

[giuseppe]

Allow to use the zstd compression for docker images now that Docker
supports it: moby/moby#41759

Marked as RFC as I am not completely sure about its implications.

Without this patch though I am not able to build any container that uses a zstd compressed image as base:

$ cat Dockerfile 
FROM docker.io/gscrivano/zstd-chunked:fedora
RUN dnf install -y nginx && dnf clean all -y

$ buildah bud --format oci --squash  .
STEP 1/2: FROM docker.io/gscrivano/zstd-chunked:fedora
error creating build container: error preparing image configuration: error converting image "containers-storage:[overlay@~/.local/share/containers/storage+/run/user/1000/containers]@c309a2d1ba2e8026019aa4c987684a1d8f84d6bc598ff88d6afe1b4cbde4d70d" from "application/vnd.oci.image.manifest.v1+json" to "application/vnd.docker.distribution.manifest.v2+json": Error during manifest conversion: "application/vnd.oci.image.layer.v1.tar+zstd": zstd compression is not supported for docker images 2

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[rhatdan]

LGTM
@vrothberg @mtrmac PTAL

[mtrmac]

Most importantly, where did

	DockerV2Schema2LayerMediaTypeZstd = "application/vnd.docker.image.rootfs.diff.tar.zst"

come from? I can’t see anything in moby/moby#41759 that would introduce, generate, or support such a value.

IOW, what did moby/moby#41759 actually define as the new format? I realize that it now happens to accept Zstd via auto-detection, but what formats are valid for Buildah to generate?

Without defining that, this PR feels premature. (Or, alternatively, this might be the place where that gets defined, sure, but I suspect schema2 MIME types should at least involve the schema2 format owners.)

[mtrmac]

If this is introduced, it must also exist in schema2CompressionMIMETypeSets.

[mtrmac]

The other direction needs to exist (and be tested) as well.

---

What about MediaTypeImageLayerNonDistributableZstd? Intentionally unsupported? Actually supported? Forgotten about?

[mtrmac]

I’d expect a non-trivial test implementation here, detailing the expected results.

(Similarly for any other new conversions.

Overall most of the code in c/image/manifest and c/image/image should have good test coverage, because it can be tricky, it’s easy to miss corner cases in testing, and it has no state so it is fairly easy to write tests.)

[mtrmac]

Without this patch though I am not able to build any container that uses a zstd compressed image as base:

…
error creating build container: error preparing image configuration: error converting image "containers-storage:[overlay@~/.local/share/containers/storage+/run/user/1000/containers]@c309a2d1ba2e8026019aa4c987684a1d8f84d6bc598ff88d6afe1b4cbde4d70d" from "application/vnd.oci.image.manifest.v1+json" to "application/vnd.docker.distribution.manifest.v2+json": Error during manifest conversion: "application/vnd.oci.image.layer.v1.tar+zstd": zstd compression is not supported for docker images 2

To me, this just primarily looks like a long-standing Buildah design decision (problem?), in that it expects almost everything about the images to be simultaneously representable as schema2 and OCI. See the two unmarshalConvertedConfig calls in Builder.initConfig, and how this fails converting to schema2 even if the user explicitly asked for an OCI-formatted output. If we are going to support any kind of long-term format evolution/divergence, I think that Buildah design constraint needs to be eliminated Buildah-side. (To be clear, it’s not at all obvious to me how to cleanly design the build process, without this constraint; I just think that it needs to happen, somehow, eventually.)

There may well be other contributing factors to how this fails right now (e.g. it’s of dubious value to even talk about the compressed MIME types in manifests for in-c/storage decompressed values); I didn’t look into it closely.

[giuseppe]

To me, this just primarily looks like a long-standing Buildah design decision (problem?), in that it expects almost everything about the images to be simultaneously representable as schema2 and OCI. See the two unmarshalConvertedConfig calls in Builder.initConfig, and how this fails converting to schema2 even if the user explicitly asked for an OCI-formatted output. If we are going to support any kind of long-term format evolution/divergence, I think that Buildah design constraint needs to be eliminated Buildah-side. (To be clear, it’s not at all obvious to me how to cleanly design the build process, without this constraint; I just think that it needs to happen, somehow, eventually.)

thanks for the hint. I tried to remove the unmarshalConvertedConfig to manifest.DockerV2Schema2MediaType when building an OCI image and I could build successfully the image (I also had to drop a check for number of layers == number of items in the history).

I am not familiar with this part of Buildah. I don't know what the solution should be and what are the implications if I drop these check. @nalind do you have any suggestions how we can solve this issue?