diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index d72b522047e..f6c6394cdb2 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -16,6 +16,8 @@ #include #include #include +#include +#include #include @@ -171,18 +173,29 @@ void TeamMgr::cleanTeamProcesses() SWSS_LOG_ENTER(); SWSS_LOG_NOTICE("Cleaning up LAGs during shutdown..."); - std::unordered_map aliasPidMap; + std::unordered_map aliasPidMap; for (const auto& alias: m_lagList) { - std::string res; pid_t pid; + // Sleep for 10 milliseconds so as to not overwhelm the netlink + // socket buffers with events about interfaces going down + std::this_thread::sleep_for(std::chrono::milliseconds(10)); try { - std::stringstream cmd; - cmd << "cat " << shellquote("/var/run/teamd/" + alias + ".pid"); - EXEC_WITH_ERROR_THROW(cmd.str(), res); + ifstream pidFile("/var/run/teamd/" + alias + ".pid"); + if (pidFile.is_open()) + { + pidFile >> pid; + aliasPidMap[alias] = pid; + SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid); + } + else + { + SWSS_LOG_NOTICE("Unable to read pid file for %s, skipping...", alias.c_str()); + continue; + } } catch (const std::exception &e) { @@ -191,32 +204,15 @@ void TeamMgr::cleanTeamProcesses() continue; } - try - { - pid = static_cast(std::stoul(res, nullptr, 10)); - aliasPidMap[alias] = pid; - - SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid); - } - catch (const std::exception &e) + if (kill(pid, SIGTERM)) { - SWSS_LOG_ERROR("Failed to read port channel %s pid: %s", alias.c_str(), e.what()); - continue; + SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, strerror(errno)); + aliasPidMap.erase(alias); } - - try + else { - std::stringstream cmd; - cmd << "kill -TERM " << pid; - EXEC_WITH_ERROR_THROW(cmd.str(), res); - SWSS_LOG_NOTICE("Sent SIGTERM to port channel %s pid %d", alias.c_str(), pid); } - catch (const std::exception &e) - { - SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, e.what()); - aliasPidMap.erase(alias); - } } for (const auto& cit: aliasPidMap) @@ -224,13 +220,12 @@ void TeamMgr::cleanTeamProcesses() const auto &alias = cit.first; const auto &pid = cit.second; - std::stringstream cmd; - std::string res; - SWSS_LOG_NOTICE("Waiting for port channel %s pid %d to stop...", alias.c_str(), pid); - cmd << "tail -f --pid=" << pid << " /dev/null"; - EXEC_WITH_ERROR_THROW(cmd.str(), res); + while (!kill(pid, 0)) + { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } } SWSS_LOG_NOTICE("LAGs cleanup is done"); @@ -658,42 +653,25 @@ bool TeamMgr::removeLag(const string &alias) { SWSS_LOG_ENTER(); - stringstream cmd; - string res; pid_t pid; - try - { - std::stringstream cmd; - cmd << "cat " << shellquote("/var/run/teamd/" + alias + ".pid"); - EXEC_WITH_ERROR_THROW(cmd.str(), res); - } - catch (const std::exception &e) { - SWSS_LOG_NOTICE("Failed to remove non-existent port channel %s pid...", alias.c_str()); - return false; - } - - try - { - pid = static_cast(std::stoul(res, nullptr, 10)); - SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid); - } - catch (const std::exception &e) - { - SWSS_LOG_ERROR("Failed to read port channel %s pid: %s", alias.c_str(), e.what()); - return false; + ifstream pidfile("/var/run/teamd/" + alias + ".pid"); + if (pidfile.is_open()) + { + pidfile >> pid; + SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid); + } + else + { + SWSS_LOG_NOTICE("Failed to remove non-existent port channel %s pid...", alias.c_str()); + return false; + } } - try - { - std::stringstream cmd; - cmd << "kill -TERM " << pid; - EXEC_WITH_ERROR_THROW(cmd.str(), res); - } - catch (const std::exception &e) + if (kill(pid, SIGTERM)) { - SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, e.what()); + SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, strerror(errno)); return false; } diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 126f4e88c5f..0bba827f732 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -225,8 +225,8 @@ tests_teammgrd_SOURCES = teammgrd/teammgr_ut.cpp \ tests_teammgrd_INCLUDES = $(tests_INCLUDES) -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/lib tests_teammgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) tests_teammgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_teammgrd_INCLUDES) -tests_teammgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ - -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main +tests_teammgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -ldl -lhiredis \ + -lswsscommon -lgtest -lgtest_main -lzmq -lpthread -lgmock -lgmock_main ## fpmsyncd unit tests diff --git a/tests/mock_tests/teammgrd/teammgr_ut.cpp b/tests/mock_tests/teammgrd/teammgr_ut.cpp index 32f064f5526..a40f39f4841 100644 --- a/tests/mock_tests/teammgrd/teammgr_ut.cpp +++ b/tests/mock_tests/teammgrd/teammgr_ut.cpp @@ -1,22 +1,128 @@ #include "gtest/gtest.h" #include "../mock_table.h" #include "teammgr.h" +#include extern int (*callback)(const std::string &cmd, std::string &stdout); extern std::vector mockCallArgs; +static std::vector< std::pair > mockKillCommands; +static std::map pidFiles; + +static int (*callback_kill)(pid_t pid, int sig) = NULL; +static std::pair (*callback_fopen)(const char *pathname, const char *mode) = NULL; + +static int cb_kill(pid_t pid, int sig) +{ + mockKillCommands.push_back(std::make_pair(pid, sig)); + if (!sig) + { + errno = ESRCH; + return -1; + } + else + { + return 0; + } +} + +int kill(pid_t pid, int sig) +{ + if (callback_kill) + { + return callback_kill(pid, sig); + } + int (*realfunc)(pid_t, int) = + (int(*)(pid_t, int))(dlsym (RTLD_NEXT, "kill")); + return realfunc(pid, sig); +} + +static std::pair cb_fopen(const char *pathname, const char *mode) +{ + auto pidFileSearch = pidFiles.find(pathname); + if (pidFileSearch != pidFiles.end()) + { + if (!pidFileSearch->second) + { + errno = ENOENT; + } + return std::make_pair(true, pidFileSearch->second); + } + else + { + return std::make_pair(false, (FILE*)NULL); + } +} + +FILE* fopen(const char *pathname, const char *mode) +{ + if (callback_fopen) + { + std::pair callback_fd = callback_fopen(pathname, mode); + if (callback_fd.first) + { + return callback_fd.second; + } + } + FILE* (*realfunc)(const char *, const char *) = + (FILE* (*)(const char *, const char *))(dlsym (RTLD_NEXT, "fopen")); + return realfunc(pathname, mode); +} + +FILE* fopen64(const char *pathname, const char *mode) +{ + if (callback_fopen) + { + std::pair callback_fd = callback_fopen(pathname, mode); + if (callback_fd.first) + { + return callback_fd.second; + } + } + FILE* (*realfunc)(const char *, const char *) = + (FILE* (*)(const char *, const char *))(dlsym (RTLD_NEXT, "fopen64")); + return realfunc(pathname, mode); +} int cb(const std::string &cmd, std::string &stdout) { mockCallArgs.push_back(cmd); - if (cmd.find("/usr/bin/teamd -r -t PortChannel1") != std::string::npos) + if (cmd.find("/usr/bin/teamd -r -t PortChannel382") != std::string::npos) { + mkdir("/var/run/teamd", 0755); + std::FILE* pidFile = std::tmpfile(); + std::fputs("1234", pidFile); + std::rewind(pidFile); + pidFiles["/var/run/teamd/PortChannel382.pid"] = pidFile; return 1; } - else if (cmd.find("cat \"/var/run/teamd/PortChannel1.pid\"") != std::string::npos) + else if (cmd.find("/usr/bin/teamd -r -t PortChannel812") != std::string::npos) { - stdout = "1234"; + pidFiles["/var/run/teamd/PortChannel812.pid"] = NULL; + return 1; + } + else if (cmd.find("/usr/bin/teamd -r -t PortChannel495") != std::string::npos) + { + mkdir("/var/run/teamd", 0755); + std::FILE* pidFile = std::tmpfile(); + std::fputs("5678", pidFile); + std::rewind(pidFile); + pidFiles["/var/run/teamd/PortChannel495.pid"] = pidFile; return 0; } + else if (cmd.find("/usr/bin/teamd -r -t PortChannel198") != std::string::npos) + { + pidFiles["/var/run/teamd/PortChannel198.pid"] = NULL; + } + else + { + for (int i = 600; i < 620; i++) + { + if (cmd.find(std::string("/usr/bin/teamd -r -t PortChannel") + std::to_string(i)) != std::string::npos) + { + pidFiles[std::string("/var/run/teamd/PortChannel") + std::to_string(i) + std::string(".pid")] = NULL; + } + } + } return 0; } @@ -53,7 +159,18 @@ namespace teammgr_ut cfg_lag_tables = tables; mockCallArgs.clear(); + mockKillCommands.clear(); + pidFiles.clear(); callback = cb; + callback_kill = cb_kill; + callback_fopen = cb_fopen; + } + + virtual void TearDown() override + { + callback = NULL; + callback_kill = NULL; + callback_fopen = NULL; } }; @@ -61,18 +178,90 @@ namespace teammgr_ut { swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); - cfg_lag_table.set("PortChannel1", { { "admin_status", "up" }, + cfg_lag_table.set("PortChannel382", { { "admin_status", "up" }, { "mtu", "9100" }, { "lacp_key", "auto" }, { "min_links", "2" } }); teammgr.addExistingData(&cfg_lag_table); teammgr.doTask(); - int kill_cmd_called = 0; - for (auto cmd : mockCallArgs){ - if (cmd.find("kill -TERM 1234") != std::string::npos){ - kill_cmd_called++; - } + ASSERT_NE(mockCallArgs.size(), 0); + EXPECT_NE(mockCallArgs.front().find("/usr/bin/teamd -r -t PortChannel382"), std::string::npos); + EXPECT_EQ(mockCallArgs.size(), 1); + EXPECT_EQ(mockKillCommands.size(), 1); + EXPECT_EQ(mockKillCommands.front().first, 1234); + EXPECT_EQ(mockKillCommands.front().second, SIGTERM); + } + + TEST_F(TeamMgrTest, testProcessPidFileMissingAfterAddLagFailure) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + cfg_lag_table.set("PortChannel812", { { "admin_status", "up" }, + { "mtu", "9100" }, + { "fallback", "true" }, + { "lacp_key", "auto" }, + { "min_links", "1" } }); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + ASSERT_NE(mockCallArgs.size(), 0); + EXPECT_NE(mockCallArgs.front().find("/usr/bin/teamd -r -t PortChannel812"), std::string::npos); + EXPECT_EQ(mockCallArgs.size(), 1); + EXPECT_EQ(mockKillCommands.size(), 0); + } + + TEST_F(TeamMgrTest, testProcessCleanupAfterAddLag) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + cfg_lag_table.set("PortChannel495", { { "admin_status", "up" }, + { "mtu", "9100" }, + { "lacp_key", "auto" }, + { "min_links", "2" } }); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + ASSERT_EQ(mockCallArgs.size(), 3); + ASSERT_NE(mockCallArgs.front().find("/usr/bin/teamd -r -t PortChannel495"), std::string::npos); + teammgr.cleanTeamProcesses(); + EXPECT_EQ(mockKillCommands.size(), 2); + EXPECT_EQ(mockKillCommands.front().first, 5678); + EXPECT_EQ(mockKillCommands.front().second, SIGTERM); + } + + TEST_F(TeamMgrTest, testProcessPidFileMissingDuringCleanup) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + cfg_lag_table.set("PortChannel198", { { "admin_status", "up" }, + { "mtu", "9100" }, + { "fallback", "true" }, + { "lacp_key", "auto" }, + { "min_links", "1" } }); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + ASSERT_NE(mockCallArgs.size(), 0); + EXPECT_NE(mockCallArgs.front().find("/usr/bin/teamd -r -t PortChannel198"), std::string::npos); + EXPECT_EQ(mockCallArgs.size(), 3); + teammgr.cleanTeamProcesses(); + EXPECT_EQ(mockKillCommands.size(), 0); + } + + TEST_F(TeamMgrTest, testSleepDuringCleanup) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + for (int i = 600; i < 620; i++) + { + cfg_lag_table.set(std::string("PortChannel") + std::to_string(i), { { "admin_status", "up" }, + { "mtu", "9100" }, + { "lacp_key", "auto" } }); } - ASSERT_EQ(kill_cmd_called, 1); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + ASSERT_EQ(mockCallArgs.size(), 60); + std::chrono::steady_clock::time_point begin = std::chrono::steady_clock::now(); + teammgr.cleanTeamProcesses(); + std::chrono::steady_clock::time_point end = std::chrono::steady_clock::now(); + EXPECT_EQ(mockKillCommands.size(), 0); + EXPECT_GE(std::chrono::duration_cast(end - begin).count(), 200); } -} \ No newline at end of file +}