-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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/systemd-boot: Add mirroredBoots #246897
Conversation
Only tested with # Use the systemd-boot EFI boot loader.
boot.loader.systemd-boot.enable = true;
boot.loader.efi.canTouchEfiVariables = true;
# https://github.com/NixOS/nixpkgs/pull/246897
boot.loader.systemd-boot.mirroredBoots = [
"/boot0"
"/boot4"
]; Any plans to incorporate this into And what is the reason to rename |
I can really accept this only with a nixosTest, the code looks pretty much good though. |
Renamed |
i'll try to get to it tonight |
yeah I haven't been able to figure out how to make a nixosTest with a custom partition scheme setup before the bootloader is installed |
@RaitoBezarius can you take a look at the second commit I just pushed, I feel like I'm so close to it working but I can't figure out exactly what I'm doing wrong |
This reminds me of #226692 with a slightly different scope. However I'm not sure I love the idea of mirrored boot. It seems like there is a potential for side effects by calling Also see this comment about the dangers of doing so: #152155 (comment) |
That's talking about doing mdadm raid1 on your ESP...
I mean there's also |
The linked thread is still insightful:
systemd/systemd#12468 (comment) mirroredBoots would suffer from the same issue even if its not RAID. This also means that just by virtue of implementing this bespoke feature we will make our lives harder keeping up with systemd updates since they have already signalled that mirroring the ESP is not something they even consider. Couldn't you implement this more cleanly by writing a systemd service that checks for file changes on the ESP and then copies all files to the configured locations? This would also take care of multi boot, boot counting etc.
I believe |
I completely agree with not following grub A systemd service should work, but that kinda goes against the use case of mirroring the ESP: Booting with complete redundancy |
Why? AFAIU, mirroredBoots suffer from none of these issues. But I would double check how
Why would that be any better? That seems just like a more brittle way to implement mirroredBoots. Why do you think boot counting breaks with mirroredBoots? |
The issue is that if the bootloader or another OS (when you dual boot) modifies files on the ESP, these modifications are not accounted for when you mirror the ESP via the mechanism from this PR. If you just rsync the entire ESP to a different location, you also copy these modifications. I'd also wager that rsyncing your ESP is actually less brittle because it just copies things instead of calling a stateful installation procedure repeatedly.
As fas as I can tell, your implementation doesn't guarantee that all ESPs are written in a single transaction (i.e. when writing to one fails, others might still be written to). So then that's not better than having a service that (within a few seconds of writing to the main ESP) copies the main ESP to the backups. I'd argue both ways have the same (weak) guarantees on redundancy. |
not if the modifications are done to the slave... |
I'm thinking the exact opposite. I think syncing the boot counting or other state across multiple ESPs somewhat defeats the purpose -- if all boot attempts are are spent on device A, you want firmware to try device B next, but if both devices have the same state now (thanks to background rsync) I think firmware will not attempt device B because it too seems to have spent all boot attempts. With a systemd service / rsync I'd also expect difficulties with bidirectional syncing (already mentioned in this thread) and that the time window where the mirrored devices are out of sync increases. My mental model of the ESP is it only changes when NixOS boot configuration changes, and firmware can have a bit of state on the side (not affecting NixOS). That means (1) we only need to write to the ESP when NixOS changes (no background sync needed), and (2) we shouldn't mess with the firmware state (backgrouns sync harmful). I think this is what this PR does. |
Yes and this is precisely what you do not want. With this pr, the information gets duplicated from a source that is known to be intact. When you rsync the existing boot partition, any error present there (e.g. due to corruption of the disk) will propagate to the second copy. |
We could always go with the nuclear option... Wipe each ESP every time Edit: this is a joke... Mostly |
Getting following with a deploy-rs run on activation:
|
You joke but... #226168 |
Another thing to note is the "random seed": https://systemd.io/RANDOM_SEEDS/
(Emphasis mine) This is actually security critical. |
Isn't that only if you rsync though? |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
installDirs = | ||
if cfg.mirroredBoots != [] | ||
then cfg.mirroredBoots | ||
else [efi.efiSysMountPoint]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought was that mirroredBoots should be additional mountpoints, but seeing this I guess the implementation is only these mountpoints (the "main" ESP mountpoint gets ignored)? Am I the only one that find that surprising?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, and AFAIU the grub.nix
case, mirroredBoots
are additionals mountpoints there:
nixpkgs/nixos/modules/system/boot/loader/grub/grub.nix
Lines 708 to 710 in ec6cc9c
boot.loader.grub.mirroredBoots = optionals (cfg.devices != [ ]) [ | |
{ path = "/boot"; inherit (cfg) devices; inherit (efi) efiSysMountPoint; } | |
]; |
else [efi.efiSysMountPoint]; | ||
in | ||
pkgs.writeShellScript "install-systemd-boot.sh" | ||
(lib.concatMapStrings (x: "${checkedSystemdBootBuilder x} \"$@\"\n") installDirs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, like in grub.nix, the usual error detection set -e
should be activated, and maybe also set -u
.
Why keeping on passing $@
? I don't see any argument neither used, nor passed (so far).
Nitpick: here each mountpoint generates a new derivation, the mountpoint could instead be passed as an envvar mountpoint=${escapeShellArg x} ${checkedSystemdBootBuilder}
. Again that's likely just a few derivations in practice, so just a nitpick.
@@ -238,6 +245,17 @@ in { | |||
''; | |||
}; | |||
|
|||
mirroredBoots = lib.mkOption { | |||
type = lib.types.listOf lib.types.str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grub.nix
's mirroredBoots' type is more sophisticated, maybe it would be more correct to use the same or a subset.
nikstur is very clearly against this, so I'm just going to close this and keep using grub for now |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Description of changes
Closes #152155
adds
boot.loader.systemd-boot.mirroredBoots
only renamed
efiSysMountPoint
tomountPoint
in the python scriptThings done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)