From 28aa309ed04a5b442487c0404db23229cec9b4c9 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Tue, 15 Nov 2022 17:21:25 +0200 Subject: [PATCH] [fpm] Fix FpmLink to read all netlink messages from FPM message (#2492) In case of using dplane_fpm_nl zebra plugin we receive RTM_DELROUTE followed by RTM_NEWROUTE in a single FPM message when route attributes change (i.e nexthops change). Current implementation can only read the first one and ignores the rest. What I did I fixed FPM implementation to read multiple nl messages in a single FPM message. Why I did it Trying to move towards using dplane_fpm_nl. How I verified it UT and using dplane_fpm_nl zebra plugin. Details if related --- .gitignore | 1 + fpmsyncd/fpmlink.cpp | 82 +++++++++++++--------- fpmsyncd/fpmlink.h | 2 + tests/mock_tests/Makefile.am | 15 +++- tests/mock_tests/fpmsyncd/test_fpmlink.cpp | 68 ++++++++++++++++++ 5 files changed, 134 insertions(+), 34 deletions(-) create mode 100644 tests/mock_tests/fpmsyncd/test_fpmlink.cpp diff --git a/.gitignore b/.gitignore index c2522ba71178..13debec21ad4 100644 --- a/.gitignore +++ b/.gitignore @@ -74,6 +74,7 @@ swssconfig/swssplayer tlm_teamd/tlm_teamd teamsyncd/teamsyncd tests/tests +tests/mock_tests/tests_fpmsyncd # Test Files # diff --git a/fpmsyncd/fpmlink.cpp b/fpmsyncd/fpmlink.cpp index c26f34e062b0..93a432641cb9 100644 --- a/fpmsyncd/fpmlink.cpp +++ b/fpmsyncd/fpmlink.cpp @@ -210,48 +210,24 @@ uint64_t FpmLink::readData() hdr = reinterpret_cast(static_cast(m_messageBuffer + start)); left = m_pos - start; if (left < FPM_MSG_HDR_LEN) + { break; + } + /* fpm_msg_len includes header size */ msg_len = fpm_msg_len(hdr); if (left < msg_len) + { break; + } if (!fpm_msg_ok(hdr, left)) - throw system_error(make_error_code(errc::bad_message), "Malformed FPM message received"); - - if (hdr->msg_type == FPM_MSG_TYPE_NETLINK) { - bool isRaw = false; - - nlmsghdr *nl_hdr = (nlmsghdr *)fpm_msg_data(hdr); - - /* - * EVPN Type5 Add Routes need to be process in Raw mode as they contain - * RMAC, VLAN and L3VNI information. - * Where as all other route will be using rtnl api to extract information - * from the netlink msg. - * */ - isRaw = isRawProcessing(nl_hdr); - - nl_msg *msg = nlmsg_convert(nl_hdr); - if (msg == NULL) - { - throw system_error(make_error_code(errc::bad_message), "Unable to convert nlmsg"); - } + throw system_error(make_error_code(errc::bad_message), "Malformed FPM message received"); + } - nlmsg_set_proto(msg, NETLINK_ROUTE); + processFpmMessage(hdr); - if (isRaw) - { - /* EVPN Type5 Add route processing */ - processRawMsg(nl_hdr); - } - else - { - NetDispatcher::getInstance().onNetlinkMessage(msg); - } - nlmsg_free(msg); - } start += msg_len; } @@ -259,3 +235,45 @@ uint64_t FpmLink::readData() m_pos = m_pos - (uint32_t)start; return 0; } + +void FpmLink::processFpmMessage(fpm_msg_hdr_t* hdr) +{ + size_t msg_len = fpm_msg_len(hdr); + + if (hdr->msg_type != FPM_MSG_TYPE_NETLINK) + { + return; + } + nlmsghdr *nl_hdr = (nlmsghdr *)fpm_msg_data(hdr); + + /* Read all netlink messages inside FPM message */ + for (; NLMSG_OK (nl_hdr, msg_len); nl_hdr = NLMSG_NEXT(nl_hdr, msg_len)) + { + /* + * EVPN Type5 Add Routes need to be process in Raw mode as they contain + * RMAC, VLAN and L3VNI information. + * Where as all other route will be using rtnl api to extract information + * from the netlink msg. + */ + bool isRaw = isRawProcessing(nl_hdr); + + nl_msg *msg = nlmsg_convert(nl_hdr); + if (msg == NULL) + { + throw system_error(make_error_code(errc::bad_message), "Unable to convert nlmsg"); + } + + nlmsg_set_proto(msg, NETLINK_ROUTE); + + if (isRaw) + { + /* EVPN Type5 Add route processing */ + processRawMsg(nl_hdr); + } + else + { + NetDispatcher::getInstance().onNetlinkMessage(msg); + } + nlmsg_free(msg); + } +} diff --git a/fpmsyncd/fpmlink.h b/fpmsyncd/fpmlink.h index 6cceef34eaf7..f56e0d4c478e 100644 --- a/fpmsyncd/fpmlink.h +++ b/fpmsyncd/fpmlink.h @@ -39,6 +39,8 @@ class FpmLink : public Selectable { m_routesync->onMsgRaw(h); }; + void processFpmMessage(fpm_msg_hdr_t* hdr); + private: RouteSync *m_routesync; unsigned int m_bufSize; diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 51b694554b05..2a1650d70017 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -4,9 +4,9 @@ P4_ORCH_DIR = $(top_srcdir)/orchagent/p4orch CFLAGS_SAI = -I /usr/include/sai -TESTS = tests tests_intfmgrd tests_portsyncd +TESTS = tests tests_intfmgrd tests_portsyncd tests_fpmsyncd -noinst_PROGRAMS = tests tests_intfmgrd tests_portsyncd +noinst_PROGRAMS = tests tests_intfmgrd tests_portsyncd tests_fpmsyncd LDADD_SAI = -lsaimeta -lsaimetadata -lsaivs -lsairedis @@ -166,3 +166,14 @@ tests_intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST tests_intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_intfmgrd_INCLUDES) tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread + +## fpmsyncd unit tests + +tests_fpmsyncd_SOURCES = fpmsyncd/test_fpmlink.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) +tests_fpmsyncd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_fpmsyncd_INCLUDES) +tests_fpmsyncd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ + -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main diff --git a/tests/mock_tests/fpmsyncd/test_fpmlink.cpp b/tests/mock_tests/fpmsyncd/test_fpmlink.cpp new file mode 100644 index 000000000000..3d3734713a4e --- /dev/null +++ b/tests/mock_tests/fpmsyncd/test_fpmlink.cpp @@ -0,0 +1,68 @@ +#include "fpmsyncd/fpmlink.h" + +#include + +#include +#include + +using namespace swss; + +using ::testing::_; + +class MockMsgHandler : public NetMsg +{ +public: + MOCK_METHOD2(onMsg, void(int, nl_object*)); +}; + +class FpmLinkTest : public ::testing::Test +{ +public: + void SetUp() override + { + NetDispatcher::getInstance().registerMessageHandler(RTM_NEWROUTE, &m_mock); + NetDispatcher::getInstance().registerMessageHandler(RTM_DELROUTE, &m_mock); + } + + void TearDown() override + { + NetDispatcher::getInstance().unregisterMessageHandler(RTM_NEWROUTE); + NetDispatcher::getInstance().unregisterMessageHandler(RTM_DELROUTE); + } + + FpmLink m_fpm{nullptr}; + MockMsgHandler m_mock; +}; + +TEST_F(FpmLinkTest, SingleNlMessageInFpmMessage) +{ + // Single FPM message containing single RTM_NEWROUTE + alignas(fpm_msg_hdr_t) unsigned char fpmMsgBuffer[] = { + 0x01, 0x01, 0x00, 0x40, 0x3C, 0x00, 0x00, 0x00, 0x18, 0x00, 0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0xE0, + 0x12, 0x6F, 0xC4, 0x02, 0x18, 0x00, 0x00, 0xFE, 0x02, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, + 0x01, 0x00, 0x01, 0x01, 0x01, 0x00, 0x08, 0x00, 0x06, 0x00, 0x14, 0x00, 0x00, 0x00, 0x08, 0x00, 0x05, + 0x00, 0xAC, 0x1E, 0x38, 0xA6, 0x08, 0x00, 0x04, 0x00, 0x06, 0x00, 0x00, 0x00 + }; + + EXPECT_CALL(m_mock, onMsg(_, _)).Times(1); + + m_fpm.processFpmMessage(reinterpret_cast(static_cast(fpmMsgBuffer))); +} + +TEST_F(FpmLinkTest, TwoNlMessagesInFpmMessage) +{ + // Single FPM message containing RTM_DELROUTE and RTM_NEWROUTE + alignas(fpm_msg_hdr_t) unsigned char fpmMsgBuffer[] = { + 0x01, 0x01, 0x00, 0x6C, 0x2C, 0x00, 0x00, 0x00, 0x19, 0x00, 0x01, 0x04, 0x00, 0x00, 0x00, 0x00, 0xE0, 0x12, + 0x6F, 0xC4, 0x02, 0x18, 0x00, 0x00, 0xFE, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00, + 0x01, 0x01, 0x01, 0x00, 0x08, 0x00, 0x06, 0x00, 0x14, 0x00, 0x00, 0x00, 0x3C, 0x00, 0x00, 0x00, 0x18, 0x00, + 0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0xE0, 0x12, 0x6F, 0xC4, 0x02, 0x18, 0x00, 0x00, 0xFE, 0x02, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00, 0x01, 0x01, 0x01, 0x00, 0x08, 0x00, 0x06, 0x00, 0x14, 0x00, + 0x00, 0x00, 0x08, 0x00, 0x05, 0x00, 0xAC, 0x1E, 0x38, 0xA7, 0x08, 0x00, 0x04, 0x00, 0x06, 0x00, 0x00, 0x00 + }; + + EXPECT_CALL(m_mock, onMsg(_, _)).Times(2); + + m_fpm.processFpmMessage(reinterpret_cast(static_cast(fpmMsgBuffer))); +} +