Skip to content

Commit

Permalink
[fpm] Fix FpmLink to read all netlink messages from FPM message (#2492)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@stepanblyschak
stepanblyschak authored Nov 15, 2022
1 parent da56bd6 commit 28aa309
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 34 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ swssconfig/swssplayer
tlm_teamd/tlm_teamd
teamsyncd/teamsyncd
tests/tests
tests/mock_tests/tests_fpmsyncd


# Test Files #
Expand Down
82 changes: 50 additions & 32 deletions fpmsyncd/fpmlink.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,52 +210,70 @@ uint64_t FpmLink::readData()
hdr = reinterpret_cast<fpm_msg_hdr_t *>(static_cast<void *>(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;
}

memmove(m_messageBuffer, m_messageBuffer + start, m_pos - start);
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);
}
}
2 changes: 2 additions & 0 deletions fpmsyncd/fpmlink.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 13 additions & 2 deletions tests/mock_tests/Makefile.am
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
68 changes: 68 additions & 0 deletions tests/mock_tests/fpmsyncd/test_fpmlink.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#include "fpmsyncd/fpmlink.h"

#include <swss/netdispatcher.h>

#include <gtest/gtest.h>
#include <gmock/gmock.h>

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<fpm_msg_hdr_t*>(static_cast<void*>(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<fpm_msg_hdr_t*>(static_cast<void*>(fpmMsgBuffer)));
}

0 comments on commit 28aa309

Please sign in to comment.