Skip to content

nixos/systemd-boot: init boot counting#273062

Merged
RaitoBezarius merged 1 commit intoNixOS:masterfrom
JulienMalka:systemd-boot-counting
Jan 10, 2024
Merged

nixos/systemd-boot: init boot counting#273062
RaitoBezarius merged 1 commit intoNixOS:masterfrom
JulienMalka:systemd-boot-counting

Conversation

@JulienMalka
Copy link
Member

@JulienMalka JulienMalka commented Dec 9, 2023 •
edited
Loading

Description of changes

This is a spin-off of #84204, that is a PR introducing automatic boot assessment for NixOS, see here. Thanks a lot to @danielfullmer for the initial work on that topic, a lot of this PR has been heavily inspired by #84204.

Most of the design is similar and have been reimplemented to fit with the current state of the systemd-boot backend.
The handling of the default generation is done using the sort-key defined in the boot loader specification. In the first iteration of this change (#84204), it was suggested to use default in the filename of the default generation entry, but since then systemd-boot is using the version field before the filename to sort the entries so this hack is no longer possible.

Todo before merging:

  • clean the code, especially the regexes
  • add documentation
  • add nixos test showing that we still conform to the default generation

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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.

Add a 👍 reaction to pull requests you find important.

@JulienMalka JulienMalka requested a review from a team as a code owner December 9, 2023 04:46
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 9, 2023
@JulienMalka JulienMalka marked this pull request as draft December 9, 2023 04:47
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 9, 2023
@RaitoBezarius RaitoBezarius mentioned this pull request Dec 9, 2023
Closed
13 tasks
@minijackson
Copy link
Member

minijackson commented Dec 9, 2023

Thanks a lot for this! I'm very interested in this PR.

How does this work with specializations? My guess is that currently if an entry fail, it will boot to the next specialization by lexicographic order, since it will be between two "main" entries. I don't think this is what we want?

@infinisil
Copy link
Member

infinisil commented Dec 9, 2023

This definitely needs a release notes entry too :D

@RaitoBezarius RaitoBezarius mentioned this pull request Dec 11, 2023
Open
@jmbaur
Copy link
Contributor

jmbaur commented Dec 12, 2023

I'm assuming this PR will also include pulling in the upstream systemd-bless-bootservice & generator and the boot-complete target from systemd?

@JulienMalka
Copy link
Member Author

JulienMalka commented Dec 12, 2023

I'm assuming this PR will also include pulling in the upstream systemd-bless-bootservice & generator and the boot-complete target from systemd?

It's in there already, no ?

@JulienMalka
Copy link
Member Author

JulienMalka commented Dec 12, 2023

Thanks a lot for this! I'm very interested in this PR.

How does this work with specializations? My guess is that currently if an entry fail, it will boot to the next specialization by lexicographic order, since it will be between two "main" entries. I don't think this is what we want?

You are right in your guess. I don't think this is anything we can go around, boot counting is going to try the entries in the same order it will list them in the boot menu. The only way we can have the specialisations listed in a satisfactory order in the boot menu is accepting they will be tested between the main entries. The problem here is that our specialisation implementation is more a hack than a real systemd-boot feature. The real path forward should be to convince upstream to implement such a feature.
In the case of boot counting, I'll clearly explain the behavior in the documentation. If you think the behavior is dangerous (more than just annoying), I can guard specialisation + boot-counting behind an option that the user have to activate to confirm they understood the behavior that boot-counting will have with specialisations.

@minijackson
Copy link
Member

minijackson commented Dec 12, 2023

@JulienMalka documenting the behavior seems good to me. If it's understood, people can also "use" specializations as a configuration to try with boot counting.

@JulienMalka
Copy link
Member Author

JulienMalka commented Dec 12, 2023 •
edited
Loading

@JulienMalka documenting the behavior seems good to me. If it's understood, people can also "use" specializations as a configuration to try with boot counting.

Exactly, it's even not unlikely that a main generation doesn't reach boot-complete for some reason but that one of its specialisations does.

@RaitoBezarius even told me even about some usecases where you could do that by design (have for example one different kernel version by specialisation)

@jmbaur
Copy link
Contributor

jmbaur commented Dec 13, 2023 •
edited
Loading

It's in there already, no ?

I think it needs to be added here: https://github.com/nixos/nixpkgs/blob/987845b73e265113c1368efe4a5825fc7eed61bb/nixos/modules/system/boot/systemd.nix#L22 nvm, I see you added it in this PR! Just curious, how does the test work without adding the bless-boot-generator to systemd.generators? Isn't that what links in the systemd-bless-boot.service unit on startup?

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes labels Jan 5, 2024
@JulienMalka JulienMalka marked this pull request as ready for review January 6, 2024 00:31
@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 6, 2024

This is ready for review.

@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 6, 2024 •
edited
Loading

@ofborg test systemd-boot

Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
Mic92
Mic92 reviewed Jan 7, 2024
View reviewed changes
@Mic92
Copy link
Member

Mic92 commented Jan 7, 2024

Should this work with lanzaboote? Than I would give this a test run on my machine.

@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 7, 2024

Should this work with lanzaboote? Than I would give this a test run on my machine.

No, this is an implementation for the systemd-boot backend only.

@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 7, 2024

Thanks for the suggestions @Mic92, applied all of them.

RaitoBezarius
RaitoBezarius approved these changes Jan 10, 2024
View reviewed changes
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.

I'm not the expert on the builder, but overall, everything looks good to me, it has solid testing and has been sitting for a while. I am inclined to send it, but I will leave you the honor to choose when to do it, Julien.

@RaitoBezarius RaitoBezarius merged commit a6303da into NixOS:master Jan 10, 2024
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jan 10, 2024

(Discussed in private for the merge.)

@spiage
Copy link

spiage commented Jan 10, 2024

Hello!
Is it expected behaviour?

building '/nix/store/j8l6jd8n0z48ykxm2w85kybl92z86pah-nixos-system-nixos-24.05.20240110.1097a6b.drv'...
$ sudo --preserve-env=NIXOS_INSTALL_BOOTLOADER -- nix-env -p /nix/var/nix/profiles/system --set /nix/store/flihcv9sd6pd7vagiiga7mdbch4vp42j-nixos-system-nixos-24.05.20240110.1097a6b
Using systemd-run to switch configuration.
$ sudo --preserve-env=NIXOS_INSTALL_BOOTLOADER -- systemd-run -E LOCALE_ARCHIVE -E NIXOS_INSTALL_BOOTLOADER --collect --no-ask-password --pty --quiet --same-dir --service-type=exec --unit=nixos-rebuild-switch-to-configuration --wait /nix/store/flihcv9sd6pd7vagiiga7mdbch4vp42j-nixos-system-nixos-24.05.20240110.1097a6b/bin/switch-to-configuration switch
Traceback (most recent call last):
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 439, in
main()
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 427, in main
install_bootloader(args)
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 383, in install_bootloader
remove_old_entries(gens, entries)
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 303, in remove_old_entries
for disk_entry in disk_entries:
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 190, in scan_entries
yield DiskEntry.from_path(path)
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/8dldxq4bh9qy3vgaqnvq9hkpfwia0xdy-systemd-boot", line 94, in from_path
assert "sort-key" in entry_map
^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
warning: error(s) occurred while switching to the new configuration

@spiage
Copy link

spiage commented Jan 10, 2024

@JulienMalka @Mic92 @RaitoBezarius

@RaitoBezarius RaitoBezarius mentioned this pull request Jan 11, 2024
Merged
@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 11, 2024

@JulienMalka @Mic92 @RaitoBezarius

Not normal, thanks for catching that.
Reverted for now and I'll fix compatibility with old generation tomorrow and reopen.

@spiage
Copy link

spiage commented Jan 11, 2024 •
edited
Loading

I have this entries (hope it can be useful):

spiage@nixos:/boot/loader/entries$ ls
nixos-generation-2064.conf  nixos-generation-2078.conf  nixos-generation-2092.conf  nixos-generation-2106.conf  nixos-generation-2120.conf  nixos-generation-2134.conf  nixos-generation-2148.conf
nixos-generation-2065.conf  nixos-generation-2079.conf  nixos-generation-2093.conf  nixos-generation-2107.conf  nixos-generation-2121.conf  nixos-generation-2135.conf  nixos-generation-2149.conf
nixos-generation-2066.conf  nixos-generation-2080.conf  nixos-generation-2094.conf  nixos-generation-2108.conf  nixos-generation-2122.conf  nixos-generation-2136.conf  nixos-generation-2150.conf
nixos-generation-2067.conf  nixos-generation-2081.conf  nixos-generation-2095.conf  nixos-generation-2109.conf  nixos-generation-2123.conf  nixos-generation-2137.conf  nixos-generation-2151.conf
nixos-generation-2068.conf  nixos-generation-2082.conf  nixos-generation-2096.conf  nixos-generation-2110.conf  nixos-generation-2124.conf  nixos-generation-2138.conf  nixos-generation-2152.conf
nixos-generation-2069.conf  nixos-generation-2083.conf  nixos-generation-2097.conf  nixos-generation-2111.conf  nixos-generation-2125.conf  nixos-generation-2139.conf  nixos-generation-2153.conf
nixos-generation-2070.conf  nixos-generation-2084.conf  nixos-generation-2098.conf  nixos-generation-2112.conf  nixos-generation-2126.conf  nixos-generation-2140.conf  nixos-generation-2154.conf
nixos-generation-2071.conf  nixos-generation-2085.conf  nixos-generation-2099.conf  nixos-generation-2113.conf  nixos-generation-2127.conf  nixos-generation-2141.conf  nixos-generation-2155.conf
nixos-generation-2072.conf  nixos-generation-2086.conf  nixos-generation-2100.conf  nixos-generation-2114.conf  nixos-generation-2128.conf  nixos-generation-2142.conf  nixos-generation-2156.conf
nixos-generation-2073.conf  nixos-generation-2087.conf  nixos-generation-2101.conf  nixos-generation-2115.conf  nixos-generation-2129.conf  nixos-generation-2143.conf  nixos-generation-2157.conf
nixos-generation-2074.conf  nixos-generation-2088.conf  nixos-generation-2102.conf  nixos-generation-2116.conf  nixos-generation-2130.conf  nixos-generation-2144.conf  nixos-generation-2158.conf
nixos-generation-2075.conf  nixos-generation-2089.conf  nixos-generation-2103.conf  nixos-generation-2117.conf  nixos-generation-2131.conf  nixos-generation-2145.conf  nixos-generation-2159.conf
nixos-generation-2076.conf  nixos-generation-2090.conf  nixos-generation-2104.conf  nixos-generation-2118.conf  nixos-generation-2132.conf  nixos-generation-2146.conf  nixos-generation-2160.conf
nixos-generation-2077.conf  nixos-generation-2091.conf  nixos-generation-2105.conf  nixos-generation-2119.conf  nixos-generation-2133.conf  nixos-generation-2147.conf
spiage@nixos:/boot/loader/entries$

@JulienMalka
Copy link
Member Author

JulienMalka commented Jan 11, 2024

I know what the problem is, there is an assertion that there is a "sort-key" line in entries, which doesn't exist for entries generated with older versions of the builder.

@spiage
Copy link

spiage commented Jan 11, 2024

I am glad that at least I can help the project in this way )
Now there is no build issues

@JulienMalka JulienMalka mentioned this pull request Jan 26, 2024
Closed
13 tasks
@peterhoeg
Copy link
Member

peterhoeg commented Feb 5, 2024

I was about to do a write-up for a GSoC project to do this, so it's fantastic news that this is already here. Thank you so much to all involved!

@nixos-discourse
Copy link

nixos-discourse commented Mar 13, 2024

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

https://discourse.nixos.org/t/gsoc-2024-nixos-participant/41380/1

@aviallon
Copy link
Contributor

aviallon commented Nov 12, 2024

Is someone working on fixing this marvel?

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

Reviewers

@Mic92 Mic92 Mic92 left review comments

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

@ElvishJerricco ElvishJerricco Awaiting requested review from ElvishJerricco

@cole-h cole-h Awaiting requested review from cole-h

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 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 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@JulienMalka @minijackson @infinisil @jmbaur @Mic92 @RaitoBezarius @spiage @peterhoeg @nixos-discourse @aviallon