After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763524 - [review] bg/autoconnect-retries-bgo763524 - Rick Astley autoconnect mode
[review] bg/autoconnect-retries-bgo763524 - Rick Astley autoconnect mode
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: nm-review
 
 
Reported: 2016-03-12 09:45 UTC by Tore Anderson
Modified: 2016-10-16 11:08 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Tore Anderson 2016-03-12 09:45:37 UTC
nm-settings-connection.c contains the following defaults:

#define AUTOCONNECT_RETRIES_DEFAULT 4
#define AUTOCONNECT_RESET_RETRIES_TIMER 300

Basically this causes NM to retry the connection four times in rapid succession, wait five minutes, then retry four times more, and so on.

There's to the best of my knowledge no way of changing these defaults in global or per-connection config.

For some applications, network connectivity is of utmost importance, and it would be beneficial if it was possible to have NM go "never gonna give you up" about an autoconnected connection. Essentially make it possible to set AUTOCONNECT_RETRIES_DEFAULT to infinity and/or AUTOCONNECT_RESET_RETRIES_TIMER to 0 without recompiling.

The same use case exists for connections that have been manually enabled, e.g., with "nmcli con up $con". Sometimes it would be preferable to have it keep retrying and retrying to get the desired connection up rather than giving up after a timeout.
Comment 1 Beniamino Galvani 2016-10-08 08:48:11 UTC
Branch bg/autoconnect-retries-bgo763524 allows specifying a custom autoconnect-retries value for each connection, with 0 meaning 'infinite'. It also makes possible to effectively use a connection fallback strategy; for example one can have a DHCP connection with higher autoconnect priority, autoconnect-retries=1 and ipv4.dhcp-timeout=10; and a second static connection which will be activated if DHCP doesn't succeed in 10 seconds.

For manually activated connections, I think the user must always take care of retrying the connection if it fails.
Comment 2 Thomas Haller 2016-10-08 09:39:29 UTC
Can you move commit "core: handle the autoconnect-retries property" as last, after ifcfg-rh and nmcli support?



currently all connection-defaults are really per-device values. That is, you can configure

  [connection.eth0]
  match-device=eth0
  connection.autoconnect-retries=5

If only for consistency, that should be the case of the new setting as well. 
The retires-count should be a per-device *and* per-connection setting.
Note that you are always advised to pass the device to nm_config_data_get_connection_default().




+    s_con = nm_connection_get_setting_connection ((NMConnection *) self);
+    if (s_con) {
+         int retries = nm_setting_connection_get_autoconnect_retries (s_con);

the "if" is not really expected to fail, because the connection is supposed to verify. Anyway, missing setting should be treated like having all properties set to default. Thus:

     tries = -1;
     s_con = nm_connection_get_setting_connection ((NMConnection *) self);
     if (s_con)
         tries = nm_setting_connection_get_autoconnect_retries (s_con);
     
     if (tries == -1)
         ...
Comment 3 Beniamino Galvani 2016-10-10 14:52:20 UTC
(In reply to Thomas Haller from comment #2)
> Can you move commit "core: handle the autoconnect-retries property" as last,
> after ifcfg-rh and nmcli support?

Ok.

> currently all connection-defaults are really per-device values. That is, you
> can configure
> 
>   [connection.eth0]
>   match-device=eth0
>   connection.autoconnect-retries=5
> 
> If only for consistency, that should be the case of the new setting as well. 
> The retires-count should be a per-device *and* per-connection setting.
> Note that you are always advised to pass the device to
> nm_config_data_get_connection_default().

The problem is, which device? A connection is not necessarily bound to
a specific device and can be auto-activated on a different device for
each try. These devices can have a different default value set for
autoconnect-retries and it's not clear which one should be used.

But probably that is a corner case, how about the fixup?

> +    s_con = nm_connection_get_setting_connection ((NMConnection *) self);
> +    if (s_con) {
> +         int retries = nm_setting_connection_get_autoconnect_retries
> (s_con);
> 
> the "if" is not really expected to fail, because the connection is supposed
> to verify. Anyway, missing setting should be treated like having all
> properties set to default.

Changed.
Comment 4 Thomas Haller 2016-10-10 15:27:34 UTC
> The problem is, which device? A connection is not necessarily bound to
> a specific device and can be auto-activated on a different device for
> each try. These devices can have a different default value set for
> autoconnect-retries and it's not clear which one should be used.

exactly :)

well, you could just have a regular NetworkManager.conf value like

  [main]
  autoconnect-retry-default=1

which has the device-independent meaning. I think if put it into the [connection] section, it should have the per-device semantic with the match-device and stop-match settings.
Comment 5 Beniamino Galvani 2016-10-12 15:13:52 UTC
(In reply to Thomas Haller from comment #4)
> > The problem is, which device? A connection is not necessarily bound to
> > a specific device and can be auto-activated on a different device for
> > each try. These devices can have a different default value set for
> > autoconnect-retries and it's not clear which one should be used.
> 
> exactly :)
> 
> well, you could just have a regular NetworkManager.conf value like
> 
>   [main]
>   autoconnect-retry-default=1

Yeah, I think the [connection] section is not appropriate for specifying the autoconnect retries default value. Branch updated.
Comment 6 Thomas Haller 2016-10-14 10:01:11 UTC
lgtm
Comment 7 Dan Williams 2016-10-15 16:19:48 UTC
lgtm