network interfaces: restore bridge slaves #26765
Conversation
fpletz
added
the
6.topic: nixos
|
@fpletz @mxxz I wonder if this is the right solution. We try to prevent the bridge from restarting by actually defining a reload. But when the restart can not be prevented, then yes, loosing all slaves that are not part of the bridge definition is part of the deal and imho expected behaviour. I think the correct solution is to define require- and after-dependencies from the services that place additional slaves into bridge onto the bridge service itself. So if the problem here is that containers loose their connectivity when the bridge is restarted, at first we should try to prevent the bridge from restarting and then we should make the container depend on the bridge service and therefor stop the container before the bridge is restarted and start the container again after the bridge is restarted. |
| @@ -263,6 +268,10 @@ let | |||
| ip link del "${n}" || true | |||
| rm -f /run/${n}.interfaces | |||
| ''; | |||
| preStop = '' | |||
| # Save currently attached slaves | |||
| ip link | grep master | awk '{print $2,$9}' | grep "${n}" | cut -d ":" -f 1 > /run/${n}.slaves | |||
There was a problem hiding this comment.
I know there is a webpage called useless-use-of-cat. But this here is a useless use of awk and double grep.
Why not just do ip link |grep "master ${n}" |cut -d' ' f 9 and start two processes less?
| @@ -232,7 +232,7 @@ let | |||
| before = [ "network-setup.service" (subsystemDevice n) ]; | |||
| serviceConfig.Type = "oneshot"; | |||
| serviceConfig.RemainAfterExit = true; | |||
| path = [ pkgs.iproute ]; | |||
| path = with pkgs; [ iproute gawk ]; | |||
There was a problem hiding this comment.
see below, if you can do without awk, this is also not needed.
| @@ -251,6 +251,11 @@ let | |||
| ${i} | |||
| '')}" > /run/${n}.interfaces | |||
| # Enslave previously attached interfaces | |||
| [ -f /run/${n}.slaves ] && for ifname in `cat /run/${n}.slaves`; do | |||
There was a problem hiding this comment.
As I said in a comment on the whole pull request, I don't like the idea here. If a service (container) is depending on the bridge, the dependency should be defined so that systemd can stop and start the dependencies accordingly.
The code above this change "Save list of enslaved interfaces" is saving this services state while it is running.
What you are doing is saving state while the service is not running. Which I believe is really stuff that either should be 'saved' by the whole nix packaging/configuration thing or not need saving at all.
|
I am siding with @kampfschlaefer on this one. |
|
The case where this is relevant and cannot be fixed with dependency definitions, is where interfaces are dynamically added. This happens e.g. when using KVM/libvirt and using the bridge for the VMs, we have this problem a lot and this would fix it. I cannot see a way to define any dependencies in order to fix this or any other solution currently. |
|
@volth Because we use it but it is not a general solution for NixOS. It is a bad approach because you can't remove interfaces from bridges using the config and a rebuilt. They would be added again after the reload. |
|
Your other pull request #27678 has the better solution but I haven't had the chance to test it yet. That's why the other one isn't merged yet. |
Janik-Haag
added
the
12. first-time contribution
I've noticed slaves not coming back up again after restarting the service. With this fix slaves are saved in a file in /run and restored again after start.