diff --git a/fpmsyncd/fpminterface.h b/fpmsyncd/fpminterface.h deleted file mode 100644 index 7d78b81808..0000000000 --- a/fpmsyncd/fpminterface.h +++ /dev/null @@ -1,27 +0,0 @@ -#pragma once - -#include -#include - -#include "fpm/fpm.h" - -namespace swss -{ - -/** - * @brief FPM zebra communication interface - */ -class FpmInterface : public Selectable -{ -public: - virtual ~FpmInterface() = default; - - /** - * @brief Send netlink message through FPM socket - * @param msg Netlink message - * @return True on success, otherwise false is returned - */ - virtual bool send(nlmsghdr* nl_hdr) = 0; -}; - -} diff --git a/fpmsyncd/fpmlink.cpp b/fpmsyncd/fpmlink.cpp index 13d170a805..93a432641c 100644 --- a/fpmsyncd/fpmlink.cpp +++ b/fpmsyncd/fpmlink.cpp @@ -158,17 +158,11 @@ FpmLink::FpmLink(RouteSync *rsync, unsigned short port) : m_server_up = true; m_messageBuffer = new char[m_bufSize]; - m_sendBuffer = new char[m_bufSize]; - - m_routesync->onFpmConnected(*this); } FpmLink::~FpmLink() { - m_routesync->onFpmDisconnected(); - delete[] m_messageBuffer; - delete[] m_sendBuffer; if (m_connected) close(m_connection_socket); if (m_server_up) @@ -283,36 +277,3 @@ void FpmLink::processFpmMessage(fpm_msg_hdr_t* hdr) nlmsg_free(msg); } } - -bool FpmLink::send(nlmsghdr* nl_hdr) -{ - fpm_msg_hdr_t hdr{}; - - size_t len = fpm_msg_align(sizeof(hdr) + nl_hdr->nlmsg_len); - - if (len > m_bufSize) - { - SWSS_LOG_THROW("Message length %zu is greater than the send buffer size %d", len, m_bufSize); - } - - hdr.version = FPM_PROTO_VERSION; - hdr.msg_type = FPM_MSG_TYPE_NETLINK; - hdr.msg_len = htons(static_cast(len)); - - memcpy(m_sendBuffer, &hdr, sizeof(hdr)); - memcpy(m_sendBuffer + sizeof(hdr), nl_hdr, nl_hdr->nlmsg_len); - - size_t sent = 0; - while (sent != len) - { - auto rc = ::send(m_connection_socket, m_sendBuffer + sent, len - sent, 0); - if (rc == -1) - { - SWSS_LOG_ERROR("Failed to send FPM message: %s", strerror(errno)); - return false; - } - sent += rc; - } - - return true; -} diff --git a/fpmsyncd/fpmlink.h b/fpmsyncd/fpmlink.h index c025750edf..f56e0d4c47 100644 --- a/fpmsyncd/fpmlink.h +++ b/fpmsyncd/fpmlink.h @@ -11,13 +11,13 @@ #include #include +#include "selectable.h" #include "fpm/fpm.h" -#include "fpmsyncd/fpminterface.h" #include "fpmsyncd/routesync.h" namespace swss { -class FpmLink : public FpmInterface { +class FpmLink : public Selectable { public: const int MSG_BATCH_SIZE; FpmLink(RouteSync *rsync, unsigned short port = FPM_DEFAULT_PORT); @@ -41,13 +41,10 @@ class FpmLink : public FpmInterface { void processFpmMessage(fpm_msg_hdr_t* hdr); - bool send(nlmsghdr* nl_hdr) override; - private: RouteSync *m_routesync; unsigned int m_bufSize; char *m_messageBuffer; - char *m_sendBuffer; unsigned int m_pos; bool m_connected; diff --git a/fpmsyncd/fpmsyncd.cpp b/fpmsyncd/fpmsyncd.cpp index 5e16a6a6ca..8f797e178c 100644 --- a/fpmsyncd/fpmsyncd.cpp +++ b/fpmsyncd/fpmsyncd.cpp @@ -4,14 +4,10 @@ #include "select.h" #include "selectabletimer.h" #include "netdispatcher.h" -#include "netlink.h" -#include "notificationconsumer.h" -#include "subscriberstatetable.h" #include "warmRestartHelper.h" #include "fpmsyncd/fpmlink.h" #include "fpmsyncd/routesync.h" -#include using namespace std; using namespace swss; @@ -51,47 +47,21 @@ static bool eoiuFlagsSet(Table &bgpStateTable) int main(int argc, char **argv) { swss::Logger::linkToDbNative("fpmsyncd"); - - const auto routeResponseChannelName = std::string("APPL_DB_") + APP_ROUTE_TABLE_NAME + "_RESPONSE_CHANNEL"; - DBConnector db("APPL_DB", 0); - DBConnector cfgDb("CONFIG_DB", 0); - SubscriberStateTable deviceMetadataTableSubscriber(&cfgDb, CFG_DEVICE_METADATA_TABLE_NAME); - Table deviceMetadataTable(&cfgDb, CFG_DEVICE_METADATA_TABLE_NAME); - DBConnector applStateDb("APPL_STATE_DB", 0); - std::unique_ptr routeResponseChannel; - RedisPipeline pipeline(&db); RouteSync sync(&pipeline); DBConnector stateDb("STATE_DB", 0); Table bgpStateTable(&stateDb, STATE_BGP_TABLE_NAME); - NetLink netlink; - - netlink.registerGroup(RTNLGRP_LINK); - NetDispatcher::getInstance().registerMessageHandler(RTM_NEWROUTE, &sync); NetDispatcher::getInstance().registerMessageHandler(RTM_DELROUTE, &sync); - NetDispatcher::getInstance().registerMessageHandler(RTM_NEWLINK, &sync); - NetDispatcher::getInstance().registerMessageHandler(RTM_DELLINK, &sync); - - rtnl_route_read_protocol_names(DefaultRtProtoPath); - - std::string suppressionEnabledStr; - deviceMetadataTable.hget("localhost", "suppress-fib-pending", suppressionEnabledStr); - if (suppressionEnabledStr == "enabled") - { - routeResponseChannel = std::make_unique(&applStateDb, routeResponseChannelName); - sync.setSuppressionEnabled(true); - } while (true) { try { FpmLink fpm(&sync); - Select s; SelectableTimer warmStartTimer(timespec{0, 0}); // Before eoiu flags detected, check them periodically. It also stop upon detection of reconciliation done. @@ -110,13 +80,6 @@ int main(int argc, char **argv) cout << "Connected!" << endl; s.addSelectable(&fpm); - s.addSelectable(&netlink); - s.addSelectable(&deviceMetadataTableSubscriber); - - if (sync.isSuppressionEnabled()) - { - s.addSelectable(routeResponseChannel.get()); - } /* If warm-restart feature is enabled, execute 'restoration' logic */ bool warmStartEnabled = sync.m_warmStartHelper.checkAndStart(); @@ -176,8 +139,11 @@ int main(int argc, char **argv) SWSS_LOG_NOTICE("Warm-Restart EOIU hold timer expired."); } - sync.onWarmStartEnd(applStateDb); - + if (sync.m_warmStartHelper.inProgress()) + { + sync.m_warmStartHelper.reconcile(); + SWSS_LOG_NOTICE("Warm-Restart reconciliation processed."); + } // remove the one-shot timer. s.removeSelectable(temps); pipeline.flush(); @@ -216,74 +182,6 @@ int main(int argc, char **argv) s.removeSelectable(&eoiuCheckTimer); } } - else if (temps == &deviceMetadataTableSubscriber) - { - std::deque keyOpFvsQueue; - deviceMetadataTableSubscriber.pops(keyOpFvsQueue); - - for (const auto& keyOpFvs: keyOpFvsQueue) - { - const auto& key = kfvKey(keyOpFvs); - const auto& op = kfvOp(keyOpFvs); - const auto& fvs = kfvFieldsValues(keyOpFvs); - - if (op != SET_COMMAND) - { - continue; - } - - if (key != "localhost") - { - continue; - } - - for (const auto& fv: fvs) - { - const auto& field = fvField(fv); - const auto& value = fvValue(fv); - - if (field != "suppress-fib-pending") - { - continue; - } - - bool shouldEnable = (value == "enabled"); - - if (shouldEnable && !sync.isSuppressionEnabled()) - { - routeResponseChannel = std::make_unique(&applStateDb, routeResponseChannelName); - sync.setSuppressionEnabled(true); - s.addSelectable(routeResponseChannel.get()); - } - else if (!shouldEnable && sync.isSuppressionEnabled()) - { - /* When disabling suppression we mark all existing routes offloaded in zebra - * as there could be some transient routes which are pending response from - * orchagent, thus such updates might be missing. Since we are disabling suppression - * we no longer care about real HW offload status and can mark all routes as offloaded - * to avoid routes stuck in suppressed state after transition. */ - sync.markRoutesOffloaded(db); - - sync.setSuppressionEnabled(false); - s.removeSelectable(routeResponseChannel.get()); - routeResponseChannel.reset(); - } - } // end for fvs - } // end for keyOpFvsQueue - } - else if (routeResponseChannel && (temps == routeResponseChannel.get())) - { - std::deque notifications; - routeResponseChannel->pops(notifications); - - for (const auto& notification: notifications) - { - const auto& key = kfvKey(notification); - const auto& fieldValues = kfvFieldsValues(notification); - - sync.onRouteResponse(key, fieldValues); - } - } else if (!warmStartEnabled || sync.m_warmStartHelper.isReconciled()) { pipeline.flush(); diff --git a/fpmsyncd/routesync.cpp b/fpmsyncd/routesync.cpp index caf6210084..e87b9fa323 100644 --- a/fpmsyncd/routesync.cpp +++ b/fpmsyncd/routesync.cpp @@ -10,7 +10,6 @@ #include "fpmsyncd/fpmlink.h" #include "fpmsyncd/routesync.h" #include "macaddress.h" -#include "converter.h" #include #include @@ -45,36 +44,6 @@ using namespace swss; #define ETHER_ADDR_STRLEN (3*ETH_ALEN) -/* Returns name of the protocol passed number represents */ -static string getProtocolString(int proto) -{ - static constexpr size_t protocolNameBufferSize = 128; - char buffer[protocolNameBufferSize] = {}; - - if (!rtnl_route_proto2str(proto, buffer, sizeof(buffer))) - { - return std::to_string(proto); - } - - return buffer; -} - -/* Helper to create unique pointer with custom destructor */ -template -static decltype(auto) makeUniqueWithDestructor(T* ptr, F func) -{ - return std::unique_ptr(ptr, func); -} - -template -static decltype(auto) makeNlAddr(const T& ip) -{ - nl_addr* addr; - nl_addr_parse(ip.to_string().c_str(), AF_UNSPEC, &addr); - return makeUniqueWithDestructor(addr, nl_addr_put); -} - - RouteSync::RouteSync(RedisPipeline *pipeline) : m_routeTable(pipeline, APP_ROUTE_TABLE_NAME, true), m_label_routeTable(pipeline, APP_LABEL_ROUTE_TABLE_NAME, true), @@ -500,8 +469,6 @@ void RouteSync::onEvpnRouteMsg(struct nlmsghdr *h, int len) return; } - sendOffloadReply(h); - switch (rtm->rtm_type) { case RTN_BLACKHOLE: @@ -602,12 +569,6 @@ void RouteSync::onMsgRaw(struct nlmsghdr *h) void RouteSync::onMsg(int nlmsg_type, struct nl_object *obj) { - if (nlmsg_type == RTM_NEWLINK || nlmsg_type == RTM_DELLINK) - { - nl_cache_refill(m_nl_sock, m_link_cache); - return; - } - struct rtnl_route *route_obj = (struct rtnl_route *)obj; /* Supports IPv4 or IPv6 address, otherwise return immediately */ @@ -724,11 +685,6 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf) return; } - if (!isSuppressionEnabled()) - { - sendOffloadReply(route_obj); - } - switch (rtnl_route_get_type(route_obj)) { case RTN_BLACKHOLE: @@ -807,15 +763,10 @@ void RouteSync::onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf) } } - auto proto_num = rtnl_route_get_protocol(route_obj); - auto proto_str = getProtocolString(proto_num); - vector fvVector; - FieldValueTuple proto("protocol", proto_str); FieldValueTuple gw("nexthop", gw_list); FieldValueTuple intf("ifname", intf_list); - fvVector.push_back(proto); fvVector.push_back(gw); fvVector.push_back(intf); if (!mpls_list.empty()) @@ -879,8 +830,6 @@ void RouteSync::onLabelRouteMsg(int nlmsg_type, struct nl_object *obj) return; } - sendOffloadReply(route_obj); - /* Get the index of the master device */ uint32_t master_index = rtnl_route_get_table(route_obj); /* if the table_id is not set in the route obj then route is for default vrf. */ @@ -986,8 +935,6 @@ void RouteSync::onVnetRouteMsg(int nlmsg_type, struct nl_object *obj, string vne return; } - sendOffloadReply(route_obj); - switch (rtnl_route_get_type(route_obj)) { case RTN_UNICAST: @@ -1088,18 +1035,6 @@ bool RouteSync::getIfName(int if_index, char *if_name, size_t name_len) return true; } -rtnl_link* RouteSync::getLinkByName(const char *name) -{ - auto link = rtnl_link_get_by_name(m_link_cache, name); - if (link == nullptr) - { - /* Trying to refill cache */ - nl_cache_refill(m_nl_sock ,m_link_cache); - link = rtnl_link_get_by_name(m_link_cache, name); - } - return link; -} - /* * getNextHopList() - parses next hop list attached to route_obj * @arg route_obj (input) Netlink route object @@ -1313,198 +1248,3 @@ string RouteSync::getNextHopWt(struct rtnl_route *route_obj) return result; } - -bool RouteSync::sendOffloadReply(struct nlmsghdr* hdr) -{ - SWSS_LOG_ENTER(); - - if (hdr->nlmsg_type != RTM_NEWROUTE) - { - return false; - } - - // Add request flag (required by zebra) - hdr->nlmsg_flags |= NLM_F_REQUEST; - - rtmsg *rtm = static_cast(NLMSG_DATA(hdr)); - - // Add offload flag - rtm->rtm_flags |= RTM_F_OFFLOAD; - - if (!m_fpmInterface) - { - SWSS_LOG_ERROR("Cannot send offload reply to zebra: FPM is disconnected"); - return false; - } - - // Send to zebra - if (!m_fpmInterface->send(hdr)) - { - SWSS_LOG_ERROR("Failed to send reply to zebra"); - return false; - } - - return true; -} - -bool RouteSync::sendOffloadReply(struct rtnl_route* route_obj) -{ - SWSS_LOG_ENTER(); - - nl_msg* msg{}; - rtnl_route_build_add_request(route_obj, NLM_F_CREATE, &msg); - - auto nlMsg = makeUniqueWithDestructor(msg, nlmsg_free); - - return sendOffloadReply(nlmsg_hdr(nlMsg.get())); -} - -void RouteSync::setSuppressionEnabled(bool enabled) -{ - SWSS_LOG_ENTER(); - - m_isSuppressionEnabled = enabled; - - SWSS_LOG_NOTICE("Pending routes suppression is %s", (m_isSuppressionEnabled ? "enabled": "disabled")); -} - -void RouteSync::onRouteResponse(const std::string& key, const std::vector& fieldValues) -{ - IpPrefix prefix; - std::string vrfName; - std::string protocol; - - bool isSetOperation{false}; - bool isSuccessReply{false}; - - if (!isSuppressionEnabled()) - { - return; - } - - auto colon = key.find(':'); - if (colon != std::string::npos && key.substr(0, colon).find(VRF_PREFIX) != std::string::npos) - { - vrfName = key.substr(0, colon); - prefix = IpPrefix{key.substr(colon + 1)}; - } - else - { - prefix = IpPrefix{key}; - } - - for (const auto& fieldValue: fieldValues) - { - std::string field = fvField(fieldValue); - std::string value = fvValue(fieldValue); - - if (field == "err_str") - { - isSuccessReply = (value == "SWSS_RC_SUCCESS"); - } - else if (field == "protocol") - { - // If field "protocol" is present in the field values then - // it is a SET operation. This field is absent only if we are - // processing DEL operation. - isSetOperation = true; - protocol = value; - } - } - - if (!isSetOperation) - { - SWSS_LOG_DEBUG("Received response for prefix %s(%s) deletion, ignoring ", - prefix.to_string().c_str(), vrfName.c_str()); - return; - } - - if (!isSuccessReply) - { - SWSS_LOG_INFO("Received failure response for prefix %s(%s)", - prefix.to_string().c_str(), vrfName.c_str()); - return; - } - - auto routeObject = makeUniqueWithDestructor(rtnl_route_alloc(), rtnl_route_put); - auto dstAddr = makeNlAddr(prefix); - - rtnl_route_set_dst(routeObject.get(), dstAddr.get()); - - auto proto = rtnl_route_str2proto(protocol.c_str()); - if (proto < 0) - { - proto = swss::to_uint(protocol); - } - - rtnl_route_set_protocol(routeObject.get(), static_cast(proto)); - rtnl_route_set_family(routeObject.get(), prefix.isV4() ? AF_INET : AF_INET6); - - unsigned int vrfIfIndex = 0; - if (!vrfName.empty()) - { - auto* link = getLinkByName(vrfName.c_str()); - if (!link) - { - SWSS_LOG_DEBUG("Failed to find VRF when constructing response message for prefix %s(%s). " - "This message is probably outdated", prefix.to_string().c_str(), - vrfName.c_str()); - return; - } - vrfIfIndex = rtnl_link_get_ifindex(link); - } - - rtnl_route_set_table(routeObject.get(), vrfIfIndex); - - if (!sendOffloadReply(routeObject.get())) - { - SWSS_LOG_ERROR("Failed to send RTM_NEWROUTE message to zebra on prefix %s(%s)", - prefix.to_string().c_str(), vrfName.c_str()); - return; - } - - SWSS_LOG_INFO("Sent response to zebra for prefix %s(%s)", - prefix.to_string().c_str(), vrfName.c_str()); -} - -void RouteSync::sendOffloadReply(DBConnector& db, const std::string& tableName) -{ - SWSS_LOG_ENTER(); - - Table routeTable{&db, tableName}; - - std::vector keys; - routeTable.getKeys(keys); - - for (const auto& key: keys) - { - std::vector fieldValues; - routeTable.get(key, fieldValues); - fieldValues.emplace_back("err_str", "SWSS_RC_SUCCESS"); - - onRouteResponse(key, fieldValues); - } -} - -void RouteSync::markRoutesOffloaded(swss::DBConnector& db) -{ - SWSS_LOG_ENTER(); - - sendOffloadReply(db, APP_ROUTE_TABLE_NAME); -} - -void RouteSync::onWarmStartEnd(DBConnector& applStateDb) -{ - SWSS_LOG_ENTER(); - - if (isSuppressionEnabled()) - { - markRoutesOffloaded(applStateDb); - } - - if (m_warmStartHelper.inProgress()) - { - m_warmStartHelper.reconcile(); - SWSS_LOG_NOTICE("Warm-Restart reconciliation processed."); - } -} diff --git a/fpmsyncd/routesync.h b/fpmsyncd/routesync.h index fd18b9d25a..2e53bb8d17 100644 --- a/fpmsyncd/routesync.h +++ b/fpmsyncd/routesync.h @@ -4,20 +4,10 @@ #include "dbconnector.h" #include "producerstatetable.h" #include "netmsg.h" -#include "linkcache.h" -#include "fpminterface.h" #include "warmRestartHelper.h" #include #include -#include - -// Add RTM_F_OFFLOAD define if it is not there. -// Debian buster does not provide one but it is neccessary for compilation. -#ifndef RTM_F_OFFLOAD -#define RTM_F_OFFLOAD 0x4000 /* route is offloaded */ -#endif - using namespace std; /* Parse the Raw netlink msg */ @@ -26,9 +16,6 @@ extern void netlink_parse_rtattr(struct rtattr **tb, int max, struct rtattr *rta namespace swss { -/* Path to protocol name database provided by iproute2 */ -constexpr auto DefaultRtProtoPath = "/etc/iproute2/rt_protos"; - class RouteSync : public NetMsg { public: @@ -39,31 +26,6 @@ class RouteSync : public NetMsg virtual void onMsg(int nlmsg_type, struct nl_object *obj); virtual void onMsgRaw(struct nlmsghdr *obj); - - void setSuppressionEnabled(bool enabled); - - bool isSuppressionEnabled() const - { - return m_isSuppressionEnabled; - } - - void onRouteResponse(const std::string& key, const std::vector& fieldValues); - - void onWarmStartEnd(swss::DBConnector& applStateDb); - - /* Mark all routes from DB with offloaded flag */ - void markRoutesOffloaded(swss::DBConnector& db); - - void onFpmConnected(FpmInterface& fpm) - { - m_fpmInterface = &fpm; - } - - void onFpmDisconnected() - { - m_fpmInterface = nullptr; - } - WarmStartHelper m_warmStartHelper; private: @@ -78,9 +40,6 @@ class RouteSync : public NetMsg struct nl_cache *m_link_cache; struct nl_sock *m_nl_sock; - bool m_isSuppressionEnabled{false}; - FpmInterface* m_fpmInterface {nullptr}; - /* Handle regular route (include VRF route) */ void onRouteMsg(int nlmsg_type, struct nl_object *obj, char *vrf); @@ -104,9 +63,6 @@ class RouteSync : public NetMsg /* Get interface name based on interface index */ bool getIfName(int if_index, char *if_name, size_t name_len); - /* Get interface if_index based on interface name */ - rtnl_link* getLinkByName(const char *name); - void getEvpnNextHopSep(string& nexthops, string& vni_list, string& mac_list, string& intf_list); @@ -131,15 +87,6 @@ class RouteSync : public NetMsg /* Get next hop weights*/ string getNextHopWt(struct rtnl_route *route_obj); - - /* Sends FPM message with RTM_F_OFFLOAD flag set to zebra */ - bool sendOffloadReply(struct nlmsghdr* hdr); - - /* Sends FPM message with RTM_F_OFFLOAD flag set to zebra */ - bool sendOffloadReply(struct rtnl_route* route_obj); - - /* Sends FPM message with RTM_F_OFFLOAD flag set for all routes in the table */ - void sendOffloadReply(swss::DBConnector& db, const std::string& table); }; } diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index b8b9056439..43d58dc649 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -48,8 +48,6 @@ RouteOrch::RouteOrch(DBConnector *db, vector &tableNames, { SWSS_LOG_ENTER(); - m_publisher.setBuffered(true); - sai_attribute_t attr; attr.id = SAI_SWITCH_ATTR_NUMBER_OF_ECMP_GROUPS; @@ -502,7 +500,7 @@ void RouteOrch::doTask(Consumer& consumer) auto rc = toBulk.emplace(std::piecewise_construct, std::forward_as_tuple(key, op), - std::forward_as_tuple(key, (op == SET_COMMAND))); + std::forward_as_tuple()); bool inserted = rc.second; auto& ctx = rc.first->second; @@ -633,11 +631,6 @@ void RouteOrch::doTask(Consumer& consumer) if (fvField(i) == "seg_src") srv6_source = fvValue(i); - - if (fvField(i) == "protocol") - { - ctx.protocol = fvValue(i); - } } /* @@ -858,10 +851,6 @@ void RouteOrch::doTask(Consumer& consumer) /* fullmask subnet route is same as ip2me route */ else if (ip_prefix.isFullMask() && m_intfsOrch->isPrefixSubnet(ip_prefix, alsv[0])) { - /* The prefix is full mask (/32 or /128) and it is an interface subnet route, so IntfOrch has already - * created an IP2ME route for it and we skip programming such route here as it already exists. - * However, to keep APPL_DB and APPL_STATE_DB consistent we have to publish it. */ - publishRouteState(ctx); it = consumer.m_toSync.erase(it); } /* subnet route, vrf leaked route, etc */ @@ -891,9 +880,7 @@ void RouteOrch::doTask(Consumer& consumer) } else { - /* Duplicate entry. Publish route state anyway since there could be multiple DEL, SET operations - * consolidated by ConsumerStateTable leading to orchagent receiving only the last SET update. */ - publishRouteState(ctx); + /* Duplicate entry */ it = consumer.m_toSync.erase(it); } @@ -2321,9 +2308,6 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey notifyNextHopChangeObservers(vrf_id, ipPrefix, nextHops, true); - /* Publish and update APPL STATE DB route entry programming status */ - publishRouteState(ctx); - /* * If the route uses a temporary synced NHG owned by NhgOrch, return false * in order to keep trying to update the route in case the NHG is updated, @@ -2538,9 +2522,6 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx) SWSS_LOG_INFO("Remove route %s with next hop(s) %s", ipPrefix.to_string().c_str(), it_route->second.nhg_key.to_string().c_str()); - - /* Publish removal status, removes route entry from APPL STATE DB */ - publishRouteState(ctx); if (ipPrefix.isDefaultRoute() && vrf_id == gVirtualRouterId) { @@ -2697,22 +2678,3 @@ void RouteOrch::decNhgRefCount(const std::string &nhg_index) gCbfNhgOrch->decNhgRefCount(nhg_index); } } - -void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status) -{ - SWSS_LOG_ENTER(); - - std::vector fvs; - - /* Leave the fvs empty if the operation type is "DEL". - * An empty fvs makes ResponsePublisher::publish() remove the state entry from APPL_STATE_DB - */ - if (ctx.is_set) - { - fvs.emplace_back("protocol", ctx.protocol); - } - - const bool replace = false; - - m_publisher.publish(APP_ROUTE_TABLE_NAME, ctx.key, fvs, status, replace); -} diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index b232137766..f87b56b4d3 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -122,12 +122,8 @@ struct RouteBulkContext // using_temp_nhg will track if the NhgOrch's owned NHG is temporary or not bool using_temp_nhg; - std::string key; // Key in database table - std::string protocol; // Protocol string - bool is_set; // True if set operation - - RouteBulkContext(const std::string& key, bool is_set) - : key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set) + RouteBulkContext() + : excp_intfs_flag(false), using_temp_nhg(false) { } @@ -143,8 +139,6 @@ struct RouteBulkContext excp_intfs_flag = false; vrf_id = SAI_NULL_OBJECT_ID; using_temp_nhg = false; - key.clear(); - protocol.clear(); } }; @@ -276,8 +270,6 @@ class RouteOrch : public Orch, public Subject const NhgBase &getNhg(const std::string& nhg_index); void incNhgRefCount(const std::string& nhg_index); void decNhgRefCount(const std::string& nhg_index); - - void publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status = ReturnCode(SAI_STATUS_SUCCESS)); }; #endif /* SWSS_ROUTEORCH_H */ diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index b5da2e2fbb..f2d45a19aa 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -148,7 +148,7 @@ tests_SOURCES += $(P4_ORCH_DIR)/p4orch.cpp \ $(P4_ORCH_DIR)/ext_tables_manager.cpp \ $(P4_ORCH_DIR)/tests/mock_sai_switch.cpp -tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) +tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_INCLUDES) tests_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis -lpthread \ -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lgmock -lgmock_main -lprotobuf -ldashapi @@ -218,16 +218,7 @@ tests_teammgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhired ## fpmsyncd unit tests tests_fpmsyncd_SOURCES = fpmsyncd/test_fpmlink.cpp \ - fpmsyncd/test_routesync.cpp \ - fake_netlink.cpp \ - fake_warmstarthelper.cpp \ - fake_producerstatetable.cpp \ - mock_dbconnector.cpp \ - mock_table.cpp \ - mock_hiredis.cpp \ - $(top_srcdir)/warmrestart/ \ - $(top_srcdir)/fpmsyncd/fpmlink.cpp \ - $(top_srcdir)/fpmsyncd/routesync.cpp + $(top_srcdir)/fpmsyncd/fpmlink.cpp tests_fpmsyncd_INCLUDES = $(tests_INCLUDES) -I$(top_srcdir)/tests_fpmsyncd -I$(top_srcdir)/lib -I$(top_srcdir)/warmrestart tests_fpmsyncd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) diff --git a/tests/mock_tests/fake_netlink.cpp b/tests/mock_tests/fake_netlink.cpp deleted file mode 100644 index 2370e13129..0000000000 --- a/tests/mock_tests/fake_netlink.cpp +++ /dev/null @@ -1,18 +0,0 @@ -#include -#include - -static rtnl_link* g_fakeLink = [](){ - auto fakeLink = rtnl_link_alloc(); - rtnl_link_set_ifindex(fakeLink, 42); - return fakeLink; -}(); - -extern "C" -{ - -struct rtnl_link* rtnl_link_get_by_name(struct nl_cache *cache, const char *name) -{ - return g_fakeLink; -} - -} diff --git a/tests/mock_tests/fake_producerstatetable.cpp b/tests/mock_tests/fake_producerstatetable.cpp deleted file mode 100644 index 6221556f63..0000000000 --- a/tests/mock_tests/fake_producerstatetable.cpp +++ /dev/null @@ -1,11 +0,0 @@ -#include "producerstatetable.h" - -using namespace std; - -namespace swss -{ -ProducerStateTable::ProducerStateTable(RedisPipeline *pipeline, const string &tableName, bool buffered) - : TableBase(tableName, SonicDBConfig::getSeparator(pipeline->getDBConnector())), TableName_KeySet(tableName) {} - -ProducerStateTable::~ProducerStateTable() {} -} diff --git a/tests/mock_tests/fake_warmstarthelper.cpp b/tests/mock_tests/fake_warmstarthelper.cpp deleted file mode 100644 index 147227df15..0000000000 --- a/tests/mock_tests/fake_warmstarthelper.cpp +++ /dev/null @@ -1,79 +0,0 @@ -#include "warmRestartHelper.h" - -static swss::DBConnector gDb("APPL_DB", 0); - -namespace swss { - -WarmStartHelper::WarmStartHelper(RedisPipeline *pipeline, - ProducerStateTable *syncTable, - const std::string &syncTableName, - const std::string &dockerName, - const std::string &appName) : - m_restorationTable(&gDb, "") -{ -} - -WarmStartHelper::~WarmStartHelper() -{ -} - -void WarmStartHelper::setState(WarmStart::WarmStartState state) -{ -} - -WarmStart::WarmStartState WarmStartHelper::getState() const -{ - return WarmStart::WarmStartState::INITIALIZED; -} - -bool WarmStartHelper::checkAndStart() -{ - return false; -} - -bool WarmStartHelper::isReconciled() const -{ - return false; -} - -bool WarmStartHelper::inProgress() const -{ - return false; -} - -uint32_t WarmStartHelper::getRestartTimer() const -{ - return 0; -} - -bool WarmStartHelper::runRestoration() -{ - return false; -} - -void WarmStartHelper::insertRefreshMap(const KeyOpFieldsValuesTuple &kfv) -{ -} - -void WarmStartHelper::reconcile() -{ -} - -const std::string WarmStartHelper::printKFV(const std::string &key, - const std::vector &fv) -{ - return ""; -} - -bool WarmStartHelper::compareAllFV(const std::vector &left, - const std::vector &right) -{ - return false; -} - -bool WarmStartHelper::compareOneFV(const std::string &v1, const std::string &v2) -{ - return false; -} - -} diff --git a/tests/mock_tests/fpmsyncd/test_fpmlink.cpp b/tests/mock_tests/fpmsyncd/test_fpmlink.cpp index 258ba669a8..3d3734713a 100644 --- a/tests/mock_tests/fpmsyncd/test_fpmlink.cpp +++ b/tests/mock_tests/fpmsyncd/test_fpmlink.cpp @@ -30,10 +30,7 @@ class FpmLinkTest : public ::testing::Test NetDispatcher::getInstance().unregisterMessageHandler(RTM_DELROUTE); } - DBConnector m_db{"APPL_DB", 0}; - RedisPipeline m_pipeline{&m_db, 1}; - RouteSync m_routeSync{&m_pipeline}; - FpmLink m_fpm{&m_routeSync}; + FpmLink m_fpm{nullptr}; MockMsgHandler m_mock; }; diff --git a/tests/mock_tests/fpmsyncd/test_routesync.cpp b/tests/mock_tests/fpmsyncd/test_routesync.cpp deleted file mode 100644 index debfa16d21..0000000000 --- a/tests/mock_tests/fpmsyncd/test_routesync.cpp +++ /dev/null @@ -1,172 +0,0 @@ -#include "fpmsyncd/routesync.h" - -#include -#include - -using namespace swss; - -using ::testing::_; - -class MockFpm : public FpmInterface -{ -public: - MockFpm(RouteSync* routeSync) : - m_routeSync(routeSync) - { - m_routeSync->onFpmConnected(*this); - } - - ~MockFpm() override - { - m_routeSync->onFpmDisconnected(); - } - - MOCK_METHOD1(send, bool(nlmsghdr*)); - MOCK_METHOD0(getFd, int()); - MOCK_METHOD0(readData, uint64_t()); - -private: - RouteSync* m_routeSync{}; -}; - -class FpmSyncdResponseTest : public ::testing::Test -{ -public: - void SetUp() override - { - EXPECT_EQ(rtnl_route_read_protocol_names(DefaultRtProtoPath), 0); - m_routeSync.setSuppressionEnabled(true); - } - - void TearDown() override - { - } - - DBConnector m_db{"APPL_DB", 0}; - RedisPipeline m_pipeline{&m_db, 1}; - RouteSync m_routeSync{&m_pipeline}; - MockFpm m_mockFpm{&m_routeSync}; -}; - -TEST_F(FpmSyncdResponseTest, RouteResponseFeedbackV4) -{ - // Expect the message to zebra is sent - EXPECT_CALL(m_mockFpm, send(_)).WillOnce([&](nlmsghdr* hdr) -> bool { - rtnl_route* routeObject{}; - - rtnl_route_parse(hdr, &routeObject); - - // table is 0 when no in default VRF - EXPECT_EQ(rtnl_route_get_table(routeObject), 0); - EXPECT_EQ(rtnl_route_get_protocol(routeObject), RTPROT_KERNEL); - - // Offload flag is set - EXPECT_EQ(rtnl_route_get_flags(routeObject) & RTM_F_OFFLOAD, RTM_F_OFFLOAD); - - return true; - }); - - m_routeSync.onRouteResponse("1.0.0.0/24", { - {"err_str", "SWSS_RC_SUCCESS"}, - {"protocol", "kernel"}, - }); -} - -TEST_F(FpmSyncdResponseTest, RouteResponseFeedbackV4Vrf) -{ - // Expect the message to zebra is sent - EXPECT_CALL(m_mockFpm, send(_)).WillOnce([&](nlmsghdr* hdr) -> bool { - rtnl_route* routeObject{}; - - rtnl_route_parse(hdr, &routeObject); - - // table is 42 (returned by fake link cache) when in non default VRF - EXPECT_EQ(rtnl_route_get_table(routeObject), 42); - EXPECT_EQ(rtnl_route_get_protocol(routeObject), 200); - - // Offload flag is set - EXPECT_EQ(rtnl_route_get_flags(routeObject) & RTM_F_OFFLOAD, RTM_F_OFFLOAD); - - return true; - }); - - m_routeSync.onRouteResponse("Vrf0:1.0.0.0/24", { - {"err_str", "SWSS_RC_SUCCESS"}, - {"protocol", "200"}, - }); -} - -TEST_F(FpmSyncdResponseTest, RouteResponseFeedbackV6) -{ - // Expect the message to zebra is sent - EXPECT_CALL(m_mockFpm, send(_)).WillOnce([&](nlmsghdr* hdr) -> bool { - rtnl_route* routeObject{}; - - rtnl_route_parse(hdr, &routeObject); - - // table is 0 when no in default VRF - EXPECT_EQ(rtnl_route_get_table(routeObject), 0); - EXPECT_EQ(rtnl_route_get_protocol(routeObject), RTPROT_KERNEL); - - // Offload flag is set - EXPECT_EQ(rtnl_route_get_flags(routeObject) & RTM_F_OFFLOAD, RTM_F_OFFLOAD); - - return true; - }); - - m_routeSync.onRouteResponse("1::/64", { - {"err_str", "SWSS_RC_SUCCESS"}, - {"protocol", "kernel"}, - }); -} - -TEST_F(FpmSyncdResponseTest, RouteResponseFeedbackV6Vrf) -{ - // Expect the message to zebra is sent - EXPECT_CALL(m_mockFpm, send(_)).WillOnce([&](nlmsghdr* hdr) -> bool { - rtnl_route* routeObject{}; - - rtnl_route_parse(hdr, &routeObject); - - // table is 42 (returned by fake link cache) when in non default VRF - EXPECT_EQ(rtnl_route_get_table(routeObject), 42); - EXPECT_EQ(rtnl_route_get_protocol(routeObject), 200); - - // Offload flag is set - EXPECT_EQ(rtnl_route_get_flags(routeObject) & RTM_F_OFFLOAD, RTM_F_OFFLOAD); - - return true; - }); - - m_routeSync.onRouteResponse("Vrf0:1::/64", { - {"err_str", "SWSS_RC_SUCCESS"}, - {"protocol", "200"}, - }); -} - -TEST_F(FpmSyncdResponseTest, WarmRestart) -{ - std::vector fieldValues = { - {"protocol", "kernel"}, - }; - - DBConnector applStateDb{"APPL_STATE_DB", 0}; - Table routeStateTable{&applStateDb, APP_ROUTE_TABLE_NAME}; - - routeStateTable.set("1.0.0.0/24", fieldValues); - routeStateTable.set("2.0.0.0/24", fieldValues); - routeStateTable.set("Vrf0:3.0.0.0/24", fieldValues); - - EXPECT_CALL(m_mockFpm, send(_)).Times(3).WillRepeatedly([&](nlmsghdr* hdr) -> bool { - rtnl_route* routeObject{}; - - rtnl_route_parse(hdr, &routeObject); - - // Offload flag is set - EXPECT_EQ(rtnl_route_get_flags(routeObject) & RTM_F_OFFLOAD, RTM_F_OFFLOAD); - - return true; - }); - - m_routeSync.onWarmStartEnd(applStateDb); -} diff --git a/tests/mock_tests/routeorch_ut.cpp b/tests/mock_tests/routeorch_ut.cpp index 8073aee92b..a96819e56e 100644 --- a/tests/mock_tests/routeorch_ut.cpp +++ b/tests/mock_tests/routeorch_ut.cpp @@ -7,14 +7,10 @@ #include "ut_helper.h" #include "mock_orchagent_main.h" #include "mock_table.h" -#include "mock_response_publisher.h" #include "bulker.h" extern string gMySwitchType; -extern std::unique_ptr gMockResponsePublisher; - -using ::testing::_; namespace routeorch_test { @@ -286,10 +282,6 @@ namespace routeorch_test {"mac_addr", "00:00:00:00:00:00" }}); intfTable.set("Ethernet0:10.0.0.1/24", { { "scope", "global" }, { "family", "IPv4" }}); - intfTable.set("Ethernet4", { {"NULL", "NULL" }, - {"mac_addr", "00:00:00:00:00:00" }}); - intfTable.set("Ethernet4:11.0.0.1/32", { { "scope", "global" }, - { "family", "IPv4" }}); gIntfsOrch->addExistingData(&intfTable); static_cast(gIntfsOrch)->doTask(); @@ -430,58 +422,4 @@ namespace routeorch_test ASSERT_EQ(current_set_count + 1, set_route_count); ASSERT_EQ(sai_fail_count, 0); } - - TEST_F(RouteOrchTest, RouteOrchTestSetDelResponse) - { - gMockResponsePublisher = std::make_unique(); - - std::deque entries; - std::string key = "2.2.2.0/24"; - std::vector fvs{{"ifname", "Ethernet0,Ethernet0"}, {"nexthop", "10.0.0.2,10.0.0.3"}, {"protocol", "bgp"}}; - entries.push_back({key, "SET", fvs}); - - auto consumer = dynamic_cast(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME)); - consumer->addToSync(entries); - - EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1); - static_cast(gRouteOrch)->doTask(); - - // add entries again to the consumer queue (in case of rapid DEL/SET operations from fpmsyncd, routeorch just gets the last SET update) - consumer->addToSync(entries); - - EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1); - static_cast(gRouteOrch)->doTask(); - - entries.clear(); - - // Route deletion - - entries.clear(); - entries.push_back({key, "DEL", {}}); - - consumer->addToSync(entries); - - EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1); - static_cast(gRouteOrch)->doTask(); - - gMockResponsePublisher.reset(); - } - - TEST_F(RouteOrchTest, RouteOrchSetFullMaskSubnetPrefix) - { - gMockResponsePublisher = std::make_unique(); - - std::deque entries; - std::string key = "11.0.0.1/32"; - std::vector fvs{{"ifname", "Ethernet4"}, {"nexthop", "0.0.0.0"}, {"protocol", "bgp"}}; - entries.push_back({key, "SET", fvs}); - - auto consumer = dynamic_cast(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME)); - consumer->addToSync(entries); - - EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1); - static_cast(gRouteOrch)->doTask(); - - gMockResponsePublisher.reset(); - } } diff --git a/tests/test_route.py b/tests/test_route.py index dfa6d04cc4..ed96a34bde 100644 --- a/tests/test_route.py +++ b/tests/test_route.py @@ -1035,84 +1035,6 @@ def test_PerfAddRemoveRoute(self, dvs, testlog): dvs.servers[1].runcmd("ip route del default dev eth0") dvs.servers[1].runcmd("ip address del 10.0.0.3/31 dev eth0") -class TestFpmSyncResponse(TestRouteBase): - @pytest.fixture - def setup(self, dvs): - self.setup_db(dvs) - - # create l3 interface - self.create_l3_intf("Ethernet0", "") - # set ip address - self.add_ip_address("Ethernet0", "10.0.0.0/31") - # bring up interface - self.set_admin_status("Ethernet0", "up") - - # set ip address and default route - dvs.servers[0].runcmd("ip address add 10.0.0.1/31 dev eth0") - dvs.servers[0].runcmd("ip route add default via 10.0.0.0") - - dvs.runcmd("ping -c 1 10.0.0.1") - - yield - - # remove ip address and default route - dvs.servers[0].runcmd("ip route del default dev eth0") - dvs.servers[0].runcmd("ip address del 10.0.0.1/31 dev eth0") - - # bring interface down - self.set_admin_status("Ethernet0", "down") - # remove ip address - self.remove_ip_address("Ethernet0", "10.0.0.0/31") - # remove l3 interface - self.remove_l3_intf("Ethernet0") - - def is_offloaded(self, dvs, route): - rc, output = dvs.runcmd(f"vtysh -c 'show ip route {route} json'") - assert rc == 0 - - route_entry = json.loads(output) - return bool(route_entry[route][0].get('offloaded')) - - @pytest.mark.xfail(reason="Requires VS docker update in https://github.com/sonic-net/sonic-buildimage/pull/12853") - @pytest.mark.parametrize("suppress_state", ["enabled", "disabled"]) - def test_offload(self, suppress_state, setup, dvs): - route = "1.1.1.0/24" - - # enable route suppression - rc, _ = dvs.runcmd(f"config suppress-fib-pending {suppress_state}") - assert rc == 0, "Failed to configure suppress-fib-pending" - - time.sleep(5) - - try: - rc, _ = dvs.runcmd("bash -c 'kill -SIGSTOP $(pidof orchagent)'") - assert rc == 0, "Failed to suspend orchagent" - - rc, _ = dvs.runcmd(f"ip route add {route} via 10.0.0.1 proto bgp") - assert rc == 0, "Failed to configure route" - - time.sleep(5) - - if suppress_state == 'disabled': - assert self.is_offloaded(dvs,route), f"{route} is expected to be offloaded (suppression is {suppress_state})" - return - - assert not self.is_offloaded(dvs, route), f"{route} is expected to be not offloaded (suppression is {suppress_state})" - - rc, _ = dvs.runcmd("bash -c 'kill -SIGCONT $(pidof orchagent)'") - assert rc == 0, "Failed to resume orchagent" - - def check_offloaded(): - return (self.is_offloaded(dvs, route), None) - - wait_for_result(check_offloaded, failure_message=f"{route} is expected to be offloaded after orchagent resume") - finally: - dvs.runcmd("bash -c 'kill -SIGCONT $(pidof orchagent)'") - dvs.runcmd(f"ip route del {route}") - - # make sure route suppression is disabled - dvs.runcmd("config suppress-fib-pending disabled") - # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying def test_nonflaky_dummy():