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/systemd-shutdown: use prebuilt shutdown ramfs derivation #216866

Conversation

lilyinstarlight
Copy link
Member

Description of changes

I changed the shutdown ramfs to be built alongside the system config rather than be built on-the-fly at shutdown. See discussion starting at #185085 (comment) for details on motivation, but basically there's not much reason to only do this at shutdown (even if it's quick to run), dracut does not do this, and for cross systems it requires building a cross rustc toolchain for make-initrd-ng even if no other Rust programs exist on the target image just to run the one script.

This change technically could be breaking, but only if the user manually made a unit that dependend on generate-shutdown-ramfs.service, which doesn't seem like a use-case that would give someone any benefit anyway with the options available in the module. I can change the service name back to generate if someone feels otherwise that this will be a real problem.

I opted not to unpack the booted initrd image for shutdown because unpacking it is a hard problem, and that would ignore the options already available in this module (and are already used at least by the ZFS module and some people's configs).

It would be nice if we could try to use the same systemd version and other software versions that we booted with (just to ensure no issues), but that would involve modifying system top-level build and loading from something like /run/booted-system/shutdownramfs, which would on the whole be more complicated than the potential benefit I feel it would give.

cc: @ElvishJerricco @oxalica

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.

I changed the shutdown ramfs to be built alongside the system config
rather than be built on-the-fly at shutdown. See discussion starting at
NixOS#185085 (comment) for
details on motivation, but basically there's not much reason to only do
this at shutdown (even if it's quick to run), dracut does not do this,
and it requires building a cross rustc toolchain for make-initrd-ng even
if no other Rust programs exist on the target image just to run the one
script.

This change technically could be breaking, but only if the user manually
made a unit that dependend on `generate-shutdown-ramfs.service`, which
doesn't seem like a use-case that would give someone any benefit anyway
with the options available in the module. I can change the service name
back to `generate` if someone feels otherwise that this will be a real
problem.

I opted not to unpack the booted initrd image for shutdown because
unpacking it is a hard problem, and that would ignore the options
already available in this module (and are already used at least by the
ZFS module and some people's configs).

It would be nice if we could try to use the same systemd version and
other software versions that we booted with (just to ensure no issues),
but that would involve modifying system top-level build and loading from
something like `/run/booted-system/shutdownramfs`, which would on the
whole be more complicated than the potential benefit I feel it would
give.
@lilyinstarlight
Copy link
Member Author

@ofborg build nixosTests.systemd-shutdown

@winterqt
Copy link
Member

Will isn't in the systemd team, so I've requested his review directly, for posterity if nothing else :)

@ElvishJerricco
Copy link
Contributor

@lilyinstarlight I'm sure it's small, but could you comment the difference in system closure size caused by this?

If we care about the shutdown ramfs matching the initrd, we could unpack it at boot instead of shutdown, and just leave it in there all boot long. This of course consumes extra system memory though. Tradeoffs, tradeoffs :)

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Feb 18, 2023

@lilyinstarlight I'm sure it's small, but could you comment the difference in system closure size caused by this?

Yep, will do!

If we care about the shutdown ramfs matching the initrd, we could unpack it at boot instead of shutdown, and just leave it in there all boot long. This of course consumes extra system memory though. Tradeoffs, tradeoffs :)

I also thought about that but idk how much I trust /run/initramfs to continue existing from bootup to shutdown on every person's system every time...

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.

As long as we're ok with the closure size increase, this looks ok to me.

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Feb 18, 2023

Okay I just checked and the closure size difference for nixosTests.systemd-shutdown.config.nodes.machine.system.build.toplevel for before and after 03dcc0d is 598.25 MiB -> 619.32 MiB

So ~21.07 MiB increase

Is that acceptable @ElvishJerricco?

@ElvishJerricco
Copy link
Contributor

That's about what I expected. We should also probably confirm that this correctly fixes the cross compiling situation before we merge.

@ElvishJerricco
Copy link
Contributor

So, funny thing about this... It does remove the need to cross compile make-initrd-ng. But a cross-targeting rustc is still in the build graph because of #214153

@lilyinstarlight
Copy link
Member Author

So, funny thing about this... It does remove the need to cross compile make-initrd-ng. But a cross-targeting rustc is still in the build graph because of #214153

Ah, neat (guess I waited too long to actually open this PR...). Well I'll probably rethink this change then if that's no longer a motivation

What we really should be doing anyway is pivoting back to booted initrd

Do you think we could keep the unpacked version of the initrd somewhere or at least the contents file reachable from toplevel? Or is there a reliable way to unpack an arbitrary /run/booted-system/initrd?

@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Feb 19, 2023

We could always copy the files out of the initrd during initrd so that we don't have to think about any kind of archive format stuff. Of course that relies on those files remaining intact all along until shutdown. Could mount the tmpfs at /run/initramfs with ro though I guess. Not sure... I'd like to know how other distros do this part, if at all. Ah, you pointed out in the other PR that they extract the initrd from the initrd manually. Looks like they do this at shutdown via ExecStop on an otherwise dummy oneshot service that's started at boot with RemainAfterExit=yes

@lilyinstarlight
Copy link
Member Author

We could always copy the files out of the initrd during initrd so that we don't have to think about any kind of archive format stuff. Of course that relies on those files remaining intact all along until shutdown. Could mount the tmpfs at /run/initramfs with ro though I guess. Not sure...

Yeah I just worry that it may disappear while a user is mucking with mounts or something, but perhaps if they are mucking with that it's their own fault if shutdown can't pivot to back to initrd... (it's not like that's fatal, systemd just will silently not use it if /run/initramfs/shutdown does not exist)

I'd like to know how other distros do this part, if at all. Ah, you pointed out in the other PR that they extract the initrd from the initrd manually. Looks like they do this at shutdown via ExecStop on an otherwise dummy oneshot service that's started at boot with RemainAfterExit=yes

Yes and dracut does so via trying multiple different directories for the initrd associated with the running kernel version, using their own skipcpio executable (which I may extract to a separate output or even derivation from the dracut derivation in nixpkgs), and then trying different compression programs until one works

Which seems fragile and we can probably do a lot better than that given we'll have /run/booted-system/initrd guaranteed available and can theoretically know the compressor used (although we probably can't support an arbitrary compressor and also for u-boot systems that initrd link is only to the u-boot image and the original initramfs file is not in the output and idk whether that matters or not because I've never mucked with u-boot)


So I'll look more at this soon and may just implement a sketchy /run/initramfs dir/mount during bootup that it gets copied to (tbh I wonder if you can have the original initramfs / remounted at /run/initramfs during the pivot and prevent the kernel from freeing it 🤔)

@ElvishJerricco
Copy link
Contributor

tbh I wonder if you can have the original initramfs / remounted at /run/initramfs during the pivot and prevent the kernel from freeing it thinking

This is an interesting thought. I don't think systemd will let you do that though; it literally wipes the old root if it can (I've read this is because the initrd's root fs can't actually be unmounted for some reason, but I have no idea where that idea comes from or if it's true)

@lilyinstarlight
Copy link
Member Author

I'll go ahead and close this and open a new PR soon, since we'll try to do something more "correct" for this

@lilyinstarlight lilyinstarlight deleted the feature/prebuilt-shutdown-ramfs branch February 19, 2023 16:13
@oxalica
Copy link
Contributor

oxalica commented Feb 20, 2023

So, funny thing about this... It does remove the need to cross compile make-initrd-ng. But a cross-targeting rustc is still in the build graph because of #214153

That's unfortunate. Maybe it's time to make cross-targeting rustc more fast to compile instead, if it's required anyway? Only the std and friends need recompilation actually.

we'll try to do something more "correct" for this

I don't like the idea of keeping shutdown-ramfs in memory all the time, even after cherry picking some of them from original initrd (~22M systemd-shutdown closure for me, larger if zfs is also inside). Memories are much expensive than even high end SSDs. Users, especially with no swap enabled, don't want a permanent memory usage during the whole uptime just for "a more graceful shutdown".

I'm not sure if there a better way to do this now. 😢

@ElvishJerricco
Copy link
Contributor

That's unfortunate. Maybe it's time to make cross-targeting rustc more fast to compile instead, if it's required anyway? Only the std and friends need recompilation actually.

Yea rustc definitely has the chops to use the same build for a lot of targets, so it would be great if we could separate stuff like std into separate target-specific derivations. AFAIK though, that's basically unsupported by the build system. So the actual binary can do it just fine, but the tooling is not so friendly.

@flokli
Copy link
Contributor

flokli commented Feb 20, 2023

I'm pretty sure (and/or hopeful) more rust will end up in low-level system services, so having to have a rustc in the build closure is probably not too bad after all :-)

Agreed that we should probably see how we can get this to be a bit more enjoyable, and maybe have it in the binary caches for the most part.

@lilyinstarlight
Copy link
Member Author

I don't like the idea of keeping shutdown-ramfs in memory all the time, even after cherry picking some of them from original initrd (~22M systemd-shutdown closure for me, larger if zfs is also inside). Memories are much expensive than even high end SSDs. Users, especially with no swap enabled, don't want a permanent memory usage during the whole uptime just for "a more graceful shutdown".

Noted. Thinking about it more, maybe just doing best-effort /run/booted-system/initrd decompression isn't that bad of an idea, since failure to create /run/initramfs is non-fatal

Status-quo isn't the most terrible too, since I mean it is effectively equivalent to what mkinitcpio does (mkinitcpio just sketches me out a lot in general, so I'm pretty apprehensive about replicating any decisions it makes...)

@ElvishJerricco
Copy link
Contributor

I think that what we currently have is probably better than a "best effort" sort of thing.

@oxalica
Copy link
Contributor

oxalica commented Feb 22, 2023

Noted. Thinking about it more, maybe just doing best-effort /run/booted-system/initrd decompression isn't that bad of an idea.

I've tried it once but soon gave it up. Despite we can prepend arbitrary binary content before initrd via boot.initrd.prepend (mostly microcodes), we also allows custom compression command boot.initrd.compressor{,Args}. A quick search on GitHub shows there are many complex usages of these arguments, like custom compressor and options. It's hard to calculate the inverse of the given command, and it may require extra packages.

I'm pretty sure (and/or hopeful) more rust will end up in low-level system services, so having to have a rustc in the build closure is probably not too bad after all :-)

I think that what we currently have is probably better than a "best effort" sort of thing.

Yeah, especially now Linux also has some Rust code. I agree that the current solution is good enough. We should probably just keep it. The shutdown ramfs can always be disabled if necessary via systemd.shutdownRamfs.enable = lib.mkForce false;.

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

5 participants