GNOME Bugzilla – Bug 736406
NetworkManager.service should use KillMode=mixed
Last modified: 2016-03-18 16:03:51 UTC
Created attachment 285815 [details] [review] patch to fix the issue Long time ago after bugs were reported on systems with NFS root during NetworkManager shutdown, I changed KillMode from default to "process" to avoid terminating the dhclient process. I just realized that "process" leaves some processes running and therefore we should instead run "mixed" which causes SIGTERM to be only sent to the main process and SIGKILL to be sent first to the main process.
It is expected that NM does not tear down the network on stop. Hence you want the spawned dhclient/teamd processes keep running. I guess, the proper (more complicated) solution is to run the clients as separate systemd-services and in their own cgroup.
(In reply to comment #1) > It is expected that NM does not tear down the network on stop. Hence you want > the spawned dhclient 1) We talked about it multiple times and you don't want to leave any processes spawned because of security updates. 2) You don't need a running DHCP client for communication. NetworkManager used the default KillMode=cgroup until I changed it because of a specific race condition where a killed dhclient would result in tearing down the connection. If NetworkManager instead waited for being killed as well when dhclient was killed, we could even switch back to the default KillMode. > teamd processes keep running. I think the same applies to teamd. The communication can continue while NetworkManager is being restarted and a new teamd instance would be run which is important if we got a security update. The user should expect to have all dynamic networking features when the network management tool is stopped. > I guess, the proper (more complicated) solution is to run the clients as > separate systemd-services and in their own cgroup. Interesting idea but this bug report is about the fact that we switched from KillMode=cgroup to KillMode=process instead of KillMode=mixed of which I didn't know at the time.
(In reply to comment #2) > (In reply to comment #1) > > It is expected that NM does not tear down the network on stop. Hence you want > > the spawned dhclient > > 1) We talked about it multiple times and you don't want to leave any processes > spawned because of security updates. > > 2) You don't need a running DHCP client for communication. NetworkManager used > the default KillMode=cgroup until I changed it because of a specific race > condition where a killed dhclient would result in tearing down the connection. > If NetworkManager instead waited for being killed as well when dhclient was > killed, we could even switch back to the default KillMode. If you stop NM, dhclient will be left running. But when NM restarts, it will assume the connection. Thereby it would kill the old dhclient instance and spawn a new instance. I was wrongly thinking, that while dhclient runs without NM, it would be able to extend the leases. IOW, if you kill NM, the orphaned dhclient instance could by itself renew the addresses. However, that is not the cases, because dhclient still calls to /usr/libexec/nm-dhcp-helper -- which fails with error. Note that with this however, security-updates of dhclient do work... you either have to restart NM (which is non-disruptive for the netwrok), or you re-activate the connection (which is non-disruptive for NM). In the current case, I agree that having dhclient running has no advantage and it could be just as well killed > > teamd processes keep running. > > I think the same applies to teamd. The communication can continue while > NetworkManager is being restarted and a new teamd instance would be run which > is important if we got a security update. > > The user should expect to have all dynamic networking features when the network > management tool is stopped. Yes, I agree it should work that way. But there might be some issues and it would require some testing first. For example, there is a bug that NM disconnects the team interface, if teamd dies. I guess, we would need some fixing, to happily restart teamd also during connection-assume. > > I guess, the proper (more complicated) solution is to run the clients as > > separate systemd-services and in their own cgroup. > > Interesting idea but this bug report is about the fact that we switched from > KillMode=cgroup to KillMode=process instead of KillMode=mixed of which I didn't > know at the time. Btw. mixed does not exists for older systemd versions. So, this should be a configure option.
Which systemd versions does Mixed not work for? Maybe by this point we can just use it unconditionally.
The "mixed" KillMode was added in v209, which was released back in Feb 2014. I have a little bit of concern about the warning from Thomas, about potential teamd issues... but I would move to mixed mode, following Dan advice and using it unconditionally.
I think we want to leave teamd running on quit so that we can retrieve its configuration upon restart and use it to assume the right connection (see [1] - "the team connection becomes incorrect after I restart NetworkManager"). So probably KillMode=mixed can be used only after (if) we implement [2] ("NM: use systemd to spawn teamd"). [1] https://bugzilla.redhat.com/show_bug.cgi?id=1294728 [2] https://bugzilla.redhat.com/show_bug.cgi?id=1085813
Ok, I am closing this as WONTFIX. I think there will be services that we don't want to kill when NetworkManager stops. VPN daemons and teamd comes to mind. Eventually, these should be spawned via D-Bus activation or as separate systemd services and not as direct childs of NetworkManager. If we have any processes running that we don't need, NM should explicitly kill them on shutdown. (our shutdown procedure anyway needs major improvement, for example on SIGTERM we just quit the mainloop but don't wait for killed processes to terminate. But we cannot just enter the mainloop again to wait for them, because there are still NMDevice instances around that might react on other events). This is all a long way to go, and changing the KillMode comes at very last.