From 569c8b1a64c4a1a6284941bbfd9447984f6c0bc1 Mon Sep 17 00:00:00 2001 From: Bryan Crossland Date: Mon, 3 Oct 2022 22:35:11 -0500 Subject: [PATCH 1/5] 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 +++ 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/orchagent/main.cpp b/orchagent/main.cpp index a4f30a9f2d..119d8fb4b7 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -55,7 +55,6 @@ int gBatchSize = DEFAULT_BATCH_SIZE; bool gSairedisRecord = true; bool gSwssRecord = true; bool gLogRotate = false; -bool gSaiRedisLogRotate = false; bool gSyncMode = false; sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC; string gAsicInstance; diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index dad916f1a9..3ab58e4fa6 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -43,6 +43,7 @@ BfdOrch *gBfdOrch; QosOrch *gQosOrch; bool gIsNatSupported = false; +bool gSaiRedisLogRotate = false; #define DEFAULT_MAX_BULK_SIZE 1000 size_t gMaxBulkSize = DEFAULT_MAX_BULK_SIZE; @@ -529,24 +530,24 @@ void OrchDaemon::flush() SWSS_LOG_ERROR("Failed to flush redis pipeline %d", status); exit(EXIT_FAILURE); } +} - // check if logroate is requested - if (gSaiRedisLogRotate) - { - 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); +/* 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_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) { @@ -590,6 +591,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 725c94be85..d6dcb66b37 100644 --- a/orchagent/orchdaemon.h +++ b/orchagent/orchdaemon.h @@ -33,8 +33,10 @@ #include "natorch.h" #include "muxorch.h" #include "bfdorch.h" +#include using namespace swss; +extern bool gSaiRedisLogRotate; class OrchDaemon { @@ -49,6 +51,7 @@ class OrchDaemon bool warmRestoreValidation(); bool warmRestartCheck(); + void logRotate(); private: DBConnector *m_applDb; DBConnector *m_configDb; From caa0fb9a96ac69269a4414b2505925a86af99578 Mon Sep 17 00:00:00 2001 From: Bryan Crossland Date: Tue, 4 Oct 2022 14:26:17 -0500 Subject: [PATCH 2/5] [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 --- tests/mock_tests/Makefile.am | 6 ++-- tests/mock_tests/mock_sai_switch.cpp | 14 ++++++++ tests/mock_tests/mock_sai_switch.h | 24 +++++++++++++ tests/mock_tests/orchdaemon_ut.cpp | 52 ++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 tests/mock_tests/mock_sai_switch.cpp create mode 100644 tests/mock_tests/mock_sai_switch.h create mode 100644 tests/mock_tests/orchdaemon_ut.cpp diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 75d92ea947..9d3488544b 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -1,7 +1,7 @@ FLEX_CTR_DIR = $(top_srcdir)/orchagent/flex_counter DEBUG_CTR_DIR = $(top_srcdir)/orchagent/debug_counter -INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib +INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib -I$(top_srcdir)/orchagent CFLAGS_SAI = -I /usr/include/sai @@ -33,6 +33,8 @@ tests_SOURCES = aclorch_ut.cpp \ mock_hiredis.cpp \ mock_redisreply.cpp \ bulker_ut.cpp \ + mock_sai_switch.cpp \ + orchdaemon_ut.cpp \ $(top_srcdir)/lib/gearboxutils.cpp \ $(top_srcdir)/orchagent/orchdaemon.cpp \ $(top_srcdir)/orchagent/orch.cpp \ @@ -76,4 +78,4 @@ tests_SOURCES += $(DEBUG_CTR_DIR)/debug_counter.cpp $(DEBUG_CTR_DIR)/drop_counte 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 diff --git a/tests/mock_tests/mock_sai_switch.cpp b/tests/mock_tests/mock_sai_switch.cpp new file mode 100644 index 0000000000..bdb0636bae --- /dev/null +++ b/tests/mock_tests/mock_sai_switch.cpp @@ -0,0 +1,14 @@ +#include "mock_sai_switch.h" + +MockSaiSwitch *mock_sai_switch; + +sai_status_t mock_get_switch_attribute(_In_ sai_object_id_t switch_id, _In_ uint32_t attr_count, + _Inout_ sai_attribute_t *attr_list) +{ +return mock_sai_switch->get_switch_attribute(switch_id, attr_count, attr_list); +} + +sai_status_t mock_set_switch_attribute(_In_ sai_object_id_t switch_id, _In_ const sai_attribute_t *attr) +{ +return mock_sai_switch->set_switch_attribute(switch_id, attr); +} diff --git a/tests/mock_tests/mock_sai_switch.h b/tests/mock_tests/mock_sai_switch.h new file mode 100644 index 0000000000..46bbfc3a64 --- /dev/null +++ b/tests/mock_tests/mock_sai_switch.h @@ -0,0 +1,24 @@ +#pragma once + +#include + +extern "C" +{ +#include "sai.h" +} + +// Mock Class mapping methods to switch object SAI APIs. +class MockSaiSwitch +{ +public: + MOCK_METHOD3(get_switch_attribute, sai_status_t(_In_ sai_object_id_t switch_id, _In_ uint32_t attr_count, + _Inout_ sai_attribute_t *attr_list)); + MOCK_METHOD2(set_switch_attribute, sai_status_t(_In_ sai_object_id_t switch_id, _In_ const sai_attribute_t *attr)); +}; + +extern MockSaiSwitch *mock_sai_switch; + +sai_status_t mock_get_switch_attribute(_In_ sai_object_id_t switch_id, _In_ uint32_t attr_count, + _Inout_ sai_attribute_t *attr_list); + +sai_status_t mock_set_switch_attribute(_In_ sai_object_id_t switch_id, _In_ const sai_attribute_t *attr); diff --git a/tests/mock_tests/orchdaemon_ut.cpp b/tests/mock_tests/orchdaemon_ut.cpp new file mode 100644 index 0000000000..cf51476a7c --- /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(); + } +} From e16da84fd25b41744307c204b324c24c0434cb09 Mon Sep 17 00:00:00 2001 From: Bryan Crossland Date: Tue, 4 Oct 2022 14:56:39 -0500 Subject: [PATCH 3/5] [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 --- tests/mock_tests/orchdaemon_ut.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/mock_tests/orchdaemon_ut.cpp b/tests/mock_tests/orchdaemon_ut.cpp index cf51476a7c..381518ea59 100644 --- a/tests/mock_tests/orchdaemon_ut.cpp +++ b/tests/mock_tests/orchdaemon_ut.cpp @@ -17,7 +17,6 @@ namespace orchdaemon_test 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 { @@ -33,7 +32,7 @@ namespace orchdaemon_test 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); + orchd = new OrchDaemon(&appl_db, &config_db, &state_db); }; From 7be5496d86bacecfbb7f9b0e14ba6d79b0fe7002 Mon Sep 17 00:00:00 2001 From: Bryan Crossland Date: Tue, 4 Oct 2022 17:22:54 -0500 Subject: [PATCH 4/5] [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 --- tests/mock_tests/mock_orchagent_main.h | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h index 08121860df..3a3912784e 100644 --- a/tests/mock_tests/mock_orchagent_main.h +++ b/tests/mock_tests/mock_orchagent_main.h @@ -24,7 +24,6 @@ extern int gBatchSize; extern bool gSwssRecord; extern bool gSairedisRecord; extern bool gLogRotate; -extern bool gSaiRedisLogRotate; extern ofstream gRecordOfs; extern string gRecordFile; From b46abb86ff798ff16b5453455ce4baf585ea22f8 Mon Sep 17 00:00:00 2001 From: Bryan Crossland Date: Tue, 4 Oct 2022 17:24:20 -0500 Subject: [PATCH 5/5] [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 --- tests/mock_tests/mock_orchagent_main.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/mock_tests/mock_orchagent_main.cpp b/tests/mock_tests/mock_orchagent_main.cpp index 15004b756e..69705b44d4 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;