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

Use zstd for ISO and SD images #85947

Merged
merged 4 commits into from Jun 7, 2020
Merged

Use zstd for ISO and SD images #85947

merged 4 commits into from Jun 7, 2020

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Apr 24, 2020

Motivation for this change

While experimenting with sd-image builds I found out, that zstd is much superior to bzip2 when compressing SD/ISO images (95% reduction of time needed).

Because compression used to compress the distribution images is considered a public interface, I'd like to discuss this in the PR.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@prusnak
Copy link
Member Author

prusnak commented Apr 24, 2020

I compressed a 2.7 GB aarch64 NixOS image with the following results:

bzip2

  • output size: 627353716 bytes
  • duration: 217 seconds
real	3m36.963s
user	3m35.370s
sys	0m1.552s

zstd -T4:

  • output size: 614908377 (2% decrease)
  • duration: 10 seconds (95% faster) 🤯
real	0m10.069s
user	0m36.025s
sys	0m1.431s

It seems that zstd is even in the Debian stable, so this change is worth considering.

Copy link
Member

@lovesegfault lovesegfault left a comment

Choose a reason for hiding this comment

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

I'll add this to further drive the point of zstd being the future:
https://www.archlinux.org/news/now-using-zstandard-instead-of-xz-for-package-compression/

The future is now!

@prusnak prusnak marked this pull request as ready for review May 14, 2020 19:41
@mmahut mmahut merged commit 7b9d7cc into NixOS:master Jun 7, 2020
@prusnak prusnak deleted the images-zstd branch June 7, 2020 17:10
@dasJ
Copy link
Member

dasJ commented Jun 8, 2020

I think you forgot the module arg:

hydra-eval-jobs returned exit code 1:
error: "undefined variable 'zstd' at \u001b[1m/nix/store/8n8i7js5i4qzly36vpqvmvwjn3hr6z1b-source/nixos/lib/make-iso9660-image.nix\u001b[0m:51:36"
error: "undefined variable 'zstd' at \u001b[1m/nix/store/8n8i7js5i4qzly36vpqvmvwjn3hr6z1b-source/nixos/lib/make-iso9660-image.nix\u001b[0m:51:36"

@prusnak
Copy link
Member Author

prusnak commented Jun 9, 2020

@dasJ Can you please review #89883 ?

@prusnak prusnak mentioned this pull request Jun 13, 2020
@@ -277,6 +277,9 @@ environment.systemPackages = [
<title>Other Notable Changes</title>

<itemizedlist>
<listitem>
<para>SD and ISO images are now compressed by default using <literal>zstd</literal>.</para>
Copy link
Member

@samueldr samueldr Jun 14, 2020

Choose a reason for hiding this comment

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

This is wrong, only the SD image is now compressed by default using zstd.

The ISO image needs to be configured to be compressed to be compressed, and then it would now be zstd.

@samueldr
Copy link
Member

±0 for the SD image compression scheme. Hopefully no one is in a situation where using zstd is much harder than bzip.


Now, I misunderstood the change when I was reading the code, and was building in parallel. The following was written under the assumption that the isos were now default to being compressed.

This should now be read as a warning for someone that wants to change the compressImage default to true for the ISO image.

-1 for the ISO image being compressed with zstd, or compressed further at all. In fact, the only reason the SD image was compressed was because it grew to a bit over the size limits of Hydra.

Compressing the iso image needs changes at other places in the NixOS infrastructure, and documentation. I'm not sure what will happen with the mirror scripts, reading them makes me think it should end up copying the right thing.

The website would need changes.

The docs would need to be adjusted.

And, furthermore, the iso is already compressed using squashfs. Sure, it's not as compressed as this fancy currently in vogue compression, but it is compressed already. The squashfs settings maybe can be looked at to change the compression settings, though.

Note that I have no issues with zstd per se, but with needlessly compressing the iso. It is a usability annoyance, at best.

@erictapen
Copy link
Member

@prusnak I wonder wether 8a67595 is necessary. The cost here is that this broke cross building of NixOS systems for me, as zstd currently doesn't cross compile. Also this increases closure size for every NixOS system, even if it is never needed.

I guess it is needed at runtime of the system to decrypt the install ISO/image? Couldn't we include it conditionally in

nixos/modules/installer/cd-dvd/sd-image.nix
nixos/modules/installer/cd-dvd/iso-image.nix

like

{
  environment.systemPackages = lib.optional config.isoImage.compressImage [ pkgs.zstd ];
}

?

@prusnak
Copy link
Member Author

prusnak commented Jun 20, 2020

@erictapen No strong opinion about 8a67595. If you think zstd does not belong to a base system, open a PR with a revert. The cross-compilation of zstd was fixed in #91177

@erictapen
Copy link
Member

Thanks for the link to the fix, missed that. I just realized that gzip, xz, bzip2 are all part of the base system, so maybe it makes sense to have zstd as well. Sorry for bothering you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants