[libteam] Support multiple member ports for LACP fallback #10823

lukasstockner · 2022-05-13T00:51:53Z

Why I did it

While the LACP fallback HLD correctly describes the behavior for LAGs with multiple ports, the actual implementation only worked for single-port LAGs.

When multiple ports are present, the fallback mode would select all ports, therefore making it useless for the purpose described in the HLD (PXE-booting).

How I did it

This commit adds a patch to implement the desired behavior:
If all non-disabled ports are defaulted, one port goes into fallback mode and gets selected for the LAG. As soon as any port receives a LACPDU, the fallback port goes back to defaulted.
The fallback port is selected by higher port priority or, if both have equal priority (the default), by lower port number (as described in the HLD).

How to verify it

See #7555 - start a DVS container, create a Portchannel with two members and fallback enabled, create a bond on the non-DVS end of the interfaces, bring it up and down and observe the state of the Portchannel.

Which release branch to backport (provide reason below if selected)

Not sure if this counts as a fix or feature, but I'd argue fix since this behavior was described in the HLD all along and the fallback is not particularly useful without it.

Description for the changelog

Support multiple member ports for LACP fallback.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

lukasstockner · 2023-04-22T10:33:55Z

@saiarcot895 @vaibhavhd based on recent PRs, you might be the right people to ask for a review here? Any help would be appreciated.

puffc · 2023-07-07T10:23:22Z

@rodnymolina @liat-grozovik Could you please review this PR?

puffc · 2023-07-27T10:12:19Z

@saiarcot895 Ping

saiarcot895 · 2023-08-09T01:10:11Z

src/libteam/patch/0015-Extend-LACP-fallback-support-to-multiple-ports.patch

+		}
+		if (lacp_port->state == PORT_STATE_CURRENT || lacp_port->state == PORT_STATE_EXPIRED) {
+			/* If at least one port is currently not defaulted, don't do any fallback. */
+			have_selected_port = true;


Could you rename this to something like have_active_port or similar? have_selected_port sounds like a port was selected in this loop.

@saiarcot895 Acutally "selected" is a terminology used in IEEE 802.3ad. When an aggregator port received LACPDU, the RX state machine will transit to "CURRENT" state and the selection logic update the state machine variable "selected" to "SELECTED", "UNSELECTED" or "STANDBY". See below excerpted from IEEE 802.3ad:
"
A value of SELECTED indicates that the Selection Logic has selected an appropriate
Aggregator. A value of UNSELECTED indicates that no Aggregator is currently selected. A
value of STANDBY indicates that although the Selection Logic has selected an appropriate
Aggregator, aggregation restrictions currently prevent the Aggregation Port from being enabled
as part of the aggregation, and so the Aggregation Port is being held in a standby condition.
This variable can only be set to SELECTED or STANDBY by the operation of the Aggregation
Port’s Selection Logic. It can be set to UNSELECTED by the operation of the Aggregation
Port’s Receive machine, or by the operation of the Selection Logic associated with another
Aggregation Port.
"

I'm not certain that selected is perfectly accurate here, so I've renamed this to do_fallback, which is a better description of the purpose anyways - it's supposed to track whether we want to use fallback logic or not.

saiarcot895 · 2023-08-09T01:11:43Z

Note that you'll need to resolve the merge conflicts as well.

puffc · 2023-08-09T03:42:39Z

src/libteam/patch/0015-Extend-LACP-fallback-support-to-multiple-ports.patch

+		}
+		if (lacp_port->state == PORT_STATE_CURRENT || lacp_port->state == PORT_STATE_EXPIRED) {
+			/* If at least one port is currently not defaulted, don't do any fallback. */
+			have_selected_port = true;


@saiarcot895 Acutally "selected" is a terminology used in IEEE 802.3ad. When an aggregator port received LACPDU, the RX state machine will transit to "CURRENT" state and the selection logic update the state machine variable "selected" to "SELECTED", "UNSELECTED" or "STANDBY". See below excerpted from IEEE 802.3ad:
"
A value of SELECTED indicates that the Selection Logic has selected an appropriate
Aggregator. A value of UNSELECTED indicates that no Aggregator is currently selected. A
value of STANDBY indicates that although the Selection Logic has selected an appropriate
Aggregator, aggregation restrictions currently prevent the Aggregation Port from being enabled
as part of the aggregation, and so the Aggregation Port is being held in a standby condition.
This variable can only be set to SELECTED or STANDBY by the operation of the Aggregation
Port’s Selection Logic. It can be set to UNSELECTED by the operation of the Aggregation
Port’s Receive machine, or by the operation of the Selection Logic associated with another
Aggregation Port.
"

puffc · 2023-08-09T04:11:22Z

src/libteam/patch/series

@@ -12,3 +12,4 @@
 0012-Increase-min_ports-upper-limit-to-1024.patch
 0013-set-port-to-disabled-state-during-removal.patch
 0014-dont-move-the-port-state-from-disabled-when-admin-state-is-down.patch
+0015-Extend-LACP-fallback-support-to-multiple-ports.patch


@lukasstockner Need your help to modify this line because 0015 had already been used.

@lukasstockner Ping

Rebased now, sorry for the delay.

Fixes sonic-net#7555 and sonic-net#8866. While the LACP fallback HLD correctly describes the behavior for LAGs with multiple ports, the actual implementation only worked for single-port LAGs. When multiple ports are present, the fallback mode would select all ports, therefore making it useless for the purpose described in the HLD (PXE-booting). Therefore, this commit adds a patch to implement the desired behavior: If all non-disabled ports are defaulted, *one* port goes into fallback mode and gets selected for the LAG. As soon as any port receives a LACPDU, the fallback port goes back to defaulted. The fallback port is selected by higher port priority or, if both have equal priority (the default), by lower port number (as described in the HLD). Signed-off-by: Lukas Stockner <[email protected]>

puffc · 2023-08-15T10:25:41Z

@saiarcot895 Please review this PR again. Thanks.

saiarcot895 · 2024-03-30T01:04:46Z

/azpw run Azure.sonic-buildimage

robertlperry · 2024-06-25T15:22:55Z

Hi @yxieca and @zhangyanzhao this is the PR I mention it today meeting. If it can be backported to release 202305, that would be great. If not, the earliest release available is fine. Thank you.

robertlperry · 2024-09-17T15:17:10Z

Hi @saiarcot895, is there anything needed to move forward with this PR. I can confirm this PR works and fixes the issue. Thanks.

lukasstockner requested a review from lguohan as a code owner May 13, 2022 00:51

lukasstockner force-pushed the lacp-fallback branch from 0e667bf to 2d5d46c Compare May 13, 2022 00:54

lukasstockner force-pushed the lacp-fallback branch from 2d5d46c to c027eb4 Compare November 10, 2022 15:27

lukasstockner force-pushed the lacp-fallback branch from c027eb4 to 8daab64 Compare April 22, 2023 10:30

saiarcot895 self-requested a review July 10, 2023 19:09

puffc approved these changes Jul 27, 2023

View reviewed changes

saiarcot895 reviewed Aug 9, 2023

View reviewed changes

puffc suggested changes Aug 9, 2023

View reviewed changes

lukasstockner force-pushed the lacp-fallback branch from 8daab64 to d2d2816 Compare August 14, 2023 23:33

grische approved these changes Dec 11, 2023

View reviewed changes

saiarcot895 mentioned this pull request Mar 30, 2024

[SWSS] Static LAG Support sonic-net/sonic-swss#3090

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libteam] Support multiple member ports for LACP fallback #10823

[libteam] Support multiple member ports for LACP fallback #10823

lukasstockner commented May 13, 2022

lukasstockner commented Apr 22, 2023

puffc commented Jul 7, 2023

puffc commented Jul 27, 2023

saiarcot895 Aug 9, 2023

puffc Aug 9, 2023

lukasstockner Aug 14, 2023

saiarcot895 commented Aug 9, 2023

puffc Aug 9, 2023

puffc Aug 9, 2023

puffc Aug 11, 2023

lukasstockner Aug 14, 2023

puffc commented Aug 15, 2023

saiarcot895 commented Mar 30, 2024

robertlperry commented Jun 25, 2024

robertlperry commented Sep 17, 2024

[libteam] Support multiple member ports for LACP fallback #10823

Are you sure you want to change the base?

[libteam] Support multiple member ports for LACP fallback #10823

Conversation

lukasstockner commented May 13, 2022

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

lukasstockner commented Apr 22, 2023

puffc commented Jul 7, 2023

puffc commented Jul 27, 2023

saiarcot895 Aug 9, 2023

Choose a reason for hiding this comment

puffc Aug 9, 2023

Choose a reason for hiding this comment

lukasstockner Aug 14, 2023

Choose a reason for hiding this comment

saiarcot895 commented Aug 9, 2023

puffc Aug 9, 2023

Choose a reason for hiding this comment

puffc Aug 9, 2023

Choose a reason for hiding this comment

puffc Aug 11, 2023

Choose a reason for hiding this comment

lukasstockner Aug 14, 2023

Choose a reason for hiding this comment

puffc commented Aug 15, 2023

saiarcot895 commented Mar 30, 2024

robertlperry commented Jun 25, 2024

robertlperry commented Sep 17, 2024