[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.
sonic-net · Feb 14, 2023 · 35385ad · 35385ad
1 parent 0704f78
commit 35385ad
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 7 deletions.
diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp
@@ -47,6 +47,8 @@ RouteOrch::RouteOrch(DBConnector *db, vector<table_name_with_pri_t> &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<FieldValueTuple> 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
@@ -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
@@ -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
@@ -2,19 +2,36 @@
 #include <vector>
 
 #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<MockResponsePublisher> gMockResponsePublisher;
 
 ResponsePublisher::ResponsePublisher(bool buffered) : m_db(std::make_unique<swss::DBConnector>("APPL_STATE_DB", 0)), m_buffered(buffered) {}
 
 void ResponsePublisher::publish(
     const std::string& table, const std::string& key,
     const std::vector<swss::FieldValueTuple>& intent_attrs,
     const ReturnCode& status,
-    const std::vector<swss::FieldValueTuple>& state_attrs, bool replace) {}
+    const std::vector<swss::FieldValueTuple>& 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<swss::FieldValueTuple>& 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
@@ -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<MockResponsePublisher> 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<Orch *>(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<MockResponsePublisher>();
+
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        std::string key = "2.2.2.0/24";
+        std::vector<FieldValueTuple> 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<Consumer *>(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME));
+        consumer->addToSync(entries);
+
+        EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector<FieldValueTuple>{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
+        static_cast<Orch *>(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<FieldValueTuple>{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
+        static_cast<Orch *>(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<FieldValueTuple>{}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
+        static_cast<Orch *>(gRouteOrch)->doTask();
+
+        gMockResponsePublisher.reset();
+    }
+
+    TEST_F(RouteOrchTest, RouteOrchSetFullMaskSubnetPrefix)
+    {
+        gMockResponsePublisher = std::make_unique<MockResponsePublisher>();
+
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        std::string key = "11.0.0.1/32";
+        std::vector<FieldValueTuple> fvs{{"ifname", "Ethernet4"}, {"nexthop", "0.0.0.0"}, {"protocol", "bgp"}};
+        entries.push_back({key, "SET", fvs});
+
+        auto consumer = dynamic_cast<Consumer *>(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME));
+        consumer->addToSync(entries);
+
+        EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector<FieldValueTuple>{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
+        static_cast<Orch *>(gRouteOrch)->doTask();
+
+        gMockResponsePublisher.reset();
+    }
 }