Added support for EVPN L3 VXLAN as described in the PR Azure/SONiC#437 #1267

tapashdas · 2020-04-21T12:03:25Z

Added support for EVPN L3 VXLAN as described in the PR sonic-net/SONiC#437

What I did
VRFMgr Changes to update VRF VNI Map information and notify VRFOrch.
Neighbor Orch changes to Add and Remove Tunnel Nexthops.
Nexthop Key changes to accommodate VNI and Router MAC.
Route Orch changes to Add and Remove Overlay routes, Create and Delete Overlay Nexthops.
Port Orch changes to bring Up / Down Router Interface (IRB Vlan interface) on VRF - VNI Bind / Unbind.

Signed-off-by: Tapash Das tapash.das@broadcom.com

Why I did it

How I verified it

Details if related

…C#437

prsunny

As a general comment, almost all the logs are in NOTICE level and seems to be for debugging. Kindly change it to INFO or DEBUG levels to avoid cluttering the syslog.

prsunny · 2020-10-08T20:33:40Z

cfgmgr/vrfmgr.h

    void doTask(Consumer &consumer);

    std::map<std::string, uint32_t> m_vrfTableMap;
    std::set<uint32_t> m_freeTables;
+    VRFNameVNIMapTable vrf_vni_map_table_;


Please follow the same naming convention as in the rest of the class

ok. Will do.

prsunny · 2020-10-08T20:34:27Z

cfgmgr/vrfmgr.h

 class VrfMgr : public Orch
 {
 public:
    VrfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const std::vector<std::string> &tableNames);
    using Orch::doTask;
+    std::string m_evpnVxlanTunnel;
+
+    uint32_t getVRFmappedVNI(const std::string& vrf_name) const


Please have {} for if/else and move this to .cpp file

ok. will do.

prsunny · 2020-10-08T20:45:12Z

cfgmgr/vrfmgr.cpp

+        }
+    }
+
+    if(m_evpnVxlanTunnel.empty()) {


Please open { in new line. Also for the rest of the if in this PR

ok. will do.

prsunny · 2020-10-08T20:50:49Z

cfgmgr/vrfmgr.cpp

+
+    old_vni = getVRFmappedVNI(vrf_name);
+    SWSS_LOG_NOTICE("VRF VNI map update vrf %s, vni %d, old_vni %d", vrf_name.c_str(), vni, old_vni);
+    if (vni != old_vni)


So what triggers this value change? VRF_TABLE is part of config_db right? so is this some user config that they change the vni value?

Yes. VRF VNI Map is stored in CONFIG DB VRF_TABLE, This takes care of scenario if VRF VNI mapping is changed.

prsunny · 2020-10-08T20:53:11Z

cfgmgr/vrfmgr.cpp

+    SWSS_LOG_ENTER();
+    string key;
+
+    if(m_evpnVxlanTunnel.empty()) {


space after if

ok. will do.

prsunny · 2020-10-08T22:26:42Z

orchagent/routeorch.cpp

@@ -1021,7 +1147,7 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const

        /* Increase the ref_count for the next hop (group) entry */
        increaseNextHopRefCount(nextHops);
-        SWSS_LOG_INFO("Create route %s with next hop(s) %s",
+        SWSS_LOG_NOTICE("Create route %s with next hop(s) %s",


Please change this to INFO

Ok. Will do.

prsunny · 2020-10-08T22:26:47Z

orchagent/routeorch.cpp

        }
-        SWSS_LOG_INFO("Set route %s with next hop(s) %s",
+        SWSS_LOG_NOTICE("Set route %s with next hop(s) %s",


Please change this to INFO

Ok. Will do.

prsunny · 2020-10-08T22:37:48Z

orchagent/routeorch.cpp

+    EvpnNvoOrch* evpn_orch = gDirectory.get<EvpnNvoOrch*>();
+    VxlanTunnelOrch* tunnel_orch = gDirectory.get<VxlanTunnelOrch*>();
+    bool status = false;
+    int ip_refcnt = -2;


Why is this initialized to -2? Can we have it as 0?

Ok. Will do. Used it for debug purpose.

prsunny · 2020-10-08T22:39:09Z

orchagent/routeorch.cpp

+    bool status = false;
+    int ip_refcnt = -2;
+
+    SWSS_LOG_NOTICE("Routeorch Add Remote VTEP %s, VNI %d, VR_ID %lx",


You could remove this log as it is same as one below

Ok. Will do.

prsunny · 2020-10-08T22:39:45Z

orchagent/routeorch.cpp

+
+    SWSS_LOG_NOTICE("Routeorch Add Remote VTEP %s, VNI %d, VR_ID %lx",
+            nextHop.ip_address.to_string().c_str(), nextHop.vni, vrf_id);
+    status = tunnel_orch->addTunnelUser(nextHop.ip_address.to_string(), nextHop.vni, 0, TNL_SRC_IP, vrf_id);


Where is TNL_SRC_IP defined?

It is defined in PR 1264.
https://github.com/Azure/sonic-swss/pull/1264/files : orchagent/vxlanorch.h

prvattem · 2020-10-16T03:23:08Z

cfgmgr/vrfmgr.cpp

-                m_appVnetTableProducer.set(vrfName, kfvFieldsValues(t));
+                if (!setLink(vrfName))
+                {
+                    SWSS_LOG_ERROR("Failed to create vrf netdev %s", vrfName.c_str());


How would we recover from Vrf creation failures?

This is existing vrfmgr code in base/master branch. Not changed due to EVPN changes. Need to address this separately.

prvattem · 2020-10-16T03:29:50Z

cfgmgr/vrfmgr.cpp

+        m_evpnVxlanTunnel = "";
+    }
+
+    SWSS_LOG_NOTICE("Removed evpn nvo tunnel %s", m_evpnVxlanTunnel.c_str());


If m_evpnVxlanTunnel is empty, this log wouldnt print anything.

If m_evpnVxlanTunnel is not empty
m_evpnVxlanTunnel will be empty after m_evpnVxlanTunnel = "";

Finally this log wouldnt print anything for m_evpnVxlanTunnel ?

Ok. Will update.

prvattem · 2020-10-16T03:37:49Z

cfgmgr/vrfmgr.cpp

+
+    }
+
+    if ((vni == 0) && add)


What will be this case, where VNI is 0 and is still an 'add' condition?

This is to take care of the scenario when VRF is configured but VRF to VNI Mapping is not configured.

prvattem · 2020-10-16T06:25:13Z

cfgmgr/vrfmgr.cpp

+    }
+
+    SWSS_LOG_NOTICE("VRF VNI map update vrf %s, s_vni %s, add %d", vrf_name.c_str(), s_vni.c_str(), add);
+    doVrfVxlanTableUpdate(vrf_name, s_vni, add);


There could be VNIs which are only mapped to VLANs but not VRFs. How do we make sure that, we are using unique VNIs here?

VNI can be both L2 + L3. For the case where Vlan associated with VNI is tenant Vlan as well.
So L3 VNI need not be unique.

Didnt quite understand this. An L2 VNI between 2 leafs for a given Tenant, could also be a L3 VNI for different Tenant?
My understanding is that, either L2 or L3 VNI, they have to be unique in a given system.

VLAN-X +VNI-X -- L2 VXLAN
VLAN-Y + VNI-Y + VRF-Y -- L3 VXLAN (for TYPE-5)
Where do we verify if VNI-X and VNI-Y are not overlapping?

Currently it is taken care in CLI Validation.

@tapashdas If the configuration is done directly in config_db, we would need to check the validity of the global VLAN/VRF-->VNI mapping. It will be better to maintain or validate global vlan/vrf<-->VNI mapping in the cfgmgr.

@tapashdas In vxlanmgr.cpp, if the vni is already mapped to a vlan, mapper entry is rejected. But here the mapping entry is updated. Can we reject the config similar to vxlanmgr?

ok. Will do.

prvattem · 2020-10-23T16:08:48Z

Type-5 routes are applicable only for non-default VRF Tenants right? Could you please let me know, where do we block this for default VRF?

prvattem · 2020-10-23T16:22:18Z

For remote Type-5 Prefixes, the routes need to be programmed with ONLINK attribute right? Are those changes, part of this pull request? or please point me to that PR, if that is different.

prvattem · 2020-10-23T16:33:09Z

Are local Static Route Prefixes supported?

tapashdas · 2020-10-30T16:52:25Z

Are local Static Route Prefixes supported?

No. Static route over tunnel is currently not supported. This is EVPN L3 VXLAN PR so out of scope of this PR.

tapashdas · 2020-10-30T16:54:16Z

Type-5 routes are applicable only for non-default VRF Tenants right? Could you please let me know, where do we block this for default VRF?

Yes Type-5 routes are applicable only for non-default VRF. FRR will update APP DB route table only for non-default VRF.

kishorekunal01 · 2020-10-30T16:57:37Z

For remote Type-5 Prefixes, the routes need to be programmed with ONLINK attribute right? Are those changes, part of this pull request? or please point me to that PR, if that is different.

ONLINK is programmed only in the Kernel not in the hardware. Hardware is programed with VLAN, VNI, RMAC with the prefix.

prvattem · 2020-10-30T17:13:32Z

For remote Type-5 Prefixes, the routes need to be programmed with ONLINK attribute right? Are those changes, part of this pull request? or please point me to that PR, if that is different.

ONLINK is programmed only in the Kernel not in the hardware. Hardware is programed with VLAN, VNI, RMAC with the prefix.

I understand that it is programmed in Kernel. What I was asking is, to point me to the PR where it is done.

prvattem · 2020-10-30T17:17:40Z

Are local Static Route Prefixes supported?

No. Static route over tunnel is currently not supported. This is EVPN L3 VXLAN PR so out of scope of this PR.

If it is not currently supported, I agree to that. I dont agree if we say it is not a EVPN feature.
It is very well a supported feature for EVPN L3 VXLAN in other implementations. If user wants to advertise a prefix that is not learnt by any host subnets, we should allow that to happen.

tapashdas · 2020-12-03T04:17:50Z

retest vs please

tapashdas · 2020-12-04T07:23:51Z

retest vs please

kperumalbfn · 2020-12-11T23:41:20Z

cfgmgr/vrfmgr.cpp

+    }
+
+    SWSS_LOG_NOTICE("VRF VNI map update vrf %s, s_vni %s, add %d", vrf_name.c_str(), s_vni.c_str(), add);
+    doVrfVxlanTableUpdate(vrf_name, s_vni, add);


@tapashdas If the configuration is done directly in config_db, we would need to check the validity of the global VLAN/VRF-->VNI mapping. It will be better to maintain or validate global vlan/vrf<-->VNI mapping in the cfgmgr.

kperumalbfn · 2020-12-11T23:44:15Z

cfgmgr/vrfmgr.cpp

+    }
+
+    SWSS_LOG_NOTICE("VRF VNI map update vrf %s, s_vni %s, add %d", vrf_name.c_str(), s_vni.c_str(), add);
+    doVrfVxlanTableUpdate(vrf_name, s_vni, add);


@tapashdas In vxlanmgr.cpp, if the vni is already mapped to a vlan, mapper entry is rejected. But here the mapping entry is updated. Can we reject the config similar to vxlanmgr?

kperumalbfn · 2020-12-11T23:50:52Z

orchagent/routeorch.cpp

+                        SWSS_LOG_NOTICE("Failed to create remote vtep %s", nexthop.to_string(overlay_nh).c_str());
+                        return false;
+                    }
+                    next_hop_id = m_neighOrch->addTunnelNextHop(nexthop);


NULL object-id for tunnel-nhop failure is not checked.

We could change this API to return status instead of returning the object-id and check the status for success/failure

ok. Will do.

kperumalbfn · 2020-12-12T00:15:48Z

orchagent/routeorch.cpp

+                status = deleteRemoteVtep(vrf_id, tunnel_nh);
+                if (status == false)
+                {
+                    SWSS_LOG_NOTICE("Failed to delete remote vtep %s ecmp", tunnel_nh.to_string(true).c_str());


error log and return false? Please check the error status in the caller methods as well

ok. Will do.

tapashdas · 2020-12-14T04:27:22Z

Will take care of the comments in cfgmgr/vrfmgr.cpp. Currently all validation checks are at CLI level.

prsunny · 2020-12-18T03:14:43Z

orchagent/nexthopkey.h

@@ -45,19 +49,44 @@ struct NextHopKey
            throw std::invalid_argument(err);
        }
    }
+    NextHopKey(const std::string &str, bool overlay_nh)
+	{


please fix alignment

prsunny · 2020-12-18T03:14:56Z

orchagent/nexthopkey.h

    const std::string to_string() const
    {
        return ip_address.to_string() + NH_DELIMITER + alias;
    }

+    const std::string to_string(bool overlay_nh) const
+	{


please fix alignment

prsunny · 2020-12-18T03:15:53Z

orchagent/vrforch.cpp

@@ -133,9 +156,99 @@ bool VRFOrch::delOperation(const Request& request)

    vrf_table_.erase(vrf_name);
    vrf_id_table_.erase(router_id);
+    error = delVrfVNIMap(vrf_name, 0);
+    if (error == false)


please add {

prsunny · 2020-12-18T03:16:13Z

orchagent/vrforch.cpp

+    old_vni = getVRFmappedVNI(vrf_name);
+    SWSS_LOG_INFO("VRF '%s' vni %d old_vni %d", vrf_name.c_str(), vni, old_vni);
+
+    if (old_vni != vni) {


{ -> next line

prsunny · 2020-12-18T03:16:51Z

orchagent/vxlanorch.cpp

@@ -2132,7 +2134,7 @@ bool EvpnNvoOrch::addOperation(const Request& request)
    source_vtep_ptr = tunnel_orch->getVxlanTunnel(vtep_name);

    SWSS_LOG_INFO("evpnnvo: %s vtep : %s \n",nvo_name.c_str(), vtep_name.c_str());
-
+  


I think tab space is added here

prsunny · 2020-12-18T03:17:05Z

orchagent/vrforch.cpp

@@ -84,6 +95,12 @@ bool VRFOrch::addOperation(const Request& request)
        vrf_table_[vrf_name].vrf_id = router_id;
        vrf_table_[vrf_name].ref_count = 0;
        vrf_id_table_[router_id] = vrf_name;
+        if (vni != 0) {


{ -> new line

prsunny · 2020-12-18T03:29:48Z

orchagent/routeorch.cpp


    if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end())
    {
        m_syncdRoutes.emplace(vrf_id, RouteTable());
        m_vrfOrch->increaseVrfRefCount(vrf_id);
    }

+    if(nextHops.is_overlay_nexthop())


space after if

prsunny · 2020-12-18T03:30:08Z

orchagent/routeorch.cpp

    auto it_route = m_syncdRoutes.at(vrf_id).find(ipPrefix);

    /* The route is pointing to a next hop */
    if (nextHops.getSize() == 1)
    {
-        NextHopKey nexthop(nextHops.to_string());
+        NextHopKey nexthop;
+        if (overlay_nh) {


{ -> next line

prsunny · 2020-12-18T16:27:46Z

orchagent/routeorch.cpp

@@ -1212,13 +1320,55 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
            /* Try to create a new next hop group */
            if (!addNextHopGroup(nextHops))
            {
+                /* NextHopGroup is in "Ip1|alias1,Ip2|alias2,..." format*/
+                std::vector<std::string> nhops = tokenize(nextHops.to_string(), ',');


Please explain this flow.

…nic-net#1267) Add subcommand to show midplane status for modular chassis and related unit tests

Support a new ACL table type called L3V4V6. This table supports both v4 and v6 Match types. Add unit tests for this new ACL table type. HLD: sonic-net/SONiC#1267 Signed-off-by: Ravi(Marvell) rck@innovium.com

* Combine v4 and v6 L3 ACL rules on optimized platforms #1267

…onic-net#2735) * Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267

tapashdas added 2 commits April 21, 2020 04:40

Added support for EVPN L3 VXLAN as described in the PR sonic-net/SONi…

16c7f0a

…C#437

Added Unit Test Fixes and code to handle overlay ECMP group

11cf178

tapashdas marked this pull request as ready for review June 24, 2020 07:43

tapashdas mentioned this pull request Jun 24, 2020

PY Test script for EVPN L3 VxLAN #1330

Merged

prsunny reviewed Oct 8, 2020

View reviewed changes

tapashdas and others added 2 commits October 10, 2020 11:31

Merge branch 'master' into evpn_L3_vxlan

2171210

Updated Review Comments. And fixed master branch merge issues.

b0c9342

prvattem reviewed Oct 16, 2020

View reviewed changes

tapashdas and others added 2 commits December 2, 2020 13:39

Merge branch 'master' into evpn_L3_vxlan

9b3fe9d

Resolved Code merge issues after PR1264 merge.

8419ee2

Fix for mirror VS test failures.

cd8c5df

kperumalbfn reviewed Dec 12, 2020

View reviewed changes

Updated Review Comments.

f542d8f

prvattem previously approved these changes Dec 18, 2020

View reviewed changes

prsunny reviewed Dec 18, 2020

View reviewed changes

Updated review comments.

c9ccfe4

tapashdas dismissed prvattem’s stale review via c9ccfe4 December 18, 2020 06:46

prvattem approved these changes Dec 18, 2020

View reviewed changes

prsunny reviewed Dec 18, 2020

View reviewed changes

prsunny approved these changes Dec 18, 2020

View reviewed changes

prsunny merged commit b369bb2 into sonic-net:master Dec 18, 2020

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

[show] Add subcommand to show midplane status for modular chassis (so…

be7cac5

…nic-net#1267) Add subcommand to show midplane status for modular chassis and related unit tests

prsunny pushed a commit that referenced this pull request May 25, 2023

Combine v4 and v6 L3 ACL rules on optimized platforms #1267 (#2735)

1b1c10a

* Combine v4 and v6 L3 ACL rules on optimized platforms #1267

theasianpianist pushed a commit to theasianpianist/sonic-swss that referenced this pull request Jul 20, 2023

Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267 (s…

7903721

…onic-net#2735) * Combine v4 and v6 L3 ACL rules on optimized platforms sonic-net#1267

		@@ -2132,7 +2134,7 @@ bool EvpnNvoOrch::addOperation(const Request& request)
		source_vtep_ptr = tunnel_orch->getVxlanTunnel(vtep_name);

		SWSS_LOG_INFO("evpnnvo: %s vtep : %s \n",nvo_name.c_str(), vtep_name.c_str());

Added support for EVPN L3 VXLAN as described in the PR Azure/SONiC#437 #1267

Added support for EVPN L3 VXLAN as described in the PR Azure/SONiC#437 #1267

Conversation

tapashdas commented Apr 21, 2020 • edited Loading

prsunny left a 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

tapashdas Oct 9, 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

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

prvattem commented Oct 23, 2020

prvattem commented Oct 23, 2020

prvattem commented Oct 23, 2020

tapashdas commented Oct 30, 2020

tapashdas commented Oct 30, 2020

kishorekunal01 commented Oct 30, 2020

prvattem commented Oct 30, 2020

prvattem commented Oct 30, 2020

tapashdas commented Dec 3, 2020

tapashdas commented Dec 4, 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

tapashdas commented Dec 14, 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

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

tapashdas commented Apr 21, 2020 •

edited

Loading

tapashdas Oct 9, 2020 •

edited

Loading