prometheus-smartctl-exporter: fix nvme scanning #205165

Frostman · 2022-12-08T18:57:22Z

As discussed a few times, it seems to be the only way to make smartctl work for NVMe devices and so enabling it when smartctl exporter is enabled.

12c26ac

It's independent of #205123 and compatible with both versions

Not the most elegant way to handle it, would be happy to learn a better way

P.S. I've tested it multiple time in a VM and it seems to be working correctly

Description of changes

Things done

Frostman · 2022-12-13T17:23:00Z

@mweinelt hi, do you still think we need it? thanks

robryk · 2023-01-10T13:27:22Z

Do we know what causes disk block devices to be created with group set to disk?

Why isn't this change to udev rules something we'd want to happen always, regardless of whether anyone's using smartctl_exporter? If this change causes undesired effects, we shouldn't make it silently even for those who use smartctl_exporter. If it doesn't, I don't see why it's not useful always.

mweinelt · 2023-01-10T13:59:01Z

The group assignment comes from udev rule maintained by systemd upstream, see https://github.com/systemd/systemd/blob/0cf2dcf15402c60498165dbd3f14536766c05051/rules.d/50-udev-default.rules.in#L74.

We should probably create an issue an ask whether it would be fine to add nvme char devices into the disk group as well.

mweinelt · 2023-01-10T13:59:33Z

@mweinelt hi, do you still think we need it? thanks

Yes, we need this, but I'm very unhappy about the placement we're apparently being forced into.

cc @WilliButz

robryk · 2023-01-10T15:10:11Z

I've created an upstream issue in systemd about the inconsistent permissions: systemd/systemd#26009

robryk · 2023-01-10T15:58:38Z

The group assignment comes from udev rule maintained by systemd upstream, see https://github.com/systemd/systemd/blob/0cf2dcf15402c60498165dbd3f14536766c05051/rules.d/50-udev-default.rules.in#L74.

Actually, it's unclear for me what causes that file to have effect. It's not present in /etc/udev/rules.d on the running system, and the rules that write out rules.d seem to suggest that neither it is in initrd. Does systemd-udevd look for rules at a relative path from its binary or something equally magical?

mweinelt · 2023-07-08T12:19:09Z

Given the upstream reaction we'd probably want to move this into a separate group, that encapsulates raw disk access, e.g. rawio, to make sure we're not handing anything raw io permissions, by allowing the disk group to do it.

nixos-discourse · 2023-07-08T12:27:39Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/smartctl-exporter/19110/13

robryk · 2023-08-27T15:54:00Z

What's your plan on dealing with this? It doesn't seem to me that systemd-udevd's built-in rules are likely to change anytime soon (or maybe ever).

mweinelt · 2023-08-27T16:12:03Z

nixos/modules/services/monitoring/prometheus/exporters.nix

@@ -183,6 +183,9 @@ let
        "ip46tables -A nixos-fw ${conf.firewallFilter} "
        "-m comment --comment ${name}-exporter -j nixos-fw-accept"
      ]);
+      services.udev.extraRules = mkIf ("${name}" == "smartctl") ''
+        SUBSYSTEM=="nvme", KERNEL=="nvme[0-9]*", GROUP="disk"


I would be fine with a dedicated rawio group here. The disk group already offers something else.

As an afterthought, it doesn't feel good to create a generic group rawio when configuring smartctl-exporter. I'll push a commit in a moment with it.

I'm not convinced that disk is different, and I think that rawio would be misleading. TTBOMK the only interesting thing offered by top-level devices (as opposed to namespace devices, which are group disk) is creation and management of namespaces. I don't think that's "raw I/O" by any stretch. (Note that namespace devices do offer quering of smart status, but smartctl --scan will query the top-level ones. Arguably that's a sound choice, because smart status is per-device and not per-namespace)

Should we just have a dedicated group like smartctl-exporter-nvme with granted permission created only if exporter is enabled? In that case, it'll be isolated to the problem we're solving here and it'll not mess with the generic groups.

Well, we need to set (via an udev rule) the group owner on nvme devices to something. You can't set it to two different values (udev rules don't really support POSIX ACLs), so setting it to something smartctl-exporter-specific would conflict with anyone else trying to use this.

Given that, it might be useful to set this group unconditionally, so that people don't encounter very mysterious interactions between smartctl-exporter and rest of their configuration (either by it relying on what smartctl-exporter's setup does, or it being in conflict with that).

My own suggestion would be to, unconditionally, set this group to disk, but I expect many would disagree. I think that arguments against that stemming from different kinds of access have a very weak basis, for reasons I gave above.

I'm just opposed to mixing it with disk, as that stems from a convention that systemd dictates. I'm also opposed to making the group smartctl-exporter specific, as the exporter shouldn't have a special claim on the resouce in question.

So we are just looking for a proper name. We could narrow it down to just this single use case and go with nvme and possibly some descriptive suffix.

I think nvme is an okay name for it and having it in a separate group wouldn't hurt and will not imperceptibly add permissions to existing services/users using the disk group. We should have it outside of the exporter in that case and unconditional, right?

Unfortunately, I wasn't able to find a good place for the nvme group creation yet. Maybe any suggestions? Thank you.

nixosRules in services/hardware/udev.nix possibly.

Well, we need to set (via an udev rule) the group owner on nvme devices to something. You can't set it to two different values (udev rules don't really support POSIX ACLs), so setting it to something smartctl-exporter-specific would conflict with anyone else trying to use this.

#333961 uses a smartctl_exporter specific group and POSIX ACL in the udev rule. The result is we can keep the base/standard root:root ownership of the NVMe device nodes while simultaneously giving smartctl_exporter access. I think this is a pretty clean solution, withouth side effects (AFAICT).

As discussed a few times, it seems to be the only way to make smartctl work for NVMe devices and so enabling it when smartctl exporter is enabled. NixOS@12c26ac

justinas · 2023-12-21T22:59:13Z

Still relevant in 23.11, any progress?

ibizaman · 2024-04-29T06:28:59Z

nixos/modules/services/monitoring/prometheus/exporters.nix

-      users.groups = (mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {
-        "${name}-exporter" = {};
-      });
+      users.groups."${name}-exporter" = (mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {});


Suggested change

users.groups."${name}-exporter" = (mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {});

users.groups."${name}-exporter" = mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {};

ibizaman · 2024-05-02T06:08:48Z

Adding that this worked for me but I did it by using the GROUP="disk" version.

Frostman requested a review from WilliButz as a code owner December 8, 2022 18:57

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 8, 2022

Frostman requested a review from mweinelt December 8, 2022 18:57

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

mweinelt mentioned this pull request Jan 10, 2023

smartctl_exporter ignores nvme devices by default #210041

Closed

Frostman closed this Jul 28, 2023

mweinelt reviewed Aug 27, 2023

View reviewed changes

prometheus-smartctl-exporter: fix nvme scanning

36256ae

As discussed a few times, it seems to be the only way to make smartctl work for NVMe devices and so enabling it when smartctl exporter is enabled. NixOS@12c26ac

Frostman reopened this Aug 27, 2023

Frostman force-pushed the prometheus-smartctl-exporter-fix-nvme branch from f387d2d to ff1ba40 Compare August 27, 2023 17:06

prometheus-smartctl-exporter: use rawio group

f7d9f11

Frostman force-pushed the prometheus-smartctl-exporter-fix-nvme branch from ff1ba40 to f7d9f11 Compare August 27, 2023 17:11

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 5, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 5, 2024

ibizaman reviewed Apr 29, 2024

View reviewed changes

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024

bjornfor mentioned this pull request Aug 11, 2024

nixos/prometheus-smartctl-exporter: fix NVMe scanning #333961

Merged

13 tasks

stale bot removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prometheus-smartctl-exporter: fix nvme scanning #205165

prometheus-smartctl-exporter: fix nvme scanning #205165

Frostman commented Dec 8, 2022 •

edited

Loading

Frostman commented Dec 13, 2022

robryk commented Jan 10, 2023 •

edited

Loading

mweinelt commented Jan 10, 2023

mweinelt commented Jan 10, 2023 •

edited

Loading

robryk commented Jan 10, 2023

robryk commented Jan 10, 2023

mweinelt commented Jul 8, 2023

nixos-discourse commented Jul 8, 2023

robryk commented Aug 27, 2023

mweinelt Aug 27, 2023

Frostman Aug 27, 2023

robryk Aug 27, 2023

Frostman Aug 27, 2023

robryk Aug 27, 2023

mweinelt Aug 27, 2023

Frostman Aug 28, 2023

Frostman Aug 29, 2023

mweinelt Aug 29, 2023

bjornfor Aug 16, 2024

justinas commented Dec 21, 2023

ibizaman Apr 29, 2024

ibizaman commented May 2, 2024

	users.groups."${name}-exporter" = (mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {});
	users.groups."${name}-exporter" = mkIf (conf.group == "${name}-exporter" && !enableDynamicUser) {};

prometheus-smartctl-exporter: fix nvme scanning #205165

Are you sure you want to change the base?

prometheus-smartctl-exporter: fix nvme scanning #205165

Conversation

Frostman commented Dec 8, 2022 • edited Loading

Description of changes

Things done

Frostman commented Dec 13, 2022

robryk commented Jan 10, 2023 • edited Loading

mweinelt commented Jan 10, 2023

mweinelt commented Jan 10, 2023 • edited Loading

robryk commented Jan 10, 2023

robryk commented Jan 10, 2023

mweinelt commented Jul 8, 2023

nixos-discourse commented Jul 8, 2023

robryk commented Aug 27, 2023

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

justinas commented Dec 21, 2023

Choose a reason for hiding this comment

ibizaman commented May 2, 2024

Frostman commented Dec 8, 2022 •

edited

Loading

robryk commented Jan 10, 2023 •

edited

Loading

mweinelt commented Jan 10, 2023 •

edited

Loading