From 35385ad0a3ecc93f172aa99487d3f4787204558c Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Tue, 14 Feb 2023 21:23:16 +0200 Subject: [PATCH] [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB (#2512) * [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB * Support route supression in BGP. Orchagent has to sends route programming feedback back to fpmsyncd which communicates with zebra. --- orchagent/routeorch.cpp | 42 ++++++++++++- orchagent/routeorch.h | 12 +++- tests/mock_tests/Makefile.am | 2 +- tests/mock_tests/fake_response_publisher.cpp | 21 ++++++- tests/mock_tests/routeorch_ut.cpp | 62 ++++++++++++++++++++ 5 files changed, 132 insertions(+), 7 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index a793ab8dcc4c..247d945d1c56 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -47,6 +47,8 @@ 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; @@ -499,7 +501,7 @@ void RouteOrch::doTask(Consumer& consumer) auto rc = toBulk.emplace(std::piecewise_construct, std::forward_as_tuple(key, op), - std::forward_as_tuple()); + std::forward_as_tuple(key, (op == SET_COMMAND))); bool inserted = rc.second; auto& ctx = rc.first->second; @@ -630,6 +632,11 @@ void RouteOrch::doTask(Consumer& consumer) if (fvField(i) == "seg_src") srv6_source = fvValue(i); + + if (fvField(i) == "protocol") + { + ctx.protocol = fvValue(i); + } } /* @@ -831,6 +838,10 @@ 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 */ @@ -860,7 +871,9 @@ void RouteOrch::doTask(Consumer& consumer) } else { - /* Duplicate entry */ + /* 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); it = consumer.m_toSync.erase(it); } @@ -2226,6 +2239,9 @@ 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, @@ -2419,6 +2435,9 @@ 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) { @@ -2574,3 +2593,22 @@ 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 5f297c6a0e6b..cc89005d7a7a 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -122,8 +122,12 @@ struct RouteBulkContext // using_temp_nhg will track if the NhgOrch's owned NHG is temporary or not bool using_temp_nhg; - RouteBulkContext() - : excp_intfs_flag(false), using_temp_nhg(false) + 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) { } @@ -139,6 +143,8 @@ struct RouteBulkContext excp_intfs_flag = false; vrf_id = SAI_NULL_OBJECT_ID; using_temp_nhg = false; + key.clear(); + protocol.clear(); } }; @@ -269,6 +275,8 @@ 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 6b648d5f4547..8adda0be11a3 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -171,7 +171,7 @@ tests_intfmgrd_INCLUDES = $(tests_INCLUDES) -I$(top_srcdir)/cfgmgr -I$(top_srcdi tests_intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) 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 + -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main ## fpmsyncd unit tests diff --git a/tests/mock_tests/fake_response_publisher.cpp b/tests/mock_tests/fake_response_publisher.cpp index 4c2c2b037098..29a28d23607e 100644 --- a/tests/mock_tests/fake_response_publisher.cpp +++ b/tests/mock_tests/fake_response_publisher.cpp @@ -2,6 +2,11 @@ #include #include "response_publisher.h" +#include "mock_response_publisher.h" + +/* This mock plugs into this fake response publisher implementation + * when needed to test code that uses response publisher. */ +std::unique_ptr gMockResponsePublisher; ResponsePublisher::ResponsePublisher(bool buffered) : m_db(std::make_unique("APPL_STATE_DB", 0)), m_buffered(buffered) {} @@ -9,12 +14,24 @@ void ResponsePublisher::publish( const std::string& table, const std::string& key, const std::vector& intent_attrs, const ReturnCode& status, - const std::vector& state_attrs, bool replace) {} + const std::vector& state_attrs, bool replace) +{ + if (gMockResponsePublisher) + { + gMockResponsePublisher->publish(table, key, intent_attrs, status, state_attrs, replace); + } +} void ResponsePublisher::publish( const std::string& table, const std::string& key, const std::vector& intent_attrs, - const ReturnCode& status, bool replace) {} + const ReturnCode& status, bool replace) +{ + if (gMockResponsePublisher) + { + gMockResponsePublisher->publish(table, key, intent_attrs, status, replace); + } +} void ResponsePublisher::writeToDB( const std::string& table, const std::string& key, diff --git a/tests/mock_tests/routeorch_ut.cpp b/tests/mock_tests/routeorch_ut.cpp index 2c1c4b8535cf..091dabed6a34 100644 --- a/tests/mock_tests/routeorch_ut.cpp +++ b/tests/mock_tests/routeorch_ut.cpp @@ -7,10 +7,14 @@ #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 { @@ -282,6 +286,10 @@ 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(); @@ -422,4 +430,58 @@ 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(); + } }