Bug 1062301

This request was not resolved in time for the current release.
Red Hat invites you to ask your support representative to
propose this request, if still desired, for consideration in
the next release of Red Hat Enterprise Linux.

Regarding the RFE for reloading of configuration, this is identical to bug 1066697. This bug now depends on bug 1066697.

So, the remaining part of the bug is adding new API to write out resolv.conf.

(In reply to Thomas Haller from comment #3)
> Regarding the RFE for reloading of configuration, this is identical to bug
> 1066697. This bug now depends on bug 1066697.

I don't think that's what he meant by configuration. He appears to be talking about making the network configuration match the set of currently-active connections. With "rewrite resolv.conf" being one example of that.

I think we could basically live without the resolv.conf rewrite feature if NetworkManager used a private resolv.conf, wrote it whenever it sees fit and used a symlink in /etc/resolv.conf. We could then restore the symlink when we hand over control back to NetworkManager. We don't alter any other configuration, so it might be more practical to treat the non-resolv.conf parts of this bug as mere suggestions.

Ready for review: lr/dns-reconfig-rh1062301

1e67e57 dns-manager: react to dns management mode changes
fcb39aa config: move dns mode configuration to NMConfigData

>> config: move dns mode configuration to NMConfigData
 
-    if (g_strcmp0 (nm_config_data_get_dns_mode (old_data), nm_config_data_get_dns_mode (new_data)))
+    if (g_strcmp0 (priv_old->dns_mode, priv_new->dns_mode))
          changes |= NM_CONFIG_CHANGE_DNS_MODE;


>> dns-manager: react to dns management mode changes

whitespace error in dispose(), nm_dns_manager_init()


maybe:
-    if (!(changes & NM_CONFIG_CHANGE_DNS_MODE))
-         return;
-
-    init_resolv_conf_mode (self);
+    if (NM_FLAGS_HAS (changes, NM_CONFIG_CHANGE_DNS_MODE))
+        init_resolv_conf_mode (self);



Otherwise, LGTM (didn't test it though).

*** Bug 1066697 has been marked as a duplicate of this bug. ***

I closed bug 1066697 as duplicate of this.

What lr/dns-reconfig-rh1062301 does, pretty much solves bug 1066697.



Note that the dupe talks about reloading ~only~ DNS information on some special signal. I am not sure we need this granular reloading, but if we do, then it is missing in lr/dns-reconfig-rh1062301.

for that, we would have to listen to some special signal (maybe not even SIGUSR2, maybe use something between SIGRTMIN and SIGRTMAX).
Would be trivial to extend nm_config_reload() to accept a flags argument to reload only a partial configuration.

(In reply to Thomas Haller from comment #9)
> for that, we would have to listen to some special signal (maybe not even
> SIGUSR2, maybe use something between SIGRTMIN and SIGRTMAX).
> Would be trivial to extend nm_config_reload() to accept a flags argument to
> reload only a partial configuration.

Alternatively, instead of signals, we could add a privileged D-Bus method "ReloadConfig", that gets a set of flags "what" to reload, e.g. "all", "connectivity", "dns"...

I believe a D-Bus config that would reload specific parts of the configuration is unneeded complexity at this point. There's no point keeping the configuration that should not be effective around for longer periods of time. It's common for UNIX daemons to reload the whole configuration after something (puppet, RPM) replaces/modifies it and sends HUP shortly afterwards.

Pushed updated branch.

Merged to master

aa672b2 config: move dns mode configuration to NMConfigData
73e8aea dns-manager: react to dns management mode changes
d7f977e dns-manager,config: merge branch 'lr/dns-reconfig-rh1062301'

Apart from commits from comment #12, thomas' th/rh1066697_reload_config branch would need to be merged.

Rebased on nm-1-0 here: lr/nm-1-0-rh1066697_reload_config

(In reply to Lubomir Rintel from comment #15)
> Apart from commits from comment #12, thomas' th/rh1066697_reload_config
> branch would need to be merged.
> 
> Rebased on nm-1-0 here: lr/nm-1-0-rh1066697_reload_config

did all the cherry-picking succeed without conflicts?

Maybe redo it with "git cherry-pick -x"? I feel that the commit message is valuable information for a backport.

(In reply to Thomas Haller from comment #16)
> (In reply to Lubomir Rintel from comment #15)
> > Apart from commits from comment #12, thomas' th/rh1066697_reload_config
> > branch would need to be merged.
> > 
> > Rebased on nm-1-0 here: lr/nm-1-0-rh1066697_reload_config
> 
> did all the cherry-picking succeed without conflicts?

Not really, a couple of conflicts.
Reasonably trivial ones, though.
 
> Maybe redo it with "git cherry-pick -x"? I feel that the commit message is
> valuable information for a backport.

Yes, I intend to tidy up the commit messages if we agree to merge this into 1.0.

Merged to nm-1-0 (post 1.2 release)

33f9aab dns-manager: react to dns management mode changes
68d9a8e config: move dns mode configuration to NMConfigData

Depends on bug #1066697 comment #39

Hm, I see that we only rewrite resolv.conf, if the config actually changed.
I think we should rewrite it on every SIGHUP...

Pushed two additional patches to upstream branch th/dns-update-on-sigusr1-rh1062301.

Please review.

(In reply to Thomas Haller from comment #26)
> Pushed two additional patches to upstream branch
> th/dns-update-on-sigusr1-rh1062301.
> 
> Please review.

LGTM

> config: pass signals to nm_config_reload()

+               nm_log_info (LOGD_CORE, "config: update %s (%s)", nm_config_data_get_config_description (new_data),
+                            (log_str = nm_config_change_flags_to_string (changes)));

+               nm_log_info (LOGD_CORE, "config: signal %s (no changes from disk)", (log_str = nm_config_change_flags_to_string (changes)));

+               nm_log_info (LOGD_CORE, "config: signal %s", (log_str = nm_config_change_flags_to_string (changes)));

Are these assignments to log_str needed?

The rest LGTM.

merged upstream:

master:
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=7bf78d9511c15e9fdae0e7e0cd6b3beeffeddd0f

nm-1-0:
http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a12bc0060f68e8009cb522ecaac9730fa0d9c85a

kill -SIGUSR1 $(pidof NetworkManager) doesn't work on s390x.. moving back to ASSIGNED

(In reply to Vladimir Benes from comment #38)
> kill -SIGUSR1 $(pidof NetworkManager) doesn't work on s390x.. moving back to
> ASSIGNED


The problem is, that in config_changed_cb ( http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/dns-manager/nm-dns-manager.c?id=f768f09a861c6cbe01b476ab64b256adf47479c6#n1136 ) the @changes argument is zero, although it shouldn't be.

I think this is a bug in glib. Opened bug 1260577 for that.

Technically, we could replace the enum-typed argument that causes the issue by a guint-typed argument. That fixes the issue but introduces ugliness in the code and it's not clear if other signals in the code base are not affected as well.

So, I'd rather wait what happens to the glib bug...

Created attachment 1070936 [details]
[patch] use guint type instead of GFlags for config-changed signal

Patch applies on master (e6c64af8be89374ed318ed1a395ead7cd00a6753)

This patch would workaround the test failure on s390x and ppc64.

(In reply to Thomas Haller from comment #39)
> (In reply to Vladimir Benes from comment #38)
> > kill -SIGUSR1 $(pidof NetworkManager) doesn't work on s390x.. moving back to
> > ASSIGNED
> 
> I think this is a bug in glib. Opened bug 1260577 for that.

pushed workaround upstream:

master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e7d66f1df61ebdc7652ba34a4e1baddcc86b9e26

nm-1-0: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9aecabe29f39350c5e31a61af4d49062108dfc89



Note that there seem to be no other signals affected by this bug (http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=52cd5ee6120b643aa143cbd439f81a0c02c8313e)

Verified on *all* supported architectures now. Both ppc64 and s390x work, too.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2015-2315.html