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/stratis: initrd support for stratis root volumes #229767

Merged
merged 10 commits into from
May 25, 2023
Merged

nixos/stratis: initrd support for stratis root volumes #229767

merged 10 commits into from
May 25, 2023

Conversation

mberndt123
Copy link
Contributor

@mberndt123 mberndt123 commented May 3, 2023

Hey,

I added initrd support for booting from a Stratis volume. It's mostly based on the way that it's done in Stratis' dracut module, except that I've decided not to use the stratis_setup_generator and instead just added a systemd service configuration with the required STRATIS_ROOTFS_UUID env variable.

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.

@mberndt123 mberndt123 requested a review from dasJ as a code owner May 3, 2023 23:17
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels May 3, 2023
@ofborg ofborg bot requested a review from NickCao May 3, 2023 23:48
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 3, 2023
Copy link
Contributor

@ElvishJerricco ElvishJerricco left a comment

Choose a reason for hiding this comment

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

It'd be great to get something in nixos/tests/ for this.

nixos/modules/system/boot/stratisroot.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/stratisroot.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/stratisroot.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/stratisroot.nix Outdated Show resolved Hide resolved
@NickCao
Copy link
Member

NickCao commented May 4, 2023

Still I prefer using the generator, that would give us the opportunity to use stratis in the initrd without being restricted to the rootfs.

@mberndt123
Copy link
Contributor Author

@NickCao ,

I'm afraid I don't understand your comment regarding the generator. All it does is read the stratis.rootfs.pool_uuid kernel parameter and generate a service unit with an Environment=STRATIS_ROOTFS_UUID=… line from that. The reason they do it that way is that you can use the same initrd to boot from different stratis volumes just by changing the kernel command line. But in NixOS that is impossible anyway because there is no root=… parameter on the kernel command line, so being able to configure the pool UUID on the kernel command line is pointless.

@NickCao
Copy link
Member

NickCao commented May 5, 2023

Ah I confuse the fstab generator with the root setup ones, now I see that your implementation is reasonable. A bit busy to review this these days, would you mind adding a test for this first? A side note: why split the initrd related files to a separate output? These are merely text files and should be pretty small.

@mberndt123
Copy link
Contributor Author

@NickCao, @ElvishJerricco,

There is now an installer test for this. Since Stratis can be used without encryption, Plymouth is actually not strictly required, so I've decided to remove the Plymouth requirement. And since the nixos-generate-config script generates device names like /dev/disk/by-uuid/2862e89f-b2d6-4a4b-83d4-61c4a5aecc3c rather than /dev/stratis/foo/bar and that works just as well, I've decided to remove that assertion too.

@mberndt123 mberndt123 requested a review from ElvishJerricco May 8, 2023 11:01
@ElvishJerricco
Copy link
Contributor

@mberndt123 I'm not a fan of this amendConfig concept, since we have extraConfig already. I can see why it's done; we don't know at eval time what the UUID is going to be. But I think there's a couple of alternatives that would be better.

  1. You could put in extraConfig something like boot.stratis.rootPoolUuid = builtins.readFile ./uuid.txt; and then create /mnt/etc/nixos/uuid.txt in createPartitions.
  2. If stratis will allow you to manually select a UUID when creating the pool, you could just hard code one.

I would prefer the second, but I don't know if stratis will allow you to choose the UUID.

That said, this does raise an interesting point. Shouldn't the boot.stratis.rootPoolUuid option be set in hardware-configuration.nix and generated by nixos-generate-config?

nixos/tests/installer.nix Outdated Show resolved Hide resolved
nixos/tests/installer.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/stratisroot.nix Outdated Show resolved Hide resolved
mberndt123 added 3 commits May 8, 2023 23:18
it is now possible to supply a stratis pool uuid
for every filesystem, and if that filesystem
is required for boot, the relevant pool will be
started in the initramfs.
@mberndt123 mberndt123 requested a review from ElvishJerricco May 17, 2023 03:02
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels May 17, 2023
@mberndt123
Copy link
Contributor Author

I've now changed this so that the stratis pool UUID can be configured for every individual file system, and if any of them are required for booting, then initramfs support will be enabled automatically.

unitConfig.DefaultDependencies = "no";
conflicts = [ "shutdown.target" "initrd-switch-root.target" ];
onFailure = [ "emergency.target" ];
unitConfig.OnFailureJobMode = "isolate";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isolate? Seems like the upstream units all use replace-irreversibly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, I just modeled it after this:
https://github.com/stratis-storage/stratisd/blob/7d0125be75a68d25329a6e72db4b581fadee536b/src/bin/generators/stratis_setup_generator.rs#L44
I can change it to replace-irreversibly if you want.

Comment on lines +39 to +47
stratis.poolUuid = lib.mkOption {
type = types.uniq (types.nullOr types.str);
description = lib.mdDoc ''
UUID of the stratis pool that the fs is located in
'';
example = "04c68063-90a5-4235-b9dd-6180098a20d9";
default = null;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how I feel about extending the fileSystems options for this. Can't multiple FSes be in the same pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. But I don't see how that creates a problem. Due to how attrsets.mapAttrs' works, this is automatically deduplicated, so only one stage 1 service unit will be generated per pool.

I could put this information somewhere else, I just found it convenient this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I think this makes the most sense. Associating it with the FS gives the most useful information.

nixos/tests/installer.nix Show resolved Hide resolved
nixos/tests/installer.nix Outdated Show resolved Hide resolved
@ElvishJerricco ElvishJerricco merged commit fe43923 into NixOS:master May 25, 2023
@ElvishJerricco
Copy link
Contributor

Thanks!

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants