Keep attribute order in bulk mode

orchagent/bulker.h

@@ -208,10 +208,10 @@ class EntityBulker
         if (found_setting != setting_entries.end())
         {
             // Mark old one as done
-            auto& attrmap = found_setting->second;
-            for (auto& attr: attrmap)
+            auto& attrs = found_setting->second;
+            for (auto& attr: attrs)
             {
-                *attr.second.second = SAI_STATUS_SUCCESS;
+                *attr.second = SAI_STATUS_SUCCESS;
             }
             // Erase old one
             setting_entries.erase(found_setting);
@@ -252,27 +252,15 @@ class EntityBulker
         if (!attr) throw std::invalid_argument("attr is null");
 
         // Insert or find the key (entry)
-        auto& attrmap = setting_entries.emplace(std::piecewise_construct,
+        auto& attrs = setting_entries.emplace(std::piecewise_construct,
                 std::forward_as_tuple(*entry),
                 std::forward_as_tuple()
         ).first->second;
 
-        // Insert or find the key (attr)
-        auto rc = attrmap.emplace(std::piecewise_construct,
-                std::forward_as_tuple(attr->id),
-                std::forward_as_tuple());
-        bool inserted = rc.second;
-        auto it = rc.first;
-
-        // If inserted new key, assign the attr
-        // If found existing key, overwrite the old attr
-        it->second.first = *attr;
-        if (!inserted)
-        {
-            // If found existing key, mark old status as success
-            *it->second.second = SAI_STATUS_SUCCESS;
-        }
-        it->second.second = object_status;
+        // Insert attr
+        attrs.emplace_back(std::piecewise_construct,
+                std::forward_as_tuple(*attr),
+                std::forward_as_tuple(object_status));
         *object_status = SAI_STATUS_NOT_EXECUTED;
     }
 
@@ -351,19 +339,21 @@ class EntityBulker
         {
             std::vector<Te> rs;
             std::vector<sai_attribute_t> ts;
+            std::vector<sai_status_t*> status_vector;
 
             for (auto const& i: setting_entries)
             {
                 auto const& entry = i.first;
-                auto const& attrmap = i.second;
-                for (auto const& ia: attrmap)
+                auto const& attrs = i.second;
+                for (auto const& ia: attrs)
                 {
-                    auto const& attr = ia.second.first;
-                    sai_status_t *object_status = ia.second.second;
+                    auto const& attr = ia.first;
+                    sai_status_t *object_status = ia.second;
                     if (*object_status == SAI_STATUS_NOT_EXECUTED)
                     {
                         rs.push_back(entry);
                         ts.push_back(attr);
+                        status_vector.push_back(object_status);
                     }
                 }
             }
@@ -375,9 +365,7 @@ class EntityBulker
 
             for (size_t ir = 0; ir < count; ir++)
             {
-                auto& entry = rs[ir];
-                auto& attr_id = ts[ir].id;
-                sai_status_t *object_status = setting_entries[entry][attr_id].second;
+                sai_status_t *object_status = status_vector[ir];
                 if (object_status)
                 {
                     SWSS_LOG_INFO("EntityBulker.flush setting_entries status[%zu]=%d(0x%8p)\n", ir, statuses[ir], object_status);
@@ -426,8 +414,7 @@ class EntityBulker
 
     std::unordered_map<                                     // A map of
             Te,                                             // entry ->
-            std::unordered_map<                             //     another map of
-                    sai_attr_id_t,                          //     attr_id ->
+            std::vector<                                    //     vector of attribute and status
                     std::pair<
                             sai_attribute_t,                //     (attr_value, OUT object_status)
                             sai_status_t *

tests/mock_tests/Makefile.am

@@ -31,6 +31,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 mock_table.cpp \
                 mock_hiredis.cpp \
                 mock_redisreply.cpp \
+                bulker_ut.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \
                 $(top_srcdir)/orchagent/orchdaemon.cpp \
                 $(top_srcdir)/orchagent/orch.cpp \

tests/mock_tests/bulker_ut.cpp

@@ -0,0 +1,106 @@
+#include "ut_helper.h"
+#include "bulker.h"
+
+extern sai_route_api_t *sai_route_api;
+
+namespace bulker_test
+{
+    using namespace std;
+
+    struct BulkerTest : public ::testing::Test
+    {
+        BulkerTest()
+        {
+        }
+
+        void SetUp() override
+        {
+            ASSERT_EQ(sai_route_api, nullptr);
+            sai_route_api = new sai_route_api_t();
+        }
+
+        void TearDown() override
+        {
+            delete sai_route_api;
+            sai_route_api = nullptr;
+        }
+    };
+
+    TEST_F(BulkerTest, BulkerAttrOrder)
+    {
+        // Create bulker
+        EntityBulker<sai_route_api_t> gRouteBulker(sai_route_api);
+        deque<sai_status_t> object_statuses;
+
+        // Create a dummy route entry
+        sai_route_entry_t route_entry;
+        route_entry.destination.addr_family = SAI_IP_ADDR_FAMILY_IPV4;
+        route_entry.destination.addr.ip4 = htonl(0x0a00000f);
+        route_entry.destination.mask.ip4 = htonl(0xffffff00);
+        route_entry.vr_id = 0x0;
+        route_entry.switch_id = 0x0;
+
+        // Set packet action for route first
+        sai_attribute_t route_attr;
+        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);
+
+        // Set next hop for route
+        route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
+        route_attr.value.oid = SAI_NULL_OBJECT_ID;
+
+        object_statuses.emplace_back();
+        gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
+
+        // Check number of routes in bulk
+        ASSERT_EQ(gRouteBulker.setting_entries_count(), 1);
+
+        // Confirm the order of attributes in bulk is the same as being set
+        auto const& attrs = gRouteBulker.setting_entries[route_entry];
+        ASSERT_EQ(attrs.size(), 2);
+        auto ia = attrs.begin();
+        ASSERT_EQ(ia->first.id, SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION);
+        ASSERT_EQ(ia->first.value.s32, SAI_PACKET_ACTION_FORWARD);
+        ia++;
+        ASSERT_EQ(ia->first.id, SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID);
+        ASSERT_EQ(ia->first.value.oid, SAI_NULL_OBJECT_ID);
+
+        // Clear the bulk
+        gRouteBulker.clear();
+        object_statuses.clear();
+
+        // Check the bulker has been cleared
+        ASSERT_EQ(gRouteBulker.setting_entries_count(), 0);
+
+        // Test the inverse order
+        // Set next hop for route first
+        route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID;
+        route_attr.value.oid = SAI_NULL_OBJECT_ID;
+
+        object_statuses.emplace_back();
+        gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr);
+
+        // Set packet action for 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);
+
+        // Check number of routes in bulk
+        ASSERT_EQ(gRouteBulker.setting_entries_count(), 1);
+
+        // Confirm the order of attributes in bulk is the same as being set
+        auto const& attrs_reverse = gRouteBulker.setting_entries[route_entry];
+        ASSERT_EQ(attrs_reverse.size(), 2);
+        ia = attrs_reverse.begin();
+        ASSERT_EQ(ia->first.id, SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID);
+        ASSERT_EQ(ia->first.value.oid, SAI_NULL_OBJECT_ID);
+        ia++;
+        ASSERT_EQ(ia->first.id, SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION);
+        ASSERT_EQ(ia->first.value.s32, SAI_PACKET_ACTION_FORWARD);
+    }
+}