From 24d29f184997b9d87aed3b0d4c30d45c7d84c72d Mon Sep 17 00:00:00 2001 From: Bryan Crossland 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 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 +#include +#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 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(); + } +}