[fpmsyncd][WR] Relax the static schema constraint for ROUTE_TABLE (#2981

) (#2983)

Remove the assert when the schema changes and in that case, imply a diff and pass it down to orchagent

tests/mock_tests/Makefile.am

@@ -54,6 +54,8 @@ tests_SOURCES = aclorch_ut.cpp \
                 mux_rollback_ut.cpp \
                 warmrestartassist_ut.cpp \
                 test_failure_handling.cpp \
+                warmrestarthelper_ut.cpp \
+                $(top_srcdir)/warmrestart/warmRestartHelper.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \
                 $(top_srcdir)/lib/subintf.cpp \
                 $(top_srcdir)/orchagent/orchdaemon.cpp \

tests/mock_tests/warmrestarthelper_ut.cpp

@@ -0,0 +1,106 @@
+#include "warmRestartHelper.h"
+#include "warm_restart.h"
+#include "mock_table.h"
+#include "ut_helper.h"
+
+using namespace testing_db;
+
+namespace wrhelper_test
+{
+    struct WRHelperTest : public ::testing::Test
+    {
+        std::shared_ptr<swss::DBConnector> m_app_db;
+        std::shared_ptr<swss::RedisPipeline> m_pipeline;
+        std::shared_ptr<swss::Table> m_routeTable;
+        std::shared_ptr<swss::ProducerStateTable> m_routeProducerTable;
+        std::shared_ptr<swss::WarmStartHelper> wrHelper;
+
+        void SetUp() override
+        {
+            m_app_db = std::make_shared<swss::DBConnector>("APPL_DB", 0);
+            m_pipeline = std::make_shared<swss::RedisPipeline>(m_app_db.get());
+            m_routeTable = std::make_shared<swss::Table>(m_app_db.get(), "ROUTE_TABLE");
+            m_routeProducerTable = std::make_shared<swss::ProducerStateTable>(m_app_db.get(), "ROUTE_TABLE");
+            wrHelper = std::make_shared<swss::WarmStartHelper>(m_pipeline.get(), m_routeProducerTable.get(), "ROUTE_TABLE", "bgp", "bgp");
+            testing_db::reset();
+        }
+
+        void TearDown() override {
+        }
+    };
+
+    TEST_F(WRHelperTest, testReconciliation)
+    {
+        /* Initialize WR */
+        wrHelper->setState(WarmStart::INITIALIZED);
+        ASSERT_EQ(wrHelper->getState(), WarmStart::INITIALIZED);
+
+        /* Old-life entries */
+        m_routeTable->set("1.0.0.0/24",
+                        {
+                            {"ifname", "eth1"},
+                            {"nexthop", "2.0.0.0"}
+                        });
+        m_routeTable->set("1.1.0.0/24",
+                        {
+                            {"ifname", "eth2"},
+                            {"nexthop", "2.1.0.0"},
+                            {"weight", "1"},
+                        });
+        m_routeTable->set("1.2.0.0/24",
+                        {
+                            {"ifname", "eth2"},
+                            {"nexthop", "2.2.0.0"},
+                            {"weight", "1"},
+                            {"random_attrib", "random_val"},
+                        });
+        wrHelper->runRestoration();
+        ASSERT_EQ(wrHelper->getState(), WarmStart::RESTORED);
+
+        /* Insert new life entries */
+        wrHelper->insertRefreshMap({
+                                    "1.0.0.0/24",
+                                    "SET",
+                                    {
+                                        {"ifname", "eth1"},
+                                        {"nexthop", "2.0.0.0"},
+                                        {"protocol", "kernel"}
+                                    }
+                                });
+        wrHelper->insertRefreshMap({
+                                    "1.1.0.0/24",
+                                    "SET",
+                                    {
+                                        {"ifname", "eth2"},
+                                        {"nexthop", "2.1.0.0,2.5.0.0"},
+                                        {"weight", "4"},
+                                        {"protocol", "kernel"}
+                                    }
+                                });
+        wrHelper->insertRefreshMap({
+                                    "1.2.0.0/24",
+                                    "SET",
+                                    {
+                                        {"ifname", "eth2"},
+                                        {"nexthop", "2.2.0.0"},
+                                        {"weight", "1"},
+                                        {"protocol", "kernel"}
+                                    }
+                                });
+        wrHelper->reconcile();
+        ASSERT_EQ(wrHelper->getState(), WarmStart::RECONCILED);
+
+        std::string val;
+        ASSERT_TRUE(m_routeTable->hget("1.0.0.0/24", "protocol", val));
+        ASSERT_EQ(val, "kernel");
+
+        m_routeTable->hget("1.1.0.0/24", "protocol", val);
+        ASSERT_EQ(val, "kernel");
+
+        m_routeTable->hget("1.1.0.0/24", "weight", val);
+        ASSERT_EQ(val, "4");
+
+        m_routeTable->hget("1.2.0.0/24", "protocol", val);
+        ASSERT_EQ(val, "kernel");
+    }
+}

warmrestart/warmRestartHelper.cpp

@@ -264,7 +264,7 @@ void WarmStartHelper::reconcile(void)
  * Compare all field-value-tuples within two vectors.
  *
  * Example: v1 {nexthop: 10.1.1.1, ifname: eth1}
- *          v2 {nexthop: 10.1.1.2, ifname: eth2}
+ *          v2 {nexthop: 10.1.1.2, ifname: eth2, protocol: kernel, weight: 1}
  *
  * Returns:
  *
@@ -274,25 +274,24 @@ void WarmStartHelper::reconcile(void)
 bool WarmStartHelper::compareAllFV(const std::vector<FieldValueTuple> &v1,
                                    const std::vector<FieldValueTuple> &v2)
 {
+    /* Size mismatch implies a diff */
+    if (v1.size() != v2.size())
+    {
+        return true;
+    }
+
     std::unordered_map<std::string, std::string> v1Map((v1.begin()), v1.end());
 
      /* Iterate though all v2 tuples to check if their content match v1 ones */
     for (auto &v2fv : v2)
     {
         auto v1Iter = v1Map.find(v2fv.first);
-        /*
-         * The sizes of both tuple-vectors should always match within any
-         * given application. In other words, all fields within v1 should be
-         * also present in v2.
-         *
-         * To make this possible, every application should continue relying on a
-         * uniform schema to create/generate information. For example, fpmsyncd
-         * will be always expected to push FieldValueTuples with "nexthop" and
-         * "ifname" fields; neighsyncd is expected to make use of "family" and
-         * "neigh" fields, etc. The existing reconciliation logic will rely on
-         * this assumption.
-         */
-        assert(v1Iter != v1Map.end());
+
+        /* Return true when v2 has a new field */
+        if (v1Iter == v1Map.end())
+        {
+            return true;
+        }
 
         if (compareOneFV(v1Map[fvField(*v1Iter)], fvValue(v2fv)))
         {