treewide: depend on `shutdown.target` if `DefaultDependencies=no` in almost every case by philiptaron · Pull Request #271326 · NixOS/nixpkgs

philiptaron · 2023-12-01T00:17:54Z

Description of changes

Made almost all systemd service units throughout the NixOS tree that specify DefaultDependencies=no conflict with with and come before shutdown.target.

@ElvishJerricco:

I mean it makes sense. Before=sysinit.target means it's trying to be part of sysinit, which is almost always the case for services with DefaultDependencies=no. And the ordering and conflict with shutdown.target is just common sense in case a very early shutdown occurs.

shutdown.target

A special target unit that terminates the services on system shutdown.
Services that shall be terminated on system shutdown shall add Conflicts= and Before= dependencies to this unit for their service unit, which is implicitly done when DefaultDependencies=yes is set (the default).)

The few exceptions:

Things done

This looks like it's got a few other idiosyncrasies, but I'll leave it alone for now.

Also, mark this service as `oneshot`, since it is.

Also, mark the service as `oneshot` since it is.

…hutdown.target

nixos/modules/tasks/filesystems/zfs.nix

nikstur · 2023-12-01T00:31:30Z

nixos/modules/tasks/network-interfaces.nix

+      conflicts = [ "shutdown.target" ];
      unitConfig.DefaultDependencies = false;
      serviceConfig.ExecStart = ''${pkgs.nettools}/bin/domainname "${cfg.domain}"'';
+      serviceConfig.Type = "oneshot";


While we're at it, we should probably also add RemainAfterExit

Only if it makes sense. I don't really know anything about this service. Should it be considered present from the completion of this service? Like, is there a reason to consider it running, or is there a stop procedure when the system shuts down?

domainname sets the NIS domain name. It's fundamentally a call to setdomainname(2).

I don't think it should RemainAfterExit=yes since it really is just a one-shot configuration. Could be wrong though.

I think the question is, does it make sense to start it multiple times during a boot cycle? If it doesn't, then it should RemainAfterExit imo. Since in my understanding domainname is idempotent, it should RemainAfterExit.

does it make sense to start it multiple times during a boot cycle? If it doesn't, then it should RemainAfterExit imo.

What? No, that's not what that's for. RemainAfterExit is there so that things like ExecStop can have meaning for oneshot logic (and also so that dependent units can have BindsTo mean something). If we need a way to designate oneshots that shouldn't be started again by switch-to-configuration, we should make another custom X- thing like X-NoStartOnSwitch or something.

philiptaron · 2023-12-06T15:57:39Z

ping @nikstur @ElvishJerricco :-)

nikstur

Sorry for the delay. I thinks this is good to go.

mweinelt · 2023-12-30T16:09:55Z

This broke evaluation of the lxdVirtualMachine image jobs.

https://hydra.nixos.org/job/nixos/trunk-combined/nixos.lxdVirtualMachineImage.aarch64-linux
https://hydra.nixos.org/job/nixos/trunk-combined/nixos.lxdVirtualMachineImage.x86_64-linux

in job ‘nixos.lxdVirtualMachineImage.x86_64-linux’:
error:
       … while evaluating a branch condition

         at /nix/store/0cgy1xrv40pvk2j1d6sjpgczx8qg7ds4-source/nixos/release-combined.nix:17:28:

           16|
           17|   removeMaintainers = set: if builtins.isAttrs set
             |                            ^
           18|     then if (set.type or "") == "derivation"

       … while calling the 'isAttrs' builtin

         at /nix/store/0cgy1xrv40pvk2j1d6sjpgczx8qg7ds4-source/nixos/release-combined.nix:17:31:

           16|
           17|   removeMaintainers = set: if builtins.isAttrs set
             |                               ^
           18|     then if (set.type or "") == "derivation"

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: The option `systemd.services.lxd-agent.unitConfig.Before' has conflicting definition values:
       - In `/nix/store/0cgy1xrv40pvk2j1d6sjpgczx8qg7ds4-source/nixos/modules/system/boot/systemd.nix': "shutdown.target"
       - In `/nix/store/0cgy1xrv40pvk2j1d6sjpgczx8qg7ds4-source/nixos/modules/virtualisation/lxd-agent.nix': [ ]
       Use `lib.mkForce value` or `lib.mkDefault value` to change the priority on any of these definitions.

philiptaron · 2023-12-30T16:14:21Z

@mweinelt would you like me to prepare a patch, or are you on it?

mweinelt · 2023-12-30T16:15:48Z

Feel free to come up with a patch.

philiptaron added 18 commits November 30, 2023 14:56

nixos/swap: ensure correct ordering w.r.t. shutdown.target

28bb978

nixos/apparmor: ensure correct ordering w.r.t. shutdown.target

454f3cb

nixos/auditd: ensure correct ordering w.r.t. shutdown.target

407ef67

This looks like it's got a few other idiosyncrasies, but I'll leave it alone for now.

nixos/duosec: ensure correct ordering w.r.t. shutdown.target

d7ab46e

nixos/suid-sgid-wrappers: ensure correct ordering w.r.t. shutdown.target

a7a5b2e

nixos/firewall-iptables: ensure correct ordering w.r.t. shutdown.target

5ab8a12

nixos/growpart: ensure correct ordering w.r.t. shutdown.target

e4136ed

nixos/initrd-ssh: ensure correct ordering w.r.t. shutdown.target

0db4d5b

nixos/initrd-secrets: ensure correct ordering w.r.t. shutdown.target

1f73c2a

nixos/filesystems: ensure correct ordering w.r.t. shutdown.target

9c505de

nixos/bcachefs: ensure correct ordering w.r.t. shutdown.target

d4f57da

nixos/zfs: ensure correct ordering w.r.t. shutdown.target

e95b3d3

nixos/network-interfaces: ensure correct ordering w.r.t. shutdown.target

5406410

Also, mark this service as `oneshot`, since it is.

nixos/lxd-agent: ensure correct ordering w.r.t. shutdown.target

07004b4

nixos/qemu: ensure correct ordering w.r.t. shutdown.target

0a226a0

nixos/tests/dhparams: ensure correct ordering w.r.t. shutdown.target

d8c9b26

nixos/tests/stunnel: ensure correct ordering w.r.t. shutdown.target

88dc5de

Also, mark the service as `oneshot` since it is.

nixos/tests/systemd-initrd-networkd: ensure correct ordering w.r.t. s…

6f80911

…hutdown.target

philiptaron requested review from a team, RaitoBezarius and dasJ as code owners December 1, 2023 00:17

philiptaron requested review from ElvishJerricco and nikstur and removed request for a team, RaitoBezarius and dasJ December 1, 2023 00:17

philiptaron requested a review from RaitoBezarius December 1, 2023 00:19

philiptaron requested a review from a team December 1, 2023 00:19

nikstur reviewed Dec 1, 2023

View reviewed changes

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 1, 2023

philiptaron changed the title ~~tree-wide: depend on shutdown.target if DefaultDependencies=no in almost every case~~ treewide: depend on shutdown.target if DefaultDependencies=no in almost every case Dec 1, 2023

philiptaron requested a review from nikstur December 1, 2023 16:21

nikstur approved these changes Dec 23, 2023

View reviewed changes

nikstur merged commit c9569af into NixOS:master Dec 27, 2023

philiptaron deleted the shutdown.target branch December 27, 2023 15:07

nikstur mentioned this pull request Dec 29, 2023

nixos: replace activationScripts 2/x #267983

Merged

13 tasks

philiptaron added a commit to philiptaron/nixpkgs that referenced this pull request Dec 30, 2023

nixos/lxd-agent: fix evaluation regression caused by NixOS#271326

da90576

philiptaron mentioned this pull request Dec 30, 2023

nixos/lxd-agent: fix evaluation regression #277752

Merged

13 tasks

marcusramberg pushed a commit to marcusramberg/nixpkgs that referenced this pull request Dec 30, 2023

nixos/lxd-agent: fix evaluation regression caused by NixOS#271326

052c531

philiptaron mentioned this pull request Sep 3, 2024

nixos: support dm-verity #336983

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

treewide: depend on `shutdown.target` if `DefaultDependencies=no` in almost every case#271326

treewide: depend on `shutdown.target` if `DefaultDependencies=no` in almost every case#271326
nikstur merged 18 commits intoNixOS:masterfrom
philiptaron:shutdown.target

philiptaron commented Dec 1, 2023

Uh oh!

Uh oh!

nikstur Dec 1, 2023

Uh oh!

ElvishJerricco Dec 1, 2023 •

edited

Loading

Uh oh!

philiptaron Dec 1, 2023 •

edited

Loading

Uh oh!

nikstur Dec 23, 2023

Uh oh!

ElvishJerricco Dec 27, 2023

Uh oh!

philiptaron commented Dec 6, 2023

Uh oh!

nikstur left a comment

Uh oh!

mweinelt commented Dec 30, 2023

Uh oh!

philiptaron commented Dec 30, 2023

Uh oh!

mweinelt commented Dec 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

philiptaron commented Dec 1, 2023

Description of changes

Related

Things done

Uh oh!

Uh oh!

nikstur Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

ElvishJerricco Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philiptaron Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikstur Dec 23, 2023

Choose a reason for hiding this comment

Uh oh!

ElvishJerricco Dec 27, 2023

Choose a reason for hiding this comment

Uh oh!

philiptaron commented Dec 6, 2023

Uh oh!

nikstur left a comment

Choose a reason for hiding this comment

Uh oh!

mweinelt commented Dec 30, 2023

Uh oh!

philiptaron commented Dec 30, 2023

Uh oh!

mweinelt commented Dec 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ElvishJerricco Dec 1, 2023 •

edited

Loading

philiptaron Dec 1, 2023 •

edited

Loading