diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index b385a5096a..38c0418a7a 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -31,29 +31,9 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) // Set the port MTU in application database to update both // the port MTU and possibly the port based router interface MTU - vector fvs; - FieldValueTuple fv("mtu", mtu); - fvs.push_back(fv); - m_appPortTable.set(alias, fvs); - - return true; + return writeConfigToAppDb(alias, "mtu", mtu); } -bool PortMgr::setPortTpid(const string &alias, const string &tpid) -{ - stringstream cmd; - string res; - - // Set the port TPID in application database to update port TPID - vector fvs; - FieldValueTuple fv("tpid", tpid); - fvs.push_back(fv); - m_appPortTable.set(alias, fvs); - - return true; -} - - bool PortMgr::setPortAdminStatus(const string &alias, const bool up) { stringstream cmd; @@ -63,23 +43,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up) cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down"); EXEC_WITH_ERROR_THROW(cmd.str(), res); - vector fvs; - FieldValueTuple fv("admin_status", (up ? "up" : "down")); - fvs.push_back(fv); - m_appPortTable.set(alias, fvs); - - return true; -} - -bool PortMgr::setPortLearnMode(const string &alias, const string &learn_mode) -{ - // Set the port MAC learn mode in application database - vector fvs; - FieldValueTuple fv("learn_mode", learn_mode); - fvs.push_back(fv); - m_appPortTable.set(alias, fvs); - - return true; + return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down")); } bool PortMgr::isPortStateOk(const string &alias) @@ -117,14 +81,14 @@ void PortMgr::doTask(Consumer &consumer) if (op == SET_COMMAND) { - if (!isPortStateOk(alias)) - { - SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str()); - it++; - continue; - } + /* portOk=true indicates that the port has been created in kernel. + * We should not call any ip command if portOk=false. However, it is + * valid to put port configuration to APP DB which will trigger port creation in kernel. + */ + bool portOk = isPortStateOk(alias); - string admin_status, mtu, learn_mode, tpid; + string admin_status, mtu; + std::vector field_values; bool configured = (m_portList.find(alias) != m_portList.end()); @@ -138,6 +102,11 @@ void PortMgr::doTask(Consumer &consumer) m_portList.insert(alias); } + else if (!portOk) + { + it++; + continue; + } for (auto i : kfvFieldsValues(t)) { @@ -149,38 +118,42 @@ void PortMgr::doTask(Consumer &consumer) { admin_status = fvValue(i); } - else if (fvField(i) == "learn_mode") - { - learn_mode = fvValue(i); - } - else if (fvField(i) == "tpid") + else { - tpid = fvValue(i); + field_values.emplace_back(i); } } - if (!mtu.empty()) + for (auto &entry : field_values) { - setPortMtu(alias, mtu); - SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str()); + writeConfigToAppDb(alias, fvField(entry), fvValue(entry)); + SWSS_LOG_NOTICE("Configure %s %s to %s", alias.c_str(), fvField(entry).c_str(), fvValue(entry).c_str()); } - if (!admin_status.empty()) + if (!portOk) { - setPortAdminStatus(alias, admin_status == "up"); - SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str()); + SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str()); + + writeConfigToAppDb(alias, "mtu", mtu); + writeConfigToAppDb(alias, "admin_status", admin_status); + field_values.clear(); + field_values.emplace_back("mtu", mtu); + field_values.emplace_back("admin_status", admin_status); + it->second = KeyOpFieldsValuesTuple{alias, SET_COMMAND, field_values}; + it++; + continue; } - if (!learn_mode.empty()) + if (!mtu.empty()) { - setPortLearnMode(alias, learn_mode); - SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str()); + setPortMtu(alias, mtu); + SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str()); } - if (!tpid.empty()) + if (!admin_status.empty()) { - setPortTpid(alias, tpid); - SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str()); + setPortAdminStatus(alias, admin_status == "up"); + SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str()); } } else if (op == DEL_COMMAND) @@ -193,3 +166,13 @@ void PortMgr::doTask(Consumer &consumer) it = consumer.m_toSync.erase(it); } } + +bool PortMgr::writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value) +{ + vector fvs; + FieldValueTuple fv(field, value); + fvs.push_back(fv); + m_appPortTable.set(alias, fvs); + + return true; +} diff --git a/cfgmgr/portmgr.h b/cfgmgr/portmgr.h index 809cd1c004..dde346bfe1 100644 --- a/cfgmgr/portmgr.h +++ b/cfgmgr/portmgr.h @@ -29,10 +29,9 @@ class PortMgr : public Orch std::set m_portList; void doTask(Consumer &consumer); + bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value); bool setPortMtu(const std::string &alias, const std::string &mtu); - bool setPortTpid(const std::string &alias, const std::string &tpid); bool setPortAdminStatus(const std::string &alias, const bool up); - bool setPortLearnMode(const std::string &alias, const std::string &learn_mode); bool isPortStateOk(const std::string &alias); }; diff --git a/orchagent/port.h b/orchagent/port.h index a561a221cf..e5ba8134f5 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -87,6 +87,12 @@ class Port UNKNOWN } ; + enum AutoNegMode { + AUTONEG_NOT_SET = -1, + AUTONEG_OFF = 0, + AUTONEG_ON = 1 + }; + Port() {}; Port(std::string alias, Type type) : m_alias(alias), m_type(type) {}; @@ -112,7 +118,7 @@ class Port uint32_t m_mtu = DEFAULT_MTU; uint32_t m_speed = 0; // Mbps std::string m_learn_mode = "hardware"; - int m_autoneg = -1; // -1 means not set, 0 = disabled, 1 = enabled + AutoNegMode m_autoneg = Port::AutoNegMode::AUTONEG_NOT_SET; bool m_admin_state_up = false; bool m_init = false; bool m_l3_vni = false; @@ -148,7 +154,7 @@ class Port uint32_t m_up_member_count = 0; uint32_t m_maximum_headroom = 0; std::vector m_adv_speeds; - sai_port_interface_type_t m_interface_type; + sai_port_interface_type_t m_interface_type = SAI_PORT_INTERFACE_TYPE_NONE; std::vector m_adv_interface_types; bool m_mpls = false; /* diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 2b816d71d2..687d1e915a 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2966,6 +2966,11 @@ void PortsOrch::doPortTask(Consumer &consumer) } else { + if (admin_status.empty()) + { + admin_status = p.m_admin_state_up ? "up" : "down"; + } + if (!an_str.empty()) { if (autoneg_mode_map.find(an_str) == autoneg_mode_map.end()) @@ -3008,7 +3013,7 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } SWSS_LOG_NOTICE("Set port %s AutoNeg from %d to %d", alias.c_str(), p.m_autoneg, an); - p.m_autoneg = an; + p.m_autoneg = static_cast(an); m_portList[alias] = p; } } diff --git a/portsyncd/portsyncd.cpp b/portsyncd/portsyncd.cpp index 37e0c4232f..8243f94f8b 100644 --- a/portsyncd/portsyncd.cpp +++ b/portsyncd/portsyncd.cpp @@ -45,14 +45,12 @@ void usage() void handlePortConfigFile(ProducerStateTable &p, string file, bool warm); void handlePortConfigFromConfigDB(ProducerStateTable &p, DBConnector &cfgDb, bool warm); void handleVlanIntfFile(string file); -void handlePortConfig(ProducerStateTable &p, map &port_cfg_map); void checkPortInitDone(DBConnector *appl_db); int main(int argc, char **argv) { Logger::linkToDbNative("portsyncd"); int opt; - map port_cfg_map; while ((opt = getopt(argc, argv, "v:h")) != -1 ) { @@ -71,7 +69,6 @@ int main(int argc, char **argv) DBConnector appl_db("APPL_DB", 0); DBConnector state_db("STATE_DB", 0); ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME); - SubscriberStateTable portCfg(&cfgDb, CFG_PORT_TABLE_NAME); WarmStart::initialize("portsyncd", "swss"); WarmStart::checkWarmStart("portsyncd", "swss"); @@ -93,7 +90,6 @@ int main(int argc, char **argv) NetDispatcher::getInstance().registerMessageHandler(RTM_DELLINK, &sync); s.addSelectable(&netlink); - s.addSelectable(&portCfg); while (true) { @@ -135,28 +131,6 @@ int main(int argc, char **argv) g_init = true; } - if (!port_cfg_map.empty()) - { - handlePortConfig(p, port_cfg_map); - } - } - else if (temps == (Selectable *)&portCfg) - { - std::deque entries; - portCfg.pops(entries); - - for (auto entry: entries) - { - string key = kfvKey(entry); - - if (port_cfg_map.find(key) != port_cfg_map.end()) - { - /* For now we simply drop previous pending port config */ - port_cfg_map.erase(key); - } - port_cfg_map[key] = entry; - } - handlePortConfig(p, port_cfg_map); } else { @@ -225,31 +199,3 @@ void handlePortConfigFromConfigDB(ProducerStateTable &p, DBConnector &cfgDb, boo } } - -void handlePortConfig(ProducerStateTable &p, map &port_cfg_map) -{ - auto it = port_cfg_map.begin(); - while (it != port_cfg_map.end()) - { - KeyOpFieldsValuesTuple entry = it->second; - string key = kfvKey(entry); - string op = kfvOp(entry); - auto values = kfvFieldsValues(entry); - - /* only push down port config when port is not in hostif create pending state */ - if (g_portSet.find(key) == g_portSet.end()) - { - /* No support for port delete yet */ - if (op == SET_COMMAND) - { - p.set(key, values); - } - - it = port_cfg_map.erase(it); - } - else - { - it++; - } - } -} diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 553cd18bfe..dedd4445f7 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -39,7 +39,9 @@ tests_SOURCES = aclorch_ut.cpp \ mock_table.cpp \ mock_hiredis.cpp \ mock_redisreply.cpp \ + mock_shell_command.cpp \ bulker_ut.cpp \ + portmgr_ut.cpp \ fake_response_publisher.cpp \ swssnet_ut.cpp \ flowcounterrouteorch_ut.cpp \ @@ -98,6 +100,7 @@ tests_SOURCES = aclorch_ut.cpp \ $(top_srcdir)/orchagent/bfdorch.cpp \ $(top_srcdir)/orchagent/srv6orch.cpp \ $(top_srcdir)/orchagent/nvgreorch.cpp \ + $(top_srcdir)/cfgmgr/portmgr.cpp \ $(top_srcdir)/cfgmgr/buffermgrdyn.cpp tests_SOURCES += $(FLEX_CTR_DIR)/flex_counter_manager.cpp $(FLEX_CTR_DIR)/flex_counter_stat_manager.cpp $(FLEX_CTR_DIR)/flow_counter_handler.cpp $(FLEX_CTR_DIR)/flowcounterrouteorch.cpp diff --git a/tests/mock_tests/mock_shell_command.cpp b/tests/mock_tests/mock_shell_command.cpp new file mode 100644 index 0000000000..f3ccfbfe5e --- /dev/null +++ b/tests/mock_tests/mock_shell_command.cpp @@ -0,0 +1,15 @@ +#include +#include + +int mockCmdReturn = 0; +std::string mockCmdStdcout = ""; +std::vector mockCallArgs; + +namespace swss { + int exec(const std::string &cmd, std::string &stdout) + { + mockCallArgs.push_back(cmd); + stdout = mockCmdStdcout; + return mockCmdReturn; + } +} diff --git a/tests/mock_tests/portmgr_ut.cpp b/tests/mock_tests/portmgr_ut.cpp new file mode 100644 index 0000000000..27dc61e03e --- /dev/null +++ b/tests/mock_tests/portmgr_ut.cpp @@ -0,0 +1,126 @@ +#include "portmgr.h" +#include "gtest/gtest.h" +#include "mock_table.h" +#include "redisutility.h" + +extern std::vector mockCallArgs; + +namespace portmgr_ut +{ + using namespace swss; + using namespace std; + + struct PortMgrTest : public ::testing::Test + { + shared_ptr m_app_db; + shared_ptr m_config_db; + shared_ptr m_state_db; + shared_ptr m_portMgr; + PortMgrTest() + { + m_app_db = make_shared( + "APPL_DB", 0); + m_config_db = make_shared( + "CONFIG_DB", 0); + m_state_db = make_shared( + "STATE_DB", 0); + } + + virtual void SetUp() override + { + ::testing_db::reset(); + vector cfg_port_tables = { + CFG_PORT_TABLE_NAME, + }; + m_portMgr.reset(new PortMgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_port_tables)); + } + }; + + TEST_F(PortMgrTest, DoTask) + { + Table state_port_table(m_state_db.get(), STATE_PORT_TABLE_NAME); + Table app_port_table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table cfg_port_table(m_config_db.get(), CFG_PORT_TABLE_NAME); + + // Port is not ready, verify that doTask does not handle port configuration + + cfg_port_table.set("Ethernet0", { + {"speed", "100000"}, + {"index", "1"} + }); + mockCallArgs.clear(); + m_portMgr->addExistingData(&cfg_port_table); + m_portMgr->doTask(); + ASSERT_TRUE(mockCallArgs.empty()); + std::vector values; + app_port_table.get("Ethernet0", values); + auto value_opt = swss::fvsGetValue(values, "mtu", true); + ASSERT_TRUE(value_opt); + ASSERT_EQ(DEFAULT_MTU_STR, value_opt.get()); + value_opt = swss::fvsGetValue(values, "admin_status", true); + ASSERT_TRUE(value_opt); + ASSERT_EQ(DEFAULT_ADMIN_STATUS_STR, value_opt.get()); + value_opt = swss::fvsGetValue(values, "speed", true); + ASSERT_TRUE(value_opt); + ASSERT_EQ("100000", value_opt.get()); + value_opt = swss::fvsGetValue(values, "index", true); + ASSERT_TRUE(value_opt); + ASSERT_EQ("1", value_opt.get()); + + // Set port state to ok, verify that doTask handle port configuration + state_port_table.set("Ethernet0", { + {"state", "ok"} + }); + m_portMgr->doTask(); + ASSERT_EQ(size_t(2), mockCallArgs.size()); + ASSERT_EQ("/sbin/ip link set dev \"Ethernet0\" mtu \"9100\"", mockCallArgs[0]); + ASSERT_EQ("/sbin/ip link set dev \"Ethernet0\" down", mockCallArgs[1]); + + // Set port admin_status, verify that it could override the default value + cfg_port_table.set("Ethernet0", { + {"admin_status", "up"} + }); + m_portMgr->addExistingData(&cfg_port_table); + m_portMgr->doTask(); + app_port_table.get("Ethernet0", values); + value_opt = swss::fvsGetValue(values, "admin_status", true); + ASSERT_TRUE(value_opt); + ASSERT_EQ("up", value_opt.get()); + } + + TEST_F(PortMgrTest, ConfigureDuringRetry) + { + Table state_port_table(m_state_db.get(), STATE_PORT_TABLE_NAME); + Table app_port_table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table cfg_port_table(m_config_db.get(), CFG_PORT_TABLE_NAME); + + cfg_port_table.set("Ethernet0", { + {"speed", "100000"}, + {"index", "1"} + }); + + mockCallArgs.clear(); + m_portMgr->addExistingData(&cfg_port_table); + m_portMgr->doTask(); + ASSERT_TRUE(mockCallArgs.empty()); + + cfg_port_table.set("Ethernet0", { + {"speed", "50000"}, + {"index", "1"}, + {"mtu", "1518"}, + {"admin_status", "up"} + }); + + m_portMgr->addExistingData(&cfg_port_table); + m_portMgr->doTask(); + ASSERT_TRUE(mockCallArgs.empty()); + + state_port_table.set("Ethernet0", { + {"state", "ok"} + }); + m_portMgr->doTask(); + ASSERT_EQ(size_t(2), mockCallArgs.size()); + ASSERT_EQ("/sbin/ip link set dev \"Ethernet0\" mtu \"1518\"", mockCallArgs[0]); + ASSERT_EQ("/sbin/ip link set dev \"Ethernet0\" up", mockCallArgs[1]); + } +}