Fix mdadm monitor service #123466

ctheune · 2021-05-18T08:34:29Z

Motivation for this change

In #72394 we see a situation where enabling the built-in systemd units for mdadm caused issues for users. I have reviewed the discussion in the thread and am providing a fix that

a) works out of the box without failing units
b) allows to explicitly enable the mdadm monitor service, and
c) takes care to remind the admin to provide an email address.

This can likely be improved further inthe future but should be sufficient to mitigate #72394 and work as a reliable starting point.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)
- NixOS
- [] macOS
- [] other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
[] Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

azazel75 · 2021-05-18T09:43:31Z

Nice fixup, seems reasonable to me. Thanks @ctheune

aanderse

Thanks for fixing this.

I feel like this PR is throwing the baby out with the bathwater on the package changes. I think the upstream systemd unit provides us what we need via MDADM_MONITOR_ARGS, we just need to take advantage of it.

That being said I have not actually tested my suggestions, so I suppose if you entirely ignore them you don't need to feel guilty...

aanderse · 2021-05-18T11:50:47Z

nixos/modules/tasks/swraid.nix

+
+      services.udev.packages = [ pkgs.mdadm ];
+
+      systemd.packages = [ pkgs.mdadm ];


This line does nothing after your change to the package.

Interesting. I thought I just undid the change that introduced the systemd units and didn't check back on whether that change was related to changes in swraid.nix at that time. I'll double-check this.

aanderse · 2021-05-18T11:52:13Z

pkgs/os-specific/linux/mdadm/default.nix

@@ -12,15 +12,12 @@ stdenv.mkDerivation rec {

  makeFlags = [
    "NIXOS=1" "INSTALL=install" "BINDIR=$(out)/sbin"
-    "SYSTEMD_DIR=$(out)/lib/systemd/system"


This removes all of the following:

├── lib │ ├── systemd │ │ ├── system │ │ │ ├── mdadm-grow-continue@.service │ │ │ ├── mdadm-last-resort@.service │ │ │ ├── mdadm-last-resort@.timer │ │ │ ├── mdmonitor.service │ │ │ └── mdmon@.service │ │ └── system-shutdown │ │ └── mdadm.shutdown

Yeah. Unfortunately the original change was kind of opaque and didn't have any tests either - so I'm not really sure whether those are doing anything helpful anyway ... (see below)

@fmap Would you be available to comment on this topic, as you added this line? My guess would be it might be useful for running Nixpkgs' mdadm on a non-NixOS machine, but not using one myself I couldn't say for sure.

I really think we should leave this in and then override the upstream units where necessary. I have already pointed out in another comment how this can be achieved... Let me know if any help is needed.

aanderse · 2021-05-18T11:57:09Z

nixos/modules/tasks/swraid.nix

+          systemd.services.mdadm-monitor = {
+            description = "Monitor RAID disks";
+            wantedBy = [ "multi-user.target" ];
+            script = "${pkgs.mdadm}/bin/mdadm --monitor -m ${assert cfg.adminAddr != null; cfg.adminAddr} --scan";
+          };


Wouldn't it have worked equally well to not remove the systemd units from the package, and instead use this:

systemd.services.mdadm-monitor = { wantedBy = [ "multi-user.target" ]; environment.MDADM_MONITOR_ARGS = "-m ${assert cfg.adminAddr != null; cfg.adminAddr} --scan"; };

Additionally we could create an option like systemd.services.monitor.extraArgs and rewrite the above environment directive as such:

environment.MDADM_MONITOR_ARGS = "-m ${assert cfg.adminAddr != null; cfg.adminAddr} --scan " + (lib.concatMapStringsSep " " cfg.monitor.extraArgs);

That is a possibility, yes. I was under the (probably wrong) impression that the original change that introduced the units didn't really do anything helpful. I'd be happy to do it this way as well. I'm planning to consider adding more tests for the things that we expect to happen so we don't have to scratch our heads in the future. This may likely need a few days until I get around to adding more effort here, but I'll get to it. :)

It'd be awesome if you could add tests indeed! That said if you don't have the time, I think this PR already makes things better so could probably land :)

Ekleog · 2021-06-18T21:48:41Z

nixos/modules/tasks/swraid.nix

+          systemd.services.mdadm-monitor = {
+            description = "Monitor RAID disks";
+            wantedBy = [ "multi-user.target" ];
+            script = "${pkgs.mdadm}/bin/mdadm --monitor -m ${assert cfg.adminAddr != null; cfg.adminAddr} --scan";


This should use a NixOS assertion rather than a raw assert, as it makes the user experience better. You can find more information about NixOS assertions here: https://nixos.org/manual/nixos/stable/index.html#sec-assertions

Ekleog · 2021-06-18T21:48:46Z

pkgs/os-specific/linux/mdadm/default.nix

@@ -12,15 +12,12 @@ stdenv.mkDerivation rec {

  makeFlags = [
    "NIXOS=1" "INSTALL=install" "BINDIR=$(out)/sbin"
-    "SYSTEMD_DIR=$(out)/lib/systemd/system"


@fmap Would you be available to comment on this topic, as you added this line? My guess would be it might be useful for running Nixpkgs' mdadm on a non-NixOS machine, but not using one myself I couldn't say for sure.

Ekleog · 2021-06-18T21:51:26Z

nixos/modules/tasks/swraid.nix

+          systemd.services.mdadm-monitor = {
+            description = "Monitor RAID disks";
+            wantedBy = [ "multi-user.target" ];
+            script = "${pkgs.mdadm}/bin/mdadm --monitor -m ${assert cfg.adminAddr != null; cfg.adminAddr} --scan";
+          };


It'd be awesome if you could add tests indeed! That said if you don't have the time, I think this PR already makes things better so could probably land :)

devhell · 2021-06-30T10:11:16Z

This looks awesome, can't wait for it to be merged. Thank you for the hard work @ctheune!

stale · 2022-01-06T02:24:22Z

I marked this as stale due to inactivity. → More info

ctheune · 2022-09-18T17:40:22Z

I just stumbled over this again on my personal server and wondered whether we should reactivate this as it was so close to being merged anyway? I'd be happy update the PR.

azazel75 · 2022-09-19T08:46:55Z

Il 18 settembre 2022 19:40:33 CEST, Christian Theune ***@***.***> ha scritto:

I just stumbled over this again on my personal server and wondered whether we should reactivate this as it was so close to being merged anyway? I'd be happy update the PR.

I would be nice to have this closed! -- Inviato dal mio dispositivo Android con K-9 Mail. Perdonate la brevità.

ctheune · 2023-09-10T13:06:17Z

@GrahamcOfBorg test systemd-initrd-swraid

ctheune · 2023-09-10T13:22:30Z

@GrahamcOfBorg test systemd-initrd-swraid

trofi · 2023-09-10T13:49:20Z

I don't knowingly use mdadm on my system (default desktop). Eval started failing on master as:

$ nix build --no-link -f nixos system --keep-going
trace: warning: mdadm: Neither MAILADDR nor PROGRAM has been set. This will cause the `mdmon` service to crash.
error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'nixos-system-nz-23.11pre-git'
         whose name attribute is located at /mnt/archive/big/git/nixpkgs-master/pkgs/stdenv/generic/make-derivation.nix:300:7

       … while evaluating attribute 'buildCommand' of derivation 'nixos-system-nz-23.11pre-git'

         at /mnt/archive/big/git/nixpkgs-master/nixos/modules/system/activation/top-level.nix:53:5:

           52|     passAsFile = [ "extraDependencies" ];
           53|     buildCommand = systemBuilder;
             |     ^
           54|

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

       error: The option `boot.initrd.extraFiles."/etc/mdadm.conf".enable' does not exist. Definition values:
       - In `/mnt/archive/big/git/nixpkgs-master/nixos/modules/tasks/swraid.nix': true

ajs124 · 2023-09-10T14:50:10Z

nixos/modules/tasks/swraid.nix

@@ -59,12 +67,10 @@ in {
        $out/bin/mdadm --version
      '';

-      extraFiles."/etc/mdadm.conf".source = pkgs.writeText "mdadm.conf" config.boot.swraid.mdadmConf;
+      extraFiles."/etc/mdadm.conf" = mdadm_conf;


maybe because of this. extraFiles."/etc/mdadm.conf".source = mdadm_conf.source; could fix it.

It does. However, I can't figure out why and I would like to adjust the test so that it doesn't happen again, because we explicitly thought this would work and the tests and my live system agreed with me.

Even when I explicitly set environment.etc."mdadm.conf".enable = true the test doesn't break and I also do not even see why it should, as nothing iterates over the superfluous attributes.

ajs124 · 2023-09-10T15:20:31Z

see #254429 @trofi

trofi · 2023-09-10T16:18:34Z

see #254429 @trofi

This fixes the eval for me. Thank you!

The only annoyance is that I'm still getting the following warning at eval time: "trace: warning: mdadm: Neither MAILADDR nor PROGRAM has been set. This will cause the mdmon service to crash."

ajs124 · 2023-09-10T18:19:52Z

It's interesting that you're getting this, with your normal system config.
That means something sets boot.swraid.enable = true;, in your config. e.g. if I'm evaluating nixosTests.simple it doesn't print the warning.

There's only one file in nixpkgs that I can find that would do that, which is modules/profiles/installation-device.nix.

trofi · 2023-09-10T18:52:13Z

Ah, that's probably because I'm using system with an old state version: system.stateVersion = "21.11"; which defaults to swraid.enable in a roundabout way:

# nixos/modules/tasks/swraid.nix
  options.boot.swraid = {
    enable = lib.mkEnableOption (lib.mdDoc "swraid support using mdadm") // {
      default = lib.versionOlder config.system.stateVersion "23.11";
# ...

ajs124 · 2023-09-10T20:56:31Z

Good point… maybe printing this warning (by default) on every system with a stateVersion < 23.11 isn't that great 😬

johnrichardrinehart · 2023-09-11T01:30:49Z

Ah, that's probably because I'm using system with an old state version: system.stateVersion = "21.11"; which defaults to swraid.enable in a roundabout way:
# nixos/modules/tasks/swraid.nix
  options.boot.swraid = {
    enable = lib.mkEnableOption (lib.mdDoc "swraid support using mdadm") // {
      default = lib.versionOlder config.system.stateVersion "23.11";
# ...

Wow, thanks. I ran into this today, too.

warning: Git tree '/home/john/code/repos/mine/JohnOS' is dirty
building the system configuration...
warning: Git tree '/home/john/code/repos/mine/JohnOS' is dirty
trace: warning: mdadm: Neither MAILADDR nor PROGRAM has been set. This will cause the `mdmon` service to crash.
error:
       … while calling the 'derivationStrict' builtin

         at //builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'nixos-system-framie-23.11.20230911.ca40349'
         whose name attribute is located at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/pkgs/stdenv/generic/make-derivation.nix:300:7

       … while evaluating attribute 'buildCommand' of derivation 'nixos-system-framie-23.11.20230911.ca40349'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/nixos/modules/system/activation/top-level.nix:53:5:

           52|     passAsFile = [ "extraDependencies" ];
           53|     buildCommand = systemBuilder;
             |     ^
           54|

       … while calling 'g'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/attrsets.nix:599:19:

          598|           g =
          599|             name: value:
             |                   ^
          600|             if isAttrs value && cond value

       … from call site

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/attrsets.nix:602:20:

          601|               then recurse (path ++ [name]) value
          602|               else f (path ++ [name]) value;
             |                    ^
          603|         in mapAttrs g;

       … while calling anonymous lambda

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:242:72:

          241|           # For definitions that have an associated option
          242|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
             |                                                                        ^
          243|

       … while evaluating the option `system.systemBuilderCommands':

       … while calling anonymous lambda

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:815:28:

          814|         # Process mkMerge and mkIf properties.
          815|         defs' = concatMap (m:
             |                            ^
          816|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

       … while evaluating definitions from `/nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/nixos/modules/system/boot/kernel.nix':

       … from call site

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:816:137:

          815|         defs' = concatMap (m:
          816|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
             |                                                                                                                                         ^
          817|         ) defs;

       … while calling 'dischargeProperties'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:887:25:

          886|   */
          887|   dischargeProperties = def:
             |                         ^
          888|     if def._type or "" == "merge" then

       … from call site

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:893:11:

          892|         if def.condition then
          893|           dischargeProperties def.content
             |           ^
          894|         else

       … while calling 'dischargeProperties'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:887:25:

          886|   */
          887|   dischargeProperties = def:
             |                         ^
          888|     if def._type or "" == "merge" then

       … while evaluating derivation 'initrd-linux-6.5.2'
         whose name attribute is located at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/pkgs/stdenv/generic/make-derivation.nix:300:7

       … while evaluating attribute 'exportReferencesGraph' of derivation 'initrd-linux-6.5.2'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/pkgs/build-support/kernel/make-initrd.nix:107:3:

          106|   # See #36268.
          107|   exportReferencesGraph =
             |   ^
          108|     lib.zipListsWith

       … while calling anonymous lambda

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/types.nix:541:22:

          540|       merge = loc: defs:
          541|         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
             |                      ^
          542|             (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue

       … from call site

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/modules.nix:837:59:

          836|       if isDefined then
          837|         if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
             |                                                           ^
          838|         else let allInvalid = filter (def: ! type.check def.value) defsFinal;

       … while calling 'merge'

         at /nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/lib/types.nix:762:22:

          761|         check = x: isAttrs x || isFunction x || path.check x;
          762|         merge = loc: defs:
             |                      ^
          763|           (base.extendModules {

       error: The option `boot.initrd.extraFiles."/etc/mdadm.conf".enable' does not exist. Definition values:
       - In `/nix/store/j6d62blp6npckwcj6cjxf0hyga4w94c1-source/nixos/modules/tasks/swraid.nix': true

Manually editing system.stateVersion to be 23.11 (so, ignoring the scary "Did you read the comment?" message) removed the warning and fixed my build.

vcunat · 2023-09-11T05:19:48Z

nixpkgs-unstable channel seems to be blocked on this, BTW.
https://hydra.nixos.org/build/234796902/nixlog/1/tail

EDIT: oh, also nixos-unstable's jobset won't even evaluate.

ctheune · 2023-09-11T06:08:33Z

@vcunat yeah, I've already prepared #254429 that fixes this and it's waiting to be merged.

ncfavier · 2023-10-02T10:59:27Z

The default ISO image configuration now raises a warning.

SuperSandro2000 · 2023-11-12T02:39:50Z

Maybe we should add an option to disable the mon service sending mail for the iso.

ctheune · 2023-11-15T10:43:37Z

Maybe we should add an option to disable the mon service sending mail for the iso.

The ISO can simply set a non-sensical e-mail-adress like "nobody@example.com".

Disabling individual services is really a fight against windmills - we'd have to talk to upstream about that as they enable/disable services automatically depending on various udev events...

ctheune changed the title ~~Work on fix for #72394~~ Fix mdadm monitor service May 18, 2021

ctheune marked this pull request as draft May 18, 2021 08:34

github-actions bot added 6.topic: nixos 8.has: module (update) labels May 18, 2021

ofborg bot requested a review from Ekleog May 18, 2021 08:43

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500 labels May 18, 2021

aanderse reviewed May 18, 2021

View reviewed changes

Ekleog reviewed Jun 18, 2021

View reviewed changes

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 6, 2022

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 18, 2022

ctheune force-pushed the 72394-cleanup-mdmonitor-service branch from fe49099 to a534b4c Compare September 9, 2023 19:38

ofborg bot requested a review from Ekleog September 9, 2023 20:27

ctheune force-pushed the 72394-cleanup-mdmonitor-service branch 2 times, most recently from b7e8c2a to 67a110f Compare September 10, 2023 13:01

ctheune marked this pull request as ready for review September 10, 2023 13:02

nixos/swraid: fix monitor service

380d73c

ctheune force-pushed the 72394-cleanup-mdmonitor-service branch from 67a110f to 380d73c Compare September 10, 2023 13:20

ajs124 approved these changes Sep 10, 2023

View reviewed changes

mweinelt approved these changes Sep 10, 2023

View reviewed changes

mweinelt merged commit 7f341bb into NixOS:master Sep 10, 2023
8 of 10 checks passed

ctheune mentioned this pull request Sep 10, 2023

mdmonitor service in 19.09 is misconfigured and fails to start #72394

Closed

ajs124 reviewed Sep 10, 2023

View reviewed changes

ctheune mentioned this pull request Sep 11, 2023

swraid/mdcheck: add mdcheck timer for swraid device scrub #204713

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mdadm monitor service #123466

Fix mdadm monitor service #123466

ctheune commented May 18, 2021 •

edited

azazel75 commented May 18, 2021

aanderse left a comment

aanderse May 18, 2021

ctheune May 18, 2021

aanderse May 18, 2021

ctheune May 18, 2021

Ekleog Jun 18, 2021

aanderse Jun 23, 2021

aanderse May 18, 2021

ctheune May 18, 2021

Ekleog Jun 18, 2021

Ekleog Jun 18, 2021

Ekleog Jun 18, 2021

Ekleog Jun 18, 2021

devhell commented Jun 30, 2021

stale bot commented Jan 6, 2022

ctheune commented Sep 18, 2022

azazel75 commented Sep 19, 2022 via email

ctheune commented Sep 10, 2023

ctheune commented Sep 10, 2023

trofi commented Sep 10, 2023

ajs124 Sep 10, 2023

ctheune Sep 10, 2023

ajs124 commented Sep 10, 2023

trofi commented Sep 10, 2023

ajs124 commented Sep 10, 2023

trofi commented Sep 10, 2023

ajs124 commented Sep 10, 2023

johnrichardrinehart commented Sep 11, 2023 •

edited

vcunat commented Sep 11, 2023 •

edited

ctheune commented Sep 11, 2023 •

edited

ncfavier commented Oct 2, 2023

SuperSandro2000 commented Nov 12, 2023

ctheune commented Nov 15, 2023


		services.udev.packages = [ pkgs.mdadm ];

		systemd.packages = [ pkgs.mdadm ];

Fix mdadm monitor service #123466

Fix mdadm monitor service #123466

Conversation

ctheune commented May 18, 2021 • edited

Motivation for this change

Things done

azazel75 commented May 18, 2021

aanderse left a 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

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

devhell commented Jun 30, 2021

stale bot commented Jan 6, 2022

ctheune commented Sep 18, 2022

azazel75 commented Sep 19, 2022 via email

ctheune commented Sep 10, 2023

ctheune commented Sep 10, 2023

trofi commented Sep 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajs124 commented Sep 10, 2023

trofi commented Sep 10, 2023

ajs124 commented Sep 10, 2023

trofi commented Sep 10, 2023

ajs124 commented Sep 10, 2023

johnrichardrinehart commented Sep 11, 2023 • edited

vcunat commented Sep 11, 2023 • edited

ctheune commented Sep 11, 2023 • edited

ncfavier commented Oct 2, 2023

SuperSandro2000 commented Nov 12, 2023

ctheune commented Nov 15, 2023

ctheune commented May 18, 2021 •

edited

johnrichardrinehart commented Sep 11, 2023 •

edited

vcunat commented Sep 11, 2023 •

edited

ctheune commented Sep 11, 2023 •

edited