GNOME Bugzilla – Bug 766996
cannot reload dnsmasq config with "dns=dnsmasq"
Last modified: 2016-06-01 17:11:54 UTC
We have a complex DNS setup (over VPN) that standard NetworkManager settings can't handle, so instead we want to fiddle with the settings via dnsmasq itself. However, when we have dynamically created the /etc/NetworkManager/dnsmasq.d/FILENAME we want, we need to restart dnsmasq to make it read it (dnsmasq doesn't support a reload option for it's config) - but that is controlled by NetworkManager Restarting NetworkManager completely works just fine - but that's a catastrophic fix - total network outage just to make a change to the local DNS settings - and runs the risk of an infinite loop as this is all about changing DNS settings based on a VPN tunnel - which we just tore down due to restarting NetworkManager. Under Ubuntu 16.04/Fedora-24, killing dnsmasq triggers NetworkManager to notice that event in the syslogs, but NetworkManager doesn't recover/auto-restart. Under Fedora-22, *I think* killing dnsmasq actually auto-restarted it (which was great) - I'm not sure - maybe I jerry-rigged a more workable solution back then Is this something that should be doable, and if so, how? Thanks!
NetworkManager cannot handle every complex DNS setup. You may consider disabling managing DNS in NetworkManager and do your own thing. Before 1.2, NetworkManager was unable to reconfigure dnsmasq at runtime. Every time a DNS configuration changed, it would kill the current dnsmasq process, write a new configuration file, and restart dnsmasq. So, that way, you got your desired behavior. With 1.2.2, NetworkManager uses D-Bus to reconfigure DNS on the same instance. Thus, the configuration does not get reloaded. Indeed, killing dnsmasq should cause NM to respawn it (and thus, it would pickup the changed configuration).
(In reply to jhaar from comment #0) > We have a complex DNS setup (over VPN) that standard NetworkManager settings > can't handle, so instead we want to fiddle with the settings via dnsmasq > itself. Which kind of DNS setup are you using? If it contains only static servers (possibly with split domains), note that NM 1.2 allows users to specify a static global DNS configuration in NetworkManager.conf. The advantage of this over a custom dnsmasq configuration file is that the global configuration is reloaded and re-applied on SIGHUP, so this would solve your problem.
Yeah, there was a bug that dnsmasq would not be respawned when killed. Fix here: th/dns-reload-bgo766996. I took the opportunity, to add a Reload D-Bus call.
>> manager: add Reload() D-Bus command hm, something is still wrong. I always have full permission via PolicyKit...
(In reply to Thomas Haller from comment #3) > Fix here: th/dns-reload-bgo766996. > > I took the opportunity, to add a Reload D-Bus call. > dns: reload DNS plugin in SIGHUP Maybe restarting the plugin can be avoided in most cases when users only want to reload configuration with SIGHUP (because it interrupts the resolution service). Instead SIGUSR1, which is explicitly for reconfiguring DNS, could also restart plugins. But either way is fine for me. Can you update the NetworkManager man page to describe the new effect of SIGHUP? > config,dns: support Reload flags to specify that only parts should be reloaded Support 3 new flags for Reload: - 0x01 (CONF): reload the configuration from disk - 0x02 (DNS_RC): write DNS configuration to resolv.conf - 0x04 (DNS_FULL): restart DNS plugin Probably DNS_RC is not the best name, as it doesn't necessarily update resolv.conf (for example if dns=dnsmasq it updates dnsmasq). But TBH I don't have any other suggestion :) /* The flags for Reload. Currently these are internal defines, + * only their numeric value matters and must be stable as + * they are public API! Also, the enum must fit in uint32. */ +enum { /*< skip >*/ + NM_MANAGER_RELOAD_FLAGS_NONE = 0, Then can this go in nm-dbus-interface.h ? Also, test-config fails here with: /config/no-auto-default: (src/tests/config/test-config:21302): GLib-CRITICAL **: Did not see expected message NetworkManager-INFO: *config: update * (NO_AUTO_DEFAULT,no-auto-default)* (but seems to pass on travis, I haven't investigated) The rest LGTM (pushed a trivial fixup).
(In reply to Beniamino Galvani from comment #5) > (In reply to Thomas Haller from comment #3) > > Fix here: th/dns-reload-bgo766996. > > > > I took the opportunity, to add a Reload D-Bus call. > > > dns: reload DNS plugin in SIGHUP > > Maybe restarting the plugin can be avoided in most cases when users > only want to reload configuration with SIGHUP (because it interrupts > the resolution service). Instead SIGUSR1, which is explicitly for > reconfiguring DNS, could also restart plugins. But either way is fine > for me. Maybe yes. Currently SIGUSR1 is only used for DNS. But I see it like this: SIGHUP should reload *everything* that is possible, like D-Bus Reload(0). SIGUSR1 should be a subset of that, without reloading NetworkManager.conf. It also should be more graceful, thus restarting DNS plugin is too destructive for it. We only have 3 signals (HUP,USR1,USR2). I would not say, SIGUSR1 == DNS_RC, and SIGUSR2 == DNS_FULL, because then we have no more signal to reload/update/rewrite something else. Signals are just not flexible enough, which is a large reason for the D-Bus Reload command. So, I'd be fine with how it is. What do you think? > Can you update the NetworkManager man page to describe the new effect > of SIGHUP? Done. How about man: update documenting signals in NetworkManager manual fixup! config,dns: support Reload flags to specify that only parts > > config,dns: support Reload flags to specify that only parts should be reloaded > > Support 3 new flags for Reload: > > - 0x01 (CONF): reload the configuration from disk > - 0x02 (DNS_RC): write DNS configuration to resolv.conf > - 0x04 (DNS_FULL): restart DNS plugin > > > Probably DNS_RC is not the best name, as it doesn't necessarily update > resolv.conf (for example if dns=dnsmasq it updates dnsmasq). But TBH I > don't have any other suggestion :) Currently it's just internal name. I didn't yet put it to a public header, nor document it :) Still accepting suggestions... > /* The flags for Reload. Currently these are internal defines, > + * only their numeric value matters and must be stable as > + * they are public API! Also, the enum must fit in uint32. */ > +enum { /*< skip >*/ > + NM_MANAGER_RELOAD_FLAGS_NONE = 0, > > Then can this go in nm-dbus-interface.h ? Yeah, I didn't yet want to go all the way and add public API and document it (although the numeric values of the flags are effectively part of API). Basically, I didn't know how to name them, nor where to put them. Suggestions welcome. TODO. > Also, test-config fails here with: > /config/no-auto-default: > (src/tests/config/test-config:21302): GLib-CRITICAL **: Did not see > expected message NetworkManager-INFO: *config: update * > (NO_AUTO_DEFAULT,no-auto-default)* > > (but seems to pass on travis, I haven't investigated) Fixed. > The rest LGTM (pushed a trivial fixup). Thanks. Repushed.
> dns: reload DNS plugin in SIGHUP I don't really like the NM_IS_DNS_DNSMASQ (priv->plugin) checks in the manager; I think we keep it generic. Can we just save the old plugin name 'priv' and check against that? > config,dns: support Reload flags to specify that only parts should be reloaded We need D-Bus doc updates to describe the new flags... Also, I'm not sure DNS_FULL should really refer to the DNS plugin. Should just say something like "updates all DNS information including /etc/resolv.conf and restarts any DNS plugin that may be used". That doesn't cover the case of switching plugins on-the-fly though... Also, if we're going to declare these flags as public API, I'm not sure we should use their value as the NM_CONFIG_CHANGE_CAUSE_* flags. If they would diverge in the future it'll be more work to handle. We could just map the values in the Reload() call for now, that's trivial. General comment: It seems there are 4 places that we define PolKit permissions strings. Can we get that down to one place? I know that's not caused by this branch, but maybe we can make that better here.
(In reply to Thomas Haller from comment #6) > We only have 3 signals (HUP,USR1,USR2). I would not say, > SIGUSR1 == DNS_RC, and SIGUSR2 == DNS_FULL, because then we have no more > signal to reload/update/rewrite something else. Signals are just not > flexible enough, which is a large reason for the D-Bus Reload command. > > > So, I'd be fine with how it is. What do you think? Makes sense. > > Can you update the NetworkManager man page to describe the new effect > > of SIGHUP? > > Done. How about > man: update documenting signals in NetworkManager manual > fixup! config,dns: support Reload flags to specify that only parts LGTM.
(In reply to Dan Williams from comment #7) > > dns: reload DNS plugin in SIGHUP > > I don't really like the NM_IS_DNS_DNSMASQ (priv->plugin) checks in the > manager; I think we keep it generic. Can we just save the old plugin name > 'priv' and check against that? FWIW, before my patch it wasn't generic either... We do if (nm_streq0 (mode, "dnsmasq")) { if (!NM_IS_DNS_DNSMASQ (priv->plugin)) priv->plugin = nm_dns_dnsmasq_new (); else init_resolv_conf_mode() is fully aware of the available dns plugins. It's the one place which knows the name of the @mode and the corresponding NMDnsPlugin type. Not generic, not dynamic. More generic would be a infrastructure where plugins register themselves, for example like for internal device factories where: NM_DEVICE_FACTORY_DEFINE_INTERNAL() defines: static void __attribute__((constructor)) register_device_factory_internal_##lower (void) \ { \ g_type_ensure (NM_TYPE_##upper##_FACTORY); \ } and then G_DEFINE_TYPE_EXTENDED (NM##mixed##Factory, nm_##lower##_factory,... G_IMPLEMENT_INTERFACE (NM_TYPE_DEVICE_FACTORY,... which calls _nm_device_factory_internal_register_type This is generic -- for something that is actually static, so, we only do this, because it seems more elegant, not because it's dynamic. IMO, generic can make code cleaner or more complicated. IMO, generic is only really needed if the types are not known at compile-time. For, static choices, it may or may not be cleaner than a large know-them-all factory method. (for external NMDeviceFactories it is different, because we find them by loading shared libraries and invoking nm_device_factory_create()). It is dynamic and of course must be generic). We have literally 2 plugins. IMO a static, hard-coded init_resolv_conf_mode() is more maintainable then a static, generic, self-registering factory thing. > > config,dns: support Reload flags to specify that only parts should be reloaded > > We need D-Bus doc updates to describe the new flags... Added fixup commits. How about that? > Also, I'm not sure DNS_FULL should really refer to the DNS plugin. Should > just say something like "updates all DNS information including > /etc/resolv.conf and restarts any DNS plugin that may be used". It says exactly this, doesn't it? > That doesn't cover the case of switching plugins on-the-fly though... Sorry, I don't follow. If you do Reload(DNS_FULL), it will not re-read NetworkManager.conf, and thus not switch plugins (because the @mode is the same as before). Indeed it does recreate the new DNS plugin instance of the same type. But that is only because DNS plugins don't have a "restart_full()" function. Instead, a full-restart is implemented by throwing away the old instance and create a new one. This is only an implementation detail. > Also, if we're going to declare these flags as public API, I'm not sure we > should use their value as the NM_CONFIG_CHANGE_CAUSE_* flags. If they would > diverge in the future it'll be more work to handle. We could just map the > values in the Reload() call for now, that's trivial. Initially I had: NM_CONFIG_CHANGE_CAUSE_SIGHUP = (1L << 0), NM_CONFIG_CHANGE_CAUSE_SIGUSR1 = (1L << 1), NM_CONFIG_CHANGE_CAUSE_SIGUSR2 = (1L << 2), NM_CONFIG_CHANGE_CAUSE_NO_AUTO_DEFAULT = (1L << 3), NM_CONFIG_CHANGE_CAUSE_SET_VALUES = (1L << 4), + NM_CONFIG_CHANGE_CAUSE_CONF = (1L << 5), + NM_CONFIG_CHANGE_CAUSE_DNS_RC = (1L << 6), + NM_CONFIG_CHANGE_CAUSE_DNS_FULL = (1L << 7), then I changed it to: + NM_CONFIG_CHANGE_CAUSE_CONF = NM_MANAGER_RELOAD_FLAGS_CONF, + NM_CONFIG_CHANGE_CAUSE_DNS_RC = NM_MANAGER_RELOAD_FLAG..., + NM_CONFIG_CHANGE_CAUSE_DNS_FULL = NM_MANAGER_RELOAD_FLAGS..., + NM_CONFIG_CHANGE_CAUSE_SIGHUP = (1L << 3), + NM_CONFIG_CHANGE_CAUSE_SIGUSR1 = (1L << 4), + NM_CONFIG_CHANGE_CAUSE_SIGUSR2 = (1L << 5), + NM_CONFIG_CHANGE_CAUSE_NO_AUTO_DEFAULT = (1L << 6), + NM_CONFIG_CHANGE_CAUSE_SET_VALUES = (1L << 7), NM_CONFIG_CHANGE_* are entirely internal. I let NM_CONFIG_CHANGE_CAUSE_CONF have the same numeric value as NM_MANAGER_RELOAD_FLAGS_CONF, because they are somehow related (I wanted that if you grep for NM_MANAGER_RELOAD_FLAGS_CONF, you find it). There is no strong reason, the numeric values can change arbitrarily, as long as they are unique power-of-2s. Anyway, I don't mind. I reverted to what I had previously. See fixup. > General comment: > > It seems there are 4 places that we define PolKit permissions strings. Can > we get that down to one place? I know that's not caused by this branch, but > maybe we can make that better here. Added new patches: libnm: implement missing NM_AUTH_PERMISSION_SETTINGS_MODIFY_GLOBAL_DNS shared: add "nm-nmmacros.h" header all: move NM_AUTH_PERMISSION_* defines to "nm-nmmacros.h" header How about now?
> I am running short on a good name, considering so much > shared/core/macros/utils headers. So, forgive me the name, but > it may make some sense. nm-common perhaps? LGTM
(In reply to Lubomir Rintel from comment #10) > > I am running short on a good name, considering so much > > shared/core/macros/utils headers. So, forgive me the name, but > > it may make some sense. > > nm-common perhaps? renamed to "nm-common-macros.h"
Seems this branch was sufficiently discussed and has enough ACKs. Thanks. I merge it :) https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=431c70832dba9477b637702cac04468711713332