From 8295efa51309f4005a138463179ecd52e423c479 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 20 Mar 2020 01:44:18 +0000 Subject: [PATCH 01/42] (temp1) --- orchagent/bulker.h | 531 ++++++++++++++++++++++++++++++++++++++++ orchagent/routeorch.cpp | 3 + 2 files changed, 534 insertions(+) create mode 100644 orchagent/bulker.h diff --git a/orchagent/bulker.h b/orchagent/bulker.h new file mode 100644 index 0000000000..db19dd7e28 --- /dev/null +++ b/orchagent/bulker.h @@ -0,0 +1,531 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include "sai.h" + +/* +static inline bool operator==(const sai_ip_prefix_t& a, const sai_ip_prefix_t& b) +{ + if (a.addr_family != b.addr_family) return false; + + if (a.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + { + return a.addr.ip4 == b.addr.ip4 + && a.mask.ip4 == b.mask.ip4 + ; + } + else if (a.addr_family == SAI_IP_ADDR_FAMILY_IPV6) + { + return memcmp(a.addr.ip6, b.addr.ip6, sizeof(a.addr.ip6)) == 0 + && memcmp(a.mask.ip6, b.mask.ip6, sizeof(a.mask.ip6)) == 0 + ; + } + else + { + throw std::invalid_argument("a has invalid addr_family"); + } +} + +static inline bool operator==(const sai_route_entry_t& a, const sai_route_entry_t& b) +{ + return a.switch_id == b.switch_id + && a.vr_id == b.vr_id + && a.destination == b.destination + ; +} +*/ + +static inline std::size_t hash_value(const sai_ip_prefix_t& a) +{ + size_t seed = 0; + boost::hash_combine(seed, a.addr_family); + if (a.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + { + boost::hash_combine(seed, a.addr.ip4); + boost::hash_combine(seed, a.mask.ip4); + } + else if (a.addr_family == SAI_IP_ADDR_FAMILY_IPV6) + { + boost::hash_combine(seed, a.addr.ip6); + boost::hash_combine(seed, a.mask.ip6); + } + return seed; +} + +namespace std +{ + template <> + struct hash + { + size_t operator()(const sai_route_entry_t& a) const noexcept + { + size_t seed = 0; + boost::hash_combine(seed, a.switch_id); + boost::hash_combine(seed, a.vr_id); + boost::hash_combine(seed, a.destination); + return seed; + } + }; + + template <> + struct hash + { + size_t operator()(const sai_fdb_entry_t& a) const noexcept + { + size_t seed = 0; + boost::hash_combine(seed, a.switch_id); + boost::hash_combine(seed, a.mac_address); + boost::hash_combine(seed, a.bv_id); + return seed; + } + }; +} + +/* +struct NextHopGroupEntry +{ + sai_object_id_t next_hop_group_id; // next hop group id + std::set next_hop_group_members; // next hop group member ids + int ref_count; // reference count +}; +*/ + +/* NextHopGroupTable: next hop group IP addersses, NextHopGroupEntry */ +//typedef std::map NextHopGroupTable; + +// SAI typedef which is not available in SAI 1.5 +// TODO: remove after available +typedef sai_status_t (*sai_bulk_create_fdb_entry_fn)( + _In_ uint32_t object_count, + _In_ const sai_fdb_entry_t *fdb_entry, + _In_ const uint32_t *attr_count, + _In_ const sai_attribute_t **attr_list, + _In_ sai_bulk_op_error_mode_t mode, + _Out_ sai_status_t *object_statuses); +typedef sai_status_t (*sai_bulk_remove_fdb_entry_fn)( + _In_ uint32_t object_count, + _In_ const sai_fdb_entry_t *fdb_entry, + _In_ sai_bulk_op_error_mode_t mode, + _Out_ sai_status_t *object_statuses); +typedef sai_status_t (*sai_bulk_set_fdb_entry_attribute_fn)( + _In_ uint32_t object_count, + _In_ const sai_fdb_entry_t *fdb_entry, + _In_ const sai_attribute_t *attr_list, + _In_ sai_bulk_op_error_mode_t mode, + _Out_ sai_status_t *object_statuses); + +template +struct saitraits +{ +}; + +template<> +struct saitraits +{ + using api_t = sai_route_api_t; + using create_entry_fn = sai_create_route_entry_fn; + using remove_entry_fn = sai_remove_route_entry_fn; + using set_entry_attribute_fn = sai_set_route_entry_attribute_fn; + using bulk_create_entry_fn = sai_bulk_create_route_entry_fn; + using bulk_remove_entry_fn = sai_bulk_remove_route_entry_fn; + using bulk_set_entry_attribute_fn = sai_bulk_set_route_entry_attribute_fn; +}; + +template<> +struct saitraits +{ + using api_t = sai_fdb_api_t; + using create_entry_fn = sai_create_fdb_entry_fn; + using remove_entry_fn = sai_remove_fdb_entry_fn; + using set_entry_attribute_fn = sai_set_fdb_entry_attribute_fn; + using bulk_create_entry_fn = sai_bulk_create_fdb_entry_fn; + using bulk_remove_entry_fn = sai_bulk_remove_fdb_entry_fn; + using bulk_set_entry_attribute_fn = sai_bulk_set_fdb_entry_attribute_fn; +}; + +/* +template<> +struct saitraits +{ + using api_t = sai_fdb_api_t; + using create_entry_fn = sai_create_fdb_entry_fn; + using remove_entry_fn = sai_remove_fdb_entry_fn; + using set_entry_attribute_fn = sai_set_fdb_entry_attribute_fn; + using bulk_create_entry_fn = sai_bulk_create_fdb_entry_fn; + using bulk_remove_entry_fn = sai_bulk_remove_fdb_entry_fn; + using bulk_set_entry_attribute_fn = sai_bulk_set_fdb_entry_attribute_fn; +}; +*/ + +template > +class RouteBulker +{ +public: + RouteBulker(typename Ts::api_t *api/*, sai_object_id_t switch_id = SAI_NULL_OBJECT_ID*/); + + sai_status_t create_route_entry( + _In_ const T *route_entry, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) + { + creating_entries.emplace(std::piecewise_construct, + std::forward_as_tuple(*route_entry), + std::forward_as_tuple(attr_list, attr_list + attr_count)); + SWSS_LOG_DEBUG("bulk.create_route_entry %zu, %zu, %d\n", creating_entries.size(), creating_entries[*route_entry].size(), (int)creating_entries[*route_entry][0].id); + return SAI_STATUS_SUCCESS; + } + + sai_status_t remove_route_entry( + _In_ const T *route_entry) + { + assert(route_entry); + if (!route_entry) throw std::invalid_argument("route_entry is null"); + + auto found_setting = setting_entries.find(*route_entry); + if (found_setting != setting_entries.end()) + { + setting_entries.erase(found_setting); + } + + auto found_creating = creating_entries.find(*route_entry); + if (found_creating != creating_entries.end()) + { + creating_entries.erase(found_creating); + } + else + { + removing_entries.emplace(*route_entry); + } + + return SAI_STATUS_SUCCESS; + } + + sai_status_t set_route_entry_attribute( + _In_ const T *route_entry, + _In_ const sai_attribute_t *attr) + { + auto found_setting = setting_entries.find(*route_entry); + if (found_setting != setting_entries.end()) + { + // For simplicity, just insert new attribute at the vector end, no merging + found_setting->second.emplace_back(*attr); + } + else + { + // Create a new key if not exists in the map + setting_entries.emplace(std::piecewise_construct, + std::forward_as_tuple(*route_entry), + std::forward_as_tuple(1, *attr)); + } + + return SAI_STATUS_SUCCESS; + } + + void flush() + { + // Removing + if (!removing_entries.empty()) + { + vector rs; + + for (auto i: removing_entries) + { + auto& route_entry = i; + rs.push_back(route_entry); + } + uint32_t route_count = (uint32_t)removing_entries.size(); + vector statuses(route_count); + (*remove_route_entries)(route_count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + + SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); + + removing_entries.clear(); + } + + // Creating + if (!creating_entries.empty()) + { + vector rs; + vector tss; + vector cs; + + for (auto const& i: creating_entries) + { + auto const& route_entry = i.first; + auto const& attrs = i.second; + + rs.push_back(route_entry); + tss.push_back(attrs.data()); + cs.push_back((uint32_t)attrs.size()); + } + uint32_t route_count = (uint32_t)creating_entries.size(); + vector statuses(route_count); + (*create_route_entries)(route_count, rs.data(), cs.data(), tss.data() + , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + + SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); + + creating_entries.clear(); + } + + // Setting + if (!setting_entries.empty()) + { + vector rs; + vector ts; + vector cs; + + for (auto const& i: setting_entries) + { + auto const& route_entry = i.first; + auto const& attrs = i.second; + for (auto const& attr: attrs) + { + rs.push_back(route_entry); + ts.push_back(attr); + } + } + uint32_t route_count = (uint32_t)setting_entries.size(); + vector statuses(route_count); + (*set_route_entries_attribute)(route_count, rs.data(), ts.data() + , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + + SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); + + setting_entries.clear(); + } + } + + void clear() + { + removing_entries.clear(); + creating_entries.clear(); + setting_entries.clear(); + } + +private: + sai_object_id_t switch_id = SAI_NULL_OBJECT_ID; + std::unordered_map> creating_entries; + std::unordered_map> setting_entries; + std::unordered_set removing_entries; + + typename Ts::bulk_create_entry_fn create_route_entries; + typename Ts::bulk_remove_entry_fn remove_route_entries; + typename Ts::bulk_set_entry_attribute_fn set_route_entries_attribute; +}; + +template +RouteBulker::RouteBulker(typename Ts::api_t *api) +{ + throw std::logic_error("Not implemented"); +} + +template <> +RouteBulker::RouteBulker(saitraits::api_t *api) +{ + create_route_entries = api->create_route_entries; + remove_route_entries = api->remove_route_entries; + set_route_entries_attribute = api->set_route_entries_attribute; +} + + +template <> +RouteBulker::RouteBulker(saitraits::api_t *api) +{ + // TODO: implement after create_fdb_entries() is available in SAI + throw std::logic_error("Not implemented"); + /* + create_route_entries = api->create_fdb_entries; + remove_route_entries = api->remove_fdb_entries; + set_route_entries_attribute = api->set_fdb_entries_attribute; + */ +} + +#if 0 +class NextHopGroupBulker +{ +public: + NextHopGroupBulker(sai_next_hop_group_api_t* next_hop_group_api, RouteBulker* routebulker, int maxNextHopGroupCount, sai_object_id_t switchId) + : sai_next_hop_group_api(next_hop_group_api) + , m_nextHopGroupCount(0) + , route_bulker(routebulker) + , m_maxNextHopGroupCount(maxNextHopGroupCount) + , m_switchId(switchId) + { + } + + bool hasNextHopGroup(const IpAddresses& ipAddresses) const + { + return m_syncdNextHopGroups.find(ipAddresses) != m_syncdNextHopGroups.end(); + } + + sai_object_id_t getNextHopGroupId(const IpAddresses& ipAddresses) + { + assert(hasNextHopGroup(ipAddresses)); + return m_syncdNextHopGroups[ipAddresses].next_hop_group_id; + } + + void increaseNextHopRefCount(const IpAddresses& ipAddresses) + { + assert(ipAddresses.getSize() > 1); + m_syncdNextHopGroups[ipAddresses].ref_count ++; + } + void decreaseNextHopRefCount(const IpAddresses& ipAddresses) + { + assert(ipAddresses.getSize() > 1); + m_syncdNextHopGroups[ipAddresses].ref_count --; + } + + bool isRefCounterZero(const IpAddresses& ipAddresses) const + { + if (!hasNextHopGroup(ipAddresses)) + { + return true; + } + + return m_syncdNextHopGroups.at(ipAddresses).ref_count == 0; + } + + bool addNextHopGroup(const IpAddresses& ipAddresses, const vector& next_hop_ids) + { + if (m_nextHopGroupCount >= m_maxNextHopGroupCount) + { + SWSS_LOG_DEBUG("Failed to create new next hop group. \ + Reaching maximum number of next hop groups."); + return false; + } + + route_bulker->flush(); + + sai_attribute_t nhg_attr; + vector nhg_attrs; + + nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE; + nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_ECMP; + nhg_attrs.push_back(nhg_attr); + + sai_object_id_t next_hop_group_id; + sai_status_t status = sai_next_hop_group_api-> + create_next_hop_group(&next_hop_group_id, m_switchId, (uint32_t)nhg_attrs.size(), nhg_attrs.data()); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to create next hop group %s, rv:%d", + ipAddresses.to_string().c_str(), status); + return false; + } + + m_nextHopGroupCount ++; + SWSS_LOG_NOTICE("Create next hop group %s", ipAddresses.to_string().c_str()); + + NextHopGroupEntry next_hop_group_entry; + next_hop_group_entry.next_hop_group_id = next_hop_group_id; + + uint32_t object_count = (uint32_t)next_hop_ids.size(); + vector attr_count; + vector> attrs; + vector object_id(object_count); + vector object_statuses(object_count); + + // Create a next hop group members + for (auto nhid: next_hop_ids) + { + attrs.emplace_back(); + auto& nhgm_attrs = attrs.back(); + + sai_attribute_t nhgm_attr; + nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID; + nhgm_attr.value.oid = next_hop_group_id; + nhgm_attrs.push_back(nhgm_attr); + + nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_ID; + nhgm_attr.value.oid = nhid; + nhgm_attrs.push_back(nhgm_attr); + + attr_count.push_back((uint32_t)nhgm_attrs.size()); + } + + vector attrs_array; + for (auto const& i: attrs) + { + attrs_array.push_back(i.data()); + } + status = sai_next_hop_group_api->create_next_hop_group_members(m_switchId, object_count, attr_count.data(), attrs_array.data() + , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, object_id.data(), object_statuses.data()); + + for (uint32_t idx = 0; idx < object_count; idx++) + { + sai_object_id_t next_hop_group_member_id = object_id[idx]; + if (status != SAI_STATUS_SUCCESS) + { + // TODO: do we need to clean up? + SWSS_LOG_ERROR("Failed to create next hop group %lx member %lx: %d\n", + next_hop_group_id, next_hop_group_member_id, status); + return false; + } + + } + // Save the membership into next hop structure + next_hop_group_entry.next_hop_group_members.insert(object_id.begin(), object_id.end()); + + /* + * Initialize the next hop group structure with ref_count as 0. This + * count will increase once the route is successfully syncd. + */ + next_hop_group_entry.ref_count = 0; + m_syncdNextHopGroups[ipAddresses] = next_hop_group_entry; + return true; + } + + bool removeNextHopGroup(const IpAddresses& ipAddresses) + { + sai_status_t status; + assert(hasNextHopGroup(ipAddresses)); + + route_bulker->flush(); + if (m_syncdNextHopGroups[ipAddresses].ref_count == 0) + { + auto next_hop_group_entry = m_syncdNextHopGroups[ipAddresses]; + sai_object_id_t next_hop_group_id = next_hop_group_entry.next_hop_group_id; + + for (auto next_hop_group_member_id: next_hop_group_entry.next_hop_group_members) + { + status = sai_next_hop_group_api->remove_next_hop_group_member(next_hop_group_member_id); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove next hop group member %lx, rv:%d", next_hop_group_member_id, status); + return false; + } + } + + sai_status_t status = sai_next_hop_group_api->remove_next_hop_group(next_hop_group_id); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to remove next hop group %lx, rv:%d", next_hop_group_id, status); + return false; + } + + m_nextHopGroupCount --; + m_syncdNextHopGroups.erase(ipAddresses); + return true; + } + else + { + return false; + } + } + +private: + int m_nextHopGroupCount; + int m_maxNextHopGroupCount; + NextHopGroupTable m_syncdNextHopGroups; + sai_next_hop_group_api_t * sai_next_hop_group_api; + RouteBulker * route_bulker; + sai_object_id_t m_switchId; +}; +#endif diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index eb28ff824b..e22b8276bb 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -4,6 +4,7 @@ #include "logger.h" #include "swssnet.h" #include "crmorch.h" +#include "bulker.h" extern sai_object_id_t gVirtualRouterId; extern sai_object_id_t gSwitchId; @@ -21,6 +22,8 @@ extern CrmOrch *gCrmOrch; const int routeorch_pri = 5; +RouteBulker t(sai_route_api); + RouteOrch::RouteOrch(DBConnector *db, string tableName, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch) : Orch(db, tableName, routeorch_pri), m_neighOrch(neighOrch), From fa744c868fba703befd342c84cdfaaa296b43659 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sun, 22 Mar 2020 00:32:52 +0000 Subject: [PATCH 02/42] (buildable) --- orchagent/bulker.h | 363 +++++++++++++++++++++------------------------ 1 file changed, 171 insertions(+), 192 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index db19dd7e28..56e9a272e7 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -120,9 +120,7 @@ typedef sai_status_t (*sai_bulk_set_fdb_entry_attribute_fn)( _Out_ sai_status_t *object_statuses); template -struct saitraits -{ -}; +struct saitraits { }; template<> struct saitraits @@ -148,68 +146,71 @@ struct saitraits using bulk_set_entry_attribute_fn = sai_bulk_set_fdb_entry_attribute_fn; }; -/* template<> struct saitraits { - using api_t = sai_fdb_api_t; - using create_entry_fn = sai_create_fdb_entry_fn; - using remove_entry_fn = sai_remove_fdb_entry_fn; - using set_entry_attribute_fn = sai_set_fdb_entry_attribute_fn; - using bulk_create_entry_fn = sai_bulk_create_fdb_entry_fn; - using bulk_remove_entry_fn = sai_bulk_remove_fdb_entry_fn; - using bulk_set_entry_attribute_fn = sai_bulk_set_fdb_entry_attribute_fn; + using api_t = sai_next_hop_group_api_t; + using create_entry_fn = sai_create_next_hop_group_member_fn; + using remove_entry_fn = sai_remove_next_hop_group_member_fn; + using set_entry_attribute_fn = sai_set_next_hop_group_member_attribute_fn; + using bulk_create_entry_fn = sai_bulk_object_create_fn; + using bulk_remove_entry_fn = sai_bulk_object_remove_fn; + // TODO: wait until available in SAI + //using bulk_set_entry_attribute_fn = sai_bulk_object_set_attribute_fn; }; -*/ template > class RouteBulker { public: - RouteBulker(typename Ts::api_t *api/*, sai_object_id_t switch_id = SAI_NULL_OBJECT_ID*/); + RouteBulker(typename Ts::api_t *api) + { + throw std::logic_error("Not implemented"); + } - sai_status_t create_route_entry( - _In_ const T *route_entry, + sai_status_t create_entry( + _In_ const T *entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { - creating_entries.emplace(std::piecewise_construct, - std::forward_as_tuple(*route_entry), + auto rc = creating_entries.emplace(std::piecewise_construct, + std::forward_as_tuple(*entry), std::forward_as_tuple(attr_list, attr_list + attr_count)); - SWSS_LOG_DEBUG("bulk.create_route_entry %zu, %zu, %d\n", creating_entries.size(), creating_entries[*route_entry].size(), (int)creating_entries[*route_entry][0].id); - return SAI_STATUS_SUCCESS; + bool inserted = rc.second; + SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), creating_entries[*entry].size(), (int)creating_entries[*entry][0].id, inserted); + return inserted ? SAI_STATUS_SUCCESS : SAI_STATUS_ITEM_ALREADY_EXISTS; } - sai_status_t remove_route_entry( - _In_ const T *route_entry) + sai_status_t remove_entry( + _In_ const T *entry) { - assert(route_entry); - if (!route_entry) throw std::invalid_argument("route_entry is null"); + assert(entry); + if (!entry) throw std::invalid_argument("entry is null"); - auto found_setting = setting_entries.find(*route_entry); + auto found_setting = setting_entries.find(*entry); if (found_setting != setting_entries.end()) { setting_entries.erase(found_setting); } - auto found_creating = creating_entries.find(*route_entry); + auto found_creating = creating_entries.find(*entry); if (found_creating != creating_entries.end()) { creating_entries.erase(found_creating); } else { - removing_entries.emplace(*route_entry); + removing_entries.emplace(*entry); } return SAI_STATUS_SUCCESS; } - sai_status_t set_route_entry_attribute( - _In_ const T *route_entry, + sai_status_t set_entry_attribute( + _In_ const T *entry, _In_ const sai_attribute_t *attr) { - auto found_setting = setting_entries.find(*route_entry); + auto found_setting = setting_entries.find(*entry); if (found_setting != setting_entries.end()) { // For simplicity, just insert new attribute at the vector end, no merging @@ -219,7 +220,7 @@ class RouteBulker { // Create a new key if not exists in the map setting_entries.emplace(std::piecewise_construct, - std::forward_as_tuple(*route_entry), + std::forward_as_tuple(*entry), std::forward_as_tuple(1, *attr)); } @@ -235,12 +236,12 @@ class RouteBulker for (auto i: removing_entries) { - auto& route_entry = i; - rs.push_back(route_entry); + auto& entry = i; + rs.push_back(entry); } uint32_t route_count = (uint32_t)removing_entries.size(); vector statuses(route_count); - (*remove_route_entries)(route_count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + (*remove_entries)(route_count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); @@ -256,16 +257,16 @@ class RouteBulker for (auto const& i: creating_entries) { - auto const& route_entry = i.first; + auto const& entry = i.first; auto const& attrs = i.second; - rs.push_back(route_entry); + rs.push_back(entry); tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } uint32_t route_count = (uint32_t)creating_entries.size(); vector statuses(route_count); - (*create_route_entries)(route_count, rs.data(), cs.data(), tss.data() + (*create_entries)(route_count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); @@ -278,21 +279,20 @@ class RouteBulker { vector rs; vector ts; - vector cs; for (auto const& i: setting_entries) { - auto const& route_entry = i.first; + auto const& entry = i.first; auto const& attrs = i.second; for (auto const& attr: attrs) { - rs.push_back(route_entry); + rs.push_back(entry); ts.push_back(attr); } } uint32_t route_count = (uint32_t)setting_entries.size(); vector statuses(route_count); - (*set_route_entries_attribute)(route_count, rs.data(), ts.data() + (*set_entries_attribute)(route_count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); @@ -309,28 +309,21 @@ class RouteBulker } private: - sai_object_id_t switch_id = SAI_NULL_OBJECT_ID; std::unordered_map> creating_entries; std::unordered_map> setting_entries; std::unordered_set removing_entries; - typename Ts::bulk_create_entry_fn create_route_entries; - typename Ts::bulk_remove_entry_fn remove_route_entries; - typename Ts::bulk_set_entry_attribute_fn set_route_entries_attribute; + typename Ts::bulk_create_entry_fn create_entries; + typename Ts::bulk_remove_entry_fn remove_entries; + typename Ts::bulk_set_entry_attribute_fn set_entries_attribute; }; -template -RouteBulker::RouteBulker(typename Ts::api_t *api) -{ - throw std::logic_error("Not implemented"); -} - template <> RouteBulker::RouteBulker(saitraits::api_t *api) { - create_route_entries = api->create_route_entries; - remove_route_entries = api->remove_route_entries; - set_route_entries_attribute = api->set_route_entries_attribute; + create_entries = api->create_route_entries; + remove_entries = api->remove_route_entries; + set_entries_attribute = api->set_route_entries_attribute; } @@ -340,192 +333,178 @@ RouteBulker::RouteBulker(saitraits::api_t *api // TODO: implement after create_fdb_entries() is available in SAI throw std::logic_error("Not implemented"); /* - create_route_entries = api->create_fdb_entries; - remove_route_entries = api->remove_fdb_entries; - set_route_entries_attribute = api->set_fdb_entries_attribute; + create_entries = api->create_fdb_entries; + remove_entries = api->remove_fdb_entries; + set_entries_attribute = api->set_fdb_entries_attribute; */ } -#if 0 +template > class NextHopGroupBulker { public: - NextHopGroupBulker(sai_next_hop_group_api_t* next_hop_group_api, RouteBulker* routebulker, int maxNextHopGroupCount, sai_object_id_t switchId) - : sai_next_hop_group_api(next_hop_group_api) - , m_nextHopGroupCount(0) - , route_bulker(routebulker) - , m_maxNextHopGroupCount(maxNextHopGroupCount) - , m_switchId(switchId) + struct object_entry { - } - - bool hasNextHopGroup(const IpAddresses& ipAddresses) const + sai_object_id_t *object_id; + vector attrs; + }; + + NextHopGroupBulker(typename Ts::api_t* next_hop_group_api, sai_object_id_t switch_id) { - return m_syncdNextHopGroups.find(ipAddresses) != m_syncdNextHopGroups.end(); + throw std::logic_error("Not implemented"); } - - sai_object_id_t getNextHopGroupId(const IpAddresses& ipAddresses) + + sai_status_t create_entry( + _Out_ sai_object_id_t *object_id, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) { - assert(hasNextHopGroup(ipAddresses)); - return m_syncdNextHopGroups[ipAddresses].next_hop_group_id; + creating_entries.emplace_back(std::piecewise_construct, + std::forward_as_tuple(object_id), + std::forward_as_tuple(attr_list, attr_list + attr_count)); + SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), creating_entries.back().size(), (int)creating_entries.back()[0].id); + + *object_id = SAI_NULL_OBJECT_ID; // not created immediately, postponed until flush + return SAI_STATUS_SUCCESS; } - void increaseNextHopRefCount(const IpAddresses& ipAddresses) - { - assert(ipAddresses.getSize() > 1); - m_syncdNextHopGroups[ipAddresses].ref_count ++; - } - void decreaseNextHopRefCount(const IpAddresses& ipAddresses) + sai_status_t remove_entry( + _In_ sai_object_id_t object_id) { - assert(ipAddresses.getSize() > 1); - m_syncdNextHopGroups[ipAddresses].ref_count --; - } + assert(object_id != SAI_NULL_OBJECT_ID); + if (object_id == SAI_NULL_OBJECT_ID) throw std::invalid_argument("object_id is null"); - bool isRefCounterZero(const IpAddresses& ipAddresses) const - { - if (!hasNextHopGroup(ipAddresses)) + auto found_setting = setting_entries.find(object_id); + if (found_setting != setting_entries.end()) { - return true; + setting_entries.erase(found_setting); } - return m_syncdNextHopGroups.at(ipAddresses).ref_count == 0; + removing_entries.push_back(object_id); + return SAI_STATUS_SUCCESS; } - bool addNextHopGroup(const IpAddresses& ipAddresses, const vector& next_hop_ids) + // TODO: wait until available in SAI + /* + sai_status_t set_entry_attribute( + _In_ sai_object_id_t object_id, + _In_ const sai_attribute_t *attr) { - if (m_nextHopGroupCount >= m_maxNextHopGroupCount) + auto found_setting = setting_entries.find(object_id); + if (found_setting != setting_entries.end()) { - SWSS_LOG_DEBUG("Failed to create new next hop group. \ - Reaching maximum number of next hop groups."); - return false; + // For simplicity, just insert new attribute at the vector end, no merging + found_setting->second.emplace_back(*attr); } - - route_bulker->flush(); - - sai_attribute_t nhg_attr; - vector nhg_attrs; - - nhg_attr.id = SAI_NEXT_HOP_GROUP_ATTR_TYPE; - nhg_attr.value.s32 = SAI_NEXT_HOP_GROUP_TYPE_ECMP; - nhg_attrs.push_back(nhg_attr); - - sai_object_id_t next_hop_group_id; - sai_status_t status = sai_next_hop_group_api-> - create_next_hop_group(&next_hop_group_id, m_switchId, (uint32_t)nhg_attrs.size(), nhg_attrs.data()); - - if (status != SAI_STATUS_SUCCESS) + else { - SWSS_LOG_ERROR("Failed to create next hop group %s, rv:%d", - ipAddresses.to_string().c_str(), status); - return false; + // Create a new key if not exists in the map + setting_entries.emplace(std::piecewise_construct, + std::forward_as_tuple(object_id), + std::forward_as_tuple(1, *attr)); } - m_nextHopGroupCount ++; - SWSS_LOG_NOTICE("Create next hop group %s", ipAddresses.to_string().c_str()); - - NextHopGroupEntry next_hop_group_entry; - next_hop_group_entry.next_hop_group_id = next_hop_group_id; - - uint32_t object_count = (uint32_t)next_hop_ids.size(); - vector attr_count; - vector> attrs; - vector object_id(object_count); - vector object_statuses(object_count); + return SAI_STATUS_SUCCESS; + } + */ - // Create a next hop group members - for (auto nhid: next_hop_ids) + void flush() + { + // Removing + if (!removing_entries.empty()) { - attrs.emplace_back(); - auto& nhgm_attrs = attrs.back(); - - sai_attribute_t nhgm_attr; - nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID; - nhgm_attr.value.oid = next_hop_group_id; - nhgm_attrs.push_back(nhgm_attr); - - nhgm_attr.id = SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_ID; - nhgm_attr.value.oid = nhid; - nhgm_attrs.push_back(nhgm_attr); - - attr_count.push_back((uint32_t)nhgm_attrs.size()); + uint32_t count = (uint32_t)removing_entries.size(); + vector statuses(count); + (*remove_entries)(removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); + + SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); + + removing_entries.clear(); } - vector attrs_array; - for (auto const& i: attrs) + // Creating + if (!creating_entries.empty()) { - attrs_array.push_back(i.data()); - } - status = sai_next_hop_group_api->create_next_hop_group_members(m_switchId, object_count, attr_count.data(), attrs_array.data() - , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, object_id.data(), object_statuses.data()); + vector rs; + vector tss; + vector cs; - for (uint32_t idx = 0; idx < object_count; idx++) - { - sai_object_id_t next_hop_group_member_id = object_id[idx]; - if (status != SAI_STATUS_SUCCESS) + for (auto const& i: creating_entries) { - // TODO: do we need to clean up? - SWSS_LOG_ERROR("Failed to create next hop group %lx member %lx: %d\n", - next_hop_group_id, next_hop_group_member_id, status); - return false; + auto const& route_entry = i.first; + auto const& attrs = i.second; + + rs.push_back(route_entry); + tss.push_back(attrs.data()); + cs.push_back((uint32_t)attrs.size()); } + uint32_t count = (uint32_t)creating_entries.size(); + vector statuses(count); + (*create_entries)(switch_id, count, rs.data(), cs.data(), tss.data() + , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); + + SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); + creating_entries.clear(); } - // Save the membership into next hop structure - next_hop_group_entry.next_hop_group_members.insert(object_id.begin(), object_id.end()); + // Setting + // TODO: wait until available in SAI /* - * Initialize the next hop group structure with ref_count as 0. This - * count will increase once the route is successfully syncd. - */ - next_hop_group_entry.ref_count = 0; - m_syncdNextHopGroups[ipAddresses] = next_hop_group_entry; - return true; - } - - bool removeNextHopGroup(const IpAddresses& ipAddresses) - { - sai_status_t status; - assert(hasNextHopGroup(ipAddresses)); - - route_bulker->flush(); - if (m_syncdNextHopGroups[ipAddresses].ref_count == 0) + if (!setting_entries.empty()) { - auto next_hop_group_entry = m_syncdNextHopGroups[ipAddresses]; - sai_object_id_t next_hop_group_id = next_hop_group_entry.next_hop_group_id; + vector rs; + vector ts; - for (auto next_hop_group_member_id: next_hop_group_entry.next_hop_group_members) + for (auto const& i: setting_entries) { - status = sai_next_hop_group_api->remove_next_hop_group_member(next_hop_group_member_id); - if (status != SAI_STATUS_SUCCESS) + auto const& route_entry = i.first; + auto const& attrs = i.second; + for (auto const& attr: attrs) { - SWSS_LOG_ERROR("Failed to remove next hop group member %lx, rv:%d", next_hop_group_member_id, status); - return false; + rs.push_back(route_entry); + ts.push_back(attr); } } + uint32_t count = (uint32_t)setting_entries.size(); + vector statuses(count); + (*set_entries_attribute)(count, rs.data(), ts.data() + , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - sai_status_t status = sai_next_hop_group_api->remove_next_hop_group(next_hop_group_id); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to remove next hop group %lx, rv:%d", next_hop_group_id, status); - return false; - } + SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); - m_nextHopGroupCount --; - m_syncdNextHopGroups.erase(ipAddresses); - return true; - } - else - { - return false; + setting_entries.clear(); } + */ + } + + void clear() + { + removing_entries.clear(); + creating_entries.clear(); + setting_entries.clear(); } private: - int m_nextHopGroupCount; - int m_maxNextHopGroupCount; - NextHopGroupTable m_syncdNextHopGroups; - sai_next_hop_group_api_t * sai_next_hop_group_api; - RouteBulker * route_bulker; - sai_object_id_t m_switchId; + sai_object_id_t switch_id; + + std::vector creating_entries; + std::unordered_map> + setting_entries; + std::vector removing_entries; + + typename Ts::bulk_create_entry_fn create_entries; + typename Ts::bulk_remove_entry_fn remove_entries; + // TODO: wait until available in SAI + //typename Ts::bulk_set_entry_attribute_fn set_entries_attribute; }; -#endif + +template <> +NextHopGroupBulker::NextHopGroupBulker(saitraits::api_t *api, sai_object_id_t switch_id) + : switch_id(switch_id) +{ + create_entries = api->create_next_hop_group_members; + remove_entries = api->remove_next_hop_group_members; + // TODO: wait until available in SAI + //set_entries_attribute = ; +} From 67eb82733c4d085324aaaf5215a97faf535699f9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sun, 22 Mar 2020 05:40:42 +0000 Subject: [PATCH 03/42] (refactor template argument) --- orchagent/bulker.h | 90 ++++++++++++++++++++++------------------- orchagent/routeorch.cpp | 4 +- 2 files changed, 52 insertions(+), 42 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 56e9a272e7..b311089103 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -123,8 +123,9 @@ template struct saitraits { }; template<> -struct saitraits +struct saitraits { + using entry_t = sai_route_entry_t; using api_t = sai_route_api_t; using create_entry_fn = sai_create_route_entry_fn; using remove_entry_fn = sai_remove_route_entry_fn; @@ -135,8 +136,9 @@ struct saitraits }; template<> -struct saitraits +struct saitraits { + using entry_t = sai_fdb_entry_t; using api_t = sai_fdb_api_t; using create_entry_fn = sai_create_fdb_entry_fn; using remove_entry_fn = sai_remove_fdb_entry_fn; @@ -149,6 +151,7 @@ struct saitraits template<> struct saitraits { + using entry_t = sai_object_id_t; using api_t = sai_next_hop_group_api_t; using create_entry_fn = sai_create_next_hop_group_member_fn; using remove_entry_fn = sai_remove_next_hop_group_member_fn; @@ -159,17 +162,20 @@ struct saitraits //using bulk_set_entry_attribute_fn = sai_bulk_object_set_attribute_fn; }; -template > -class RouteBulker +template +class EntityBulker { public: - RouteBulker(typename Ts::api_t *api) + using Ts = saitraits; + using Te = typename Ts::entry_t; + + EntityBulker(typename Ts::api_t *api) { throw std::logic_error("Not implemented"); } sai_status_t create_entry( - _In_ const T *entry, + _In_ const Te *entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { @@ -182,7 +188,7 @@ class RouteBulker } sai_status_t remove_entry( - _In_ const T *entry) + _In_ const Te *entry) { assert(entry); if (!entry) throw std::invalid_argument("entry is null"); @@ -207,7 +213,7 @@ class RouteBulker } sai_status_t set_entry_attribute( - _In_ const T *entry, + _In_ const Te *entry, _In_ const sai_attribute_t *attr) { auto found_setting = setting_entries.find(*entry); @@ -232,16 +238,16 @@ class RouteBulker // Removing if (!removing_entries.empty()) { - vector rs; + vector rs; for (auto i: removing_entries) { auto& entry = i; rs.push_back(entry); } - uint32_t route_count = (uint32_t)removing_entries.size(); - vector statuses(route_count); - (*remove_entries)(route_count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + uint32_t count = (uint32_t)removing_entries.size(); + vector statuses(count); + (*remove_entries)(count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); @@ -251,7 +257,7 @@ class RouteBulker // Creating if (!creating_entries.empty()) { - vector rs; + vector rs; vector tss; vector cs; @@ -264,9 +270,9 @@ class RouteBulker tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } - uint32_t route_count = (uint32_t)creating_entries.size(); - vector statuses(route_count); - (*create_entries)(route_count, rs.data(), cs.data(), tss.data() + uint32_t count = (uint32_t)creating_entries.size(); + vector statuses(count); + (*create_entries)(count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); @@ -277,7 +283,7 @@ class RouteBulker // Setting if (!setting_entries.empty()) { - vector rs; + vector rs; vector ts; for (auto const& i: setting_entries) @@ -290,9 +296,9 @@ class RouteBulker ts.push_back(attr); } } - uint32_t route_count = (uint32_t)setting_entries.size(); - vector statuses(route_count); - (*set_entries_attribute)(route_count, rs.data(), ts.data() + uint32_t count = (uint32_t)setting_entries.size(); + vector statuses(count); + (*set_entries_attribute)(count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); @@ -309,9 +315,9 @@ class RouteBulker } private: - std::unordered_map> creating_entries; - std::unordered_map> setting_entries; - std::unordered_set removing_entries; + std::unordered_map> creating_entries; + std::unordered_map> setting_entries; + std::unordered_set removing_entries; typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; @@ -319,7 +325,7 @@ class RouteBulker }; template <> -RouteBulker::RouteBulker(saitraits::api_t *api) +EntityBulker::EntityBulker(sai_route_api_t *api) { create_entries = api->create_route_entries; remove_entries = api->remove_route_entries; @@ -328,7 +334,7 @@ RouteBulker::RouteBulker(saitraits::api_t template <> -RouteBulker::RouteBulker(saitraits::api_t *api) +EntityBulker::EntityBulker(sai_fdb_api_t *api) { // TODO: implement after create_fdb_entries() is available in SAI throw std::logic_error("Not implemented"); @@ -339,17 +345,13 @@ RouteBulker::RouteBulker(saitraits::api_t *api */ } -template > -class NextHopGroupBulker +template +class ObjectBulker { public: - struct object_entry - { - sai_object_id_t *object_id; - vector attrs; - }; - - NextHopGroupBulker(typename Ts::api_t* next_hop_group_api, sai_object_id_t switch_id) + using Ts = saitraits; + + ObjectBulker(typename Ts::api_t* next_hop_group_api, sai_object_id_t switch_id) { throw std::logic_error("Not implemented"); } @@ -425,16 +427,16 @@ class NextHopGroupBulker // Creating if (!creating_entries.empty()) { - vector rs; + vector rs; vector tss; vector cs; for (auto const& i: creating_entries) { - auto const& route_entry = i.first; + auto const& entry = i.first; auto const& attrs = i.second; - rs.push_back(route_entry); + rs.push_back(entry); tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } @@ -458,11 +460,11 @@ class NextHopGroupBulker for (auto const& i: setting_entries) { - auto const& route_entry = i.first; + auto const& entry = i.first; auto const& attrs = i.second; for (auto const& attr: attrs) { - rs.push_back(route_entry); + rs.push_back(entry); ts.push_back(attr); } } @@ -486,9 +488,15 @@ class NextHopGroupBulker } private: + struct object_entry + { + sai_object_id_t *object_id; + vector attrs; + }; + sai_object_id_t switch_id; - std::vector creating_entries; + std::vector creating_entries; std::unordered_map> setting_entries; std::vector removing_entries; @@ -500,7 +508,7 @@ class NextHopGroupBulker }; template <> -NextHopGroupBulker::NextHopGroupBulker(saitraits::api_t *api, sai_object_id_t switch_id) +ObjectBulker::ObjectBulker(saitraits::api_t *api, sai_object_id_t switch_id) : switch_id(switch_id) { create_entries = api->create_next_hop_group_members; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index e22b8276bb..8a4b0121aa 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -22,7 +22,9 @@ extern CrmOrch *gCrmOrch; const int routeorch_pri = 5; -RouteBulker t(sai_route_api); +EntityBulker gRouteBulker(sai_route_api); +extern sai_fdb_api_t* sai_fdb_api; EntityBulker gFdbBulker(sai_fdb_api); +ObjectBulker gNextHopGroupMemberBulkder(sai_next_hop_group_api, gSwitchId); RouteOrch::RouteOrch(DBConnector *db, string tableName, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch) : Orch(db, tableName, routeorch_pri), From 9df550cd2ecfcd3f21c15f582f2815567692c258 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sun, 22 Mar 2020 07:04:48 +0000 Subject: [PATCH 04/42] Fix size_t, fix object_entry ctor --- orchagent/bulker.h | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index b311089103..d1016748c8 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -245,9 +245,9 @@ class EntityBulker auto& entry = i; rs.push_back(entry); } - uint32_t count = (uint32_t)removing_entries.size(); + size_t count = removing_entries.size(); vector statuses(count); - (*remove_entries)(count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); @@ -270,9 +270,9 @@ class EntityBulker tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } - uint32_t count = (uint32_t)creating_entries.size(); + size_t count = creating_entries.size(); vector statuses(count); - (*create_entries)(count, rs.data(), cs.data(), tss.data() + (*create_entries)((uint32_t)count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); @@ -296,9 +296,9 @@ class EntityBulker ts.push_back(attr); } } - uint32_t count = (uint32_t)setting_entries.size(); + size_t count = setting_entries.size(); vector statuses(count); - (*set_entries_attribute)(count, rs.data(), ts.data() + (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); @@ -361,10 +361,8 @@ class ObjectBulker _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { - creating_entries.emplace_back(std::piecewise_construct, - std::forward_as_tuple(object_id), - std::forward_as_tuple(attr_list, attr_list + attr_count)); - SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), creating_entries.back().size(), (int)creating_entries.back()[0].id); + creating_entries.emplace_back(object_id, attr_list, attr_list + attr_count); + SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d\n", creating_entries.size(), creating_entries.back().attrs.size(), (int)creating_entries.back().attrs[0].id); *object_id = SAI_NULL_OBJECT_ID; // not created immediately, postponed until flush return SAI_STATUS_SUCCESS; @@ -415,9 +413,9 @@ class ObjectBulker // Removing if (!removing_entries.empty()) { - uint32_t count = (uint32_t)removing_entries.size(); + size_t count = removing_entries.size(); vector statuses(count); - (*remove_entries)(removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); + (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); @@ -440,9 +438,9 @@ class ObjectBulker tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } - uint32_t count = (uint32_t)creating_entries.size(); + size_t count = creating_entries.size(); vector statuses(count); - (*create_entries)(switch_id, count, rs.data(), cs.data(), tss.data() + (*create_entries)(switch_id, (uint32_t)count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); @@ -468,9 +466,9 @@ class ObjectBulker ts.push_back(attr); } } - uint32_t count = (uint32_t)setting_entries.size(); + size_t count = setting_entries.size(); vector statuses(count); - (*set_entries_attribute)(count, rs.data(), ts.data() + (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); @@ -492,6 +490,12 @@ class ObjectBulker { sai_object_id_t *object_id; vector attrs; + template + object_entry(sai_object_id_t *object_id, InputIterator first, InputIterator last) + : object_id(object_id) + , attrs(first, last) + { + } }; sai_object_id_t switch_id; From b1c2bf59dd5f0455314b3186e220b6aeba82d32d Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 23 Mar 2020 01:33:14 +0000 Subject: [PATCH 05/42] Use nhgm bulker in RouteOrch --- orchagent/bulker.h | 59 ++++++++++++++++++++++------------ orchagent/routeorch.cpp | 70 ++++++++++++++++++++++++++++------------- 2 files changed, 87 insertions(+), 42 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index d1016748c8..de6969f699 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -184,7 +184,7 @@ class EntityBulker std::forward_as_tuple(attr_list, attr_list + attr_count)); bool inserted = rc.second; SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), creating_entries[*entry].size(), (int)creating_entries[*entry][0].id, inserted); - return inserted ? SAI_STATUS_SUCCESS : SAI_STATUS_ITEM_ALREADY_EXISTS; + return inserted ? SAI_STATUS_NOT_EXECUTED : SAI_STATUS_ITEM_ALREADY_EXISTS; } sai_status_t remove_entry( @@ -203,13 +203,14 @@ class EntityBulker if (found_creating != creating_entries.end()) { creating_entries.erase(found_creating); + return SAI_STATUS_SUCCESS; } else { removing_entries.emplace(*entry); + return SAI_STATUS_NOT_EXECUTED; } - return SAI_STATUS_SUCCESS; } sai_status_t set_entry_attribute( @@ -230,7 +231,7 @@ class EntityBulker std::forward_as_tuple(1, *attr)); } - return SAI_STATUS_SUCCESS; + return SAI_STATUS_NOT_EXECUTED; } void flush() @@ -361,14 +362,17 @@ class ObjectBulker _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { - creating_entries.emplace_back(object_id, attr_list, attr_list + attr_count); - SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d\n", creating_entries.size(), creating_entries.back().attrs.size(), (int)creating_entries.back().attrs[0].id); + creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); + + auto last_attrs = std::get<1>(creating_entries.back()); + SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %u\n", creating_entries.size(), last_attrs.size(), last_attrs[0].id); *object_id = SAI_NULL_OBJECT_ID; // not created immediately, postponed until flush - return SAI_STATUS_SUCCESS; + return SAI_STATUS_NOT_EXECUTED; } sai_status_t remove_entry( + _Out_ sai_status_t *object_status, _In_ sai_object_id_t object_id) { assert(object_id != SAI_NULL_OBJECT_ID); @@ -379,9 +383,11 @@ class ObjectBulker { setting_entries.erase(found_setting); } - - removing_entries.push_back(object_id); - return SAI_STATUS_SUCCESS; + + removing_entries.emplace_back(object_id); + removing_statuses.emplace_back(object_status); + *object_status = SAI_STATUS_NOT_EXECUTED; + return *object_status; } // TODO: wait until available in SAI @@ -425,26 +431,28 @@ class ObjectBulker // Creating if (!creating_entries.empty()) { - vector rs; vector tss; vector cs; for (auto const& i: creating_entries) { - auto const& entry = i.first; - auto const& attrs = i.second; - - rs.push_back(entry); + auto const& attrs = std::get<1>(i); tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } size_t count = creating_entries.size(); + vector object_ids(count); vector statuses(count); - (*create_entries)(switch_id, (uint32_t)count, rs.data(), cs.data(), tss.data() - , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - + (*create_entries)(switch_id, (uint32_t)count, cs.data(), tss.data() + , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, object_ids.data(), statuses.data()); SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); + for (size_t i = 0; i < count; i++) + { + auto pid = std::get<0>(creating_entries[i]); + *pid = (statuses[i] == SAI_STATUS_SUCCESS) ? object_ids[i] : SAI_NULL_OBJECT_ID; + } + creating_entries.clear(); } @@ -500,10 +508,21 @@ class ObjectBulker sai_object_id_t switch_id; - std::vector creating_entries; - std::unordered_map> - setting_entries; + std::vector // - attrs + >> creating_entries; + + std::unordered_map< // An map of + sai_object_id_t, // object_id -> (object_status, attributes) + std::pair< + sai_status_t, + std::vector + > + > setting_entries; + std::vector removing_entries; + std::vector removing_statuses; typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 8a4b0121aa..b018fa0c85 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -722,6 +722,11 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) it.to_string().c_str(), nexthops.to_string().c_str()); return false; } + + // skip next hop group member create for neighbor from down port + if (m_neighOrch->isNextHopFlagSet(it, NHFLAGS_IFDOWN)) { + continue; + } sai_object_id_t next_hop_id = m_neighOrch->getNextHopId(it); next_hop_ids.push_back(next_hop_id); @@ -756,12 +761,12 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) NextHopGroupEntry next_hop_group_entry; next_hop_group_entry.next_hop_group_id = next_hop_group_id; - for (auto nhid: next_hop_ids) + size_t npid_count = next_hop_ids.size(); + vector nhgm_ids(npid_count); + for (size_t i = 0; i < npid_count; i++) + //for (auto nhid: next_hop_ids) { - // skip next hop group member create for neighbor from down port - if (m_neighOrch->isNextHopFlagSet(nhopgroup_members_set[nhid], NHFLAGS_IFDOWN)) { - continue; - } + auto nhid = next_hop_ids[i]; // Create a next hop group member vector nhgm_attrs; @@ -775,17 +780,26 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) nhgm_attr.value.oid = nhid; nhgm_attrs.push_back(nhgm_attr); - sai_object_id_t next_hop_group_member_id; - status = sai_next_hop_group_api->create_next_hop_group_member(&next_hop_group_member_id, - gSwitchId, - (uint32_t)nhgm_attrs.size(), - nhgm_attrs.data()); - - if (status != SAI_STATUS_SUCCESS) + //sai_object_id_t next_hop_group_member_id; + gNextHopGroupMemberBulkder.create_entry(&nhgm_ids[i], + (uint32_t)nhgm_attrs.size(), + nhgm_attrs.data()); + // status = sai_next_hop_group_api->create_next_hop_group_member(&next_hop_group_member_id, + // gSwitchId, + // (uint32_t)nhgm_attrs.size(), + // nhgm_attrs.data()); + } + + gNextHopGroupMemberBulkder.flush(); + for (size_t i = 0; i < npid_count; i++) + { + auto nhid = next_hop_ids[i]; + auto nhgm_id = nhgm_ids[i]; + if (nhgm_id == SAI_NULL_OBJECT_ID) { // TODO: do we need to clean up? SWSS_LOG_ERROR("Failed to create next hop group %" PRIx64 " member %" PRIx64 ": %d\n", - next_hop_group_id, next_hop_group_member_id, status); + next_hop_group_id, nhgm_ids[i], status); return false; } @@ -793,7 +807,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) // Save the membership into next hop structure next_hop_group_entry.nhopgroup_members[nhopgroup_members_set.find(nhid)->second] = - next_hop_group_member_id; + nhgm_id; } /* Increment the ref_count for the next hops used by the next hop group. */ @@ -829,27 +843,39 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) next_hop_group_id = next_hop_group_entry->second.next_hop_group_id; SWSS_LOG_NOTICE("Delete next hop group %s", nexthops.to_string().c_str()); - for (auto nhop = next_hop_group_entry->second.nhopgroup_members.begin(); - nhop != next_hop_group_entry->second.nhopgroup_members.end();) + vector next_hop_ids; + auto& nhgm = next_hop_group_entry->second.nhopgroup_members; + for (auto nhop = nhgm.begin(); nhop != nhgm.end();) { - if (m_neighOrch->isNextHopFlagSet(nhop->first, NHFLAGS_IFDOWN)) { SWSS_LOG_WARN("NHFLAGS_IFDOWN set for next hop group member %s with next_hop_id %" PRIx64, nhop->first.to_string().c_str(), nhop->second); - nhop = next_hop_group_entry->second.nhopgroup_members.erase(nhop); + nhop = nhgm.erase(nhop); continue; } - status = sai_next_hop_group_api->remove_next_hop_group_member(nhop->second); - if (status != SAI_STATUS_SUCCESS) + + next_hop_ids.push_back(nhop->second); + nhop = nhgm.erase(nhop); + } + + size_t nhid_count = next_hop_ids.size(); + vector statuses(nhid_count); + for (size_t i = 0; i < nhid_count; i++) + { + //status = sai_next_hop_group_api->remove_next_hop_group_member(nhop->second); + gNextHopGroupMemberBulkder.remove_entry(&statuses[i], next_hop_ids[i]); + } + for (size_t i = 0; i < nhid_count; i++) + { + if (statuses[i] != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove next hop group member %" PRIx64 ", rv:%d", - nhop->second, status); + next_hop_ids[i], statuses[i]); return false; } gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_NEXTHOP_GROUP_MEMBER); - nhop = next_hop_group_entry->second.nhopgroup_members.erase(nhop); } status = sai_next_hop_group_api->remove_next_hop_group(next_hop_group_id); From fa95a19ac85c0bdd20056faac802719b60a31ee6 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 25 Mar 2020 06:11:00 +0000 Subject: [PATCH 06/42] (pass gtest) --- orchagent/bulker.h | 9 +++++---- orchagent/routeorch.cpp | 9 ++++----- orchagent/routeorch.h | 4 ++++ tests/mock_tests/aclorch_ut.cpp | 2 ++ 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index de6969f699..73f1a76e0f 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -1,3 +1,5 @@ +#pragma once + #include #include #include @@ -326,16 +328,15 @@ class EntityBulker }; template <> -EntityBulker::EntityBulker(sai_route_api_t *api) +inline EntityBulker::EntityBulker(sai_route_api_t *api) { create_entries = api->create_route_entries; remove_entries = api->remove_route_entries; set_entries_attribute = api->set_route_entries_attribute; } - template <> -EntityBulker::EntityBulker(sai_fdb_api_t *api) +inline EntityBulker::EntityBulker(sai_fdb_api_t *api) { // TODO: implement after create_fdb_entries() is available in SAI throw std::logic_error("Not implemented"); @@ -531,7 +532,7 @@ class ObjectBulker }; template <> -ObjectBulker::ObjectBulker(saitraits::api_t *api, sai_object_id_t switch_id) +inline ObjectBulker::ObjectBulker(saitraits::api_t *api, sai_object_id_t switch_id) : switch_id(switch_id) { create_entries = api->create_next_hop_group_members; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index b018fa0c85..84f8b7cd6b 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -4,7 +4,6 @@ #include "logger.h" #include "swssnet.h" #include "crmorch.h" -#include "bulker.h" extern sai_object_id_t gVirtualRouterId; extern sai_object_id_t gSwitchId; @@ -12,6 +11,7 @@ extern sai_object_id_t gSwitchId; extern sai_next_hop_group_api_t* sai_next_hop_group_api; extern sai_route_api_t* sai_route_api; extern sai_switch_api_t* sai_switch_api; +extern sai_fdb_api_t* sai_fdb_api; extern PortsOrch *gPortsOrch; extern CrmOrch *gCrmOrch; @@ -22,11 +22,10 @@ extern CrmOrch *gCrmOrch; const int routeorch_pri = 5; -EntityBulker gRouteBulker(sai_route_api); -extern sai_fdb_api_t* sai_fdb_api; EntityBulker gFdbBulker(sai_fdb_api); -ObjectBulker gNextHopGroupMemberBulkder(sai_next_hop_group_api, gSwitchId); - RouteOrch::RouteOrch(DBConnector *db, string tableName, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch) : + gRouteBulker(sai_route_api), + //gFdbBulker(sai_fdb_api), + gNextHopGroupMemberBulkder(sai_next_hop_group_api, gSwitchId), Orch(db, tableName, routeorch_pri), m_neighOrch(neighOrch), m_intfsOrch(intfsOrch), diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 84550d7589..817b4ff948 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -10,6 +10,7 @@ #include "ipaddresses.h" #include "ipprefix.h" #include "nexthopgroupkey.h" +#include "bulker.h" #include @@ -98,6 +99,9 @@ class RouteOrch : public Orch, public Subject void addLinkLocalRouteToMe(sai_object_id_t vrf_id, IpPrefix linklocal_prefix); void doTask(Consumer& consumer); + EntityBulker gRouteBulker; + //EntityBulker gFdbBulker; + ObjectBulker gNextHopGroupMemberBulkder; }; #endif /* SWSS_ROUTEORCH_H */ diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index dc784c75ae..b7c057fce0 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -18,6 +18,7 @@ extern sai_port_api_t *sai_port_api; extern sai_vlan_api_t *sai_vlan_api; extern sai_bridge_api_t *sai_bridge_api; extern sai_route_api_t *sai_route_api; +extern sai_next_hop_group_api_t* sai_next_hop_group_api; namespace aclorch_test { @@ -258,6 +259,7 @@ namespace aclorch_test sai_api_query(SAI_API_VLAN, (void **)&sai_vlan_api); sai_api_query(SAI_API_ROUTE, (void **)&sai_route_api); sai_api_query(SAI_API_ACL, (void **)&sai_acl_api); + sai_api_query(SAI_API_NEXT_HOP_GROUP, (void **)&sai_next_hop_group_api); sai_attribute_t attr; From 9bf019ed735bae5e85aa2ba303495e125484d1ee Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 25 Mar 2020 19:07:08 +0000 Subject: [PATCH 07/42] Missing flush() --- orchagent/routeorch.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 84f8b7cd6b..28764486cf 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -865,6 +865,7 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) //status = sai_next_hop_group_api->remove_next_hop_group_member(nhop->second); gNextHopGroupMemberBulkder.remove_entry(&statuses[i], next_hop_ids[i]); } + gNextHopGroupMemberBulkder.flush(); for (size_t i = 0; i < nhid_count; i++) { if (statuses[i] != SAI_STATUS_SUCCESS) From 36dbd996faaa8de6bf87483e00d2d3167c3b901e Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 25 Mar 2020 23:09:51 +0000 Subject: [PATCH 08/42] Return removing status back --- orchagent/bulker.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 73f1a76e0f..9e1a1569dc 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -423,9 +423,12 @@ class ObjectBulker size_t count = removing_entries.size(); vector statuses(count); (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); - + + for (size_t i = 0; i < count; i++) + { + *removing_statuses[i] = statuses[i]; + } removing_entries.clear(); } From acc67b94fb320628de15ffb5dfed9d1784c6b6f0 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 26 Mar 2020 00:35:13 +0000 Subject: [PATCH 09/42] Clear another removing queue --- orchagent/bulker.h | 5 +++-- orchagent/routeorch.cpp | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 9e1a1569dc..014de6c31a 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -422,14 +422,15 @@ class ObjectBulker { size_t count = removing_entries.size(); vector statuses(count); - (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); + sai_status_t status = (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); + SWSS_LOG_NOTICE("bulk.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); for (size_t i = 0; i < count; i++) { *removing_statuses[i] = statuses[i]; } removing_entries.clear(); + removing_statuses.clear(); } // Creating diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 28764486cf..b23aea9840 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -870,8 +870,8 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) { if (statuses[i] != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to remove next hop group member %" PRIx64 ", rv:%d", - next_hop_ids[i], statuses[i]); + SWSS_LOG_ERROR("Failed to remove next hop group member[%zu] %" PRIx64 ", rv:%d", + i, next_hop_ids[i], statuses[i]); return false; } From 11a8352a499a756aff7d495ed5cff9bf5591a7a1 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 28 Mar 2020 01:23:20 +0000 Subject: [PATCH 10/42] Implement EntityBulker partially --- orchagent/bulker.h | 54 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 014de6c31a..71ec1d5954 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -10,7 +10,6 @@ #include #include "sai.h" -/* static inline bool operator==(const sai_ip_prefix_t& a, const sai_ip_prefix_t& b) { if (a.addr_family != b.addr_family) return false; @@ -40,7 +39,6 @@ static inline bool operator==(const sai_route_entry_t& a, const sai_route_entry_ && a.destination == b.destination ; } -*/ static inline std::size_t hash_value(const sai_ip_prefix_t& a) { @@ -177,19 +175,33 @@ class EntityBulker } sai_status_t create_entry( + _Out_ sai_status_t *object_status, _In_ const Te *entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { auto rc = creating_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), - std::forward_as_tuple(attr_list, attr_list + attr_count)); + std::forward_as_tuple()); + auto it = rc.first; bool inserted = rc.second; - SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), creating_entries[*entry].size(), (int)creating_entries[*entry][0].id, inserted); - return inserted ? SAI_STATUS_NOT_EXECUTED : SAI_STATUS_ITEM_ALREADY_EXISTS; + if (!inserted) + { + SWSS_LOG_DEBUG("bulk.create_entry not inserted %zu\n", creating_entries.size()); + *object_status = SAI_STATUS_ITEM_ALREADY_EXISTS; + return *object_status; + } + + auto& attrs = it->second.first; + attrs.insert(attrs.end(), attr_list, attr_list + attr_count); + it->second.second = object_status; + SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), it->second.first.size(), (int)it->second.first[0].id, inserted); + *object_status = SAI_STATUS_NOT_EXECUTED; + return *object_status; } sai_status_t remove_entry( + _Out_ sai_status_t *object_status, _In_ const Te *entry) { assert(entry); @@ -205,14 +217,14 @@ class EntityBulker if (found_creating != creating_entries.end()) { creating_entries.erase(found_creating); - return SAI_STATUS_SUCCESS; + *object_status = SAI_STATUS_SUCCESS; + return *object_status; } - else - { - removing_entries.emplace(*entry); - return SAI_STATUS_NOT_EXECUTED; - } - + removing_entries.emplace(std::piecewise_construct, + std::forward_as_tuple(*entry), + std::forward_as_tuple(object_status)); + *object_status = SAI_STATUS_NOT_EXECUTED; + return *object_status; } sai_status_t set_entry_attribute( @@ -318,9 +330,18 @@ class EntityBulker } private: - std::unordered_map> creating_entries; - std::unordered_map> setting_entries; - std::unordered_set removing_entries; + std::unordered_map< + Te, + std::pair< + std::vector, + sai_status_t * + > + > creating_entries; + std::unordered_map> setting_entries; + std::unordered_map< + Te, + sai_status_t * + > removing_entries; typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; @@ -494,6 +515,7 @@ class ObjectBulker void clear() { removing_entries.clear(); + removing_statuses.clear(); creating_entries.clear(); setting_entries.clear(); } @@ -521,7 +543,7 @@ class ObjectBulker std::unordered_map< // An map of sai_object_id_t, // object_id -> (object_status, attributes) std::pair< - sai_status_t, + sai_status_t *, std::vector > > setting_entries; From 71c70f87333e8b6ed6c5c43ae78a138ee3f1bfcf Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 28 Mar 2020 07:33:18 +0000 Subject: [PATCH 11/42] EntityBulker is compiled Signed-off-by: Qi Luo --- orchagent/bulker.h | 97 +++++++++++++++++++++++++++++++--------------- 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 71ec1d5954..0878daf33c 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -210,12 +210,14 @@ class EntityBulker auto found_setting = setting_entries.find(*entry); if (found_setting != setting_entries.end()) { + // TODO: mark old ones as done setting_entries.erase(found_setting); } auto found_creating = creating_entries.find(*entry); if (found_creating != creating_entries.end()) { + // TODO: mark old ones as done creating_entries.erase(found_creating); *object_status = SAI_STATUS_SUCCESS; return *object_status; @@ -228,24 +230,32 @@ class EntityBulker } sai_status_t set_entry_attribute( + _Out_ sai_status_t *object_status, _In_ const Te *entry, _In_ const sai_attribute_t *attr) { - auto found_setting = setting_entries.find(*entry); - if (found_setting != setting_entries.end()) - { - // For simplicity, just insert new attribute at the vector end, no merging - found_setting->second.emplace_back(*attr); - } - else - { - // Create a new key if not exists in the map - setting_entries.emplace(std::piecewise_construct, + auto attrmap = setting_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), - std::forward_as_tuple(1, *attr)); + std::forward_as_tuple() + ).first->second; + + 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 done + *it->second.second = SAI_STATUS_SUCCESS; } - - return SAI_STATUS_NOT_EXECUTED; + it->second.second = object_status; + *object_status = SAI_STATUS_NOT_EXECUTED; + return *object_status; } void flush() @@ -257,15 +267,20 @@ class EntityBulker for (auto i: removing_entries) { - auto& entry = i; + auto& entry = i.first; rs.push_back(entry); } - size_t count = removing_entries.size(); + size_t count = rs.size(); vector statuses(count); (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + SWSS_LOG_NOTICE("EntityBulker.flush removing_entries %zu\n", removing_entries.size()); - SWSS_LOG_NOTICE("bulk.flush removing_entries %zu\n", removing_entries.size()); - + for (size_t ir = 0; ir < count; ir++) + { + auto& entry = rs[ir]; + sai_status_t *object_status = removing_entries[entry]; + *object_status = statuses[ir]; + } removing_entries.clear(); } @@ -279,19 +294,24 @@ class EntityBulker for (auto const& i: creating_entries) { auto const& entry = i.first; - auto const& attrs = i.second; + auto const& attrs = i.second.first; rs.push_back(entry); tss.push_back(attrs.data()); cs.push_back((uint32_t)attrs.size()); } - size_t count = creating_entries.size(); + size_t count = rs.size(); vector statuses(count); (*create_entries)((uint32_t)count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + SWSS_LOG_NOTICE("EntityBulker.flush creating_entries %zu\n", creating_entries.size()); - SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); - + for (size_t ir = 0; ir < count; ir++) + { + auto& entry = rs[ir]; + sai_status_t *object_status = creating_entries[entry].second; + *object_status = statuses[ir]; + } creating_entries.clear(); } @@ -304,20 +324,26 @@ class EntityBulker for (auto const& i: setting_entries) { auto const& entry = i.first; - auto const& attrs = i.second; - for (auto const& attr: attrs) + auto const& attrmap = i.second; + for (auto const& ia: attrmap) { rs.push_back(entry); - ts.push_back(attr); + ts.push_back(ia.second.first); } } - size_t count = setting_entries.size(); + size_t count = rs.size(); vector statuses(count); (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); + SWSS_LOG_NOTICE("EntityBulker.flush setting_entries %zu\n", setting_entries.size()); - SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); - + 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; + *object_status = statuses[ir]; + } setting_entries.clear(); } } @@ -337,7 +363,16 @@ class EntityBulker sai_status_t * > > creating_entries; - std::unordered_map> setting_entries; + std::unordered_map< + Te, + std::unordered_map< + sai_attr_id_t, + std::pair< + sai_attribute_t, + sai_status_t * + > + > + > setting_entries; std::unordered_map< Te, sai_status_t * @@ -444,7 +479,7 @@ class ObjectBulker size_t count = removing_entries.size(); vector statuses(count); sai_status_t status = (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("bulk.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); + SWSS_LOG_NOTICE("ObjectBulker.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); for (size_t i = 0; i < count; i++) { @@ -471,7 +506,7 @@ class ObjectBulker vector statuses(count); (*create_entries)(switch_id, (uint32_t)count, cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, object_ids.data(), statuses.data()); - SWSS_LOG_NOTICE("bulk.flush creating_entries %zu\n", creating_entries.size()); + SWSS_LOG_NOTICE("ObjectBulker.flush creating_entries %zu\n", creating_entries.size()); for (size_t i = 0; i < count; i++) { @@ -505,7 +540,7 @@ class ObjectBulker (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("bulk.flush setting_entries %zu\n", setting_entries.size()); + SWSS_LOG_NOTICE("ObjectBulker.flush setting_entries %zu\n", setting_entries.size()); setting_entries.clear(); } From 3025e9eb9383894ddf0fa7f339bd4a85178b761d Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 28 Mar 2020 22:37:50 +0000 Subject: [PATCH 12/42] (simple use route bulker) --- orchagent/routeorch.cpp | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index b23aea9840..9f7ba65da1 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1028,7 +1028,12 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const route_attr.value.oid = next_hop_id; /* Default SAI_ROUTE_ATTR_PACKET_ACTION is SAI_PACKET_ACTION_FORWARD */ - sai_status_t status = sai_route_api->create_route_entry(&route_entry, 1, &route_attr); + //sai_status_t status = sai_route_api->create_route_entry(&route_entry, 1, &route_attr); + sai_status_t object_status; + sai_status_t status = gRouteBulker.create_entry(&object_status, &route_entry, 1, &route_attr); + gRouteBulker.flush(); + status = object_status; + if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to create route %s with next hop(s) %s", @@ -1065,7 +1070,12 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD; - status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); + //status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); + sai_status_t object_status; + status = gRouteBulker.set_entry_attribute(&object_status, &route_entry, &route_attr); + gRouteBulker.flush(); + status = object_status; + if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", @@ -1078,7 +1088,11 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const route_attr.value.oid = next_hop_id; /* Set the next hop ID to a new value */ - status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); + sai_status_t object_status; + status = gRouteBulker.set_entry_attribute(&object_status, &route_entry, &route_attr); + gRouteBulker.flush(); + status = object_status; + if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set route %s with next hop(s) %s", @@ -1161,7 +1175,12 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) } else { - sai_status_t status = sai_route_api->remove_route_entry(&route_entry); + //sai_status_t status = sai_route_api->remove_route_entry(&route_entry); + sai_status_t object_status; + sai_status_t status = gRouteBulker.remove_entry(&object_status, &route_entry); + gRouteBulker.flush(); + status = object_status; + if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove route prefix:%s\n", ipPrefix.to_string().c_str()); From 31f6cd3c0992e2853ba31f689ceb839527c0e69a Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 28 Mar 2020 23:29:04 +0000 Subject: [PATCH 13/42] Handle null pointer Signed-off-by: Qi Luo --- orchagent/bulker.h | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 0878daf33c..9e4e166dd8 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -234,11 +234,13 @@ class EntityBulker _In_ const Te *entry, _In_ const sai_attribute_t *attr) { + // Insert or find the key (entry) auto attrmap = 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()); @@ -279,7 +281,10 @@ class EntityBulker { auto& entry = rs[ir]; sai_status_t *object_status = removing_entries[entry]; - *object_status = statuses[ir]; + if (object_status) + { + *object_status = statuses[ir]; + } } removing_entries.clear(); } @@ -310,7 +315,10 @@ class EntityBulker { auto& entry = rs[ir]; sai_status_t *object_status = creating_entries[entry].second; - *object_status = statuses[ir]; + if (object_status) + { + *object_status = statuses[ir]; + } } creating_entries.clear(); } @@ -342,7 +350,10 @@ class EntityBulker auto& entry = rs[ir]; auto& attr_id = ts[ir].id; sai_status_t *object_status = setting_entries[entry][attr_id].second; - *object_status = statuses[ir]; + if (object_status) + { + *object_status = statuses[ir]; + } } setting_entries.clear(); } @@ -483,7 +494,11 @@ class ObjectBulker for (size_t i = 0; i < count; i++) { - *removing_statuses[i] = statuses[i]; + sai_status_t *object_status = removing_statuses[i]; + if (object_status) + { + *object_status = statuses[i]; + } } removing_entries.clear(); removing_statuses.clear(); From 1f5106990358186bd04477562d2f637a3e98f3d9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 30 Mar 2020 01:46:57 +0000 Subject: [PATCH 14/42] Refine set_entry_attribute return value --- orchagent/bulker.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 9e4e166dd8..310a8296e2 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -229,7 +229,7 @@ class EntityBulker return *object_status; } - sai_status_t set_entry_attribute( + void set_entry_attribute( _Out_ sai_status_t *object_status, _In_ const Te *entry, _In_ const sai_attribute_t *attr) @@ -252,12 +252,11 @@ class EntityBulker it->second.first = *attr; if (!inserted) { - // If found existing key, mark old status as done + // If found existing key, mark old status as success *it->second.second = SAI_STATUS_SUCCESS; } it->second.second = object_status; *object_status = SAI_STATUS_NOT_EXECUTED; - return *object_status; } void flush() @@ -430,7 +429,7 @@ class ObjectBulker _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { - creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); + creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); auto last_attrs = std::get<1>(creating_entries.back()); SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %u\n", creating_entries.size(), last_attrs.size(), last_attrs[0].id); From e217e317730f8dc97603ddccd516558f2454cce9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 3 Apr 2020 00:23:38 +0000 Subject: [PATCH 15/42] Fix reference, implement doTask() with bulker --- orchagent/bulker.h | 18 +- orchagent/routeorch.cpp | 354 ++++++++++++++++++++++++++++++++-------- orchagent/routeorch.h | 24 ++- 3 files changed, 313 insertions(+), 83 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 310a8296e2..5c9d991cb9 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -220,11 +220,15 @@ class EntityBulker // TODO: mark old ones as done creating_entries.erase(found_creating); *object_status = SAI_STATUS_SUCCESS; + SWSS_LOG_INFO("EntityBulker.remove_entry quickly removed %zu, creating_entries.size=%zu\n", removing_entries.size(), creating_entries.size()); return *object_status; } - removing_entries.emplace(std::piecewise_construct, + auto rc = removing_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), std::forward_as_tuple(object_status)); + bool inserted = rc.second; + SWSS_LOG_INFO("EntityBulker.remove_entry %zu, %d\n", removing_entries.size(), inserted); + *object_status = SAI_STATUS_NOT_EXECUTED; return *object_status; } @@ -235,7 +239,7 @@ class EntityBulker _In_ const sai_attribute_t *attr) { // Insert or find the key (entry) - auto attrmap = setting_entries.emplace(std::piecewise_construct, + auto& attrmap = setting_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), std::forward_as_tuple() ).first->second; @@ -266,7 +270,7 @@ class EntityBulker { vector rs; - for (auto i: removing_entries) + for (auto& i: removing_entries) { auto& entry = i.first; rs.push_back(entry); @@ -342,7 +346,7 @@ class EntityBulker vector statuses(count); (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); - SWSS_LOG_NOTICE("EntityBulker.flush setting_entries %zu\n", setting_entries.size()); + SWSS_LOG_NOTICE("EntityBulker.flush setting_entries %zu, count %zu\n", setting_entries.size(), count); for (size_t ir = 0; ir < count; ir++) { @@ -431,8 +435,8 @@ class ObjectBulker { creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); - auto last_attrs = std::get<1>(creating_entries.back()); - SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %u\n", creating_entries.size(), last_attrs.size(), last_attrs[0].id); + auto& last_attrs = std::get<1>(creating_entries.back()); + SWSS_LOG_INFO("ObjectBulker.create_entry %zu, %zu, %u\n", creating_entries.size(), last_attrs.size(), last_attrs[0].id); *object_id = SAI_NULL_OBJECT_ID; // not created immediately, postponed until flush return SAI_STATUS_NOT_EXECUTED; @@ -524,7 +528,7 @@ class ObjectBulker for (size_t i = 0; i < count; i++) { - auto pid = std::get<0>(creating_entries[i]); + sai_object_id_t *pid = std::get<0>(creating_entries[i]); *pid = (statuses[i] == SAI_STATUS_SUCCESS) ? object_ids[i] : SAI_NULL_OBJECT_ID; } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 9f7ba65da1..36905c08f0 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1,5 +1,6 @@ #include #include +#include #include "routeorch.h" #include "logger.h" #include "swssnet.h" @@ -374,6 +375,8 @@ void RouteOrch::doTask(Consumer& consumer) return; } + m_toBulk.clear(); + auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { @@ -381,6 +384,11 @@ void RouteOrch::doTask(Consumer& consumer) string key = kfvKey(t); string op = kfvOp(t); + + auto inserted = m_toBulk.emplace(std::piecewise_construct, + std::forward_as_tuple(key, op), + std::forward_as_tuple()); + auto& object_statuses = inserted.first->second; /* Get notification from application */ /* resync application: @@ -484,7 +492,7 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoute(vrf_id, ip_prefix)) + if (removeRoute(object_statuses, vrf_id, ip_prefix)) it = consumer.m_toSync.erase(it); else it++; @@ -525,19 +533,17 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - if (addRoute(vrf_id, ip_prefix, nhg)) + if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) it = consumer.m_toSync.erase(it); else it++; } - continue; } - - if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || + else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoute(vrf_id, ip_prefix, nhg)) + if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) it = consumer.m_toSync.erase(it); else it++; @@ -549,7 +555,7 @@ void RouteOrch::doTask(Consumer& consumer) else if (op == DEL_COMMAND) { /* Cannot locate the route or remove succeed */ - if (removeRoute(vrf_id, ip_prefix)) + if (removeRoute(object_statuses, vrf_id, ip_prefix)) it = consumer.m_toSync.erase(it); else it++; @@ -560,6 +566,132 @@ void RouteOrch::doTask(Consumer& consumer) it = consumer.m_toSync.erase(it); } } + + gRouteBulker.flush(); + + it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + if (m_resync) + { + it++; + continue; + } + + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + + auto found = m_toBulk.find(make_pair(key, op)); + if (found == m_toBulk.end()) + { + it++; + continue; + } + + auto& object_statuses = found->second; + if (object_statuses.empty()) + { + it++; + continue; + } + + sai_object_id_t vrf_id; + IpPrefix ip_prefix; + + if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) + { + size_t found = key.find(':'); + string vrf_name = key.substr(0, found); + + if (!m_vrfOrch->isVRFexists(vrf_name)) + { + it++; + continue; + } + vrf_id = m_vrfOrch->getVRFid(vrf_name); + ip_prefix = IpPrefix(key.substr(found+1)); + } + else + { + vrf_id = gVirtualRouterId; + ip_prefix = IpPrefix(key); + } + + if (op == SET_COMMAND) + { + string ips; + string aliases; + bool excp_intfs_flag = false; + + for (auto i : kfvFieldsValues(t)) + { + if (fvField(i) == "nexthop") + ips = fvValue(i); + + if (fvField(i) == "ifname") + aliases = fvValue(i); + } + vector ipv = tokenize(ips, ','); + vector alsv = tokenize(aliases, ','); + + for (auto alias : alsv) + { + if (alias == "eth0" || alias == "lo" || alias == "docker0") + { + excp_intfs_flag = true; + break; + } + } + + // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty + if (excp_intfs_flag) + { + /* If any existing routes are updated to point to the + * above interfaces, remove them from the ASIC. */ + if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + it = consumer.m_toSync.erase(it); + else + it++; + continue; + } + + string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; + for (uint32_t i = 1; i < ipv.size(); i++) + { + nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; + } + + NextHopGroupKey nhg(nhg_str); + + if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) + { + if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + it = consumer.m_toSync.erase(it); + else + it++; + } + else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || + m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || + m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) + { + if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + it = consumer.m_toSync.erase(it); + else + it++; + } + } + else if (op == DEL_COMMAND) + { + /* Cannot locate the route or remove succeed */ + if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + it = consumer.m_toSync.erase(it); + else + it++; + } + } + m_toBulk.clear(); } void RouteOrch::notifyNextHopChangeObservers(sai_object_id_t vrf_id, const IpPrefix &prefix, const NextHopGroupKey &nexthops, bool add) @@ -898,7 +1030,7 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) return true; } -void RouteOrch::addTempRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); @@ -919,7 +1051,7 @@ void RouteOrch::addTempRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, c /* Return if next_hop_set is empty */ if (next_hop_set.empty()) - return; + return false; /* Randomly pick an address from the set */ auto it = next_hop_set.begin(); @@ -927,10 +1059,18 @@ void RouteOrch::addTempRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, c /* Set the route's temporary next hop to be the randomly picked one */ NextHopGroupKey tmp_next_hop((*it).to_string()); - addRoute(vrf_id, ipPrefix, tmp_next_hop); + bool rc = addRoute(object_statuses, vrf_id, ipPrefix, tmp_next_hop); + if (rc) + { + // TRICK! TRICK! TRICK! + // Even we only successfully invoke bulker, not SAI, we write the temp route + // into m_syncdRoutes so addRoutePost will know what happened + m_syncdRoutes[vrf_id][ipPrefix] = tmp_next_hop; + } + return rc; } -bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); @@ -999,9 +1139,20 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const /* Add a temporary route when a next hop group cannot be added, * and there is no temporary route right now or the current temporary * route is not pointing to a member of the next hop group to sync. */ - addTempRoute(vrf_id, ipPrefix, nextHops); - /* Return false since the original route is not successfully added */ - return false; + bool rc = addTempRoute(object_statuses, vrf_id, ipPrefix, nextHops); + if (rc) + { + /* Push failure into object_statuses so addRoutePost will fail + * so the original route is not successfully added, and will retry */ + object_statuses.emplace_back(SAI_STATUS_INSUFFICIENT_RESOURCES); + /* Return true so addRoutePost will be called */ + return true; + } + else + { + // Not added to route bulker + return false; + } } } @@ -1029,12 +1180,68 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const /* Default SAI_ROUTE_ATTR_PACKET_ACTION is SAI_PACKET_ACTION_FORWARD */ //sai_status_t status = sai_route_api->create_route_entry(&route_entry, 1, &route_attr); - sai_status_t object_status; - sai_status_t status = gRouteBulker.create_entry(&object_status, &route_entry, 1, &route_attr); - gRouteBulker.flush(); - status = object_status; - - if (status != SAI_STATUS_SUCCESS) + object_statuses.emplace_back(); + sai_status_t status = gRouteBulker.create_entry(&object_statuses.back(), &route_entry, 1, &route_attr); + if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) + { + SWSS_LOG_ERROR("Failed to create route %s with next hop(s) %s: already exists in bulker", + ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); + return false; + } + } + else + { + /* Set the packet action to forward when there was no next hop (dropped) */ + if (it_route->second.getSize() == 0) + { + route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; + route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD; + + //status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); + object_statuses.emplace_back(); + gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr); + } + + route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; + route_attr.value.oid = next_hop_id; + + /* Set the next hop ID to a new value */ + object_statuses.emplace_back(); + gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr); + } + return true; +} + +bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +{ + SWSS_LOG_ENTER(); + + if (object_statuses.empty()) + { + // Something went wrong before router bulker, will retry + return false; + } + + if (nextHops.getSize() > 1) + { + if (!hasNextHopGroup(nextHops)) + { + // Previous added an temporary route + auto& tmp_next_hop = m_syncdRoutes[vrf_id][ipPrefix]; + bool rc = addRoutePost(object_statuses, vrf_id, ipPrefix, tmp_next_hop); + if (!rc) + { + m_syncdRoutes[vrf_id].erase(ipPrefix); + } + return false; + } + } + + auto it_status = object_statuses.begin(); + auto it_route = m_syncdRoutes.at(vrf_id).find(ipPrefix); + if (it_route == m_syncdRoutes.at(vrf_id).end()) + { + if (*it_status++ != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to create route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); @@ -1045,8 +1252,8 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const } return false; } - - if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + + if (ipPrefix.isV4()) { gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); } @@ -1057,49 +1264,27 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const /* Increase the ref_count for the next hop (group) entry */ increaseNextHopRefCount(nextHops); - SWSS_LOG_INFO("Create route %s with next hop(s) %s", + SWSS_LOG_NOTICE("Create route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } else { - sai_status_t status; - - /* Set the packet action to forward when there was no next hop (dropped) */ - if (it_route->second.getSize() == 0) + sai_status_t status = *it_status++; + if (status != SAI_STATUS_SUCCESS) { - route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; - route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD; - - //status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); - sai_status_t object_status; - status = gRouteBulker.set_entry_attribute(&object_status, &route_entry, &route_attr); - gRouteBulker.flush(); - status = object_status; - - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", - ipPrefix.to_string().c_str(), status); - return false; - } + SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", + ipPrefix.to_string().c_str(), status); + return false; } - - route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; - route_attr.value.oid = next_hop_id; - - /* Set the next hop ID to a new value */ - sai_status_t object_status; - status = gRouteBulker.set_entry_attribute(&object_status, &route_entry, &route_attr); - gRouteBulker.flush(); - status = object_status; + status = *it_status++; if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); return false; } - + /* Increase the ref_count for the next hop (group) entry */ increaseNextHopRefCount(nextHops); @@ -1119,7 +1304,7 @@ bool RouteOrch::addRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const return true; } -bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) { SWSS_LOG_ENTER(); @@ -1150,7 +1335,45 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; attr.value.s32 = SAI_PACKET_ACTION_DROP; - sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); + //sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); + object_statuses.emplace_back(); + gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &attr); + + attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; + attr.value.oid = SAI_NULL_OBJECT_ID; + + //status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); + object_statuses.emplace_back(); + gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &attr); + } + else + { + //sai_status_t status = sai_route_api->remove_route_entry(&route_entry); + object_statuses.emplace_back(); + gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); + } + + return true; +} + +bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +{ + SWSS_LOG_ENTER(); + + if (object_statuses.empty()) + { + // Something went wrong before router bulker, will retry + return false; + } + + auto it_route_table = m_syncdRoutes.find(vrf_id); + auto it_route = it_route_table->second.find(ipPrefix); + auto it_status = object_statuses.begin(); + + // set to blackhole for default route + if (ipPrefix.isDefaultRoute()) + { + sai_status_t status = *it_status++; if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set route %s packet action to drop, rv:%d", @@ -1158,12 +1381,9 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) return false; } - SWSS_LOG_INFO("Set route %s packet action to drop", ipPrefix.to_string().c_str()); - - attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; - attr.value.oid = SAI_NULL_OBJECT_ID; + SWSS_LOG_NOTICE("Set route %s packet action to drop", ipPrefix.to_string().c_str()); - status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); + status = *it_status++; if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to set route %s next hop ID to NULL, rv:%d", @@ -1171,23 +1391,18 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) return false; } - SWSS_LOG_INFO("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str()); + SWSS_LOG_NOTICE("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str()); } else { - //sai_status_t status = sai_route_api->remove_route_entry(&route_entry); - sai_status_t object_status; - sai_status_t status = gRouteBulker.remove_entry(&object_status, &route_entry); - gRouteBulker.flush(); - status = object_status; - + sai_status_t status = *it_status++; if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove route prefix:%s\n", ipPrefix.to_string().c_str()); return false; } - if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + if (ipPrefix.isV4()) { gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); } @@ -1195,7 +1410,6 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) { gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE); } - } /* @@ -1234,6 +1448,6 @@ bool RouteOrch::removeRoute(sai_object_id_t vrf_id, const IpPrefix &ipPrefix) m_vrfOrch->decreaseVrfRefCount(vrf_id); } } - + return true; -} +} \ No newline at end of file diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 817b4ff948..03c8187950 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -91,17 +91,29 @@ class RouteOrch : public Orch, public Subject NextHopObserverTable m_nextHopObservers; - void addTempRoute(sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool addRoute(sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool removeRoute(sai_object_id_t, const IpPrefix&); + using StatusInserter = std::deque&; + std::map< + std::pair< + std::string, // Key + std::string // Op + >, + std::deque // Bulk statuses + > m_toBulk; + + EntityBulker gRouteBulker; + //EntityBulker gFdbBulker; + ObjectBulker gNextHopGroupMemberBulkder; + + bool addTempRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); + bool addRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); + bool addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops); + bool removeRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&); + bool removeRoutePost(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&); std::string getLinkLocalEui64Addr(void); void addLinkLocalRouteToMe(sai_object_id_t vrf_id, IpPrefix linklocal_prefix); void doTask(Consumer& consumer); - EntityBulker gRouteBulker; - //EntityBulker gFdbBulker; - ObjectBulker gNextHopGroupMemberBulkder; }; #endif /* SWSS_ROUTEORCH_H */ From 0c6dac0312e5d880404d9820589eadafa1b25908 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 3 Apr 2020 01:11:55 +0000 Subject: [PATCH 16/42] Test test_CrmNexthopGroup pass --- orchagent/routeorch.cpp | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 36905c08f0..dc6a7da794 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -492,10 +492,8 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoute(object_statuses, vrf_id, ip_prefix)) - it = consumer.m_toSync.erase(it); - else - it++; + removeRoute(object_statuses, vrf_id, ip_prefix); + it++; continue; } @@ -533,20 +531,16 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) - it = consumer.m_toSync.erase(it); - else - it++; + addRoute(object_statuses, vrf_id, ip_prefix, nhg); + it++; } } else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) - it = consumer.m_toSync.erase(it); - else - it++; + addRoute(object_statuses, vrf_id, ip_prefix, nhg); + it++; } else /* Duplicate entry */ @@ -554,11 +548,8 @@ void RouteOrch::doTask(Consumer& consumer) } else if (op == DEL_COMMAND) { - /* Cannot locate the route or remove succeed */ - if (removeRoute(object_statuses, vrf_id, ip_prefix)) - it = consumer.m_toSync.erase(it); - else - it++; + removeRoute(object_statuses, vrf_id, ip_prefix); + it++; } else { From 388478978417690ae4fd4a5a89f7d95911c859d9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 00:16:38 +0000 Subject: [PATCH 17/42] When nexthopgroup count reach maximal, flush route bulk --- orchagent/bulker.h | 30 ++++++++++++++++++++++++++++++ orchagent/routeorch.cpp | 15 +++++++++++++-- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 5c9d991cb9..5a0be61d3f 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -369,6 +369,21 @@ class EntityBulker setting_entries.clear(); } + size_t creating_entries_count() + { + return creating_entries.size(); + } + + size_t setting_entries_count() + { + return setting_entries.size(); + } + + size_t removing_entries_count() + { + return removing_entries.size(); + } + private: std::unordered_map< Te, @@ -573,6 +588,21 @@ class ObjectBulker setting_entries.clear(); } + size_t creating_entries_count() + { + return creating_entries.size(); + } + + size_t setting_entries_count() + { + return setting_entries.size(); + } + + size_t removing_entries_count() + { + return removing_entries.size(); + } + private: struct object_entry { diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index dc6a7da794..a1d8014630 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -505,6 +505,7 @@ void RouteOrch::doTask(Consumer& consumer) NextHopGroupKey nhg(nhg_str); + bool addedRoute = false; if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { /* blackhole to be done */ @@ -531,7 +532,7 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - addRoute(object_statuses, vrf_id, ip_prefix, nhg); + addedRoute = addRoute(object_statuses, vrf_id, ip_prefix, nhg); it++; } } @@ -539,12 +540,22 @@ void RouteOrch::doTask(Consumer& consumer) m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - addRoute(object_statuses, vrf_id, ip_prefix, nhg); + addedRoute = addRoute(object_statuses, vrf_id, ip_prefix, nhg); it++; } else /* Duplicate entry */ it = consumer.m_toSync.erase(it); + + // If just added a route, and already exhaust the nexthop groups, and there are pending removing routes in bulker, + // flush the bulker and possibly collect some released nexthop groups + if (addedRoute + && m_nextHopGroupCount >= m_maxNextHopGroupCount + && gRouteBulker.removing_entries_count() > 0 + ) + { + break; + } } else if (op == DEL_COMMAND) { From 00115657d8becd8ef72f0a333e4d0816d57d0df9 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 00:36:54 +0000 Subject: [PATCH 18/42] Change log messages --- orchagent/routeorch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index a1d8014630..fb535614b3 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1266,7 +1266,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf /* Increase the ref_count for the next hop (group) entry */ increaseNextHopRefCount(nextHops); - SWSS_LOG_NOTICE("Create route %s with next hop(s) %s", + SWSS_LOG_NOTICE("Post create route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } else @@ -1296,7 +1296,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf { removeNextHopGroup(it_route->second); } - SWSS_LOG_INFO("Set route %s with next hop(s) %s", + SWSS_LOG_NOTICE("Post set route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } From e1f7bfdb5a682318fd14717fac14a3cce76e17ce Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 00:40:37 +0000 Subject: [PATCH 19/42] (reformat) --- orchagent/bulker.h | 42 ++++++++++++++++++++--------------------- orchagent/routeorch.cpp | 40 +++++++++++++++++++-------------------- orchagent/routeorch.h | 4 ++-- 3 files changed, 43 insertions(+), 43 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 5a0be61d3f..fd228cc342 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -71,7 +71,7 @@ namespace std return seed; } }; - + template <> struct hash { @@ -118,7 +118,7 @@ typedef sai_status_t (*sai_bulk_set_fdb_entry_attribute_fn)( _In_ const sai_attribute_t *attr_list, _In_ sai_bulk_op_error_mode_t mode, _Out_ sai_status_t *object_statuses); - + template struct saitraits { }; @@ -191,7 +191,7 @@ class EntityBulker *object_status = SAI_STATUS_ITEM_ALREADY_EXISTS; return *object_status; } - + auto& attrs = it->second.first; attrs.insert(attrs.end(), attr_list, attr_list + attr_count); it->second.second = object_status; @@ -228,7 +228,7 @@ class EntityBulker std::forward_as_tuple(object_status)); bool inserted = rc.second; SWSS_LOG_INFO("EntityBulker.remove_entry %zu, %d\n", removing_entries.size(), inserted); - + *object_status = SAI_STATUS_NOT_EXECUTED; return *object_status; } @@ -243,14 +243,14 @@ class EntityBulker 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; @@ -269,7 +269,7 @@ class EntityBulker if (!removing_entries.empty()) { vector rs; - + for (auto& i: removing_entries) { auto& entry = i.first; @@ -279,7 +279,7 @@ class EntityBulker vector statuses(count); (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_NOTICE("EntityBulker.flush removing_entries %zu\n", removing_entries.size()); - + for (size_t ir = 0; ir < count; ir++) { auto& entry = rs[ir]; @@ -373,12 +373,12 @@ class EntityBulker { return creating_entries.size(); } - + size_t setting_entries_count() { return setting_entries.size(); } - + size_t removing_entries_count() { return removing_entries.size(); @@ -406,7 +406,7 @@ class EntityBulker Te, sai_status_t * > removing_entries; - + typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; typename Ts::bulk_set_entry_attribute_fn set_entries_attribute; @@ -442,17 +442,17 @@ class ObjectBulker { throw std::logic_error("Not implemented"); } - + sai_status_t create_entry( _Out_ sai_object_id_t *object_id, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); - + auto& last_attrs = std::get<1>(creating_entries.back()); SWSS_LOG_INFO("ObjectBulker.create_entry %zu, %zu, %u\n", creating_entries.size(), last_attrs.size(), last_attrs[0].id); - + *object_id = SAI_NULL_OBJECT_ID; // not created immediately, postponed until flush return SAI_STATUS_NOT_EXECUTED; } @@ -469,7 +469,7 @@ class ObjectBulker { setting_entries.erase(found_setting); } - + removing_entries.emplace_back(object_id); removing_statuses.emplace_back(object_status); *object_status = SAI_STATUS_NOT_EXECUTED; @@ -592,12 +592,12 @@ class ObjectBulker { return creating_entries.size(); } - + size_t setting_entries_count() { return setting_entries.size(); } - + size_t removing_entries_count() { return removing_entries.size(); @@ -615,14 +615,14 @@ class ObjectBulker { } }; - + sai_object_id_t switch_id; std::vector // - attrs >> creating_entries; - + std::unordered_map< // An map of sai_object_id_t, // object_id -> (object_status, attributes) std::pair< @@ -630,10 +630,10 @@ class ObjectBulker std::vector > > setting_entries; - + std::vector removing_entries; std::vector removing_statuses; - + typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; // TODO: wait until available in SAI diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index fb535614b3..56afa53245 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -384,7 +384,7 @@ void RouteOrch::doTask(Consumer& consumer) string key = kfvKey(t); string op = kfvOp(t); - + auto inserted = m_toBulk.emplace(std::piecewise_construct, std::forward_as_tuple(key, op), std::forward_as_tuple()); @@ -546,7 +546,7 @@ void RouteOrch::doTask(Consumer& consumer) else /* Duplicate entry */ it = consumer.m_toSync.erase(it); - + // If just added a route, and already exhaust the nexthop groups, and there are pending removing routes in bulker, // flush the bulker and possibly collect some released nexthop groups if (addedRoute @@ -568,9 +568,9 @@ void RouteOrch::doTask(Consumer& consumer) it = consumer.m_toSync.erase(it); } } - + gRouteBulker.flush(); - + it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { @@ -579,26 +579,26 @@ void RouteOrch::doTask(Consumer& consumer) it++; continue; } - + KeyOpFieldsValuesTuple t = it->second; string key = kfvKey(t); string op = kfvOp(t); - + auto found = m_toBulk.find(make_pair(key, op)); if (found == m_toBulk.end()) { it++; continue; } - + auto& object_statuses = found->second; if (object_statuses.empty()) { it++; continue; } - + sai_object_id_t vrf_id; IpPrefix ip_prefix; @@ -855,7 +855,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) it.to_string().c_str(), nexthops.to_string().c_str()); return false; } - + // skip next hop group member create for neighbor from down port if (m_neighOrch->isNextHopFlagSet(it, NHFLAGS_IFDOWN)) { continue; @@ -922,7 +922,7 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) // (uint32_t)nhgm_attrs.size(), // nhgm_attrs.data()); } - + gNextHopGroupMemberBulkder.flush(); for (size_t i = 0; i < npid_count; i++) { @@ -987,11 +987,11 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) nhop = nhgm.erase(nhop); continue; } - + next_hop_ids.push_back(nhop->second); nhop = nhgm.erase(nhop); } - + size_t nhid_count = next_hop_ids.size(); vector statuses(nhid_count); for (size_t i = 0; i < nhid_count; i++) @@ -1223,7 +1223,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf // Something went wrong before router bulker, will retry return false; } - + if (nextHops.getSize() > 1) { if (!hasNextHopGroup(nextHops)) @@ -1238,7 +1238,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf return false; } } - + auto it_status = object_statuses.begin(); auto it_route = m_syncdRoutes.at(vrf_id).find(ipPrefix); if (it_route == m_syncdRoutes.at(vrf_id).end()) @@ -1254,7 +1254,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf } return false; } - + if (ipPrefix.isV4()) { gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); @@ -1278,7 +1278,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf ipPrefix.to_string().c_str(), status); return false; } - + status = *it_status++; if (status != SAI_STATUS_SUCCESS) { @@ -1286,7 +1286,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); return false; } - + /* Increase the ref_count for the next hop (group) entry */ increaseNextHopRefCount(nextHops); @@ -1340,7 +1340,7 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ //sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); object_statuses.emplace_back(); gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &attr); - + attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; attr.value.oid = SAI_NULL_OBJECT_ID; @@ -1354,7 +1354,7 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ object_statuses.emplace_back(); gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); } - + return true; } @@ -1450,6 +1450,6 @@ bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t m_vrfOrch->decreaseVrfRefCount(vrf_id); } } - + return true; } \ No newline at end of file diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 03c8187950..e4cb59fe9b 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -99,11 +99,11 @@ class RouteOrch : public Orch, public Subject >, std::deque // Bulk statuses > m_toBulk; - + EntityBulker gRouteBulker; //EntityBulker gFdbBulker; ObjectBulker gNextHopGroupMemberBulkder; - + bool addTempRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); bool addRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); bool addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops); From 1b58294b4b8dcf7e39194ec42a4de2410c2109aa Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 01:00:49 +0000 Subject: [PATCH 20/42] (comment) --- orchagent/bulker.h | 42 ++++++++++++++++------------------------- orchagent/routeorch.cpp | 13 ------------- orchagent/routeorch.h | 1 - 3 files changed, 16 insertions(+), 40 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index fd228cc342..9b98f88932 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -86,18 +86,6 @@ namespace std }; } -/* -struct NextHopGroupEntry -{ - sai_object_id_t next_hop_group_id; // next hop group id - std::set next_hop_group_members; // next hop group member ids - int ref_count; // reference count -}; -*/ - -/* NextHopGroupTable: next hop group IP addersses, NextHopGroupEntry */ -//typedef std::map NextHopGroupTable; - // SAI typedef which is not available in SAI 1.5 // TODO: remove after available typedef sai_status_t (*sai_bulk_create_fdb_entry_fn)( @@ -385,26 +373,28 @@ class EntityBulker } private: - std::unordered_map< - Te, + std::unordered_map< // A map of + Te, // entry -> std::pair< - std::vector, + std::vector, // (attributes, OUT object_status) sai_status_t * > > creating_entries; - std::unordered_map< - Te, - std::unordered_map< - sai_attr_id_t, + + std::unordered_map< // A map of + Te, // entry -> + std::unordered_map< // another map of + sai_attr_id_t, // attr_id -> std::pair< - sai_attribute_t, + sai_attribute_t, // (attr_value, OUT object_status) sai_status_t * > > > setting_entries; - std::unordered_map< - Te, - sai_status_t * + + std::unordered_map< // A map of + Te, // entry -> + sai_status_t * // OUT object_status > removing_entries; typename Ts::bulk_create_entry_fn create_entries; @@ -618,13 +608,13 @@ class ObjectBulker sai_object_id_t switch_id; - std::vector // - attrs >> creating_entries; - std::unordered_map< // An map of - sai_object_id_t, // object_id -> (object_status, attributes) + std::unordered_map< // A map of + sai_object_id_t, // object_id -> (OUT object_status, attributes) std::pair< sai_status_t *, std::vector diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 56afa53245..0f2d0b3827 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -25,7 +25,6 @@ const int routeorch_pri = 5; RouteOrch::RouteOrch(DBConnector *db, string tableName, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch) : gRouteBulker(sai_route_api), - //gFdbBulker(sai_fdb_api), gNextHopGroupMemberBulkder(sai_next_hop_group_api, gSwitchId), Orch(db, tableName, routeorch_pri), m_neighOrch(neighOrch), @@ -897,7 +896,6 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) size_t npid_count = next_hop_ids.size(); vector nhgm_ids(npid_count); for (size_t i = 0; i < npid_count; i++) - //for (auto nhid: next_hop_ids) { auto nhid = next_hop_ids[i]; @@ -913,14 +911,9 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) nhgm_attr.value.oid = nhid; nhgm_attrs.push_back(nhgm_attr); - //sai_object_id_t next_hop_group_member_id; gNextHopGroupMemberBulkder.create_entry(&nhgm_ids[i], (uint32_t)nhgm_attrs.size(), nhgm_attrs.data()); - // status = sai_next_hop_group_api->create_next_hop_group_member(&next_hop_group_member_id, - // gSwitchId, - // (uint32_t)nhgm_attrs.size(), - // nhgm_attrs.data()); } gNextHopGroupMemberBulkder.flush(); @@ -996,7 +989,6 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) vector statuses(nhid_count); for (size_t i = 0; i < nhid_count; i++) { - //status = sai_next_hop_group_api->remove_next_hop_group_member(nhop->second); gNextHopGroupMemberBulkder.remove_entry(&statuses[i], next_hop_ids[i]); } gNextHopGroupMemberBulkder.flush(); @@ -1181,7 +1173,6 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, route_attr.value.oid = next_hop_id; /* Default SAI_ROUTE_ATTR_PACKET_ACTION is SAI_PACKET_ACTION_FORWARD */ - //sai_status_t status = sai_route_api->create_route_entry(&route_entry, 1, &route_attr); object_statuses.emplace_back(); sai_status_t status = gRouteBulker.create_entry(&object_statuses.back(), &route_entry, 1, &route_attr); if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) @@ -1199,7 +1190,6 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, route_attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; route_attr.value.s32 = SAI_PACKET_ACTION_FORWARD; - //status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); object_statuses.emplace_back(); gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr); } @@ -1337,20 +1327,17 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; attr.value.s32 = SAI_PACKET_ACTION_DROP; - //sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); object_statuses.emplace_back(); gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &attr); attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; attr.value.oid = SAI_NULL_OBJECT_ID; - //status = sai_route_api->set_route_entry_attribute(&route_entry, &attr); object_statuses.emplace_back(); gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &attr); } else { - //sai_status_t status = sai_route_api->remove_route_entry(&route_entry); object_statuses.emplace_back(); gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); } diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index e4cb59fe9b..2bb5f30f3a 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -101,7 +101,6 @@ class RouteOrch : public Orch, public Subject > m_toBulk; EntityBulker gRouteBulker; - //EntityBulker gFdbBulker; ObjectBulker gNextHopGroupMemberBulkder; bool addTempRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); From 6ba9cf3186903803322e9243e68c284c248ff1a2 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 01:20:58 +0000 Subject: [PATCH 21/42] Change some messages log level Signed-off-by: Qi Luo --- orchagent/bulker.h | 17 +++++++++-------- orchagent/routeorch.cpp | 8 ++++---- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 9b98f88932..783b421f42 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -175,7 +175,7 @@ class EntityBulker bool inserted = rc.second; if (!inserted) { - SWSS_LOG_DEBUG("bulk.create_entry not inserted %zu\n", creating_entries.size()); + SWSS_LOG_INFO("EntityBulker.create_entry not inserted %zu\n", creating_entries.size()); *object_status = SAI_STATUS_ITEM_ALREADY_EXISTS; return *object_status; } @@ -183,7 +183,7 @@ class EntityBulker auto& attrs = it->second.first; attrs.insert(attrs.end(), attr_list, attr_list + attr_count); it->second.second = object_status; - SWSS_LOG_DEBUG("bulk.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), it->second.first.size(), (int)it->second.first[0].id, inserted); + SWSS_LOG_INFO("EntityBulker.create_entry %zu, %zu, %d, %d\n", creating_entries.size(), it->second.first.size(), (int)it->second.first[0].id, inserted); *object_status = SAI_STATUS_NOT_EXECUTED; return *object_status; } @@ -266,7 +266,7 @@ class EntityBulker size_t count = rs.size(); vector statuses(count); (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); - SWSS_LOG_NOTICE("EntityBulker.flush removing_entries %zu\n", removing_entries.size()); + SWSS_LOG_INFO("EntityBulker.flush removing_entries %zu\n", removing_entries.size()); for (size_t ir = 0; ir < count; ir++) { @@ -300,7 +300,7 @@ class EntityBulker vector statuses(count); (*create_entries)((uint32_t)count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); - SWSS_LOG_NOTICE("EntityBulker.flush creating_entries %zu\n", creating_entries.size()); + SWSS_LOG_INFO("EntityBulker.flush creating_entries %zu\n", creating_entries.size()); for (size_t ir = 0; ir < count; ir++) { @@ -334,7 +334,7 @@ class EntityBulker vector statuses(count); (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); - SWSS_LOG_NOTICE("EntityBulker.flush setting_entries %zu, count %zu\n", setting_entries.size(), count); + SWSS_LOG_INFO("EntityBulker.flush setting_entries %zu, count %zu\n", setting_entries.size(), count); for (size_t ir = 0; ir < count; ir++) { @@ -343,6 +343,7 @@ class EntityBulker sai_status_t *object_status = setting_entries[entry][attr_id].second; if (object_status) { + SWSS_LOG_INFO("EntityBulker.flush setting_entries status[%zu]=%d(0x%8p)\n", ir, statuses[ir], object_status); *object_status = statuses[ir]; } } @@ -498,7 +499,7 @@ class ObjectBulker size_t count = removing_entries.size(); vector statuses(count); sai_status_t status = (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("ObjectBulker.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); + SWSS_LOG_INFO("ObjectBulker.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); for (size_t i = 0; i < count; i++) { @@ -529,7 +530,7 @@ class ObjectBulker vector statuses(count); (*create_entries)(switch_id, (uint32_t)count, cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, object_ids.data(), statuses.data()); - SWSS_LOG_NOTICE("ObjectBulker.flush creating_entries %zu\n", creating_entries.size()); + SWSS_LOG_INFO("ObjectBulker.flush creating_entries %zu\n", creating_entries.size()); for (size_t i = 0; i < count; i++) { @@ -563,7 +564,7 @@ class ObjectBulker (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); - SWSS_LOG_NOTICE("ObjectBulker.flush setting_entries %zu\n", setting_entries.size()); + SWSS_LOG_INFO("ObjectBulker.flush setting_entries %zu\n", setting_entries.size()); setting_entries.clear(); } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 0f2d0b3827..4f50750048 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1256,7 +1256,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf /* Increase the ref_count for the next hop (group) entry */ increaseNextHopRefCount(nextHops); - SWSS_LOG_NOTICE("Post create route %s with next hop(s) %s", + SWSS_LOG_INFO("Post create route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } else @@ -1286,7 +1286,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf { removeNextHopGroup(it_route->second); } - SWSS_LOG_NOTICE("Post set route %s with next hop(s) %s", + SWSS_LOG_INFO("Post set route %s with next hop(s) %s", ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } @@ -1370,7 +1370,7 @@ bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t return false; } - SWSS_LOG_NOTICE("Set route %s packet action to drop", ipPrefix.to_string().c_str()); + SWSS_LOG_INFO("Set route %s packet action to drop", ipPrefix.to_string().c_str()); status = *it_status++; if (status != SAI_STATUS_SUCCESS) @@ -1380,7 +1380,7 @@ bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t return false; } - SWSS_LOG_NOTICE("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str()); + SWSS_LOG_INFO("Set route %s next hop ID to NULL", ipPrefix.to_string().c_str()); } else { From a64fee341a3f19a2fc429c683a3778b1920fe018 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 4 Apr 2020 01:58:42 +0000 Subject: [PATCH 22/42] Mark old one as done Signed-off-by: Qi Luo --- orchagent/bulker.h | 13 +++++++++++-- orchagent/routeorch.cpp | 3 ++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 783b421f42..88436f7a5d 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -198,15 +198,24 @@ class EntityBulker auto found_setting = setting_entries.find(*entry); if (found_setting != setting_entries.end()) { - // TODO: mark old ones as done + // Mark old one as done + auto& attrmap = found_setting->second; + for (auto& attr: attrmap) + { + *attr.second.second = SAI_STATUS_SUCCESS; + } + // Erase old one setting_entries.erase(found_setting); } auto found_creating = creating_entries.find(*entry); if (found_creating != creating_entries.end()) { - // TODO: mark old ones as done + // Mark old ones as done + *found_creating->second.second = SAI_STATUS_SUCCESS; + // Erase old one creating_entries.erase(found_creating); + // No need to keep in bulker, claim success immediately *object_status = SAI_STATUS_SUCCESS; SWSS_LOG_INFO("EntityBulker.remove_entry quickly removed %zu, creating_entries.size=%zu\n", removing_entries.size(), creating_entries.size()); return *object_status; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 4f50750048..f38ede766f 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1439,4 +1439,5 @@ bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t } return true; -} \ No newline at end of file +} + From 74093598fdf978625c00456105e73c90243bf457 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sun, 5 Apr 2020 03:56:15 +0000 Subject: [PATCH 23/42] Fix addRoute, removeRoute return values --- orchagent/bulker.h | 17 +++++++---- orchagent/routeorch.cpp | 66 +++++++++++++++++++++-------------------- 2 files changed, 45 insertions(+), 38 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 88436f7a5d..34de531816 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -367,21 +367,26 @@ class EntityBulker setting_entries.clear(); } - size_t creating_entries_count() + size_t creating_entries_count() const { return creating_entries.size(); } - size_t setting_entries_count() + size_t setting_entries_count() const { return setting_entries.size(); } - size_t removing_entries_count() + size_t removing_entries_count() const { return removing_entries.size(); } + size_t creating_entries_count(const Te& entry) const + { + return creating_entries.count(entry); + } + private: std::unordered_map< // A map of Te, // entry -> @@ -588,17 +593,17 @@ class ObjectBulker setting_entries.clear(); } - size_t creating_entries_count() + size_t creating_entries_count() const { return creating_entries.size(); } - size_t setting_entries_count() + size_t setting_entries_count() const { return setting_entries.size(); } - size_t removing_entries_count() + size_t removing_entries_count() const { return removing_entries.size(); } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index f38ede766f..aaa9c55c5b 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -491,8 +491,10 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - removeRoute(object_statuses, vrf_id, ip_prefix); - it++; + if (removeRoute(object_statuses, vrf_id, ip_prefix)) + it = consumer.m_toSync.erase(it); + else + it++; continue; } @@ -504,7 +506,6 @@ void RouteOrch::doTask(Consumer& consumer) NextHopGroupKey nhg(nhg_str); - bool addedRoute = false; if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { /* blackhole to be done */ @@ -531,35 +532,38 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - addedRoute = addRoute(object_statuses, vrf_id, ip_prefix, nhg); - it++; + if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + it = consumer.m_toSync.erase(it); + else + it++; } } else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - addedRoute = addRoute(object_statuses, vrf_id, ip_prefix, nhg); - it++; + if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + it = consumer.m_toSync.erase(it); + else + it++; } else /* Duplicate entry */ it = consumer.m_toSync.erase(it); - // If just added a route, and already exhaust the nexthop groups, and there are pending removing routes in bulker, + // If already exhaust the nexthop groups, and there are pending removing routes in bulker, // flush the bulker and possibly collect some released nexthop groups - if (addedRoute - && m_nextHopGroupCount >= m_maxNextHopGroupCount - && gRouteBulker.removing_entries_count() > 0 - ) + if (m_nextHopGroupCount >= m_maxNextHopGroupCount && gRouteBulker.removing_entries_count() > 0) { break; } } else if (op == DEL_COMMAND) { - removeRoute(object_statuses, vrf_id, ip_prefix); - it++; + if (removeRoute(object_statuses, vrf_id, ip_prefix)) + it = consumer.m_toSync.erase(it); + else + it++; } else { @@ -1053,15 +1057,17 @@ bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf /* Set the route's temporary next hop to be the randomly picked one */ NextHopGroupKey tmp_next_hop((*it).to_string()); - bool rc = addRoute(object_statuses, vrf_id, ipPrefix, tmp_next_hop); - if (rc) + size_t sz = gRouteBulker.creating_entries_count(); + addRoute(object_statuses, vrf_id, ipPrefix, tmp_next_hop); + if (gRouteBulker.creating_entries_count() > sz) { // TRICK! TRICK! TRICK! // Even we only successfully invoke bulker, not SAI, we write the temp route // into m_syncdRoutes so addRoutePost will know what happened m_syncdRoutes[vrf_id][ipPrefix] = tmp_next_hop; + return true; } - return rc; + return false; } bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) @@ -1139,14 +1145,9 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, /* Push failure into object_statuses so addRoutePost will fail * so the original route is not successfully added, and will retry */ object_statuses.emplace_back(SAI_STATUS_INSUFFICIENT_RESOURCES); - /* Return true so addRoutePost will be called */ - return true; - } - else - { - // Not added to route bulker - return false; } + // Only added to route bulker, not immediately finished + return false; } } @@ -1201,7 +1202,7 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, object_statuses.emplace_back(); gRouteBulker.set_entry_attribute(&object_statuses.back(), &route_entry, &route_attr); } - return true; + return false; } bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) @@ -1307,19 +1308,20 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ return true; } + sai_route_entry_t route_entry; + route_entry.vr_id = vrf_id; + route_entry.switch_id = gSwitchId; + copy(route_entry.destination, ipPrefix); + auto it_route = it_route_table->second.find(ipPrefix); - if (it_route == it_route_table->second.end()) + size_t creating = gRouteBulker.creating_entries_count(route_entry); + if (it_route == it_route_table->second.end() && creating == 0) { SWSS_LOG_INFO("Failed to find route entry, vrf_id 0x%" PRIx64 ", prefix %s\n", vrf_id, ipPrefix.to_string().c_str()); return true; } - sai_route_entry_t route_entry; - route_entry.vr_id = vrf_id; - route_entry.switch_id = gSwitchId; - copy(route_entry.destination, ipPrefix); - // set to blackhole for default route if (ipPrefix.isDefaultRoute()) { @@ -1342,7 +1344,7 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); } - return true; + return false; } bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) From 2364f8b8e3148f556e227f7b9e0fdba5af94520e Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Mon, 6 Apr 2020 23:14:51 +0000 Subject: [PATCH 24/42] If already exhaust the nexthop groups, and there are pending removing routes in bulker, flush the bulker and possibly collect some released nexthop groups --- orchagent/routeorch.cpp | 485 ++++++++++++++++++++-------------------- 1 file changed, 244 insertions(+), 241 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index aaa9c55c5b..4b5fa933d5 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -379,324 +379,327 @@ void RouteOrch::doTask(Consumer& consumer) auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { - KeyOpFieldsValuesTuple t = it->second; - - string key = kfvKey(t); - string op = kfvOp(t); - - auto inserted = m_toBulk.emplace(std::piecewise_construct, - std::forward_as_tuple(key, op), - std::forward_as_tuple()); - auto& object_statuses = inserted.first->second; - - /* Get notification from application */ - /* resync application: - * When routeorch receives 'resync' message, it marks all current - * routes as dirty and waits for 'resync complete' message. For all - * newly received routes, if they match current dirty routes, it unmarks - * them dirty. After receiving 'resync complete' message, it creates all - * newly added routes and removes all dirty routes. - */ - if (key == "resync") + while (it != consumer.m_toSync.end()) { - if (op == "SET") + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + + auto inserted = m_toBulk.emplace(std::piecewise_construct, + std::forward_as_tuple(key, op), + std::forward_as_tuple()); + auto& object_statuses = inserted.first->second; + + /* Get notification from application */ + /* resync application: + * When routeorch receives 'resync' message, it marks all current + * routes as dirty and waits for 'resync complete' message. For all + * newly received routes, if they match current dirty routes, it unmarks + * them dirty. After receiving 'resync complete' message, it creates all + * newly added routes and removes all dirty routes. + */ + if (key == "resync") { - /* Mark all current routes as dirty (DEL) in consumer.m_toSync map */ - SWSS_LOG_NOTICE("Start resync routes\n"); - for (auto j : m_syncdRoutes) + if (op == "SET") { - string vrf; - - if (j.first != gVirtualRouterId) - { - vrf = m_vrfOrch->getVRFname(j.first) + ":"; - } - - for (auto i : j.second) + /* Mark all current routes as dirty (DEL) in consumer.m_toSync map */ + SWSS_LOG_NOTICE("Start resync routes\n"); + for (auto j : m_syncdRoutes) { - vector v; - key = vrf + i.first.to_string(); - auto x = KeyOpFieldsValuesTuple(key, DEL_COMMAND, v); - consumer.addToSync(x); + string vrf; + + if (j.first != gVirtualRouterId) + { + vrf = m_vrfOrch->getVRFname(j.first) + ":"; + } + + for (auto i : j.second) + { + vector v; + key = vrf + i.first.to_string(); + auto x = KeyOpFieldsValuesTuple(key, DEL_COMMAND, v); + consumer.addToSync(x); + } } + m_resync = true; + } + else + { + SWSS_LOG_NOTICE("Complete resync routes\n"); + m_resync = false; } - m_resync = true; - } - else - { - SWSS_LOG_NOTICE("Complete resync routes\n"); - m_resync = false; - } - - it = consumer.m_toSync.erase(it); - continue; - } - - if (m_resync) - { - it++; - continue; - } - - sai_object_id_t vrf_id; - IpPrefix ip_prefix; - if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) - { - size_t found = key.find(':'); - string vrf_name = key.substr(0, found); + it = consumer.m_toSync.erase(it); + continue; + } - if (!m_vrfOrch->isVRFexists(vrf_name)) + if (m_resync) { it++; continue; } - vrf_id = m_vrfOrch->getVRFid(vrf_name); - ip_prefix = IpPrefix(key.substr(found+1)); - } - else - { - vrf_id = gVirtualRouterId; - ip_prefix = IpPrefix(key); - } - if (op == SET_COMMAND) - { - string ips; - string aliases; - bool excp_intfs_flag = false; + sai_object_id_t vrf_id; + IpPrefix ip_prefix; - for (auto i : kfvFieldsValues(t)) + if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) { - if (fvField(i) == "nexthop") - ips = fvValue(i); - - if (fvField(i) == "ifname") - aliases = fvValue(i); - } - vector ipv = tokenize(ips, ','); - vector alsv = tokenize(aliases, ','); + size_t found = key.find(':'); + string vrf_name = key.substr(0, found); - for (auto alias : alsv) - { - if (alias == "eth0" || alias == "lo" || alias == "docker0") + if (!m_vrfOrch->isVRFexists(vrf_name)) { - excp_intfs_flag = true; - break; + it++; + continue; } + vrf_id = m_vrfOrch->getVRFid(vrf_name); + ip_prefix = IpPrefix(key.substr(found+1)); } - - // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty - if (excp_intfs_flag) + else { - /* If any existing routes are updated to point to the - * above interfaces, remove them from the ASIC. */ - if (removeRoute(object_statuses, vrf_id, ip_prefix)) - it = consumer.m_toSync.erase(it); - else - it++; - continue; + vrf_id = gVirtualRouterId; + ip_prefix = IpPrefix(key); } - string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; - for (uint32_t i = 1; i < ipv.size(); i++) + if (op == SET_COMMAND) { - nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; - } + string ips; + string aliases; + bool excp_intfs_flag = false; - NextHopGroupKey nhg(nhg_str); + for (auto i : kfvFieldsValues(t)) + { + if (fvField(i) == "nexthop") + ips = fvValue(i); - if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) - { - /* blackhole to be done */ - if (alsv[0] == "unknown") + if (fvField(i) == "ifname") + aliases = fvValue(i); + } + vector ipv = tokenize(ips, ','); + vector alsv = tokenize(aliases, ','); + + for (auto alias : alsv) { - /* add addBlackholeRoute or addRoute support empty nhg */ - it = consumer.m_toSync.erase(it); + if (alias == "eth0" || alias == "lo" || alias == "docker0") + { + excp_intfs_flag = true; + break; + } } - /* directly connected route to VRF interface which come from kernel */ - else if (!alsv[0].compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) + + // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty + if (excp_intfs_flag) { - it = consumer.m_toSync.erase(it); + /* If any existing routes are updated to point to the + * above interfaces, remove them from the ASIC. */ + if (removeRoute(object_statuses, vrf_id, ip_prefix)) + it = consumer.m_toSync.erase(it); + else + it++; + continue; } - /* skip prefix which is linklocal or multicast */ - else if (ip_prefix.getIp().getAddrScope() != IpAddress::GLOBAL_SCOPE) + + string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; + for (uint32_t i = 1; i < ipv.size(); i++) { - it = consumer.m_toSync.erase(it); + nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; } - /* fullmask subnet route is same as ip2me route */ - else if (ip_prefix.isFullMask() && m_intfsOrch->isPrefixSubnet(ip_prefix, alsv[0])) + + NextHopGroupKey nhg(nhg_str); + + if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { - it = consumer.m_toSync.erase(it); + /* blackhole to be done */ + if (alsv[0] == "unknown") + { + /* add addBlackholeRoute or addRoute support empty nhg */ + it = consumer.m_toSync.erase(it); + } + /* directly connected route to VRF interface which come from kernel */ + else if (!alsv[0].compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) + { + it = consumer.m_toSync.erase(it); + } + /* skip prefix which is linklocal or multicast */ + else if (ip_prefix.getIp().getAddrScope() != IpAddress::GLOBAL_SCOPE) + { + it = consumer.m_toSync.erase(it); + } + /* fullmask subnet route is same as ip2me route */ + else if (ip_prefix.isFullMask() && m_intfsOrch->isPrefixSubnet(ip_prefix, alsv[0])) + { + it = consumer.m_toSync.erase(it); + } + /* subnet route, vrf leaked route, etc */ + else + { + if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + it = consumer.m_toSync.erase(it); + else + it++; + } } - /* subnet route, vrf leaked route, etc */ - else + else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || + m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || + m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) - it = consumer.m_toSync.erase(it); + it = consumer.m_toSync.erase(it); else it++; } + else + /* Duplicate entry */ + it = consumer.m_toSync.erase(it); + + // If already exhaust the nexthop groups, and there are pending removing routes in bulker, + // flush the bulker and possibly collect some released nexthop groups + if (m_nextHopGroupCount >= m_maxNextHopGroupCount && gRouteBulker.removing_entries_count() > 0) + { + break; + } } - else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || - m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || - m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) + else if (op == DEL_COMMAND) { - if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + if (removeRoute(object_statuses, vrf_id, ip_prefix)) it = consumer.m_toSync.erase(it); else it++; } else - /* Duplicate entry */ - it = consumer.m_toSync.erase(it); - - // If already exhaust the nexthop groups, and there are pending removing routes in bulker, - // flush the bulker and possibly collect some released nexthop groups - if (m_nextHopGroupCount >= m_maxNextHopGroupCount && gRouteBulker.removing_entries_count() > 0) { - break; - } - } - else if (op == DEL_COMMAND) - { - if (removeRoute(object_statuses, vrf_id, ip_prefix)) + SWSS_LOG_ERROR("Unknown operation type %s\n", op.c_str()); it = consumer.m_toSync.erase(it); - else - it++; - } - else - { - SWSS_LOG_ERROR("Unknown operation type %s\n", op.c_str()); - it = consumer.m_toSync.erase(it); - } - } - - gRouteBulker.flush(); - - it = consumer.m_toSync.begin(); - while (it != consumer.m_toSync.end()) - { - if (m_resync) - { - it++; - continue; + } } - KeyOpFieldsValuesTuple t = it->second; - - string key = kfvKey(t); - string op = kfvOp(t); - - auto found = m_toBulk.find(make_pair(key, op)); - if (found == m_toBulk.end()) - { - it++; - continue; - } + gRouteBulker.flush(); - auto& object_statuses = found->second; - if (object_statuses.empty()) + auto it_prev = consumer.m_toSync.begin(); + while (it_prev != it) { - it++; - continue; - } + if (m_resync) + { + it_prev++; + continue; + } - sai_object_id_t vrf_id; - IpPrefix ip_prefix; + KeyOpFieldsValuesTuple t = it_prev->second; - if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) - { - size_t found = key.find(':'); - string vrf_name = key.substr(0, found); + string key = kfvKey(t); + string op = kfvOp(t); - if (!m_vrfOrch->isVRFexists(vrf_name)) + auto found = m_toBulk.find(make_pair(key, op)); + if (found == m_toBulk.end()) { - it++; + it_prev++; continue; } - vrf_id = m_vrfOrch->getVRFid(vrf_name); - ip_prefix = IpPrefix(key.substr(found+1)); - } - else - { - vrf_id = gVirtualRouterId; - ip_prefix = IpPrefix(key); - } - - if (op == SET_COMMAND) - { - string ips; - string aliases; - bool excp_intfs_flag = false; - for (auto i : kfvFieldsValues(t)) + auto& object_statuses = found->second; + if (object_statuses.empty()) { - if (fvField(i) == "nexthop") - ips = fvValue(i); - - if (fvField(i) == "ifname") - aliases = fvValue(i); + it_prev++; + continue; } - vector ipv = tokenize(ips, ','); - vector alsv = tokenize(aliases, ','); - for (auto alias : alsv) + sai_object_id_t vrf_id; + IpPrefix ip_prefix; + + if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) { - if (alias == "eth0" || alias == "lo" || alias == "docker0") + size_t found = key.find(':'); + string vrf_name = key.substr(0, found); + + if (!m_vrfOrch->isVRFexists(vrf_name)) { - excp_intfs_flag = true; - break; + it_prev++; + continue; } + vrf_id = m_vrfOrch->getVRFid(vrf_name); + ip_prefix = IpPrefix(key.substr(found+1)); } - - // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty - if (excp_intfs_flag) + else { - /* If any existing routes are updated to point to the - * above interfaces, remove them from the ASIC. */ - if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) - it = consumer.m_toSync.erase(it); - else - it++; - continue; + vrf_id = gVirtualRouterId; + ip_prefix = IpPrefix(key); } - string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; - for (uint32_t i = 1; i < ipv.size(); i++) + if (op == SET_COMMAND) { - nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; - } + string ips; + string aliases; + bool excp_intfs_flag = false; - NextHopGroupKey nhg(nhg_str); + for (auto i : kfvFieldsValues(t)) + { + if (fvField(i) == "nexthop") + ips = fvValue(i); - if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) - { - if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) - it = consumer.m_toSync.erase(it); - else - it++; + if (fvField(i) == "ifname") + aliases = fvValue(i); + } + vector ipv = tokenize(ips, ','); + vector alsv = tokenize(aliases, ','); + + for (auto alias : alsv) + { + if (alias == "eth0" || alias == "lo" || alias == "docker0") + { + excp_intfs_flag = true; + break; + } + } + + // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty + if (excp_intfs_flag) + { + /* If any existing routes are updated to point to the + * above interfaces, remove them from the ASIC. */ + if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + it_prev = consumer.m_toSync.erase(it_prev); + else + it_prev++; + continue; + } + + string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; + for (uint32_t i = 1; i < ipv.size(); i++) + { + nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; + } + + NextHopGroupKey nhg(nhg_str); + + if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) + { + if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + it_prev = consumer.m_toSync.erase(it_prev); + else + it_prev++; + } + else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || + m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || + m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) + { + if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + it_prev = consumer.m_toSync.erase(it_prev); + else + it_prev++; + } } - else if (m_syncdRoutes.find(vrf_id) == m_syncdRoutes.end() || - m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || - m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) + else if (op == DEL_COMMAND) { - if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) - it = consumer.m_toSync.erase(it); + /* Cannot locate the route or remove succeed */ + if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + it_prev = consumer.m_toSync.erase(it_prev); else - it++; + it_prev++; } } - else if (op == DEL_COMMAND) - { - /* Cannot locate the route or remove succeed */ - if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) - it = consumer.m_toSync.erase(it); - else - it++; - } + m_toBulk.clear(); } - m_toBulk.clear(); } void RouteOrch::notifyNextHopChangeObservers(sai_object_id_t vrf_id, const IpPrefix &prefix, const NextHopGroupKey &nexthops, bool add) From 87a994ef1e39697e81daae44fcd49f255b570360 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 11 Apr 2020 16:13:06 +0000 Subject: [PATCH 25/42] Fix addRoutePost --- orchagent/routeorch.cpp | 62 ++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 4b5fa933d5..4fedd95355 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1218,17 +1218,47 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf return false; } - if (nextHops.getSize() > 1) + /* next_hop_id indicates the next hop id or next hop group id of this route */ + sai_object_id_t next_hop_id; + + /* The route is pointing to a next hop */ + if (nextHops.getSize() == 1) { - if (!hasNextHopGroup(nextHops)) + NextHopKey nexthop(nextHops.to_string()); + if (nexthop.ip_address.isZero()) { - // Previous added an temporary route - auto& tmp_next_hop = m_syncdRoutes[vrf_id][ipPrefix]; - bool rc = addRoutePost(object_statuses, vrf_id, ipPrefix, tmp_next_hop); - if (!rc) + next_hop_id = m_intfsOrch->getRouterIntfsId(nexthop.alias); + /* rif is not created yet */ + if (next_hop_id == SAI_NULL_OBJECT_ID) { - m_syncdRoutes[vrf_id].erase(ipPrefix); + SWSS_LOG_INFO("Failed to get next hop %s for %s", + nextHops.to_string().c_str(), ipPrefix.to_string().c_str()); + return false; + } + } + else + { + if (m_neighOrch->hasNextHop(nexthop)) + { + next_hop_id = m_neighOrch->getNextHopId(nexthop); } + else + { + SWSS_LOG_INFO("Failed to get next hop %s for %s", + nextHops.to_string().c_str(), ipPrefix.to_string().c_str()); + return false; + } + } + } + /* The route is pointing to a next hop group */ + else + { + if (!hasNextHopGroup(nextHops)) + { + // Previous added an temporary route + auto tmp_next_hop = m_syncdRoutes[vrf_id][ipPrefix]; + m_syncdRoutes[vrf_id].erase(ipPrefix); + addRoutePost(object_statuses, vrf_id, ipPrefix, tmp_next_hop); return false; } } @@ -1265,13 +1295,19 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf } else { - sai_status_t status = *it_status++; - if (status != SAI_STATUS_SUCCESS) + sai_status_t status; + + /* Set the packet action to forward when there was no next hop (dropped) */ + if (it_route->second.getSize() == 0) { - SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", - ipPrefix.to_string().c_str(), status); - return false; - } + status = *it_status++; + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", + ipPrefix.to_string().c_str(), status); + return false; + } + } status = *it_status++; if (status != SAI_STATUS_SUCCESS) From 021848601fe265af94194061a0f3a3abda0deb1e Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 11 Apr 2020 16:48:33 +0000 Subject: [PATCH 26/42] Fix addTempRoute --- orchagent/routeorch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 4fedd95355..ad309b3703 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1060,9 +1060,9 @@ bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf /* Set the route's temporary next hop to be the randomly picked one */ NextHopGroupKey tmp_next_hop((*it).to_string()); - size_t sz = gRouteBulker.creating_entries_count(); + size_t sz = object_statuses.size(); addRoute(object_statuses, vrf_id, ipPrefix, tmp_next_hop); - if (gRouteBulker.creating_entries_count() > sz) + if (object_statuses.size() > sz) { // TRICK! TRICK! TRICK! // Even we only successfully invoke bulker, not SAI, we write the temp route From 308d159d0f5719b2d2d48b1aad8bf536b070b002 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 11 Apr 2020 19:38:06 +0000 Subject: [PATCH 27/42] Initilize empty object_statuses if already exists --- orchagent/routeorch.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index ad309b3703..8389f8f095 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -386,10 +386,16 @@ void RouteOrch::doTask(Consumer& consumer) string key = kfvKey(t); string op = kfvOp(t); - auto inserted = m_toBulk.emplace(std::piecewise_construct, + auto rc = m_toBulk.emplace(std::piecewise_construct, std::forward_as_tuple(key, op), std::forward_as_tuple()); - auto& object_statuses = inserted.first->second; + + bool inserted = rc.second; + auto& object_statuses = rc.first->second; + if (!inserted) + { + object_statuses.clear(); + } /* Get notification from application */ /* resync application: From 23305fb41b9d8ffe2b8a1b85a9168915257cd477 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 11 Apr 2020 21:33:19 +0000 Subject: [PATCH 28/42] refactor --- orchagent/routeorch.cpp | 29 +++++++++++++++-------------- orchagent/routeorch.h | 11 ++--------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 8389f8f095..2fa31f6d7d 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -12,7 +12,6 @@ extern sai_object_id_t gSwitchId; extern sai_next_hop_group_api_t* sai_next_hop_group_api; extern sai_route_api_t* sai_route_api; extern sai_switch_api_t* sai_switch_api; -extern sai_fdb_api_t* sai_fdb_api; extern PortsOrch *gPortsOrch; extern CrmOrch *gCrmOrch; @@ -374,11 +373,19 @@ void RouteOrch::doTask(Consumer& consumer) return; } - m_toBulk.clear(); - auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { + // Route bulk results will be stored in a map + std::map< + std::pair< + std::string, // Key + std::string // Op + >, + std::deque // Bulk statuses + > toBulk; + + // Add or remove routes with a route bulker while (it != consumer.m_toSync.end()) { KeyOpFieldsValuesTuple t = it->second; @@ -386,7 +393,7 @@ void RouteOrch::doTask(Consumer& consumer) string key = kfvKey(t); string op = kfvOp(t); - auto rc = m_toBulk.emplace(std::piecewise_construct, + auto rc = toBulk.emplace(std::piecewise_construct, std::forward_as_tuple(key, op), std::forward_as_tuple()); @@ -580,24 +587,19 @@ void RouteOrch::doTask(Consumer& consumer) } } + // Flush the route bulker, so routes will be written to syncd and ASIC gRouteBulker.flush(); + // Go through the bulker results auto it_prev = consumer.m_toSync.begin(); while (it_prev != it) { - if (m_resync) - { - it_prev++; - continue; - } - KeyOpFieldsValuesTuple t = it_prev->second; string key = kfvKey(t); string op = kfvOp(t); - - auto found = m_toBulk.find(make_pair(key, op)); - if (found == m_toBulk.end()) + auto found = toBulk.find(make_pair(key, op)); + if (found == toBulk.end()) { it_prev++; continue; @@ -704,7 +706,6 @@ void RouteOrch::doTask(Consumer& consumer) it_prev++; } } - m_toBulk.clear(); } } diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 2bb5f30f3a..d78ff8237a 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -78,6 +78,8 @@ class RouteOrch : public Orch, public Subject void notifyNextHopChangeObservers(sai_object_id_t, const IpPrefix&, const NextHopGroupKey&, bool); private: + using StatusInserter = std::deque&; + NeighOrch *m_neighOrch; IntfsOrch *m_intfsOrch; VRFOrch *m_vrfOrch; @@ -91,15 +93,6 @@ class RouteOrch : public Orch, public Subject NextHopObserverTable m_nextHopObservers; - using StatusInserter = std::deque&; - std::map< - std::pair< - std::string, // Key - std::string // Op - >, - std::deque // Bulk statuses - > m_toBulk; - EntityBulker gRouteBulker; ObjectBulker gNextHopGroupMemberBulkder; From f917b3078267ca22532f0f7e70c530a7f6c358c1 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sun, 12 Apr 2020 05:03:26 +0000 Subject: [PATCH 29/42] Add RouteBulkContext, and simplify doTask --- orchagent/nexthopgroupkey.h | 5 ++ orchagent/routeorch.cpp | 118 ++++++++++++++---------------------- orchagent/routeorch.h | 34 ++++++++--- 3 files changed, 78 insertions(+), 79 deletions(-) diff --git a/orchagent/nexthopgroupkey.h b/orchagent/nexthopgroupkey.h index d60aee8a32..aeeca8ba96 100644 --- a/orchagent/nexthopgroupkey.h +++ b/orchagent/nexthopgroupkey.h @@ -131,6 +131,11 @@ class NextHopGroupKey return nhs_str; } + void clear() + { + m_nexthops.clear(); + } + private: std::set m_nexthops; }; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 2fa31f6d7d..c037184c15 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -382,7 +382,7 @@ void RouteOrch::doTask(Consumer& consumer) std::string, // Key std::string // Op >, - std::deque // Bulk statuses + RouteBulkContext > toBulk; // Add or remove routes with a route bulker @@ -398,10 +398,10 @@ void RouteOrch::doTask(Consumer& consumer) std::forward_as_tuple()); bool inserted = rc.second; - auto& object_statuses = rc.first->second; + auto& ctx = rc.first->second; if (!inserted) { - object_statuses.clear(); + ctx.clear(); } /* Get notification from application */ @@ -453,8 +453,8 @@ void RouteOrch::doTask(Consumer& consumer) continue; } - sai_object_id_t vrf_id; - IpPrefix ip_prefix; + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ip_prefix = ctx.ip_prefix; if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) { @@ -506,7 +506,7 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoute(object_statuses, vrf_id, ip_prefix)) + if (removeRoute(ctx, vrf_id, ip_prefix)) it = consumer.m_toSync.erase(it); else it++; @@ -547,7 +547,7 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + if (addRoute(ctx, vrf_id, ip_prefix, nhg)) it = consumer.m_toSync.erase(it); else it++; @@ -557,7 +557,7 @@ void RouteOrch::doTask(Consumer& consumer) m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoute(object_statuses, vrf_id, ip_prefix, nhg)) + if (addRoute(ctx, vrf_id, ip_prefix, nhg)) it = consumer.m_toSync.erase(it); else it++; @@ -575,7 +575,7 @@ void RouteOrch::doTask(Consumer& consumer) } else if (op == DEL_COMMAND) { - if (removeRoute(object_statuses, vrf_id, ip_prefix)) + if (removeRoute(ctx, vrf_id, ip_prefix)) it = consumer.m_toSync.erase(it); else it++; @@ -605,34 +605,17 @@ void RouteOrch::doTask(Consumer& consumer) continue; } - auto& object_statuses = found->second; + auto& ctx = found->second; + auto& object_statuses = ctx.object_statuses; if (object_statuses.empty()) { it_prev++; continue; } - sai_object_id_t vrf_id; - IpPrefix ip_prefix; - - if (!key.compare(0, strlen(VRF_PREFIX), VRF_PREFIX)) - { - size_t found = key.find(':'); - string vrf_name = key.substr(0, found); - - if (!m_vrfOrch->isVRFexists(vrf_name)) - { - it_prev++; - continue; - } - vrf_id = m_vrfOrch->getVRFid(vrf_name); - ip_prefix = IpPrefix(key.substr(found+1)); - } - else - { - vrf_id = gVirtualRouterId; - ip_prefix = IpPrefix(key); - } + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ip_prefix = ctx.ip_prefix; + SWSS_LOG_NOTICE("vrf_id=%lu, ip_prefix=%s", vrf_id, ip_prefix.to_string().c_str()); if (op == SET_COMMAND) { @@ -665,7 +648,7 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + if (removeRoutePost(ctx, vrf_id, ip_prefix)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -682,7 +665,7 @@ void RouteOrch::doTask(Consumer& consumer) if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { - if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + if (addRoutePost(ctx, vrf_id, ip_prefix, nhg)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -691,7 +674,7 @@ void RouteOrch::doTask(Consumer& consumer) m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoutePost(object_statuses, vrf_id, ip_prefix, nhg)) + if (addRoutePost(ctx, vrf_id, ip_prefix, nhg)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -700,7 +683,7 @@ void RouteOrch::doTask(Consumer& consumer) else if (op == DEL_COMMAND) { /* Cannot locate the route or remove succeed */ - if (removeRoutePost(object_statuses, vrf_id, ip_prefix)) + if (removeRoutePost(ctx, vrf_id, ip_prefix)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -1038,7 +1021,7 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) return true; } -bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +void RouteOrch::addTempRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); @@ -1059,7 +1042,7 @@ bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf /* Return if next_hop_set is empty */ if (next_hop_set.empty()) - return false; + return; /* Randomly pick an address from the set */ auto it = next_hop_set.begin(); @@ -1067,20 +1050,12 @@ bool RouteOrch::addTempRoute(StatusInserter object_statuses, sai_object_id_t vrf /* Set the route's temporary next hop to be the randomly picked one */ NextHopGroupKey tmp_next_hop((*it).to_string()); - size_t sz = object_statuses.size(); - addRoute(object_statuses, vrf_id, ipPrefix, tmp_next_hop); - if (object_statuses.size() > sz) - { - // TRICK! TRICK! TRICK! - // Even we only successfully invoke bulker, not SAI, we write the temp route - // into m_syncdRoutes so addRoutePost will know what happened - m_syncdRoutes[vrf_id][ipPrefix] = tmp_next_hop; - return true; - } - return false; + ctx.tmp_next_hop = tmp_next_hop; + + addRoute(ctx, vrf_id, ipPrefix, tmp_next_hop); } -bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); @@ -1149,14 +1124,8 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, /* Add a temporary route when a next hop group cannot be added, * and there is no temporary route right now or the current temporary * route is not pointing to a member of the next hop group to sync. */ - bool rc = addTempRoute(object_statuses, vrf_id, ipPrefix, nextHops); - if (rc) - { - /* Push failure into object_statuses so addRoutePost will fail - * so the original route is not successfully added, and will retry */ - object_statuses.emplace_back(SAI_STATUS_INSUFFICIENT_RESOURCES); - } - // Only added to route bulker, not immediately finished + addTempRoute(ctx, vrf_id, ipPrefix, nextHops); + /* Return false since the original route is not successfully added */ return false; } } @@ -1171,6 +1140,7 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, copy(route_entry.destination, ipPrefix); sai_attribute_t route_attr; + auto& object_statuses = ctx.object_statuses; /* If the prefix is not in m_syncdRoutes, then we need to create the route * for this prefix with the new next hop (group) id. If the prefix is already @@ -1215,10 +1185,12 @@ bool RouteOrch::addRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, return false; } -bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); + auto& object_statuses = ctx.object_statuses; + if (object_statuses.empty()) { // Something went wrong before router bulker, will retry @@ -1263,9 +1235,8 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf if (!hasNextHopGroup(nextHops)) { // Previous added an temporary route - auto tmp_next_hop = m_syncdRoutes[vrf_id][ipPrefix]; - m_syncdRoutes[vrf_id].erase(ipPrefix); - addRoutePost(object_statuses, vrf_id, ipPrefix, tmp_next_hop); + auto& tmp_next_hop = ctx.tmp_next_hop; + addRoutePost(ctx, vrf_id, ipPrefix, tmp_next_hop); return false; } } @@ -1307,14 +1278,14 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf /* Set the packet action to forward when there was no next hop (dropped) */ if (it_route->second.getSize() == 0) { - status = *it_status++; - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", - ipPrefix.to_string().c_str(), status); - return false; - } - } + status = *it_status++; + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set route %s with packet action forward, %d", + ipPrefix.to_string().c_str(), status); + return false; + } + } status = *it_status++; if (status != SAI_STATUS_SUCCESS) @@ -1343,7 +1314,7 @@ bool RouteOrch::addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf return true; } -bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +bool RouteOrch::removeRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) { SWSS_LOG_ENTER(); @@ -1368,6 +1339,8 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ return true; } + auto& object_statuses = ctx.object_statuses; + // set to blackhole for default route if (ipPrefix.isDefaultRoute()) { @@ -1393,10 +1366,12 @@ bool RouteOrch::removeRoute(StatusInserter object_statuses, sai_object_id_t vrf_ return false; } -bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +bool RouteOrch::removeRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) { SWSS_LOG_ENTER(); + auto& object_statuses = ctx.object_statuses; + if (object_statuses.empty()) { // Something went wrong before router bulker, will retry @@ -1488,4 +1463,3 @@ bool RouteOrch::removeRoutePost(StatusInserter object_statuses, sai_object_id_t return true; } - diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index d78ff8237a..9b6738ddf5 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -55,6 +55,28 @@ struct NextHopObserverEntry list observers; }; +struct RouteBulkContext +{ + std::deque object_statuses; // Bulk statuses + NextHopGroupKey tmp_next_hop; // Temporary next hop + sai_object_id_t vrf_id; + IpPrefix ip_prefix; + + RouteBulkContext() + { + } + + // Disable any copy constructors + RouteBulkContext(const RouteBulkContext&) = delete; + RouteBulkContext(RouteBulkContext&&) = delete; + + void clear() + { + object_statuses.clear(); + tmp_next_hop.clear(); + } +}; + class RouteOrch : public Orch, public Subject { public: @@ -78,8 +100,6 @@ class RouteOrch : public Orch, public Subject void notifyNextHopChangeObservers(sai_object_id_t, const IpPrefix&, const NextHopGroupKey&, bool); private: - using StatusInserter = std::deque&; - NeighOrch *m_neighOrch; IntfsOrch *m_intfsOrch; VRFOrch *m_vrfOrch; @@ -96,11 +116,11 @@ class RouteOrch : public Orch, public Subject EntityBulker gRouteBulker; ObjectBulker gNextHopGroupMemberBulkder; - bool addTempRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool addRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool addRoutePost(StatusInserter object_statuses, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops); - bool removeRoute(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&); - bool removeRoutePost(StatusInserter object_statuses, sai_object_id_t, const IpPrefix&); + void addTempRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); + bool addRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); + bool addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops); + bool removeRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&); + bool removeRoutePost(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&); std::string getLinkLocalEui64Addr(void); void addLinkLocalRouteToMe(sai_object_id_t vrf_id, IpPrefix linklocal_prefix); From ef5841bc5965c7087b827d3a64a3c27d8c2ff65c Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 14 Apr 2020 23:28:00 +0000 Subject: [PATCH 30/42] Fix typo --- orchagent/routeorch.cpp | 10 +++++----- orchagent/routeorch.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index c037184c15..71612bf20b 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -24,7 +24,7 @@ const int routeorch_pri = 5; RouteOrch::RouteOrch(DBConnector *db, string tableName, NeighOrch *neighOrch, IntfsOrch *intfsOrch, VRFOrch *vrfOrch) : gRouteBulker(sai_route_api), - gNextHopGroupMemberBulkder(sai_next_hop_group_api, gSwitchId), + gNextHopGroupMemberBulker(sai_next_hop_group_api, gSwitchId), Orch(db, tableName, routeorch_pri), m_neighOrch(neighOrch), m_intfsOrch(intfsOrch), @@ -908,12 +908,12 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) nhgm_attr.value.oid = nhid; nhgm_attrs.push_back(nhgm_attr); - gNextHopGroupMemberBulkder.create_entry(&nhgm_ids[i], + gNextHopGroupMemberBulker.create_entry(&nhgm_ids[i], (uint32_t)nhgm_attrs.size(), nhgm_attrs.data()); } - gNextHopGroupMemberBulkder.flush(); + gNextHopGroupMemberBulker.flush(); for (size_t i = 0; i < npid_count; i++) { auto nhid = next_hop_ids[i]; @@ -986,9 +986,9 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) vector statuses(nhid_count); for (size_t i = 0; i < nhid_count; i++) { - gNextHopGroupMemberBulkder.remove_entry(&statuses[i], next_hop_ids[i]); + gNextHopGroupMemberBulker.remove_entry(&statuses[i], next_hop_ids[i]); } - gNextHopGroupMemberBulkder.flush(); + gNextHopGroupMemberBulker.flush(); for (size_t i = 0; i < nhid_count; i++) { if (statuses[i] != SAI_STATUS_SUCCESS) diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 9b6738ddf5..64fb8e0855 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -114,7 +114,7 @@ class RouteOrch : public Orch, public Subject NextHopObserverTable m_nextHopObservers; EntityBulker gRouteBulker; - ObjectBulker gNextHopGroupMemberBulkder; + ObjectBulker gNextHopGroupMemberBulker; void addTempRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); bool addRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); From 09d3aad98a2c96979719302a57cab389d1e550b5 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 15 Apr 2020 07:23:46 +0000 Subject: [PATCH 31/42] Remove dead code --- orchagent/routeorch.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 71612bf20b..7dc6209bc3 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1217,11 +1217,7 @@ bool RouteOrch::addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, cons } else { - if (m_neighOrch->hasNextHop(nexthop)) - { - next_hop_id = m_neighOrch->getNextHopId(nexthop); - } - else + if (!m_neighOrch->hasNextHop(nexthop)) { SWSS_LOG_INFO("Failed to get next hop %s for %s", nextHops.to_string().c_str(), ipPrefix.to_string().c_str()); From 9647451b80f8be01c08aee84ee199a61418c981d Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Wed, 15 Apr 2020 16:36:23 +0000 Subject: [PATCH 32/42] Fix warnings --- orchagent/bulker.h | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 34de531816..870ce32a5f 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -1,7 +1,6 @@ #pragma once #include -#include #include #include #include @@ -9,6 +8,7 @@ #include #include #include "sai.h" +#include "logger.h" static inline bool operator==(const sai_ip_prefix_t& a, const sai_ip_prefix_t& b) { @@ -265,7 +265,7 @@ class EntityBulker // Removing if (!removing_entries.empty()) { - vector rs; + std::vector rs; for (auto& i: removing_entries) { @@ -273,7 +273,7 @@ class EntityBulker rs.push_back(entry); } size_t count = rs.size(); - vector statuses(count); + std::vector statuses(count); (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_INFO("EntityBulker.flush removing_entries %zu\n", removing_entries.size()); @@ -292,9 +292,9 @@ class EntityBulker // Creating if (!creating_entries.empty()) { - vector rs; - vector tss; - vector cs; + std::vector rs; + std::vector tss; + std::vector cs; for (auto const& i: creating_entries) { @@ -306,7 +306,7 @@ class EntityBulker cs.push_back((uint32_t)attrs.size()); } size_t count = rs.size(); - vector statuses(count); + std::vector statuses(count); (*create_entries)((uint32_t)count, rs.data(), cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_INFO("EntityBulker.flush creating_entries %zu\n", creating_entries.size()); @@ -326,8 +326,8 @@ class EntityBulker // Setting if (!setting_entries.empty()) { - vector rs; - vector ts; + std::vector rs; + std::vector ts; for (auto const& i: setting_entries) { @@ -340,7 +340,7 @@ class EntityBulker } } size_t count = rs.size(); - vector statuses(count); + std::vector statuses(count); (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, statuses.data()); SWSS_LOG_INFO("EntityBulker.flush setting_entries %zu, count %zu\n", setting_entries.size(), count); @@ -511,7 +511,7 @@ class ObjectBulker if (!removing_entries.empty()) { size_t count = removing_entries.size(); - vector statuses(count); + std::vector statuses(count); sai_status_t status = (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); SWSS_LOG_INFO("ObjectBulker.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); @@ -530,8 +530,8 @@ class ObjectBulker // Creating if (!creating_entries.empty()) { - vector tss; - vector cs; + std::vector tss; + std::vector cs; for (auto const& i: creating_entries) { @@ -540,8 +540,8 @@ class ObjectBulker cs.push_back((uint32_t)attrs.size()); } size_t count = creating_entries.size(); - vector object_ids(count); - vector statuses(count); + std::vector object_ids(count); + std::vector statuses(count); (*create_entries)(switch_id, (uint32_t)count, cs.data(), tss.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, object_ids.data(), statuses.data()); SWSS_LOG_INFO("ObjectBulker.flush creating_entries %zu\n", creating_entries.size()); @@ -560,8 +560,8 @@ class ObjectBulker /* if (!setting_entries.empty()) { - vector rs; - vector ts; + std::vector rs; + std::vector ts; for (auto const& i: setting_entries) { @@ -574,7 +574,7 @@ class ObjectBulker } } size_t count = setting_entries.size(); - vector statuses(count); + std::vector statuses(count); (*set_entries_attribute)((uint32_t)count, rs.data(), ts.data() , SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); @@ -612,7 +612,7 @@ class ObjectBulker struct object_entry { sai_object_id_t *object_id; - vector attrs; + std::vector attrs; template object_entry(sai_object_id_t *object_id, InputIterator first, InputIterator last) : object_id(object_id) @@ -625,7 +625,7 @@ class ObjectBulker std::vector // - attrs + std::vector // - attrs >> creating_entries; std::unordered_map< // A map of From 98f31b9b5c33212fa36f1703dc80d11585ffc949 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 18 Apr 2020 02:01:29 +0000 Subject: [PATCH 33/42] Remove redundant parameters Signed-off-by: Qi Luo --- orchagent/routeorch.cpp | 46 +++++++++++++++++++++++++++-------------- orchagent/routeorch.h | 10 ++++----- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 7dc6209bc3..80db617160 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -506,7 +506,7 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoute(ctx, vrf_id, ip_prefix)) + if (removeRoute(ctx)) it = consumer.m_toSync.erase(it); else it++; @@ -547,7 +547,7 @@ void RouteOrch::doTask(Consumer& consumer) /* subnet route, vrf leaked route, etc */ else { - if (addRoute(ctx, vrf_id, ip_prefix, nhg)) + if (addRoute(ctx, nhg)) it = consumer.m_toSync.erase(it); else it++; @@ -557,7 +557,7 @@ void RouteOrch::doTask(Consumer& consumer) m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoute(ctx, vrf_id, ip_prefix, nhg)) + if (addRoute(ctx, nhg)) it = consumer.m_toSync.erase(it); else it++; @@ -575,7 +575,7 @@ void RouteOrch::doTask(Consumer& consumer) } else if (op == DEL_COMMAND) { - if (removeRoute(ctx, vrf_id, ip_prefix)) + if (removeRoute(ctx)) it = consumer.m_toSync.erase(it); else it++; @@ -648,7 +648,7 @@ void RouteOrch::doTask(Consumer& consumer) { /* If any existing routes are updated to point to the * above interfaces, remove them from the ASIC. */ - if (removeRoutePost(ctx, vrf_id, ip_prefix)) + if (removeRoutePost(ctx)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -665,7 +665,7 @@ void RouteOrch::doTask(Consumer& consumer) if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { - if (addRoutePost(ctx, vrf_id, ip_prefix, nhg)) + if (addRoutePost(ctx, nhg)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -674,7 +674,7 @@ void RouteOrch::doTask(Consumer& consumer) m_syncdRoutes.at(vrf_id).find(ip_prefix) == m_syncdRoutes.at(vrf_id).end() || m_syncdRoutes.at(vrf_id).at(ip_prefix) != nhg) { - if (addRoutePost(ctx, vrf_id, ip_prefix, nhg)) + if (addRoutePost(ctx, nhg)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -683,7 +683,7 @@ void RouteOrch::doTask(Consumer& consumer) else if (op == DEL_COMMAND) { /* Cannot locate the route or remove succeed */ - if (removeRoutePost(ctx, vrf_id, ip_prefix)) + if (removeRoutePost(ctx)) it_prev = consumer.m_toSync.erase(it_prev); else it_prev++; @@ -1021,10 +1021,12 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) return true; } -void RouteOrch::addTempRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +void RouteOrch::addTempRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); + IpPrefix& ipPrefix = ctx.ip_prefix; + auto next_hop_set = nextHops.getNextHops(); /* Remove next hops that are not in m_syncdNextHops */ @@ -1052,13 +1054,16 @@ void RouteOrch::addTempRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, cons NextHopGroupKey tmp_next_hop((*it).to_string()); ctx.tmp_next_hop = tmp_next_hop; - addRoute(ctx, vrf_id, ipPrefix, tmp_next_hop); + addRoute(ctx, tmp_next_hop); } -bool RouteOrch::addRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ipPrefix = ctx.ip_prefix; + /* next_hop_id indicates the next hop id or next hop group id of this route */ sai_object_id_t next_hop_id; @@ -1124,7 +1129,7 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const Ip /* Add a temporary route when a next hop group cannot be added, * and there is no temporary route right now or the current temporary * route is not pointing to a member of the next hop group to sync. */ - addTempRoute(ctx, vrf_id, ipPrefix, nextHops); + addTempRoute(ctx, nextHops); /* Return false since the original route is not successfully added */ return false; } @@ -1185,10 +1190,13 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const Ip return false; } -bool RouteOrch::addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoutePost(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ipPrefix = ctx.ip_prefix; + auto& object_statuses = ctx.object_statuses; if (object_statuses.empty()) @@ -1232,7 +1240,7 @@ bool RouteOrch::addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, cons { // Previous added an temporary route auto& tmp_next_hop = ctx.tmp_next_hop; - addRoutePost(ctx, vrf_id, ipPrefix, tmp_next_hop); + addRoutePost(ctx, tmp_next_hop); return false; } } @@ -1310,10 +1318,13 @@ bool RouteOrch::addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, cons return true; } -bool RouteOrch::removeRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +bool RouteOrch::removeRoute(RouteBulkContext& ctx) { SWSS_LOG_ENTER(); + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ipPrefix = ctx.ip_prefix; + auto it_route_table = m_syncdRoutes.find(vrf_id); if (it_route_table == m_syncdRoutes.end()) { @@ -1362,10 +1373,13 @@ bool RouteOrch::removeRoute(RouteBulkContext& ctx, sai_object_id_t vrf_id, const return false; } -bool RouteOrch::removeRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix) +bool RouteOrch::removeRoutePost(RouteBulkContext& ctx) { SWSS_LOG_ENTER(); + sai_object_id_t& vrf_id = ctx.vrf_id; + IpPrefix& ipPrefix = ctx.ip_prefix; + auto& object_statuses = ctx.object_statuses; if (object_statuses.empty()) diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 64fb8e0855..f8bfd96302 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -116,11 +116,11 @@ class RouteOrch : public Orch, public Subject EntityBulker gRouteBulker; ObjectBulker gNextHopGroupMemberBulker; - void addTempRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool addRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&, const NextHopGroupKey&); - bool addRoutePost(RouteBulkContext& ctx, sai_object_id_t vrf_id, const IpPrefix &ipPrefix, const NextHopGroupKey &nextHops); - bool removeRoute(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&); - bool removeRoutePost(RouteBulkContext& ctx, sai_object_id_t, const IpPrefix&); + void addTempRoute(RouteBulkContext& ctx, const NextHopGroupKey&); + bool addRoute(RouteBulkContext& ctx, const NextHopGroupKey&); + bool addRoutePost(RouteBulkContext& ctx, const NextHopGroupKey &nextHops); + bool removeRoute(RouteBulkContext& ctx); + bool removeRoutePost(RouteBulkContext& ctx); std::string getLinkLocalEui64Addr(void); void addLinkLocalRouteToMe(sai_object_id_t vrf_id, IpPrefix linklocal_prefix); From 2797e760597b9ea705046aa64f890e65aa16da53 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Sat, 18 Apr 2020 06:15:57 +0000 Subject: [PATCH 34/42] Move more context to RouteBulkContext, and fix const correctness --- orchagent/routeorch.cpp | 40 +++++++++++++++------------------------- orchagent/routeorch.h | 7 +++++-- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 80db617160..256172f3f0 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -479,7 +479,7 @@ void RouteOrch::doTask(Consumer& consumer) { string ips; string aliases; - bool excp_intfs_flag = false; + bool& excp_intfs_flag = ctx.excp_intfs_flag; for (auto i : kfvFieldsValues(t)) { @@ -605,23 +605,22 @@ void RouteOrch::doTask(Consumer& consumer) continue; } - auto& ctx = found->second; - auto& object_statuses = ctx.object_statuses; + const auto& ctx = found->second; + const auto& object_statuses = ctx.object_statuses; if (object_statuses.empty()) { it_prev++; continue; } - sai_object_id_t& vrf_id = ctx.vrf_id; - IpPrefix& ip_prefix = ctx.ip_prefix; - SWSS_LOG_NOTICE("vrf_id=%lu, ip_prefix=%s", vrf_id, ip_prefix.to_string().c_str()); + const sai_object_id_t& vrf_id = ctx.vrf_id; + const IpPrefix& ip_prefix = ctx.ip_prefix; if (op == SET_COMMAND) { string ips; string aliases; - bool excp_intfs_flag = false; + const bool& excp_intfs_flag = ctx.excp_intfs_flag; for (auto i : kfvFieldsValues(t)) { @@ -634,16 +633,6 @@ void RouteOrch::doTask(Consumer& consumer) vector ipv = tokenize(ips, ','); vector alsv = tokenize(aliases, ','); - for (auto alias : alsv) - { - if (alias == "eth0" || alias == "lo" || alias == "docker0") - { - excp_intfs_flag = true; - break; - } - } - - // TODO: cannot trust m_portsOrch->getPortIdByAlias because sometimes alias is empty if (excp_intfs_flag) { /* If any existing routes are updated to point to the @@ -853,7 +842,8 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops) } // skip next hop group member create for neighbor from down port - if (m_neighOrch->isNextHopFlagSet(it, NHFLAGS_IFDOWN)) { + if (m_neighOrch->isNextHopFlagSet(it, NHFLAGS_IFDOWN)) + { continue; } @@ -1190,14 +1180,14 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) return false; } -bool RouteOrch::addRoutePost(RouteBulkContext& ctx, const NextHopGroupKey &nextHops) +bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey &nextHops) { SWSS_LOG_ENTER(); - sai_object_id_t& vrf_id = ctx.vrf_id; - IpPrefix& ipPrefix = ctx.ip_prefix; + const sai_object_id_t& vrf_id = ctx.vrf_id; + const IpPrefix& ipPrefix = ctx.ip_prefix; - auto& object_statuses = ctx.object_statuses; + const auto& object_statuses = ctx.object_statuses; if (object_statuses.empty()) { @@ -1373,12 +1363,12 @@ bool RouteOrch::removeRoute(RouteBulkContext& ctx) return false; } -bool RouteOrch::removeRoutePost(RouteBulkContext& ctx) +bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx) { SWSS_LOG_ENTER(); - sai_object_id_t& vrf_id = ctx.vrf_id; - IpPrefix& ipPrefix = ctx.ip_prefix; + const sai_object_id_t& vrf_id = ctx.vrf_id; + const IpPrefix& ipPrefix = ctx.ip_prefix; auto& object_statuses = ctx.object_statuses; diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index f8bfd96302..e71a574237 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -61,8 +61,10 @@ struct RouteBulkContext NextHopGroupKey tmp_next_hop; // Temporary next hop sai_object_id_t vrf_id; IpPrefix ip_prefix; + bool excp_intfs_flag; RouteBulkContext() + : excp_intfs_flag(false) { } @@ -74,6 +76,7 @@ struct RouteBulkContext { object_statuses.clear(); tmp_next_hop.clear(); + excp_intfs_flag = false; } }; @@ -118,9 +121,9 @@ class RouteOrch : public Orch, public Subject void addTempRoute(RouteBulkContext& ctx, const NextHopGroupKey&); bool addRoute(RouteBulkContext& ctx, const NextHopGroupKey&); - bool addRoutePost(RouteBulkContext& ctx, const NextHopGroupKey &nextHops); + bool addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey &nextHops); bool removeRoute(RouteBulkContext& ctx); - bool removeRoutePost(RouteBulkContext& ctx); + bool removeRoutePost(const RouteBulkContext& ctx); std::string getLinkLocalEui64Addr(void); void addLinkLocalRouteToMe(sai_object_id_t vrf_id, IpPrefix linklocal_prefix); From 84b6d6752dae902d98e1ea6927d146553e6c88ee Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 23 Apr 2020 19:56:37 +0000 Subject: [PATCH 35/42] Merge master --- cfgmgr/buffermgr.cpp | 15 +++++++++++++ orchagent/chassisorch.cpp | 1 - orchagent/main.cpp | 20 +++++++++++++++--- orchagent/port.h | 1 + orchagent/portsorch.cpp | 4 ++-- orchagent/routeorch.cpp | 30 ++++++++++++++++++++++++++ orchagent/vnetorch.cpp | 16 ++++++++++++++ orchagent/vnetorch.h | 1 + tests/test_crm.py | 20 +++++++++--------- tests/test_fdb.py | 1 - tests/test_nat.py | 4 ++-- tests/test_nhg.py | 6 +++--- tests/test_port.py | 12 +++++++---- tests/test_warm_reboot.py | 44 +++++++++++++++++++-------------------- 14 files changed, 127 insertions(+), 48 deletions(-) diff --git a/cfgmgr/buffermgr.cpp b/cfgmgr/buffermgr.cpp index ad738f2b50..fbce96e40e 100644 --- a/cfgmgr/buffermgr.cpp +++ b/cfgmgr/buffermgr.cpp @@ -182,6 +182,21 @@ task_process_status BufferMgr::doSpeedUpdateTask(string port, string speed) buffer_profile_key + "]"; + /* Check if PG Mapping is already then log message and return. */ + + m_cfgBufferPgTable.get(buffer_pg_key, fvVector); + + for (auto& prop : fvVector) + { + if ((fvField(prop) == "profile") && (profile_ref == fvValue(prop))) + { + SWSS_LOG_NOTICE("PG to Buffer Profile Mapping %s already present", buffer_pg_key.c_str()); + return task_process_status::task_success; + } + } + + fvVector.clear(); + fvVector.push_back(make_pair("profile", profile_ref)); m_cfgBufferPgTable.set(buffer_pg_key, fvVector); return task_process_status::task_success; diff --git a/orchagent/chassisorch.cpp b/orchagent/chassisorch.cpp index 8a089a8cfb..519aef84cf 100644 --- a/orchagent/chassisorch.cpp +++ b/orchagent/chassisorch.cpp @@ -51,7 +51,6 @@ void ChassisOrch::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); - const std::string & tableName = consumer.getTableName(); auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { diff --git a/orchagent/main.cpp b/orchagent/main.cpp index 2f32743997..ce87a3802c 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -13,6 +13,8 @@ extern "C" { #include #include #include +#include +#include #include #include "timestamp.h" @@ -49,6 +51,7 @@ bool gSwssRecord = true; bool gLogRotate = false; bool gSaiRedisLogRotate = false; bool gSyncMode = false; +char *gAsicInstance = NULL; extern bool gIsNatSupported; @@ -57,7 +60,7 @@ string gRecordFile; void usage() { - cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-b batch_size] [-m MAC] [-s]" << endl; + cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-b batch_size] [-m MAC] [-i INST_ID] [-s]" << endl; cout << " -h: display this message" << endl; cout << " -r record_type: record orchagent logs with type (default 3)" << endl; cout << " 0: do not record logs" << endl; @@ -67,6 +70,7 @@ void usage() cout << " -d record_location: set record logs folder location (default .)" << endl; cout << " -b batch_size: set consumer table pop operation batch size (default 128)" << endl; cout << " -m MAC: set switch MAC address" << endl; + cout << " -i INST_ID: set the ASIC instance_id in multi-asic platform" << endl; cout << " -s: enable synchronous mode" << endl; } @@ -116,13 +120,17 @@ int main(int argc, char **argv) string record_location = "."; - while ((opt = getopt(argc, argv, "b:m:r:d:hs")) != -1) + while ((opt = getopt(argc, argv, "b:m:r:d:i:hs")) != -1) { switch (opt) { case 'b': gBatchSize = atoi(optarg); break; + case 'i': + gAsicInstance = (char *)calloc(strlen(optarg)+1, sizeof(char)); + memcpy(gAsicInstance, optarg, strlen(optarg)); + break; case 'm': gMacAddress = MacAddress(optarg); break; @@ -182,7 +190,6 @@ int main(int argc, char **argv) attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; attr.value.booldata = true; attrs.push_back(attr); - attr.id = SAI_SWITCH_ATTR_FDB_EVENT_NOTIFY; attr.value.ptr = (void *)on_fdb_event; attrs.push_back(attr); @@ -226,6 +233,13 @@ int main(int argc, char **argv) sai_switch_api->set_switch_attribute(gSwitchId, &attr); } + if (gAsicInstance) + { + attr.id = SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO; + attr.value.s8list.count = (uint32_t)(strlen(gAsicInstance)+1); + attr.value.s8list.list = (int8_t*)gAsicInstance; + attrs.push_back(attr); + } status = sai_switch_api->create_switch(&gSwitchId, (uint32_t)attrs.size(), attrs.data()); if (status != SAI_STATUS_SUCCESS) diff --git a/orchagent/port.h b/orchagent/port.h index 963ee5688b..74f805ddec 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -10,6 +10,7 @@ extern "C" { #include #include #include +#include #define DEFAULT_PORT_VLAN_ID 1 /* diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 6ed03dc00e..4596336347 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -3572,7 +3572,7 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port) bool PortsOrch::setCollectionOnLagMember(Port &lagMember, bool enableCollection) { /* Port must be LAG member */ - assert(port.m_lag_member_id); + assert(lagMember.m_lag_member_id); sai_status_t status = SAI_STATUS_FAILURE; sai_attribute_t attr {}; @@ -3595,7 +3595,7 @@ bool PortsOrch::setCollectionOnLagMember(Port &lagMember, bool enableCollection) bool PortsOrch::setDistributionOnLagMember(Port &lagMember, bool enableDistribution) { /* Port must be LAG member */ - assert(port.m_lag_member_id); + assert(lagMember.m_lag_member_id); sai_status_t status = SAI_STATUS_FAILURE; sai_attribute_t attr {}; diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 256172f3f0..c28b65d71a 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -492,6 +492,36 @@ void RouteOrch::doTask(Consumer& consumer) vector ipv = tokenize(ips, ','); vector alsv = tokenize(aliases, ','); + /* + * For backward compatibility, adjust ip string from old format to + * new format. Meanwhile it can deal with some abnormal cases. + */ + + /* Resize the ip vector to match ifname vector + * as tokenize(",", ',') will miss the last empty segment. */ + if (alsv.size() == 0) + { + SWSS_LOG_WARN("Skip the route %s, for it has an empty ifname field.", key.c_str()); + it = consumer.m_toSync.erase(it); + continue; + } + else if (alsv.size() != ipv.size()) + { + SWSS_LOG_NOTICE("Route %s: resize ipv to match alsv, %zd -> %zd.", key.c_str(), ipv.size(), alsv.size()); + ipv.resize(alsv.size()); + } + + /* Set the empty ip(s) to zero + * as IpAddress("") will construst a incorrect ip. */ + for (auto &ip : ipv) + { + if (ip.empty()) + { + SWSS_LOG_NOTICE("Route %s: set the empty nexthop ip to zero.", key.c_str()); + ip = ip_prefix.isV4() ? "0.0.0.0" : "::"; + } + } + for (auto alias : alsv) { if (alias == "eth0" || alias == "lo" || alias == "docker0") diff --git a/orchagent/vnetorch.cpp b/orchagent/vnetorch.cpp index 9155d31693..e2167fbeab 100644 --- a/orchagent/vnetorch.cpp +++ b/orchagent/vnetorch.cpp @@ -429,6 +429,10 @@ VnetBridgeInfo VNetBitmapObject::getBridgeInfoByVni(uint32_t vni, string tunnelN attr.value.oid = info.bridge_id; rif_attrs.push_back(attr); + attr.id = SAI_ROUTER_INTERFACE_ATTR_MTU; + attr.value.u32 = VNET_BITMAP_RIF_MTU; + rif_attrs.push_back(attr); + status = sai_router_intfs_api->create_router_interface( &info.rif_id, gSwitchId, @@ -791,6 +795,12 @@ bool VNetBitmapObject::addTunnelRoute(IpPrefix& ipPrefix, tunnelEndpoint& endp) sai_ip_address_t underlayAddr; copy(underlayAddr, endp.ip); + if (tunnelRouteMap_.find(ipPrefix) != tunnelRouteMap_.end()) + { + SWSS_LOG_WARN("VNET tunnel route %s exists", ipPrefix.to_string().c_str()); + return true; + } + VNetOrch* vnet_orch = gDirectory.get(); for (auto peer : peer_list) { @@ -1093,6 +1103,12 @@ bool VNetBitmapObject::addRoute(IpPrefix& ipPrefix, nextHop& nh) Port port; RouteInfo routeInfo; + if (routeMap_.find(ipPrefix) != routeMap_.end()) + { + SWSS_LOG_WARN("VNET route %s exists", ipPrefix.to_string().c_str()); + return true; + } + bool is_subnet = (!nh.ips.getSize() || nh.ips.contains("0.0.0.0")) ? true : false; if (is_subnet && (!gPortsOrch->getPort(nh.ifname, port) || (port.m_rif_id == SAI_NULL_OBJECT_ID))) diff --git a/orchagent/vnetorch.h b/orchagent/vnetorch.h index 2cd3a4d2ea..1827e8af76 100644 --- a/orchagent/vnetorch.h +++ b/orchagent/vnetorch.h @@ -17,6 +17,7 @@ #define VNET_TUNNEL_SIZE 40960 #define VNET_NEIGHBOR_MAX 0xffff #define VXLAN_ENCAP_TTL 128 +#define VNET_BITMAP_RIF_MTU 9100 extern sai_object_id_t gVirtualRouterId; diff --git a/tests/test_crm.py b/tests/test_crm.py index f99c3cf69f..9d52550af1 100644 --- a/tests/test_crm.py +++ b/tests/test_crm.py @@ -138,7 +138,7 @@ def test_CrmIpv4Route(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.runcmd("crm config polling interval 1") @@ -205,7 +205,7 @@ def test_CrmIpv6Route(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet0|fc00::1/126", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.servers[0].runcmd("ifconfig eth0 inet6 add fc00::2/126") dvs.servers[0].runcmd("ip -6 route add default via fc00::1") @@ -271,7 +271,7 @@ def test_CrmIpv4Nexthop(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) intf_tbl.set("Ethernet0", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.runcmd("crm config polling interval 1") @@ -330,7 +330,7 @@ def test_CrmIpv6Nexthop(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet0|fc00::1/126", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.runcmd("crm config polling interval 1") @@ -385,7 +385,7 @@ def test_CrmIpv4Neighbor(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.runcmd("crm config polling interval 1") @@ -444,7 +444,7 @@ def test_CrmIpv6Neighbor(self, dvs, testlog): fvs = swsscommon.FieldValuePairs([("NULL","NULL")]) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet0|fc00::1/126", fvs) - dvs.runcmd("ifconfig Ethernet0 up") + dvs.runcmd("config interface startup Ethernet0") dvs.runcmd("crm config polling interval 1") @@ -501,8 +501,8 @@ def test_CrmNexthopGroup(self, dvs, testlog): intf_tbl.set("Ethernet4", fvs) intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) intf_tbl.set("Ethernet4|10.0.0.2/31", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") dvs.runcmd("crm config polling interval 1") @@ -576,8 +576,8 @@ def test_CrmNexthopGroupMember(self, dvs, testlog): intf_tbl.set("Ethernet4", fvs) intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) intf_tbl.set("Ethernet4|10.0.0.2/31", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") dvs.runcmd("crm config polling interval 1") diff --git a/tests/test_fdb.py b/tests/test_fdb.py index f883acb1bd..15a386f4d7 100644 --- a/tests/test_fdb.py +++ b/tests/test_fdb.py @@ -382,4 +382,3 @@ def test_FdbAddedAfterMemberCreated(self, dvs, testlog): dvs.runcmd("sonic-clear fdb all") dvs.remove_vlan_member("2", "Ethernet0") dvs.remove_vlan("2") - diff --git a/tests/test_nat.py b/tests/test_nat.py index f93025025e..0f60a2aa00 100644 --- a/tests/test_nat.py +++ b/tests/test_nat.py @@ -12,8 +12,8 @@ def set_interfaces(self, dvs): self.config_db.create_entry("INTERFACE", "Ethernet4|18.18.18.1/24", fvs) self.config_db.create_entry("INTERFACE", "Ethernet0", fvs) self.config_db.create_entry("INTERFACE", "Ethernet4", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") dvs.servers[0].runcmd("ip link set down dev eth0") dvs.servers[0].runcmd("ip link set up dev eth0") diff --git a/tests/test_nhg.py b/tests/test_nhg.py index 56f3bdb0b6..ac26b5107f 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -18,9 +18,9 @@ def test_route_nhg(self, dvs, testlog): intf_tbl.set("Ethernet0|10.0.0.0/31", fvs) intf_tbl.set("Ethernet4|10.0.0.2/31", fvs) intf_tbl.set("Ethernet8|10.0.0.4/31", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") - dvs.runcmd("ifconfig Ethernet8 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") + dvs.runcmd("config interface startup Ethernet8") dvs.runcmd("arp -s 10.0.0.1 00:00:00:00:00:01") dvs.runcmd("arp -s 10.0.0.3 00:00:00:00:00:02") diff --git a/tests/test_port.py b/tests/test_port.py index d57031077f..c2878d4df5 100644 --- a/tests/test_port.py +++ b/tests/test_port.py @@ -26,9 +26,11 @@ def test_PortMtu(self, dvs, testlog): assert fv[1] == "9100" def test_PortNotification(self, dvs, testlog): + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface ip add Ethernet0 10.0.0.0/31") - dvs.runcmd("ifconfig Ethernet0 10.0.0.0/31 up") == 0 - dvs.runcmd("ifconfig Ethernet4 10.0.0.2/31 up") == 0 + dvs.runcmd("config interface startup Ethernet4") + dvs.runcmd("config interface ip add Ethernet4 10.0.0.2/31") dvs.servers[0].runcmd("ip link set down dev eth0") == 0 @@ -69,9 +71,11 @@ def test_PortNotification(self, dvs, testlog): assert oper_status == "up" def test_PortFec(self, dvs, testlog): + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface ip add Ethernet0 10.0.0.0/31") - dvs.runcmd("ifconfig Ethernet0 10.0.0.0/31 up") == 0 - dvs.runcmd("ifconfig Ethernet4 10.0.0.2/31 up") == 0 + dvs.runcmd("config interface startup Ethernet4") + dvs.runcmd("config interface ip add Ethernet4 10.0.0.2/31") dvs.servers[0].runcmd("ip link set down dev eth0") == 0 diff --git a/tests/test_warm_reboot.py b/tests/test_warm_reboot.py index 6427453a1f..bfce08fda3 100644 --- a/tests/test_warm_reboot.py +++ b/tests/test_warm_reboot.py @@ -244,8 +244,8 @@ def test_PortSyncdWarmRestart(self, dvs, testlog): dvs.runcmd("config warm_restart enable swss") - dvs.runcmd("ifconfig Ethernet16 up") - dvs.runcmd("ifconfig Ethernet20 up") + dvs.runcmd("config interface startup Ethernet16") + dvs.runcmd("config interface startup Ethernet20") time.sleep(1) @@ -256,8 +256,8 @@ def test_PortSyncdWarmRestart(self, dvs, testlog): intf_tbl.set("Ethernet20|11.0.0.9/29", fvs) intf_tbl.set("Ethernet16", fvs) intf_tbl.set("Ethernet20", fvs) - dvs.runcmd("ifconfig Ethernet16 up") - dvs.runcmd("ifconfig Ethernet20 up") + dvs.runcmd("config interface startup Ethernet16") + dvs.runcmd("config interface startup Ethernet20") dvs.servers[4].runcmd("ip link set down dev eth0") == 0 dvs.servers[4].runcmd("ip link set up dev eth0") == 0 @@ -315,7 +315,7 @@ def test_PortSyncdWarmRestart(self, dvs, testlog): check_port_oper_status(appl_db, "Ethernet16", "up") check_port_oper_status(appl_db, "Ethernet20", "up") - check_port_oper_status(appl_db, "Ethernet24", "up") + check_port_oper_status(appl_db, "Ethernet24", "down") swss_app_check_RestoreCount_single(state_db, restore_count, "portsyncd") @@ -336,8 +336,8 @@ def test_VlanMgrdWarmRestart(self, dvs, testlog): dvs.runcmd("ifconfig Ethernet16 0") dvs.runcmd("ifconfig Ethernet20 0") - dvs.runcmd("ifconfig Ethernet16 up") - dvs.runcmd("ifconfig Ethernet20 up") + dvs.runcmd("config interface startup Ethernet16 ") + dvs.runcmd("config interface startup Ethernet20 ") time.sleep(1) @@ -384,8 +384,8 @@ def test_VlanMgrdWarmRestart(self, dvs, testlog): intf_tbl.set("Vlan20|11.0.0.9/29", fvs) intf_tbl.set("Vlan16", fvs) intf_tbl.set("Vlan20", fvs) - dvs.runcmd("ifconfig Vlan16 up") - dvs.runcmd("ifconfig Vlan20 up") + dvs.runcmd("config interface startup Vlan16") + dvs.runcmd("config interface startup Vlan20") dvs.servers[4].runcmd("ifconfig eth0 11.0.0.2/29") dvs.servers[4].runcmd("ip route add default via 11.0.0.1") @@ -478,8 +478,8 @@ def test_swss_neighbor_syncup(self, dvs, testlog): intf_tbl.set("{}".format(intfs[1]), fvs) intf_tbl.set("{}".format(intfs[0]), fvs) intf_tbl.set("{}".format(intfs[1]), fvs) - dvs.runcmd("ifconfig {} up".format(intfs[0])) - dvs.runcmd("ifconfig {} up".format(intfs[1])) + dvs.runcmd("config interface startup {}".format(intfs[0])) + dvs.runcmd("config interface startup {}".format(intfs[1])) ips = ["24.0.0.2", "24.0.0.3", "28.0.0.2", "28.0.0.3"] v6ips = ["2400::2", "2400::3", "2800::2", "2800::3"] @@ -831,8 +831,8 @@ def test_OrchagentWarmRestartReadyCheck(self, dvs, testlog): intf_tbl.set("Ethernet4|10.0.0.2/31", fvs) intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet4", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") dvs.servers[0].runcmd("ifconfig eth0 10.0.0.1/31") dvs.servers[0].runcmd("ip route add default via 10.0.0.0") @@ -904,9 +904,9 @@ def test_swss_port_state_syncup(self, dvs, testlog): intf_tbl.set("Ethernet0", fvs) intf_tbl.set("Ethernet4", fvs) intf_tbl.set("Ethernet8", fvs) - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") - dvs.runcmd("ifconfig Ethernet8 up") + dvs.runcmd("config interface startup Ethernet0") + dvs.runcmd("config interface startup Ethernet4") + dvs.runcmd("config interface startup Ethernet8") dvs.runcmd("arp -s 10.0.0.1 00:00:00:00:00:01") dvs.runcmd("arp -s 10.0.0.3 00:00:00:00:00:02") @@ -1075,9 +1075,9 @@ def test_routing_WarmRestart(self, dvs, testlog): intf_tbl.set("{}".format(intfs[1]), fvs) intf_tbl.set("{}".format(intfs[2]), fvs) intf_tbl.set("{}".format(intfs[2]), fvs) - dvs.runcmd("ip link set {} up".format(intfs[0])) - dvs.runcmd("ip link set {} up".format(intfs[1])) - dvs.runcmd("ip link set {} up".format(intfs[2])) + dvs.runcmd("config interface startup {}".format(intfs[0])) + dvs.runcmd("config interface startup {}".format(intfs[1])) + dvs.runcmd("config interface startup {}".format(intfs[2])) time.sleep(1) @@ -1835,7 +1835,7 @@ def test_system_warmreboot_neighbor_syncup(self, dvs, testlog): intf_tbl.set("Ethernet{}|{}00::1/64".format(i*4, i*4), fvs) intf_tbl.set("Ethernet{}".format(i*4, i*4), fvs) intf_tbl.set("Ethernet{}".format(i*4, i*4), fvs) - dvs.runcmd("ip link set Ethernet{} up".format(i*4, i*4)) + dvs.runcmd("config interface startup Ethernet{}".format(i*4, i*4)) dvs.servers[i].runcmd("ip link set up dev eth0") dvs.servers[i].runcmd("ip addr flush dev eth0") #result = dvs.servers[i].runcmd_output("ifconfig eth0 | grep HWaddr | awk '{print $NF}'") @@ -2082,8 +2082,8 @@ def test_VrfMgrdWarmRestart(self, dvs, testlog): dvs.runcmd("config warm_restart enable swss") # bring up interface - dvs.runcmd("ifconfig Ethernet0 up") - dvs.runcmd("ifconfig Ethernet4 up") + dvs.runcmd("config interface startup Ethernet0 ") + dvs.runcmd("config interface startup Ethernet4 ") # create vrf create_entry_tbl(conf_db, "VRF", "Vrf_1", [('empty', 'empty')]) From 077004e74e2c6050269d2b14efe7f1ca4b98ce14 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 23 Apr 2020 20:31:38 +0000 Subject: [PATCH 36/42] Add more into RouteBulkContext --- orchagent/routeorch.cpp | 28 ++++++---------------------- orchagent/routeorch.h | 5 +++++ 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index c28b65d71a..bc9174be93 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -489,7 +489,8 @@ void RouteOrch::doTask(Consumer& consumer) if (fvField(i) == "ifname") aliases = fvValue(i); } - vector ipv = tokenize(ips, ','); + vector& ipv = ctx.ipv; + ipv = tokenize(ips, ','); vector alsv = tokenize(aliases, ','); /* @@ -549,7 +550,8 @@ void RouteOrch::doTask(Consumer& consumer) nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; } - NextHopGroupKey nhg(nhg_str); + NextHopGroupKey& nhg = ctx.nhg; + nhg = NextHopGroupKey(nhg_str); if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { @@ -648,20 +650,8 @@ void RouteOrch::doTask(Consumer& consumer) if (op == SET_COMMAND) { - string ips; - string aliases; const bool& excp_intfs_flag = ctx.excp_intfs_flag; - - for (auto i : kfvFieldsValues(t)) - { - if (fvField(i) == "nexthop") - ips = fvValue(i); - - if (fvField(i) == "ifname") - aliases = fvValue(i); - } - vector ipv = tokenize(ips, ','); - vector alsv = tokenize(aliases, ','); + const vector& ipv = ctx.ipv; if (excp_intfs_flag) { @@ -674,13 +664,7 @@ void RouteOrch::doTask(Consumer& consumer) continue; } - string nhg_str = ipv[0] + NH_DELIMITER + alsv[0]; - for (uint32_t i = 1; i < ipv.size(); i++) - { - nhg_str += NHG_DELIMITER + ipv[i] + NH_DELIMITER + alsv[i]; - } - - NextHopGroupKey nhg(nhg_str); + const NextHopGroupKey& nhg = ctx.nhg; if (ipv.size() == 1 && IpAddress(ipv[0]).isZero()) { diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index e71a574237..8cd71aee27 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -59,9 +59,11 @@ struct RouteBulkContext { std::deque object_statuses; // Bulk statuses NextHopGroupKey tmp_next_hop; // Temporary next hop + NextHopGroupKey nhg; sai_object_id_t vrf_id; IpPrefix ip_prefix; bool excp_intfs_flag; + std::vector ipv; RouteBulkContext() : excp_intfs_flag(false) @@ -76,7 +78,10 @@ struct RouteBulkContext { object_statuses.clear(); tmp_next_hop.clear(); + nhg.clear(); + ipv.clear(); excp_intfs_flag = false; + vrf_id = SAI_NULL_OBJECT_ID; } }; From a21bcbe3ec80866556d1d9e0ba4571104adc70fc Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Fri, 24 Apr 2020 20:12:14 +0000 Subject: [PATCH 37/42] Refactor SaiBulkerTraits --- orchagent/bulker.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 870ce32a5f..1098c105f7 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -108,10 +108,10 @@ typedef sai_status_t (*sai_bulk_set_fdb_entry_attribute_fn)( _Out_ sai_status_t *object_statuses); template -struct saitraits { }; +struct SaiBulkerTraits { }; template<> -struct saitraits +struct SaiBulkerTraits { using entry_t = sai_route_entry_t; using api_t = sai_route_api_t; @@ -124,7 +124,7 @@ struct saitraits }; template<> -struct saitraits +struct SaiBulkerTraits { using entry_t = sai_fdb_entry_t; using api_t = sai_fdb_api_t; @@ -137,7 +137,7 @@ struct saitraits }; template<> -struct saitraits +struct SaiBulkerTraits { using entry_t = sai_object_id_t; using api_t = sai_next_hop_group_api_t; @@ -154,7 +154,7 @@ template class EntityBulker { public: - using Ts = saitraits; + using Ts = SaiBulkerTraits; using Te = typename Ts::entry_t; EntityBulker(typename Ts::api_t *api) @@ -441,7 +441,7 @@ template class ObjectBulker { public: - using Ts = saitraits; + using Ts = SaiBulkerTraits; ObjectBulker(typename Ts::api_t* next_hop_group_api, sai_object_id_t switch_id) { @@ -646,7 +646,7 @@ class ObjectBulker }; template <> -inline ObjectBulker::ObjectBulker(saitraits::api_t *api, sai_object_id_t switch_id) +inline ObjectBulker::ObjectBulker(SaiBulkerTraits::api_t *api, sai_object_id_t switch_id) : switch_id(switch_id) { create_entries = api->create_next_hop_group_members; From 6a10c22d21d8932a9d0977b4f653aaeb551ba736 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 28 Apr 2020 23:16:36 +0000 Subject: [PATCH 38/42] Refine bulk flush(): skip entries which are already executed --- orchagent/bulker.h | 68 ++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 1098c105f7..4969c5eb3e 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -269,8 +269,12 @@ class EntityBulker for (auto& i: removing_entries) { - auto& entry = i.first; - rs.push_back(entry); + auto const& entry = i.first; + sai_status_t *object_status = i.second; + if (*object_status == SAI_STATUS_NOT_EXECUTED) + { + rs.push_back(entry); + } } size_t count = rs.size(); std::vector statuses(count); @@ -300,10 +304,13 @@ class EntityBulker { auto const& entry = i.first; auto const& attrs = i.second.first; - - rs.push_back(entry); - tss.push_back(attrs.data()); - cs.push_back((uint32_t)attrs.size()); + sai_status_t *object_status = i.second.second; + if (*object_status == SAI_STATUS_NOT_EXECUTED) + { + rs.push_back(entry); + tss.push_back(attrs.data()); + cs.push_back((uint32_t)attrs.size()); + } } size_t count = rs.size(); std::vector statuses(count); @@ -335,8 +342,13 @@ class EntityBulker auto const& attrmap = i.second; for (auto const& ia: attrmap) { - rs.push_back(entry); - ts.push_back(ia.second.first); + auto const& attr = ia.second.first; + sai_status_t *object_status = ia.second.second; + if (*object_status == SAI_STATUS_NOT_EXECUTED) + { + rs.push_back(entry); + ts.push_back(attr); + } } } size_t count = rs.size(); @@ -475,8 +487,7 @@ class ObjectBulker setting_entries.erase(found_setting); } - removing_entries.emplace_back(object_id); - removing_statuses.emplace_back(object_status); + removing_entries.emplace(object_id, object_status); *object_status = SAI_STATUS_NOT_EXECUTED; return *object_status; } @@ -510,21 +521,28 @@ class ObjectBulker // Removing if (!removing_entries.empty()) { - size_t count = removing_entries.size(); + std::vector rs; + for (auto const& i: removing_entries) + { + auto const& entry = i.first; + sai_status_t *object_status = i.second; + if (*object_status == SAI_STATUS_NOT_EXECUTED) + { + rs.push_back(entry); + } + } + size_t count = rs.size(); std::vector statuses(count); - sai_status_t status = (*remove_entries)((uint32_t)count, removing_entries.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); + sai_status_t status = (*remove_entries)((uint32_t)count, rs.data(), SAI_BULK_OP_ERROR_MODE_STOP_ON_ERROR, statuses.data()); SWSS_LOG_INFO("ObjectBulker.flush removing_entries %zu rc=%d statuses[0]=%d\n", removing_entries.size(), status, statuses[0]); for (size_t i = 0; i < count; i++) { - sai_status_t *object_status = removing_statuses[i]; - if (object_status) - { - *object_status = statuses[i]; - } + auto const& entry = rs[i]; + sai_status_t object_status = statuses[i]; + *removing_entries[entry] = object_status; } removing_entries.clear(); - removing_statuses.clear(); } // Creating @@ -535,9 +553,13 @@ class ObjectBulker for (auto const& i: creating_entries) { + sai_object_id_t *pid = std::get<0>(i); auto const& attrs = std::get<1>(i); - tss.push_back(attrs.data()); - cs.push_back((uint32_t)attrs.size()); + if (*pid == SAI_NULL_OBJECT_ID) + { + tss.push_back(attrs.data()); + cs.push_back((uint32_t)attrs.size()); + } } size_t count = creating_entries.size(); std::vector object_ids(count); @@ -588,7 +610,6 @@ class ObjectBulker void clear() { removing_entries.clear(); - removing_statuses.clear(); creating_entries.clear(); setting_entries.clear(); } @@ -636,8 +657,9 @@ class ObjectBulker > > setting_entries; - std::vector removing_entries; - std::vector removing_statuses; + // A map of + // object_id -> object_status + std::unordered_map removing_entries; typename Ts::bulk_create_entry_fn create_entries; typename Ts::bulk_remove_entry_fn remove_entries; From ffdf5d985d389ffe19830af681eca1d0ec908247 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 28 Apr 2020 23:42:40 +0000 Subject: [PATCH 39/42] Input validation --- orchagent/bulker.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/orchagent/bulker.h b/orchagent/bulker.h index 4969c5eb3e..e3e464c62d 100644 --- a/orchagent/bulker.h +++ b/orchagent/bulker.h @@ -168,6 +168,13 @@ class EntityBulker _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { + assert(object_status); + if (!object_status) throw std::invalid_argument("object_status is null"); + assert(entry); + if (!entry) throw std::invalid_argument("entry is null"); + assert(attr_list); + if (!attr_list) throw std::invalid_argument("attr_list is null"); + auto rc = creating_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), std::forward_as_tuple()); @@ -192,6 +199,8 @@ class EntityBulker _Out_ sai_status_t *object_status, _In_ const Te *entry) { + assert(object_status); + if (!object_status) throw std::invalid_argument("object_status is null"); assert(entry); if (!entry) throw std::invalid_argument("entry is null"); @@ -235,6 +244,13 @@ class EntityBulker _In_ const Te *entry, _In_ const sai_attribute_t *attr) { + assert(object_status); + if (!object_status) throw std::invalid_argument("object_status is null"); + assert(entry); + if (!entry) throw std::invalid_argument("entry is null"); + assert(attr); + if (!attr) throw std::invalid_argument("attr is null"); + // Insert or find the key (entry) auto& attrmap = setting_entries.emplace(std::piecewise_construct, std::forward_as_tuple(*entry), @@ -465,6 +481,11 @@ class ObjectBulker _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list) { + assert(object_id); + if (!object_id) throw std::invalid_argument("object_id is null"); + assert(attr_list); + if (!attr_list) throw std::invalid_argument("attr_list is null"); + creating_entries.emplace_back(std::piecewise_construct, std::forward_as_tuple(object_id), std::forward_as_tuple(attr_list, attr_list + attr_count)); auto& last_attrs = std::get<1>(creating_entries.back()); @@ -478,6 +499,8 @@ class ObjectBulker _Out_ sai_status_t *object_status, _In_ sai_object_id_t object_id) { + assert(object_status); + if (!object_status) throw std::invalid_argument("object_status is null"); assert(object_id != SAI_NULL_OBJECT_ID); if (object_id == SAI_NULL_OBJECT_ID) throw std::invalid_argument("object_id is null"); From 05066944d1831bd562eba8742cd6040af34cfb98 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Thu, 30 Apr 2020 02:06:16 +0000 Subject: [PATCH 40/42] Add test to exhaust nexthop group Signed-off-by: Qi Luo --- tests/test_nhg.py | 204 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 198 insertions(+), 6 deletions(-) diff --git a/tests/test_nhg.py b/tests/test_nhg.py index ac26b5107f..6b971bfbd0 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -3,6 +3,7 @@ import time import json import pytest +import ipaddress from swsscommon import swsscommon @@ -26,13 +27,13 @@ def test_route_nhg(self, dvs, testlog): dvs.runcmd("arp -s 10.0.0.3 00:00:00:00:00:02") dvs.runcmd("arp -s 10.0.0.5 00:00:00:00:00:03") - dvs.servers[0].runcmd("ip link set down dev eth0") == 0 - dvs.servers[1].runcmd("ip link set down dev eth0") == 0 - dvs.servers[2].runcmd("ip link set down dev eth0") == 0 + assert dvs.servers[0].runcmd("ip link set down dev eth0") == 0 + assert dvs.servers[1].runcmd("ip link set down dev eth0") == 0 + assert dvs.servers[2].runcmd("ip link set down dev eth0") == 0 - dvs.servers[0].runcmd("ip link set up dev eth0") == 0 - dvs.servers[1].runcmd("ip link set up dev eth0") == 0 - dvs.servers[2].runcmd("ip link set up dev eth0") == 0 + assert dvs.servers[0].runcmd("ip link set up dev eth0") == 0 + assert dvs.servers[1].runcmd("ip link set up dev eth0") == 0 + assert dvs.servers[2].runcmd("ip link set up dev eth0") == 0 db = swsscommon.DBConnector(0, dvs.redis_sock, 0) ps = swsscommon.ProducerStateTable(db, "ROUTE_TABLE") @@ -138,3 +139,194 @@ def test_route_nhg(self, dvs, testlog): for v in fvs: if v[0] == "SAI_NEXT_HOP_GROUP_MEMBER_ATTR_NEXT_HOP_GROUP_ID": assert v[1] == nhgid + + def test_route_nhg_exhaust(self, dvs, testlog): + """ + Test the situation of exhausting ECMP group, assume SAI_SWITCH_ATTR_NUMBER_OF_ECMP_GROUPS is 512 + + In order to achieve that, we will config + 1. 9 ports + 2. 512 routes with different nexthop group + + See Also + -------- + SwitchStateBase::set_number_of_ecmp_groups() + https://github.com/Azure/sonic-sairedis/blob/master/vslib/src/SwitchStateBase.cpp + + """ + + # TODO: check ECMP 512 + + def port_name(i): + return "Ethernet" + str(i * 4) + + def port_ip(i): + return "10.0.0." + str(i * 2) + + def peer_ip(i): + return "10.0.0." + str(i * 2 + 1) + + def port_ipprefix(i): + return port_ip(i) + "/31" + + def port_mac(i): + return "00:00:00:00:00:0" + str(i) + + def gen_ipprefix(r): + """ Construct route like 2.X.X.0/24 """ + ip = ipaddress.IPv4Address(IP_INTEGER_BASE + r * 256) + ip = str(ip) + ipprefix = ip + "/24" + return ipprefix + + def gen_nhg_fvs(binary): + nexthop = [] + ifname = [] + for i in range(MAX_PORT_COUNT): + if binary[i] == '1': + nexthop.append(peer_ip(i)) + ifname.append(port_name(i)) + + nexthop = ','.join(nexthop) + ifname = ','.join(ifname) + fvs = swsscommon.FieldValuePairs([("nexthop", nexthop), ("ifname", ifname)]) + return fvs + + def asic_route_exists(ipprefix): + keys = rtbl.getKeys() + for k in keys: + rt_key = json.loads(k) + + if rt_key['dest'] == ipprefix: + return k + else: + return None + + def asic_route_nhg_fvs(k): + (status, fvs) = rtbl.get(k) + if not status: + return None + + nhgid = None + for v in fvs: + if v[0] == "SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID": + nhgid = v[1] + break + else: + return None + + (status, fvs) = nhgtbl.get(nhgid) + if not status: + return None + return fvs + + MAX_ECMP_COUNT = 512 + MAX_PORT_COUNT = 10 + IP_INTEGER_BASE = int(ipaddress.IPv4Address(unicode("2.2.2.0"))) + + config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) + intf_tbl = swsscommon.Table(config_db, "INTERFACE") + fvs = swsscommon.FieldValuePairs([("NULL", "NULL")]) + + for i in range(MAX_PORT_COUNT): + intf_tbl.set(port_name(i), fvs) + intf_tbl.set("{}|{}".format(port_name(i), port_ipprefix(i)), fvs) + dvs.runcmd("config interface startup " + port_name(i)) + dvs.runcmd("arp -s {} {}".format(peer_ip(i), port_mac(i))) + assert dvs.servers[i].runcmd("ip link set down dev eth0") == 0 + assert dvs.servers[i].runcmd("ip link set up dev eth0") == 0 + + db = swsscommon.DBConnector(0, dvs.redis_sock, 0) + ps = swsscommon.ProducerStateTable(db, "ROUTE_TABLE") + + # Add first batch of routes with unique nexthop groups in AppDB + route_count = 0 + r = 0 + while route_count < MAX_ECMP_COUNT: + r += 1 + fmt = '{{0:0{}b}}'.format(MAX_PORT_COUNT) + binary = fmt.format(r) + # We need at least 2 ports for a nexthop group + if binary.count('1') <= 1: + continue + fvs = gen_nhg_fvs(binary) + route_ipprefix = gen_ipprefix(route_count) + ps.set(route_ipprefix, fvs) + route_count += 1 + + time.sleep(1) + + adb = swsscommon.DBConnector(1, dvs.redis_sock, 0) + rtbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY") + nhgtbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP") + + # check ASIC DB on the count of nexthop groups used + nhg_count = len(nhgtbl.getKeys()) + assert nhg_count == MAX_ECMP_COUNT + + # Add second batch of routes with unique nexthop groups in AppDB + # Add more routes with new nexthop group in AppDBdd + route_ipprefix = gen_ipprefix(route_count) + base_ipprefix = route_ipprefix + base = route_count + route_count = 0 + while route_count < 10: + r += 1 + fmt = '{{0:0{}b}}'.format(MAX_PORT_COUNT) + binary = fmt.format(r) + # We need at least 2 ports for a nexthop group + if binary.count('1') <= 1: + continue + fvs = gen_nhg_fvs(binary) + route_ipprefix = gen_ipprefix(base + route_count) + ps.set(route_ipprefix, fvs) + route_count += 1 + last_ipprefix = route_ipprefix + + time.sleep(1) + + # Check ASIC DB on the count of nexthop groups used, and it should not increase + nhg_count = len(nhgtbl.getKeys()) + assert nhg_count == MAX_ECMP_COUNT + + # Check the route points to next hop group + k = asic_route_exists("2.2.2.0/24") + assert k is not None + fvs = asic_route_nhg_fvs(k) + assert fvs is not None + + # Check the second batch does not point to next hop group + k = asic_route_exists(base_ipprefix) + assert k is not None + fvs = asic_route_nhg_fvs(k) + assert fvs is None + + # Remove part of first batch of routes with unique nexthop groups in AppDB + route_count = 0 + r = 0 + while route_count < MAX_ECMP_COUNT: + r += 1 + fmt = '{{0:0{}b}}'.format(MAX_PORT_COUNT) + binary = fmt.format(r) + # We need at least 2 ports for a nexthop group + if binary.count('1') <= 1: + continue + route_ipprefix = gen_ipprefix(route_count) + ps._del(route_ipprefix) + route_count += 1 + + time.sleep(1) + + # Check the second batch points to next hop group + k = asic_route_exists(base_ipprefix) + assert k is not None + fvs = asic_route_nhg_fvs(k) + assert fvs is not None + k = asic_route_exists(last_ipprefix) + assert k is not None + fvs = asic_route_nhg_fvs(k) + assert fvs is not None + + # Check ASIC DB on the count of nexthop groups used, and it should not increase or decrease + nhg_count = len(nhgtbl.getKeys()) + assert nhg_count == 10 From 73592f54cbc98876dee13ac9b81e64d72c1e796a Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 5 May 2020 05:36:23 +0000 Subject: [PATCH 41/42] Refine vs tests: follow new convention and remove all sleep --- tests/test_nhg.py | 55 +++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/tests/test_nhg.py b/tests/test_nhg.py index 6b971bfbd0..d01a68c6b8 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -192,8 +192,7 @@ def gen_nhg_fvs(binary): fvs = swsscommon.FieldValuePairs([("nexthop", nexthop), ("ifname", ifname)]) return fvs - def asic_route_exists(ipprefix): - keys = rtbl.getKeys() + def asic_route_exists(keys, ipprefix): for k in keys: rt_key = json.loads(k) @@ -224,8 +223,8 @@ def asic_route_nhg_fvs(k): MAX_PORT_COUNT = 10 IP_INTEGER_BASE = int(ipaddress.IPv4Address(unicode("2.2.2.0"))) - config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) - intf_tbl = swsscommon.Table(config_db, "INTERFACE") + config_db = dvs.get_config_db() + intf_tbl = swsscommon.Table(config_db.db_connection, "INTERFACE") fvs = swsscommon.FieldValuePairs([("NULL", "NULL")]) for i in range(MAX_PORT_COUNT): @@ -236,8 +235,8 @@ def asic_route_nhg_fvs(k): assert dvs.servers[i].runcmd("ip link set down dev eth0") == 0 assert dvs.servers[i].runcmd("ip link set up dev eth0") == 0 - db = swsscommon.DBConnector(0, dvs.redis_sock, 0) - ps = swsscommon.ProducerStateTable(db, "ROUTE_TABLE") + app_db = dvs.get_app_db() + ps = swsscommon.ProducerStateTable(app_db.db_connection, "ROUTE_TABLE") # Add first batch of routes with unique nexthop groups in AppDB route_count = 0 @@ -254,15 +253,13 @@ def asic_route_nhg_fvs(k): ps.set(route_ipprefix, fvs) route_count += 1 - time.sleep(1) - - adb = swsscommon.DBConnector(1, dvs.redis_sock, 0) - rtbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY") - nhgtbl = swsscommon.Table(adb, "ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP") + asic_db = dvs.get_asic_db() + rtbl = swsscommon.Table(asic_db.db_connection, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY") + nhgtbl = swsscommon.Table(asic_db.db_connection, "ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP") - # check ASIC DB on the count of nexthop groups used - nhg_count = len(nhgtbl.getKeys()) - assert nhg_count == MAX_ECMP_COUNT + # Wait and check ASIC DB the count of nexthop groups used + asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", MAX_ECMP_COUNT) + asic_routes_count = len(rtbl.getKeys()) # Add second batch of routes with unique nexthop groups in AppDB # Add more routes with new nexthop group in AppDBdd @@ -283,25 +280,24 @@ def asic_route_nhg_fvs(k): route_count += 1 last_ipprefix = route_ipprefix - time.sleep(1) - - # Check ASIC DB on the count of nexthop groups used, and it should not increase - nhg_count = len(nhgtbl.getKeys()) - assert nhg_count == MAX_ECMP_COUNT + # Wait until we get expected routes and check ASIC DB on the count of nexthop groups used, and it should not increase + keys = asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY", asic_routes_count + 10) + asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", MAX_ECMP_COUNT) # Check the route points to next hop group - k = asic_route_exists("2.2.2.0/24") + # Note: no need to wait here + k = asic_route_exists(keys, "2.2.2.0/24") assert k is not None fvs = asic_route_nhg_fvs(k) assert fvs is not None # Check the second batch does not point to next hop group - k = asic_route_exists(base_ipprefix) + k = asic_route_exists(keys, base_ipprefix) assert k is not None fvs = asic_route_nhg_fvs(k) assert fvs is None - # Remove part of first batch of routes with unique nexthop groups in AppDB + # Remove first batch of routes with unique nexthop groups in AppDB route_count = 0 r = 0 while route_count < MAX_ECMP_COUNT: @@ -315,18 +311,15 @@ def asic_route_nhg_fvs(k): ps._del(route_ipprefix) route_count += 1 - time.sleep(1) - - # Check the second batch points to next hop group - k = asic_route_exists(base_ipprefix) + # Wait and check the second batch points to next hop group + # Check ASIC DB on the count of nexthop groups used, and it should not increase or decrease + asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", 10) + keys = rtbl.getKeys() + k = asic_route_exists(keys, base_ipprefix) assert k is not None fvs = asic_route_nhg_fvs(k) assert fvs is not None - k = asic_route_exists(last_ipprefix) + k = asic_route_exists(keys, last_ipprefix) assert k is not None fvs = asic_route_nhg_fvs(k) assert fvs is not None - - # Check ASIC DB on the count of nexthop groups used, and it should not increase or decrease - nhg_count = len(nhgtbl.getKeys()) - assert nhg_count == 10 From f35f530eddeb95dd5b3c9de05158f46961888c09 Mon Sep 17 00:00:00 2001 From: Qi Luo Date: Tue, 5 May 2020 21:56:39 +0000 Subject: [PATCH 42/42] Refine vs tests: follow new convention --- tests/test_nhg.py | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/tests/test_nhg.py b/tests/test_nhg.py index d01a68c6b8..4a12590d9a 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -202,21 +202,16 @@ def asic_route_exists(keys, ipprefix): return None def asic_route_nhg_fvs(k): - (status, fvs) = rtbl.get(k) - if not status: + fvs = asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY", k) + if not fvs: return None - nhgid = None - for v in fvs: - if v[0] == "SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID": - nhgid = v[1] - break - else: + print fvs + nhgid = fvs.get("SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID") + if nhgid is None: return None - (status, fvs) = nhgtbl.get(nhgid) - if not status: - return None + fvs = asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", nhgid) return fvs MAX_ECMP_COUNT = 512 @@ -224,12 +219,11 @@ def asic_route_nhg_fvs(k): IP_INTEGER_BASE = int(ipaddress.IPv4Address(unicode("2.2.2.0"))) config_db = dvs.get_config_db() - intf_tbl = swsscommon.Table(config_db.db_connection, "INTERFACE") - fvs = swsscommon.FieldValuePairs([("NULL", "NULL")]) + fvs = {"NULL": "NULL"} for i in range(MAX_PORT_COUNT): - intf_tbl.set(port_name(i), fvs) - intf_tbl.set("{}|{}".format(port_name(i), port_ipprefix(i)), fvs) + config_db.create_entry("INTERFACE", port_name(i), fvs) + config_db.create_entry("INTERFACE", "{}|{}".format(port_name(i), port_ipprefix(i)), fvs) dvs.runcmd("config interface startup " + port_name(i)) dvs.runcmd("arp -s {} {}".format(peer_ip(i), port_mac(i))) assert dvs.servers[i].runcmd("ip link set down dev eth0") == 0 @@ -254,12 +248,10 @@ def asic_route_nhg_fvs(k): route_count += 1 asic_db = dvs.get_asic_db() - rtbl = swsscommon.Table(asic_db.db_connection, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY") - nhgtbl = swsscommon.Table(asic_db.db_connection, "ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP") # Wait and check ASIC DB the count of nexthop groups used asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", MAX_ECMP_COUNT) - asic_routes_count = len(rtbl.getKeys()) + asic_routes_count = len(asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY")) # Add second batch of routes with unique nexthop groups in AppDB # Add more routes with new nexthop group in AppDBdd @@ -295,7 +287,7 @@ def asic_route_nhg_fvs(k): k = asic_route_exists(keys, base_ipprefix) assert k is not None fvs = asic_route_nhg_fvs(k) - assert fvs is None + assert not(fvs) # Remove first batch of routes with unique nexthop groups in AppDB route_count = 0 @@ -314,7 +306,7 @@ def asic_route_nhg_fvs(k): # Wait and check the second batch points to next hop group # Check ASIC DB on the count of nexthop groups used, and it should not increase or decrease asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_NEXT_HOP_GROUP", 10) - keys = rtbl.getKeys() + keys = asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY") k = asic_route_exists(keys, base_ipprefix) assert k is not None fvs = asic_route_nhg_fvs(k)