[RDY] networkmanager: enrich dispatcher PATH #39142
Conversation
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
teto
changed the title
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
This is not necessarely related to my PR but might make me add a fix; it seems like on nixos-unstable, dispatched scripts are not run ? while the script is executable and owned by root In networkmanager module, scripts are dispatched via |
|
According to the manpage: |
teto
force-pushed
the
nm_dispatchers
branch
2 times, most recently
from
edbd86d
to
0635687
Compare
|
I updated the patch to also enrich hooks PATH with coreutils ! |
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
This seems to me like a pretty safe merge. |
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
teto
changed the title
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
| ''; | ||
| }; | ||
|
|
||
| type = mkOption { | ||
| type = types.enum (attrNames dispatcherTypesSubdirMap); | ||
| default = "basic"; | ||
| description = '' | ||
| Dispatcher hook type. Only basic hooks are currently available. | ||
| Dispatcher hook type. Look up the hooks described at |
There was a problem hiding this comment.
I am confused. type is defined as enum, which supports only 3 types (removed comment suggests that 2 of them don't work):
dispatcherTypesSubdirMap = {
"basic" = "";
"pre-up" = "pre-up.d/";
"pre-down" = "pre-down.d/";
};
But suggested link lists 12 of them!
There was a problem hiding this comment.
I got confused at first too, my first tackle at the PR added:
dispatcherTypesSubdirMap = {
"basic" = "";
"pre-up" = "pre-up.d/";
"up" = "";
"pre-down" = "pre-down.d/";
"down" = "";
"vpn-pre-up" = "pre-up.d/";
"vpn-up" = "";
"vpn-pre-down" = "pre-down.d/";
"vpn-down" = "";
"hostname" = "";
"dhcp4-change" = "";
"dhcp6-change" = "";
"connectivity-change" = "";
};```
but then as I got to deploy my script realized that the addition might be misleading since the scripts end up in the same folder and they must check themselves their second parameter value ( e.g., if `[ "$2"= "dhcp4-change"] then .... fi` to act only for dchp4 events) so I reverted the change
There was a problem hiding this comment.
Ok, then maybe hide this option (make it internal, not deprecated)? I don't see any value in it except link in description.
There was a problem hiding this comment.
It has value because some events call hooks only in the specific folders "pre-down.d/" / "pre-up.d/";
| @@ -229,15 +229,19 @@ in { | |||
| source = mkOption { | |||
| type = types.path; | |||
| description = '' | |||
| A script. | |||
| Path to the hook script. Beware of the networkmanager dispatcher | |||
| service PATH (coreutils/iproute are available). | |||
There was a problem hiding this comment.
First sentence is fine. Second is redundant I think (it would be rather unexpected to NOT have coreutils available). Probably can be merged with description of type if it turns out that type option isn't actually supported.
There was a problem hiding this comment.
coreutils wasn't available until this PR so I was not sure it's sthg to be assumed for nixos.
There was a problem hiding this comment.
I agree, this is more of my personal preference. I'll try to explain why I think the previous situation was an actual bug.
When NixOS presents a types.lines option, it is still confusing not to have basic commands in default environment, but one can fix this with inline ${package}/bin/executable.
When NixOS presents a types.path option, it looks like that file/script can be used on it's own, and most probably it doesn't contain references to /nix/store/... executables, but rather relies on $PATH.
NixOS newcomers even think that user's or root's $PATH inherits into service's PATH, but then spend half a day to fix hostname: command not found (happend to me :( for some other NixOS options)
Also, second sentence refers to "networkmanager dispatcher service PATH" which is more of implementation detail. When you read NixOS manual, you won't find anywhere that "networkmanager dispatcher service PATH", only in NixOS source code. It is correct to refer to it only If it is abstracted to an option in networkmanager.* namespace.
There was a problem hiding this comment.
I agree it's more of an implementation detail. I wish nixos systemd side of things were documented in nixpkgs too.
|
Also, while you are here, can you add some simple hook script as literal |
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
I added a literalExample |
|
|
||
| # coreutils and iproute are in PATH too | ||
| logger "Device $DEVICE_IFACE coming up" | ||
| ''; |
There was a problem hiding this comment.
The closing ''' for pkgs.writeText is missing
See examples
I do run I have the script installed in |
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Is that what you expected for the example ? The message won't appear under the network-manager service but with the user name by default (you can change it with logger -t IIRC). Maybe as a first trial grep through |
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
| @@ -224,21 +224,22 @@ in { | |||
| }; | |||
|
|
|||
| dispatcherScripts = mkOption { | |||
| example = [ { | |||
| source = pkgs.writeText "upHook" '' | |||
|
|
|||
There was a problem hiding this comment.
Almost good now! Except that pkgs.writeText will get executed/evaluated on manual build...
You can see yourself what happens: nix-build nixos/release.nix -A manual.x86_64-linux && firefox .../share/doc/nixos/index.html
networking.networkmanager.dispatcherScripts
A list of scripts which will be executed in response to network events.
Type: list of submodules
Default: [ ]
Example: [ { source = (build of upHook); type = "basic"; } ]
Declared by:
<nixpkgs/nixos/modules/services/networking/networkmanager.nix>
This is why for any nontrivial types you have to set plaintext literalExample instead of example.
|
@teto Your're right! I've grepped through |
|
cool ! I moved the example after default as it is usually done. I generated the doc and here at least the alignement of braces is good. |
| ''; | ||
| }; | ||
|
|
||
| type = mkOption { | ||
| type = types.enum (attrNames dispatcherTypesSubdirMap); | ||
| default = "basic"; | ||
| description = '' | ||
| Dispatcher hook type. Only basic hooks are currently available. | ||
| Dispatcher hook type. Look up the hooks described at | ||
| https://developer.gnome.org/NetworkManager/stable/NetworkManager.html |
There was a problem hiding this comment.
One more, please. Change this to <xlink>, otherwise it isn't rendered as link
<link xlink:href="https://developer.gnome.org/NetworkManager/stable/NetworkManager.html">https://developer.gnome.org/NetworkManager/stable/NetworkManager.html</link>
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
@matthewbauer as you already helped me with the iproute change, I wonder if you could review and eventually merge it ? I've been using it with success for some time. |
teto
force-pushed
the
nm_dispatchers
branch
2 times, most recently
from
44601fd
to
d7a4e7d
Compare
|
Success on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
rebased and squashed. Hope this can be merged. |
|
cc @peterhoeg @Mic92 for review |
First change is to override the nm-dispatcher systemd service so that it puts coreutils (wc/env/...) and iproute in PATH. Second change is to make sure userscripts have the execute bit.
|
rebased. Hope this can make it to the next release. |
|
Success on aarch64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: networkmanager Partial log (click to expand)
|
|
@samueldr low priority PR which I only dare to bother you about because of https://discourse.nixos.org/t/nixos-18-09-jellyfish-to-be-forked-off-in-a-month/596/10 :D (feel free to ignore if you are time constrained). This PR allows to write network manager hooks which is very practical when dealing with advanced network configurations. |
Motivation for this change
I needed to use some unavailable hooks.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)