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

image,shared: Allow specifying compression level #680

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

gnustomp
Copy link
Contributor

@gnustomp gnustomp commented Nov 29, 2022

Allow specifying compression levels when the underlying tool or mksquashfs understands it. For example, gzip-1 or zstd-22. Compression levels are checked pre-run.

Signed-off-by: James Ye jamesye@google.com

image/lxd.go Outdated Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
@monstermunchkin
Copy link
Member

monstermunchkin commented Nov 30, 2022

@gnustomp a couple of things are still missing:

  • support for lxc images
  • update docs (building.md)
  • update compressionDescription variable in main.go
  • lzo compression level support

Also, I believe we should do early validation here, here, here, and here. Some builds take quite a while, and it'd be a shame if they failed late due to a typo in the compression string.

@gnustomp
Copy link
Contributor Author

gnustomp commented Dec 1, 2022

Thanks for the feedback, @monstermunchkin. I have refactored the PR so that it supports specifying levels for most compression methods and have added tests.

  • As I understand, LXC images are handled by the tarball code, so it should not require any special support, unlike LXD which can use squashfs?
  • Since distrobuilder currently doesn't validate the compression method, I have retained this behaviour for tarball outputs in this PR. A compression level cannot be specified for unknown methods, however, and the output is uncompressed.
  • While mksquashfs supports specifying a level for its lzo method, the corresponding tool for this algorithm is named lzop, so a split squashfs image currently can't be generated while specifying a compression level. (Without specifying a compression level, the metadata tarball is left uncompressed.) This could be worked around by redirecting squashfs lzo to the lzop binary or vice versa, but I think this should be a separate PR?

Allow specifying compression levels when the underlying tool or
mksquashfs understands it. For example, gzip-1 or zstd-22. Compression
levels are checked pre-run.

Signed-off-by: James Ye <jamesye@google.com>
@gnustomp gnustomp changed the title image,shared: Allow specifying compression level for gzip and zstd image,shared: Allow specifying compression level Dec 1, 2022
Copy link
Member

@monstermunchkin monstermunchkin left a comment

Choose a reason for hiding this comment

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

This looks really good. Just a few changes regarding lzo and lzop to avoid confusion.

distrobuilder/main.go Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
shared/util.go Outdated Show resolved Hide resolved
@monstermunchkin
Copy link
Member

  • This could be worked around by redirecting squashfs lzo to the lzop binary or vice versa, but I think this should be a separate PR?

Let's add it to this PR, just a separate commit.

mksquashfs accepts "lzo" for compression, but the standalone tool is
named "lzop". Accept both "lzo" and "lzop" to improve handling of
LZO-compressed LXD split images and for convenience.

Signed-off-by: James Ye <jamesye@google.com>
@gnustomp
Copy link
Contributor Author

gnustomp commented Dec 2, 2022

Let's add it to this PR, just a separate commit.

Done.

@monstermunchkin monstermunchkin merged commit ac63a0c into lxc:master Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants