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.

Note this is only applicable to OCI manifests.

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

[edsantiago]

Drive-by comment: I'm uncomfortable with hardcoded magic strings. Shouldn't this be defined as a constant in c/image?

[mtrmac]

Yes, if this should happen, it would be in c/image/v5/manifest.

(Buildkit does seem to use this constant, but it is not a part of the public API.)

[mtrmac]

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.

There may be various ways to do it:

• Add some kind of “manifest type” abstraction, and call it everywhere the code currently computes both values
• Continue building both formats all the time, but somehow represent “this format failed”, and check that at the very end (might be less invasive, but also rather more convoluted)
• Maybe some other design.

[mtrmac]

This comment is accurate: Docker images can’t represent Zstd images layers.

We can’t very well just silently ignore that problem and create something that doesn’t work.

[aaronlehmann]

I'm happy to defer to you and refuse to create schema2 images with zstd compression. But I'm curious to understand why this is a problem - BuildKit seems happy to create them using this media type, and they seem to work fine with Docker and kaniko (but not buildah...).

[mtrmac]

One place is our own https://github.com/containers/image/blob/1c648b405e0763472db0bcc189136ddb6a5b61c6/manifest/docker_schema2.go#L173 . In retrospect, being that strict might have been a mistake, but either way that code refusing unexpected MIME types has been there for many years.

(IIRC, Docker is not checking MIME types at all when pulling; that’s the other extreme option.)

So we would need to think about a migration mechanism, and giving users enough tools to manage it. For a format feature which is, AFAICS, undocumented. Meanwhile, “if you want to use Zstd, migrate to OCI” is a simple story that works. for all implementations.

[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 #5452 (comment) , 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 #5452 (comment) be merged, or do we need to completely refactor with a "manifest type" abstraction as described in #5452 (comment)?

[rhatdan]

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

[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?

[mtrmac]

Yes, this LGTM.

Buildah maintainers, do you want tests added? The PR has a “no new tests” label but I can’t tell why.

[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: #5452 (comment)

[aaronlehmann]

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

[flouthoc]

I am thinking if we can wire up --compress then it will be easier to test in bud.bats, I can't see any specific reason to leave --compress as no-op. @nalind @rhatdan @mtrmac WDYT ?

[mtrmac]

072ee88 suggests that --compress is supposed to apply to inputs to a build, not to outputs.

I didn’t investigate further, is that incorrect?

[aaronlehmann]

@flouthoc: Any thoughts on @mtrmac's question about --compress semantics?

[flouthoc]

There is no intuitive way to test this without exposing a user exposed feature or hidden CLI flag, since this does not affects default behaviour of buildah, maybe we can merge this without test. @nalind any thoughts ?

[aaronlehmann]

@nalind WDYT?

[aaronlehmann]

@nalind @flouthoc: Sorry to keep nagging... anything we can do to move this forward?

[nalind]

The change itself LGTM, but without a test of some kind, I fear we wouldn't notice if it broke. There's one on the tip of https://github.com/nalind/buildah/tree/aaronlehmann-support-zstd-commit, if you'd like to cherry-pick it.

[aaronlehmann]

Thanks so much! I went ahead and cherry-picked this test (and also rebased the PR).

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[aaronlehmann]

@flouthoc: I think this is ready to go now. Please let me know if you'd like any other changes.

[flouthoc]

LGTM
/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaronlehmann, flouthoc, mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [flouthoc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[aaronlehmann]

🎉 Thank you!