From 5d5c1692198c5c9c3b43f79406f074d2831c0884 Mon Sep 17 00:00:00 2001
From: Shi Su <67605788+shi-su@users.noreply.github.com>
Date: Mon, 13 Dec 2021 22:50:14 -0800
Subject: [PATCH] [bulk mode] Fix bulk conflict when in case there are both
 remove and set operations (#2071)

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

Fixes Azure/sonic-buildimage#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.
---
 orchagent/bulker.h             |  5 +++++
 orchagent/mplsrouteorch.cpp    |  6 +++++-
 orchagent/routeorch.cpp        |  6 +++++-
 tests/mock_tests/bulker_ut.cpp | 36 ++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/orchagent/bulker.h b/orchagent/bulker.h
index a4a49b105da1..2ff86644acb3 100644
--- a/orchagent/bulker.h
+++ b/orchagent/bulker.h
@@ -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 ->
diff --git a/orchagent/mplsrouteorch.cpp b/orchagent/mplsrouteorch.cpp
index 122bb6e8e166..ef40987a19de 100644
--- a/orchagent/mplsrouteorch.cpp
+++ b/orchagent/mplsrouteorch.cpp
@@ -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)
diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp
index d3a48cb5aa06..dc9928bee81b 100644
--- a/orchagent/routeorch.cpp
+++ b/orchagent/routeorch.cpp
@@ -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)
         {
diff --git a/tests/mock_tests/bulker_ut.cpp b/tests/mock_tests/bulker_ut.cpp
index a2cdaa07a300..6210cc0969d3 100644
--- a/tests/mock_tests/bulker_ut.cpp
+++ b/tests/mock_tests/bulker_ut.cpp
@@ -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));
+    }
 }