Fix issue #13341 ARP entry can be out of sync between kernel and APPL_DB if multiple updates are received from RTNL

tests/mock_tests/Makefile.am

@@ -21,7 +21,7 @@ LDADD_GTEST = -L/usr/src/gtest
 
 ## Orchagent Unit Tests
 
-tests_INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/orchagent -I$(P4_ORCH_DIR)/tests
+tests_INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/orchagent -I$(P4_ORCH_DIR)/tests -I$(top_srcdir)/warmrestart
 
 tests_SOURCES = aclorch_ut.cpp \
                 portsorch_ut.cpp \
@@ -49,6 +49,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 swssnet_ut.cpp \
                 flowcounterrouteorch_ut.cpp \
                 orchdaemon_ut.cpp \
+                warmrestartassist_ut.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \
                 $(top_srcdir)/lib/subintf.cpp \
                 $(top_srcdir)/orchagent/orchdaemon.cpp \
@@ -105,7 +106,8 @@ tests_SOURCES = aclorch_ut.cpp \
                 $(top_srcdir)/orchagent/srv6orch.cpp \
                 $(top_srcdir)/orchagent/nvgreorch.cpp \
                 $(top_srcdir)/cfgmgr/portmgr.cpp \
-                $(top_srcdir)/cfgmgr/buffermgrdyn.cpp
+                $(top_srcdir)/cfgmgr/buffermgrdyn.cpp \
+                $(top_srcdir)/warmrestart/warmRestartAssist.cpp
 
 tests_SOURCES += $(FLEX_CTR_DIR)/flex_counter_manager.cpp $(FLEX_CTR_DIR)/flex_counter_stat_manager.cpp $(FLEX_CTR_DIR)/flow_counter_handler.cpp $(FLEX_CTR_DIR)/flowcounterrouteorch.cpp
 tests_SOURCES += $(DEBUG_CTR_DIR)/debug_counter.cpp $(DEBUG_CTR_DIR)/drop_counter.cpp

tests/mock_tests/warmrestartassist_ut.cpp

@@ -0,0 +1,64 @@
+#define protected public
+#include "orch.h"
+#undef protected
+#include "ut_helper.h"
+//#include "mock_orchagent_main.h"
+#include "mock_table.h"
+#include "warm_restart.h"
+#define private public
+#include "warmRestartAssist.h"
+#undef private
+
+#define APP_WRA_TEST_TABLE_NAME "TEST_TABLE"
+
+namespace warmrestartassist_test
+{
+    using namespace std;
+
+    shared_ptr<swss::DBConnector> m_app_db = make_shared<swss::DBConnector>("APPL_DB", 0);
+    shared_ptr<swss::RedisPipeline> m_app_db_pipeline = make_shared<swss::RedisPipeline>(m_app_db.get());
+    shared_ptr<swss::ProducerStateTable> m_wra_test_table = make_shared<swss::ProducerStateTable>(m_app_db.get(), APP_WRA_TEST_TABLE_NAME);
+
+    AppRestartAssist *appRestartAssist;
+
+    struct WarmrestartassistTest : public ::testing::Test
+    {
+        WarmrestartassistTest()
+        {
+            appRestartAssist = new AppRestartAssist(m_app_db_pipeline.get(), "testsyncd", "swss", 0);
+            appRestartAssist->m_warmStartInProgress = true;
+            appRestartAssist->registerAppTable(APP_WRA_TEST_TABLE_NAME, m_wra_test_table.get());
+        }
+
+        void SetUp() override
+        {
+            testing_db::reset();
+
+            Table testTable = Table(m_app_db.get(), APP_WRA_TEST_TABLE_NAME);
+            testTable.set("key",
+                          {
+                              {"field", "value0"},
+                          });
+        }
+
+        void TearDown() override
+        {
+        }
+    };
+
+    TEST_F(WarmrestartassistTest, warmRestartAssistTest)
+    {
+        appRestartAssist->readTablesToMap();
+        vector<FieldValueTuple> fvVector;
+        fvVector.emplace_back("field", "value1");
+        appRestartAssist->insertToMap(APP_WRA_TEST_TABLE_NAME, "key", fvVector, false);
+        appRestartAssist->insertToMap(APP_WRA_TEST_TABLE_NAME, "key", fvVector, false);
+        appRestartAssist->reconcile();
+
+        fvVector.clear();
+        Table testTable = Table(m_app_db.get(), APP_WRA_TEST_TABLE_NAME);
+        ASSERT_TRUE(testTable.get("key", fvVector));
+        ASSERT_EQ(fvField(fvVector[0]), "field");
+        ASSERT_EQ(fvValue(fvVector[0]), "value1");
+    }
+}

warmrestart/warmRestartAssist.cpp

@@ -208,10 +208,31 @@ void AppRestartAssist::insertToMap(string tableName, string key, vector<FieldVal
         }
         else
         {
-            SWSS_LOG_INFO("%s, found key: %s, same value", tableName.c_str(), key.c_str());
-
-            // mark as SAME flag
-            setCacheEntryState(found->second, SAME);
+            auto state = getCacheEntryState(found->second);
+            /*
+             * In case an entry has been updated for more than once with the same value but different from the stored one,
+             * keep the state as NEW.
+             * Eg.
+             * Assume the entry's value that is restored from last warm reboot is V0.
+             * 1. The first update with value V1 is received and handled by the above `if (found != appTableCacheMap[tableName].end())` branch,
+             *    - state is set to NEW
+             *    - value is updated to V1
+             * 2. The second update with the same value V1 is received and handled by this branch
+             *    - Originally, state was set to SAME, which is wrong because V1 is different from the stored value V0
+             *    - The correct logic should be: set the state to same only if the state is not NEW
+             * This is a very rare case because in most of times the entry won't be updated for multiple times
+             */
+            if (state == NEW)
+            {
+                SWSS_LOG_NOTICE("%s, found key: %s, it has been updated for the second time, keep state as NEW",
+                                tableName.c_str(), key.c_str());
+            }
+            else
+            {
+                SWSS_LOG_INFO("%s, found key: %s, same value", tableName.c_str(), key.c_str());
+                // mark as SAME flag
+                setCacheEntryState(found->second, SAME);
+            }
         }
     }
     else