Added Change for given Route ECMP to fallback on Default Route ECMP #3389

abdosi · 2024-11-24T06:06:00Z

What I did:
Added Change for given Route ECMP to fallback on Default Route ECMP. When all the Members of Route are Link Down and if route is eligible for fallback to default route the ECMP Member in SAI Nexthop Goup are updated to the Default Route Nexthop/Nexthop's Members.

This change does not take care of this scenarios:

When the Route which is fallback on Default Route Nexthops if the original nexthop become active [link comes up] it does not move back to original path. Reason is we except this should transient case as the Route which is fallback should get deleted once all the links are down
If Default Routes gets updated [BGP Updates] or if default Route nexthops become link down we do not update ECMP members of Routes that are already fallback to default. Again Reason being Route which is fallback should get deleted once all the links are down and is during this short window getting default routes update is very corner case. We can optimize if needed.

Why I did:
For Faster of Traffic Convergence for Routes where it is ok to send traffic over default route when most specific prefix/route do not have any valid nexthops for transient time before more specific route gets deleted.

How I verified:
UT updated
Ixia based Traffic Convergance.

Signed-off-by: Abhishek Dosi <[email protected]>

arlakshm · 2024-11-25T16:09:48Z

orchagent/routeorch.cpp

+
+    if (default_nhg_key.getSize() == 1)
+    {
+       current_default_route_nhops.insert(*default_nhg_key.getNextHops().begin());


nit: indentation #Closed

arlakshm · 2024-11-25T16:20:46Z

orchagent/routeorch.cpp

+
+        if (nhopgroup->second.nh_member_install_count == 0 && nhopgroup->second.eligible_for_default_route_nh_swap && !nhopgroup->second.is_default_route_nh_swap)
+        {
+            if(nexthop.ip_address.isV4())


if at this time the default route from bgp is not present. will the v4_active_default_route_nhops have the drop port?

@arlakshm : if no default route than existing behavior will happen where nexthop group will not have any members which will cause drop as expected.

arlakshm · 2024-11-25T16:24:16Z

orchagent/routeorch.cpp

+                    {
+                       if (ip_prefix.isV4())


nit: indentation #Resolved

arlakshm · 2024-11-25T16:29:55Z

orchagent/routeorch.cpp

@@ -673,6 +761,10 @@ void RouteOrch::doTask(Consumer& consumer)
                    {
                        ctx.protocol = fvValue(i);
                    }
+                    if (fvField(i) == "fallback_to_default_route")
+		    {


fix indentation. mix of tabs and spaces #Closed

arlakshm · 2024-11-25T16:29:58Z

orchagent/routeorch.cpp

+                    if (fvField(i) == "fallback_to_default_route")
+		    {
+                        fallback_to_default_route = fvValue(i) == "true";
+		    }


fix indentation. mix of tabs and spaces #Closed

arlakshm · 2024-11-25T16:30:34Z

orchagent/routeorch.cpp

@@ -1067,9 +1174,19 @@ void RouteOrch::doTask(Consumer& consumer)
            }
            else if (m_syncdNextHopGroups[it_nhg.first].ref_count == 0)
            {
-                removeNextHopGroup(it_nhg.first);
+               // Pass the flag to indicate if the NextHop Group as Default Route NH Members as swapped.
+               removeNextHopGroup(it_nhg.first, m_syncdNextHopGroups[it_nhg.first].is_default_route_nh_swap);


fix indentation #Resolved

arlakshm · 2024-11-25T16:30:54Z

orchagent/routeorch.cpp

+            updateDefaultRouteSwapSet(v4_default_nhg_key, v4_active_default_route_nhops);
+
+        if (v6_default_nhg_key.getSize())
+           updateDefaultRouteSwapSet(v6_default_nhg_key, v6_active_default_route_nhops);


fix indentation #Resolved

arlakshm · 2024-11-25T16:32:24Z

orchagent/routeorch.h


    std::string                         key;       // Key in database table
    std::string                         protocol;  // Protocol string
    bool                                is_set;    // True if set operation

    RouteBulkContext(const std::string& key, bool is_set)
-        : key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set)
+        : key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set),
+	  fallback_to_default_route(false)


mix of tabs and spaces #Closed

arlakshm · 2024-11-25T16:32:39Z

orchagent/routeorch.h

@@ -146,6 +152,7 @@ struct RouteBulkContext
        using_temp_nhg = false;
        key.clear();
        protocol.clear();
+	fallback_to_default_route = false;


remove tabs #Closed

arlakshm

/Azp run Azure.sonic-swss

Signed-off-by: Abhishek Dosi <[email protected]>

arlakshm · 2024-11-28T23:02:03Z

/Azp run Azure.sonic-swss

azure-pipelines · 2024-11-28T23:02:13Z

Azure Pipelines successfully started running 1 pipeline(s).

arlakshm

lgtm

This reverts commit 8d2d008.

Signed-off-by: Abhishek Dosi <[email protected]>

…into default-route

tests/test_nhg.py

@@ -188,7 +204,7 @@
        ntf.send("bfd_session_state_change", ntf_data, fvp)
    # BFD utilities for static route BFD and ecmp acceleration -- end

-    def init_test(self, dvs, num_intfs):
+    def init_test(self, dvs, num_intfs, is_ipv6_needed=False):


prsunny

Few questions on approach:

Why don't we simply delete the route if NHGroup is empty for a route and this option is set?
Why not reuse the default route NHgroup instead of creating a new one?

prsunny · 2024-12-10T19:49:46Z

orchagent/routeorch.cpp

            }
        }
+
+        if (!(v4_default_nhg_key.getSize()) && !(v6_default_nhg_key.getSize()))


please add { for if checks

prsunny · 2024-12-10T19:50:23Z

orchagent/routeorch.cpp

@@ -2506,6 +2645,11 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx)
        updateDefRouteState(ipPrefix.to_string());

        SWSS_LOG_INFO("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str());
+
+        if (ipPrefix.isV4())


Same here for {

prsunny · 2024-12-10T19:51:44Z

orchagent/routeorch.cpp

+
+    if (default_nhg_key.getSize() == 1)
+    {
+        current_default_route_nhops.insert(*default_nhg_key.getNextHops().begin());


fix indentation

prsunny · 2024-12-10T19:53:26Z

orchagent/routeorch.cpp

@@ -673,6 +761,10 @@ void RouteOrch::doTask(Consumer& consumer)
                    {
                        ctx.protocol = fvValue(i);
                    }
+                    if (fvField(i) == "fallback_to_default_route")


who sets this attribute?

it is being done here: #3353

Signed-off-by: Abhishek Dosi <[email protected]>

tests/test_nhg.py

abdosi · 2024-12-11T16:59:58Z

/azp run Azure.sonic-swss

azure-pipelines · 2024-12-11T17:00:25Z

Azure Pipelines successfully started running 1 pipeline(s).

abdosi added 3 commits November 24, 2024 06:05

Changes

72b5825

Signed-off-by: Abhishek Dosi <[email protected]>

More changes

e3b48ae

Signed-off-by: Abhishek Dosi <[email protected]>

Added UT

9c5c2e7

Signed-off-by: Abhishek Dosi <[email protected]>

abdosi marked this pull request as ready for review November 25, 2024 03:45

abdosi requested a review from prsunny as a code owner November 25, 2024 03:45

abdosi changed the title ~~Changes for fallback to default route~~ Added Change for given Route ECMP to fallback on Default Route ECMP Nov 25, 2024

abdosi requested a review from arlakshm November 25, 2024 03:56

abdosi added the Request for 202405 Branch label Nov 25, 2024

arlakshm reviewed Nov 25, 2024

View reviewed changes

abdosi mentioned this pull request Nov 25, 2024

Changes to read route tag value from Netlink Message and take appropriate action #3353

Open

abdosi and others added 6 commits November 25, 2024 21:08

Fix indenation and missed file changes

126c201

Signed-off-by: Abhishek Dosi <[email protected]>

Fix indenation

4751f41

Signed-off-by: Abhishek Dosi <[email protected]>

Fix

1fef1c2

Signed-off-by: Abhishek Dosi <[email protected]>

Fixed flaky test case

de8e979

Signed-off-by: Abhishek Dosi <[email protected]>

Merge remote-tracking branch 'upstream/master' into default-route

b374597

Merge branch 'master' into default-route

540dd52

rlhui assigned abdosi Nov 27, 2024

Merge branch 'master' into default-route

de037d5

arlakshm previously approved these changes Nov 28, 2024

View reviewed changes

Update test_nhg.py

8d2d008

abdosi dismissed arlakshm’s stale review via 8d2d008 November 29, 2024 01:56

abdosi and others added 6 commits November 29, 2024 21:50

Revert "Update test_nhg.py"

a90ba41

This reverts commit 8d2d008.

More changes

d9272ee

Signed-off-by: Abhishek Dosi <[email protected]>

Merge branch 'master' into default-route

0c6ec8b

Fix

5985e34

Signed-off-by: Abhishek Dosi <[email protected]>

Merge branch 'default-route' of https://github.com/abdosi/sonic-swss …

780d69d

…into default-route

Merge branch 'master' into default-route

d65732d

github-advanced-security bot found potential problems Dec 10, 2024

View reviewed changes

prsunny reviewed Dec 10, 2024

View reviewed changes

abdosi added 3 commits December 11, 2024 09:36

Merge remote-tracking branch 'upstream/master' into default-route

a6d1c77

Address review comment

409b268

Signed-off-by: Abhishek Dosi <[email protected]>

Add the test case back

7718dfa

Signed-off-by: Abhishek Dosi <[email protected]>

github-advanced-security bot found potential problems Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Change for given Route ECMP to fallback on Default Route ECMP #3389

Added Change for given Route ECMP to fallback on Default Route ECMP #3389

abdosi commented Nov 24, 2024 •

edited

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024

abdosi Nov 25, 2024

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm left a comment

arlakshm commented Nov 28, 2024

azure-pipelines bot commented Nov 28, 2024

arlakshm left a comment

prsunny left a comment

prsunny Dec 10, 2024

abdosi Dec 11, 2024

prsunny Dec 10, 2024

abdosi Dec 11, 2024

prsunny Dec 10, 2024

prsunny Dec 10, 2024

abdosi Dec 11, 2024

abdosi commented Dec 11, 2024

azure-pipelines bot commented Dec 11, 2024

Added Change for given Route ECMP to fallback on Default Route ECMP #3389

Are you sure you want to change the base?

Added Change for given Route ECMP to fallback on Default Route ECMP #3389

Conversation

abdosi commented Nov 24, 2024 • edited Loading

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm Nov 25, 2024 • edited by abdosi Loading

Choose a reason for hiding this comment

arlakshm left a comment

Choose a reason for hiding this comment

arlakshm commented Nov 28, 2024

azure-pipelines bot commented Nov 28, 2024

arlakshm left a comment

Choose a reason for hiding this comment

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

abdosi commented Dec 11, 2024

azure-pipelines bot commented Dec 11, 2024

abdosi commented Nov 24, 2024 •

edited

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading

arlakshm Nov 25, 2024 •

edited by abdosi

Loading