Added change not to create ECMP Group in SAI and program the route if none of ECMP members are active/link-up by abdosi · Pull Request #3394 · sonic-net/sonic-swss

abdosi · 2024-11-26T02:49:47Z

What I did:
Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up.
Also do not program the Temp Route if Neigh is not active (Link Down)

Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case.

Why I did:
In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable.

Also in this case no point to add Temp Route if neighbor is link down.

How I verify:

Compile time test was failing because of this chnage so added oper_status as UP in the mock tests
Modified subport router interface UT for this scenario.
Ixia Convergence tests

Reference to full context of this changes:
Swss_route_enhancemnts.docx

all the members of ECMP groups are inactive. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

rlhui · 2024-11-27T22:32:03Z

@prsunny reminder to review, thanks.

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

…o-ecmp

prsunny · 2024-12-03T02:40:17Z

@abdosi , i think the test failures are specific to this PR as I see it passing for all others

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi · 2024-12-12T02:18:24Z

/azp run Azure.sonic-swss

azure-pipelines · 2024-12-12T02:18:34Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

tests/test_sub_port_intf.py

abdosi · 2024-12-12T14:48:51Z

/azp run Azure.sonic-swss

azure-pipelines · 2024-12-12T14:49:02Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2024-12-13T08:17:30Z

/azp run

azure-pipelines · 2024-12-13T08:17:40Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2024-12-13T08:32:40Z

/azp run

azure-pipelines · 2024-12-13T08:32:51Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2024-12-13T10:08:51Z

/azp run

bingwang-ms · 2024-12-24T17:21:29Z

Readding label for cherry-pick

… none of ECMP members are active/link-up (sonic-net#3394) What I did: Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up. Also do not program the Temp Route if Neigh is not active (Link Down) Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case. Why I did: In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable. Also in this case no point to add Temp Route if neighbor is link down.

mssonicbld · 2024-12-24T17:23:23Z

Cherry-pick PR to 202405: #3442

… none of ECMP members are active/link-up (#3394) What I did: Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up. Also do not program the Temp Route if Neigh is not active (Link Down) Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case. Why I did: In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable. Also in this case no point to add Temp Route if neighbor is link down.

stephenxs · 2025-01-20T14:09:38Z

orchagent/routeorch.cpp

    {
-        SWSS_LOG_INFO("Failed to find route entry, vrf_id 0x%" PRIx64 ", prefix %s\n", vrf_id,
-                ipPrefix.to_string().c_str());
+       if (it_route_table->second.size() == 0)


I suspect it's unsafe to remove m_syncdRoutes[vrf_id] here.
gRouteBulker.creating_entries_count(route_entry) can give unexpected result. it just counts the number of certain prefix that are pending creation.
however, if no such prefix but other prefixes belong to the VRF pending creation, it still returns 0 and the VRF routing table is removed.
After that, once the bulk returns, it can not find the VRF table when handling the routing entries, which causes an exception.

We saw the following uncaught C++ exception

(gdb) bt #0 0x00007f5791aedebc in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007f5791a9efb2 in raise () from /lib/x86_64-linux-gnu/libc.so.6 #2 0x00007f5791a89472 in abort () from /lib/x86_64-linux-gnu/libc.so.6 #3 0x00007f5791de0919 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6 #4 0x00007f5791debe1a in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6 #5 0x00007f5791debe85 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6 #6 0x00007f5791dec0d8 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6 #7 0x00007f5791de3240 in std::__throw_out_of_range(char const*) () from /lib/x86_64-linux-gnu/libstdc++.so.6 #8 0x00005594e856d956 in std::map<unsigned long, std::map<swss::IpPrefix, RouteNhg, std::less<swss::IpPrefix>, std::allocator<std::pair<swss::IpPrefix const, RouteNhg> > >, std::less<unsigned long>, std::allocator<std::pair<unsigned long const, std::map<swss::IpPrefix, RouteNhg, std::less<swss::IpPrefix>, std::allocator<std::pair<swss::IpPrefix const, RouteNhg> > > > > >::at (this=<optimized out>, __k=<optimized out>) at /usr/include/c++/12/bits/stl_map.h:551 #9 0x00005594e8564beb in RouteOrch::addRoutePost (this=this@entry=0x5594ea13e080, ctx=..., nextHops=...) at ./orchagent/routeorch.cpp:2145 #10 0x00005594e856b0b2 in RouteOrch::doTask (this=0x5594ea13e080, consumer=...) at ./orchagent/routeorch.cpp:1021 #11 0x00005594e85282d2 in Orch::doTask (this=0x5594ea13e080) at ./orchagent/orch.cpp:553 #12 0x00005594e851909a in OrchDaemon::start (this=this@entry=0x5594ea0a0950) at ./orchagent/orchdaemon.cpp:895 #13 0x00005594e8485632 in main (argc=<optimized out>, argv=<optimized out>) at ./orchagent/main.cpp:818 (gdb) frame 9 #9 0x00005594e8564beb in RouteOrch::addRoutePost (this=this@entry=0x5594ea13e080, ctx=..., nextHops=...) at ./orchagent/routeorch.cpp:2145

#3477 to fix it

… none of ECMP members are active/link-up (sonic-net#3394) What I did: Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up. Also do not program the Temp Route if Neigh is not active (Link Down) Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case. Why I did: In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable. Also in this case no point to add Temp Route if neighbor is link down.

mssonicbld · 2025-02-19T05:08:00Z

Cherry-pick PR to 202411: #3522

*What I did: Added Change to Skip Route Programming if NH is link/oper down. With Scale Route testing of 60K+ routes when we toggle all the interfaces[14+ interface back to back] as done here: https://github.com/sonic-net/sonic-mgmt/blob/master/tests/snappi_tests/multidut/bgp/test_bgp_outbound_uplink_multi_po_flap.py we see because of slowness of FRR Route APP_DB processing compare to Link Notification Handling where we have updated the Nexthop Group as part of Link Notification handling to point to default route via #3389 [if eligible] FRR slowness can reprogram the Route back to Nexthop which is link down. This change is similar to #3394 which was done for Nexthop Group.

… none of ECMP members are active/link-up (sonic-net#3394) What I did: Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up. Also do not program the Temp Route if Neigh is not active (Link Down) Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case. Why I did: In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable. Also in this case no point to add Temp Route if neighbor is link down.

…-net#3520) *What I did: Added Change to Skip Route Programming if NH is link/oper down. With Scale Route testing of 60K+ routes when we toggle all the interfaces[14+ interface back to back] as done here: https://github.com/sonic-net/sonic-mgmt/blob/master/tests/snappi_tests/multidut/bgp/test_bgp_outbound_uplink_multi_po_flap.py we see because of slowness of FRR Route APP_DB processing compare to Link Notification Handling where we have updated the Nexthop Group as part of Link Notification handling to point to default route via sonic-net#3389 [if eligible] FRR slowness can reprogram the Route back to Nexthop which is link down. This change is similar to sonic-net#3394 which was done for Nexthop Group.

… none of ECMP members are active/link-up (sonic-net#3394) What I did: Added change not to create ECMP Group in SAI and program the route if none of the ECMP members are active/link-up. Also do not program the Temp Route if Neigh is not active (Link Down) Also as part of this change if Route is not programmed and if we remove that route than decrement VRF Reference count in removeRoute as removeRoutePost will not be called in this case. Why I did: In scale setup of T2 it's possible all links can go down simultaneously which case we can get Route messages with all nexthops being in down state. In such case we might create empty Nexthop Group in SAI for the given route which causes not needed SAI call for Nexthop Group creation and also create traffic blackhole for the route where that route can still forward traffic via default route if eligible/applicable. Also in this case no point to add Temp Route if neighbor is link down. Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>

…-net#3520) *What I did: Added Change to Skip Route Programming if NH is link/oper down. With Scale Route testing of 60K+ routes when we toggle all the interfaces[14+ interface back to back] as done here: https://github.com/sonic-net/sonic-mgmt/blob/master/tests/snappi_tests/multidut/bgp/test_bgp_outbound_uplink_multi_po_flap.py we see because of slowness of FRR Route APP_DB processing compare to Link Notification Handling where we have updated the Nexthop Group as part of Link Notification handling to point to default route via sonic-net#3389 [if eligible] FRR slowness can reprogram the Route back to Nexthop which is link down. This change is similar to sonic-net#3394 which was done for Nexthop Group. Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>

Added change not to create ECMP Group in SAI and program the route if

76712fc

all the members of ECMP groups are inactive. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

abdosi requested a review from prsunny as a code owner November 26, 2024 02:49

abdosi requested review from prsunny and removed request for prsunny November 26, 2024 02:50

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

abdosi changed the title ~~Added change not to create ECMP Group in SAI and program the route if not of ECMP members are active/link-up~~ Added change not to create ECMP Group in SAI and program the route if none of ECMP members are active/link-up Nov 26, 2024

abdosi and others added 3 commits November 25, 2024 22:48

Update test_nhg.py

3517208

Merge branch 'master' into no-ecmp

8cb5b8e

Merge branch 'master' into no-ecmp

e957850

rlhui assigned abdosi Nov 27, 2024

abdosi and others added 3 commits November 28, 2024 19:46

Added change to interface status when adding temp route.

d186344

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Merge branch 'no-ecmp' of https://github.com/abdosi/sonic-swss into n…

2b8118d

…o-ecmp

Merge branch 'master' into no-ecmp

c3f5e82

abdosi and others added 3 commits December 6, 2024 13:15

fix

760b20b

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

Merge branch 'master' into no-ecmp

1e552e8

Merge branch 'master' into no-ecmp

6aadcfc

Modified subport_interface test case

f090fde

Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>

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

View reviewed changes

tests/test_sub_port_intf.py Fixed Show fixed Hide fixed

tests/test_sub_port_intf.py Fixed Show fixed Hide fixed

Merge branch 'master' into no-ecmp

8cf4e24

prsunny merged commit 8465c1d into sonic-net:master Dec 18, 2024

abdosi deleted the no-ecmp branch December 18, 2024 17:49

bingwang-ms added Approved for 202405 Branch and removed Approved for 202405 Branch labels Dec 18, 2024

mssonicbld mentioned this pull request Dec 24, 2024

[action] [PR:3394] Added change not to create ECMP Group in SAI and program the route if none of ECMP members are active/link-up (#3394) #3442

Merged

mssonicbld added the Created PR to 202405 Branch label Dec 24, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Dec 24, 2024

bingwang-ms mentioned this pull request Dec 28, 2024

[PR test blocker] PR test failed on kvmtest-multi-asic-t1-lag due to IPv6 BGP sessions are down sonic-net/sonic-buildimage#21294

Open

stephenxs reviewed Jan 20, 2025

View reviewed changes

stephenxs mentioned this pull request Feb 5, 2025

Avoid removing a VRF routing table when there are pending creation entries in gRouteBulker #3477

Merged

abdosi mentioned this pull request Feb 18, 2025

Added Change to Skip Route Programming if NH is link/oper down #3520

Merged

kperumalbfn added the Approved for 202411 Branch label Feb 19, 2025

mssonicbld mentioned this pull request Feb 19, 2025

[action] [PR:3394] Added change not to create ECMP Group in SAI and program the route if none of ECMP members are active/link-up #3522

Merged

mssonicbld added the Created PR to 202411 Branch label Feb 19, 2025

mssonicbld added Included in 202411 Branch and removed Created PR to 202411 Branch labels Feb 19, 2025

Conversation

abdosi commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlhui commented Nov 27, 2024

Uh oh!

prsunny commented Dec 3, 2024

Uh oh!

abdosi commented Dec 12, 2024

Uh oh!

azure-pipelines bot commented Dec 12, 2024

Uh oh!

Uh oh!

Uh oh!

abdosi commented Dec 12, 2024

Uh oh!

azure-pipelines bot commented Dec 12, 2024

Uh oh!

mssonicbld commented Dec 13, 2024

Uh oh!

azure-pipelines bot commented Dec 13, 2024

Uh oh!

mssonicbld commented Dec 13, 2024

Uh oh!

azure-pipelines bot commented Dec 13, 2024

Uh oh!

mssonicbld commented Dec 13, 2024

Uh oh!

bingwang-ms commented Dec 24, 2024

Uh oh!

mssonicbld commented Dec 24, 2024

Uh oh!

stephenxs Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenxs Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

abdosi commented Nov 26, 2024 •

edited

Loading

stephenxs Jan 20, 2025 •

edited

Loading