From 24d29f184997b9d87aed3b0d4c30d45c7d84c72d Mon Sep 17 00:00:00 2001
From: Bryan Crossland <bacrossland@gmail.com>
Date: Mon, 3 Oct 2022 00:56:45 -0500
Subject: [PATCH] [orchdaemon]: Fixed sairedis record file rotation (#2299)
* [orchdaemon]: Fixed sairedis record file rotation
* What I did
Fix Azure/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
---
orchagent/main.cpp | 1 -
orchagent/orchdaemon.cpp | 34 +++++++++++-----
orchagent/orchdaemon.h | 3 ++
orchagent/p4orch/tests/test_main.cpp | 1 -
tests/mock_tests/Makefile.am | 8 ++--
tests/mock_tests/mock_orchagent_main.cpp | 1 -
tests/mock_tests/mock_orchagent_main.h | 1 -
tests/mock_tests/orchdaemon_ut.cpp | 52 ++++++++++++++++++++++++
8 files changed, 84 insertions(+), 17 deletions(-)
create mode 100644 tests/mock_tests/orchdaemon_ut.cpp
diff --git a/orchagent/main.cpp b/orchagent/main.cpp
index 5a074450dabb..2e140d5893b2 100644
--- 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
index 68032d16893c..cd6c94c213b3 100644
--- a/orchagent/orchdaemon.cpp
+++ b/orchagent/orchdaemon.cpp
@@ -57,6 +57,7 @@ FlowCounterRouteOrch *gFlowCounterRouteOrch;
DebugCounterOrch *gDebugCounterOrch;
bool gIsNatSupported = false;
+bool gSaiRedisLogRotate = false;
event_handle_t g_events_handle;
#define DEFAULT_MAX_BULK_SIZE 1000
@@ -676,24 +677,26 @@ 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)
{
@@ -737,6 +740,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
index def4b7862940..998e72335a2e 100644
--- a/orchagent/orchdaemon.h
+++ b/orchagent/orchdaemon.h
@@ -45,8 +45,10 @@
#include "bfdorch.h"
#include "srv6orch.h"
#include "nvgreorch.h"
+#include <sairedis.h>
using namespace swss;
+extern bool gSaiRedisLogRotate;
class OrchDaemon
{
@@ -67,6 +69,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
index 203344e434c3..e1edb3584cde 100644
--- a/orchagent/p4orch/tests/test_main.cpp
+++ b/orchagent/p4orch/tests/test_main.cpp
@@ -43,7 +43,6 @@ size_t gMaxBulkSize = DEFAULT_MAX_BULK_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
index 6ee6faef460e..51b694554b05 100644
--- a/tests/mock_tests/Makefile.am
+++ b/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
+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_SOURCES = aclorch_ut.cpp \
portsorch_ut.cpp \
@@ -47,6 +47,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 \
@@ -120,12 +121,13 @@ tests_SOURCES += $(P4_ORCH_DIR)/p4orch.cpp \
$(P4_ORCH_DIR)/wcmp_manager.cpp \
$(P4_ORCH_DIR)/mirror_session_manager.cpp \
$(P4_ORCH_DIR)/gre_tunnel_manager.cpp \
- $(P4_ORCH_DIR)/l3_admit_manager.cpp
+ $(P4_ORCH_DIR)/l3_admit_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) $(tests_INCLUDES)
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
## portsyncd unit tests
diff --git a/tests/mock_tests/mock_orchagent_main.cpp b/tests/mock_tests/mock_orchagent_main.cpp
index 62a03dc7700c..ff2f902f3963 100644
--- 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
index 0acba4ef1c20..f378a53de867 100644
--- 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
new file mode 100644
index 000000000000..ab3e4a226cb6
--- /dev/null
+++ 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();
+ }
+}