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.

Sign up for GitHub

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

Support zstd compression in image commit #5452

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support zstd compression in image commit #5452

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,9 @@ func computeLayerMIMEType(what string, layerCompression archive.Compression) (om
// how to decompress them, we can't try to compress layers with xz.
return "", "", errors.New("media type for xz-compressed layers is not defined")
case archive.Zstd:
// Until the image specs define a media type for zstd-compressed layers, even if we know
// how to decompress them, we can't try to compress layers with zstd.
Copy link
Contributor

@mtrmac mtrmac Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return "", "", errors.New("media type for zstd-compressed layers is not defined")
omediaType = v1.MediaTypeImageLayerZstd
dmediaType = "application/vnd.docker.image.rootfs.diff.tar.zstd"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

logrus.Debugf("compressing %s with zstd", what)
default:
logrus.Debugf("compressing %s with unknown compressor(?)", what)
}
Expand Down
Loading