From 840fe1de7269ed5b79c7070b2eb727ca7ecfea9b Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Sun, 19 Mar 2023 16:22:58 +0200 Subject: [PATCH] [fpmsyncd] Implement pending route suppression feature (#2551) DEPENDS: #2512 What I did I implemented support to enable pending routes suppression feature. When this feature is enabled, fpmsyncd will wait for reply from orchagent before sending offload status message to zebra. Why I did it This is done to not announce routes which aren't yet offloaded in HW. How I verified it UT and manual tests. --- fpmsyncd/fpminterface.h | 27 ++ fpmsyncd/fpmlink.cpp | 39 +++ fpmsyncd/fpmlink.h | 7 +- fpmsyncd/fpmsyncd.cpp | 112 +++++++- fpmsyncd/routesync.cpp | 260 +++++++++++++++++++ fpmsyncd/routesync.h | 53 ++++ tests/mock_tests/Makefile.am | 13 +- tests/mock_tests/fake_netlink.cpp | 18 ++ tests/mock_tests/fake_producerstatetable.cpp | 11 + tests/mock_tests/fake_warmstarthelper.cpp | 79 ++++++ tests/mock_tests/fpmsyncd/test_fpmlink.cpp | 5 +- tests/mock_tests/fpmsyncd/test_routesync.cpp | 172 ++++++++++++ tests/test_route.py | 78 ++++++ 13 files changed, 864 insertions(+), 10 deletions(-) create mode 100644 fpmsyncd/fpminterface.h create mode 100644 tests/mock_tests/fake_netlink.cpp create mode 100644 tests/mock_tests/fake_producerstatetable.cpp create mode 100644 tests/mock_tests/fake_warmstarthelper.cpp create mode 100644 tests/mock_tests/fpmsyncd/test_routesync.cpp diff --git a/fpmsyncd/fpminterface.h b/fpmsyncd/fpminterface.h new file mode 100644 index 0000000000..7d78b81808 --- /dev/null +++ b/fpmsyncd/fpminterface.h @@ -0,0 +1,27 @@ +#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 93a432641c..13d170a805 100644 --- a/fpmsyncd/fpmlink.cpp +++ b/fpmsyncd/fpmlink.cpp @@ -158,11 +158,17 @@ 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) @@ -277,3 +283,36 @@ 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 f56e0d4c47..c025750edf 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 Selectable { +class FpmLink : public FpmInterface { public: const int MSG_BATCH_SIZE; FpmLink(RouteSync *rsync, unsigned short port = FPM_DEFAULT_PORT); @@ -41,10 +41,13 @@ class FpmLink : public Selectable { 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 8f797e178c..5e16a6a6ca 100644 --- a/fpmsyncd/fpmsyncd.cpp +++ b/fpmsyncd/fpmsyncd.cpp @@ -4,10 +4,14 @@ #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; @@ -47,21 +51,47 @@ 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. @@ -80,6 +110,13 @@ 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(); @@ -139,11 +176,8 @@ int main(int argc, char **argv) SWSS_LOG_NOTICE("Warm-Restart EOIU hold timer expired."); } - if (sync.m_warmStartHelper.inProgress()) - { - sync.m_warmStartHelper.reconcile(); - SWSS_LOG_NOTICE("Warm-Restart reconciliation processed."); - } + sync.onWarmStartEnd(applStateDb); + // remove the one-shot timer. s.removeSelectable(temps); pipeline.flush(); @@ -182,6 +216,74 @@ 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 e87b9fa323..caf6210084 100644 --- a/fpmsyncd/routesync.cpp +++ b/fpmsyncd/routesync.cpp @@ -10,6 +10,7 @@ #include "fpmsyncd/fpmlink.h" #include "fpmsyncd/routesync.h" #include "macaddress.h" +#include "converter.h" #include #include @@ -44,6 +45,36 @@ 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), @@ -469,6 +500,8 @@ void RouteSync::onEvpnRouteMsg(struct nlmsghdr *h, int len) return; } + sendOffloadReply(h); + switch (rtm->rtm_type) { case RTN_BLACKHOLE: @@ -569,6 +602,12 @@ 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 */ @@ -685,6 +724,11 @@ 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: @@ -763,10 +807,15 @@ 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()) @@ -830,6 +879,8 @@ 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. */ @@ -935,6 +986,8 @@ 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: @@ -1035,6 +1088,18 @@ 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 @@ -1248,3 +1313,198 @@ 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 2e53bb8d17..fd18b9d25a 100644 --- a/fpmsyncd/routesync.h +++ b/fpmsyncd/routesync.h @@ -4,10 +4,20 @@ #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 */ @@ -16,6 +26,9 @@ 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: @@ -26,6 +39,31 @@ 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: @@ -40,6 +78,9 @@ 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); @@ -63,6 +104,9 @@ 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); @@ -87,6 +131,15 @@ 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/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index fb3bb4d00e..ab0c1431d4 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -131,7 +131,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 @@ -177,7 +177,16 @@ tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhired ## fpmsyncd unit tests tests_fpmsyncd_SOURCES = fpmsyncd/test_fpmlink.cpp \ - $(top_srcdir)/fpmsyncd/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 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 new file mode 100644 index 0000000000..2370e13129 --- /dev/null +++ b/tests/mock_tests/fake_netlink.cpp @@ -0,0 +1,18 @@ +#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 new file mode 100644 index 0000000000..6221556f63 --- /dev/null +++ b/tests/mock_tests/fake_producerstatetable.cpp @@ -0,0 +1,11 @@ +#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 new file mode 100644 index 0000000000..147227df15 --- /dev/null +++ b/tests/mock_tests/fake_warmstarthelper.cpp @@ -0,0 +1,79 @@ +#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 3d3734713a..258ba669a8 100644 --- a/tests/mock_tests/fpmsyncd/test_fpmlink.cpp +++ b/tests/mock_tests/fpmsyncd/test_fpmlink.cpp @@ -30,7 +30,10 @@ class FpmLinkTest : public ::testing::Test NetDispatcher::getInstance().unregisterMessageHandler(RTM_DELROUTE); } - FpmLink m_fpm{nullptr}; + DBConnector m_db{"APPL_DB", 0}; + RedisPipeline m_pipeline{&m_db, 1}; + RouteSync m_routeSync{&m_pipeline}; + FpmLink m_fpm{&m_routeSync}; MockMsgHandler m_mock; }; diff --git a/tests/mock_tests/fpmsyncd/test_routesync.cpp b/tests/mock_tests/fpmsyncd/test_routesync.cpp new file mode 100644 index 0000000000..debfa16d21 --- /dev/null +++ b/tests/mock_tests/fpmsyncd/test_routesync.cpp @@ -0,0 +1,172 @@ +#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/test_route.py b/tests/test_route.py index ed96a34bde..dfa6d04cc4 100644 --- a/tests/test_route.py +++ b/tests/test_route.py @@ -1035,6 +1035,84 @@ 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():