diff --git a/orchagent/bulker.h b/orchagent/bulker.h index e3e464c62dc5..4f6e9cc5cb2e 100644 --- a/orchagent/bulker.h +++ b/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 rs; std::vector ts; + std::vector 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 * diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 050cd5274c76..80f9e7173e4e 100644 --- a/tests/mock_tests/Makefile.am +++ b/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 \ diff --git a/tests/mock_tests/bulker_ut.cpp b/tests/mock_tests/bulker_ut.cpp new file mode 100644 index 000000000000..1cdabcdd148b --- /dev/null +++ b/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 gRouteBulker(sai_route_api); + deque 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); + } +}