Skip to content

Commit

Permalink
[bulk mode] Fix bulk conflict when in case there are both remove and …
Browse files Browse the repository at this point in the history
…set operations (#2071)

What I did
Check if there are items pending removal in bulk before calling bulk set API.

Fixes #9434

Why I did it
When there are items pending removal in bulk before calling set API, it means the item will be removed before the set and it should do create instead.
  • Loading branch information
shi-su committed Dec 14, 2021
1 parent 8bbdbd2 commit 5d5c169
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 2 deletions.
5 changes: 5 additions & 0 deletions orchagent/bulker.h
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,11 @@ class EntityBulker
return creating_entries.count(entry);
}

bool bulk_entry_pending_removal(const Te& entry) const
{
return removing_entries.find(entry) != removing_entries.end();
}

private:
std::unordered_map< // A map of
Te, // entry ->
Expand Down
6 changes: 5 additions & 1 deletion orchagent/mplsrouteorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,12 @@ bool RouteOrch::addLabelRoute(LabelRouteBulkContext& ctx, const NextHopGroupKey
* in m_syncdLabelRoutes, then we need to update the route with a new next hop
* (group) id. The old next hop (group) is then not used and the reference
* count will decrease by 1.
*
* In case the entry is already pending removal in the bulk, it would be removed
* from m_syncdLabelRoutes during the bulk call. Therefore, such entries need to be
* re-created rather than set attribute.
*/
if (it_route == m_syncdLabelRoutes.at(vrf_id).end())
if (it_route == m_syncdLabelRoutes.at(vrf_id).end() || gLabelRouteBulker.bulk_entry_pending_removal(inseg_entry))
{
vector<sai_attribute_t> inseg_attrs;
if (blackhole)
Expand Down
6 changes: 5 additions & 1 deletion orchagent/routeorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1826,8 +1826,12 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
* in m_syncdRoutes, then we need to update the route with a new next hop
* (group) id. The old next hop (group) is then not used and the reference
* count will decrease by 1.
*
* In case the entry is already pending removal in the bulk, it would be removed
* from m_syncdRoutes during the bulk call. Therefore, such entries need to be
* re-created rather than set attribute.
*/
if (it_route == m_syncdRoutes.at(vrf_id).end())
if (it_route == m_syncdRoutes.at(vrf_id).end() || gRouteBulker.bulk_entry_pending_removal(route_entry))
{
if (blackhole)
{
Expand Down
36 changes: 36 additions & 0 deletions tests/mock_tests/bulker_ut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,40 @@ namespace bulker_test
ASSERT_EQ(ia->first.id, SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION);
ASSERT_EQ(ia->first.value.s32, SAI_PACKET_ACTION_FORWARD);
}

TEST_F(BulkerTest, BulkerPendindRemoval)
{
// Create bulker
EntityBulker<sai_route_api_t> gRouteBulker(sai_route_api, 1000);
deque<sai_status_t> object_statuses;

// Check max bulk size
ASSERT_EQ(gRouteBulker.max_bulk_size, 1000);

// Create a dummy route entry
sai_route_entry_t route_entry_remove;
route_entry_remove.destination.addr_family = SAI_IP_ADDR_FAMILY_IPV4;
route_entry_remove.destination.addr.ip4 = htonl(0x0a00000f);
route_entry_remove.destination.mask.ip4 = htonl(0xffffff00);
route_entry_remove.vr_id = 0x0;
route_entry_remove.switch_id = 0x0;

// Put route entry into remove
object_statuses.emplace_back();
gRouteBulker.remove_entry(&object_statuses.back(), &route_entry_remove);

// Confirm route entry is pending removal
ASSERT_TRUE(gRouteBulker.bulk_entry_pending_removal(route_entry_remove));

// Create another dummy route entry that will not be removed
sai_route_entry_t route_entry_non_remove;
route_entry_non_remove.destination.addr_family = SAI_IP_ADDR_FAMILY_IPV4;
route_entry_non_remove.destination.addr.ip4 = htonl(0x0a00010f);
route_entry_non_remove.destination.mask.ip4 = htonl(0xffffff00);
route_entry_non_remove.vr_id = 0x0;
route_entry_non_remove.switch_id = 0x0;

// Confirm route entry is not pending removal
ASSERT_FALSE(gRouteBulker.bulk_entry_pending_removal(route_entry_non_remove));
}
}

0 comments on commit 5d5c169

Please sign in to comment.