Mclag enhacements support code changes. #1331

Praveen-Brcm · 2020-06-24T15:38:38Z

What I did
This PR contains the code changes to support MCLAG enhancements feature #MCLAG Enhancements HLD SONiC#596
Why I did it
Implemented as per the MCLAG enhancements HLD mentioned at : MCLAG Enhancements HLD SONiC#596

How I verified it
Mclag specific VS tests included part of check-in.

Details:
The PR includes the code changes for the MCLAG enhancements support.
This PR must work with submitted PR's in other sonic repositories which are listed below.
> Iccpd: sonic-net/sonic-buildimage#4819
> swss-common: sonic-net/sonic-swss-common#405
> Mclagsyncd: #1349
> Utilities: sonic-net/sonic-utilities#1138
> Mgmt-FW: sonic-net/sonic-mgmt-framework#59
> Mgmt-Common: sonic-net/sonic-mgmt-common#25

chenkelly · 2020-07-14T04:12:14Z

Hi @Praveen-Brcm
The unique IP enhancements mentioned in this document don't include in the PR.
Do you have any plan for the feature? Is it under development? Thanks very much.

Praveen-Brcm · 2020-07-18T00:23:23Z

@chenkelly

Hi @Praveen-Brcm
The unique IP enhancements mentioned in this document don't include in the PR.
Do you have any plan for the feature? Is it under development? Thanks very much.

Thanks kelly for checking. Yes, we have the plan and the changes will be added soon.

chenkelly · 2020-07-20T00:42:58Z

Hi @Praveen-Brcm
Thanks. Please help to confirm the issue.
When we check ICCPd modification in sonic-net/sonic-buildimage#4819, can see unique IP message handle and call "iccp_mclagsyncd_mclag_unique_ip_cfg_handler" in https://github.com/Praveen-Brcm/sonic-buildimage/blob/mclag_enhacements/src/iccpd/src/mlacp_link_handler.c#L3467, but message type and function don't be found.
`
else if (msg_hdr->type == MCLAG_SYNCD_MSG_TYPE_CFG_MCLAG_UNIQUE_IP)

    {

        iccp_mclagsyncd_mclag_unique_ip_cfg_handler(sys, &msg_buf[pos]);

    }`

rathnasabapathyv · 2020-11-11T01:55:40Z

Can you please resolve conflicts

gechiang · 2020-11-11T06:30:38Z

@Praveen-Brcm Wonder if we can arrange some time for you to walk through the design/changes together?
Thanks!

Praveen-Brcm · 2020-11-16T17:35:46Z

@Praveen-Brcm Wonder if we can arrange some time for you to walk through the design/changes together?
Thanks!

@gechiang : Sure we can have a call to go over the design(captured in HLD)/code changes. please suggest your convenient time this week .? Thanks.

gechiang · 2020-11-16T18:05:38Z

@Praveen-Brcm Thanks! my calendar is open from 2 to 5 PM on all workdays except Tuesday. Let me know!
Also, You may want to first resolve the merge conflict and kick off the required tests that failed...

rlhui · 2020-11-16T18:12:45Z

retest this please

Praveen-Brcm · 2020-11-16T18:19:18Z

@Praveen-Brcm Thanks! my calendar is open from 2 to 5 PM on all workdays except Tuesday. Let me know!
Also, You may want to first resolve the merge conflict and kick off the required tests that failed...
@gechiang Thank You. will schedule at 3 PM on Wednesday 11/18/2020 .
Sure, I will be working on the conflicts resolution.

rlhui · 2020-11-16T18:23:28Z

@Praveen-Brcm - please add vs test cases for mlag. See https://github.com/Azure/sonic-swss/tree/master/tests

Praveen-Brcm · 2020-11-16T18:29:01Z

@Praveen-Brcm - please add vs test cases for mlag. See https://github.com/Azure/sonic-swss/tree/master/tests

@rlhui Thank you for the input.
Test files test_mclag.py and test_mclag_fdb.py are already part of PR.

gechiang · 2020-11-16T19:02:06Z

@Praveen-Brcm Thanks! my calendar is open from 2 to 5 PM on all workdays except Tuesday. Let me know!
Also, You may want to first resolve the merge conflict and kick off the required tests that failed...
@gechiang Thank You. will schedule at 3 PM on Wednesday 11/18/2020 .
Sure, I will be working on the conflicts resolution.

@Praveen-Brcm 3PM 11/18/2020 works for me. Thanks!

pettershao-ragilenetworks · 2021-07-30T07:17:34Z

orchagent/fdborch.cpp

-        {
-            observer->update(SUBJECT_TYPE_FDB_FLUSH_CHANGE, &flushUpdate);
-        }
+        notify(SUBJECT_TYPE_FDB_CHANGE, &flushUpdate);


SUBJECT_TYPE_FDB_FLUSH_CHANGE?

@pettershao-ragilenetworks: Thanks for the comment. Corrected the type.

pettershao-ragilenetworks · 2021-07-30T07:58:51Z

orchagent/fdborch.cpp

+    {
+        //If the MAC is dynamic_local change the origin accordingly
+        //MAC is added/updated as dynamic to allow aging.
+        storeFdbData.origin = FDB_ORIGIN_LEARN;


when modify to FDB_ORIGIN_LEARN, removeFdbEntry() 's condition (fdbData.origin != origin) will always be true, remove remote entry will directly return, this is not expective, right?

@pettershao-ragilenetworks: Thanks for the comment. Once the MAC is converted to Local, Remote FDB will not be expected from ICCPD and function remoteFdbEntry will not get called. MAC is supposed to be aged out via the event SAI_FDB_EVENT_AGED. Hope this clarifies.

but from my testing, removeFdbEntry will be called, and remote fdb entry will remove failed:

Jul 30 07:30:15.847477 sonic INFO swss#orchagent: :- removeFdbEntry: 1633: FdbOrch RemoveFDBEntry: mac=00:10:94:00:00:33 fdb origin is different; found_origin:1 delete_origin:8

@pettershao-ragilenetworks: Since the MAC is updated as local MAC, the origin is set to LOCAL. When the MAC moves from remove to local, ICCPD further updates the MCLAG_FDB_TABLE to delete the MAC entry added earlier. In the case of remove FDB called with different origin than existing origin is always ignored. This is TRUE for any MAC type L2, VxLAN, or MCLAG.

so fdb entry syncd from remote peer will not be remoted immediately when remote peer do fdb flush action, right?

so, when remote fdb added, and remote fdb deleted(remote peer flush fdb maybe), local peer will not remove the remote fdb entry by event but by age event, right?

remove

so fdb entry syncd from remote peer will not be remoted immediately when remote peer do fdb flush action, right?
@pettershao-ragilenetworks: Yes, If the ICCPD adds the MAC as local ( in certain scenarios like Iccpd session down..etc) the MAC entry is as good as Local HW learned. Remote peer flush will not cause delete , it can only be deleted by Local AGE or FLUSH events.

but the remote fdb is programed as STATIC, AGE or FLUSH will not remove them, right?

pettershao-ragilenetworks · 2021-07-30T08:51:56Z

orchagent/fdborch.cpp


    m_entries[entry] = storeFdbData;

    string key = "Vlan" + to_string(vlan.m_vlan_info.vlan_id) + ":" + entry.mac.to_string();

-    if (fdbData.origin != FDB_ORIGIN_VXLAN_ADVERTIZED)
+    status = sai_fdb_api->create_fdb_entry(&fdb_entry, (uint32_t)attrs.size(), attrs.data());


why set again since line 1362 may do the same action.

@pettershao-ragilenetworks: Thanks for the comments. this duplicate piece looks got introduced while rebase/merging. Corrected.

prsunny · 2021-07-30T23:10:05Z

orchagent/observer.h

@@ -2,12 +2,14 @@
 #define SWSS_OBSERVER_H

 #include <list>
+#include <algorithm>


Please remove this include

@prsunny :Thanks, Done.

prsunny · 2021-07-30T23:11:32Z

orchagent/observer.h


 using namespace std;
 using namespace swss;

 enum SubjectType
 {
+    SUBJECT_TYPE_ALL_CHANGES,


Where is this used? If not, please remove

@prsunny :Thanks, the code used to use was part of optimization for the subject calss, which is no more part of the PR. Removed the attribute.

prsunny · 2021-07-30T23:11:48Z

orchagent/observer.h

@@ -16,8 +18,14 @@ enum SubjectType
    SUBJECT_TYPE_MIRROR_SESSION_CHANGE,
    SUBJECT_TYPE_INT_SESSION_CHANGE,
    SUBJECT_TYPE_PORT_CHANGE,
+    SUBJECT_TYPE_BRIDGE_PORT_CHANGE,


Who is sending this update? Can you point me to code?

@prsunny : This is been processed in isolationgrouporch.cpp:233. The update is sent from portsOrch part of addBridgePort and removeBridgePort calls. thanks.

pettershao-ragilenetworks · 2021-08-01T07:36:39Z

orchagent/fdborch.cpp


    m_entries[entry] = storeFdbData;

    string key = "Vlan" + to_string(vlan.m_vlan_info.vlan_id) + ":" + entry.mac.to_string();

-    if (fdbData.origin != FDB_ORIGIN_VXLAN_ADVERTIZED)
+    if ((fdbData.origin != FDB_ORIGIN_MCLAG_ADVERTIZED) ||


i think should be &&， right?
if ((fdbData.origin != FDB_ORIGIN_MCLAG_ADVERTIZED) &&
(fdbData.origin != FDB_ORIGIN_VXLAN_ADVERTIZED))

@pettershao-ragilenetworks : Thanks for pointing to the error. You're right and is corrected now.

pettershao-ragilenetworks · 2021-08-02T03:25:38Z

orchagent/fdborch.cpp

-        SWSS_LOG_INFO("FdbOrch RemoveFDBEntry: mac=%s fdb origin is different; found_origin:%d delete_origin:%d",
-                entry.mac.to_string().c_str(), fdbData.origin, origin);
+        if ((origin == FDB_ORIGIN_MCLAG_ADVERTIZED) && (fdbData.origin == FDB_ORIGIN_LEARN) &&
+                        (port.m_oper_status == SAI_PORT_OPER_STATUS_DOWN) && (gMlagOrch->isMlagInterface(port.m_alias)))


one question:
when remote fdb added, and remote fdb deleted(flush for example), local peer will not remove the remote fdb entry(sync early)(peerlink up), but by local age event, right?

show mac(sync, port already modify to peerlink) No. Vlan MacAddress Port Type ----- ------ ----------------- ------------ ------ 1 10 00:10:94:00:00:33 PortChannel2 dynamic

@pettershao-ragilenetworks: If the MAC gets added by ICCPd as remote ( Sync MAC from peer MCLAG node) the entry will be programmed as STATIC ( while allowing MAC move) There is no AGE event expected, also as you rightly said the FLUSH will not let the HW remove the MAC entry. MAC gets removed only when ICCPd sends delete event for MAC entry.

pettershao-ragilenetworks · 2021-08-02T03:35:39Z

orchagent/fdborch.cpp

+    {
+        //If the MAC is dynamic_local change the origin accordingly
+        //MAC is added/updated as dynamic to allow aging.
+        storeFdbData.origin = FDB_ORIGIN_LEARN;


so fdb entry syncd from remote peer will not be remoted immediately when remote peer do fdb flush action, right?

pettershao-ragilenetworks · 2021-08-02T05:42:44Z

orchagent/fdborch.cpp

@@ -300,6 +389,47 @@ void FdbOrch::update(sai_fdb_event_t        type,
            }
        }

+        // If MAC is MCLAG remote do not delete for age event, Add the MAC back..
+        if (existing_entry->second.origin == FDB_ORIGIN_MCLAG_ADVERTIZED)


i think this can't be reach since addFdbEntry() will modify origin to FDB_ORIGIN_LEARN to store.

@pettershao-ragilenetworks : This is to address timing scenarios and platforms which doesnt support SAI_FDB_ENTRY_ATTR_ALLOW_MAC_MOVE. If the MAC is added by ICCPD then the origin is set as FDB_ORIGIN_MCLAG (not to age) If the FdbOrch receives an AGE event ( in timing scenarios or on the platform which doesnt have support for SAI_FDB_ENTRY_ATTR_ALLOW_MAC_MOVE) and MAC entry origin is FDB_ORIGIN_MCLAG, since the MAC is remotely leared it should not get deleted so FdbOrch re-adds the MAC entry.

Praveen-Brcm · 2021-08-02T19:39:10Z

so fdb entry syncd from remote peer will not be remoted immediately when remote peer do fdb flush action, right?
@pettershao-ragilenetworks: Yes, If the ICCPD adds the MAC as local ( in certain scenarios like Iccpd session down..etc) the MAC entry is as good as Local HW learned. Remote peer flush will not cause delete , it can only be deleted by Local AGE or FLUSH events.

Praveen-Brcm · 2021-08-02T23:16:39Z

@gechiang , @prsunny , @akokhan ,@qiluo-msft ,@pettershao-ragilenetworks : Can you please help approve the PR and merge. Thanks .

adyeung · 2021-08-03T21:18:25Z

@prsunny @akokhan @qiluo-msft @pettershao-ragilenetworks, please be reminded to approve the code PR today, this is blocking the other code PRs from sonic-buildimage

gechiang · 2021-08-04T00:33:34Z

@Praveen-Brcm Please submit a submodule PR for SWSS so that this change is included in submodule.

This reverts commit 7aca82d.

lguohan · 2021-08-11T13:15:42Z

orchagent/fdborch.cpp

    attrs.push_back(attr);

-    if ((fdbData.origin == FDB_ORIGIN_VXLAN_ADVERTIZED) && (fdbData.type == "dynamic"))
+    if ((fdbData.origin == FDB_ORIGIN_VXLAN_ADVERTIZED) || (fdbData.origin == FDB_ORIGIN_MCLAG_ADVERTIZED)
+            || (fdbData.type == "dynamic"))


THIS modification does not look RIGHT.

@lguohan : Thanks for pointing out. Thats right as part of WB, when the MAC addresses are added via the bake function, the attribute SAI_FDB_ENTRY_ATTR_ALLOW_MAC_MOVE is set wrongly.
Will raise a PR and fix this today. Thanks.

* Mclag enhacements support code changes. * Adding change to allow MCLAG remote MAC move. * Added support for adding mclag remote mac to kernel, on top of PR-1276 * Updating the change from PR1276 and PR885. * Adding new orchfiles to mock_tests * MCLAG Unique IP support changes. * Removed dependency with PR 885. * Adding observer support for mlagorch. * Fixed FDB notifiation issue * Fixing the test_mclag_fdb type attributes. * Remove as the change may not be supported on non-brcm for PortChannel settings. * Removing the isolation group handling from Mlagorch, Isolation group now will be added/updated only via mclagsyncd updates. * Added back the update function. Co-authored-by: Tapash Das <tapash.das@broadcom.com>

chenkelly · 2022-03-02T08:58:53Z

Hi @Praveen-Brcm
The mclagsyncd set "traffic_disable" in APP_LAG_TABLE_NAME
https://github.com/Azure/sonic-swss/blob/master/mclagsyncd/mclaglink.cpp#L1308

but portsorch don't handle this field in doLagTask
https://github.com/Azure/sonic-swss/blob/master/orchagent/portsorch.cpp#L3580
Should we support this traffic distribution mode for LAG?
Is it under development?
Thanks.

gechiang · 2022-03-02T17:02:27Z

@chenkelly can you please open a new issue to track this? Please show a real example on how to configure it and produce the doLagTask issue as you described in that new issue creation and I will assign it to BRCM team to investigate/fix.
Thanks!

chenkelly · 2022-03-04T09:23:50Z

@gechiang
Please check #2167. Thanks.

Mclag enhacements support code changes.

3524229

gitsabari mentioned this pull request Jul 9, 2020

Mclag enhacements #1349

Merged

Praveen-Brcm mentioned this pull request Sep 22, 2020

iccpd service is not running in the image built from latest SONIC mainline code. sonic-net/sonic-buildimage#5310

Open

rlhui assigned rlhui and gechiang and unassigned rlhui Oct 29, 2020

This was referenced Oct 30, 2020

MCLAG Enhancements Praveen-Brcm/sonic-swss-common#1

Closed

MCLAG Enhancements per HLD https://github.com/Azure/SONiC/pull/596 sonic-net/sonic-swss-common#405

Merged

MCLAG enhacements ICCPd initial code commit sonic-net/sonic-buildimage#4819

Merged

rathnasabapathyv approved these changes Nov 11, 2020

View reviewed changes

rathnasabapathyv previously approved these changes Nov 11, 2020

View reviewed changes

Merge branch 'master' into mclag_enhacements

a5cd48c

Praveen-Brcm dismissed rathnasabapathyv’s stale review via a5cd48c November 17, 2020 00:22

Updating code to fix compilation issues.

16e2cf9

pettershao-ragilenetworks reviewed Jul 30, 2021

View reviewed changes

Addressed review comments.

a28f557

Praveen-Brcm dismissed akokhan’s stale review via a28f557 July 30, 2021 21:28

prsunny reviewed Jul 30, 2021

View reviewed changes

Addressed review comments.

329016b

pettershao-ragilenetworks reviewed Aug 1, 2021

View reviewed changes

Addressed review comment.

dcbf3f4

pettershao-ragilenetworks reviewed Aug 2, 2021

View reviewed changes

Addressed a build failure.

7a18eb7

gechiang requested a review from prsunny August 2, 2021 23:49

prsunny approved these changes Aug 3, 2021

View reviewed changes

gechiang merged commit 7aca82d into sonic-net:master Aug 4, 2021

neethajohn added a commit that referenced this pull request Aug 4, 2021

Revert "Mclag enhacements support code changes. (#1331)"

def23bf

This reverts commit 7aca82d.

neethajohn mentioned this pull request Aug 4, 2021

Revert "Mclag enhacements support code changes." #1851

Closed

yxieca mentioned this pull request Aug 10, 2021

[warm-reboot] APPLY_VIEW failure in set operation "SAI_API_FDB:brcm_sai_set_fdb_entry_attribute" sonic-net/sonic-buildimage#8367

Closed

lguohan reviewed Aug 11, 2021

View reviewed changes

vaibhavhd mentioned this pull request Aug 11, 2021

Fix warmboot issue PR##8367 #1866

Merged

judyjoseph added the Included in 202106 Branch label Aug 20, 2021

chenkelly mentioned this pull request Mar 4, 2022

[MCLAG] Portsorch don't handle "traffic_disable" in APP DB LAG table #2167

Open

prasanna-cls mentioned this pull request Dec 7, 2022

ICCP Bridgeport removal failure when MCLAG interface Bridgeport exist #2535

Open

7 tasks

Mclag enhacements support code changes. #1331

Mclag enhacements support code changes. #1331

Conversation

Praveen-Brcm commented Jun 24, 2020 • edited Loading

chenkelly commented Jul 14, 2020

Praveen-Brcm commented Jul 18, 2020

chenkelly commented Jul 20, 2020 • edited Loading

rathnasabapathyv commented Nov 11, 2020

gechiang commented Nov 11, 2020

Praveen-Brcm commented Nov 16, 2020

gechiang commented Nov 16, 2020

rlhui commented Nov 16, 2020

Praveen-Brcm commented Nov 16, 2020

rlhui commented Nov 16, 2020

Praveen-Brcm commented Nov 16, 2020

gechiang commented Nov 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettershao-ragilenetworks Aug 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen-Brcm commented Aug 2, 2021

Praveen-Brcm commented Aug 2, 2021

adyeung commented Aug 3, 2021

gechiang commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenkelly commented Mar 2, 2022 • edited Loading

gechiang commented Mar 2, 2022

chenkelly commented Mar 4, 2022

Praveen-Brcm commented Jun 24, 2020 •

edited

Loading

chenkelly commented Jul 20, 2020 •

edited

Loading

pettershao-ragilenetworks Aug 1, 2021 •

edited

Loading

chenkelly commented Mar 2, 2022 •

edited

Loading