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.

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 for zstd layer #333

Closed
gdippolito opened this issue Aug 17, 2023 · 12 comments
Closed

Support for zstd layer #333

gdippolito opened this issue Aug 17, 2023 · 12 comments
Labels
enhancement New feature or request

Comments

@gdippolito
Copy link

Hello!

Thanks for starting off this project.

I was wondering if rules_oci supports zstd as image layers? In the rules_docker repository there were a couple of discussions about it see here and there was also an attempt to add support in this fork

For what concerned running the containers containerd supports zstd layers for quite some time now (1.5 May 2021) so it should not create any problems at runtime. Considering the speed difference of zstd compared with gzip compression I think it would give a good speed up for both image creation and extraction.

From my understanding bazel has native support for zstd since 5.1 see here do you think this project would support that? We are currently using rules_docker but we will be very happy to move to rules_oci if we can get some new features in.

Thanks

@ReillyBrogan
Copy link

Yes, this would be really useful. Both being able to create new image layers as zstd-compressed as well as the ability to convert existing layers to zstd (similar to the buildkit --force-compression=zstd argument). Zstd really is a significant improvement, not only are the layers a fair bit smaller but the decompression is much faster which leads to faster start times.

@ReillyBrogan
Copy link

So I looked into how this project handles layers, and ultimately support for this needs to be added to the crane tool for it to work. crane append does not currently support specifying the compressor to use for the new layer (defaults to gzip), though I think my golang skills are good enough that I can probably figure out how to add it since go-containerregistry has zstd code already in the repo.

A new crane command (crane convert ?) would likely need to be added to convert an existing image to use zstd-layers and this is unfortunately likely to be beyond my skills, though such a command would likely be useful to a wide variety of users beyond rules_oci.

@thesayyn
Copy link
Collaborator

A new crane command (crane convert ?) would likely need to be added to convert an existing image to use zstd-layers and this is unfortunately likely to be beyond my skills, though such a command would likely be useful to a wide variety of users beyond rules_oci.

we had discussions around layer conversion before, generally, it's a bad idea AFAICT due to changing digests, etc. This needs a RFC before an implementation.

So I looked into how this project handles layers, and ultimately support for this needs to be added to the crane tool for it to work. crane append does not currently support specifying the compressor to use for the new layer (defaults to gzip), though I think my golang skills are good enough that I can probably figure out how to add it since go-containerregistry has zstd code already in the repo.

this needs to be fixed there first, then we can allow zstd layers here in rules_oci. You are more than welcome to try it out.

@ReillyBrogan
Copy link

we had discussions around layer conversion before, generally, it's a bad idea AFAICT due to changing digests, etc. This needs a RFC before an implementation.

Sure, the layer and image digests would all change but such an image conversion would be hermetic. The same input digest would result in the same output digest assuming that the compressor, compression level, and crane version stayed the same.

What about something like this?

oci_pull(
    name = "distroless_java",
    digest = "sha256:161a1d97d592b3f1919801578c3a47c8e932071168a96267698f4b669c24c76d",
    image = "gcr.io/distroless/java17",
)
oci_convert_image(
    name = "distroless_java_zstd",
    base = "@distroless_java",
    compression = "zstd",
)

oci_image(
    name = "app",
    base = "@distroless_java_zstd",
    ...
)

@thesayyn
Copy link
Collaborator

I am a little hesitant to introduce another rule to our public API. And one of the design decisions we made was that rules_oci does not get involved in the manipulation/generation of layers. This rule will not only expand the public API but also violate that decision. Not to mention the increase in the complexity of rules_oci because this rule doesn't just change the compression, but reconstructs the whole image. (manifest, config, layers etc...)

And I think it is out of scope for this issue. Please make a new issue for "conversion" of existing images.

@yetanotheralex
Copy link

This #385 would basically bring zstd support for free

@ReillyBrogan
Copy link

This #385 would basically bring zstd support for free

How so? crane append which is used to construct the images doesn't support adding pre-compressed tar files so you'd still need my go-containerregistry PR in order to allow that.

@yetanotheralex
Copy link

pure zstd support as requested by @gdippolito only needs the underlying system to support it; the PR I linked is switching to native bsdtar which does zstd out of the box

@C0mbatwombat
Copy link

@yetanotheralex could you give an example of how to enable zstd compression?

@thesayyn thesayyn added the enhancement New feature or request label Dec 12, 2023
@asford
Copy link

asford commented Jan 17, 2024

How so? crane append which is used to construct the images doesn't support adding pre-compressed tar files so you'd still need my go-containerregistry PR in order to allow that.

Pending crane prs

google/go-containerregistry#1798 or google/go-containerregistry#1827

would provide support for adding pre-compressed tar files.

@thesayyn
Copy link
Collaborator

thesayyn commented May 2, 2024

I have a PR #550 that fixes this.

@thesayyn
Copy link
Collaborator

thesayyn commented May 7, 2024

zstd supported was added in #550

@thesayyn thesayyn closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants