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

nixos/tests/installer: test /boot on ZFS #223412

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Mar 27, 2023

Description of changes

Let's test / on ZFS and /boot on ZFS in separate tests since the GRUB integration for ZFS seems to be not very well maintained.
If the test breaks in the future it's easier to figure out that ZFS on /boot is at fault and either fix the issue or disable the test.
The new test creates a ZFS pool where all features not compatible with GRUB2 are disabled. The dataset is then mounted on /boot and we check that the installer correctly generates a bootable configuration.
Try to use as many ZFS features as possible to verify that GRUB can handle them.

This PR adds a test to have some more confidence in the change proposed in #195805.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@@ -634,6 +634,7 @@ in
// {
"zfs/zed.d/zed.rc".text = zedConf;
"zfs/zpool.d".source = "${cfgZfs.package}/etc/zfs/zpool.d/";
"zfs/compatibility.d".source = "${cfgZfs.package}/share/zfs/compatibility.d/";
Copy link
Member

Choose a reason for hiding this comment

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

Could you do a separate commit to introduce this change and explain why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, yeah.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Context is needed on the ZFS semantic change, other than that, LGTM, thank you for getting this test :).

@RaitoBezarius
Copy link
Member

Also, sorry probably due to me, conflicts. :)

@Luflosi Luflosi marked this pull request as draft March 27, 2023 19:57
@Luflosi Luflosi mentioned this pull request Apr 28, 2023
13 tasks
@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 7, 2023

Apparently the "zfs/compatibility.d".source = "${cfgZfs.package}/share/zfs/compatibility.d/"; line isn't needed for some reason, see #262982 (comment).

@Luflosi Luflosi marked this pull request as ready for review December 7, 2023 12:13
Let's test / on ZFS and /boot on ZFS in separate tests since the GRUB integration for ZFS seems to be not very well maintained.
If the test breaks in the future it's easier to figure out that ZFS on /boot is at fault and either fix the issue or disable the test.
The new test creates a ZFS pool where all features not compatible with GRUB2 are disabled. The dataset is then mounted on /boot and we check that the installer correctly generates a bootable configuration.
Try to use as many ZFS features as possible to verify that GRUB can handle them.
@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 7, 2023

cc @misuzu @ElvishJerricco and @RaitoBezarius
cc @Mic92 since you wrote #262982 (comment).

Copy link
Contributor

@misuzu misuzu left a comment

Choose a reason for hiding this comment

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

An interesting choice of FS combinations, but I suppose it's fine. Both tests passed for me.

@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 7, 2023

Would you prefer a different combination of filesystems? Perhaps ZFS for both / and /boot?
My reasoning for doing it this way is that I want to test both scenarios separately and keep the respective other filesystem as boring as possible.

@adamcstephens
Copy link
Contributor

adamcstephens commented Dec 7, 2023

As long as upstream has documentation showing using a boot zfs pool, we should test it. This PR seems to fulfill that need. https://openzfs.github.io/openzfs-docs/Getting%20Started/NixOS/Root%20on%20ZFS.html

@ofborg test installer.separateBootZfs installer.zfsroot

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1313

@Lassulus Lassulus merged commit 14912a2 into NixOS:master Dec 28, 2023
24 checks passed
@Luflosi Luflosi deleted the test-boot-from-zfs branch December 28, 2023 15:31
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.

6 participants