From 25544559a7d442e9d9885a6c387272d95bdbbc51 Mon Sep 17 00:00:00 2001 From: EdenGri Date: Tue, 7 Feb 2023 09:32:49 +0000 Subject: [PATCH 1/3] Fixed set admin_status for deleted subintf due to late notification --- cfgmgr/intfmgr.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index f0f4ec97fa7..bdfd8ce46c5 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -491,13 +491,24 @@ void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin) std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string &admin_status, const string &parent_admin_status) { stringstream cmd; - string res; + string res, cmd_str; if (parent_admin_status == "up" || admin_status == "down") { SWSS_LOG_INFO("subintf %s admin_status: %s", alias.c_str(), admin_status.c_str()); cmd << IP_CMD " link set " << shellquote(alias) << " " << shellquote(admin_status); - EXEC_WITH_ERROR_THROW(cmd.str(), res); + cmd_str = cmd.str(); + int ret = swss::exec(cmd_str, res); + if (ret && !isIntfStateOk(alias)) + { + // Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notification + SWSS_LOG_WARN("Setting admin_status to %s netdev failed with cmd:%s, rc:%d, error:%s", + alias.c_str(), cmd_str.c_str(), ret, res.c_str()); + } + else if (ret) + { + throw runtime_error(cmd_str + " : " + res); + } return admin_status; } else From e8e0f67c1061586ddbb24c8045bf3fe16da52854 Mon Sep 17 00:00:00 2001 From: EdenGri Date: Thu, 9 Mar 2023 08:06:47 +0000 Subject: [PATCH 2/3] add unit tests --- .../intfmgrd/add_ipv6_prefix_ut.cpp | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp b/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp index 2ed1a1b6af7..1bb8c49d3c0 100644 --- a/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp +++ b/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp @@ -26,6 +26,15 @@ int cb(const std::string &cmd, std::string &stdout){ return 0; } + +int cb2(const std::string &cmd, std::string &stdout){ + if (cmd == "/sbin/ip link set \"Ethernet64.10\" \"up\""){ + return 1; + } + return 0; +} + + // Test Fixture namespace add_ipv6_prefix_ut { @@ -106,4 +115,20 @@ namespace add_ipv6_prefix_ut } ASSERT_EQ(ip_cmd_called, 1); } + + TEST_F(IntfMgrTest, testEden){ + callback = cb2; + swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); + intfmgr.setHostSubIntfAdminStatus("Ethernet64.10", "up", "up"); + } + + TEST_F(IntfMgrTest, testEden2){ + callback = cb2; + swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); + /* Set portStateTable */ + std::vector values; + values.emplace_back("state", "ok"); + intfmgr.m_statePortTable.set("Ethernet64.10", values, "SET", ""); + EXPECT_THROW(intfmgr.setHostSubIntfAdminStatus("Ethernet64.10", "up", "up"), std::runtime_error); + } } From 79335fbc67aa801cb8596b4857ea05aad68dee02 Mon Sep 17 00:00:00 2001 From: EdenGri Date: Mon, 13 Mar 2023 11:25:22 +0000 Subject: [PATCH 3/3] Add unit tests, change the file name from add_ipv6_prefix_ut.cpp to intfmgr_ut.cpp --- tests/mock_tests/Makefile.am | 2 +- ...{add_ipv6_prefix_ut.cpp => intfmgr_ut.cpp} | 32 ++++++++----------- 2 files changed, 15 insertions(+), 19 deletions(-) rename tests/mock_tests/intfmgrd/{add_ipv6_prefix_ut.cpp => intfmgr_ut.cpp} (89%) diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 685a30b53a4..e0b429d9c74 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -153,7 +153,7 @@ tests_portsyncd_LDADD = $(LDADD_GTEST) -lnl-genl-3 -lhiredis -lhiredis \ ## intfmgrd unit tests -tests_intfmgrd_SOURCES = intfmgrd/add_ipv6_prefix_ut.cpp \ +tests_intfmgrd_SOURCES = intfmgrd/intfmgr_ut.cpp \ $(top_srcdir)/cfgmgr/intfmgr.cpp \ $(top_srcdir)/lib/subintf.cpp \ $(top_srcdir)/orchagent/orch.cpp \ diff --git a/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp b/tests/mock_tests/intfmgrd/intfmgr_ut.cpp similarity index 89% rename from tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp rename to tests/mock_tests/intfmgrd/intfmgr_ut.cpp index 1bb8c49d3c0..ef43cdeb6be 100644 --- a/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp +++ b/tests/mock_tests/intfmgrd/intfmgr_ut.cpp @@ -1,6 +1,6 @@ #include "gtest/gtest.h" #include -#include +#include #include #include #include "../mock_table.h" @@ -20,23 +20,17 @@ int cb(const std::string &cmd, std::string &stdout){ else if (cmd.find("/sbin/ip -6 address \"add\"") == 0) { return Ethernet0IPv6Set ? 0 : 2; } + else if (cmd == "/sbin/ip link set \"Ethernet64.10\" \"up\""){ + return 1; + } else { return 0; } return 0; } - -int cb2(const std::string &cmd, std::string &stdout){ - if (cmd == "/sbin/ip link set \"Ethernet64.10\" \"up\""){ - return 1; - } - return 0; -} - - // Test Fixture -namespace add_ipv6_prefix_ut +namespace intfmgr_ut { struct IntfMgrTest : public ::testing::Test { @@ -44,14 +38,14 @@ namespace add_ipv6_prefix_ut std::shared_ptr m_app_db; std::shared_ptr m_state_db; std::vector cfg_intf_tables; - + virtual void SetUp() override - { + { testing_db::reset(); m_config_db = std::make_shared("CONFIG_DB", 0); m_app_db = std::make_shared("APPL_DB", 0); m_state_db = std::make_shared("STATE_DB", 0); - + swss::WarmStart::initialize("intfmgrd", "swss"); std::vector tables = { @@ -116,14 +110,16 @@ namespace add_ipv6_prefix_ut ASSERT_EQ(ip_cmd_called, 1); } - TEST_F(IntfMgrTest, testEden){ - callback = cb2; + //This test except no runtime error when the set admin status command failed + //and the subinterface has not ok status (for example not existing subinterface) + TEST_F(IntfMgrTest, testSetAdminStatusFailToNotOkSubInt){ swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); intfmgr.setHostSubIntfAdminStatus("Ethernet64.10", "up", "up"); } - TEST_F(IntfMgrTest, testEden2){ - callback = cb2; + //This test except runtime error when the set admin status command failed + //and the subinterface has ok status + TEST_F(IntfMgrTest, testSetAdminStatusFailToOkSubInt){ swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); /* Set portStateTable */ std::vector values;