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/switch-to-configuration: add sysinit-reactivation.target #271067

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

nikstur
Copy link
Contributor

@nikstur nikstur commented Nov 30, 2023

Description of changes

Second try of #269983

This solution is my interpretation of what @ElvishJerricco described in this comment: #269983 (comment)

Although this doesnt remove any custom code from stc, now at least we can restart multiple services that are pulled in by sysinit.target while respecting the ordering between them (e.g. sysusers runs before tmpfiles).

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.

Priorities

Add a 👍 reaction to pull requests you find important.

@nikstur nikstur requested review from a team and dasJ as code owners November 30, 2023 00:21
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: systemd labels Nov 30, 2023
@nikstur
Copy link
Contributor Author

nikstur commented Nov 30, 2023

@ofborg test switchTest acme

@dasJ
Copy link
Member

dasJ commented Nov 30, 2023

In any case: This is missing test cases

@nikstur
Copy link
Contributor Author

nikstur commented Nov 30, 2023

In any case: This is missing test cases

This test: 28a3dc9#diff-01800945c1d1316489cb95dcf6d13d9e33660ffc8a4f7b952ec20873cd46ef8a excerices this new functionality. It wouldn't work when we use the direct tmpfiles invocation because it needs sysusers to run BEFORE tmpfiles.

Also the acme tests excercises this. It was the test that showed that the previous change was a channel blocker.

@nikstur
Copy link
Contributor Author

nikstur commented Nov 30, 2023

I'll add a specific test case.

@ElvishJerricco
Copy link
Contributor

@dasJ Although this doesn't directly contribute to removing perl from activation, I think it's an important step. Having a logic for reconfiguring the system via systemd seems important to me.

@r-vdp
Copy link
Contributor

r-vdp commented Dec 7, 2023

When switching to a generation with this PR, from one that didn't have it, there's this error during activation:

stopping the following units: systemd-tmpfiles-setup.service
Failed to stop systemd-tmpfiles-setup.service: Operation refused, unit systemd-tmpfiles-setup.service may be requested by dependency only (it is configured to refuse manual start/stop).
See system logs and 'systemctl status systemd-tmpfiles-setup.service' for details.

and the service is not actually restarted.

This is probably due to the original service setting RefuseManualStop = yes but switch-to-configuration.pl only looks at the new service config (which has RefuseManualStop = no) to decide whether it will stop a service or not.

It's probably not a huge issue in the end, but maybe we should at least mention this in the release notes?

@AleXoundOS
Copy link
Contributor

AleXoundOS commented Dec 13, 2023

Does this PR support the case when systemd-tmpfiles creates files in a filesystem location, which must be mounted first?
It looks like the upstream switch-to-configuration.pl works in the following order:

  1. parses fstab, but does not mount filesystems (leaving this to local-fs.target)
  2. runs systemd-tmpfiles command
  3. restarts and starts systemd units, perhaps one of which is local-fs.target?

I'm not sure about local-fs.target, but it looks like systemd-tmpfiles is called before mounting... Probably #262295 is related.

@nikstur
Copy link
Contributor Author

nikstur commented Dec 25, 2023

It's probably not a huge issue in the end, but maybe we should at least mention this in the release notes?

@r-vdp I added a release note. Hope that describes the issue clearly enough.

Does this PR support the case when systemd-tmpfiles creates files in a filesystem location, which must be mounted first?

@AleXoundOS If I understand this correctly, this is indeed a limitation of the previous system because the upstream systemd-tmpfiles unit is configured to run after local-fs.target but this is not respected right now. With the changes I propose, you can indeed order this correctly but you'd need to add the dependency to local-fs.target as described in the docs. I'd be happy to review a PR if you're interested in doing this.

@nikstur
Copy link
Contributor Author

nikstur commented Dec 25, 2023

@ofborg test sysinit-reactivation acme switchTest

@nikstur nikstur force-pushed the sysinit-reactivation branch from f7c8978 to 7518b07 Compare December 26, 2023 02:02
@nikstur nikstur requested review from cole-h and Mic92 December 26, 2023 16:49
@nikstur
Copy link
Contributor Author

nikstur commented Dec 26, 2023

@dasJ can I get you to review this one again? I added a test as you requested.

@AleXoundOS
Copy link
Contributor

AleXoundOS commented Dec 26, 2023

With the changes I propose, you can indeed order this correctly but you'd need to add the dependency to local-fs.target as described in the docs.

I'm afraid, that simply making systemd-tmpfiles dependent on FS mounts will potentially introduce locking of OS booting, e.g. if some non-crucial external FS is unavailable. It seems running systemd-tmpfiles is mandatory for OS internals, even systemd itself to work properly.
Maybe in order to circumvent locking of OS booting entirely we need to use --exclude-prefix=PATH option to ignore non-system rules and use --prefix=PATH for additional runs of systemd-tmpfiles. Thus, being more granular in dependencies. But I'm not sure whether it is the right approach.

@nikstur
Copy link
Contributor Author

nikstur commented Dec 26, 2023

I'm afraid, that simply making systemd-tmpfiles dependent on FS mounts will potentially introduce locking of OS booting,

systemd-tmpfiles already depends on local-fs.target

However, to make local-fs.target correctly REactivate, you need to add an additional dependency on sysinit-reactivation.target to it.

@r-vdp
Copy link
Contributor

r-vdp commented Dec 27, 2023

@nikstur I think there's an issue still because systemd-setup-tmpfiles.service calls systemd-tmpfiles with the --boot option, but this can break running systems when invoked during a switch because it also cleans up directories that were explicitly marked to be ignored for running systems.

For instance, dbus-broker fails when reloaded during a switch when I run with this patch, because its temporary files get wiped, presumably because we have lines like

R! /tmp/systemd-private-*
R! /var/tmp/systemd-private-*

in our tmpfiles config, which are supposed to be executed only during boot, but with this patch they are also executed during a switch.

I think we need a distinction between the tmpfiles invocation during bootup, which has --boot, and the one that we do while the system is running (like during a switch), which doesn't set --boot.

@nikstur nikstur force-pushed the sysinit-reactivation branch from 7518b07 to 5d15193 Compare December 27, 2023 18:06
@nikstur
Copy link
Contributor Author

nikstur commented Dec 27, 2023

I now added a separate service systemd-tmpfiles-resetup.service to encode restarting tmpfiles without --boot. Hope this fixes it.

dbus-broker fails is there a test you can point me to? I can't think of another way of verifying this.

@r-vdp
Copy link
Contributor

r-vdp commented Dec 27, 2023

I now added a separate service systemd-tmpfiles-resetup.service to encode restarting tmpfiles without --boot. Hope this fixes it.

dbus-broker fails is there a test you can point me to? I can't think of another way of verifying this.

I am running with the updated patch now on my system to test.

I can try to reproduce this in a VM test, I think it should be sufficient to restart the tmpfiles service (the one with --boot) and then to reload dbus-broker.

@r-vdp
Copy link
Contributor

r-vdp commented Dec 28, 2023

@nikstur: this commit introduces a test that fails with the previous implementation and succeeds now, showing the problem with dbus-broker.

I'm sure that there must be other services that would break if their temporary files get wiped during a switch, dbus-broker is just the only one that I encountered the issue with on my own system.

@nikstur
Copy link
Contributor Author

nikstur commented Dec 28, 2023

@nikstur: this commit introduces a test that fails with the previous implementation and succeeds now, showing the problem with dbus-broker.

Nice! Thank you.

So this PR is good to go from your side @r-vdp?

@r-vdp
Copy link
Contributor

r-vdp commented Dec 29, 2023

Yeah I think so, the change to not include --boot fixes the one issue that I encountered and I've been running my main system with this patch for several weeks now.

Maybe good to cherry-pick the nixos test onto this branch to avoid regressions.

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.

A little annoyance here is that reactivation units are being start twice on stc. Once from systemctl restart sysinit-reactivation.target and once from starting units ..... I don't know how much this matter realistically, just a little imperfection IMO. I don't mind ignoring it.

nixos/doc/manual/release-notes/rl-2405.section.md Outdated Show resolved Hide resolved
@nikstur nikstur force-pushed the sysinit-reactivation branch from 5d15193 to 82a5079 Compare January 17, 2024 23:44
@nikstur nikstur force-pushed the sysinit-reactivation branch from 82a5079 to 8f3abd2 Compare January 17, 2024 23:48
@dasJ dasJ dismissed ElvishJerricco’s stale review January 18, 2024 15:13

Concerns are fixed now

@dasJ
Copy link
Member

dasJ commented Jan 18, 2024

brrrrrrr

@dasJ dasJ merged commit 15c31af into NixOS:master Jan 18, 2024
20 checks passed
@Ramblurr
Copy link
Contributor

Ramblurr commented Apr 2, 2024

For those of us that are still on nixos stable (23.11) and therefore do not have systemd-tmpfiles-resetup.service and sysinit-reactivation.target. Is there anyway to ensure that my own systemd service is run before the tmpfiles rules are updated during a normal rebuild switch (i.e., not just at boot time)?

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: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants