GNOME Bugzilla – Bug 709830
problems with reload-connections=false
Last modified: 2013-11-15 19:44:39 UTC
libvirt assumes that it can (via netcf) create new files in /etc/sysconfig/network-scripts/ and then ifup them. This does not work with reload-connections=false. Having libvirt/netcf/ifup/nmcli automatically call ReloadConnections in this case would be bad, because the user might not want other connections to be reloaded. Having nmcli/NetworkManager try to automatically add the new connection in this case would also be bad though, because then if you create a new ifcfg file, ifup it, realize something is wrong, ifdown it, edit the ifcfg file, and ifup it again, then you still get the original broken state the second time you ifup, and get very confused. The number one problem reload-connections was supposed to solve was connections being deactivated because their files were deleted. But that's fixable in other ways. So how about we: 1. Revert reload-connections, ReloadConnections, and "nmcli con reload" 2. Change NMPolicy to not deactivate connections when they are removed or made not-visible. Instead, the old o.fd.NM.Settings.Connection would just get marked Unsaved, and then if the deleted connection file comes back, we would create a new o.fd.NM.Settings.Connection for it. Though what to do about UUIDs? Maybe it doesn't matter if there are two connections with the same UUID as long as only one of them is returned by ListConnections / GetConnectionByUuid? ie, the old connection would still be creatable as an NMRemoteConnection, but it would not be visible to NMRemoteSettings. 3. Change NMPolicy so that directly editing a connection file will not cause it to become autoactivatable if it wasn't already autoactivatable. So, eg, if I have an ethernet cable plugged in, and I create an ONBOOT=yes ifcfg-eth0, then NM would *not* then immediately activate that connection. And I can edit the file some more, and it would still not activate the connection. However, if I then unplugged and re-plugged the cable, NM would activate it at that point (since now the change that made it activatable was a carrier change, not a file edit). Or alternatively, I could activate the connection by hand. The goal being to not autoactivate connections if the admin might still be in the process of making changes to them.
That was one reason for "reload connections". But there were a few others, which were in no particular order: 1) handle editors moving things around and temporary files 2) handle things that rewrite the connections via scripts and may open/close/open/close many times before it's done 3) more closely align with other tools like systemd, udev, etc that all use explicit configuration reload commands 4) allow multi-file configurations to be written to disk before all changes are reloaded I think it's a worthwhile goal to keep an explicit reload command, but what about the ideas we'd had about a connection-specific reload dbus command and corresponding command for nmcli? It could take any number of argument formats, like UUID or filename, or even better, an array of arguments such that multiple files could be reloaded at the same time, maybe even returning an error and throwing away the changes if one of the set failed. That could fix another bug we have right now, which was "provide some way of validating configuration without actually loading it". For (2) above, I don't think we can skip deactivation when the connection is made invisible to the current user, because that would be very tricky internally but also violate some security guarantees; if the connection is still up but no longer visible, something had to happen to make it invisible, and that's almost always a user action. I do agree however that we could somehow mark the connection as "unsaved" when it's deleted; though we'd have to be careful about UUID conflicts if it ever appeared again with different settings. I don't think we should violate the unique UUID rules. Maybe also explore having NM reload connections automatically on activate if they have changed from disk? Not 100% sure about this one since it changes the semantics of saved/unsaved a bit.
(In reply to comment #0) > Having libvirt/netcf/ifup/nmcli automatically call ReloadConnections in this > case would be bad, because the user might not want other connections to be > reloaded. There should really be a method to reload that specific connection. > 1. Revert reload-connections, ReloadConnections, and "nmcli con reload" What about just changing reload-connections to true by default until the issue is resolved? That works well enough and doesn't remove the long-awaited feature entirely. > 2. Change NMPolicy to not deactivate connections when they are removed > or made not-visible. Instead, the old o.fd.NM.Settings.Connection > would just get marked Unsaved, and then if the deleted connection > file comes back, we would create a new o.fd.NM.Settings.Connection > for it. Though what to do about UUIDs? Maybe it doesn't matter if > there are two connections with the same UUID as long as only one of > them is returned by ListConnections / GetConnectionByUuid? ie, the > old connection would still be creatable as an NMRemoteConnection, > but it would not be visible to NMRemoteSettings. Looks very hacky. If that's just libvirt, couldn't they simply use an API to *load and activate* the connection? > 3. Change NMPolicy so that directly editing a connection file will not > cause it to become autoactivatable if it wasn't already > autoactivatable. > So, eg, if I have an ethernet cable plugged in, and I create an > ONBOOT=yes ifcfg-eth0, then NM would *not* then immediately activate > that connection. And I can edit the file some more, and it would > still not activate the connection. However, if I then unplugged and > re-plugged the cable, NM would activate it at that point (since now > the change that made it activatable was a carrier change, not a file > edit). Or alternatively, I could activate the connection by hand. Sounds possible but still hacky. Over time, services should learn to use NetworkManager API, not to write its configuration directly. Requiring them to at least call a command to actively load the connection doesn't sound like a big requirement, expecially if they already do that to activate it. When using CLI for that, it might be sensible to add a simple command line switch --reload to the command that activates a connection.
(In reply to comment #1) > I think it's a worthwhile goal to keep an explicit reload command, but what > about the ideas we'd had about a connection-specific reload dbus command and > corresponding command for nmcli? It could take any number of argument formats, > like UUID or filename Given that the API would only be used in cases where the client knows the filename involved, and given that the filename is also the identifier that the backend needs, it seems like we really only need to accept filenames. > or even better, an array of arguments such that multiple > files could be reloaded at the same time, maybe even returning an error and > throwing away the changes if one of the set failed. That could fix another bug > we have right now, which was "provide some way of validating configuration > without actually loading it". That seems like an unrelated bug. I've pushed danw/reload that adds o.fd.NM.Settings.LoadConnection, nm_remote_settings_load_connection(), and "nmcli con reload /path/to/file", with LoadConnection being implemented by ifcfg-rh and keyfile. (The APIs use "load" in the name, because you might be loading a new connection, not "reloading". But nmcli uses "reload" because it just felt like it made sense to merge this command into the existing "nmcli con reload"... But maybe we'd rather have them use the same word...) And then we'd do this to ifup: --- ifup.orig 2013-10-31 15:44:19.700337451 -0400 +++ ifup 2013-10-31 15:44:24.129337691 -0400 @@ -68,8 +68,9 @@ if [ "$_use_nm" = "true" -a -n "$UUID" ]; then if [ "foo$2" = "fooboot" ] && [ "${TYPE}" = "Wireless" ]; then exit 0 fi + nmcli con reload "/etc/sysconfig/network-scripts/$CONFIG" nmcli con up uuid "$UUID" exit $? fi
Hi, Wouldn't it be better, if LoadConnection already now accepts a list of filenames instead of just one? To load a set of several configurations together (atomically) seems very useful to me. If we don't want to implement yet this atomic, multi-file load, we can just fail with Not-Yet-Implemented for now, but the dbus-interface should be already full-fledged. - connection files.) Ass with AddConnection(), this operation + connection files.) As with AddConnection(), this operation Apart from that, all looks good to me.
Created attachment 258927 [details] [review] [PATCH] cli: adjust bash completion for `nmcli connection reload <file>` Extend bash completion, to complete for file names in `nmcli connection reload`
(In reply to comment #3) > --- ifup.orig 2013-10-31 15:44:19.700337451 -0400 > +++ ifup 2013-10-31 15:44:24.129337691 -0400 > @@ -68,8 +68,9 @@ > if [ "$_use_nm" = "true" -a -n "$UUID" ]; then > if [ "foo$2" = "fooboot" ] && [ "${TYPE}" = "Wireless" ]; then > exit 0 > fi > + nmcli con reload "/etc/sysconfig/network-scripts/$CONFIG" > nmcli con up uuid "$UUID" > exit $? > fi That looks great. (In reply to comment #4) > Hi, > > > Wouldn't it be better, if LoadConnection already now accepts a list of > filenames instead of just one? To load a set of several configurations together > (atomically) seems very useful to me. If we don't want to implement yet this > atomic, multi-file load, we can just fail with Not-Yet-Implemented for now, but > the dbus-interface should be already full-fledged. A small +1 from me, it would be really nice to be able to reload a set of related connections (e.g. master+slave). You don't even have to return a not-implemented error, you can just treat it as a partial reload, then in the future investigate to what degree atomicity matters and just document the improvements.
Multi-file would be nice from the start if it's not too much hassle? Just an 'as' on the parameter list I'd think and the associated code to handle the array. For the current code though: > core: add o.fd.NM.Settings.LoadConnection "Object path of the new connection that was just added." Should be something more like "Object path of the new or updated connection that was just read from disk." The existing text doesn't talk about what the 'path' return parameter means when the connection already exists, but got re-read. Also, should impl_settings_load_connection() check g_path_is_absolute()? > libnm-glib: add nm_remote_settings_load_connection() Since: tag on nm_remote_settings_load_connection()
> libnm-glib: add nm_remote_settings_load_connection() Add Since: 0.9.10 tag for nm_remote_settings_load_connection() load_connection_done() > cli: add "nmcli con reload /path/to/file" to reload a single connection nmcli reload description with regards to monitor-connection-files=false has been fixed before, so you don't have to mention it in the commit message now. + _("Error: Failed to load connection '%s': (%d) %s"), + info->filename, error->code, error->message); The first %s is not a connection name, but a file name. So it may be better to say something like "Failed to load a connection from file %s". We could also check if the filename exists, before passing it to nm_remote_settings_load_connection().
(In reply to comment #7) > Multi-file would be nice from the start if it's not too much hassle? ok, done, and moved to "nmcli con load" rather than "reload", and updated Thomas's bash completion stuff to match. I changed the return value to be an array of the filenames that failed to load, since it seems like that's what would be useful, given the suggested use cases... > Also, should impl_settings_load_connection() check g_path_is_absolute()? It doesn't need to, since each plugin checks that the filename starts with the right prefix ("/etc/sysconfig/network-scripts/", "/etc/NetworkManager/system-connections"). But I added a check anyway so we can give an explicit warning when people forget. Other stuff mentioned is fixed or no longer applies to the current code.
(In reply to comment #9) > I changed the return value to be an array of the filenames that failed to load, > since it seems like that's what would be useful, given the suggested use > cases... Sorry, for being pedantic on this, but do we ever want atomic loading? If yes, then the return value does not make much sense (because it would be always all or nothing). And probably it's anyway a very bad idea, to change the semantics later on. But what about adding a boolean parameter 'load_atomic' to 'LoadConnections'? If it is TRUE and more then one filename is given, it simply fails with not-yet-implemented for the moment. << +1 from me for this Or we don't plan to implement atomic loading, and if we ever add it, we need another function 'LoadConnectionsAtomic'...
(other then my comment #10, all looks good to me)
(In reply to comment #10) > But what about adding a boolean parameter 'load_atomic' to 'LoadConnections'? > If it is TRUE and more then one filename is given, it simply fails with > not-yet-implemented for the moment. << +1 from me for this It very much depends on the exact meaning of atomic. You might indeed want to choose whether you want to fail the whole process or not due to atomicity. > Or we don't plan to implement atomic loading, and if we ever add it, we need > another function 'LoadConnectionsAtomic'... Indeed you wouldn't need multi-loading in the current API so much if you'd be adding atomic loading via a separate function. Good catch!
(In reply to comment #12) > Indeed you wouldn't need multi-loading in the current API so much if you'd be > adding atomic loading via a separate function. Good catch! I think, extending LoadConnections to accept several filenames (even without atomic loading) is still nice as API and quite easy to implement (just loop over the several filenames instead of having only one). Loading several connections atomic (all or none) would be nice/useful to have, but is more complicated to implement. I am asking, what the API should look like, if we only implement the first case for now, but later might add the atomic loading.
(In reply to comment #10) > Or we don't plan to implement atomic loading, and if we ever add it, we need > another function 'LoadConnectionsAtomic'... I think this makes the most sense. There's no reason to design the current API around what we *might* want to do in the future, when we can just add a new API then if we want to do something different.
I feel like nm_remote_settings_load_connections() should return gboolean and the failures should be passed back as an out parameter... It's a bit confusing now since the function will return NULL for a successful load, then you'd have to check 'error' to find out if something failed... Other than that, everything looks good.
pushed after re-review with dcbw's suggested fix