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

systemd: 252.5 -> 253 #216826

Merged
merged 5 commits into from
Mar 13, 2023
Merged

systemd: 252.5 -> 253 #216826

merged 5 commits into from
Mar 13, 2023

Conversation

gdamjan
Copy link
Contributor

@gdamjan gdamjan commented Feb 17, 2023

Description of changes
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.

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 17, 2023

still to do:

  1. check if the not-applied patches are really not needed any more
  2. remove them if so (if not, find a new fix)
  3. make sure the libp11-kit.so.0 comment is correct
  4. improve the commit message with more info

if anyone can help by commenting here, that would be nice

@flokli
Copy link
Contributor

flokli commented Feb 17, 2023

@gdamjan i initially already prepared a draft PR, check #212624

Happy to close that one in favor of yours, but you might want to source some info from there :-)

@gdamjan gdamjan force-pushed the systemd-253 branch 3 times, most recently from 9b54886 to 6853645 Compare February 17, 2023 18:50
@gdamjan gdamjan marked this pull request as ready for review February 18, 2023 01:22
@gdamjan gdamjan requested a review from a team as a code owner February 18, 2023 01:22
@flokli flokli mentioned this pull request Feb 18, 2023
13 tasks
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

LGTM. We should probably still do some more testing before merging.

@flokli
Copy link
Contributor

flokli commented Feb 20, 2023

I tried switching my system to this nixpkgs commit, but aiohttp and samba failed to build.

I'm not sure if it's fixed in staging since you opened that PR or not. I just merged in staging in a local branch and will kick off another build to try again.

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 20, 2023

A live upgrade (or "switch" in nixos parlance) will need to trigger the udev rules, so lets block the merging of this PR before we investigate how Nixos does that.

One of outcomes of the daemon-reexec is the change of boot.mount (for example) to use new style per-partition diskseq symlinks:

[Mount]
What=/dev/disk/by-diskseq/1-part1
Where=/boot
Type=vfat
Options=umask=0077,noexec,nosuid,nodev,rw

this will make subsequent mounting of the boot partition (or the ESP) to fail, because only the new udevd will create those symlinks.

ps.
here's the relevant change in Archlinux
archlinux/svntogit-packages@cf15dcc

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 20, 2023

I'm guessing this is where the systemd switch happens:
nixos/modules/system/activation/switch-to-configuration.pl

@flokli
Copy link
Contributor

flokli commented Feb 21, 2023

cc @dasJ for input regarding the activation script logic.

@dasJ
Copy link
Member

dasJ commented Feb 21, 2023

I don't really understand the issue and what needs to be done tbh

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 21, 2023

I don't really understand the issue and what needs to be done tbh

so nixos-rebuild switch will run systemctl daemon-reload and daemon-reexec. That will replace some of the .mount units to use new-style What= (the source for the mount) that uses by-diskseq paths. But if udev is not re-triggered at the same time, those paths will not be created.

Now, worst case, this can potentially make /boot (or /efi) inaccessible, so updating the boot-loader will fail.

The recommendation by systemd developers is to always do udevadm control --reload; udevadm trigger when updating systemd/udev.

@dasJ
Copy link
Member

dasJ commented Feb 21, 2023

I'd personally go with an activation script snippet rather than touching https://switch-to-configuration.pl
The main upshot is that a lot more people actually understand the shell language (compared to Perl) and it doesn't introduce yet another oddity into a script that handles tons of edge cases.
We already have a systemd-timesyncd-migration snippet there so adding another one should be no issue. I checked and the snippets get executed before systemd is restarted. This is probably wrong. So you can just write a unit name to /run/nixos/restart-list and https://switch-to-configuration.pl will restart the unit along the other ones.

You can get the old systemd version from /run/current-system (if it exists) and the new one from $systemConfig.

@arianvp
Copy link
Member

arianvp commented Feb 21, 2023

I think we might not have this issue given we set up explicit /etc/fstab entries foor /boot with the default hardware-configuration.nix that uses /etc/disks/by-uuid/<blah> to set up the /boot mount point instead of letting systemd synthesise the .mount file

This only affects people who remove the /boot entry from fileSystems and use the systemd-builtin boot.automount. I happen to be relying on this behaviour on my hosts but it's definitely not the default in NixOS.

However retriggering udev rules is probably still a good idea; albeit maybe not a blocker?

@arianvp
Copy link
Member

arianvp commented Feb 21, 2023

Retriggering udev rules will need wider testing. E.g. lvm might run into weird issues that we might need to test thoroughly. Would an alternative be adding a Release note that people should do a nixes-rebuild boot instead of nixes-rebuild switch if they rely on the implicit boot.mount unit instead of /etc/fstab ?

@winterqt
Copy link
Member

Hi! Just gonna drive by here with a quick snippet from CONTRIBUTING.md:

Package version upgrades usually allow for simpler commit messages, including attribute name, old and new version, as well as a reference to the relevant release notes/changelog. Every once in a while a package upgrade requires more extensive changes, and that subsequently warrants a more verbose message.

You've definitely nailed the second half, but if any more changes are required to this commit, could you add a link to the upstream changelog?

Thank you!

@flokli
Copy link
Contributor

flokli commented Feb 22, 2023

I'd personally prefer adding a note to the release notes, rather than the activation script.

@winterqt
Copy link
Member

I meant the commit message (that's what the quoted section concerns), not the activation script.

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 22, 2023

I meant the commit message (that's what the quoted section concerns), not the activation script.

will do later today. I understood you the first time :D

@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 22, 2023

Ok, just to make it clear. Things left to do are:

  • improve the commit message with the NEWS from systemd v253
  • add a note in the release notes about a preference for nixes-rebuild boot rather than nixes-rebuild switch - since in some rare cases switch might fail.

@arianvp
Copy link
Member

arianvp commented Feb 23, 2023

Correct

@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 labels Feb 23, 2023
@ofborg ofborg bot requested a review from flokli February 23, 2023 12:11
@gdamjan
Copy link
Contributor Author

gdamjan commented Feb 23, 2023

ps.
we will probably get v253.1 sometime next week, so if we wanna reduce rebuilds, we might want to wait for that?

@flokli
Copy link
Contributor

flokli commented Feb 23, 2023

We don't trigger builds for staging anyways, but whenever a new staging-next cycle happens. If there's no really bad regression that 253.1 will fix I'd rather see this merged in first.

@flokli
Copy link
Contributor

flokli commented Mar 3, 2023

253.1 has been released, and we're about to start a new staging cycle. As agreed on Matrix I'll give 253.1 some testing and we'll merge this in.

@flokli
Copy link
Contributor

flokli commented Mar 4, 2023

Rolling back, at least with these cherrypicked into master, I get failures in the nixosTests.systemd-initrd-shutdown tests.

Things like

[ 0.576734] (sd-g[65]: Failed to overmount /tmp/: No such file or directory

and

machine # [ 0.646526] systemd[1]: Failed to fork off sandboxing environment for executing generators: Protr

I don't see these with 252, so this needs some further testing.

@gdamjan
Copy link
Contributor Author

gdamjan commented Mar 5, 2023

yeah, the /tmp mount point is required in the initrd cpio archive now it seems (since systemd/systemd#25723 I gather).

I've fixed hacked around the issue with this change:

diff --git a/nixos/modules/system/boot/systemd/initrd.nix b/nixos/modules/system/boot/systemd/initrd.nix
index cf76704577f..ff148266225 100644
--- a/nixos/modules/system/boot/systemd/initrd.nix
+++ b/nixos/modules/system/boot/systemd/initrd.nix
@@ -358,6 +358,7 @@ in {
       };
 
       contents = {
+        "/tmp/.dummy".text = "";
         "/init".source = "${cfg.package}/lib/systemd/systemd";
         "/etc/systemd/system".source = stage1Units;
 

… since I didn't know how to create a directory in the initrd. My VM boots now.

gdamjan and others added 3 commits March 5, 2023 04:35
systemd v253 changelog/NEWS:
https://github.com/systemd/systemd/blob/v253/NEWS

NixOS changes:
0007-hostnamed-localed-timedated-disable-methods-that-cha.patch was
dropped, because systemd gained support to handle read-only /etc.

*-add-rootprefix-to-lookup-dir-paths.patch required some updates too,
as src/basic/def.h moved to src/basic/constants.h.

systemd/systemd#25771 switched p11kit to become
dlopen()'ed, so we need to patch that path.

added a note to the 23.05 release notes to recommend `nixos-rebuild boot`

Co-authored-by: Florian Klink <[email protected]>
Changelog:

```
6c327d74aa hwdb: update to 11875a98e4f1c31e247d99e00c7774ea3653bafd
0b81fcd16d chase-symlinks: Always open a dirfd to the root directory
aa20a210a0 chase-symlinks: chase_symlinks_at() AT_FDCWD fixes
bb3e44323b escape: add missing non-NULL parameter assertions
c4e7cf2bd7 test-escape: Add tests for escaping bogus UTF-8 sequences
e906fd2421 escape: Ensure that output is always valid UTF-8
1a22006574 virt: correctly detect QEMU emulated pSeries guests
5ee19fdfa0 psi-util: fix error handling
9ffa0d439f journald: remove triplicate logging about failure to write log lines
4f7f93cc6a journald: downgrade various log messages from LOG_WARNING to LOG_INFO
a2dc51cd8c journald: make sure shall_try_append_again() logs about all return codes passed in, not just some
144ac494ec systemctl: print better message if default target is masked
791754f683 Revert "dissect-image: don't probe swap partitions needlessly"
d0e7841dce rules: remove redundant duplicate comparisons
dc98d58dd8 man: add two missing commands to synopsys
e093acd062 core/dbus-socket: check the socket path is absolute
a719c2ec2f sd-event: fix error handling
58c821af60 sd-event: always initialize sd_event.perturb
2bfb07b22f systemctl: show "Until:" field only for service and scope units
d9abd8babe tmpfiles.d: drop misleading comment
0f4dbe6367 Enable TPM by default with SetCredentialEncrypted
8d8240bdf6 stub: Fix unaligned read
44c2ff5b1e efi: drop executable-stack bit from .elf file
f2460b78b9 logind-session: make stopping of idle session visible to admins
1947b9939c sleep: check if we're on AC power before checking battery capacity
452cad62c8 install: fail early if specifier expansion failed
eae11e3f06 homectl: add missing break
9024afb994 core/manager: falling back to execute generators without sandboxing
aac692160e man/tmpfiles.d: adjust the table in synopsis, improve spelling
d2739b8c14 test: disable pipefail when testing interactive firstboot
755431b233 ukify: Set fast_load option when parsing PE files
343e90462f core: permit sending augmented enable/disable methods
ba1cb4156b process-util: show requested process name in the log
5140da8937 systemctl: edit: fix double free of instanced name
c4cdbb978f journalctl: fix output when --lines is used with --grep
6dafcad55c loop-util: fix error condition and return value
ec6c1fbf7d Correct journal misspell
6b6df9a845 cryptsetup: check the existence of salt by salt_size > 0
cd5de2811a boot: Fix assertion failure
01b90e1588 pid1: generate compat warning for SystemCallArchitectures= if seccomp is off
a3177cbe54 core/mount: fix default target for /sysusr/usr and its child
3168bda640 mkosi: configure multiarch libdir in debian/ubuntu builds
51b7acfcef tpm2: fix build failure without openssl
a88e35bf95 resolved: Fall back to TCP if UDP is blocked
```
systemd now requires the /tmp mount point in the initrd cpio archive
since systemd/systemd#25723

setting `"/tmp/.keep".text` will create the directory.

this fixes a boot failure:
```
(sd-gens): Failed to overmount /tmp/: No such file or directory
```
@github-actions github-actions bot added the 8.has: module (update) This PR changes an existing module in `nixos/` label Mar 5, 2023
`[email protected]` and `systemd-growfs-root.service` became real units since:
systemd/systemd@50072cc

we need to add them to the nixos module so growfs works again
@flokli
Copy link
Contributor

flokli commented Mar 12, 2023

@ElvishJerricco do you have any thoughts on this?

@ElvishJerricco
Copy link
Contributor

I think the /tmp/.keep fix is fine. However, this currently fails to cross compile because the new ukify python script in the output refers to the build platform's python (see: disallowedReferences in systemd/default.nix). I would prefer to keep the ukify script if we can, but I'm not sure what the cleanest way would be to have it use the host platform's python. Also, it needs the pefile python package included.

@gdamjan
Copy link
Contributor Author

gdamjan commented Mar 12, 2023

I might not have the bandwidth to fix the ukify issue soon, so I'd suggest we disable it temporary, merge this PR (if there are no other issues).

Then we can tackle the ukify issue later.

@gdamjan
Copy link
Contributor Author

gdamjan commented Mar 12, 2023

ps.
with

@@ -501,6 +501,7 @@ stdenv.mkDerivation (finalAttrs: {
     # Upstream defaulted to disable manpages since they optimize for the much
     # more frequent development builds
     "-Dman=true"
+    "-Dukify=false"
 
     "-Defi=${lib.boolToString withEfi}"
     "-Dgnu-efi=${lib.boolToString withEfi}"

nix build -f . pkgsCross.aarch64-multiplatform.systemd does compile

@ElvishJerricco
Copy link
Contributor

ElvishJerricco commented Mar 12, 2023

I'd suggest we disable it temporary, merge this PR (if there are no other issues).

Then we can tackle the ukify issue later.

That sounds fine to me.

disable it just temporarily, until a solution is found for the
cross-compilation dependency on python

see NixOS#216826 (comment)
for more context
@flokli
Copy link
Contributor

flokli commented Mar 13, 2023

The eval error seems to be a timeout, because ofborg started building something.

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 6.topic: systemd 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: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants