Support zstd compression in image commit

[aaronlehmann]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Without this change, specifying Compression: imagebuildah.Zstd in imagebuildah's `BuildOptions fails, so it is not possible to push cache to a registry with zstd compression.

https://github.com/opencontainers/image-spec/blob/main/media-types.md defines a media type for zstd-compressed layers, so I don't think the comment about lack of a defined media type is still applicable.

How to verify it

Specify Compression: imagebuildah.Zstd when building an image using imagebuildah.BuildDockerfiles,.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

n/a

Does this PR introduce a user-facing change?

None

[aaronlehmann]

If a test is required for this change, I'd appreciate suggestions about how to best add test coverage. It doesn't seem possible to test via the CLI because the Compression parameter isn't wired up as a CLI flag.

	if c.Flag("compress").Changed {
		logrus.Debugf("--compress option specified but is ignored")
	}

	compression := define.Gzip
	if iopts.DisableCompression {
		compression = define.Uncompressed
	}

[rhatdan]

@giuseppe @mtrmac @nalind PTAL

[rhatdan]

@flouthoc PTAL

[aaronlehmann]

We must refuse to create v2s2 images with zstd compression.

AFAICS that means that the whole of NewImageSource needs to be restructured around respecting i.preferredManifestType, instead of always building data in both formats.

To keep the change relatively contained, what would you think about returning an empty dmediatype from computeLayerMIMEType, and making NewImageSource return an error if the media type corresponding to manifestType is empty. Something like:

		// Figure out if we need to change the media type, in case we've changed the compression.
		omediaType, dmediaType, err = computeLayerMIMEType(what, i.compression)
		if err != nil {
			return nil, err
		}
		// The layer media type must be defined for the type of manifest we are producing.
		if (omediaType == "" && manifestType == v1.MediaTypeImageManifest) ||
			(dmediaType == "" && manifestType == manifest.DockerV2Schema2MediaType) {
			return nil, fmt.Errorf("manifest type %q does not support selected compression format", manifestType)
		}

Refactoring with a manifest type abstraction seems like a worthy idea as well. I can't see a reason why this function is constructing both in parallel (not sure if I'm missing something). I'm not sure I'm the best person to do this refactor though, as I'm brand new to the codebase.

[mtrmac]

I’ll let actual Buildah maintainers to decide how this should be organized.

[aaronlehmann]

@rhatdan: How would you like to proceed here?

[aaronlehmann]

Hoping for some direction on where to go with this PR. I'm happy to make the changes if the maintainers have thoughts on what makes most sense.

[mtrmac]

My preference would be

Add some kind of “manifest type” abstraction, and call it everywhere the code currently computes both values

but that’s the option that requires more work, and I’m not authoritative to commit Buildah to that direction, nor can I spend very much time helping with that work.

[aaronlehmann]

@rhatdan: Still looking for guidance on this one...

[rhatdan]

@giuseppe @nalind PTAL

[aaronlehmann]

@giuseppe @nalind: Any thoughts on this one? I'm happy to restructure it as necessary, but I'm looking for direction on this.

[aaronlehmann]

@giuseppe @nalind @rhatdan: I'd really like to get this upstreamed; can you please let me know how you'd like me to approach the change?

[giuseppe]

could you please define "application/vnd.docker.image.rootfs.diff.tar.zstd" in containers/image/v5/manifest as @mtrmac pointed out?

[mtrmac]

could you please define "application/vnd.docker.image.rootfs.diff.tar.zstd" in containers/image/v5/manifest as @mtrmac pointed out?

@giuseppe To this day we are explicitly rejecting unknown MIME types for v2s2 layers. (I could see an argument that that has been a mistake, but here we are anyway.) Any v2s2 image with zstd are not going to be accepted by any currently existing version of Podman.

Why should we start creating such images now, instead of using the OCI format exclusively?

[giuseppe]

@mtrmac ok thanks, that makes sense.

@aaronlehmann can you use the OCI format?

[aaronlehmann]

@giuseppe: Using the OCI format is completely fine for me; I don't care about the Docker format. However, the issue is that NewImageSource constructs OCI and Docker manifests simultaneously, and it's not clear what to do for the Docker format if we're using zstd compression. Do I need to refactor the code to make it possible to produce either type of manifest separately?

[aaronlehmann]

@giuseppe: Any thoughts?

[giuseppe]

@mtrmac do you have any suggestions for @aaronlehmann ?

[mtrmac]

… so we have come full circle :)

My preference is https://github.com/containers/buildah/pull/5452#issuecomment-2050383200 , to split the manifest creation code into per-format implementations, and to only use one instead of both.

[rhatdan]

@aaronlehmann still working on this?

[aaronlehmann]

@rhatdan: Happy to come back to this if I can get some clarity over the direction to go for the solution. Would something like https://github.com/containers/buildah/pull/5452#issuecomment-2032201348 be merged, or do we need to completely refactor with a "manifest type" abstraction as described in https://github.com/containers/buildah/pull/5452#issuecomment-2050383200?

[rhatdan]

If miloslav prefers 5452, I would go along with him. He is far smarter then me.

[aaronlehmann]

Thanks. I've created #5743 to abstract the manifest handling instead of trying to build both simultaneously in NewImageSource. Once this is reviewed/merged, I can update this PR to add zstd compression support.

[aaronlehmann]

@rhatdan: This is waiting for review of #5743 now.

[nicolaerosiaradcom]

is this related to the fact that cache gets always compressed as application/vnd.oci.image.layer.v1.tar+gzip ? I can't seem to find a way to change the compression of the cache when using buildah build

[aaronlehmann]

Now that #5743 is merged, I've updated this to only support zstd in the OCI manifest path.

[aaronlehmann]

@mtrmac: Does this look good now that it's specific to OCI manifests?

[flouthoc]

@aaronlehmann do you think a test can be added for this, do you need help with tests ?

[aaronlehmann]

@flouthoc: Yes, some help would be great! Please see my comment here: https://github.com/containers/buildah/pull/5452#issuecomment-2030553393

[aaronlehmann]

@flouthoc: Any ideas about what a test should look like?