[Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN #1264

srj102 · 2020-04-20T06:22:08Z

Existing code supports a single mapper type to be created. Either
BRIDGE or VRF type mappers are supported.
Change to support multiple mapper type creations is being added.
Here mappers of type VLAN-VNI as well as VRF-VNI will be supported.
support for single mapper type creation is not changed.
Only one mode is accepted at a time.
Changes to support P2P tunnel creation and deletion.
A refcnt is maintained per resource type (IMR or IP prefix)
and based on this a P2P tunnel is created or deleted.
The VxlanTunnel object is reused for P2P and P2MP tunnels.
State DB updation for every P2P VXLAN tunnel discovered.
Changes to support handling of VXLAN_REMOTE_VNI table.
Interfaces with portsorch to create a Port object of type
TUNNEL on tunnel discovery. This is also used to extend
SAI interface function changes to support P2P tunnels.
EVPN NVO table handling.

HLD Location : https://github.com/Azure/SONiC/pull/437/commits

Signed-off-by: Rajesh Sankaran rajesh.sankaran@broadcom.com

What I did
Please refer to the details above.

Why I did it
EVPN VXLAN Implementation

How I verified it
Tested along with PR 1266. Used test script in PR 1318.
Also ran test_vnet.py from master branch to verify there were no failures.

Details if related

orchagent/vxlanorch.h

orchagent/portsorch.cpp

gord1306 · 2020-05-13T07:45:52Z

orchagent/portsorch.cpp

+    else
+      tunnel.m_learn_mode = "disable";
+    m_portList[tunnel_alias] = tunnel;
+    portOidToName[tunnel_id] = tunnel_alias;


is this an unused variable?

This is due to dependency on #885 . Will check
on whether the dependency can be removed.

gord1306 · 2020-05-13T07:45:58Z

orchagent/portsorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    portOidToName.erase(tunnel.m_tunnel_id);


is this an unused variable?

Same reply as above.

orchagent/vxlanorch.cpp

orchagent/vxlanorch.h

to allow for compilation of the PR sonic-net/sonic-swss#1264

lgtm-com · 2020-06-10T10:55:00Z

This pull request introduces 5 alerts when merging dd6bdc2 into a3a010a - view on LGTM.com

new alerts:

5 for FIXME comment

lgtm-com · 2020-06-22T11:37:45Z

This pull request introduces 5 alerts when merging 8291755 into 17a2f93 - view on LGTM.com

new alerts:

5 for FIXME comment

lgtm-com · 2020-09-18T16:17:42Z

This pull request introduces 5 alerts when merging 4f69c4a into b4938a5 - view on LGTM.com

new alerts:

5 for FIXME comment

lgtm-com · 2020-09-21T15:29:01Z

This pull request introduces 5 alerts when merging be9ca35 into 2f5d2d9 - view on LGTM.com

new alerts:

5 for FIXME comment

orchagent/portsorch.cpp

prsunny · 2020-10-16T01:52:54Z

orchagent/portsorch.cpp

+      attr.value.oid = port.m_lag_id;
+      attrs.push_back(attr);
+    }
+    else if  (port.m_type == Port::TUNNEL)


Please remove this block and add this section to the original if/else case. And remove the #if 0

orchagent/portsorch.cpp

prsunny · 2020-10-16T01:55:06Z

orchagent/portsorch.cpp

+    else
+      tunnel.m_learn_mode = "disable";
+    m_portList[tunnel_alias] = tunnel;
+    //portOidToName[tunnel_id] = tunnel_alias;


remove commented code

Will need to uncomment once the changes related to portOidToName in PR #1275 are merged.

As discussed, this can be removed and later added based on the other PR

orchagent/vxlanorch.cpp

orchagent/portsorch.cpp

kperumalbfn · 2020-10-20T20:31:32Z

orchagent/vxlanorch.cpp

+    // dest ip
+    if ((dst_ip != nullptr) && p2p)
+    {
+        attr.id = SAI_TUNNEL_ATTR_PEER_MODE;


Is this P2P tunnel mode used only for L2 EVPN??

With the EVPN feature, the P2P tunnels will be created irrespective of L2 or L3 VXLAN. However the Tunnel Nexthops for the L3VXLAN case use the P2MP tunnel. MAC and IMET point to the created P2P tunnel.

@srj102 Since P2P tunnels are used only for L2 EVPN, we could create these tunnel objects only for L2 EVPNs.

This would mean the SAI driver has to handle creation of tunnels in the device based on both schemes. Instead creating P2P tunnels also would mean that SAI implementation can create the tunnels in the device based on
either of the triggers.

Other aspects like P2P tunnel operstatus, P2P tunnel stats(though not added as part of this release) is useful and
applicable for L3 VXLAN as well.

kperumalbfn · 2020-10-21T19:13:22Z

orchagent/vxlanorch.cpp

+        attr.value.s32 = SAI_TUNNEL_PEER_MODE_P2P;
+        tunnel_attrs.push_back(attr);
+        attr.id = SAI_TUNNEL_ATTR_ENCAP_DST_IP;
+        attr.value.ipaddr = *dst_ip;


If P2P mode used for L3 EVPN, SAI doesn't specify preference of DST_IP from tunnel object or tunnel nexthop object. Which one is used for rewrite??

When the SAI_NEXT_HOP_ATTR_TUNNEL_ID refers to a P2P tunnel object, it is expected that the SAI_NEXT_HOP_ATTR_IP and the SAI_TUNNEL_ATTR_ENCAP_DST_IP be the same. In the event of a mismatch, The SAI spec doesn't specify
a preference, however since the p2p tunnel id is being specified instead of a p2mp tunnel id, the p2p tunnel's DIP should be chosen.

lgtm-com · 2020-10-23T10:29:19Z

This pull request introduces 5 alerts when merging 105f281 into 3142693 - view on LGTM.com

new alerts:

5 for FIXME comment

dev-aviz · 2020-10-23T16:40:06Z

orchagent/portsorch.cpp

@@ -3319,6 +3335,48 @@ bool PortsOrch::addBridgePort(Port &port)
    sai_attribute_t attr;
    vector<sai_attribute_t> attrs;

+    if (port.m_type == Port::PHY)


can it be convereted to switch stmt instead if-else?

Original code also had if else and hence retained it.

orchagent/vxlanorch.cpp

kperumalbfn · 2020-10-29T18:16:11Z

orchagent/vxlanorch.cpp

+          remove_tunnel_termination(ids_.tunnel_term_id);
+      }
+
+      remove_tunnel_termination(ids_.tunnel_id);


In case of tunnel p2p mode, one tunnel mapper entry could be part of multiple tunnel objects. In that case, I think it is better to check any valid tunnelObject references before removing the mapper entry.

p2p tunnels would have used TUNNEL_MAP_USE_COMMON_ENCAP_DECAP which is a no-op in the deleteMapperHw. So this should not be an issue

orchagent/vxlanorch.cpp

kperumalbfn · 2020-10-29T18:30:04Z

orchagent/vxlanorch.cpp

+       return true;
+    }
+
+    if (gPortsOrch->isVlanMember(vlanPort, tunnelPort)) 


part of else??

Could you elaborate ? Didnt understand.

orchagent/vxlanorch.cpp

kperumalbfn · 2020-10-29T18:43:45Z

orchagent/vxlanorch.cpp


    return true;
 }

+void VxlanTunnelOrch::deleteTunnelPort(Port &tunnelPort)


When is this method instead of delTunnelUser?? There is no caller to this method.

Will be used by fdborch to delete the tunnel once all the MACs learnt against a tunnel are removed. Not part of this PR.

1. Existing code supports a single mapper type to be created. Either BRIDGE or VRF type mappers are supported. Change to support multiple mapper type creations is being added. Here mappers of type VLAN-VNI as well as VRF-VNI will be supported. support for single mapper type creation is not changed. Only one mode is accepted at a time. 2. Changes to support P2P tunnel creation and deletion. A refcnt is maintained per resource type (IMR or IP prefix) and based on this a P2P tunnel is created or deleted. The VxlanTunnel object is reused for P2P and P2MP tunnels. State DB updation for every P2P VXLAN tunnel discovered. 3. Changes to support handling of VXLAN_REMOTE_VNI table. Interfaces with portsorch to create a Port object of type TUNNEL on tunnel discovery. This is also used to extend 4. SAI interface function changes to support P2P tunnels. 5. EVPN NVO table handling. Signed-off-by: Rajesh Sankaran <rajesh.sankaran@broadcom.com>

2. Re-instated changes which were overwritten by the force push

lgtm-com · 2020-11-13T16:22:20Z

This pull request introduces 5 alerts when merging 3c4a883 into fea7ade - view on LGTM.com

new alerts:

5 for FIXME comment

This was overwritten by a force push on Nov 13.

orchagent/portsorch.cpp

prsunny · 2020-11-21T01:21:49Z

orchagent/portsorch.cpp

+    else
+      tunnel.m_learn_mode = "disable";
+    m_portList[tunnel_alias] = tunnel;
+    //portOidToName[tunnel_id] = tunnel_alias;


As discussed, this can be removed and later added based on the other PR

orchagent/portsorch.cpp

prsunny · 2020-11-21T01:23:56Z

orchagent/vxlanorch.h

+   uint32_t      ip_refcnt;
+   uint32_t      spurious_add_imr_refcnt;
+   uint32_t      spurious_del_imr_refcnt;
+   VxlanTunnel*  dip_tunnel;


I think this comment is not addressed

orchagent/vxlanorch.cpp

1. Code styling issues. 2. dip_tunnel removed from refcnts structure getDipTunnel function which was referring to the above is now removed. callers for the above function now call VxlanTunnelOrch::getVxlanTunnel 3. Changes to introduce a new method to get the tunnel name given the DIP.

srj102 · 2020-11-23T10:52:17Z

retest this please

srj102 · 2020-11-23T13:40:56Z

retest this please

prsunny · 2020-11-23T18:38:00Z

retest vs please

…#1264) * https://github.com/Azure/SONiC/pull/437/commits 1. Existing code supports a single mapper type to be created. Either BRIDGE or VRF type mappers are supported. Change to support multiple mapper type creations is being added. 2. Changes to support P2P tunnel creation and deletion. A refcnt is maintained per resource type (IMR or IP prefix) and based on this a P2P tunnel is created or deleted. 3. Changes to support handling of VXLAN_REMOTE_VNI table. Interfaces with portsorch to create a Port object of type TUNNEL on tunnel discovery. 4. SAI interface function changes to support P2P tunnels. 5. EVPN NVO table handling. Signed-off-by: Rajesh Sankaran <rajesh.sankaran@broadcom.com> Co-authored-by: Tapash Das <tapash.das@broadcom.com>

srj102 changed the title ~~Vxlanorch and Portsorch changes for EVPN VXLAN~~ [Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN Apr 21, 2020

gord1306 reviewed May 13, 2020

View reviewed changes

orchagent/vxlanorch.h Outdated Show resolved Hide resolved

gord1306 reviewed May 13, 2020

View reviewed changes

orchagent/vxlanorch.h Show resolved Hide resolved

gord1306 reviewed May 13, 2020

View reviewed changes

orchagent/portsorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 13, 2020

View reviewed changes

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Show resolved Hide resolved

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Show resolved Hide resolved

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 14, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 15, 2020

View reviewed changes

orchagent/vxlanorch.cpp Show resolved Hide resolved

gord1306 reviewed May 15, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

gord1306 reviewed May 15, 2020

View reviewed changes

orchagent/vxlanorch.h Show resolved Hide resolved

srj102 added a commit to srj102/sonic-swss-common that referenced this pull request May 20, 2020

Handled Review comments. Also added changes to warm_restart.h/cpp

86dacf5

to allow for compilation of the PR sonic-net/sonic-swss#1264

srj102 added a commit to srj102/sonic-swss-common that referenced this pull request Jun 3, 2020

Handled Review comments. Also added changes to warm_restart.h/cpp

7252397

to allow for compilation of the PR sonic-net/sonic-swss#1264

srj102 added a commit to srj102/sonic-swss-common that referenced this pull request Jun 3, 2020

Handled Review comments. Also added changes to warm_restart.h/cpp

e4c7678

to allow for compilation of the PR sonic-net/sonic-swss#1264

srj102 force-pushed the evpn_vxlan_orchagent branch from 098bcdb to 39d2a50 Compare June 3, 2020 09:28

srj102 marked this pull request as ready for review June 10, 2020 10:10

srj102 mentioned this pull request Jun 10, 2020

[VxlanOrch] pytest for EVPN VXLAN #1318

Merged

jainp1979 mentioned this pull request Jun 24, 2020

[Orchagent]: FdbOrch changes for EVPN VXLAN #1275

Merged

srj102 force-pushed the evpn_vxlan_orchagent branch from 4f69c4a to be9ca35 Compare September 21, 2020 14:42

prsunny reviewed Oct 16, 2020

View reviewed changes

kperumalbfn reviewed Oct 21, 2020

View reviewed changes

srj102 force-pushed the evpn_vxlan_orchagent branch from be9ca35 to 105f281 Compare October 23, 2020 09:49

dev-aviz reviewed Oct 23, 2020

View reviewed changes

kperumalbfn reviewed Oct 28, 2020

View reviewed changes

orchagent/vxlanorch.cpp Outdated Show resolved Hide resolved

kperumalbfn reviewed Oct 29, 2020

View reviewed changes

srj102 and others added 6 commits November 13, 2020 07:35

L3 VxLAN Changes. Update VRF MAP Table.

e4942ca

1. Compiled with the corresponding changes to swss-common.

024dae2

2. Re-instated changes which were overwritten by the force push

Tested orchagent with corresponding vxlanmgr changes and evpn pytest

a07fdd0

Removed L3VXLAN changes as it is filed in a separate PR

7b20961

Updated vxlan test script to handle multiple mappers.

3c4a883

srj102 force-pushed the evpn_vxlan_orchagent branch from 4a4ca68 to 3c4a883 Compare November 13, 2020 15:37

srj102 added 2 commits November 16, 2020 23:59

Reinstatng changes made to handle code review comments on Oct 23, 30.

d229e41

This was overwritten by a force push on Nov 13.

Fix subport intf mtu vs test failure. This was due to incorrect rebase.

35cebfd

prsunny reviewed Nov 21, 2020

View reviewed changes

prsunny approved these changes Dec 2, 2020

View reviewed changes

prsunny merged commit 095f481 into sonic-net:master Dec 2, 2020

stephenxs mentioned this pull request Dec 3, 2020

Advance submodule head for sonic-swss and sonic-swss-common stephenxs/sonic-buildimage#46

Closed

3 tasks

yxieca mentioned this pull request Dec 8, 2020

[portsorch] adjust port initialized event back to notice #1532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN #1264

[Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN #1264

srj102 commented Apr 20, 2020 •

edited

Loading

gord1306 May 13, 2020

srj102 May 13, 2020

gord1306 May 13, 2020

srj102 May 13, 2020

lgtm-com bot commented Jun 10, 2020

lgtm-com bot commented Jun 22, 2020

lgtm-com bot commented Sep 18, 2020

lgtm-com bot commented Sep 21, 2020

prsunny Oct 16, 2020

prsunny Oct 16, 2020

srj102 Oct 23, 2020

prsunny Nov 21, 2020

kperumalbfn Oct 20, 2020

srj102 Oct 23, 2020

kperumalbfn Nov 2, 2020

srj102 Nov 3, 2020

kperumalbfn Oct 21, 2020

srj102 Oct 23, 2020 •

edited

Loading

lgtm-com bot commented Oct 23, 2020

dev-aviz Oct 23, 2020

srj102 Oct 30, 2020

kperumalbfn Oct 29, 2020

srj102 Oct 30, 2020

kperumalbfn Oct 29, 2020

srj102 Oct 30, 2020

kperumalbfn Oct 29, 2020

srj102 Oct 30, 2020

lgtm-com bot commented Nov 13, 2020

prsunny Nov 21, 2020

prsunny Nov 21, 2020

srj102 commented Nov 23, 2020

srj102 commented Nov 23, 2020

prsunny commented Nov 23, 2020

[Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN #1264

[Orchagent] Vxlanorch and Portsorch changes for EVPN VXLAN #1264

Conversation

srj102 commented Apr 20, 2020 • 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

lgtm-com bot commented Jun 10, 2020

lgtm-com bot commented Jun 22, 2020

lgtm-com bot commented Sep 18, 2020

lgtm-com bot commented Sep 21, 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

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

srj102 Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Oct 23, 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

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 Nov 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srj102 commented Nov 23, 2020

srj102 commented Nov 23, 2020

prsunny commented Nov 23, 2020

srj102 commented Apr 20, 2020 •

edited

Loading

srj102 Oct 23, 2020 •

edited

Loading