Skip to content

Commit 691c37b

Browse files
authored
[Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk (#2086)
**What I did** Fix 2 issues in route bulk operation in the scenario where a SET operation follows a DEL operation on the same prefix. 1. The route entry SET operation can be ignored if it follows a DEL operation with the same prefix and next-hop. 2. DEL and then SET default route results in orchagent aborted Added test case to cover the scenarios **How I verified it** Manually test Unit test **Details if related** ***The route entry SET operation can be ignored if it follows a DEL operation with the same prefix and next-hop.*** For example, the following operations ``` 2021-12-18.06:17:54.793234|ROUTE_TABLE:1.1.1.0/24|SET|nexthop:10.0.0.33|ifname:Ethernet64 2021-12-18.06:19:53.187294|ROUTE_TABLE:1.1.1.0/24|DEL 2021-12-18.06:20:10.279850|ROUTE_TABLE:1.1.1.0/24|SET|nexthop:10.0.0.33|ifname:Ethernet64 ``` result in the next sai operations ``` 2021-12-18.06:19:31.203092|C|SAI_OBJECT_TYPE_ROUTE_ENTRY||{"dest":"1.1.1.0/24","switch_id":"oid:0x21000000000000","vr":"oid:0x3000000000010"}|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x4000000000aae 2021-12-18.06:20:23.813475|R|SAI_OBJECT_TYPE_ROUTE_ENTRY||{"dest":"1.1.1.0/24","switch_id":"oid:0x21000000000000","vr":"oid:0x3000000000010"} ``` and the last SET is lost. This is because: Both DEL and the 2nd SET operations were bundled into bulk operations. There is a logic that is not to program ASIC if the next-hop information in the route entry update isn't changed, which is done by comparing the next-hop information between route entry update and m_syncdRoutes. However, in bulk mode, a route entry that will be removed will not be removed from m_syncdRoutes until the bulk is flushed, The bulk has not been flushed when the 2nd SET is handled, which means the comparison returns "not changed" and the 2nd SET operation is ignored. The fix is to check whether the prefix is in gRouteBulker.removing_entries and treat it as non-exist if yes. ***DEL and then SET default route results in orchagent aborted*** This is because: 1. packet forward action is initialized when route entry is created 2. the default route will be set to DROP if the application asks for removing it 3. if the default route is readded later in the same bulk session, it doesn't update packet forward action but provides a valid next-hop 4. SAI complains if both DROP and a valid next hop are provided Solution: Always set packet action for the default route
1 parent a4c80c3 commit 691c37b

File tree

5 files changed

+452
-0
lines changed

5 files changed

+452
-0
lines changed

orchagent/routeorch.cpp

+25
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,11 @@ void RouteOrch::doTask(Consumer& consumer)
790790
}
791791
}
792792

793+
sai_route_entry_t route_entry;
794+
route_entry.vr_id = vrf_id;
795+
route_entry.switch_id = gSwitchId;
796+
copy(route_entry.destination, ip_prefix);
797+
793798
if (nhg.getSize() == 1 && nhg.hasIntfNextHop())
794799
{
795800
if (alsv[0] == "unknown")
@@ -833,6 +838,7 @@ void RouteOrch::doTask(Consumer& consumer)
833838
else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() ||
834839
m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() ||
835840
m_syncdRoutes.at(vrf_id).at(ip_prefix) != RouteNhg(nhg, ctx.nhg_index) ||
841+
gRouteBulker.bulk_entry_pending_removal(route_entry) ||
836842
ctx.using_temp_nhg)
837843
{
838844
if (addRoute(ctx, nhg))
@@ -1874,6 +1880,25 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
18741880
}
18751881
else
18761882
{
1883+
if (!blackhole && vrf_id == gVirtualRouterId && ipPrefix.isDefaultRoute())
1884+
{
1885+
// Always set packet action for default route to avoid conflict settings
1886+
// in case a SET follows a DEL on the default route in the same bulk.
1887+
// - On DEL default route, the packet action will be set to DROP
1888+
// - On SET default route, as the default route has NOT been removed from m_syncdRoute
1889+
// it calls SAI set_route_attributes instead of crate_route
1890+
// However, packet action is called only when a route entry is created
1891+
// This leads to conflict settings:
1892+
// - packet action: DROP
1893+
// - next hop: a valid next hop id
1894+
// To avoid this, we always set packet action for default route.
1895+
route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION;
1896+
route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD;
1897+
1898+
object_statuses.emplace_back();
1899+
gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
1900+
}
1901+
18771902
route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
18781903
route_attr.value.oid = next_hop_id;
18791904

tests/mock_tests/Makefile.am

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ LDADD_GTEST = -L/usr/src/gtest
2323

2424
tests_SOURCES = aclorch_ut.cpp \
2525
portsorch_ut.cpp \
26+
routeorch_ut.cpp \
2627
saispy_ut.cpp \
2728
consumer_ut.cpp \
2829
ut_saihelper.cpp \

tests/mock_tests/mock_orchagent_main.h

+6
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
#include "policerorch.h"
1717
#include "fgnhgorch.h"
1818
#include "flexcounterorch.h"
19+
#include "tunneldecaporch.h"
20+
#include "muxorch.h"
21+
#include "nhgorch.h"
1922
#include "directory.h"
2023

2124
extern int gBatchSize;
@@ -44,6 +47,8 @@ extern FdbOrch *gFdbOrch;
4447
extern MirrorOrch *gMirrorOrch;
4548
extern BufferOrch *gBufferOrch;
4649
extern VRFOrch *gVrfOrch;
50+
extern NhgOrch *gNhgOrch;
51+
extern Srv6Orch *gSrv6Orch;
4752
extern Directory<Orch*> gDirectory;
4853

4954
extern sai_acl_api_t *sai_acl_api;
@@ -62,3 +67,4 @@ extern sai_hostif_api_t *sai_hostif_api;
6267
extern sai_buffer_api_t *sai_buffer_api;
6368
extern sai_queue_api_t *sai_queue_api;
6469
extern sai_udf_api_t* sai_udf_api;
70+
extern sai_mpls_api_t* sai_mpls_api;

0 commit comments

Comments
 (0)