What I did

Fix sonic-net/sonic-buildimage#8162 Moved sairedis record file rotation logic out of flush() to fix issue. Why I did it Sairedis record file was not releasing the file handle on rotation. This is because the file handle release was inside the flush() which was only being called if a select timeout was triggered. Moved the logic to its own function which is called in the start() loop. How I verified it Ran a script to fill log and verified that rotation was happening correctly. Signed-off-by: Bryan Crossland bryan.crossland@target.com
target · Oct 4, 2022 · 6dc03d3 · 6dc03d3
1 parent 2f7da1e
commit 6dc03d3
Show file tree

Hide file tree

Showing 8 changed files with 82 additions and 17 deletions.
diff --git a/orchagent/main.cpp b/orchagent/main.cpp
@@ -58,7 +58,6 @@ bool gSairedisRecord = true;
 bool gSwssRecord = true;
 bool gResponsePublisherRecord = false;
 bool gLogRotate = false;
-bool gSaiRedisLogRotate = false;
 bool gResponsePublisherLogRotate = false;
 bool gSyncMode = false;
 sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC;

diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp
@@ -57,6 +57,7 @@ FlowCounterRouteOrch *gFlowCounterRouteOrch;
 DebugCounterOrch *gDebugCounterOrch;
 
 bool gIsNatSupported = false;
+bool gSaiRedisLogRotate = false;
 
 #define DEFAULT_MAX_BULK_SIZE 1000
 size_t gMaxBulkSize = DEFAULT_MAX_BULK_SIZE;
@@ -671,24 +672,25 @@ void OrchDaemon::flush()
         SWSS_LOG_ERROR("Failed to flush redis pipeline %d", status);
         abort();
     }
+}
 
-    // check if logroate is requested
-    if (gSaiRedisLogRotate)
+/* Release the file handle so the log can be rotated */
+void OrchDaemon::logRotate() {
+    SWSS_LOG_ENTER();
+    sai_attribute_t attr;
+    attr.id = SAI_REDIS_SWITCH_ATTR_PERFORM_LOG_ROTATE;
+    attr.value.booldata = true;
+    sai_status_t status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);
+    if (status != SAI_STATUS_SUCCESS)
     {
-        SWSS_LOG_NOTICE("performing log rotate");
-
-        gSaiRedisLogRotate = false;
-
-        attr.id = SAI_REDIS_SWITCH_ATTR_PERFORM_LOG_ROTATE;
-        attr.value.booldata = true;
-
-        sai_switch_api->set_switch_attribute(gSwitchId, &attr);
+        SWSS_LOG_ERROR("Failed to release the file handle on sairedis log %d", status);
     }
 }
 
 void OrchDaemon::start()
 {
     SWSS_LOG_ENTER();
+    gSaiRedisLogRotate = false;
 
     for (Orch *o : m_orchList)
     {
@@ -732,6 +734,17 @@ void OrchDaemon::start()
             continue;
         }
 
+        // check if logroate is requested
+        if (gSaiRedisLogRotate)
+        {
+            SWSS_LOG_NOTICE("performing log rotate");
+
+            gSaiRedisLogRotate = false;
+
+            logRotate();
+            continue;
+        }
+
         auto *c = (Executor *)s;
         c->execute();
 

diff --git a/orchagent/orchdaemon.h b/orchagent/orchdaemon.h
@@ -45,6 +45,7 @@
 #include "bfdorch.h"
 #include "srv6orch.h"
 #include "nvgreorch.h"
+#include <sairedis.h>
 
 using namespace swss;
 
@@ -67,6 +68,7 @@ class OrchDaemon
     {
         m_fabricEnabled = enabled;
     }
+    void logRotate();
 private:
     DBConnector *m_applDb;
     DBConnector *m_configDb;

diff --git a/orchagent/p4orch/tests/test_main.cpp b/orchagent/p4orch/tests/test_main.cpp
@@ -40,7 +40,6 @@ int gBatchSize = DEFAULT_BATCH_SIZE;
 bool gSairedisRecord = true;
 bool gSwssRecord = true;
 bool gLogRotate = false;
-bool gSaiRedisLogRotate = false;
 bool gResponsePublisherRecord = false;
 bool gResponsePublisherLogRotate = false;
 bool gSyncMode = false;

diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am
@@ -2,7 +2,7 @@ FLEX_CTR_DIR = $(top_srcdir)/orchagent/flex_counter
 DEBUG_CTR_DIR = $(top_srcdir)/orchagent/debug_counter
 P4_ORCH_DIR = $(top_srcdir)/orchagent/p4orch
 
-INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib -I $(top_srcdir)/cfgmgr
+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
 
 CFLAGS_SAI = -I /usr/include/sai
 
@@ -44,6 +44,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 fake_response_publisher.cpp \
                 swssnet_ut.cpp \
                 flowcounterrouteorch_ut.cpp \
+                orchdaemon_ut.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \
                 $(top_srcdir)/lib/subintf.cpp \
                 $(top_srcdir)/orchagent/orchdaemon.cpp \
@@ -115,12 +116,13 @@ tests_SOURCES += $(P4_ORCH_DIR)/p4orch.cpp \
 		 $(P4_ORCH_DIR)/acl_table_manager.cpp \
 		 $(P4_ORCH_DIR)/acl_rule_manager.cpp \
 		 $(P4_ORCH_DIR)/wcmp_manager.cpp \
-		 $(P4_ORCH_DIR)/mirror_session_manager.cpp
+		 $(P4_ORCH_DIR)/mirror_session_manager.cpp \
+         $(P4_ORCH_DIR)/tests/mock_sai_switch.cpp
 
 tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
 tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I$(top_srcdir)/orchagent
 tests_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis -lpthread \
-        -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3
+        -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lgmock -lgmock_main
 
 ## intfmgrd unit tests
 

diff --git a/tests/mock_tests/mock_orchagent_main.cpp b/tests/mock_tests/mock_orchagent_main.cpp
@@ -18,7 +18,6 @@ int gBatchSize = DEFAULT_BATCH_SIZE;
 bool gSairedisRecord = true;
 bool gSwssRecord = true;
 bool gLogRotate = false;
-bool gSaiRedisLogRotate = false;
 ofstream gRecordOfs;
 string gRecordFile;
 string gMySwitchType = "switch";

diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h
@@ -30,7 +30,6 @@ extern int gBatchSize;
 extern bool gSwssRecord;
 extern bool gSairedisRecord;
 extern bool gLogRotate;
-extern bool gSaiRedisLogRotate;
 extern ofstream gRecordOfs;
 extern string gRecordFile;
 

diff --git a/tests/mock_tests/orchdaemon_ut.cpp b/tests/mock_tests/orchdaemon_ut.cpp
@@ -0,0 +1,52 @@
+#include "orchdaemon.h"
+#include "dbconnector.h"
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+#include "mock_sai_switch.h"
+
+extern sai_switch_api_t* sai_switch_api;
+sai_switch_api_t test_sai_switch;
+
+namespace orchdaemon_test
+{
+
+    using ::testing::_;
+    using ::testing::Return;
+    using ::testing::StrictMock;
+
+    DBConnector appl_db("APPL_DB", 0);
+    DBConnector state_db("STATE_DB", 0);
+    DBConnector config_db("CONFIG_DB", 0);
+    DBConnector counters_db("COUNTERS_DB", 0);
+
+    class OrchDaemonTest : public ::testing::Test
+    {
+    public:
+        StrictMock<MockSaiSwitch> mock_sai_switch_;
+
+        OrchDaemon* orchd;
+
+        OrchDaemonTest()
+        {
+            mock_sai_switch = &mock_sai_switch_;
+            sai_switch_api = &test_sai_switch;
+            sai_switch_api->get_switch_attribute = &mock_get_switch_attribute;
+            sai_switch_api->set_switch_attribute = &mock_set_switch_attribute;
+
+            orchd = new OrchDaemon(&appl_db, &config_db, &state_db, &counters_db);
+
+        };
+
+        ~OrchDaemonTest()
+        {
+
+        };
+    };
+
+    TEST_F(OrchDaemonTest, logRotate)
+{
+    EXPECT_CALL(mock_sai_switch_, set_switch_attribute( _, _)).WillOnce(Return(SAI_STATUS_SUCCESS));
+
+    orchd->logRotate();
+}
+}