Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1062301 - NetworkManager should provide a way to reload a configuration and to refresh resolv.conf if necessary
Summary: NetworkManager should provide a way to reload a configuration and to refresh ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: 7.0
Assignee: Lubomir Rintel
QA Contact: Desktop QE
URL:
Whiteboard:
: 1066697 (view as bug list)
Depends On: 1260577
Blocks: 1063731
TreeView+ depends on / blocked
 
Reported: 2014-02-06 15:42 UTC by Tomáš Hozza 🤓
Modified: 2016-03-29 14:25 UTC (History)
9 users (show)

Fixed In Version: NetworkManager-1.0.6-4.el7
Doc Type: Bug Fix
Doc Text:
The resolver configuration update mode can now be changed without restarting the daemon.
Clone Of:
Environment:
Last Closed: 2015-11-19 10:52:44 UTC
Target Upstream Version:


Attachments (Terms of Use)
[patch] use guint type instead of GFlags for config-changed signal (deleted)
2015-09-07 10:22 UTC, Thomas Haller
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2015:2315 normal SHIPPED_LIVE Moderate: NetworkManager security, bug fix, and enhancement update 2015-11-19 10:06:58 UTC
GNOME Bugzilla 732941 None None None Never

Description Tomáš Hozza 🤓 2014-02-06 15:42:36 UTC
Description of problem:
Currently there is no way (nmcli command / libnm-glib API / ...) how one can
tell NetworkManager to reload (configuration) and to correct any system settings
if necessary (such as writing current information to resolv.conf).

We are especially interested in the described resolv.conf use case due to
Bug #1061370. Right now there is no other way than restart NetworkManager which
has some side-effects (disconnecting VPNs).

Version-Release number of selected component (if applicable):
NetworkManager-0.9.9.0-38.git20140131.el7

Comment 1 RHEL Product and Program Management 2014-03-22 06:05:21 UTC
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.

Comment 3 Thomas Haller 2014-07-01 14:20:38 UTC
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.

Comment 4 Dan Winship 2014-11-26 19:46:36 UTC
(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.

Comment 5 Pavel Šimerda (pavlix) 2014-11-27 11:44:52 UTC
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.

Comment 6 Lubomir Rintel 2015-02-16 16:52:48 UTC
Ready for review: lr/dns-reconfig-rh1062301

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

Comment 7 Thomas Haller 2015-02-16 17:12:50 UTC
>> 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).

Comment 8 Thomas Haller 2015-02-18 12:41:18 UTC
*** Bug 1066697 has been marked as a duplicate of this bug. ***

Comment 9 Thomas Haller 2015-02-18 12:51:23 UTC
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.

Comment 10 Thomas Haller 2015-02-18 12:55:11 UTC
(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"...

Comment 11 Lubomir Rintel 2015-02-23 08:50:52 UTC
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.

Comment 12 Lubomir Rintel 2015-02-25 17:32:16 UTC
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'

Comment 15 Lubomir Rintel 2015-04-16 16:50:22 UTC
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

Comment 16 Thomas Haller 2015-04-16 17:18:15 UTC
(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.

Comment 17 Lubomir Rintel 2015-04-16 17:28:27 UTC
(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.

Comment 18 Lubomir Rintel 2015-05-05 15:10:45 UTC
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

Comment 21 Thomas Haller 2015-06-24 20:34:56 UTC
Hm, I see that we only rewrite resolv.conf, if the config actually changed.
I think we should rewrite it on every SIGHUP...

Comment 26 Thomas Haller 2015-06-25 19:35:15 UTC
Pushed two additional patches to upstream branch th/dns-update-on-sigusr1-rh1062301.

Please review.

Comment 27 Jirka Klimes 2015-06-26 07:21:44 UTC
(In reply to Thomas Haller from comment #26)
> Pushed two additional patches to upstream branch
> th/dns-update-on-sigusr1-rh1062301.
> 
> Please review.

LGTM

Comment 28 Beniamino Galvani 2015-06-26 07:37:03 UTC
> 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.

Comment 38 Vladimir Benes 2015-09-04 08:40:57 UTC
kill -SIGUSR1 $(pidof NetworkManager) doesn't work on s390x.. moving back to ASSIGNED

Comment 39 Thomas Haller 2015-09-07 10:01:46 UTC
(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...

Comment 40 Thomas Haller 2015-09-07 10:22:41 UTC
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.

Comment 41 Thomas Haller 2015-09-09 11:13:23 UTC
(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)

Comment 43 Vladimir Benes 2015-09-09 20:48:04 UTC
Verified on *all* supported architectures now. Both ppc64 and s390x work, too.

Comment 44 errata-xmlrpc 2015-11-19 10:52:44 UTC
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


Note You need to log in before you can comment on or make changes to this bug.