[vnetorch] Add ECMP support for vnet tunnel routes #1960

shi-su · 2021-10-14T17:45:22Z

What I did
Add functions to create/remove next hop groups for vnet tunnel routes.
Count the reference count of next hop groups to create and remove as needed.
Share the counter of next hop groups with routeorch.
Add vs test

Why I did it
To add support for overlay ECMP.

How I verified it
Verify ECMP groups are properly created and removed with the functions.
Verify vs test passes

Details if related

prsunny · 2021-11-06T00:33:14Z

orchagent/vnetorch.cpp

@@ -1066,18 +1346,48 @@ bool VNetRouteOrch::handleTunnel(const Request& request)
        }
    }

+    if (!vni_list.empty() && vni_list.size() != ip_list.size())


These conditions need not be true. There could be empty comma separated list as VNI and MAC are optional fields

I thought these conditions are required. The field should be either empty or the match in length. Otherwise, it would be impossible to match the fields with endpoints. For example, if the endpoint had three addresses, the vni field should be one of the choices: empty; 0,0,0(three valid values); 0,,(comma separated empty values). The field should not be something like 0,1 since it would be impossible to figure which endpoint it corresponds to.

I also noticed that the current implementation of request parser cannot handle the case of comma-separated empty values. Therefore, I updated the value type of mac and vni such to string so that we can tokenize the values and handle them in vnetorch.

Added support for multiple endpoints to share the same vni in the latest commit.

prsunny · 2021-11-06T00:35:55Z

orchagent/vnetorch.cpp

-    uint32_t vni = 0;
+    vector<IpAddress> ip_list;
+    vector<MacAddress> mac_list;
+    vector<uint64_t> vni_list;


Can this be uint32?

Updated the vni field type and made it to uint32.

prsunny · 2021-11-06T00:38:12Z

orchagent/vnetorch.cpp

    }
    else
    {
-        vrf_obj->removeRoute(ipPrefix);
+        SWSS_LOG_ERROR("Unknown operation");


Please remove this and have the DEL as else part. We don't get any other operations in this flow

Agree, removed this part.

prsunny · 2021-11-06T00:40:10Z

orchagent/vnetorch.cpp

    {
-        if (op == SET_COMMAND && !add_route(vr_id, pfx, nh_id))
+        sai_object_id_t nh_id;
+        /* The route in pointing to one single endpoint */


Thanks for pointing it out. Fixed.

prsunny · 2021-11-06T00:43:45Z

orchagent/vnetorch.cpp

+                next_hop_group_entry.active_members[nexthop] = SAI_NULL_OBJECT_ID;
+                syncd_nexthop_groups_[vnet][nexthops] = next_hop_group_entry;
+            }
+            else {


{ -> new line

Updated as suggested.

prsunny · 2021-11-06T00:48:08Z

orchagent/vnetorch.cpp


-    if (op == SET_COMMAND)
+        if (it_route != syncd_tunnel_routes_[vnet].end())


Please add some comments for this if case

Added comments for this part.

prsunny · 2021-11-06T00:57:26Z

orchagent/vnetorch.h

@@ -12,6 +12,8 @@
 #include "ipaddresses.h"
 #include "producerstatetable.h"
 #include "observer.h"
+#include "intfsorch.h"


Remove if not used

prsunny · 2021-11-06T01:05:57Z

tests/test_vnet.py

+        vnet_obj.fetch_exist_entries(dvs)
+        assert nhg1_1 not in vnet_obj.nhgs
+
+        # Create another tunnel route to the same set of endpoints


add one case for IPv4 prefix over IPv6 endpoints in this same test 8

Added a test case as suggested.

prsunny · 2021-11-10T17:22:44Z

orchagent/vnetorch.h

+        { "endpoint",               REQ_T_IP_LIST },
+        { "ifname",                 REQ_T_STRING },
+        { "nexthop",                REQ_T_STRING },
+        { "vni",                    REQ_T_STRING },


Just thinking, if this has any backward compatibility issue?

I do not think it would, since we will convert it to the right format. I think the only difference is to process the values in the request parser or vnetorch.

prsunny · 2021-11-10T22:12:40Z

tests/test_vnet.py

@@ -790,7 +902,7 @@ def test_vnet_orch_1(self, dvs, testlog):
        vnet_obj.check_del_vnet_routes(dvs, 'Vnet_2001')

        delete_vnet_routes(dvs, "100.100.1.1/32", 'Vnet_2000')
-        vnet_obj.check_del_vnet_routes(dvs, 'Vnet_2001')
+        vnet_obj.check_del_vnet_routes(dvs, 'Vnet_2000')


Why is this changed?

I still believe this seems more like a typo, though I do not recall how I noticed this (maybe I added some more checks in the function and the test case failed). As we just removed a route from Vnet_2000, why do we want to check deleted route in Vnet_2001? I checked other calls of the same function and nowhere else use it in this way. I think it passed previously only because check_del_vnet_routes did not really check anything.

prsunny · 2021-11-13T01:20:23Z

@dgsudharsan , got to know that you were reviewing the PR. Please feel free to provide comments and we can address in a separate PR

What I did Add functions to create/remove next hop groups for vnet tunnel routes. Count the reference count of next hop groups to create and remove as needed. Share the counter of next hop groups with routeorch. Add vs test Why I did it To add support for overlay ECMP. How I verified it Verify ECMP groups are properly created and removed with the functions. Verify vs test passes

sachinv-msft · 2022-01-26T18:42:09Z

@prsunny Cisco requested this PR for 202012 branch, could you review and recommend next steps/add the necessary labels?

shi-su · 2022-01-26T18:45:31Z

@prsunny Cisco requested this PR for 202012 branch, could you review and recommend next steps/add the necessary labels?

#2104 will take the changes to 202012 branch. Should be merging soon.

…oint health monitoring (#2104) What I did Cherry-pick changes in #1960, #1883, #1955, #2058 Changes in #1960: Add functions to create/remove next hop groups for vnet tunnel routes. Count the reference count of next hop groups to create and remove as needed. Share the counter of next hop groups with routeorch. Add vs test Changes in #1883: Implement bfdorch to program hardware BFD sessions via bfd SAI. Add vs test for bfd sessions. Changes in #1955: Add functions to create/remove next hop groups for vnet tunnel routes. Count the reference count of next hop groups to create and remove as needed. Share the counter of next hop groups with routeorch. Adapt route endpoint according to the BFD state of endpoints. Changes in #2058: Advertise active vnet tunnel routes. Why I did it To add support for overlay ECMP with endpoint health monitoring.

shi-su requested a review from prsunny as a code owner October 14, 2021 17:45

prsunny mentioned this pull request Oct 19, 2021

Overlay ECMP with BFD support sonic-net/SONiC#861

Merged

shi-su force-pushed the ol_ecmp_only branch from a1fe93c to 4f4ad7f Compare November 4, 2021 06:10

prsunny reviewed Nov 6, 2021

View reviewed changes

Shi Su added 3 commits November 8, 2021 22:17

ADD support for overlay ecmp

f97d0de

Address review comments

d50d59c

make mac and vni request string

0bb8275

shi-su force-pushed the ol_ecmp_only branch from 4f4ad7f to 0bb8275 Compare November 9, 2021 18:23

prsunny reviewed Nov 10, 2021

View reviewed changes

Add support for one vni

ef828cb

prsunny approved these changes Nov 10, 2021

View reviewed changes

shi-su merged commit f502c32 into sonic-net:master Nov 11, 2021

stephenxs mentioned this pull request Nov 17, 2021

[sonic-swss] Advance submodule sonic-net/sonic-buildimage#9288

Merged

5 tasks

shi-su mentioned this pull request Jan 6, 2022

[202012] [vnetorch] Add ECMP support for vnet tunnel routes with endpoint health monitoring #2104

Merged

yxieca added Request for 202012 Branch and removed Request for 202012 Branch labels Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vnetorch] Add ECMP support for vnet tunnel routes #1960

[vnetorch] Add ECMP support for vnet tunnel routes #1960

shi-su commented Oct 14, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

shi-su Nov 10, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 6, 2021

shi-su Nov 9, 2021

prsunny Nov 10, 2021

shi-su Nov 10, 2021

prsunny Nov 10, 2021

shi-su Nov 10, 2021

prsunny Nov 10, 2021

prsunny commented Nov 13, 2021

sachinv-msft commented Jan 26, 2022

shi-su commented Jan 26, 2022


		if (op == SET_COMMAND)
		if (it_route != syncd_tunnel_routes_[vnet].end())

[vnetorch] Add ECMP support for vnet tunnel routes #1960

[vnetorch] Add ECMP support for vnet tunnel routes #1960

Conversation

shi-su commented Oct 14, 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

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

prsunny commented Nov 13, 2021

sachinv-msft commented Jan 26, 2022

shi-su commented Jan 26, 2022