nixos/switch-to-configuration: Document and test handling of socket-activated services #161838

dasJ · 2022-02-25T13:36:32Z

Motivation for this change

More tests, more docs

The manual should explain what this is doing exactly.

Things done

ncfavier · 2022-02-25T17:24:27Z

@ofborg test switchTest

dasJ · 2022-02-25T20:59:52Z

Also cc @aanderse to remind people that you want the script rewritten ;)

roberth

Found a flaw 😢 This will break services where ExecStop relies on still having the old etc.

I'm not confident that this can be fixed without fixing systemd or mutating the old units. We really need to inhibit the old service start; otherwise it seems we'll have to introduce new corner cases.

roberth · 2022-02-26T00:28:55Z

nixos/tests/switch-test.nix

+        out = switch_to_specialisation("${machine}", "simple-socket-stop-if-changed-and-execstop")
+        assert_contains(out, "stopping the following units: socket-activated.service, socket-activated.socket\n")


This switches from a non-ExecStop service to an ExecStop one, so it doesn't need to stop the socket yet, but does need to do so on the next switch, even when ExecStop is removed.

roberth · 2022-02-26T00:42:03Z

nixos/doc/manual/development/unit-handling.section.md

+    default), the behavior depends on whether `ExecStop=` is set. If set, the
+    `.socket` unit and the `.service` unit are **stop**ped and the `.socket`
+    unit is **start**ed. This is required to prevent races. If `ExecStop=` is
+    not set, there is no need to touch the `.socket` unit. Instead, the


This means that despite stopIfChanged being enabled, the old service will run until after etc has been switched.
I don't think that's acceptable.

dasJ · 2022-02-27T11:38:18Z

@roberth thank you for reviewing the change.
The whole "ExecStop is taken from the new generation" is fixable but I won't do that until we agreed what to do with the other points you mentioned.
Running the service in the new environment is imo acceptable because it's only very short and it increases general service availability.
I do however not know each and every service in NixOS and I don't know if services could suffer from this behavior.

Do you have an alternative suggestion I can implement? If we don't come to an agreement I can redo this PR to only clean the code, add test cases, and add docs to at least have the current behavior properly specified and tested.

dasJ · 2022-02-27T16:36:03Z

I removed most of the logic after a longer discussion with Robert. We came to the conclusion that there was no way we can keep up with the race-free nature of the current behavior (please correct me if I got your points wrong).
This would be possible if systemd blessed us with some magic way, but as of writing this comment, it's not.

So this PR mostly adds test cases and proper docs for handling of socket-activated units and leaves the current logic intact.

nixos/doc/manual/development/unit-handling.section.md

cheriimoya · 2022-02-27T20:21:59Z

nixos/modules/system/activation/switch-to-configuration.pl

@@ -339,21 +340,23 @@ sub handleModifiedUnit {
                # If this unit is socket-activated, then stop the
                # socket unit(s) as well, and restart the
                # socket(s) instead of the service.
-                my $socketActivated = 0;
+                my $socket_activated = 0;


why is this snake case when all the other variables are camel case?

https://metacpan.org/pod/Perl::Critic::Policy::NamingConventions::Capitalization

makes sense, but what is unitsToReload?

There is currently a mix between camel case (not yet refactored) and snake case (already refactored)

cheriimoya · 2022-02-27T20:29:44Z

nixos/tests/switch-test.nix

+        # Socket-activated services don't get started, just the socket
+        machine.fail("[ -S /run/test.sock ]")
+        out = switch_to_specialisation("${machine}", "simple-socket")
+        # assert_lacks(out, "stopping the following units:") nobody cares


why not throw it out then?

To make it clear that this check was left out explicitly and not just forgotten

cheriimoya · 2022-02-27T20:32:55Z

nixos/tests/switch-test.nix

        assert_lacks(out, "reloading the following units:")
-        assert_contains(out, "restarting the following units: test-timer.timer\n")
+        assert_contains(out, "\nrestarting the following units: test-timer.timer\n")


why aren't we stripping away the spaces from each out variable (or rather in the switch_to_specialisation function)? is it important to check the newline everywhere?

Because restarting the following units and starting the following units must somehow be differentiated and using regex would be pretty expensive to run hundreds of times

ah, i didn't know out was a string, thought it was an array, my bad

dasJ · 2022-03-02T12:11:23Z

If there are no more strong opinions, I'd merge this today or so. It's mostly docs and tests and doesn't touch the logic

nixos/tests/switch-test.nix

roberth

Just the suggestions from my comment just now. Lgtm!

stigtsp

LGTM

…vices

dasJ requested review from Mic92, domenkozar, roberth, ajs124 and primeos February 25, 2022 13:36

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 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 25, 2022

dasJ mentioned this pull request Feb 25, 2022

nixos/nix-daemon: Ensure continued availability of daemon socket #161059

Merged

13 tasks

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 25, 2022

dasJ mentioned this pull request Feb 25, 2022

nixos/tests: fix switchTest #161859

Closed

dasJ requested review from mweinelt, lovesegfault, Lassulus, stigtsp and cheriimoya February 25, 2022 20:59

roberth requested changes Feb 26, 2022

View reviewed changes

dasJ force-pushed the feat/stc-less-socket-restarts branch from 79349fe to df75fef Compare February 27, 2022 16:34

github-actions bot removed the 8.has: changelog label Feb 27, 2022

dasJ changed the title ~~nixos/switch-to-configuration: Improve handling of socket-activated services~~ nixos/switch-to-configuration: Document and test handling of socket-activated services Feb 27, 2022

cheriimoya suggested changes Feb 27, 2022

View reviewed changes

dasJ force-pushed the feat/stc-less-socket-restarts branch from df75fef to d56d805 Compare February 27, 2022 20:45

roberth reviewed Mar 2, 2022

View reviewed changes

nixos/tests/switch-test.nix Outdated Show resolved Hide resolved

roberth approved these changes Mar 2, 2022

View reviewed changes

stigtsp approved these changes Mar 2, 2022

View reviewed changes

dasJ added 2 commits March 3, 2022 20:49

nixos/switch-to-configuration: Document and test socket-activated ser…

1def557

…vices

nixos/switchTest: Make checks more precise

f6ad15f

dasJ force-pushed the feat/stc-less-socket-restarts branch from d56d805 to f6ad15f Compare March 3, 2022 19:56

dasJ merged commit 803f7d4 into NixOS:master Mar 4, 2022

dasJ deleted the feat/stc-less-socket-restarts branch March 4, 2022 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/switch-to-configuration: Document and test handling of socket-activated services #161838

nixos/switch-to-configuration: Document and test handling of socket-activated services #161838

dasJ commented Feb 25, 2022 •

edited

Loading

ncfavier commented Feb 25, 2022

dasJ commented Feb 25, 2022

roberth left a comment

roberth Feb 26, 2022

roberth Feb 26, 2022

dasJ commented Feb 27, 2022 •

edited

Loading

dasJ commented Feb 27, 2022

cheriimoya Feb 27, 2022

dasJ Feb 27, 2022

cheriimoya Feb 27, 2022

dasJ Feb 27, 2022

cheriimoya Feb 27, 2022

dasJ Feb 27, 2022

cheriimoya Feb 27, 2022

dasJ Feb 27, 2022

cheriimoya Feb 27, 2022

dasJ commented Mar 2, 2022

roberth left a comment

stigtsp left a comment

		out = switch_to_specialisation("${machine}", "simple-socket-stop-if-changed-and-execstop")
		assert_contains(out, "stopping the following units: socket-activated.service, socket-activated.socket\n")

nixos/switch-to-configuration: Document and test handling of socket-activated services #161838

nixos/switch-to-configuration: Document and test handling of socket-activated services #161838

Conversation

dasJ commented Feb 25, 2022 • edited Loading

Motivation for this change

Things done

ncfavier commented Feb 25, 2022

dasJ commented Feb 25, 2022

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasJ commented Feb 27, 2022 • edited Loading

dasJ commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasJ commented Mar 2, 2022

roberth left a comment

Choose a reason for hiding this comment

stigtsp left a comment

Choose a reason for hiding this comment

dasJ commented Feb 25, 2022 •

edited

Loading

dasJ commented Feb 27, 2022 •

edited

Loading