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/ups: add options for essential config files #213006

Merged
merged 2 commits into from
Dec 5, 2023
Merged

Conversation

Majiir
Copy link
Contributor

@Majiir Majiir commented Jan 27, 2023

Description

Design

My goal with the new options is to have sensible defaults that minimize the configuration necessary to run a basic standalone setup:

{
  power.ups = {
    enable = true;

    ups."cp600lcd" = {
      driver = "usbhid-ups";
      port = "auto";
    };

    users.upsmon = {
      passwordFile = "/path/to/upsmon.password";
      upsmon = "master";
    };

    upsmon.monitor."cp600lcd".user = "upsmon";
  };
}

This is enough to monitor the UPS and shut down the system when the battery is low.

Testing

I tested these changes with one node in a standalone configuration attached to the UPS and a second node connected in a netclient configuration. The first node uses a custom NOTIFYCMD and the second node uses upssched. I tested basic power loss scenarios and tested that activating config changes reloads services successfully.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@edwtjo
Copy link
Member

edwtjo commented Feb 3, 2023

Is there some reason to keep these commits separated? I think these should be squashed when the PR is finished, you can still keep the commit titles in the commit message body. Have you tried to hook up upsmon even though this PR doesn't do anything automatically? Yes I think this should be in release-notes when completed.

@Majiir
Copy link
Contributor Author

Majiir commented Feb 3, 2023

Is there some reason to keep these commits separated? I think these should be squashed when the PR is finished, you can still keep the commit titles in the commit message body.

I thought it would be easier to review with multiple commits. I'll squash them once the changes are otherwise ready to merge.

Have you tried to hook up upsmon even though this PR doesn't do anything automatically?

Yes, I've tested standalone and netclient modes with upsmon calling upssched.

@Majiir
Copy link
Contributor Author

Majiir commented Sep 2, 2023

Updated the secrets logic to use LoadCredential= and replace-secret to generate the config files on service startup instead of at system activation.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2683

@amaxine
Copy link
Contributor

amaxine commented Nov 24, 2023

I've been running this quite happily for a few weeks, but I'm not sure about handling the severely breaking changes - perhaps this should be gated behind state versions?

- Ensure NUT_STATEPATH exists (fixes service startup)
- Use mode option to enable services (fixes NixOS#113735)
- Remove extraneous slash in paths (fixes confusing logs)
- Support reload for upsmon and upsd
- Remove ExecStart wrapper scripts
@amaxine amaxine merged commit f73dbfa into NixOS:master Dec 5, 2023
19 checks passed
@pjones
Copy link
Contributor

pjones commented Dec 5, 2023

This is great @Majiir, thank you!

@Majiir
Copy link
Contributor Author

Majiir commented Dec 9, 2023

@amaxine Thanks for the merge. Regarding breaking changes: When upsmon is enabled, the default configuration for the new options will fail the MINSUPPLIES assertion, so users will see that something has changed at evaluation time. That assertion is only disabled if mode == "none". It's maybe not the best warning mechanism, but it should protect existing users.

@amaxine
Copy link
Contributor

amaxine commented Dec 9, 2023

@Majiir yup, in the context of that and the overhauled release notes format for 24.05 (plus 23.11 being just out the door), I felt like it's not actually that awful to merge a severely breaking change right now. Thanks a bunch for this work!

@dcarosone
Copy link

Something has been missed here. I'm sure the changes are good, and the bit about tripping the "MINSUPPLIES" assertion is true so I knew I had to go looking for something to migrate, however:

  • I found the change to the release notes. It was not helpful.

    • It suggests that the changes are only incompatible for manually-created versions of those files, which I don't have.
    • It doesn't explain what kind of migration is needed, nor give an example of what a new config looks like, and what minimum entries are needed
  • I found the commit, and from there this PR. There's an example here, but again it seems to assume I have any idea what the format of the files being generated is: I don't, I'm just using nixos options.

Again, I have no problem with breaking changes, but there needs to be better information on what I should do.

I have this as my current config,

  power.ups = {
    enable = true;
    ups.pw5110 = {
      port = "auto";
      driver = "bcmxcp_usb";
    };
  };

what should the new one look like? The example above has some passwords and users, but I don't know what they are and there's no password I'm aware of. If they're now required, can they not be generated automatically (e.g. random) if no specific settings are given?

@Majiir
Copy link
Contributor Author

Majiir commented Dec 10, 2023

@dcarosone Can you help me better understand your setup and what you intend for it to do?

If that's your only config and you didn't manually create any config files, then you would have been missing upsd.conf, upsd.users, and upsmon.conf. Your UPS would be configured for upsdrvctl, but if neither upsd nor upsmon are configured, how are you using your UPS?

Regarding the options: This module exposes options for NUT (Network UPS Tools) services and their config files. You can find information about them in their man pages (nut.conf(5), upsd(8), there are a lot of them). The descriptions of the module options refer to the relevant man pages. While the module aims to include relevant documentation, you may need to read the NUT documentation to fully understand how the various NUT components interact. For example, users must be configured in order to use upsd to query UPS status, and monitors must be configured for upsmon to act on events.

@dcarosone
Copy link

Can you help me better understand your setup and what you intend for it to do?

Sure, it's the simplest case possible: a single minipc workstation on a dedicated small ups. I just want monitoring and the box to shut down when the UPS is nearing discharge. Logging of power states/transitions and other interesting events is a bonus, but seems to be happening.

I would have expected (naively) that credentials and other similar settings were only relevant for multi-host network setups, and that seems to have been the case for me until now.

If that's your only config and you didn't manually create any config files, then you would have been missing upsd.conf, upsd.users, and upsmon.conf.

Well! I took a look..

[root@oenone:/etc/nut]# l
total 40K
drwxr-xr-x  2 root root   8 Dec 10 13:35 .
drwxr-xr-x 47 root root 108 Dec 10 13:35 ..
lrwxrwxrwx  1 root root  24 Dec 10 13:35 nut.conf -> /etc/static/nut/nut.conf
lrwxrwxrwx  1 root root  24 Dec 10 13:35 ups.conf -> /etc/static/nut/ups.conf
-rw-r--r--  1 root root   0 Dec 20  2021 upsd.conf
-rwx------  1 root root  74 Dec 21  2021 upsd.users
-r--r--r--  1 root root 16K Dec 21  2021 upsmon.conf
lrwxrwxrwx  1 root root  29 Dec 10 13:35 upssched.conf -> /etc/static/nut/upssched.conf

So,

  • I have a upsd.conf; it's a 0-byte file.
  • I have a upsmon.conf that looks like a default file, I can't really tell if anything in here has been changed from wherever it came from.
  • I have a upsd.users file, with a user and random-string password in.

I said:

Something has been missed here.

… but it seems what I missed was that I did have existing config outside nix. My apologies, I have no recollection whatsoever of having created this, but seemingly I did.

Regarding the options: This module exposes options for NUT (Network UPS Tools) services and their config files. You can find information about them in their man pages (nut.conf(5), upsd(8), there are a lot of them).

Yeah. Now that it's managed by nix, I can migrate whatever is in these files, and I guess it's worth investing the time to look at all this config. I can put the credential in agenix and configure a remote client.

For example, users must be configured in order to use upsd to query UPS status, and monitors must be configured for upsmon to act on events.

I do think an example like the one at the top of this PR, and a couple of pointers like the paragraph above, would be useful in the documentation of the new options. I'll see what concrete improvements I can suggest when I've figure out the details.

@dcarosone
Copy link

Ok, step 1 (the minimal migration) is done successfully. Other than ups specifics and agenix secrets stuff, the only real difference to your example was

      upsmon = "primary";

based on the manpage, though my existing config had "master" there, too. I assume either is valid for compat, but chose to use the better modern term encouraged by the documentation.

In terms of generated output differences, in upsmon.conf:

  • some timeout related options are missing (and were surely defaults and irrelevant).
  • There's an added NOTIFYCMD, which I can only assume will be mostly harmless
  • NOTIFYCMD and SHUTDOWNCMD now have nix store paths; cool
  • and there was previosly a POWERDOWNFLAG /etc/killpower that's now gone. Looking at the docs, it was probably not doing anything, I assume NixOS's shutdown doesn't look at this flag by default.

@PedroHLC
Copy link
Member

Thanks for leaving the example in the PR's body, it was essential to understand what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants