Created attachment 1399678 [details]
All logs when both two slaves have dhcp IP

Created attachment 1399679 [details]
All logs when only one slave NIC has dhcp IP

This issue blocked to add the rhvh to engine
"""
Failed to configure management network on host dguo-intel. Host dguo-intel has an invalid bond interface (bond0 contains less than 2 active slaves) for the management network configuration.
"""
Test version:
rhvm-4.1.10.1-0.1.el7
rhvh-4.1-0.20180218.0(el7.5)

Using redhat-virtualization-host-4.1-20180218.0 is difficult for me.  Do you have a machine that I can log into?  (I guess I would also need a way to interact with the console, so that I can fix the network config after I break it.)

However, I think I can reproduce this on Fedora 27 and RHEL 7.5.  My current hunch is that a change in NetworkManager is responsible for the new behavior.

What is your version of NetworkManager?

What happens when you reboot?

Here is a reproducer with nmcli:

# nmcli dev con eth1
# nmcli con show "System eth1"
connection.id:                          System eth1
connection.uuid:                        9c92fad9-6ecb-3e6c-eb4d-8a47c6f50c04
connection.stable-id:                   --
connection.type:                        802-3-ethernet
connection.interface-name:              eth1
connection.autoconnect:                 no
connection.autoconnect-priority:        0
connection.autoconnect-retries:         -1 (default)
connection.auth-retries:                -1
connection.timestamp:                   1519826154
connection.read-only:                   no
connection.permissions:                 --
connection.zone:                        --
connection.master:                      --
connection.slave-type:                  --
connection.autoconnect-slaves:          -1 (default)
connection.secondaries:                 --
connection.gateway-ping-timeout:        0
connection.metered:                     unknown
connection.lldp:                        default
802-3-ethernet.port:                    --
802-3-ethernet.speed:                   0
802-3-ethernet.duplex:                  --
802-3-ethernet.auto-negotiate:          no
802-3-ethernet.mac-address:             --
802-3-ethernet.cloned-mac-address:      --
802-3-ethernet.generate-mac-address-mask:--
802-3-ethernet.mac-address-blacklist:   --
802-3-ethernet.mtu:                     auto
802-3-ethernet.s390-subchannels:        --
802-3-ethernet.s390-nettype:            --
802-3-ethernet.s390-options:            --
802-3-ethernet.wake-on-lan:             default
802-3-ethernet.wake-on-lan-password:    --
ipv4.method:                            disabled
ipv4.dns:                               --
ipv4.dns-search:                        --
ipv4.dns-options:                       ""
ipv4.dns-priority:                      0
ipv4.addresses:                         --
ipv4.gateway:                           --
ipv4.routes:                            --
ipv4.route-metric:                      -1
ipv4.route-table:                       0 (unspec)
ipv4.ignore-auto-routes:                no
ipv4.ignore-auto-dns:                   no
ipv4.dhcp-client-id:                    --
ipv4.dhcp-timeout:                      0 (default)
ipv4.dhcp-send-hostname:                yes
ipv4.dhcp-hostname:                     --
ipv4.dhcp-fqdn:                         --
ipv4.never-default:                     no
ipv4.may-fail:                          yes
ipv4.dad-timeout:                       -1 (default)
ipv6.method:                            ignore
ipv6.dns:                               --
ipv6.dns-search:                        --
ipv6.dns-options:                       ""
ipv6.dns-priority:                      0
ipv6.addresses:                         --
ipv6.gateway:                           --
ipv6.routes:                            --
ipv6.route-metric:                      -1
ipv6.route-table:                       0 (unspec)
ipv6.ignore-auto-routes:                no
ipv6.ignore-auto-dns:                   no
ipv6.never-default:                     no
ipv6.may-fail:                          yes
ipv6.ip6-privacy:                       -1 (unknown)
ipv6.addr-gen-mode:                     stable-privacy
ipv6.dhcp-send-hostname:                yes
ipv6.dhcp-hostname:                     --
ipv6.token:                             --
proxy.method:                           none
proxy.browser-only:                     no
proxy.pac-url:                          --
proxy.pac-script:                       --
GENERAL.NAME:                           System eth1
GENERAL.UUID:                           9c92fad9-6ecb-3e6c-eb4d-8a47c6f50c04
GENERAL.DEVICES:                        eth1
GENERAL.STATE:                          activated
GENERAL.DEFAULT:                        no
GENERAL.DEFAULT6:                       no
GENERAL.SPEC-OBJECT:                    --
GENERAL.VPN:                            no
GENERAL.DBUS-PATH:                      /org/freedesktop/NetworkManager/ActiveCo
GENERAL.CON-PATH:                       /org/freedesktop/NetworkManager/Settings
GENERAL.ZONE:                           --
GENERAL.MASTER-PATH:                    --
IP4.GATEWAY:                            --
IP6.ADDRESS[1]:                         fd00:111:112:0:5054:1ff:fe00:1/64
IP6.ADDRESS[2]:                         fe80::5054:1ff:fe00:1/64
IP6.GATEWAY:                            fe80::5054:ff:fee3:6eab
IP6.ROUTE[1]:                           dst = ff00::/8, nh = ::, mt = 256, table
IP6.ROUTE[2]:                           dst = fe80::/64, nh = ::, mt = 256
IP6.ROUTE[3]:                           dst = ::/0, nh = fe80::5054:ff:fee3:6eab
IP6.ROUTE[4]:                           dst = fd00:111:112::/64, nh = ::, mt = 2

# nmcli con mod "System eth1" slave-type bond master bond0 autoconnect yes
# nmcli con add type bond ifname bond0 con-name bond0 mode active-backup autoconnect yes connection.autoconnect-slaves 1

On RHEL 7.5 with NetworkManager-1.10.2-12.el7.x86_64, bond0 activates, but never gets an IP address.  eth1 is still active with the same IP4.* and IP6.* values as shown above.
Bringin bond0 up again explicitly will then enslave eth1.

# nmcli dev
DEVICE      TYPE      STATE                                  CONNECTION  
eth0        ethernet  connected                              System eth0 
eth1        ethernet  connected                              System eth1 
bond0       bond      connecting (getting IP configuration)  bond0       

# nmcli dev con bond0
# nmcli dev
DEVICE      TYPE      STATE      CONNECTION  
eth0        ethernet  connected  System eth0 
bond0       bond      connected  bond0       
eth1        ethernet  connected  System eth1 

On Rhel 7.4 with NetworkManager-1.8.0-11.el7_4.x86_64, bond0 activates fully and gets an IP address.  eth1 is immediately enslaved.

# nmcli dev
DEVICE      TYPE      STATE      CONNECTION  
bond0       bond      connected  bond0       
eth0        ethernet  connected  System eth0 
eth1        ethernet  connected  System eth1

(In reply to Marius Vollmer from comment #5)
> Using redhat-virtualization-host-4.1-20180218.0 is difficult for me.  Do you
> have a machine that I can log into?  (I guess I would also need a way to
> interact with the console, so that I can fix the network config after I
> break it.)
> 
> However, I think I can reproduce this on Fedora 27 and RHEL 7.5.  My current
> hunch is that a change in NetworkManager is responsible for the new behavior.
> 
> What is your version of NetworkManager?
> 
> What happens when you reboot?

1. This issue is also existed in redhat-virtualization-host-4.2-20180218.0, I have sent machine info to you via email, please review.

2. # rpm -qa | grep NetworkManager
NetworkManager-libnm-1.10.2-11.el7.x86_64
NetworkManager-team-1.10.2-11.el7.x86_64
NetworkManager-tui-1.10.2-11.el7.x86_64
NetworkManager-config-server-1.10.2-11.el7.noarch
NetworkManager-1.10.2-11.el7.x86_64

3. 
3.1 After create bond0 over eno1 and eno2, bond0 can not get dhcp ip. 
3.2 Then reboot machine, after reboot, the bond0 can get dhcp ip successfully. eno1 and eno2 released dhcp ip automatically. (It is the expected results)
3.3 Then delete bond0, eno1 and eno2 can get dhcp ip successful.
3.4 Create bond0 again over eno1 and eno2, the issue is back: bond0 can not get dhcp ip.

Just to keep a record: We have a test in Cockpit that is supposed to catch this regression.  It unfortunately didn't since it doesn't use DHCP anymore for the interface that is enslaved.

When making a bond, Cockpit will copy the ipv4 and ipv6 settings from the primary slave to the bond, the slave had a "manual" ipv4.method, and that was enough to give the bond the expected IP address, although it wasn't fully working, just as in the reproducer in comment 6.

I will fix the test so that it fails as expected.

In my testing, it didn't matter if the slave had the host connection or not.

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

Here is the pull request that adds a new test for this scenario to Cockpit:

    https://github.com/cockpit-project/cockpit/pull/8736

The new test fails on rhel-7-5 and passes on rhel-7-4, as expected.

Here is a pull request with a pull request with a potential workaround:

    https://github.com/cockpit-project/cockpit/pull/8769

This makes rhel-7-5 pass as well.  (But it might break other things, so this is not a slam dunk yet.)

> Here is a pull request with a pull request 

Upps. :)

(In reply to Marius Vollmer from comment #6)
> Here is a reproducer with nmcli:
>
> # nmcli dev con eth1
> # nmcli con show "System eth1"
...
> # nmcli con mod "System eth1" slave-type bond master bond0 autoconnect yes
> # nmcli con add type bond ifname bond0 con-name bond0 mode active-backup
> autoconnect yes connection.autoconnect-slaves 1
>
> On RHEL 7.5 with NetworkManager-1.10.2-12.el7.x86_64, bond0 activates, but
> never gets an IP address.  eth1 is still active with the same IP4.* and
> IP6.* values as shown above.
> Bringin bond0 up again explicitly will then enslave eth1.

I think this is the expected behavior: any change to an active connection
doesn't have any effect until the connection is reactivated... this is
true for all properties and in my opinion there is no reason why
'master' should behave differently (actually a couple of properties are
applied automatically - zone and metered - but they don't tear the
interface down). Reactivating the connection automatically when
something changes leads to race conditions; for example if you do
this while the connection is active:

 nmcli con mod "System eth1" slave-type bond master bond0
 nmcli con up "System eth1"

the second command would fail in some cases because the
auto-reconnection triggered by the first command would interrupt the
user activation.

I consider the behavior that was in NM 1.8 buggy, and that bug was
caught by CI and fixed as bz1450219.

What do you think? Probably we can try to revert the change to keep
backwards compatibility, but this would introduce again the race
condition.

(In reply to Beniamino Galvani from comment #13)

> Reactivating the connection automatically when
> something changes leads to race conditions; for example if you do
> this while the connection is active:
> 
>  nmcli con mod "System eth1" slave-type bond master bond0
>  nmcli con up "System eth1"
> 
> the second command would fail in some cases because the
> auto-reconnection triggered by the first command would interrupt the
> user activation.

I think this is not the issue here.  Cockpit does not assume that simply changing connection settings will re-activate a connection.  Cockpit does this explicitly (and it also knows that renaming an interface needs a explicit disconnection as well, for example).


However, for creating bonds Cockpit relies on these assumptions:

 - Creating a new bond connection with "autoconnect: yes" will immediately activate the new connection.

 - Activating a bond connection with "autoconnect-slaves: yes" will immediately (re-) activate all the slave connections, in an order determined by NM that ensures that the bond will receive the same IP address consistently, now and on every future activation.

Are these valid assumptions?  I am not sure if this is documented anywhere.  Is it?

Cockpit does especially and explicitly not want to have anything to do with the order that the slaves are activated for a new bond.


The change in NM behavior that we observe now is this:

 - Activating a bond connection with "autoconnect-slaves: yes" will immediately activate slave connections that were inactive initially.

Is this intended behavior?  Is this actually what is happening?  It sure looks like it to me, but I have been dramatically confused before. :-)


Activating only a subset of slaves leads to obvious problems.  For example, the bond and one of the not-reactivated slaves might have the same MAC and might thus get the same IP.  See bug 1547372.

(In reply to Marius Vollmer from comment #14)

> The change in NM behavior that we observe now is this:
> 
>  - Activating a bond connection with "autoconnect-slaves: yes" will
> immediately activate slave connections that were inactive initially.

I should have added: "but it does not reactivate slave connections that were initially active".

(In reply to Marius Vollmer from comment #15)
> (In reply to Marius Vollmer from comment #14)
> 
> > The change in NM behavior that we observe now is this:
> > 
> >  - Activating a bond connection with "autoconnect-slaves: yes" will
> > immediately activate slave connections that were inactive initially.
> 
> I should have added: "but it does not reactivate slave connections that were
> initially active".

Actually, it is:

  - Creating a new bond connection with "autoconnect: yes" and
    "autoconnect-slaves: yes" will immediately activate the new bond connection
    and slave connections that were initially inactive, but it does not
    reactivate slave connections that were initially active.

Explicitly activating a bond works as before, it just the behavior at creation time that has changed.  Which makes it even more inconsistent, no?

(Sorry for taking so many tries to get the details right.)

I think it's like this:


if you activate a master (bond) connection with connection.autoconnect-slaves=yes, it will activate all matching slaves profiles that:
  (1) are not currently active otherwise
  (2) that find a device that is not currently activated with another connection.

Note that issuing `nmcli connection up "$BOND_MASTER"` for a master which is currently active, means to first bring the master down, which also means to bring all its slaves down. (3) a slave connection cannot be active without a master.


In the example from comment 6, "System eth1" is already active, but not in the function of being a slave connection. Because at the point when "System eth1" was activate, it was not yet a slave. Because "System eth1" is active, it cannot get autoactivated by the master (1).
This seemingly contradicts (3), because "System eth1" is active without master. Indeed the profile itself is in the meantime a slave connection. But the configuration that is activated on the device is still not a slave-connection as long as it stays up.



>  - Activating a bond connection with "autoconnect-slaves: yes" will 
> immediately (re-) activate all the slave connections, in an order determined
> by NM that ensures that the bond will receive the same IP address 
> consistently, now and on every future activation.

Yes, NetworkManager tries to autoactivate slaves in an order that leads to a predictable MAC address. But that only applies to profiles that can currently be activated (that is, satisfy (1) and (2)).


> Are these valid assumptions?  I am not sure if this is documented anywhere.  
> Is it?

I don't think it's documented explicitly. The behavior ultimately should make sense, and what I said above makes sense to me, doesn't it? At least, if you consider that changing a profile, will not take effects immediately. So, making a non-slave profile a slave, doesn't have immediate effects on the activated device. What we do have is Tests for such behavior (or at least, we have many tests, and if this particular scenario is not covered, we should have a test).

(In reply to Thomas Haller from comment #17)
> I think it's like this:
> 
> if you activate a master (bond) connection with
> connection.autoconnect-slaves=yes, it will activate all matching slaves
> profiles that:
>   (1) are not currently active otherwise
>   (2) that find a device that is not currently activated with another
> connection.

The NM versions in RHEL 7.4 and 7.5 behave differently.  The one in 7.4 seems to activate all slaves unconditionally, while the one in 7.5 only activates the ones that satisfy (1) and (2).

This is the "regression" we are looking at.  Can you confirm that there was a change in behavior?

> Yes, NetworkManager tries to autoactivate slaves in an order that leads to a
> predictable MAC address. But that only applies to profiles that can
> currently be activated (that is, satisfy (1) and (2)).

If you take only a subset of the slaves, you don't get a predictable MAC address.

(In reply to Marius Vollmer from comment #18)
> (In reply to Thomas Haller from comment #17)
> > I think it's like this:
> > 
> > if you activate a master (bond) connection with
> > connection.autoconnect-slaves=yes, it will activate all matching slaves
> > profiles that:
> >   (1) are not currently active otherwise
> >   (2) that find a device that is not currently activated with another
> > connection.
> 
> The NM versions in RHEL 7.4 and 7.5 behave differently.  The one in 7.4
> seems to activate all slaves unconditionally, while the one in 7.5 only
> activates the ones that satisfy (1) and (2).
> 
> This is the "regression" we are looking at.  Can you confirm that there was
> a change in behavior?

Yes, there was a change in behavior. We ensured that a connection started by user is never preempted by an internal activation. This is necessary to avoid race conditions as explained in bug 1450219. Probably we can restore the old behavior of always reconnecting slaves even if they are already active with a different master; I'll attach the patch.

Created attachment 1404969 [details]
[PATCH] manager: allow internal reactivation if the master changed

(In reply to Beniamino Galvani from comment #19)
> (In reply to Marius Vollmer from comment #18)
>
> > This is the "regression" we are looking at.  Can you confirm that there was
> > a change in behavior?
> 
> Yes, there was a change in behavior.

Right, thanks for the confirmation!  Can you say which version of NM first had the new behavior?

From looking at our tests, it seems to have changed somewhere between 1.8.0 and 1.8.2.  Does that make sense?

See: 

https://bugzilla.redhat.com/show_bug.cgi?id=1450219#c7

 $ git tag --contains 2236c3c728c49d2ebd68e83f1096b5180b2f41dd
 1.8.2
 1.8.3-dev
 1.8.4
 1.8.5-dev
 1.8.6
 1.8.7-dev

Yes, 1.8.2 is the first affected version.

(In reply to Beniamino Galvani from comment #20)
> Created attachment 1404969 [details]
> [PATCH] manager: allow internal reactivation if the master changed

I am not convinced about this. What I described in comment 17 is how it should be and how it makes sense (to me) in the larger scheme. It is at least consistent, and I didn't need to explain parts like "except, if the profile of an activated connection changed in the meantime to become a slave, then instead...".
This patch is a hack to behave differently in a specific situation.

Here it is called a "regression". But I'd call the old behavior a bug that was fixed.

Yes, there is something like bug compatiblity, and NetworkManager should try hard not to break existing applications and setups. But is this really so important to stick to the broken behavior? If at all, I'd rather only apply this patch downstream for RHEL-7.5 (because 1.10+ and 1.8.2+ was already released with IMHO the better behavior. So reverting it back to fix a "regression" compared to 1.8.0 only changes behavior again.


Why is this any different from other cases where you modify a connection? If you modify a connection's interface-name, the changes do not take effect right away (as Marius said in comment 14). Making a non-slave profile a slave, is also a large and fundamental change to the profile. Why should that be treated special?

Here is a test script to reproduce the symptoms of bug 1547372.  You need the whole Cockpit CI infrastructure to actually run it, or you can look here:

    https://github.com/cockpit-project/cockpit/pull/8776

The script:

    def testReproducer(self):
        m = self.machine

        print(m.execute("rpmquery NetworkManager"))

        iface1 = self.add_iface()
        con1 = self.iface_con_id(iface1)
        mac1 = m.execute("cat /sys/class/net/%s/address" % iface1).strip()

        iface2 = self.add_iface()
        con2 = self.iface_con_id(iface2)

        m.execute("nmcli con up %s" % con1)
        m.execute("nmcli con down %s" % con2)

        m.execute("nmcli con mod '%s' slave-type bond master bond0 autoconnect yes" % con1)
        m.execute("nmcli con mod '%s' slave-type bond master bond0 autoconnect yes" % con2)
        m.execute("nmcli con add type bond ifname bond0 con-name bond0 mode active-backup autoconnect yes connection.autoconnect-slaves 1 ethernet.cloned-mac-address %s" % mac1)

    print(m.execute("sleep 3; ip addr"))

By looking at the output of "ip addr", you can see that with recent versions of NM, iface1 does not become a slave of bond0, and both iface1 and bond0 have the same IP address.

Summary of the test results:

centos-7: works, NetworkManager-1.8.0-11.el7_4.x86_64
rhel-7-4: works, NetworkManager-1.8.0-11.el7_4.x86_64
rhel-7: works, NetworkManager-1.8.0-11.el7_4.x86_64
rhel-atomic: works, NetworkManager-1.8.0-11.el7_4.x86_64

fedora-26: broken, NetworkManager-1.8.2-4.fc26.x86_64
fedora-atomic: broken, NetworkManager-1.8.6-1.fc27.x86_64
rhel-7-5: broken, NetworkManager-1.10.2-13.el7.x86_64
fedora-27: broken, NetworkManager-1.8.6-1.fc27.x86_64
fedora-i386: broken, NetworkManager-1.8.6-1.fc27.i686

(In reply to Beniamino Galvani from comment #22)

> Yes, 1.8.2 is the first affected version.

Nice, I like it when all the facts match up. :-)

(In reply to Thomas Haller from comment #23)
> (In reply to Beniamino Galvani from comment #20)
> > Created attachment 1404969 [details]
> > [PATCH] manager: allow internal reactivation if the master changed
> 
> I am not convinced about this. What I described in comment 17 is how it
> should be and how it makes sense (to me) in the larger scheme. It is at
> least consistent, and I didn't need to explain parts like "except, if the
> profile of an activated connection changed in the meantime to become a
> slave, then instead...".
> This patch is a hack to behave differently in a specific situation.

The patch just achieves the goal of guaranteeing that when you activate a master connection with autoconnect-slaves=1 all slaves gets activated. This is simple enough to explain (in my opinion) and makes sense too. Moreover, it is what NM did until 1.8, when we inadvertently changed it. The implementation perhaps look like a hack, but the behavior certainly seems reasonable (and in fact users were relying on it).

> Here it is called a "regression". But I'd call the old behavior a bug that
> was fixed.

We have two requirements:

 (1) don't autoactivate connections that are already activated by user
 (2) activate all slaves when the master connects

Depending on whether we consider both real requirements and depending on their priorities, the old behavior can be seen either as a bug or as a feature.

> Yes, there is something like bug compatiblity, and NetworkManager should try
> hard not to break existing applications and setups. But is this really so
> important to stick to the broken behavior?

If Cockpit is fine in implementing a workaround for 1.10, I am fine with leaving things as they currently are, to avoid changing behavior again.

> If at all, I'd rather only apply
> this patch downstream for RHEL-7.5 (because 1.10+ and 1.8.2+ was already
> released with IMHO the better behavior. So reverting it back to fix a
> "regression" compared to 1.8.0 only changes behavior again.
> 
> 
> Why is this any different from other cases where you modify a connection? If
> you modify a connection's interface-name, the changes do not take effect
> right away (as Marius said in comment 14). Making a non-slave profile a
> slave, is also a large and fundamental change to the profile. Why should
> that be treated special?

As explained above, this is needed to honor the autoconnect-slaves priority. When the master gets activated there can be two cases:

 (1) the master property of the slave connection wasn't changed: then, since the master is down, the slave is too and we can just activate it
 (2) the master property was changed: then, the slave can be active with the (new) master down. In this case the patch is needed

(In reply to Thomas Haller from comment #23)

> What I described in comment 17 is how it
> should be and how it makes sense (to me) in the larger scheme.

Activating only a subset of the slaves when activating a bond does not make sense to me.  "nmcli con up bond0" doesn't do it.  It jeopardizes the predictable MAC guarantee, and causes the symptoms that people report here as blocker bugs.

NM spontaneously brings the system into a nonsensical run-time state and leaves it there forever, although the persistent configuration is sane.

> Why is this any different from other cases where you modify a connection?

This is about what happens when a newly created bond connections is activated automatically by NM.  (You call that "internal activation", right?)

It would be fine if NM wouldn't automatically activate new bond connections, but it does. And since it does, it should do it fully, as if the user had explicitly activated it.

(In reply to Thomas Haller from comment #23)

> I am not convinced about this. What I described in comment 17 is how it
> should be and how it makes sense (to me) in the larger scheme. It is at
> least consistent,

IMHO it's not. It leads to bringing up a bond with only parts of its slaves, which can (and does) easily cause IP conflicts and such. IMHO consistent behaviours for the case that a newly defined bond has currently active slaves are to either (1) don't bring up the new bond at all, or (2) forcefully tear down the current interfaces and enslave them. In either case the system is in a consistent state afterwards.

However, (2) bears the possibility of severing the ssh (or cockpit or whatever) link that you are currently using to administer your machine, so I understand it makes sense to be cautious and never automatically tear down active devices/connections. I suppose something like that was the reason for the behaviour change?

Thus it seems to me that (1) might be the solution which is both safe and consistent - i. e. if a newly created bond/bridge/etc. defines slaves that are active, don't bring it up at all automatically. An explicit request to bring them up should then tear down existing active slaves, of course.

(In reply to Martin Pitt from comment #28)

> However, (2) bears the possibility of severing the ssh (or cockpit or
> whatever) link that you are currently using to administer your machine, so I
> understand it makes sense to be cautious and never automatically tear down
> active devices/connections. I suppose something like that was the reason for
> the behaviour change?

NetworkManager has checkpoints to cope with this.

We did some work some time ago to get the predictable MAC guarantee, and Cockpit takes also other measures that make it very likely that enslaving an active interface into a bond will "seamlessly" give the bond the same IP addresses as the interface had before, and no TCP connections will be severed.  (And the bond will also get that same IP on each future activation of the bond.)

Here is the PR again with the workaround that we consider putting into Cockpit:

    https://github.com/cockpit-project/cockpit/pull/8769

I did some work on it so that it breaks less of our existing tests, and I plan to get it ready to be merged.

> The patch just achieves the goal of guaranteeing that when you activate a
> master connection with autoconnect-slaves=1 all slaves gets activated. This is 
> simple enough to explain (in my opinion) and makes sense too. Moreover, it is 
> what NM did until 1.8, when we inadvertently changed it. The implementation 
> perhaps look like a hack, but the behavior certainly seems reasonable (and in 
> fact users were relying on it).

This is a different interpretation of connnection.autoconnect-slaves, and `man nm-settings` isn't specific about what is supposed to happen.

connection.autoconnect=yes obviously only has effect for connections that are currently not active. I interpreted connection.autoconnect-slaves=yes the same. See comment 17. You now say, that connection.autoconnect-slaves should always forcefully activate all slaves. I see that that can make some sense, but it also seems dangerous, for example:

  - have non-slave profile "eth1" active.
  - have slave-profile "bond0-eth1" slave inactive
  - activate a master profile "bond0", with connection.autoconnect-slaves=yes
    (either, explicitly via `nmcli con up`, or maybe just create a new "bond0"
    profile and let the master profile autoconnect.

With the new interpretation, this would forcefully activate bond0-eth1.



I don't agree with the notion that a full master-slave setup requires *all* slaves to connect. If the slave is already busy with another profile (in the reproducer case, it's actually the same profile, but with an out-of-date non-slave configuration), then does the user really always want to steal the slave?


I think there is a point for both. If cockpit wants this forceful behavior, shouldn't it instead make sure that the slave candidates are disconnected first?


(ps, the nm-settings man-page should be fixed to clarify on the autoconnect-slaves behavior).



Ok, I don't disagree with the interpretion of autoconnect-slaves, I just didn't thought it should work that way. If everybody thinks that's better...

Our pull request with a possible workaround in Cockpit is (pretty much) ready to be merged:

    https://github.com/cockpit-project/cockpit/pull/8769

So I am confident that we have a working workaround.

What should we do with that PR? :-)  Merge?

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

Bridge also has the same issue, see Bug 1554628.

proposed CI tests: https://github.com/NetworkManager/NetworkManager-ci/pull/158

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

Beniamino - 

Is this going to make 7.5? I see a scratch build, but the flags are 7.6

It's too late for inclusion in 7.5 and the patch from comment 20 needs improvements. We can target the first z-stream.

Pushed a possible fix to branch bg/autoconnect-slaves-rh1548265.

"No such issue in rhel-7.4." ==> marking as a regression.

(In reply to Beniamino Galvani from comment #40)
> Pushed a possible fix to branch bg/autoconnect-slaves-rh1548265.

> Don't make it a
> construct property that must be always specified, so that we don't
> require every callers to set a reason even when it's not necessary.

can we make it a construct-property, and require the caller to set the correct reason? Note how currently reasons "AUTOCONNECT" and "USER_REQUEST" are unused. I think they should be used, and the caller should be forced to provide the correct reason.

(In reply to Thomas Haller from comment #43)
> (In reply to Beniamino Galvani from comment #40)
> > Pushed a possible fix to branch bg/autoconnect-slaves-rh1548265.
> 
> > Don't make it a
> > construct property that must be always specified, so that we don't
> > require every callers to set a reason even when it's not necessary.
> 
> can we make it a construct-property, and require the caller to set the
> correct reason? Note how currently reasons "AUTOCONNECT" and "USER_REQUEST"
> are unused. I think they should be used, and the caller should be forced to
> provide the correct reason.

Updated branch bg/autoconnect-slaves-rh1548265.

     if (   success
-        && nm_auth_subject_is_internal (nm_active_connection_get_subject (active))) {
+        && nm_auth_subject_is_internal (nm_active_connection_get_subject (active))
+        && nm_active_connection_get_activation_reason (active) != NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES) {


shouldn't it be 
  get_activation_reason(active) == REASON_AUTOCONNECT
?

Arguably, the effect is the same, because the only other reason is REASON_USER, for which !is_internal()


Pushed a fixup, rest lgtm

(In reply to Thomas Haller from comment #45)
>      if (   success
> -        && nm_auth_subject_is_internal (nm_active_connection_get_subject
> (active))) {
> +        && nm_auth_subject_is_internal (nm_active_connection_get_subject
> (active))
> +        && nm_active_connection_get_activation_reason (active) !=
> NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES) {
> 
> 
> shouldn't it be 
>   get_activation_reason(active) == REASON_AUTOCONNECT
> ?
> 
> Arguably, the effect is the same, because the only other reason is
> REASON_USER, for which !is_internal()

Yes, it is equivalent.

Branch merged upstream to master:

  https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=7a063a91c7f8db8f78cab124bf9ba80a242be233

and nm-1-10 branch:

  https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-10&id=58df222cbb02281268469b122eb430efaf14fac6

(In reply to Beniamino Galvani from comment #49)
> Branch merged upstream to master:
> 
>  
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/
> ?id=7a063a91c7f8db8f78cab124bf9ba80a242be233
> 
> and nm-1-10 branch:
> 
>  
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=nm-1-
> 10&id=58df222cbb02281268469b122eb430efaf14fac6

should probably also include https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=786adf969c297fc2e830b43cc9e9b1a154d43216

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://access.redhat.com/errata/RHBA-2018:3207