Skip to content

[Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk #2086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 22, 2021
Merged

[Route bulk] Fix bugs in case a SET operation follows a DEL operation in the same bulk #2086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d01b399
Fix route orch bulk issue in DEL-SET pattern
stephenxs Dec 18, 2021
5ded00a
Fix issue: del and then set default route results in orchagent aborted
stephenxs Dec 19, 2021
ea3c082
Add unit test to cover the bulk scenarios
stephenxs Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions orchagent/routeorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,11 @@ void RouteOrch::doTask(Consumer& consumer)
}
}

sai_route_entry_t route_entry;
route_entry.vr_id = vrf_id;
route_entry.switch_id = gSwitchId;
copy(route_entry.destination, ip_prefix);

if (nhg.getSize() == 1 && nhg.hasIntfNextHop())
{
if (alsv[0] == "unknown")
Expand Down Expand Up @@ -833,6 +838,7 @@ void RouteOrch::doTask(Consumer& consumer)
else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() ||
m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() ||
m_syncdRoutes.at(vrf_id).at(ip_prefix) != RouteNhg(nhg, ctx.nhg_index) ||
gRouteBulker.bulk_entry_pending_removal(route_entry) ||
ctx.using_temp_nhg)
{
if (addRoute(ctx, nhg))
Expand Down Expand Up @@ -1874,6 +1880,25 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
}
else
{
if (!blackhole && vrf_id == gVirtualRouterId && ipPrefix.isDefaultRoute())
{
// Always set packet action for default route to avoid conflict settings
// in case a SET follows a DEL on the default route in the same bulk.
// - On DEL default route, the packet action will be set to DROP
// - On SET default route, as the default route has NOT been removed from m_syncdRoute
// it calls SAI set_route_attributes instead of crate_route
// However, packet action is called only when a route entry is created
// This leads to conflict settings:
// - packet action: DROP
// - next hop: a valid next hop id
// To avoid this, we always set packet action for default route.
route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION;
route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD;

object_statuses.emplace_back();
gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
}

route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
route_attr.value.oid = next_hop_id;

Expand Down
1 change: 1 addition & 0 deletions tests/mock_tests/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ LDADD_GTEST = -L/usr/src/gtest

tests_SOURCES = aclorch_ut.cpp \
portsorch_ut.cpp \
routeorch_ut.cpp \
saispy_ut.cpp \
consumer_ut.cpp \
ut_saihelper.cpp \
Expand Down
6 changes: 6 additions & 0 deletions tests/mock_tests/mock_orchagent_main.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
#include "policerorch.h"
#include "fgnhgorch.h"
#include "flexcounterorch.h"
#include "tunneldecaporch.h"
#include "muxorch.h"
#include "nhgorch.h"
#include "directory.h"

extern int gBatchSize;
Expand Down Expand Up @@ -44,6 +47,8 @@ extern FdbOrch *gFdbOrch;
extern MirrorOrch *gMirrorOrch;
extern BufferOrch *gBufferOrch;
extern VRFOrch *gVrfOrch;
extern NhgOrch *gNhgOrch;
extern Srv6Orch *gSrv6Orch;
extern Directory<Orch*> gDirectory;

extern sai_acl_api_t *sai_acl_api;
Expand All @@ -62,3 +67,4 @@ extern sai_hostif_api_t *sai_hostif_api;
extern sai_buffer_api_t *sai_buffer_api;
extern sai_queue_api_t *sai_queue_api;
extern sai_udf_api_t* sai_udf_api;
extern sai_mpls_api_t* sai_mpls_api;
Loading