Message-ID: <15524.1682698000@famine>
Date:   Fri, 28 Apr 2023 09:06:40 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Hangbin Liu <liuhangbin@...il.com>
cc:     netdev@...r.kernel.org
Subject: Re: [Issue] Bonding can't show correct speed if lower interface is bond 802.3ad

Hangbin Liu <liuhangbin@...il.com> wrote:

>A user reported a bonding issue that if we put an active-back bond on top of a
>802.3ad bond interface. When the 802.3ad bond's speed/duplex changed
>dynamically. The upper bonding interface's speed/duplex can't be changed at
>the same time.
>
>This seems not easy to fix since we update the speed/duplex only
>when there is a failover(except 802.3ad mode) or slave netdev change.
>But the lower bonding interface doesn't trigger netdev change when the speed
>changed as ethtool get bonding speed via bond_ethtool_get_link_ksettings(),
>which not affect bonding interface itself.

	Well, this gets back into the intermittent discussion on whether
or not being able to nest bonds is useful or not, and thus whether it
should be allowed or not.  It's at best a niche use case (I don't recall
the example configurations ever being anything other than 802.3ad under
active-backup), and was broken for a number of years without much
uproar.

	In this particular case, nesting two LACP (802.3ad) bonds inside
an active-backup bond provides no functional benefit as far as I'm aware
(maybe gratuitous ARP?), as 802.3ad mode will correctly handle switching
between multiple aggregators.  The "ad_select" option provides a few
choices on the criteria for choosing the active aggregator.

	Is there a reason the user in your case doesn't use 802.3ad mode
directly?

>Here is a reproducer:
>
>```
>#!/bin/bash
>s_ns="s"
>c_ns="c"
>
>ip netns del ${c_ns} &> /dev/null
>ip netns del ${s_ns} &> /dev/null
>sleep 1
>ip netns add ${c_ns}
>ip netns add ${s_ns}
>
>ip -n ${c_ns} link add bond0 type bond mode 802.3ad miimon 100
>ip -n ${s_ns} link add bond0 type bond mode 802.3ad miimon 100
>ip -n ${s_ns} link add bond1 type bond mode active-backup miimon 100
>
>for i in $(seq 0 2); do
>        ip -n ${c_ns} link add eth${i} type veth peer name eth${i} netns ${s_ns}
>        [ $i -eq 2 ] && break
>        ip -n ${c_ns} link set eth${i} master bond0
>        ip -n ${s_ns} link set eth${i} master bond0
>done
>
>ip -n ${c_ns} link set eth2 up
>ip -n ${c_ns} link set bond0 up
>
>ip -n ${s_ns} link set bond0 master bond1
>ip -n ${s_ns} link set bond1 up
>
>sleep 5
>
>ip netns exec ${s_ns} ethtool bond0 | grep Speed
>ip netns exec ${s_ns} ethtool bond1 | grep Speed
>```
>
>When run the reproducer directly, you will see:
># ./bond_topo_lacp.sh
>        Speed: 20000Mb/s
>        Speed: 10000Mb/s
>
>So do you have any thoughts about how to fix it?

	Maybe it's time to disable nesting of bonds, update the
documentation to note that it's disabled and that 802.3ad mode is smart
enough to do multiple aggregators, and then see if anyone has some other
use case and complains.

	In the past, I've been against doing this, but only because it
might break existing configurations.  If nested configurations are going
to misbehave and require complicated shenanigans to fix, then perhaps
it's time to push users into a configuration that works without the
nesting.

	The only thing I can think of that active-backup over 802.3ad
gets is the gratuitous ARP / NS on failover.  If that's the key feature
for nesting, then I'd rather add the grat ARP to 802.3ad aggregator
selection and disable nesting.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com

Powered by blists - more mailing lists