Re: [PATCH net v3 0/5] bonding: 3ad: fix carrier state with no valid slaves

[Jay Vosburgh <jv@jvosburgh.net>]

Louis Scalbert <louis.scalbert@6wind.com> wrote:

>Hi everyone,
>
>This series addresses a blackholing issue and a subsequent link-flapping
>issue in the 802.3ad bonding driver when dealing with inactive slaves
>and the `min_links` parameter.
>
>When an 802.3ad (LACP) bonding interface has no slaves in the
>collecting/distributing state, the bonding master still reports
>carrier as up as long as at least 'min_links' slaves have carrier.
>
>In this situation, only one slave is effectively used for TX/RX,
>while traffic received on other slaves is dropped. Upper-layer
>daemons therefore consider the interface operational, even though
>traffic may be blackholed if the lack of LACP negotiation means
>the partner is not ready to deal with traffic.
>
>The current behavior is not compliant with the LACP standard. This
>patchset introduces a working behavior that is not strictly
>standard-compliant either, but is widely adopted across the industry.
>It consists of bringing the bonding master interface down to signal to
>upper-layer processes that it is not usable.

	As I've said, I believe that the current behavior is compliant
with the standard, as IEEE 802.1AX-2014 6.1.1.j, as we've discussed,
says that (to summarize) links that cannot participate in Link
Aggregation ... operate as normal, individual links.  The mechanism by
which that takes place is not defined, and we, in my opinion, are not in
violation of the standard by selecting a bond member and making it
active.

>This patchset depends on the following iproute2 change:
>ip/bond: add lacp_fallback support
>
>Patch 1 introduces the lacp_fallback configuration knob, which is
>applied in the subsequent patch. The default (legacy) mode preserves
>the existing behavior, while the strict mode is intended to force the
>bonding master carrier down in this situation.

	The above notwithstanding, I don't object in general to an
option that says, essentially, "require that only LACP-partnered ports
be made active."

	Also, after reading through the patch set, I'm comfortable
calling the entire series a "fix," i.e., suitable for net vs net-next.
Most of the changes are genuine code fixes, and the addition of the
option is less a real feature and more of a change to better manage
connectivity in edge cases, so I'm fine with that being called a fix as
well.

	-J

>Patch 2 addresses the core issue when lacp_fallback is set to strict.
>It ensures that carrier is asserted only when at least 'min_links'
>slaves are in a valid state (collecting/distributing, or collecting
>only when coupled_control is disabled).
>
>Patch 3 fixes a side effect of the first patch. Tightening the carrier 
>logic exposes a state persistence bug: when a physical link goes down, 
>the LACP collecting/distributing flags remain set. When the link returns, 
>the interface briefly hallucinates that it is ready, bounces the carrier 
>up, and then drops it again once LACP renegotiation starts. Unsetting the 
>SELECTED flag when the link goes down forces the state machine through 
>DETACHED, clearing the stale flags and preventing the flap.
>
>Patch 4 fixes a side effect of the second patch caused by clearing the
>SELECTED flag on disabled ports. After all ports in an aggregator go
>down, if only a subset of ports comes back up, those ports can no
>longer renegotiate LACP unless all aggregator ports come back up.
>
>Patch 5 adds a test for the bonding legacy and strict LACP fallback modes.
>
>Louis Scalbert (5):
>  bonding: 3ad: add lacp_fallback configuration knob
>  bonding: 3ad: fix carrier when no valid slaves
>  bonding: 3ad: fix mux port state on oper down
>  bonding: 3ad: fix stuck negotiation on recovery
>  selftests: bonding: add test for fallback mode
>
> Documentation/networking/bonding.rst          |  33 ++
> drivers/net/bonding/bond_3ad.c                |  38 ++-
> drivers/net/bonding/bond_main.c               |   1 +
> drivers/net/bonding/bond_netlink.c            |  16 +
> drivers/net/bonding/bond_options.c            |  27 ++
> include/net/bond_options.h                    |   1 +
> include/net/bonding.h                         |   1 +
> include/uapi/linux/if_link.h                  |   1 +
> .../selftests/drivers/net/bonding/Makefile    |   1 +
> .../drivers/net/bonding/bond_lacp_fallback.sh | 299 ++++++++++++++++++
> 10 files changed, 415 insertions(+), 3 deletions(-)
> create mode 100755 tools/testing/selftests/drivers/net/bonding/bond_lacp_fallback.sh
>
>-- 
>2.39.2
>

---
	-Jay Vosburgh, jv@jvosburgh.net