GNOME Bugzilla – Bug 735052
[RFE] connecting a master (bridge/bond) should offer a way to bring up slaves too
Last modified: 2015-06-19 10:26:43 UTC
When connecting a master (bridge,bond,team), no slaves will connect automatically. I think that is correct in the default cause, but it would be great to have an option (e.g. to `nmcli connection up --with-slaves`) to force connecting slaves too. One question is *which* slaves to activate: - only the autoconnect ones? - only those on devices that are currently disconnected? I think it would be better to add an argument to the DBUS API and not implement this entirely in nmcli. That way, the same functionality is easily accessible to other clients as well. Or maybe an alternative would be to support multiple connection arguments to `nmcli connection up master slave1 slave2` This is also relevant for initscripts, which in the non-NM case behave differently for bridge and bond. bridge slaves are not connected by initscripts. However, bond slaves will be connected. So, for bonds, the NM backed devices behave differently. Therefore it would be useful to have this option in nmcli, so that initscripts could be fixed. See also bug 733641.
I pushed a branch adding the posibility to bring up slaves when master is activated. The decision is made by the new autoconnect-slaves property. There is also a global configuration option that can override it. The code is in jk/master-bringup-slaves.
--- a/src/settings/plugins/ifcfg-rh/reader.c +++ b/src/settings/plugins/ifcfg-rh/reader.c @@ -170,20 +170,22 @@ make_connection_setting (const char *file, NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_MAX, NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_DEFAULT), + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES, + svTrueValue (ifcfg, "AUTOCONNECT_SLAVES", FALSE), NULL); Maybe for "bond" slaves, we should default to "TRUE", to have backward compatibility with initscripts? And for all other types, we should default to "FALSE". >> config/core: introduce master-autoconnects-slaves configuration option +const char * +nm_config_get_master_autoconnects_slaves (NMConfig *config) +{ This should be a property of NMConfigData. By putting it there, the property can be reloaded via SIGHUP. You still can decide, whether you want to use to very first (non-reloaded) value, or the one from the last reload. nm_conig_get_data_orig() vs. nm_config_get_data(). See https://bugzilla.gnome.org/show_bug.cgi?id=721200#c3 or e573977b808409ccb26562ca8536d46be701f833. You ~can~ add a NM_CONFIG_CHANGE_* flag, you don't have to.
maybe nm_config_[data_]get_master_autoconnects_slaves() should return an enum, and the string parsing + slaves_config = nm_config_get_master_autoconnects_slaves (config); + if (!g_strcmp0 (slaves_config, "never")) + should_connect_slaves = FALSE; + else if (!g_strcmp0 (slaves_config, "always")) + should_connect_slaves = TRUE; + else { should happen inside NMConfig[Data]? Including logging a warning about invalid setting once, instead at every autoconnect_slaves(). A whitespace error: + /* Schedule slave activation */ + nm_manager_activate_connection (manager, + slave_connection, ^^^^^^^^^^^ + NULL, + I like the idea with this new configuration option...
(In reply to Thomas Haller from comment #2) > --- a/src/settings/plugins/ifcfg-rh/reader.c > +++ b/src/settings/plugins/ifcfg-rh/reader.c > @@ -170,20 +170,22 @@ make_connection_setting (const char *file, > > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_MAX, > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_DEFAULT), > + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES, > + svTrueValue (ifcfg, "AUTOCONNECT_SLAVES", FALSE), > NULL); > > > Maybe for "bond" slaves, we should default to "TRUE", to have backward > compatibility with initscripts? > And for all other types, we should default to "FALSE". > Added "ifcfg-rh: default to AUTOCONNECT_SLAVES=TRUE for bonds" commit doing that. Though, I am not sure if this won't bring more confusion provided we special-case it for ifcfg and bonds. > > >> config/core: introduce master-autoconnects-slaves configuration option > > > +const char * > +nm_config_get_master_autoconnects_slaves (NMConfig *config) > +{ > > > This should be a property of NMConfigData. > By putting it there, the property can be reloaded via SIGHUP. > > You still can decide, whether you want to use to very first (non-reloaded) > value, or the one from the last reload. > nm_conig_get_data_orig() vs. nm_config_get_data(). > > > See https://bugzilla.gnome.org/show_bug.cgi?id=721200#c3 or > e573977b808409ccb26562ca8536d46be701f833. > > You ~can~ add a NM_CONFIG_CHANGE_* flag, you don't have to. Great, I pushed a fixup commit moving the option to NMConfigData. (In reply to Thomas Haller from comment #3) > maybe nm_config_[data_]get_master_autoconnects_slaves() should return an > enum, and the string parsing > > + slaves_config = nm_config_get_master_autoconnects_slaves (config); > + if (!g_strcmp0 (slaves_config, "never")) > + should_connect_slaves = FALSE; > + else if (!g_strcmp0 (slaves_config, "always")) > + should_connect_slaves = TRUE; > + else { > > should happen inside NMConfig[Data]? Including logging a warning about > invalid setting once, instead at every autoconnect_slaves(). > I added a commit switching the option to enum. > > A whitespace error: > > > + /* Schedule slave activation */ > + nm_manager_activate_connection (manager, > + slave_connection, > ^^^^^^^^^^^ > + NULL, > + > Fixed.
(In reply to Jiri Klimes from comment #4) > (In reply to Thomas Haller from comment #2) > > --- a/src/settings/plugins/ifcfg-rh/reader.c > > +++ b/src/settings/plugins/ifcfg-rh/reader.c > > @@ -170,20 +170,22 @@ make_connection_setting (const char *file, > > > > > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_MAX, > > > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_DEFAULT), > > + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES, > > + svTrueValue (ifcfg, "AUTOCONNECT_SLAVES", FALSE), > > NULL); > > > > > > Maybe for "bond" slaves, we should default to "TRUE", to have backward > > compatibility with initscripts? > > And for all other types, we should default to "FALSE". > > > Added "ifcfg-rh: default to AUTOCONNECT_SLAVES=TRUE for bonds" commit doing > that. Though, I am not sure if this won't bring more confusion provided we > special-case it for ifcfg and bonds. Looks good to me. Only: could you re-verify that initscript actually has that behavior with regards of autoconnecting masters (bond vs. bridge). -- as I claimed in the original report? I think it does, just make sure. I think if initscripts does have that legacy behavior, than we should do it too... even if it's unexpected. Compatibility with initscripts is more important IMO. > > >> config/core: introduce master-autoconnects-slaves configuration option > > > > > > +const char * > > +nm_config_get_master_autoconnects_slaves (NMConfig *config) > > +{ > > > > > > This should be a property of NMConfigData. > > By putting it there, the property can be reloaded via SIGHUP. > > > > You still can decide, whether you want to use to very first (non-reloaded) > > value, or the one from the last reload. > > nm_conig_get_data_orig() vs. nm_config_get_data(). > > > > > > See https://bugzilla.gnome.org/show_bug.cgi?id=721200#c3 or > > e573977b808409ccb26562ca8536d46be701f833. > > > > You ~can~ add a NM_CONFIG_CHANGE_* flag, you don't have to. > > Great, I pushed a fixup commit moving the option to NMConfigData. All properties of NMConfigData must be read-only (or construct-only). As you did correctly for PROP_MASTER_AUTOCONNECTS_SLAVES. But then remove the case switch in set_property(). > (In reply to Thomas Haller from comment #3) > > maybe nm_config_[data_]get_master_autoconnects_slaves() should return an > > enum, and the string parsing > > > > + slaves_config = nm_config_get_master_autoconnects_slaves (config); > > + if (!g_strcmp0 (slaves_config, "never")) > > + should_connect_slaves = FALSE; > > + else if (!g_strcmp0 (slaves_config, "always")) > > + should_connect_slaves = TRUE; > > + else { > > > > should happen inside NMConfig[Data]? Including logging a warning about > > invalid setting once, instead at every autoconnect_slaves(). > > > > I added a commit switching the option to enum. + g_param_spec_enum (NM_CONFIG_DATA_MASTER_AUTOCONNECTS_SLAVES, "", "", + NM_TYPE_CONFIG_MASTER_AUTOCONNECTS_SLAVES, + NM_CONFIG_MASTER_AUTOCONNECTS_SLAVES_NEVER, G_PARAM_READABLE | why is the default here "never"? Should be per-master. IMO this enum version is better. I would just squash this commit.
(In reply to Thomas Haller from comment #5) > (In reply to Jiri Klimes from comment #4) > > (In reply to Thomas Haller from comment #2) > > > --- a/src/settings/plugins/ifcfg-rh/reader.c > > > +++ b/src/settings/plugins/ifcfg-rh/reader.c > > > @@ -170,20 +170,22 @@ make_connection_setting (const char *file, > > > > > > > > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_MAX, > > > > > > NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY_DEFAULT), > > > + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES, > > > + svTrueValue (ifcfg, "AUTOCONNECT_SLAVES", FALSE), > > > NULL); > > > > > > > > > Maybe for "bond" slaves, we should default to "TRUE", to have backward > > > compatibility with initscripts? > > > And for all other types, we should default to "FALSE". > > > > > Added "ifcfg-rh: default to AUTOCONNECT_SLAVES=TRUE for bonds" commit doing > > that. Though, I am not sure if this won't bring more confusion provided we > > special-case it for ifcfg and bonds. > > Looks good to me. > > Only: could you re-verify that initscript actually has that behavior with > regards of autoconnecting masters (bond vs. bridge). > -- as I claimed in the original report? I think it does, just make sure. > > I think if initscripts does have that legacy behavior, than we should do it > too... even if it's unexpected. Compatibility with initscripts is more > important IMO. > It seems that initscripts also autoconnects slaves for Team - pushed a fixup for that. We should re-check more thoroughly thought - test, ask initscripts maintainers. > > > > >> config/core: introduce master-autoconnects-slaves configuration option > > > > > > > > > +const char * > > > +nm_config_get_master_autoconnects_slaves (NMConfig *config) > > > +{ > > > > > > > > > This should be a property of NMConfigData. > > > By putting it there, the property can be reloaded via SIGHUP. > > > > > > You still can decide, whether you want to use to very first (non-reloaded) > > > value, or the one from the last reload. > > > nm_conig_get_data_orig() vs. nm_config_get_data(). > > > > > > > > > See https://bugzilla.gnome.org/show_bug.cgi?id=721200#c3 or > > > e573977b808409ccb26562ca8536d46be701f833. > > > > > > You ~can~ add a NM_CONFIG_CHANGE_* flag, you don't have to. > > > > Great, I pushed a fixup commit moving the option to NMConfigData. > > All properties of NMConfigData must be read-only (or construct-only). > As you did correctly for PROP_MASTER_AUTOCONNECTS_SLAVES. > But then remove the case switch in set_property(). > Done. > > > (In reply to Thomas Haller from comment #3) > > > maybe nm_config_[data_]get_master_autoconnects_slaves() should return an > > > enum, and the string parsing > > > > > > + slaves_config = nm_config_get_master_autoconnects_slaves (config); > > > + if (!g_strcmp0 (slaves_config, "never")) > > > + should_connect_slaves = FALSE; > > > + else if (!g_strcmp0 (slaves_config, "always")) > > > + should_connect_slaves = TRUE; > > > + else { > > > > > > should happen inside NMConfig[Data]? Including logging a warning about > > > invalid setting once, instead at every autoconnect_slaves(). > > > > > > > I added a commit switching the option to enum. > > + g_param_spec_enum (NM_CONFIG_DATA_MASTER_AUTOCONNECTS_SLAVES, "", > "", > + NM_TYPE_CONFIG_MASTER_AUTOCONNECTS_SLAVES, > + NM_CONFIG_MASTER_AUTOCONNECTS_SLAVES_NEVER, > G_PARAM_READABLE | > > > why is the default here "never"? Should be per-master. > Sure, thanks. It was a typo. > IMO this enum version is better. I would just squash this commit. Squashed.
>> core: activate slaves when master is activated (bgo #735052) (rh #1158529) doesn't this mean that when a (slave) device has already an active connection, we will auto-activate a slave on it? I think, if a (slave) device is not currently down, it should be ignored for this auto-connect slave feature.
(In reply to Thomas Haller from comment #7) > >> core: activate slaves when master is activated (bgo #735052) (rh #1158529) > > doesn't this mean that when a (slave) device has already an active > connection, we will auto-activate a slave on it? > > I think, if a (slave) device is not currently down, it should be ignored for > this auto-connect slave feature. other than that, LGTM
(In reply to Thomas Haller from comment #7) > >> core: activate slaves when master is activated (bgo #735052) (rh #1158529) > > doesn't this mean that when a (slave) device has already an active > connection, we will auto-activate a slave on it? > > I think, if a (slave) device is not currently down, it should be ignored for > this auto-connect slave feature. See a comment in find_slave(): /* Search through all connections, not only inactive ones, because * even if a slave was already active, it might be deactivated during * master reactivation. */ all_connections = nm_settings_get_connections (priv->settings); I did try m_manager_get_activatable_connections() function before in order to go through inactive connections only, but it showed up that if the slave was already active it was disconnected with master deactivation/activation and we would not bring it up back again. So all connections has to be gone through.
(In reply to Jiri Klimes from comment #9) > (In reply to Thomas Haller from comment #7) > > >> core: activate slaves when master is activated (bgo #735052) (rh #1158529) > > > > doesn't this mean that when a (slave) device has already an active > > connection, we will auto-activate a slave on it? > > > > I think, if a (slave) device is not currently down, it should be ignored for > > this auto-connect slave feature. > > See a comment in find_slave(): > /* Search through all connections, not only inactive ones, because > * even if a slave was already active, it might be deactivated during > * master reactivation. > */ > all_connections = nm_settings_get_connections (priv->settings); > > I did try m_manager_get_activatable_connections() function before in order > to go through inactive connections only, but it showed up that if the slave > was already active it was disconnected with master deactivation/activation > and we would not bring it up back again. So all connections has to be gone > through. yeah... that's probably right... Because a connection is only a @candidate if it has @master_connection == @connection. As we are about to up @master_connection, this means @candidate cannot be meaningful active otherwise.
Thinking about "main.master-autoconnects-slaves" again... how about this: "NMSettingConnection:autoconnect-slaves" is a tri-state: "default", "yes", "no". "yes" and "no" have obvious meaning. "default" means to use a global configuration option. Then we have a NetworkManager.conf option like [default-setting-connection] autoconnect-slaves=default|yes|no this new section, contains "default" values for "nm-setting-connection". So, when determining whether to autoconnect slaves, we would: 1) check "NMSettingConnection:autoconnect-slaves" 2) if "default", consult default-nm-setting-connection.autoconnect-slaves. 3) if "default": - here it gets ugly, we must call a virtual function in NMSettingsConnection, so that NMIfcfgConnection can return TRUE for bonds/team. While all other connection types return FALSE. Yes, 3) is a bit complicated/ugly, because we want this initscript-legacy behavior. Advantage: 1) The options "never", "per-master" and "always" need a separate explanation for their meaning. Having configurable default values is self-explaining and logic. It's not obvious that "main.master-autoconnects-slaves" interacts in contrived ways with the "connection.autoconnect-slaves" setting. 2) an explicit per-connection setting always wins. As it is now, nmcli shows you connection.autoconnect-slaves=yes|no but that value might be ignored due to global configuration. 3) I see this as a future pattern to define default values. See also bug 695383. Like in bug 721200, we could add [default-setting-ipv6] ip6-privacy=1 4) Downside: now in the client you see only: connection.autoconnect-slaves=default and it's not obvious what that means. Compare this to 2), there you see yes|no, but that value might ignored. This could be improved by having nmcli also fetch what the default value actually means, so that it would show connection.autoconnect-slaves=default (yes) This is complicated by the special treatment of bond/team which means that default value is not globally constant. Guess, nmcli would have to ask NM to "FillInDefaults()". Hm... this legacy behavior for team/bonds really complicates this scheme. We could instead make the option a 4 state: "default", "yes", "no", and "default-yes". That would simplify the problem from 4) and NM could just expose on D-Bus a hash of default values. So nmcli fetch the global configuration options and do: connection.autoconnect-slaves=default (no) NMIfcfgConnection would then set Team/Bonds without explicit settings to "default-yes".
for the big picture: https://bugzilla.gnome.org/show_bug.cgi?id=695383#c4
Ok, here is a proof-of-concept. https://bugzilla.gnome.org/show_bug.cgi?id=695383#c5
> ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES AUTOCONNECT_SLAVES is an NetworkManager extension. initscripts always activates slaves with the master connection. NetworkManager behaviour is controlled by this variable. The default value (if missing) is "no", meaning ithe opposite the behaviour than initscripts. s/ithe opposite the behaviour/the opposite behaviour/ ? > ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES Just a thought, would it make sense to write the property only for master connections? > cli: add support for connection.autoconnect-slaves property + nmc_add_prop_funcs (GLUE (CONNECTION, AUTOCONNECT_SLAVES), + nmc_property_connection_get_autoconnect, This reads a different property, there should be a new function nmc_property_connection_get_autoconnect_slaves(). I like Thomas' idea of having [yes|no|default] values for the connection.autoconnect-slaves because IMHO it would be more intuitive that an explicit [yes|no] value overrides the global setting.
(In reply to Beniamino Galvani from comment #14) > s/ithe opposite the behaviour/the opposite behaviour/ ? > Fixed, thanks. > > ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES > > Just a thought, would it make sense to write the property only for master > connections? > It might make sense. I have pushed a fixup commit. On the other hand, we will lose the property value when saved to ifcfg. Even if the value is only relevant for master, it might cause issues. For example, in the past there were problems in nmcli editor when comparing connections due to losing some properties via ifcfg plugin. But I hope it was fixed. Does anybody see any problems? > > cli: add support for connection.autoconnect-slaves property > > + nmc_add_prop_funcs (GLUE (CONNECTION, AUTOCONNECT_SLAVES), > + nmc_property_connection_get_autoconnect, > > This reads a different property, there should be a new function > nmc_property_connection_get_autoconnect_slaves(). > Good catch! Fixed. > I like Thomas' idea of having [yes|no|default] values for the > connection.autoconnect-slaves because IMHO it would be more intuitive > that an explicit [yes|no] value overrides the global setting. I will look into Thomas comments shortly.
I have added fixup commits to the branch changing connection.autoconnect-slaves property to a three-value variable. The global "main.master-autoconnects-slaves" is removed. The property can contain a default value that is read from config file as suggested in comment #11. If the approach is agreed, I am going to squash the branch so that it is streamlined.
In my opinion the approach is ok, I only have some comments below. > libnm: add autoconnect-slaves property to NMSettingConnection The new NMSettingConnection property must also be added to the settings array in libnm-core/tests/test-general.c:test_connection_diff_a_only() otherwise there are some failing tests. > ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES When the AUTOCONNECT_SLAVES variable is not present in a ifcfg file, I think that we should set the property value as DEFAULT instead of NO. + /* Only save the value for master connections */ Trailing whitespace ^ > core: activate slaves when master is activated (bgo #735052) (rh #1158529) Probably the manual page should be updated to say that connection.autoconnect-slaves is one of the properties supporting a default value.
All issues from comment #17 fixed, thank you. The branch is squashed. (You can see the previous version with -unsquashed suffix if you want).
>> ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES reader effectively always sets the autoconnect-slaves property to either YES or NO. That means, if you use ifcfg-rh, you cannot store the default value at all and the overwrite never happens. To support the differing default-values for bond/team vs. bridge, I would do: +typedef enum { + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_NO = -2, + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_YES = -1, + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_NO = 0, + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_YES = 1, +} NMSettingConnectionAutoconnectSlaves; + g_object_class_install_property + (object_class, PROP_AUTOCONNECT_SLAVES, + g_param_spec_enum (NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES, "", "", + NM_TYPE_SETTING_CONNECTION_AUTOCONNECT_SLAVES, + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_NO, + G_PARAM_READWRITE | + G_PARAM_CONSTRUCT | + NM_SETTING_PARAM_FUZZY_IGNORE | + G_PARAM_STATIC_STRINGS)); + if ( !strcmp (type, NM_SETTING_TEAM_SETTING_NAME) + || !strcmp (type, NM_SETTING_BOND_SETTING_NAME)) + autoconnect_slaves_default = NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_YES; else + autoconnect_slaves_default = NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_NO; + /* Check autoconnect-slaves property */ + autoconnect_slaves = nm_setting_connection_get_autoconnect_slaves (s_con); + if ( autoconnect_slaves != AUTOCONNECT_SLAVES_DEFAULTN_NO) + && autoconnect_slaves != AUTOCONNECT_SLAVES_DEFAULT_YES) + goto out; + if (autoconnect_slaves == NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_NO) + return FALSE; + if (autoconnect_slaves == NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_YES) + return TRUE; + if (autoconnect_slaves == NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_YES) + return TRUE; + return FALSE;
That(In reply to Thomas Haller from comment #19) > at all and the overwrite never happens. > > To support the differing default-values for bond/team vs. bridge, I would do: > > > +typedef enum { > + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_NO = -2, > + NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT_YES = -1, Hmm, that is ugly. I would rather keep things simple. It is unfortunate to have different default behaviour depending on the setting plugin and/or connection type. Maybe let's forget what initscripts do, and just set NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES_DEFAULT in ifcfg-rh if AUTOCONNECT_SLAVES is missing. We brought incompatibility with current versions anyway, and trying to make it compatible again only complicates the issue. It would be nice if bond/team/bridge behave the same way.
(In reply to Jiri Klimes from comment #20) > That(In reply to Thomas Haller from comment #19) > Hmm, that is ugly. I would rather keep things simple. Restoring initscripts behavior has some value, but you have a valid point with: > We brought incompatibility with current versions anyway, and trying to make > it compatible again only complicates the issue. Note that adding "DEFAULT_NO" and "DEFAULT_YES" is not a special hack only for initscripts. Instead it provides a more flexible configuration option. Namely, the user or setting plugins(!) can allow the property to be overwritten by default-settings, but fallback to "YES". Whether this is too much flexibility or unlikely to be useful to anybody, is a valid question... I'd guess you can do the change with some 30 lines of code. IMO worth the effort.
(In reply to Thomas Haller from comment #21) > > Note that adding "DEFAULT_NO" and "DEFAULT_YES" is not a special hack only > for initscripts. Instead it provides a more flexible configuration option. > Namely, the user or setting plugins(!) can allow the property to be > overwritten by default-settings, but fallback to "YES". > > Whether this is too much flexibility or unlikely to be useful to anybody, is > a valid question... > > I'd guess you can do the change with some 30 lines of code. IMO worth the > effort. The problem is not with lines of code, but with the fact that simple things are being configured with unnecessary complexity. I am not sure whether it brings any value for users. Anyway, I changed the original "ifcfg-rh" commit to just set DEFAULT when AUTOCONNECT_SLAVE is missing. That would handle all connections the same way. On top of that I added fixup commits implementing what Thomas suggests. Let's vote if we want it or not.
(In reply to Jiri Klimes from comment #22) > (In reply to Thomas Haller from comment #21) > > Whether this is too much flexibility or unlikely to be useful to anybody, is > > a valid question... > > > > I'd guess you can do the change with some 30 lines of code. IMO worth the > > effort. > > The problem is not with lines of code, but with the fact that simple things > are being configured with unnecessary complexity. I am not sure whether it > brings any value for users. Since NM never supported bringing up slaves automatically, I think that it wouldn't be unreasonable defaulting to NO for all device types; this seems also simpler and less confusing. So I'm for that approach, and the commits up to "man: mention that connection.autoconnect-slaves ..." now look good to me.
(In reply to Beniamino Galvani from comment #23) > (In reply to Jiri Klimes from comment #22) > > (In reply to Thomas Haller from comment #21) > > > Whether this is too much flexibility or unlikely to be useful to anybody, is > > > a valid question... > > > > > > I'd guess you can do the change with some 30 lines of code. IMO worth the > > > effort. > > > > The problem is not with lines of code, but with the fact that simple things > > are being configured with unnecessary complexity. I am not sure whether it > > brings any value for users. > > Since NM never supported bringing up slaves automatically, I think that it > wouldn't be unreasonable defaulting to NO for all device types; this seems > also simpler and less confusing. I don't want to argue about this is you all agree. But "Since NM never supported..." is not really the point. Initscripts did autoconnect slaves for bond and team. When NM is running, initscripts instead use nmcli to up the master. As NM didn't autoconnect slaves, this changes behavior ~for initscripts~, depending on whether NM is running or not. We could fix that now... but since we already changed behavior for rhel-7 in this regard, the fix would be a change in behavior again... :( Not to mention, that it would be a change in behavior ~for NM~. Ok, autoconnect always defaulting to "DEFAULT_NO" is fine with me.
I'm fine defaulting autoconnect-slaves to "no" as well, preserving current behavior. The branch looks OK to me, except for the DEFAULT_YES/DEFAULT_NO stuff that wouldn't be needed if we just do default to no.
Code commited to master: b18f328 merge: activate slaves when master is activated (bgo #735052) (rh #1158529) f05d41d man: mention that connection.autoconnect-slaves property supports default value f4582d8 core: activate slaves when master is activated (bgo #735052) (rh #1158529) c3093d9 cli: add support for connection.autoconnect-slaves property 2a497ee ifcfg-rh: read/write autoconnect-slaves property as AUTOCONNECT_SLAVES 6caafab libnm: add autoconnect-slaves property to NMSettingConnection The autoconnect-slaves property has yes/no/default, with defaulting to no. When users want a particular behaviour, they can set it up to their needs. The thing that bridge vs. bond slaves behaved differently in legacy initscripts was rather a bug there, but everyone lived with that.
Backported to to nm-1-0 too: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-0&id=b7b47941c00abf8158eef82a2116287796952956