From 99dd9e4b77e3dde6217cb7fd0c78165e5d14d93f Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Tue, 8 Nov 2022 15:36:02 +0000 Subject: [PATCH 1/6] [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB Signed-off-by: Stepan Blyschak --- orchagent/routeorch.cpp | 37 ++++++++++++- orchagent/routeorch.h | 11 +++- tests/mock_tests/Makefile.am | 2 +- tests/mock_tests/fake_response_publisher.cpp | 21 +++++++- tests/mock_tests/routeorch_ut.cpp | 56 ++++++++++++++++++++ 5 files changed, 121 insertions(+), 6 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index a793ab8dcc..8fc3a74b8b 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)); 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,9 @@ 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])) { + /* We're not programming any route in this case but we do have to publish response + * for it because the route was already programmed by IntfOrch as part of interface configuration */ + publishRouteState(ctx, ReturnCode(SAI_STATUS_SUCCESS)); it = consumer.m_toSync.erase(it); } /* subnet route, vrf leaked route, etc */ @@ -2226,6 +2236,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, ReturnCode(SAI_STATUS_SUCCESS)); + /* * 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 +2432,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, ReturnCode(SAI_STATUS_SUCCESS)); if (ipPrefix.isDefaultRoute() && vrf_id == gVirtualRouterId) { @@ -2574,3 +2590,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; + + /* If the operation type is "DEL" then ctx.protocol is empty and fvs is left empty. + * An empty fvs makes ResponsePublisher::publish() remove the state entry from APPL_STATE_DB + */ + if (!ctx.protocol.empty()) + { + fvs.emplace_back("protocol", ctx.protocol); + }; + + const bool replace = true; + + m_publisher.publish(APP_ROUTE_TABLE_NAME, ctx.key, fvs, status, replace); +} diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 5f297c6a0e..87ce84acbd 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -122,8 +122,11 @@ 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 + + RouteBulkContext(const std::string& key) + : key(key), excp_intfs_flag(false), using_temp_nhg(false) { } @@ -139,6 +142,8 @@ struct RouteBulkContext excp_intfs_flag = false; vrf_id = SAI_NULL_OBJECT_ID; using_temp_nhg = false; + key.clear(); + protocol.clear(); } }; @@ -269,6 +274,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); }; #endif /* SWSS_ROUTEORCH_H */ diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 51b694554b..0aeb1df168 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -165,4 +165,4 @@ 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 diff --git a/tests/mock_tests/fake_response_publisher.cpp b/tests/mock_tests/fake_response_publisher.cpp index 94480913d5..3b35a4adf8 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() : m_db("APPL_STATE_DB", 0) {} @@ -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 66df4bfbcc..6586adae72 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,52 @@ 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), true)).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), true)).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), true)).Times(1); + static_cast(gRouteOrch)->doTask(); + + gMockResponsePublisher.reset(); + } } From 44d1c217fa548deb9fc59f4abe4d3426d45a3409 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 10 Nov 2022 11:41:40 +0000 Subject: [PATCH 2/6] fix comment Signed-off-by: Stepan Blyschak --- orchagent/routeorch.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 8fc3a74b8b..90ad9a0e05 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -838,8 +838,9 @@ 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])) { - /* We're not programming any route in this case but we do have to publish response - * for it because the route was already programmed by IntfOrch as part of interface configuration */ + /* 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, ReturnCode(SAI_STATUS_SUCCESS)); it = consumer.m_toSync.erase(it); } @@ -2603,7 +2604,7 @@ void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode& if (!ctx.protocol.empty()) { fvs.emplace_back("protocol", ctx.protocol); - }; + } const bool replace = true; From 47a65eb28d86b0eedd3d709b5b4cedb858c11f2a Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 17 Nov 2022 14:44:24 +0000 Subject: [PATCH 3/6] set replace false Signed-off-by: Stepan Blyschak --- orchagent/routeorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 90ad9a0e05..4eeb20047b 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2606,7 +2606,7 @@ void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode& fvs.emplace_back("protocol", ctx.protocol); } - const bool replace = true; + const bool replace = false; m_publisher.publish(APP_ROUTE_TABLE_NAME, ctx.key, fvs, status, replace); } From e9feb5ea200af528049a830622b744b0a657a738 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 17 Nov 2022 14:46:55 +0000 Subject: [PATCH 4/6] set test expect replace=false Signed-off-by: Stepan Blyschak --- tests/mock_tests/routeorch_ut.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/mock_tests/routeorch_ut.cpp b/tests/mock_tests/routeorch_ut.cpp index 6586adae72..a5949a2ec3 100644 --- a/tests/mock_tests/routeorch_ut.cpp +++ b/tests/mock_tests/routeorch_ut.cpp @@ -443,7 +443,7 @@ namespace routeorch_test 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), true)).Times(1); + 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(); @@ -455,7 +455,7 @@ namespace routeorch_test consumer->addToSync(entries); - EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{}, ReturnCode(SAI_STATUS_SUCCESS), true)).Times(1); + EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector{}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1); static_cast(gRouteOrch)->doTask(); gMockResponsePublisher.reset(); @@ -473,7 +473,7 @@ namespace routeorch_test 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), true)).Times(1); + 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(); From e50d5cd0acdc6b6621c9775687ece9d91bf76e9d Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Wed, 11 Jan 2023 15:53:01 +0200 Subject: [PATCH 5/6] use flag to determite operation type Signed-off-by: Stepan Blyschak --- orchagent/routeorch.cpp | 6 +++--- orchagent/routeorch.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 4eeb20047b..dc278cc876 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -501,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(key)); + std::forward_as_tuple(key, (op == SET_COMMAND))); bool inserted = rc.second; auto& ctx = rc.first->second; @@ -2598,10 +2598,10 @@ void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode& std::vector fvs; - /* If the operation type is "DEL" then ctx.protocol is empty and fvs is left empty. + /* 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.protocol.empty()) + if (ctx.is_set) { fvs.emplace_back("protocol", ctx.protocol); } diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 87ce84acbd..3481f7fe27 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -124,9 +124,10 @@ struct RouteBulkContext std::string key; // Key in database table std::string protocol; // Protocol string + bool is_set; // True if set operation - RouteBulkContext(const std::string& key) - : key(key), excp_intfs_flag(false), using_temp_nhg(false) + RouteBulkContext(const std::string& key, bool is_set) + : key(key), excp_intfs_flag(false), using_temp_nhg(false), is_set(is_set) { } From 0fa58de7230955d69d5ae4d8e9f403566290d7a0 Mon Sep 17 00:00:00 2001 From: Stepan Blyschak Date: Thu, 19 Jan 2023 18:00:01 +0000 Subject: [PATCH 6/6] publish route install status in case a duplicate prefix is received This is needed in case DEL/SET consolidation by ConsumerStateTable Signed-off-by: Stepan Blyschak --- orchagent/routeorch.cpp | 10 ++++++---- orchagent/routeorch.h | 2 +- tests/mock_tests/routeorch_ut.cpp | 6 ++++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index dc278cc876..247d945d1c 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -841,7 +841,7 @@ void RouteOrch::doTask(Consumer& consumer) /* 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, ReturnCode(SAI_STATUS_SUCCESS)); + publishRouteState(ctx); it = consumer.m_toSync.erase(it); } /* subnet route, vrf leaked route, etc */ @@ -871,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); } @@ -2238,7 +2240,7 @@ 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, ReturnCode(SAI_STATUS_SUCCESS)); + publishRouteState(ctx); /* * If the route uses a temporary synced NHG owned by NhgOrch, return false @@ -2435,7 +2437,7 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx) 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, ReturnCode(SAI_STATUS_SUCCESS)); + publishRouteState(ctx); if (ipPrefix.isDefaultRoute() && vrf_id == gVirtualRouterId) { diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 3481f7fe27..cc89005d7a 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -276,7 +276,7 @@ class RouteOrch : public Orch, public Subject void incNhgRefCount(const std::string& nhg_index); void decNhgRefCount(const std::string& nhg_index); - void publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status); + void publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status = ReturnCode(SAI_STATUS_SUCCESS)); }; #endif /* SWSS_ROUTEORCH_H */ diff --git a/tests/mock_tests/routeorch_ut.cpp b/tests/mock_tests/routeorch_ut.cpp index fd7d079813..091dabed6a 100644 --- a/tests/mock_tests/routeorch_ut.cpp +++ b/tests/mock_tests/routeorch_ut.cpp @@ -446,6 +446,12 @@ namespace routeorch_test 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