[Mux] Route handling based on mux status, kernel tunnel support by prsunny · Pull Request #1615 · sonic-net/sonic-swss

prsunny · 2021-01-28T07:41:39Z

What I did

Program/Reprogram routes to hardware based on Mux status
Create tunnel interface (tun0) in kernel
Add/remove tunnel routes in kernel

Why I did it
Support dual-tor SLB scenarios

How I verified it

Details if related

…rnel

prsunny · 2021-01-29T19:21:10Z

retest vs please

cfgmgr/tunnelmgr.cpp

gechiang · 2021-02-02T03:25:16Z

cfgmgr/tunnelmgr.cpp

+
+        if (it == m_tunnelCache.end())
+        {
+            SWSS_LOG_NOTICE("Tunnel %s not found", tunnelName.c_str());


Is it possible that we can ever receive a delete request for a tunnel that is not there? If not, may have to set syslog with Error or Warning for the least...

Changed to Error

cfgmgr/tunnelmgr.cpp

gechiang · 2021-02-02T03:54:51Z

orchagent/muxorch.cpp

+        neigh = NeighborEntry(it->first, alias_);
        if (!gNeighOrch->disableNeighbor(neigh))
        {
            return false;


Log error here?

gechiang · 2021-02-02T03:55:02Z

orchagent/muxorch.cpp

+        IpPrefix pfx = it->first.to_string();
+        if (create_route(pfx, it->second) != SAI_STATUS_SUCCESS)
+        {
+            return false;


Log error here?

create_route has ERR log in case of failure

gechiang · 2021-02-02T06:25:18Z

orchagent/routeorch.cpp

            return false;
        }

+        ++ count;


what is the purpose of this "count" variable? Does not look like it is being used?

This is the return value as part of function param.

orchagent/routeorch.cpp

gechiang · 2021-02-02T06:32:34Z

orchagent/routeorch.cpp

+    while(rt != it->second.end())
+    {
+        SWSS_LOG_INFO("Updating route %s", (*rt).prefix.to_string().c_str());
+        next_hop_id = m_neighOrch->getNextHopId(nextHop);


Is this really intended? Doe not look like nexchop is modified in this while loop therefore next_hop_id will always be the same each loop?

Yes, during mux state change, the nexthop id would have been changed to tunnel nexthop

lguohan · 2021-02-02T10:53:27Z

orchagent/routeorch.cpp

            return false;
        }

+        ++ count;


remove the space here?

lguohan · 2021-02-02T10:56:06Z

orchagent/neighorch.cpp


    nhop->second.nh_flags |= nh_flag;
-
+    uint32_t count;


what is the purpose of this count here?

This is the only place where valid/invalid nexthop group is used and the API is modified to return number of nexthop groups that were modified. This is a no-op here. The caller can determine what to do with returned value

lguohan · 2021-02-02T11:02:06Z

orchagent/routeorch.h


    RouteTables m_syncdRoutes;
    NextHopGroupTable m_syncdNextHopGroups;
+    NextHopRouteTable m_nextHops;


it is better to explain what does the nexthop router table store? the nexthop -> route map? does this includes all the routes to T1?

It is mentioned at line 67. This is generic to be useful for other use-cases

lguohan · 2021-02-02T11:05:33Z

orchagent/routeorch.cpp

+            RouteKey r_key = { vrf_id, ipPrefix };
+            auto nexthop = NextHopKey(nextHops.to_string());
+            addNextHopRoute(nexthop, r_key);
+        }


what is the purpose of add the <nexthop, r_key> ? is this used for track the nexthop change, so that we can find the route to be changed?

however, what about if there is a T1 route has only single nexthop?

also, what is the implication if we have this nexthop route table on T1? feel it is totally unnecessary.

In reply to: 568514575 [](ancestors = 568514575)

This is a common data structure for holding Single NH -> Routes. It includes both T1 and Mux routes. I think it is hard to differentiate with T1 routes. Do you have a suggestion?

first, if the muxorch is not enabled, we should not enable this nexthop -> route tracking.

isn't true we will know which port has muxcable, and only track nexthop if it is pointing that port?

The nexthop map is required for evpn case. Also to clarify, this does not contain T1 routes since in most cases its ECMP. This datastructure is only if a route has single nexthop all the time.

lguohan · 2021-02-02T11:09:32Z

orchagent/routeorch.cpp

+    auto rt = it->second.begin();
+    while(rt != it->second.end())
+    {
+        SWSS_LOG_INFO("Updating route %s", (*rt).prefix.to_string().c_str());


Updating route %s for what purpose? I think it is better to finish the sentence, it is unclear what do you update.

lguohan · 2021-02-02T11:09:56Z

orchagent/routeorch.cpp

+
+    if (it == m_nextHops.end())
+    {
+        SWSS_LOG_INFO("No routes found for NH %s", nextHop.ip_address.to_string().c_str());


is this debug level info?

lguohan · 2021-02-02T11:11:27Z

orchagent/muxorch.cpp

-    auto it = neighbors_.getIpAddresses().begin();
-    while (it != neighbors_.getIpAddresses().end())
+    auto l_fn = [&] (NextHopKey nh_key, uint32_t val) {
+        SWSS_LOG_INFO("Inc ref for %s by %d", nh_key.ip_address.to_string().c_str(), val);


is this debug level?

lguohan · 2021-02-02T11:16:51Z

orchagent/neighorch.cpp

+
 bool NeighOrch::hasNextHop(const NextHopKey &nexthop)
 {
+    // First check if mux has NH


why neighbor orch has to care if mux has NH or not?

When a route is programmed and if the neighbor is standby, then it will be a tunnel nexthop and not regular nexthop which will be owned by muxorch. So for mux neighbors, the nexthop management is by mux orch. For non-mux neighbor, it will follow the existing flow.

orchagent/routeorch.cpp

lguohan · 2021-02-02T11:27:45Z

orchagent/muxorch.cpp

-    auto it = neighbors_.getIpAddresses().begin();
-    while (it != neighbors_.getIpAddresses().end())
+    auto l_fn = [&] (NextHopKey nh_key, uint32_t val) {
+        SWSS_LOG_INFO("Dec ref for %s by %d", nh_key.ip_address.to_string().c_str(), val);


lguohan · 2021-02-02T11:29:29Z

orchagent/muxorch.cpp

+        for (uint32_t cnt = 0; cnt < val; ++cnt)
+        {
+            gNeighOrch->decreaseNextHopRefCount(nh_key);
+        }


instead of looping, can we just decrease the ref count by val refcount - val?

lguohan · 2021-02-02T11:29:52Z

orchagent/muxorch.cpp

+        SWSS_LOG_INFO("Inc ref for %s by %d", nh_key.ip_address.to_string().c_str(), val);
+        for (uint32_t cnt = 0; cnt < val; ++cnt)
+        {
+            gNeighOrch->increaseNextHopRefCount(nh_key);


can we increase the refcnt by value instead of looping here?

This is existing API and would need to define new if we have to increase by value. Also, considering the number of routes, prefer we can use this loop instead of defining new.

that will be cleaner, correct?

lguohan · 2021-02-02T11:33:10Z

orchagent/muxorch.cpp

+            return false;
+        }
+
+        if (!gRouteOrch->validnexthopinNextHopGroup(nh_key, num_routes))


both line 574 and 580 has num_routes, this cause readability of the code.

i think it is more readable to get del_route_num in line 574 and get add_route_num in line 580, and you can get their substraction add_route_num - del_route_num as refcnt change.

In enable flow, it is moving from Tunnel NH -> Direct NH (Neighbor). In this case, the ref count must be incremented only for the Direct NH. Similarly for disable flow, ref count is decremented for Direct NH. However, there is no ref_cnt management for tunnel NH as it is one time created.

lguohan

🕐

lguohan · 2021-02-02T17:52:32Z

orchagent/routeorch.cpp

            SWSS_LOG_NOTICE("Remove overlay Nexthop %s", ol_nextHops.to_string().c_str());
            removeOverlayNextHops(vrf_id, ol_nextHops);
        }
+        else if (ol_nextHops.getSize() == 1)


it is better to also check if the nexthop is pointing to the muxcable port or not.

tahmed-dev · 2021-02-02T23:58:40Z

cfgmgr/tunnelmgr.cpp

    const std::string & op = kfvOp(t);
+    TunnelInfo tunInfo;
+
+    for (auto idx : kfvFieldsValues(t))


nit: idx -> fieldValue

tahmed-dev · 2021-02-03T00:13:58Z

cfgmgr/tunnelmgr.cpp

+        ret = cmdIpTunnelIfAddress(ipPrefix.to_string(), res);
+        if (ret != 0)
+        {
+            SWSS_LOG_WARN("Failed to assign IP addr for tun if %s", ipPrefix.to_string().c_str());


maybe printout of res would help with debugging

tahmed-dev · 2021-02-03T00:14:36Z

cfgmgr/tunnelmgr.cpp

+        ret = cmdIpTunnelRouteAdd(prefix, res);
+        if (ret != 0)
+        {
+            SWSS_LOG_WARN("Failed to add route %s", prefix.c_str());


same regarding res here and throughout the PR.

tahmed-dev · 2021-02-03T00:16:20Z

cfgmgr/tunnelmgrd.cpp


    try
    {
+        vector<string> cfg_tun_tables = {


nit: camelCase style

orchagent/muxorch.h

tahmed-dev · 2021-02-03T00:29:32Z

orchagent/muxorch.h


    bool aclHandler(sai_object_id_t, bool = true);
-    bool nbrHandler(bool = true);
+    bool nbrHandler(bool, bool = true);


args name would?? Documentation?

orchagent/muxorch.h

As part of dual-tor features, kernel tunnel interface (tun0) is being created. The routes over this is intended for kernel forwarding and not expected to be installed in ASIC. Ref PR - sonic-net/sonic-swss#1615

prsunny · 2021-02-04T02:00:35Z

/Azurepipelines run

azure-pipelines · 2021-02-04T02:00:43Z

Azure Pipelines successfully started running 1 pipeline(s).

lguohan

Program/Reprogram routes to hardware based on Mux status Create tunnel interface (tun0) in kernel Add/remove tunnel routes in kernel

As part of dual-tor features, kernel tunnel interface (tun0) is being created. The routes over this is intended for kernel forwarding and not expected to be installed in ASIC. Ref PR - sonic-net/sonic-swss#1615

…c-net#1615) Program/Reprogram routes to hardware based on Mux status Create tunnel interface (tun0) in kernel Add/remove tunnel routes in kernel

As part of dual-tor features, kernel tunnel interface (tun0) is being created. The routes over this is intended for kernel forwarding and not expected to be installed in ASIC. Ref PR - sonic-net/sonic-swss#1615

…c-net#1615) Program/Reprogram routes to hardware based on Mux status Create tunnel interface (tun0) in kernel Add/remove tunnel routes in kernel

* [show] add support for muxcable metrics What I did Added support for show muxcable metrics. This essentially records what events came to different modules per se for toggling the mux from one state to another. for example admin@sonic$ show muxcable metrics Ethernet0 --json { "linkmgrd_switch_active_start": "2021-May-13 10:00:21.420898", "linkmgrd_switch_standby_end": "2021-May-13 10:01:15.696728", "linkmgrd_switch_unknown_end": "2021-May-13 10:00:26.123319", "xcvrd_switch_standby_end": "2021-May-13 10:01:15.696051", "xcvrd_switch_standby_start": "2021-May-13 10:01:15.690835" } or admin@sonic:$ show muxcable metrics Ethernet0 PORT EVENT TIME --------- ---------------------------- --------------------------- Ethernet0 linkmgrd_switch_active_start 2021-May-13 10:00:21.420898 Ethernet0 linkmgrd_switch_standby_end 2021-May-13 10:01:15.696728 Ethernet0 linkmgrd_switch_unknown_end 2021-May-13 10:00:26.123319 Ethernet0 xcvrd_switch_standby_end 2021-May-13 10:01:15.696051 Ethernet0 xcvrd_switch_standby_start 2021-May-13 10:01:15.690835 How I did it added changes in show/muxcable.py by reading and publishing the state DB contents for the corresponding table Signed-off-by: vaibhav-dahiya <[email protected]>

As part of dual-tor features, kernel tunnel interface (tun0) is being created. The routes over this is intended for kernel forwarding and not expected to be installed in ASIC. Ref PR - sonic-net/sonic-swss#1615

…c-net#1615) Program/Reprogram routes to hardware based on Mux status Create tunnel interface (tun0) in kernel Add/remove tunnel routes in kernel

Route handling based on mux status, add tunnel interface/routes in ke…

6d73276

…rnel

prsunny requested review from gechiang and tahmed-dev February 1, 2021 21:24

gechiang reviewed Feb 2, 2021

View reviewed changes

lguohan reviewed Feb 2, 2021

View reviewed changes

orchagent/routeorch.cpp Show resolved Hide resolved

lguohan reviewed Feb 2, 2021

View reviewed changes

lguohan requested changes Feb 2, 2021

View reviewed changes

lguohan reviewed Feb 2, 2021

View reviewed changes

tahmed-dev reviewed Feb 3, 2021

View reviewed changes

Address review comments, removed NH route table

1a5ac4e

prsunny mentioned this pull request Feb 3, 2021

[route_check.py] Skip route check for tun0 interfaces sonic-net/sonic-utilities#1399

Merged

lguohan approved these changes Feb 4, 2021

View reviewed changes

gechiang approved these changes Feb 4, 2021

View reviewed changes

tahmed-dev approved these changes Feb 4, 2021

View reviewed changes

prsunny merged commit 0b0d24c into sonic-net:master Feb 4, 2021

prsunny deleted the muxtunnel branch February 4, 2021 22:27

prsunny added the Request for 202012 Branch label Feb 4, 2021

daall pushed a commit that referenced this pull request Feb 5, 2021

[Mux] Route handling based on mux status, kernel tunnel support (#1615)

fc06176

Program/Reprogram routes to hardware based on Mux status Create tunnel interface (tun0) in kernel Add/remove tunnel routes in kernel

daall added the Included in 202012 Branch label Feb 5, 2021

prsunny restored the muxtunnel branch September 20, 2021 20:49

Conversation

prsunny commented Jan 28, 2021

Uh oh!

prsunny commented Jan 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment