Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[teammgrd]: Use port-channel id as LACP key so that multiple port-channels from the peer can be distinguished #1117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sandeep-kulambi
Copy link

What I did
When a member port is added to port-channel, ensure the lacp-key is updated by using the Port-channel id value as the key value.

Why I did it
If LACP key is not set, then the peer will not be able to distinguish the ports which are connected to different port-channels, as it will receive the actor key (lacp key) as 0 for all the ports from different port-channels.

Sonic== 2links ===3rd party device
On Sonic 2 POs are created.:
Link1 on Sonic is configured on PO1
Link2 on Sonic is configured on PO2.
On 3rd party device single PO is created and both links are added to same PO.
All the links are in sync on both POs on Sonic and also in PO on 3rd party device.

How I verified it
Configure the devices as mentioned above and see on the 3rd party device only one of the port comes up in the PO and the other one stays down.

@prsunny
Copy link
Collaborator

prsunny commented Nov 6, 2019

In the current use-case which I'm aware, both sides are configured as two different port-channels. In your example, why is one side configured as single PC? Is this any specific use-case like MLAG or something?

@sandeep-kulambi
Copy link
Author

sandeep-kulambi commented Nov 6, 2019 via email

}
PoId = lag.substr(found+1);
/* Ignore all 0's in the Port-channel id*/
PoId2 = PoId.erase(0, min(PoId.find_first_not_of('0'), PoId.size()-1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part of logic can be improved, it should try to find the first digit, then use stol to get an integert and use it later.

@lguohan
Copy link
Contributor

lguohan commented Jan 29, 2020

retest this please

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach will not work. Because lacp key_id is 16-bit number. What if someone creates PortChannel with id greater than 65535?
Also, What if we have PortChannel01 and PortChannel1? It is the same key id.

I suggest to introduce an additional field to config db for LAG: key_id.
It allows us to put desired key_id in the configuration.
Another approach would be to restrict PortChannel format to have a proper format.

@lmcerbo-ka
Copy link

lmcerbo-ka commented Apr 15, 2020

This approach will not work. Because lacp key_id is 16-bit number. What if someone creates PortChannel with id greater than 65535?
Also, What if we have PortChannel01 and PortChannel1? It is the same key id.

I suggest to introduce an additional field to config db for LAG: key_id.
It allows us to put desired key_id in the configuration.
Another approach would be to restrict PortChannel format to have a proper format.

We are testing LAG in our lab as well with SONiC and having a configurable "key_id" as suggested by pavel-shirshov would be more "future" proof (IMHO).

Also, Key=0 should not be allowed since from IEEE-8021ax, paragraph 6.3.5 Capability identification:

An administrative Key value and an operational Key value shall also be associated with each Aggregator. The operational Key is the Key that is currently in active use for the purposes of forming aggregations.
The administrative Key allows manipulation of Key values by management.
The values of administrative and operational Key for an Aggregator may differ in the same manner as that of Aggregation Port Keys, per item e) and item f) in this subclause.

Aggregation Ports that are members of a given Key Group can only be bound
to Aggregators that share the same operational Key value.

All Keys are 16-bit identifiers.
All values except the null value (all zeros) are available for local use.

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
Upgrade submodule SAI head to latest commit 566d4a8

Signed-off-by: richardyu-ms <richard.yu@microsoft.com>

Signed-off-by: richardyu-ms <richard.yu@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants