EVPN VxLAN enhancement to support P2MP tunnel based programming for Layer2 extension #1858

dgsudharsan · 2021-08-07T04:17:32Z

What I did
Added support to identify based on sai attribute capability if P2P tunnels are support and if not achieve L2 extension using P2MP tunnels. This involves adding P2MP tunnel bridgeport, remote endpoint IP combination to L2MC group in VLAN that is using combined mode.

Why I did it
Certain platforms support P2P tunnels and while few prefer using P2MP tunnels for L2 extension due to resource constraints.

How I verified it
Added VS test cases to cover all the existing scenarios using P2MP model for programming SAI.

Details if related
Refactored existing tests, to reuse the helper APIs across both P2P and P2MP.

…p2mp

lgtm-com · 2021-08-07T05:01:58Z

This pull request introduces 11 alerts and fixes 9 when merging 6cc06ec into df96059 - view on LGTM.com

new alerts:

6 for Unused import
5 for Unused local variable

fixed alerts:

9 for Unused local variable

lgtm-com · 2021-08-07T18:44:37Z

This pull request fixes 9 alerts when merging beeb58a into df96059 - view on LGTM.com

fixed alerts:

9 for Unused local variable

srj102 · 2021-08-27T08:58:48Z

orchagent/vxlanorch.h

@@ -190,11 +203,13 @@ class VxlanTunnel
    // Total Routes using the DIP tunnel. 
    int getDipTunnelRefCnt(const std::string);
    int getDipTunnelIMRRefCnt(const std::string);
-    int getDipTunnelIPRefCnt(const std::string);
+    int getRemoteEndPointIPRefCnt(const std::string);


Not sure why this name change was required. The previous 2 functions have the name as getDipTunnelXXX so
I would recommend keeping it the same as earlier. IPRefcnt represented the IP routes.

After this implementation there are two logics, one using DIP tunnels and anther not using it. There are some APIs that are common on both places and the current naming with DipTunnel naming will lead to confusion. So I renamed the APIs that are used commonly in both logics generically so they can be used in both flows.

Sure I understand that we would like a naming common for P2P and P2MP. So it might be better to change the naming for the IMR as well as the total refcnt i.e. we can eliminate the use of DipTunnel in the name for all the common cases.

Modified as suggested.

orchagent/vxlanorch.h

orchagent/vxlanorch.cpp

srj102 · 2021-08-27T10:00:34Z

orchagent/vxlanorch.cpp

+        }
+        else
+        {
+            it->second.ip_refcnt++;


shouldn't the tnl_users_[remote_vtep] need to be updated here ?

Both are same. auto it = tnl_users_.find(remote_vtep); -->If remote vtep exists in the map it.second will be referencing the value of tnl_users_[remote_vtep]

orchagent/vxlanorch.cpp

dgsudharsan · 2021-09-01T05:36:14Z

@srj102 I have given explanation for some review comments and the rest I have addressed in code (marked those as resolved).

lgtm-com · 2021-09-01T07:09:45Z

This pull request fixes 9 alerts when merging 04858de into eb79ca4 - view on LGTM.com

fixed alerts:

9 for Unused local variable

srj102 · 2021-09-03T09:29:10Z

orchagent/fdborch.cpp

@@ -804,9 +821,6 @@ void FdbOrch::doTask(Consumer& consumer)
                    }
                    port = tunnel_orch->getTunnelPortName(remote_ip);
                }
-
-


Is this an intentional change ? By not removing this line the message will continue to remain in the m_tosync queue and be processed infinitely.

That's a good catch. It got removed during a merge conflict. I have added it back.

srj102 · 2021-09-03T10:09:24Z

orchagent/fdborch.cpp

@@ -1147,7 +1161,7 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name,
    }

    /* Retry until port is member of vlan*/
-    if (vlan.m_members.find(port_name) == vlan.m_members.end())
+    if (check_vlan_member && vlan.m_members.find(port_name) == vlan.m_members.end())


I see that the EvpnRemoteVnip2mpOrch::addOperation already has a call to
gPortsOrch->addVlanMember in which case I don't see why this check is required.
if we call portsorch->isVlanMember.. It should work for both P2P and P2MP ?

Actually I thought of a better logic to include checks for P2P and P2MP. In P2MP scenario, the vlan member differentiator is remote IP (since only one port exists based on SIP tunnel) and thus including that in check will ensure it works in both scenarios.

srj102 · 2021-09-03T10:11:07Z

orchagent/fdborch.cpp

@@ -1121,15 +1135,15 @@ void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
 }

 bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name,
-        FdbData fdbData)
+        FdbData fdbData, bool check_vlan_member)


Do we need to change the API signature ? If there is a need to check for vlan member it can be done using the isDipTunnelSupported call ?

Removed changes to API signature.

lgtm-com · 2021-09-04T04:59:53Z

This pull request fixes 9 alerts when merging 9e07e78 into 62f7200 - view on LGTM.com

fixed alerts:

9 for Unused local variable

dgsudharsan · 2021-09-09T06:02:46Z

@srj102 Can you please review? I have addressed your review comments.

srj102 · 2021-09-13T14:29:03Z

@srj102 Can you please review? I have addressed your review comments.

Thanks for taking care of the comments. Changes look good.

dgsudharsan · 2021-09-15T18:00:37Z

/azpw run

mssonicbld · 2021-09-15T18:00:38Z

/AzurePipelines run

azure-pipelines · 2021-09-15T18:00:49Z

Azure Pipelines successfully started running 1 pipeline(s).

lgtm-com · 2021-09-17T00:31:27Z

This pull request fixes 9 alerts when merging a74eb57 into 57d21e7 - view on LGTM.com

fixed alerts:

9 for Unused local variable

dgsudharsan · 2021-09-17T15:49:42Z

/azpw run

mssonicbld · 2021-09-17T15:49:43Z

/AzurePipelines run

azure-pipelines · 2021-09-17T15:49:54Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2021-09-22T21:32:28Z

/azpw run

mssonicbld · 2021-09-22T21:32:30Z

/AzurePipelines run

azure-pipelines · 2021-09-22T21:32:40Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2021-10-08T20:57:54Z

orchagent/main.cpp

@@ -643,6 +644,32 @@ int main(int argc, char **argv)
        orchDaemon = make_shared<FabricOrchDaemon>(&appl_db, &config_db, &state_db, chassis_app_db.get()); 
    }

+    uint32_t max_tunnel_modes = 2;


Could you check if this can be moved to vxlan constructor?

prsunny · 2021-10-08T20:59:05Z

orchagent/portsorch.cpp

@@ -4348,8 +4406,10 @@ bool PortsOrch::addVlan(string vlan_alias)

    sai_vlan_id_t vlan_id = (uint16_t)stoi(vlan_alias.substr(4));
    sai_attribute_t attr;
+


Please keep existing changes as is

prsunny · 2021-10-08T21:02:09Z

orchagent/portsorch.cpp

+
+}
+
+bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode, string end_point_ip)


Can you check if you can keep it similar to removeVlanMember?

lgtm-com · 2021-10-12T05:21:45Z

This pull request fixes 9 alerts when merging c750f17 into ef6b5d4 - view on LGTM.com

fixed alerts:

9 for Unused local variable

…p2mp

lgtm-com · 2021-10-12T19:18:12Z

This pull request fixes 9 alerts when merging c0631dc into fd0cafe - view on LGTM.com

fixed alerts:

9 for Unused local variable

dgsudharsan · 2021-10-12T21:53:01Z

/azpw run

mssonicbld · 2021-10-12T21:53:03Z

/AzurePipelines run

azure-pipelines · 2021-10-12T21:53:13Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2021-10-13T03:19:21Z

/azpw run

mssonicbld · 2021-10-13T03:19:22Z

/AzurePipelines run

azure-pipelines · 2021-10-13T03:19:33Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2021-10-19T15:38:47Z

@srj102 Can you please reapprove? I addressed some comments from Prince and your approval got removed

…net#1858) #### What I did Made a change for aclshow and counterpoll that adds support for ACL flex counters. DEPENDS ON: sonic-net/sonic-swss-common#533 sonic-net/sonic-sairedis#953 sonic-net#1943 HLD: sonic-net/SONiC#857 #### How I did it Modified aclshow and counterpoll and UT. #### How to verify it Together with depends PRs. Run ACL/Everflow test suite.

dgsudharsan added 6 commits June 30, 2021 17:58

Vxlan evpn p2mp changes for Layer2 functionality

d9e23ad

Fixing compilation

3b6620b

Added referencing logic

979860f

Addressing code review comments

c458dba

Addressing code review comments

67ca9cc

Merge branch 'master' of github.com:Azure/sonic-swss into vxlan_evpn_…

6cc06ec

…p2mp

dgsudharsan requested a review from prsunny as a code owner August 7, 2021 04:17

Fixing lgtm

beeb58a

dgsudharsan mentioned this pull request Aug 9, 2021

EVPN VxLAN update for platforms using P2MP tunnel based L2 forwarding sonic-net/SONiC#806

Merged

srj102 requested changes Aug 27, 2021

View reviewed changes

Addressing code review comments

04858de

srj102 requested changes Sep 3, 2021

View reviewed changes

dgsudharsan added 3 commits September 3, 2021 21:43

Addressing code review comments

e2061ec

Merge branch 'master' of github.com:Azure/sonic-swss into HEAD

d0c92ef

Fixing test issues and flaky tests

9e07e78

srj102 previously approved these changes Sep 13, 2021

View reviewed changes

dgsudharsan mentioned this pull request Sep 13, 2021

Added query Enum value capability for vxlan EVPN feature sonic-net/sonic-sairedis#920

Merged

Merge remote-tracking branch 'upstream/master' into vxlan_evpn_p2mp

a74eb57

prsunny reviewed Oct 8, 2021

View reviewed changes

Addressing code review comments

ffbf06a

dgsudharsan dismissed srj102’s stale review via ffbf06a October 12, 2021 04:13

Cosmetic cleanups

c750f17

dgsudharsan added 2 commits October 12, 2021 16:42

Merge branch 'master' of github.com:Azure/sonic-swss into vxlan_evpn_…

dd9781a

…p2mp

Updating tests

c0631dc

prsunny approved these changes Oct 19, 2021

View reviewed changes

srj102 approved these changes Oct 20, 2021

View reviewed changes

prsunny merged commit b0aa6a0 into sonic-net:master Oct 20, 2021

fastiuk mentioned this pull request May 18, 2022

Image-installer: Fix duplication of image prefix fastiuk/sonic-buildimage#1

Closed

6 tasks

dgsudharsan deleted the vxlan_evpn_p2mp branch March 9, 2023 02:00

muhammadalihussnain mentioned this pull request May 6, 2024

DHCP DOS Mitigation HLD in SONiC sonic-net/SONiC#1651

Merged

		@@ -4348,8 +4406,10 @@ bool PortsOrch::addVlan(string vlan_alias)

		sai_vlan_id_t vlan_id = (uint16_t)stoi(vlan_alias.substr(4));
		sai_attribute_t attr;


		}

		bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode, string end_point_ip)

EVPN VxLAN enhancement to support P2MP tunnel based programming for Layer2 extension #1858

EVPN VxLAN enhancement to support P2MP tunnel based programming for Layer2 extension #1858

Conversation

dgsudharsan commented Aug 7, 2021 • edited Loading

lgtm-com bot commented Aug 7, 2021

lgtm-com bot commented Aug 7, 2021

Choose a reason for hiding this comment

dgsudharsan Aug 31, 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

dgsudharsan commented Sep 1, 2021

lgtm-com bot commented Sep 1, 2021

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

lgtm-com bot commented Sep 4, 2021

dgsudharsan commented Sep 9, 2021

srj102 commented Sep 13, 2021

dgsudharsan commented Sep 15, 2021

mssonicbld commented Sep 15, 2021

azure-pipelines bot commented Sep 15, 2021

lgtm-com bot commented Sep 17, 2021

dgsudharsan commented Sep 17, 2021

mssonicbld commented Sep 17, 2021

azure-pipelines bot commented Sep 17, 2021

dgsudharsan commented Sep 22, 2021

mssonicbld commented Sep 22, 2021

azure-pipelines bot commented Sep 22, 2021

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

lgtm-com bot commented Oct 12, 2021

lgtm-com bot commented Oct 12, 2021

dgsudharsan commented Oct 12, 2021

mssonicbld commented Oct 12, 2021

azure-pipelines bot commented Oct 12, 2021

dgsudharsan commented Oct 13, 2021

mssonicbld commented Oct 13, 2021

azure-pipelines bot commented Oct 13, 2021

dgsudharsan commented Oct 19, 2021

dgsudharsan commented Aug 7, 2021 •

edited

Loading

dgsudharsan Aug 31, 2021 •

edited

Loading