diff --git a/orchagent/muxorch.cpp b/orchagent/muxorch.cpp index 4cd0e7dbe7..aa4aea2f87 100644 --- a/orchagent/muxorch.cpp +++ b/orchagent/muxorch.cpp @@ -744,8 +744,8 @@ void MuxNbrHandler::update(NextHopKey nh, sai_object_id_t tunnelId, bool add, Mu bool MuxNbrHandler::enable(bool update_rt) { NeighborEntry neigh; - std::list bulk_neigh_ctx_list; - std::list bulk_route_ctx_list; + std::list neigh_ctx_list; + std::list route_ctx_list; auto it = neighbors_.begin(); while (it != neighbors_.end()) @@ -753,11 +753,12 @@ bool MuxNbrHandler::enable(bool update_rt) SWSS_LOG_INFO("Enabling neigh %s on %s", it->first.to_string().c_str(), alias_.c_str()); neigh = NeighborEntry(it->first, alias_); - bulk_neigh_ctx_list.push_back(NeighborBulkContext(neigh, true)); + // Create neighbor context with bulk_op enabled + neigh_ctx_list.push_back(NeighborContext(neigh, true)); it++; } - if (!gNeighOrch->createBulkNeighborEntries(bulk_neigh_ctx_list) || !gNeighOrch->flushBulkNeighborEntries(bulk_neigh_ctx_list)) + if (!gNeighOrch->enableNeighbors(neigh_ctx_list)) { return false; } @@ -804,7 +805,7 @@ bool MuxNbrHandler::enable(bool update_rt) IpPrefix pfx = it->first.to_string(); if (update_rt) { - bulk_route_ctx_list.push_back(MuxRouteBulkContext(pfx, false)); + route_ctx_list.push_back(MuxRouteBulkContext(pfx)); } it++; @@ -812,15 +813,8 @@ bool MuxNbrHandler::enable(bool update_rt) if (update_rt) { - if (!createBulkRouteEntries(bulk_route_ctx_list)) + if (!removeRoutes(route_ctx_list)) { - gRouteBulker.clear(); - return false; - } - gRouteBulker.flush(); - if (!processBulkRouteEntries(bulk_route_ctx_list)) - { - gRouteBulker.clear(); return false; } @@ -844,8 +838,8 @@ bool MuxNbrHandler::enable(bool update_rt) bool MuxNbrHandler::disable(sai_object_id_t tnh) { NeighborEntry neigh; - std::list bulk_neigh_ctx_list; - std::list bulk_route_ctx_list; + std::list neigh_ctx_list; + std::list route_ctx_list; auto it = neighbors_.begin(); while (it != neighbors_.end()) @@ -887,32 +881,25 @@ bool MuxNbrHandler::disable(sai_object_id_t tnh) updateTunnelRoute(nh_key, true); IpPrefix pfx = it->first.to_string(); - bulk_route_ctx_list.push_back(MuxRouteBulkContext(pfx, it->second, true)); + route_ctx_list.push_back(MuxRouteBulkContext(pfx, it->second)); neigh = NeighborEntry(it->first, alias_); - bulk_neigh_ctx_list.push_back(NeighborBulkContext(neigh, false)); + // Create neighbor context with bulk_op enabled + neigh_ctx_list.push_back(NeighborContext(neigh, true)); it++; } - if (!createBulkRouteEntries(bulk_route_ctx_list)) - { - gRouteBulker.clear(); - return false; - } - gRouteBulker.flush(); - if (!processBulkRouteEntries(bulk_route_ctx_list)) + if (!addRoutes(route_ctx_list)) { - gRouteBulker.clear(); return false; } - if (!gNeighOrch->createBulkNeighborEntries(bulk_neigh_ctx_list) || !gNeighOrch->flushBulkNeighborEntries(bulk_neigh_ctx_list)) + if (!gNeighOrch->disableNeighbors(neigh_ctx_list)) { return false; } - gRouteBulker.clear(); return true; } @@ -927,13 +914,13 @@ sai_object_id_t MuxNbrHandler::getNextHopId(const NextHopKey nhKey) return SAI_NULL_OBJECT_ID; } -bool MuxNbrHandler::createBulkRouteEntries(std::list& bulk_ctx_list) +bool MuxNbrHandler::addRoutes(std::list& bulk_ctx_list) { - int count = 0; - - SWSS_LOG_INFO("Creating %d bulk route entries", (int)bulk_ctx_list.size()); + sai_status_t status; + bool ret = true; - for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + auto ctx = bulk_ctx_list.begin(); + while (ctx != bulk_ctx_list.end()) { auto& object_statuses = ctx->object_statuses; sai_route_entry_t route_entry; @@ -942,54 +929,120 @@ bool MuxNbrHandler::createBulkRouteEntries(std::list& bulk_ copy(route_entry.destination, ctx->pfx); subnet(route_entry.destination, route_entry.destination); - SWSS_LOG_INFO("Creating route entry %s, nh %" PRIx64 ", add:%d", ctx->pfx.getIp().to_string().c_str(), ctx->nh, ctx->add); + SWSS_LOG_INFO("Adding route entry %s, nh %" PRIx64 " to bulker", ctx->pfx.getIp().to_string().c_str(), ctx->nh); object_statuses.emplace_back(); - if (ctx->add) + sai_attribute_t attr; + vector attrs; + + attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; + attr.value.s32 = SAI_PACKET_ACTION_FORWARD; + attrs.push_back(attr); + + attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; + attr.value.oid = ctx->nh; + attrs.push_back(attr); + + status = gRouteBulker.create_entry(&object_statuses.back(), &route_entry, (uint32_t)attrs.size(), attrs.data()); + if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) { - sai_attribute_t attr; - vector attrs; + // entry exists in bulker, erase and continue + ctx = bulk_ctx_list.erase(ctx); + continue; + } - attr.id = SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION; - attr.value.s32 = SAI_PACKET_ACTION_FORWARD; - attrs.push_back(attr); + ctx++; + } - attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; - attr.value.oid = ctx->nh; - attrs.push_back(attr); + gRouteBulker.flush(); - sai_status_t status = gRouteBulker.create_entry(&object_statuses.back(), &route_entry, (uint32_t)attrs.size(), attrs.data()); - if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) - { - SWSS_LOG_ERROR("Failed to create add entry for tunnel route in bulker object, entry already exists %s,nh %" PRIx64, - ctx->pfx.getIp().to_string().c_str(), ctx->nh); + for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + { + auto& object_statuses = ctx->object_statuses; + + if (object_statuses.empty()) + { + continue; + } + + auto it_status = object_statuses.begin(); + status = *it_status++; + + sai_route_entry_t route_entry; + route_entry.switch_id = gSwitchId; + route_entry.vr_id = gVirtualRouterId; + copy(route_entry.destination, ctx->pfx); + subnet(route_entry.destination, route_entry.destination); + + if (status != SAI_STATUS_SUCCESS) + { + if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) { + SWSS_LOG_INFO("Tunnel route to %s already exists", ctx->pfx.to_string().c_str()); continue; } + SWSS_LOG_ERROR("Failed to create tunnel route %s,nh %" PRIx64 " rv:%d", + ctx->pfx.getIp().to_string().c_str(), ctx->nh, status); + ret = false; + continue; + } + + if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + { + gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); } else { - sai_status_t status = gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); - if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) - { - SWSS_LOG_ERROR("Failed to create remove entry for tunnel route in bulker object, entry already exists %s,nh %" PRIx64, - ctx->pfx.getIp().to_string().c_str(), ctx->nh); - continue; - } + gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE); } - count++; + + SWSS_LOG_NOTICE("Created tunnel route to %s ", ctx->pfx.to_string().c_str()); } - SWSS_LOG_INFO("Successfully created %d bulk route entries", count); - return true; + gRouteBulker.clear(); + return ret; } -bool MuxNbrHandler::processBulkRouteEntries(std::list& bulk_ctx_list) +bool MuxNbrHandler::removeRoutes(std::list& bulk_ctx_list) { + sai_status_t status; + bool ret = true; + + auto ctx = bulk_ctx_list.begin(); + while (ctx != bulk_ctx_list.end()) + { + auto& object_statuses = ctx->object_statuses; + sai_route_entry_t route_entry; + route_entry.switch_id = gSwitchId; + route_entry.vr_id = gVirtualRouterId; + copy(route_entry.destination, ctx->pfx); + subnet(route_entry.destination, route_entry.destination); + + SWSS_LOG_INFO("Removing route entry %s, nh %" PRIx64 "", ctx->pfx.getIp().to_string().c_str(), ctx->nh); + + object_statuses.emplace_back(); + status = gRouteBulker.remove_entry(&object_statuses.back(), &route_entry); + if (status == SAI_STATUS_SUCCESS) + { + // entry already removed, clear from list and continue + ctx = bulk_ctx_list.erase(ctx); + continue; + } + ctx++; + } + + gRouteBulker.flush(); + for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) { auto& object_statuses = ctx->object_statuses; + + if (object_statuses.empty()) + { + continue; + } + auto it_status = object_statuses.begin(); - sai_status_t status = *it_status++; + status = *it_status++; sai_route_entry_t route_entry; route_entry.switch_id = gSwitchId; @@ -997,56 +1050,32 @@ bool MuxNbrHandler::processBulkRouteEntries(std::list& bulk copy(route_entry.destination, ctx->pfx); subnet(route_entry.destination, route_entry.destination); - if (ctx->add) + if (status != SAI_STATUS_SUCCESS) { - if (status != SAI_STATUS_SUCCESS) - { - if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) { - SWSS_LOG_NOTICE("Tunnel route to %s already exists", ctx->pfx.to_string().c_str()); - continue; - } - SWSS_LOG_ERROR("Failed to create tunnel route %s,nh %" PRIx64 " rv:%d", - ctx->pfx.getIp().to_string().c_str(), ctx->nh, status); - return false; - } - - if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) - { - gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); - } - else - { - gCrmOrch->incCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE); + if (status == SAI_STATUS_ITEM_NOT_FOUND) { + SWSS_LOG_INFO("Tunnel route to %s already removed", ctx->pfx.to_string().c_str()); + continue; } + SWSS_LOG_ERROR("Failed to remove tunnel route %s, rv:%d", + ctx->pfx.getIp().to_string().c_str(), status); + ret = false; + continue; + } - SWSS_LOG_NOTICE("Created tunnel route to %s ", ctx->pfx.to_string().c_str()); + if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + { + gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); } else { - if (status != SAI_STATUS_SUCCESS) - { - if (status == SAI_STATUS_ITEM_NOT_FOUND) { - SWSS_LOG_NOTICE("Tunnel route to %s already removed", ctx->pfx.to_string().c_str()); - continue; - } - SWSS_LOG_ERROR("Failed to remove tunnel route %s, rv:%d", - ctx->pfx.getIp().to_string().c_str(), status); - return false; - } - - if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) - { - gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV4_ROUTE); - } - else - { - gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE); - } - - SWSS_LOG_NOTICE("Removed tunnel route to %s ", ctx->pfx.to_string().c_str()); + gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_ROUTE); } + + SWSS_LOG_NOTICE("Removed tunnel route to %s ", ctx->pfx.to_string().c_str()); } - return true; + + gRouteBulker.clear(); + return ret; } void MuxNbrHandler::updateTunnelRoute(NextHopKey nh, bool add) diff --git a/orchagent/muxorch.h b/orchagent/muxorch.h index 4f4ab8117c..3a6d165db4 100644 --- a/orchagent/muxorch.h +++ b/orchagent/muxorch.h @@ -41,15 +41,14 @@ struct MuxRouteBulkContext std::deque object_statuses; // Bulk statuses IpPrefix pfx; // Route prefix sai_object_id_t nh; // nexthop id - bool add; // add route bool - MuxRouteBulkContext(IpPrefix pfx, bool add) - : pfx(pfx), add(add) + MuxRouteBulkContext(IpPrefix pfx) + : pfx(pfx) { } - MuxRouteBulkContext(IpPrefix pfx, sai_object_id_t nh, bool add) - : pfx(pfx), nh(nh), add(add) + MuxRouteBulkContext(IpPrefix pfx, sai_object_id_t nh) + : pfx(pfx), nh(nh) { } }; @@ -97,8 +96,8 @@ class MuxNbrHandler string getAlias() const { return alias_; }; private: - bool createBulkRouteEntries(std::list& bulk_ctx_list); - bool processBulkRouteEntries(std::list& bulk_ctx_list); + bool removeRoutes(std::list& bulk_ctx_list); + bool addRoutes(std::list& bulk_ctx_list); inline void updateTunnelRoute(NextHopKey, bool = true); diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index e14f8c0294..3b7daa9abf 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -795,6 +795,8 @@ void NeighOrch::doTask(Consumer &consumer) NeighborEntry neighbor_entry = { ip_address, alias }; + NeighborContext ctx = NeighborContext(neighbor_entry); + if (op == SET_COMMAND) { Port p; @@ -820,6 +822,8 @@ void NeighOrch::doTask(Consumer &consumer) mac_address = MacAddress(fvValue(*i)); } + ctx.mac = mac_address; + bool nbr_not_found = (m_syncdNeighbors.find(neighbor_entry) == m_syncdNeighbors.end()); if (nbr_not_found || m_syncdNeighbors[neighbor_entry].mac != mac_address) { @@ -848,7 +852,7 @@ void NeighOrch::doTask(Consumer &consumer) it = consumer.m_toSync.erase(it); } } - else if (addNeighbor(neighbor_entry, mac_address)) + else if (addNeighbor(ctx)) { it = consumer.m_toSync.erase(it); } @@ -879,7 +883,7 @@ void NeighOrch::doTask(Consumer &consumer) { if (m_syncdNeighbors.find(neighbor_entry) != m_syncdNeighbors.end()) { - if (removeNeighbor(neighbor_entry)) + if (removeNeighbor(ctx)) { it = consumer.m_toSync.erase(it); } @@ -900,13 +904,18 @@ void NeighOrch::doTask(Consumer &consumer) } } -bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress &macAddress) +bool NeighOrch::addNeighbor(NeighborContext& ctx) { SWSS_LOG_ENTER(); sai_status_t status; + auto& object_statuses = ctx.object_statuses; + + const MacAddress &macAddress = ctx.mac; + const NeighborEntry neighborEntry = ctx.neighborEntry; IpAddress ip_address = neighborEntry.ip_address; string alias = neighborEntry.alias; + bool bulk_op = ctx.bulk_op; sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); if (rif_id == SAI_NULL_OBJECT_ID) @@ -975,7 +984,8 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress SWSS_LOG_NOTICE("Neighbor %s already learned on %s in VRF %s, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str(), vrf_name.c_str()); } - if (!removeNeighbor(temp_entry)) + NeighborContext removeContext = NeighborContext(temp_entry); + if (!removeNeighbor(removeContext)) { SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); return false; @@ -997,6 +1007,20 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress if (!hw_config && mux_orch->isNeighborActive(ip_address, macAddress, alias)) { + // Using bulker, return and post-process later + if (bulk_op) + { + SWSS_LOG_INFO("Adding neighbor entry %s on %s to bulker.", ip_address.to_string().c_str(), alias.c_str()); + object_statuses.emplace_back(); + status = gNeighBulker.create_entry(&object_statuses.back(), &neighbor_entry, (uint32_t)neighbor_attrs.size(), neighbor_attrs.data()); + if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) + { + // clear object_statuses so that this neighbor entry is skipped + object_statuses.clear(); + } + return true; + } + status = sai_neighbor_api->create_neighbor_entry(&neighbor_entry, (uint32_t)neighbor_attrs.size(), neighbor_attrs.data()); if (status != SAI_STATUS_SUCCESS) @@ -1062,8 +1086,16 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress } else if (isHwConfigured(neighborEntry)) { + ctx.set_neigh_attr_count = (int)neighbor_attrs.size(); for (auto itr : neighbor_attrs) { + if (bulk_op) + { + object_statuses.emplace_back(); + gNeighBulker.set_entry_attribute(&object_statuses.back(), &neighbor_entry, &itr); + return true; + } + status = sai_neighbor_api->set_neighbor_entry_attribute(&neighbor_entry, &itr); if (status != SAI_STATUS_SUCCESS) { @@ -1093,13 +1125,17 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress return true; } -bool NeighOrch::removeNeighbor(const NeighborEntry &neighborEntry, bool disable) +bool NeighOrch::removeNeighbor(NeighborContext& ctx, bool disable) { SWSS_LOG_ENTER(); sai_status_t status; - IpAddress ip_address = neighborEntry.ip_address; + auto& object_statuses = ctx.object_statuses; + + const NeighborEntry neighborEntry = ctx.neighborEntry; string alias = neighborEntry.alias; + IpAddress ip_address = neighborEntry.ip_address; + bool bulk_op = ctx.bulk_op; NextHopKey nexthop = { ip_address, alias }; if(m_intfsOrch->isRemoteSystemPortIntf(alias)) @@ -1170,6 +1206,18 @@ bool NeighOrch::removeNeighbor(const NeighborEntry &neighborEntry, bool disable) SWSS_LOG_NOTICE("Removed next hop %s on %s", ip_address.to_string().c_str(), alias.c_str()); + if (bulk_op) + { + object_statuses.emplace_back(); + status = gNeighBulker.remove_entry(&object_statuses.back(), &neighbor_entry); + if (status == SAI_STATUS_SUCCESS) + { + // clear object_statuses so that this neighbor entry is skipped + object_statuses.clear(); + } + return true; + } + status = sai_neighbor_api->remove_neighbor_entry(&neighbor_entry); if (status != SAI_STATUS_SUCCESS) { @@ -1229,114 +1277,7 @@ bool NeighOrch::removeNeighbor(const NeighborEntry &neighborEntry, bool disable) return true; } -/** - * @brief Creates a neighbor add entry and adds it to bulker. - * @param ctx NeighborBulkContext contains neighbor information and list of object statuses. - */ -bool NeighOrch::addBulkNeighbor(NeighborBulkContext& ctx) -{ - SWSS_LOG_ENTER(); - - sai_status_t status; - auto& object_statuses = ctx.object_statuses; - - const MacAddress &macAddress = ctx.mac; - const NeighborEntry neighborEntry = ctx.neighborEntry; - string alias = neighborEntry.alias; - IpAddress ip_address = neighborEntry.ip_address; - - SWSS_LOG_INFO("Adding neighbor entry %s on %s to bulker.", ip_address.to_string().c_str(), alias.c_str()); - - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); - if (rif_id == SAI_NULL_OBJECT_ID) - { - SWSS_LOG_INFO("Failed to get rif_id for %s", alias.c_str()); - return false; - } - - sai_neighbor_entry_t neighbor_entry; - neighbor_entry.rif_id = rif_id; - neighbor_entry.switch_id = gSwitchId; - copy(neighbor_entry.ip_address, ip_address); - - vector neighbor_attrs; - sai_attribute_t neighbor_attr; - - neighbor_attr.id = SAI_NEIGHBOR_ENTRY_ATTR_DST_MAC_ADDRESS; - memcpy(neighbor_attr.value.mac, macAddress.getMac(), 6); - neighbor_attrs.push_back(neighbor_attr); - - if ((ip_address.getAddrScope() == IpAddress::LINK_SCOPE) && (ip_address.isV4())) - { - /* Check if this prefix is a configured ip, if not allow */ - IpPrefix ipll_prefix(ip_address.getV4Addr(), 16); - if (!m_intfsOrch->isPrefixSubnet (ipll_prefix, alias)) - { - neighbor_attr.id = SAI_NEIGHBOR_ENTRY_ATTR_NO_HOST_ROUTE; - neighbor_attr.value.booldata = 1; - neighbor_attrs.push_back(neighbor_attr); - } - } - - PortsOrch* ports_orch = gDirectory.get(); - auto vlan_ports = ports_orch->getAllVlans(); - - for (auto vlan_port: vlan_ports) - { - if (vlan_port == alias) - { - continue; - } - NeighborEntry temp_entry = { ip_address, vlan_port }; - if (m_syncdNeighbors.find(temp_entry) != m_syncdNeighbors.end()) - { - SWSS_LOG_NOTICE("Neighbor %s on %s already exists, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str()); - if (!removeNeighbor(temp_entry)) - { - SWSS_LOG_ERROR("Failed to create remove neighbor entry %s on %s", ip_address.to_string().c_str(), vlan_port.c_str()); - return false; - } - } - } - - if (gMySwitchType == "voq") - { - if (!addVoqEncapIndex(alias, ip_address, neighbor_attrs)) - { - return false; - } - } - - bool hw_config = isHwConfigured(neighborEntry); - MuxOrch* mux_orch = gDirectory.get(); - if (!hw_config && mux_orch->isNeighborActive(ip_address, macAddress, alias)) - { - object_statuses.emplace_back(); - status = gNeighBulker.create_entry(&object_statuses.back(), &neighbor_entry, (uint32_t)neighbor_attrs.size(), neighbor_attrs.data()); - if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) - { - SWSS_LOG_NOTICE("Neighbor add entry %s already exists in bulker.", ip_address.to_string().c_str()); - return true; - } - } - else if (hw_config) - { - ctx.set_neigh_attr_count = (int)neighbor_attrs.size(); - for (int i = 0; i < ctx.set_neigh_attr_count; i++) - { - object_statuses.emplace_back(); - gNeighBulker.set_entry_attribute(&object_statuses.back(), &neighbor_entry, neighbor_attrs.data()); - } - } - - return true; -} - -/** - * @brief Checks statuses of bulker add operations. - * @param ctx NeighborBulkContext contains NeighborEntry and status list - */ -bool NeighOrch::addBulkNeighborPost(NeighborBulkContext& ctx) +bool NeighOrch::addNeighborPost(NeighborContext& ctx) { SWSS_LOG_ENTER(); @@ -1349,7 +1290,7 @@ bool NeighOrch::addBulkNeighborPost(NeighborBulkContext& ctx) string alias = neighborEntry.alias; IpAddress ip_address = neighborEntry.ip_address; - SWSS_LOG_INFO("Checking neighbor entry %s on %s status.", ip_address.to_string().c_str(), alias.c_str()); + SWSS_LOG_INFO("Adding neighbor entry %s on %s to bulker.", ip_address.to_string().c_str(), alias.c_str()); sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); if (rif_id == SAI_NULL_OBJECT_ID) @@ -1374,7 +1315,6 @@ bool NeighOrch::addBulkNeighborPost(NeighborBulkContext& ctx) { SWSS_LOG_ERROR("Neighbor exists: neighbor %s on %s, skipping: status:%s", macAddress.to_string().c_str(), alias.c_str(), sai_serialize_status(status).c_str()); - /* Returning True so as to skip retry */ return true; } else @@ -1464,107 +1404,7 @@ bool NeighOrch::addBulkNeighborPost(NeighborBulkContext& ctx) return true; } -/** - * @brief Creates a neighbor remove entry and adds it to bulker. - * @param ctx NeighborBulkContext contains neighbor information and list of object statuses. - */ -bool NeighOrch::removeBulkNeighbor(NeighborBulkContext& ctx) -{ - SWSS_LOG_ENTER(); - - sai_status_t status; - auto& object_statuses = ctx.object_statuses; - - const NeighborEntry neighborEntry = ctx.neighborEntry; - string alias = neighborEntry.alias; - IpAddress ip_address = neighborEntry.ip_address; - - NextHopKey nexthop = { ip_address, alias }; - if(m_intfsOrch->isRemoteSystemPortIntf(alias)) - { - //For remote system ports kernel nexthops are always on inband. Change the key - Port inbp; - gPortsOrch->getInbandPort(inbp); - assert(inbp.m_alias.length()); - - nexthop.alias = inbp.m_alias; - } - - if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) - { - return true; - } - - if (m_syncdNextHops.find(nexthop) != m_syncdNextHops.end() && m_syncdNextHops[nexthop].ref_count > 0) - { - SWSS_LOG_INFO("Failed to remove still referenced neighbor %s on %s", - m_syncdNeighbors[neighborEntry].mac.to_string().c_str(), alias.c_str()); - return false; - } - - if (isHwConfigured(neighborEntry)) - { - sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias); - - sai_neighbor_entry_t neighbor_entry; - neighbor_entry.rif_id = rif_id; - neighbor_entry.switch_id = gSwitchId; - copy(neighbor_entry.ip_address, ip_address); - - sai_object_id_t next_hop_id = m_syncdNextHops[nexthop].next_hop_id; - status = sai_next_hop_api->remove_next_hop(next_hop_id); - if (status != SAI_STATUS_SUCCESS) - { - /* When next hop is not found, we continue to remove neighbor entry. */ - if (status == SAI_STATUS_ITEM_NOT_FOUND) - { - SWSS_LOG_NOTICE("Next hop %s on %s doesn't exist, rv:%d", - ip_address.to_string().c_str(), alias.c_str(), status); - } - else - { - SWSS_LOG_ERROR("Failed to remove next hop %s on %s, rv:%d", - ip_address.to_string().c_str(), alias.c_str(), status); - task_process_status handle_status = handleSaiRemoveStatus(SAI_API_NEXT_HOP, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } - } - } - - if (status != SAI_STATUS_ITEM_NOT_FOUND) - { - if (neighbor_entry.ip_address.addr_family == SAI_IP_ADDR_FAMILY_IPV4) - { - gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV4_NEXTHOP); - } - else - { - gCrmOrch->decCrmResUsedCounter(CrmResourceType::CRM_IPV6_NEXTHOP); - } - } - - SWSS_LOG_NOTICE("Removed next hop %s on %s", - ip_address.to_string().c_str(), alias.c_str()); - - object_statuses.emplace_back(); - status = gNeighBulker.remove_entry(&object_statuses.back(), &neighbor_entry); - if (status == SAI_STATUS_ITEM_ALREADY_EXISTS) - { - SWSS_LOG_ERROR("Failed to remove neighbor %s: already exists in bulker", ip_address.to_string().c_str()); - return false; - } - } - - return true; -} - -/** - * @brief Checks statuses of bulker remove operations. - * @param ctx NeighborBulkContext contains NeighborEntry and status list - */ -bool NeighOrch::removeBulkNeighborPost(NeighborBulkContext& ctx, bool disable) +bool NeighOrch::removeNeighborPost(NeighborContext& ctx, bool disable) { SWSS_LOG_ENTER(); @@ -1683,7 +1523,11 @@ bool NeighOrch::enableNeighbor(const NeighborEntry& neighborEntry) return true; } - return addNeighbor(neighborEntry, m_syncdNeighbors[neighborEntry].mac); + NeighborEntry neigh = neighborEntry; + NeighborContext ctx = NeighborContext(neigh); + ctx.mac = m_syncdNeighbors[neighborEntry].mac; + + return addNeighbor(ctx); } bool NeighOrch::disableNeighbor(const NeighborEntry& neighborEntry) @@ -1702,96 +1546,121 @@ bool NeighOrch::disableNeighbor(const NeighborEntry& neighborEntry) return true; } - return removeNeighbor(neighborEntry, true); + NeighborContext ctx = NeighborContext(neighborEntry); + + return removeNeighbor(ctx, true); } -/** - * @brief Enters neighbor entries into neighbor bulker. - * @param bulk_ctx_list List of NeighborBulkContext entries to add to bulker. - */ -bool NeighOrch::createBulkNeighborEntries(std::list& bulk_ctx_list) +bool NeighOrch::enableNeighbors(std::list& bulk_ctx_list) { - int count = 0; - - SWSS_LOG_INFO("Creating %d bulk neighbor entries", (int)bulk_ctx_list.size()); + bool ret = true; - for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + auto ctx = bulk_ctx_list.begin(); + while (ctx != bulk_ctx_list.end()) { + if (!ctx->bulk_op) + { + SWSS_LOG_ERROR("Calling enableNeighbors with bulking disabled is not supported"); + ctx++; + continue; + } const NeighborEntry& neighborEntry = ctx->neighborEntry; ctx->mac = m_syncdNeighbors[neighborEntry].mac; if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) { SWSS_LOG_INFO("Neighbor %s not found", neighborEntry.ip_address.to_string().c_str()); + ctx++; continue; } - if (ctx->enable && isHwConfigured(neighborEntry)) + if (isHwConfigured(neighborEntry)) { SWSS_LOG_INFO("Neighbor %s is already programmed to HW", neighborEntry.ip_address.to_string().c_str()); + ctx++; continue; } - if (ctx->enable) - { - SWSS_LOG_NOTICE("Neighbor enable request for %s ", neighborEntry.ip_address.to_string().c_str()); + SWSS_LOG_NOTICE("Neighbor enable request for %s ", neighborEntry.ip_address.to_string().c_str()); - if(!addBulkNeighbor(*ctx)) - { - SWSS_LOG_INFO("Adding bulk neighbor entry failed for %s", neighborEntry.ip_address.to_string().c_str()); - return false; - } - } - else + if(!addNeighbor(*ctx)) { - SWSS_LOG_NOTICE("Neighbor disable request for %s ", neighborEntry.ip_address.to_string().c_str()); + SWSS_LOG_INFO("Adding bulk neighbor entry failed for %s", neighborEntry.ip_address.to_string().c_str()); + /* remove from list, and continue */ + ctx = bulk_ctx_list.erase(ctx); + continue; + } - if(!removeBulkNeighbor(*ctx)) - { - SWSS_LOG_INFO("Removing bulk neighbor entry failed for %s", neighborEntry.ip_address.to_string().c_str()); - return false; - } + ctx++; + } + + gNeighBulker.flush(); + + for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + { + const NeighborEntry& neighborEntry = ctx->neighborEntry; + if (!addNeighborPost(*ctx)) + { + SWSS_LOG_INFO("Enable neighbor failed for %s", neighborEntry.ip_address.to_string().c_str()); + /* finish processing bulk entries */ + ret = false; } - count++; } - SWSS_LOG_INFO("Successfully created %d bulk neighbor entries", count); - return true; + + gNeighBulker.clear(); + return ret; } -/** - * @brief Processes neighbor entries in bulker. - * @param bulk_ctx_list List of neighbor context entries to be processed. - */ -bool NeighOrch::flushBulkNeighborEntries(std::list& bulk_ctx_list) +bool NeighOrch::disableNeighbors(std::list& bulk_ctx_list) { - SWSS_LOG_INFO("Processing %d bulk add neighbor entries", (int)bulk_ctx_list.size()); - gNeighBulker.flush(); + bool ret = true; - for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + auto ctx = bulk_ctx_list.begin(); + while (ctx != bulk_ctx_list.end()) { + if (!ctx->bulk_op) + { + SWSS_LOG_ERROR("Calling disableNeighbors with bulking disabled is not supported"); + ctx++; + continue; + } const NeighborEntry& neighborEntry = ctx->neighborEntry; - if (ctx->enable) + ctx->mac = m_syncdNeighbors[neighborEntry].mac; + + if (m_syncdNeighbors.find(neighborEntry) == m_syncdNeighbors.end()) { - if (!addBulkNeighborPost(*ctx)) - { - SWSS_LOG_INFO("Enable neighbor failed for %s", neighborEntry.ip_address.to_string().c_str()); - gNeighBulker.clear(); - return false; - } + SWSS_LOG_INFO("Neighbor %s not found", neighborEntry.ip_address.to_string().c_str()); + ctx++; + continue; } - else + + SWSS_LOG_NOTICE("Neighbor disable request for %s ", neighborEntry.ip_address.to_string().c_str()); + + if(!removeNeighbor(*ctx)) { - if (!removeBulkNeighborPost(*ctx, true)) - { - gNeighBulker.clear(); - return false; - } + SWSS_LOG_INFO("Removing bulk neighbor entry failed for %s", neighborEntry.ip_address.to_string().c_str()); + /* remove from list, and continue */ + ctx = bulk_ctx_list.erase(ctx); + continue; + } + ctx++; + } + + gNeighBulker.flush(); + + for (auto ctx = bulk_ctx_list.begin(); ctx != bulk_ctx_list.end(); ctx++) + { + const NeighborEntry& neighborEntry = ctx->neighborEntry; + if (!removeNeighborPost(*ctx, true)) + { + SWSS_LOG_INFO("Disable neighbor failed for %s", neighborEntry.ip_address.to_string().c_str()); + /* finish processing bulk entries */ + ret = false; } } - SWSS_LOG_INFO("Succeeded in processing %d bulk add neighbor entries", (int)bulk_ctx_list.size()); gNeighBulker.clear(); - return true; + return ret; } sai_object_id_t NeighOrch::addTunnelNextHop(const NextHopKey& nh) @@ -1973,7 +1842,8 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) SWSS_LOG_NOTICE("VOQ encap index set failed for neighbor %s. Removing and re-adding", kfvKey(t).c_str()); //Remove neigh from SAI - if (removeNeighbor(neighbor_entry)) + NeighborContext ctx = NeighborContext(neighbor_entry); + if (removeNeighbor(ctx)) { //neigh successfully deleted from SAI. Set STATE DB to signal to remove entries from kernel m_stateSystemNeighTable->del(state_key); @@ -2004,7 +1874,9 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) } //Add neigh to SAI - if (addNeighbor(neighbor_entry, mac_address)) + NeighborContext ctx = NeighborContext(neighbor_entry); + ctx.mac = mac_address; + if (addNeighbor(ctx)) { //neigh successfully added to SAI. Set STATE DB to signal kernel programming by neighbor manager @@ -2057,7 +1929,8 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer) if (m_syncdNeighbors.find(neighbor_entry) != m_syncdNeighbors.end()) { //Remove neigh from SAI - if (removeNeighbor(neighbor_entry)) + NeighborContext ctx = NeighborContext(neighbor_entry); + if (removeNeighbor(ctx)) { //neigh successfully deleted from SAI. Set STATE DB to signal to remove entries from kernel m_stateSystemNeighTable->del(state_key); diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h index b9733fe611..179354fcb7 100644 --- a/orchagent/neighorch.h +++ b/orchagent/neighorch.h @@ -44,16 +44,25 @@ struct NeighborUpdate bool add; }; -struct NeighborBulkContext +/* + * Keeps track of neighbor entry information primarily for bulk operations + */ +struct NeighborContext { - std::deque object_statuses; // Bulk statuses - NeighborEntry neighborEntry; // Neighbor entry to process + NeighborEntry neighborEntry; // neighbor entry to process + std::deque object_statuses; // bulk statuses MacAddress mac; // neighbor mac - bool enable; // enable/disable - int set_neigh_attr_count = 0; // Keeps track of number of attr set - NeighborBulkContext(NeighborEntry neighborEntry, bool enable) - : neighborEntry(neighborEntry), enable(enable) + bool bulk_op = false; // use bulker (only for mux use for now) + int set_neigh_attr_count = 0; // number of attr set + + NeighborContext(NeighborEntry neighborEntry) + : neighborEntry(neighborEntry) + { + } + + NeighborContext(NeighborEntry neighborEntry, bool bulk_op) + : neighborEntry(neighborEntry), bulk_op(bulk_op) { } }; @@ -81,8 +90,8 @@ class NeighOrch : public Orch, public Subject, public Observer bool enableNeighbor(const NeighborEntry&); bool disableNeighbor(const NeighborEntry&); - bool createBulkNeighborEntries(std::list&); - bool flushBulkNeighborEntries(std::list&); + bool enableNeighbors(std::list&); + bool disableNeighbors(std::list&); bool isHwConfigured(const NeighborEntry&); sai_object_id_t addTunnelNextHop(const NextHopKey&); @@ -116,12 +125,10 @@ class NeighOrch : public Orch, public Subject, public Observer bool removeNextHop(const IpAddress&, const string&); - bool addNeighbor(const NeighborEntry&, const MacAddress&); - bool removeNeighbor(const NeighborEntry&, bool disable = false); - bool addBulkNeighbor(NeighborBulkContext& ctx); - bool addBulkNeighborPost(NeighborBulkContext& ctx); - bool removeBulkNeighbor(NeighborBulkContext& ctx); - bool removeBulkNeighborPost(NeighborBulkContext& ctx, bool disable = false); + bool addNeighbor(NeighborContext& ctx); + bool removeNeighbor(NeighborContext& ctx, bool disable = false); + bool addNeighborPost(NeighborContext& ctx); + bool removeNeighborPost(NeighborContext& ctx, bool disable = false); bool setNextHopFlag(const NextHopKey &, const uint32_t); bool clearNextHopFlag(const NextHopKey &, const uint32_t); diff --git a/orchagent/p4orch/tests/mock_bulker.h b/orchagent/p4orch/tests/mock_bulker.h new file mode 100644 index 0000000000..087bb70394 --- /dev/null +++ b/orchagent/p4orch/tests/mock_bulker.h @@ -0,0 +1,49 @@ +#pragma once + +#include +#define MOCK_MAX_BULK_SIZE 1000 + +extern "C" +{ +#include "sai.h" +} + +class MockNeighborBulker +{ + public: + MOCK_METHOD4(create_entry, sai_status_t(_Out_ sai_status_t *object_status, _In_ const sai_neighbor_entry_t *entry, _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list)); + + MOCK_METHOD2(remove_entry, sai_status_t(_Out_ sai_status_t *object_status, _In_ const sai_neighbor_entry_t *entry)); + + MOCK_METHOD0(flush, void()); + + MOCK_METHOD0(clear, void()); +}; + +MockNeighborBulker *mock_neighbor_bulker; + +sai_status_t mock_create_entry( + _Out_ sai_status_t *object_status, + _In_ const sai_neighbor_entry_t *entry, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) +{ + return mock_neighbor_bulker->create_entry(object_status, entry, attr_count, attr_list); +} + +sai_status_t mock_remove_entry( + _Out_ sai_status_t *object_status, + _In_ const sai_neighbor_entry_t *entry) +{ + return mock_neighbor_bulker->remove_entry(object_status, entry); +} + +void mock_flush() +{ + return mock_neighbor_bulker->flush(); +} + +void mock_clear() +{ + return mock_neighbor_bulker->clear(); +} \ No newline at end of file