ZFS: Adding ZED configuration options #64306
Conversation
| # matching subclasses are excluded from logging. | ||
| #ZED_SYSLOG_SUBCLASS_INCLUDE="checksum|scrub_*|vdev.*" | ||
| #ZED_SYSLOG_SUBCLASS_EXCLUDE="statechange|config_*|history_event" | ||
| '' |
There was a problem hiding this comment.
I'd like to see this as a more general option as an abstraction, something like
{
options.services.zfs.zed.settings = mkOption {
type = with types; attrsOf (nullOr (either str (either int bool)));
# ...
};
}, leaning towards NixOS/rfcs#42, then people can override these settings and add ones not supported by the module.
Then you can move these definitions into the config section of the module like this:
{
services.zfs.zed.settings = {
ZED_DEBUG_LOG = mkDefault cfgZED.debugLog;
ZED_EMAIL_ADDR = mkDefault (concatStringsSep " " cfgZED.email.addresses);
# ...
};
}And the config file gets generated only from this settings option (where null values mean that it won't get set, which simplifies things and makes sense).
| email.program = mkOption { | ||
| type = types.nullOr types.str; | ||
| default = null; | ||
| example = "mail"; |
There was a problem hiding this comment.
It might make sense to use pkgs.system-sendmail by default here.
Also need to make sure ZED can get access to these executables through PATH (if using a non-path executable). Should maybe be noted in the description or supported somehow.
There was a problem hiding this comment.
How to use pkgs.system-sendmail here?
There was a problem hiding this comment.
Like default = "${pkgs.system-sendmail}/bin/sendmail"
| }; | ||
|
|
||
| email.options = mkOption { | ||
| type = types.str; |
There was a problem hiding this comment.
types.listOf types.str is nicer for a list of commandline arguments.
There was a problem hiding this comment.
But they need to be in a given order according to the mail program being used. So I think types.listOf types.str is less usefull in this case.
There was a problem hiding this comment.
Usually command line tools don't care about the order of options, and listOf str keeps everything in order anyways, it just concatenates all assignments together. Also it can deal with escaping properly, which might be very important here since this will be used by data received from arbitrary mail subjects and such.
| @@ -89,6 +91,202 @@ let | |||
| } | |||
| ''; | |||
|
|
|||
| zedConf = concatStrings [ '' | |||
There was a problem hiding this comment.
This value is never used as of now
|
Why close it? |
|
Why not close it? There's no point in pursuing this anymore. |
|
Well if you don't want to do the suggested changes then sure, but there's no inherent reason this would have to be closed |
There lies the whole problem. You call them suggested meaning there's an option to accept them or not but in reality there is no such option. So there's no point in pursuing this any further. |
|
Sorry but as a reviewer of PR's it's my job to ensure quality code, and that's especially important for modules. I'm willing to discuss the suggestions if you don't think they're justified, but ultimately I am the one who would have to merge this and therefore be responsible for having this code in nixpkgs |
Motivation for this change
The ZFS Events Daemon (ZED) offer notifications upon zpool events like errors on scrubbing, failing disks and more (if desired). Enabling/altering those options in the configuration.nix helps to lessen the administrative tasks of regularly checking the status of the zpools.
Most of the options that are configurable in the '/etc/zfs/zed.d/zed.rc' file can be configured now through the configuration.nix. A few options, like changing the lock folder, were left alone as the use of those doesn't seem to be widespread.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)