Keep attribute order in bulk mode (#1659)

Keep attributes for the same entry in the order that the attributes are set in bulk mode. And add a unit test for the scenario in bulker.

Some attributes have a dependency on others in SAI operations (e.g., setting nexthop requires packet action to be forward in route set operation). Without keeping the order of the attributes, the configuration may not be successfully applied to hardware. Therefore, it is necessary to keep the order of attributes 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);
+    }
+}