From 0e0c8af4903fdf546746e979fbf1cf2d54357a8b Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Wed, 15 Sep 2021 01:29:04 -0700 Subject: [PATCH 1/8] Add support for long name and short name routed subinterfaces. Handling Routed subinterface ADMIN status dependency with parent interface Handling Routed subinterface MTU dependency with parent interface --- cfgmgr/intfmgr.cpp | 233 +++++++++++++++++++++++++++++++++++++--- cfgmgr/intfmgr.h | 24 ++++- cfgmgr/portmgr.cpp | 2 + orchagent/intfsorch.cpp | 6 +- orchagent/portsorch.cpp | 14 +-- orchagent/portsorch.h | 2 +- 6 files changed, 256 insertions(+), 25 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 3e0ed862be0..f50ca05aa6e 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -9,6 +9,8 @@ #include "shellcmd.h" #include "macaddress.h" #include "warm_restart.h" +#include "subscriberstatetable.h" +#include "ifcommon.h" using namespace std; using namespace swss; @@ -22,6 +24,7 @@ using namespace swss; #define VRF_MGMT "mgmt" #define LOOPBACK_DEFAULT_MTU_STR "65536" +#define PORT_MTU_DEFAULT 9100 IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames) : Orch(cfgDb, tableNames), @@ -34,8 +37,19 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_stateVlanTable(stateDb, STATE_VLAN_TABLE_NAME), m_stateVrfTable(stateDb, STATE_VRF_TABLE_NAME), m_stateIntfTable(stateDb, STATE_INTERFACE_TABLE_NAME), - m_appIntfTableProducer(appDb, APP_INTF_TABLE_NAME) + m_appIntfTableProducer(appDb, APP_INTF_TABLE_NAME), + m_appLagTable(appDb, APP_LAG_TABLE_NAME) { + auto subscriberAppTable = new swss::SubscriberStateTable(appDb, + APP_LAG_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100); + auto appConsumer = new Consumer(subscriberAppTable, this, APP_LAG_TABLE_NAME); + Orch::addExecutor(appConsumer); + + auto subscriberStateTable = new swss::SubscriberStateTable(stateDb, + STATE_PORT_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 200); + auto stateConsumer = new Consumer(subscriberStateTable, this, STATE_PORT_TABLE_NAME); + Orch::addExecutor(stateConsumer); + if (!WarmStart::isWarmStart()) { flushLoopbackIntfs(); @@ -287,22 +301,149 @@ void IntfMgr::addHostSubIntf(const string&intf, const string &subIntf, const str EXEC_WITH_ERROR_THROW(cmd.str(), res); } -void IntfMgr::setHostSubIntfMtu(const string &subIntf, const string &mtu) + +std::string IntfMgr::getPortAdminStatus(const string &alias) +{ + Table *portTable; + if (!alias.compare(0, strlen("Eth"), "Eth")) + { + portTable = &m_statePortTable; + } + else if (!alias.compare(0, strlen("Po"), "Po")) + { + portTable = &m_appLagTable; + } + else + { + return "down"; + } + + string admin = "down"; + vector temp; + portTable->get(alias, temp); + + for (auto idx : temp) + { + const auto &field = fvField(idx); + const auto &value = fvValue(idx); + if (field == "admin_status") + { + admin = value; + } + } + return admin; +} + +std::string IntfMgr::getPortMtu(const string &alias) +{ + Table *portTable; + if (!alias.compare(0, strlen("Eth"), "Eth")) + { + portTable = &m_statePortTable; + } + else if (!alias.compare(0, strlen("Po"), "Po")) + { + portTable = &m_appLagTable; + } + else + { + return "0"; + } + vector temp; + portTable->get(alias, temp); + string mtu = "0"; + for (auto idx : temp) + { + const auto &field = fvField(idx); + const auto &value = fvValue(idx); + if (field == "mtu") + { + mtu = value; + } + } + if (mtu.empty()) + mtu = std::to_string(PORT_MTU_DEFAULT); + return mtu; +} + +void IntfMgr::updateSubIntfMtu(const string &alias, const string &mtu) +{ + string intf; + for (auto entry : m_subIntfList) + { + intf = entry.first; + subIntf subIf(intf); + if (subIf.parentIntf() == alias) + { + std::vector fvVector; + string subintf_mtu = setHostSubIntfMtu(intf, m_subIntfList[intf].mtu, mtu); + FieldValueTuple fvTuple("mtu", subintf_mtu); + fvVector.push_back(fvTuple); + m_appIntfTableProducer.set(intf, fvVector); + } + } +} + +std::string IntfMgr::setHostSubIntfMtu(const string &alias, const string &configMtu, const string &parentMtu) { stringstream cmd; string res; - cmd << IP_CMD " link set " << shellquote(subIntf) << " mtu " << shellquote(mtu); + string subifMtu = configMtu; + subIntf subIf(alias); + + int pmtu = (uint32_t)stoul(parentMtu); + int cmtu = (uint32_t)stoul(configMtu); + + if (pmtu < cmtu) + { + subifMtu = parentMtu; + } + cmd << IP_CMD " link set " << shellquote(alias) << " mtu " << shellquote(subifMtu); EXEC_WITH_ERROR_THROW(cmd.str(), res); + + return subifMtu; +} + +void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin) +{ + string intf; + for (auto entry : m_subIntfList) + { + intf = entry.first; + subIntf subIf(intf); + if (subIf.parentIntf() == alias) + { + /*Avoid duplicate parent admin UP event when parent goes down + * This avoids intfmgrd carsh due to duplicate port up event when parent is actually going admin down*/ + string curr_admin = m_subIntfList[intf].currAdminStatus; + if (curr_admin == "up" && curr_admin == admin) + continue; + std::vector fvVector; + string subintf_admin = setHostSubIntfAdminStatus(intf, m_subIntfList[intf].adminStatus, admin); + m_subIntfList[intf].currAdminStatus = subintf_admin; + FieldValueTuple fvTuple("admin_status", subintf_admin); + fvVector.push_back(fvTuple); + m_appIntfTableProducer.set(intf, fvVector); + } + } } -void IntfMgr::setHostSubIntfAdminStatus(const string &subIntf, const string &adminStatus) +std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string &configAdmin, const string &parentAdmin) { stringstream cmd; string res; - cmd << IP_CMD " link set " << shellquote(subIntf) << " " << shellquote(adminStatus); - EXEC_WITH_ERROR_THROW(cmd.str(), res); + if (parentAdmin == "up" || configAdmin == "down") + { + cmd << IP_CMD " link set " << shellquote(alias) << " " << shellquote(configAdmin); + EXEC_WITH_ERROR_THROW(cmd.str(), res); + return configAdmin; + } + else + { + return "down"; + } } void IntfMgr::removeHostSubIntf(const string &subIntf) @@ -459,13 +600,26 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, string vlanId; string parentAlias; size_t found = alias.find(VLAN_SUB_INTERFACE_SEPARATOR); + subIntf subIf(alias); if (found != string::npos) { - // This is a sub interface // alias holds the complete sub interface name // while parentAlias holds the parent port name - vlanId = alias.substr(found + 1); - parentAlias = alias.substr(0, found); + /*Check if subinterface is valid and sub interface name length is < 15(IFNAMSIZ)*/ + if (!subIf.isValid()) + { + SWSS_LOG_ERROR("Invalid subnitf: %s", alias.c_str()); + return true; + } + parentAlias = subIf.parentIntf(); + int subIntfId = subIf.subIntfIdx(); + /*If long name format, subinterface Id is vlanid */ + if (!subIf.isShortName()) + { + vlanId = std::to_string(subIntfId); + FieldValueTuple vlanTuple("vlan", vlanId); + data.push_back(vlanTuple); + } } bool is_lo = !alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX); string mac = ""; @@ -515,6 +669,10 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, { ipv6_link_local_mode = value; } + else if (field == "vlan") + { + vlanId = value; + } } if (op == SET_COMMAND) @@ -580,6 +738,11 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, { if (m_subIntfList.find(alias) == m_subIntfList.end()) { + if (vlanId == "0" || vlanId.empty()) + { + SWSS_LOG_INFO("Vlan ID not configured for sub interface %s", alias.c_str()); + return false; + } try { addHostSubIntf(parentAlias, alias, vlanId); @@ -590,25 +753,33 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, return false; } - m_subIntfList.insert(alias); + m_subIntfList[alias].vlanId = vlanId; } if (!mtu.empty()) { + string subintf_mtu; try { - setHostSubIntfMtu(alias, mtu); + string parentMtu = getPortMtu(subIf.parentIntf()); + subintf_mtu = setHostSubIntfMtu(alias, mtu, parentMtu); + FieldValueTuple fvTuple("mtu", mtu); + std::remove(data.begin(), data.end(), fvTuple); + FieldValueTuple newMtuFvTuple("mtu", subintf_mtu); + data.push_back(newMtuFvTuple); } catch (const std::runtime_error &e) { SWSS_LOG_NOTICE("Sub interface ip link set mtu failure. Runtime error: %s", e.what()); return false; } + m_subIntfList[alias].mtu = mtu; } else { FieldValueTuple fvTuple("mtu", MTU_INHERITANCE); data.push_back(fvTuple); + m_subIntfList[alias].mtu = MTU_INHERITANCE; } if (adminStatus.empty()) @@ -619,13 +790,20 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, } try { - setHostSubIntfAdminStatus(alias, adminStatus); + string parentAdmin = getPortAdminStatus(subIf.parentIntf()); + string subintf_admin = setHostSubIntfAdminStatus(alias, adminStatus, parentAdmin); + m_subIntfList[alias].currAdminStatus = subintf_admin; + FieldValueTuple fvTuple("admin_status", adminStatus); + std::remove(data.begin(), data.end(), fvTuple); + FieldValueTuple newAdminFvTuple("admin_status", subintf_admin); + data.push_back(newAdminFvTuple); } catch (const std::runtime_error &e) { SWSS_LOG_NOTICE("Sub interface ip link set admin status %s failure. Runtime error: %s", adminStatus.c_str(), e.what()); return false; } + m_subIntfList[alias].adminStatus = adminStatus; // set STATE_DB port state setSubIntfStateOk(alias); @@ -782,7 +960,12 @@ void IntfMgr::doTask(Consumer &consumer) while (it != consumer.m_toSync.end()) { KeyOpFieldsValuesTuple t = it->second; - + if ((table_name == STATE_PORT_TABLE_NAME) || (table_name == APP_LAG_TABLE_NAME)) + { + doPortTableTask(kfvKey(t), kfvFieldsValues(t), kfvOp(t)); + } + else + { vector keys = tokenize(kfvKey(t), config_db_key_delimiter); const vector& data = kfvFieldsValues(t); string op = kfvOp(t); @@ -827,6 +1010,7 @@ void IntfMgr::doTask(Consumer &consumer) { SWSS_LOG_ERROR("Invalid key %s", kfvKey(t).c_str()); } + } it = consumer.m_toSync.erase(it); } @@ -836,3 +1020,26 @@ void IntfMgr::doTask(Consumer &consumer) setWarmReplayDoneState(); } } + +void IntfMgr::doPortTableTask(const string& key, vector data, string op) +{ + if (op == SET_COMMAND) + { + for (auto idx : data) + { + const auto &field = fvField(idx); + const auto &value = fvValue(idx); + + if (field == "admin_status") + { + SWSS_LOG_INFO("Port %s Admin %s", key.c_str(), value.c_str()); + updateSubIntfAdminStatus(key, value); + } + else if (field == "mtu") + { + SWSS_LOG_INFO("Port %s MTU %s", key.c_str(), value.c_str()); + updateSubIntfMtu(key, value); + } + } + } +} diff --git a/cfgmgr/intfmgr.h b/cfgmgr/intfmgr.h index 655fb4deeb6..f22e86e28e8 100644 --- a/cfgmgr/intfmgr.h +++ b/cfgmgr/intfmgr.h @@ -9,6 +9,16 @@ #include #include +struct SubIntfInfo +{ + std::string vlanId; + std::string mtu; + std::string adminStatus; + std::string currAdminStatus; +}; + +typedef std::map SubIntfMap; + namespace swss { class IntfMgr : public Orch @@ -20,9 +30,9 @@ class IntfMgr : public Orch private: ProducerStateTable m_appIntfTableProducer; Table m_cfgIntfTable, m_cfgVlanIntfTable, m_cfgLagIntfTable, m_cfgLoopbackIntfTable; - Table m_statePortTable, m_stateLagTable, m_stateVlanTable, m_stateVrfTable, m_stateIntfTable; + Table m_statePortTable, m_stateLagTable, m_stateVlanTable, m_stateVrfTable, m_stateIntfTable, m_appLagTable; - std::set m_subIntfList; + SubIntfMap m_subIntfList; std::set m_loopbackIntfList; std::set m_pendingReplayIntfList; @@ -34,6 +44,7 @@ class IntfMgr : public Orch bool doIntfGeneralTask(const std::vector& keys, std::vector data, const std::string& op); bool doIntfAddrTask(const std::vector& keys, const std::vector& data, const std::string& op); void doTask(Consumer &consumer); + void doPortTableTask(const std::string& key, std::vector data, std::string op); bool isIntfStateOk(const std::string &alias); bool isIntfCreated(const std::string &alias); @@ -46,9 +57,11 @@ class IntfMgr : public Orch void delLoopbackIntf(const std::string &alias); void flushLoopbackIntfs(void); + std::string getPortAdminStatus(const std::string &alias); + std::string getPortMtu(const std::string &alias); void addHostSubIntf(const std::string&intf, const std::string &subIntf, const std::string &vlan); - void setHostSubIntfMtu(const std::string &subIntf, const std::string &mtu); - void setHostSubIntfAdminStatus(const std::string &subIntf, const std::string &admin_status); + std::string setHostSubIntfMtu(const std::string &alias, const std::string &configMtu, const std::string &parentMtu); + std::string setHostSubIntfAdminStatus(const std::string &alias, const std::string &configAdmin, const std::string &parentAdmin); void removeHostSubIntf(const std::string &subIntf); void setSubIntfStateOk(const std::string &alias); void removeSubIntfState(const std::string &alias); @@ -56,6 +69,9 @@ class IntfMgr : public Orch bool setIntfProxyArp(const std::string &alias, const std::string &proxy_arp); bool setIntfGratArp(const std::string &alias, const std::string &grat_arp); + void updateSubIntfAdminStatus(const std::string &alias, const std::string &admin); + void updateSubIntfMtu(const std::string &alias, const std::string &mtu); + bool m_replayDone {false}; }; diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index 8fc43ca47f2..cde2c9168ef 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -35,6 +35,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) fvs.push_back(fv); m_appPortTable.set(alias, fvs); + m_statePortTable.set(alias, fvs); return true; } @@ -67,6 +68,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up) fvs.push_back(fv); m_appPortTable.set(alias, fvs); + m_statePortTable.set(alias, fvs); return true; } diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index b270a0c5e59..687acf8b346 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -749,6 +749,10 @@ void IntfsOrch::doTask(Consumer &consumer) { inband_type = value; } + else if (field == "vlan") + { + vlan = value; + } } if (alias == "eth0" || alias == "docker0") @@ -818,7 +822,7 @@ void IntfsOrch::doTask(Consumer &consumer) { if (!ip_prefix_in_key && isSubIntf) { - if (!gPortsOrch->addSubPort(port, alias, adminUp, mtu)) + if (!gPortsOrch->addSubPort(port, alias, vlan, adminUp, mtu)) { it++; continue; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 76babdf24d2..c1ae7603fa6 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -31,6 +31,7 @@ #include "fdborch.h" #include "stringutility.h" #include "subscriberstatetable.h" +#include "ifcommon.h" extern sai_switch_api_t *sai_switch_api; extern sai_bridge_api_t *sai_bridge_api; @@ -763,7 +764,7 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port return false; } -bool PortsOrch::addSubPort(Port &port, const string &alias, const bool &adminUp, const uint32_t &mtu) +bool PortsOrch::addSubPort(Port &port, const string &alias, const string &vlan, const bool &adminUp, const uint32_t &mtu) { SWSS_LOG_ENTER(); @@ -773,21 +774,22 @@ bool PortsOrch::addSubPort(Port &port, const string &alias, const bool &adminUp, SWSS_LOG_ERROR("%s is not a sub interface", alias.c_str()); return false; } - string parentAlias = alias.substr(0, found); - string vlanId = alias.substr(found + 1); + subIntf subIf(alias); + string subport = subIf.longName(); + string parentAlias = subIf.parentIntf(); sai_vlan_id_t vlan_id; try { - vlan_id = static_cast(stoul(vlanId)); + vlan_id = static_cast(stoul(vlan)); } catch (const std::invalid_argument &e) { - SWSS_LOG_ERROR("Invalid argument %s to %s()", vlanId.c_str(), e.what()); + SWSS_LOG_ERROR("Invalid argument %s to %s()", vlan.c_str(), e.what()); return false; } catch (const std::out_of_range &e) { - SWSS_LOG_ERROR("Out of range argument %s to %s()", vlanId.c_str(), e.what()); + SWSS_LOG_ERROR("Out of range argument %s to %s()", vlan.c_str(), e.what()); return false; } if (vlan_id > MAX_VALID_VLAN_ID) diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 22efce3561d..387f81ba4eb 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -132,7 +132,7 @@ class PortsOrch : public Orch, public Subject void refreshPortStatus(); bool removeAclTableGroup(const Port &p); - bool addSubPort(Port &port, const string &alias, const bool &adminUp = true, const uint32_t &mtu = 0); + bool addSubPort(Port &port, const string &alias, const string &vlan, const bool &adminUp = true, const uint32_t &mtu = 0); bool removeSubPort(const string &alias); bool updateL3VniStatus(uint16_t vlan_id, bool status); void getLagMember(Port &lag, vector &portv); From ed4877b50793b3610ba8191179a441b95f2dcf8c Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Thu, 11 Nov 2021 22:38:11 -0800 Subject: [PATCH 2/8] Addressing review comments: 1. use statedb PORT_TABLE and statedb LAG_TABLE to get admin and mtu on front panel and port channel interfaces. Portsyncd and teamsyncd now updates admin and MTU status of corresponding netdev to statedb PORT_TABLE and LAG_TABLE. 2. Subinterface library moved to swss/lib as part of PR #2017. Makefile and header inclusion changes updates accordingly. --- cfgmgr/Makefile.am | 2 +- cfgmgr/intfmgr.cpp | 33 +++++++++++++++++++++++---------- cfgmgr/intfmgr.h | 2 +- cfgmgr/portmgr.cpp | 2 -- orchagent/Makefile.am | 1 + orchagent/intfsorch.cpp | 1 + orchagent/portsorch.cpp | 2 +- portsyncd/linksync.cpp | 5 +++++ teamsyncd/teamsync.cpp | 23 +++++++++++++++++------ teamsyncd/teamsync.h | 2 ++ tests/mock_tests/Makefile.am | 1 + 11 files changed, 53 insertions(+), 21 deletions(-) diff --git a/cfgmgr/Makefile.am b/cfgmgr/Makefile.am index d55f6b9a52a..fec8b2f96cc 100644 --- a/cfgmgr/Makefile.am +++ b/cfgmgr/Makefile.am @@ -38,7 +38,7 @@ portmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) portmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) portmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -intfmgrd_SOURCES = intfmgrd.cpp intfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +intfmgrd_SOURCES = intfmgrd.cpp intfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/lib/subintf.cpp shellcmd.h intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) intfmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index f50ca05aa6e..d50d9ef1774 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -10,7 +10,7 @@ #include "macaddress.h" #include "warm_restart.h" #include "subscriberstatetable.h" -#include "ifcommon.h" +#include "subintf.h" using namespace std; using namespace swss; @@ -24,7 +24,7 @@ using namespace swss; #define VRF_MGMT "mgmt" #define LOOPBACK_DEFAULT_MTU_STR "65536" -#define PORT_MTU_DEFAULT 9100 +#define DEFAULT_MTU_STR 9100 IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector &tableNames) : Orch(cfgDb, tableNames), @@ -40,16 +40,21 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_appIntfTableProducer(appDb, APP_INTF_TABLE_NAME), m_appLagTable(appDb, APP_LAG_TABLE_NAME) { - auto subscriberAppTable = new swss::SubscriberStateTable(appDb, + /*auto subscriberAppTable = new swss::SubscriberStateTable(appDb, APP_LAG_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100); auto appConsumer = new Consumer(subscriberAppTable, this, APP_LAG_TABLE_NAME); - Orch::addExecutor(appConsumer); + Orch::addExecutor(appConsumer);*/ auto subscriberStateTable = new swss::SubscriberStateTable(stateDb, - STATE_PORT_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 200); + STATE_PORT_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100); auto stateConsumer = new Consumer(subscriberStateTable, this, STATE_PORT_TABLE_NAME); Orch::addExecutor(stateConsumer); + auto subscriberStateLagTable = new swss::SubscriberStateTable(stateDb, + STATE_LAG_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 200); + auto stateLagConsumer = new Consumer(subscriberStateLagTable, this, STATE_LAG_TABLE_NAME); + Orch::addExecutor(stateLagConsumer); + if (!WarmStart::isWarmStart()) { flushLoopbackIntfs(); @@ -302,7 +307,7 @@ void IntfMgr::addHostSubIntf(const string&intf, const string &subIntf, const str } -std::string IntfMgr::getPortAdminStatus(const string &alias) +std::string IntfMgr::getIntfAdminStatus(const string &alias) { Table *portTable; if (!alias.compare(0, strlen("Eth"), "Eth")) @@ -362,7 +367,7 @@ std::string IntfMgr::getPortMtu(const string &alias) } } if (mtu.empty()) - mtu = std::to_string(PORT_MTU_DEFAULT); + mtu = std::to_string(DEFAULT_MTU_STR); return mtu; } @@ -376,7 +381,13 @@ void IntfMgr::updateSubIntfMtu(const string &alias, const string &mtu) if (subIf.parentIntf() == alias) { std::vector fvVector; - string subintf_mtu = setHostSubIntfMtu(intf, m_subIntfList[intf].mtu, mtu); + + string subif_config_mtu = m_subIntfList[intf].mtu; + if (subif_config_mtu == MTU_INHERITANCE || subif_config_mtu.empty()) + subif_config_mtu = std::to_string(DEFAULT_MTU_STR); + + string subintf_mtu = setHostSubIntfMtu(intf, subif_config_mtu, mtu); + FieldValueTuple fvTuple("mtu", subintf_mtu); fvVector.push_back(fvTuple); m_appIntfTableProducer.set(intf, fvVector); @@ -399,6 +410,7 @@ std::string IntfMgr::setHostSubIntfMtu(const string &alias, const string &config { subifMtu = parentMtu; } + SWSS_LOG_INFO("subintf %s active mtu: %s", alias.c_str(), subifMtu.c_str()); cmd << IP_CMD " link set " << shellquote(alias) << " mtu " << shellquote(subifMtu); EXEC_WITH_ERROR_THROW(cmd.str(), res); @@ -436,6 +448,7 @@ std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string if (parentAdmin == "up" || configAdmin == "down") { + SWSS_LOG_INFO("subintf %s admin_status: %s", alias.c_str(), configAdmin.c_str()); cmd << IP_CMD " link set " << shellquote(alias) << " " << shellquote(configAdmin); EXEC_WITH_ERROR_THROW(cmd.str(), res); return configAdmin; @@ -790,7 +803,7 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, } try { - string parentAdmin = getPortAdminStatus(subIf.parentIntf()); + string parentAdmin = getIntfAdminStatus(subIf.parentIntf()); string subintf_admin = setHostSubIntfAdminStatus(alias, adminStatus, parentAdmin); m_subIntfList[alias].currAdminStatus = subintf_admin; FieldValueTuple fvTuple("admin_status", adminStatus); @@ -960,7 +973,7 @@ void IntfMgr::doTask(Consumer &consumer) while (it != consumer.m_toSync.end()) { KeyOpFieldsValuesTuple t = it->second; - if ((table_name == STATE_PORT_TABLE_NAME) || (table_name == APP_LAG_TABLE_NAME)) + if ((table_name == STATE_PORT_TABLE_NAME) || (table_name == STATE_LAG_TABLE_NAME)) { doPortTableTask(kfvKey(t), kfvFieldsValues(t), kfvOp(t)); } diff --git a/cfgmgr/intfmgr.h b/cfgmgr/intfmgr.h index f22e86e28e8..65c6e3d36fb 100644 --- a/cfgmgr/intfmgr.h +++ b/cfgmgr/intfmgr.h @@ -57,7 +57,7 @@ class IntfMgr : public Orch void delLoopbackIntf(const std::string &alias); void flushLoopbackIntfs(void); - std::string getPortAdminStatus(const std::string &alias); + std::string getIntfAdminStatus(const std::string &alias); std::string getPortMtu(const std::string &alias); void addHostSubIntf(const std::string&intf, const std::string &subIntf, const std::string &vlan); std::string setHostSubIntfMtu(const std::string &alias, const std::string &configMtu, const std::string &parentMtu); diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index cde2c9168ef..8fc43ca47f2 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -35,7 +35,6 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) fvs.push_back(fv); m_appPortTable.set(alias, fvs); - m_statePortTable.set(alias, fvs); return true; } @@ -68,7 +67,6 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up) fvs.push_back(fv); m_appPortTable.set(alias, fvs); - m_statePortTable.set(alias, fvs); return true; } diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index d4f36272032..b52ffd7abde 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -34,6 +34,7 @@ endif orchagent_SOURCES = \ main.cpp \ $(top_srcdir)/lib/gearboxutils.cpp \ + $(top_srcdir)/lib/subintf.cpp \ orchdaemon.cpp \ orch.cpp \ notifications.cpp \ diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 687acf8b346..4d1d0a463ff 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -662,6 +662,7 @@ void IntfsOrch::doTask(Consumer &consumer) string proxy_arp = ""; string inband_type = ""; bool mpls = false; + string vlan = ""; for (auto idx : data) { diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index c1ae7603fa6..f9e4319e246 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -5,6 +5,7 @@ #include "gearboxutils.h" #include "vxlanorch.h" #include "directory.h" +#include "subintf.h" #include #include @@ -31,7 +32,6 @@ #include "fdborch.h" #include "stringutility.h" #include "subscriberstatetable.h" -#include "ifcommon.h" extern sai_switch_api_t *sai_switch_api; extern sai_bridge_api_t *sai_bridge_api; diff --git a/portsyncd/linksync.cpp b/portsyncd/linksync.cpp index 08df3ff9ec6..4a2b351ee07 100644 --- a/portsyncd/linksync.cpp +++ b/portsyncd/linksync.cpp @@ -182,6 +182,7 @@ void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj) unsigned int ifindex = rtnl_link_get_ifindex(link); int master = rtnl_link_get_master(link); char *type = rtnl_link_get_type(link); + unsigned int mtu = rtnl_link_get_mtu(link); if (type) { @@ -251,10 +252,14 @@ void LinkSync::onMsg(int nlmsg_type, struct nl_object *obj) { g_portSet.erase(key); FieldValueTuple tuple("state", "ok"); + FieldValueTuple admin_status("admin_status", (admin ? "up" : "down")); + FieldValueTuple port_mtu("mtu", to_string(mtu)); vector vector; vector.push_back(tuple); FieldValueTuple op("netdev_oper_status", oper ? "up" : "down"); vector.push_back(op); + vector.push_back(admin_status); + vector.push_back(port_mtu); m_statePortTable.set(key, vector); SWSS_LOG_NOTICE("Publish %s(ok:%s) to state db", key.c_str(), oper ? "up" : "down"); } diff --git a/teamsyncd/teamsync.cpp b/teamsyncd/teamsync.cpp index 846d474a6cd..661da4ee648 100644 --- a/teamsyncd/teamsync.cpp +++ b/teamsyncd/teamsync.cpp @@ -157,11 +157,19 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state, SWSS_LOG_INFO("Add %s admin_status:%s oper_status:%s, mtu: %d", lagName.c_str(), admin_state ? "up" : "down", oper_state ? "up" : "down", mtu); + bool lag_update = true; + auto it = m_teamSelectables.find(lagName); /* Return when the team instance has already been tracked */ if (m_teamSelectables.find(lagName) != m_teamSelectables.end()) - return; + { + auto tsync = m_teamSelectables[lagName]; + if (tsync->admin_state == admin_state && tsync->mtu == mtu) + return; + tsync->admin_state = admin_state; + tsync->mtu = mtu; + lag_update = false; + } - fvVector.clear(); FieldValueTuple s("state", "ok"); fvVector.push_back(s); if (m_warmstart) @@ -173,10 +181,13 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state, m_stateLagTable.set(lagName, fvVector); } - /* Create the team instance */ - auto sync = make_shared(lagName, ifindex, &m_lagMemberTable); - m_teamSelectables[lagName] = sync; - m_selectablesToAdd.insert(lagName); + if (lag_update) + { + /* Create the team instance */ + auto sync = make_shared(lagName, ifindex, &m_lagMemberTable); + m_teamSelectables[lagName] = sync; + m_selectablesToAdd.insert(lagName); + } } void TeamSync::removeLag(const string &lagName) diff --git a/teamsyncd/teamsync.h b/teamsyncd/teamsync.h index 406953e3121..deb5d841293 100644 --- a/teamsyncd/teamsync.h +++ b/teamsyncd/teamsync.h @@ -43,6 +43,8 @@ class TeamSync : public NetMsg /* member_name -> enabled|disabled */ std::map m_lagMembers; + bool admin_state; + unsigned int mtu; protected: int onChange(); static int teamdHandler(struct team_handle *th, void *arg, diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 5fed8001a4e..169edb1238e 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -33,6 +33,7 @@ tests_SOURCES = aclorch_ut.cpp \ mock_redisreply.cpp \ bulker_ut.cpp \ $(top_srcdir)/lib/gearboxutils.cpp \ + $(top_srcdir)/lib/subintf.cpp \ $(top_srcdir)/orchagent/orchdaemon.cpp \ $(top_srcdir)/orchagent/orch.cpp \ $(top_srcdir)/orchagent/notifications.cpp \ From 7d84b06b2f47fa94e91e07a47ae03db2ff7015f6 Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Thu, 11 Nov 2021 23:48:18 -0800 Subject: [PATCH 3/8] Addressing left over comments --- cfgmgr/intfmgr.cpp | 104 +++++++++++++++++++--------------------- cfgmgr/intfmgr.h | 6 +-- orchagent/portsorch.cpp | 1 - 3 files changed, 53 insertions(+), 58 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 2caeca3968b..1eafcb56d5c 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -41,11 +41,6 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c m_appIntfTableProducer(appDb, APP_INTF_TABLE_NAME), m_appLagTable(appDb, APP_LAG_TABLE_NAME) { - /*auto subscriberAppTable = new swss::SubscriberStateTable(appDb, - APP_LAG_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100); - auto appConsumer = new Consumer(subscriberAppTable, this, APP_LAG_TABLE_NAME); - Orch::addExecutor(appConsumer);*/ - auto subscriberStateTable = new swss::SubscriberStateTable(stateDb, STATE_PORT_TABLE_NAME, TableConsumable::DEFAULT_POP_BATCH_SIZE, 100); auto stateConsumer = new Consumer(subscriberStateTable, this, STATE_PORT_TABLE_NAME); @@ -340,7 +335,7 @@ std::string IntfMgr::getIntfAdminStatus(const string &alias) return admin; } -std::string IntfMgr::getPortMtu(const string &alias) +std::string IntfMgr::getIntfMtu(const string &alias) { Table *portTable; if (!alias.compare(0, strlen("Eth"), "Eth")) @@ -368,7 +363,9 @@ std::string IntfMgr::getPortMtu(const string &alias) } } if (mtu.empty()) + { mtu = std::to_string(DEFAULT_MTU_STR); + } return mtu; } @@ -396,20 +393,20 @@ void IntfMgr::updateSubIntfMtu(const string &alias, const string &mtu) } } -std::string IntfMgr::setHostSubIntfMtu(const string &alias, const string &configMtu, const string &parentMtu) +std::string IntfMgr::setHostSubIntfMtu(const string &alias, const string &mtu, const string &parent_mtu) { stringstream cmd; string res; - string subifMtu = configMtu; + string subifMtu = mtu; subIntf subIf(alias); - int pmtu = (uint32_t)stoul(parentMtu); - int cmtu = (uint32_t)stoul(configMtu); + int pmtu = (uint32_t)stoul(parent_mtu); + int cmtu = (uint32_t)stoul(mtu); if (pmtu < cmtu) { - subifMtu = parentMtu; + subifMtu = parent_mtu; } SWSS_LOG_INFO("subintf %s active mtu: %s", alias.c_str(), subifMtu.c_str()); cmd << IP_CMD " link set " << shellquote(alias) << " mtu " << shellquote(subifMtu); @@ -427,8 +424,7 @@ void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin) subIntf subIf(intf); if (subIf.parentIntf() == alias) { - /*Avoid duplicate parent admin UP event when parent goes down - * This avoids intfmgrd carsh due to duplicate port up event when parent is actually going admin down*/ + /* Avoid duplicate interface admin UP event. */ string curr_admin = m_subIntfList[intf].currAdminStatus; if (curr_admin == "up" && curr_admin == admin) continue; @@ -442,17 +438,17 @@ void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin) } } -std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string &configAdmin, const string &parentAdmin) +std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string &admin_status, const string &parent_admin_status) { stringstream cmd; string res; - if (parentAdmin == "up" || configAdmin == "down") + if (parent_admin_status == "up" || admin_status == "down") { - SWSS_LOG_INFO("subintf %s admin_status: %s", alias.c_str(), configAdmin.c_str()); - cmd << IP_CMD " link set " << shellquote(alias) << " " << shellquote(configAdmin); + 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); - return configAdmin; + return admin_status; } else { @@ -780,7 +776,7 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, string subintf_mtu; try { - string parentMtu = getPortMtu(subIf.parentIntf()); + string parentMtu = getIntfMtu(subIf.parentIntf()); subintf_mtu = setHostSubIntfMtu(alias, mtu, parentMtu); FieldValueTuple fvTuple("mtu", mtu); std::remove(data.begin(), data.end(), fvTuple); @@ -985,51 +981,51 @@ void IntfMgr::doTask(Consumer &consumer) } else { - vector keys = tokenize(kfvKey(t), config_db_key_delimiter); - const vector& data = kfvFieldsValues(t); - string op = kfvOp(t); + vector keys = tokenize(kfvKey(t), config_db_key_delimiter); + const vector& data = kfvFieldsValues(t); + string op = kfvOp(t); - if (keys.size() == 1) - { - if((table_name == CFG_VOQ_INBAND_INTERFACE_TABLE_NAME) && - (op == SET_COMMAND)) + if (keys.size() == 1) { - //No further processing needed. Just relay to orchagent - m_appIntfTableProducer.set(keys[0], data); - m_stateIntfTable.hset(keys[0], "vrf", ""); + if((table_name == CFG_VOQ_INBAND_INTERFACE_TABLE_NAME) && + (op == SET_COMMAND)) + { + //No further processing needed. Just relay to orchagent + m_appIntfTableProducer.set(keys[0], data); + m_stateIntfTable.hset(keys[0], "vrf", ""); - it = consumer.m_toSync.erase(it); - continue; - } - if (!doIntfGeneralTask(keys, data, op)) - { - it++; - continue; - } - else - { - //Entry programmed, remove it from pending list if present - m_pendingReplayIntfList.erase(keys[0]); + it = consumer.m_toSync.erase(it); + continue; + } + if (!doIntfGeneralTask(keys, data, op)) + { + it++; + continue; + } + else + { + //Entry programmed, remove it from pending list if present + m_pendingReplayIntfList.erase(keys[0]); + } } - } - else if (keys.size() == 2) - { - if (!doIntfAddrTask(keys, data, op)) + else if (keys.size() == 2) { - it++; - continue; + if (!doIntfAddrTask(keys, data, op)) + { + it++; + continue; + } + else + { + //Entry programmed, remove it from pending list if present + m_pendingReplayIntfList.erase(keys[0] + config_db_key_delimiter + keys[1] ); + } } else { - //Entry programmed, remove it from pending list if present - m_pendingReplayIntfList.erase(keys[0] + config_db_key_delimiter + keys[1] ); + SWSS_LOG_ERROR("Invalid key %s", kfvKey(t).c_str()); } } - else - { - SWSS_LOG_ERROR("Invalid key %s", kfvKey(t).c_str()); - } - } it = consumer.m_toSync.erase(it); } diff --git a/cfgmgr/intfmgr.h b/cfgmgr/intfmgr.h index 65c6e3d36fb..e1a5049fff8 100644 --- a/cfgmgr/intfmgr.h +++ b/cfgmgr/intfmgr.h @@ -58,10 +58,10 @@ class IntfMgr : public Orch void flushLoopbackIntfs(void); std::string getIntfAdminStatus(const std::string &alias); - std::string getPortMtu(const std::string &alias); + std::string getIntfMtu(const std::string &alias); void addHostSubIntf(const std::string&intf, const std::string &subIntf, const std::string &vlan); - std::string setHostSubIntfMtu(const std::string &alias, const std::string &configMtu, const std::string &parentMtu); - std::string setHostSubIntfAdminStatus(const std::string &alias, const std::string &configAdmin, const std::string &parentAdmin); + std::string setHostSubIntfMtu(const std::string &alias, const std::string &mtu, const std::string &parent_mtu); + std::string setHostSubIntfAdminStatus(const std::string &alias, const std::string &admin_status, const std::string &parent_admin_status); void removeHostSubIntf(const std::string &subIntf); void setSubIntfStateOk(const std::string &alias); void removeSubIntfState(const std::string &alias); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 2b9ce954849..33a81130c45 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -837,7 +837,6 @@ bool PortsOrch::addSubPort(Port &port, const string &alias, const string &vlan, return false; } subIntf subIf(alias); - string subport = subIf.longName(); string parentAlias = subIf.parentIntf(); sai_vlan_id_t vlan_id; try From 386a243db3f17572392609d8cfede0c50959df1e Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Tue, 16 Nov 2021 05:57:00 -0800 Subject: [PATCH 4/8] Fixing compilation issue --- teamsyncd/teamsync.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/teamsyncd/teamsync.cpp b/teamsyncd/teamsync.cpp index 661da4ee648..6d8c025911b 100644 --- a/teamsyncd/teamsync.cpp +++ b/teamsyncd/teamsync.cpp @@ -158,7 +158,6 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state, lagName.c_str(), admin_state ? "up" : "down", oper_state ? "up" : "down", mtu); bool lag_update = true; - auto it = m_teamSelectables.find(lagName); /* Return when the team instance has already been tracked */ if (m_teamSelectables.find(lagName) != m_teamSelectables.end()) { From e38a62c29195ca8925d134bc66796e3ac795299e Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Thu, 18 Nov 2021 06:43:45 -0800 Subject: [PATCH 5/8] UT Fixes and subport Pytest script update --- cfgmgr/intfmgr.cpp | 13 +++- orchagent/intfsorch.cpp | 12 +++- tests/test_sub_port_intf.py | 130 ++++++++++++++++++++++++++++++------ 3 files changed, 133 insertions(+), 22 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 1eafcb56d5c..7e97f824a37 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -18,6 +18,7 @@ using namespace swss; #define VLAN_PREFIX "Vlan" #define LAG_PREFIX "PortChannel" +#define SUBINTF_LAG_PREFIX "Po" #define LOOPBACK_PREFIX "Loopback" #define VNET_PREFIX "Vnet" #define MTU_INHERITANCE "0" @@ -469,7 +470,7 @@ void IntfMgr::setSubIntfStateOk(const string &alias) { vector fvTuples = {{"state", "ok"}}; - if (!alias.compare(0, strlen(LAG_PREFIX), LAG_PREFIX)) + if (!alias.compare(0, strlen(SUBINTF_LAG_PREFIX), SUBINTF_LAG_PREFIX)) { m_stateLagTable.set(alias, fvTuples); } @@ -482,7 +483,7 @@ void IntfMgr::setSubIntfStateOk(const string &alias) void IntfMgr::removeSubIntfState(const string &alias) { - if (!alias.compare(0, strlen(LAG_PREFIX), LAG_PREFIX)) + if (!alias.compare(0, strlen(SUBINTF_LAG_PREFIX), SUBINTF_LAG_PREFIX)) { m_stateLagTable.del(alias); } @@ -601,6 +602,14 @@ bool IntfMgr::isIntfStateOk(const string &alias) { return true; } + else if (!alias.compare(0, strlen(SUBINTF_LAG_PREFIX), SUBINTF_LAG_PREFIX)) + { + if (m_stateLagTable.get(alias, temp)) + { + SWSS_LOG_DEBUG("Lag %s is ready", alias.c_str()); + return true; + } + } return false; } diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 4d1d0a463ff..1feebb4d753 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -657,7 +657,8 @@ void IntfsOrch::doTask(Consumer &consumer) MacAddress mac; uint32_t mtu = 0; - bool adminUp = false; + bool adminUp; + bool adminStateChanged = false; uint32_t nat_zone_id = 0; string proxy_arp = ""; string inband_type = ""; @@ -737,6 +738,7 @@ void IntfsOrch::doTask(Consumer &consumer) SWSS_LOG_WARN("Sub interface %s unknown admin status %s", alias.c_str(), value.c_str()); } } + adminStateChanged = true; } else if (field == "nat_zone") { @@ -823,6 +825,10 @@ void IntfsOrch::doTask(Consumer &consumer) { if (!ip_prefix_in_key && isSubIntf) { + if (adminStateChanged == false) + { + adminUp = port.m_admin_state_up; + } if (!gPortsOrch->addSubPort(port, alias, vlan, adminUp, mtu)) { it++; @@ -863,6 +869,10 @@ void IntfsOrch::doTask(Consumer &consumer) } else { + if (adminStateChanged == false) + { + adminUp = port.m_admin_state_up; + } if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu)) { it++; diff --git a/tests/test_sub_port_intf.py b/tests/test_sub_port_intf.py index ac65a306969..9182a2bccb6 100644 --- a/tests/test_sub_port_intf.py +++ b/tests/test_sub_port_intf.py @@ -36,11 +36,14 @@ VRF_NAME = "vrf_name" ETHERNET_PREFIX = "Ethernet" +SUBINTF_LAG_PREFIX = "Po" LAG_PREFIX = "PortChannel" VLAN_SUB_INTERFACE_SEPARATOR = "." APPL_DB_SEPARATOR = ":" +ETHERNET_PORT_DEFAULT_MTU = "1500" + class TestSubPortIntf(object): SUB_PORT_INTERFACE_UNDER_TEST = "Ethernet64.10" @@ -81,7 +84,62 @@ def connect_dbs(self, dvs): if phy_port in key: self.buf_q_fvs[key] = self.config_db.get_entry("BUFFER_QUEUE", key) + def get_subintf_longname(self, port_name): + if port_name is None: + return None + sub_intf_sep_idx = port_name.find(VLAN_SUB_INTERFACE_SEPARATOR) + if sub_intf_sep_idx == -1: + return str(port_name) + parent_intf = port_name[:sub_intf_sep_idx] + sub_intf_idx = port_name[(sub_intf_sep_idx+1):] + if port_name.startswith("Eth"): + if port_name.startswith("Ethernet"): + intf_index=port_name[len("Ethernet"):sub_intf_sep_idx] + else: + intf_index=port_name[len("Eth"):sub_intf_sep_idx] + return "Ethernet"+intf_index+VLAN_SUB_INTERFACE_SEPARATOR+sub_intf_idx + elif port_name.startswith("Po"): + if port_name.startswith("PortChannel"): + intf_index=port_name[len("PortChannel"):sub_intf_sep_idx] + else: + intf_index=port_name[len("Po"):sub_intf_sep_idx] + return "PortChannel"+intf_index+VLAN_SUB_INTERFACE_SEPARATOR+sub_intf_idx + else: + return str(port_name) + + def get_port_longname(self, port_name): + if port_name is None: + return None + + if VLAN_SUB_INTERFACE_SEPARATOR in port_name: + return self.get_subintf_longname(port_name) + else: + if port_name.startswith("Eth"): + if port_name.startswith("Ethernet"): + return port_name + intf_index=port_name[len("Eth"):len(port_name)] + return "Ethernet"+intf_index + elif port_name.startswith("Po"): + if port_name.startswith("PortChannel"): + return port_name + intf_index=port_name[len("Po"):len(port_name)] + return "PortChannel"+intf_index + else: + return port_name + + def get_parent_port(self, port_name): + port = self.get_port_longname(port_name) + idx = port.find(VLAN_SUB_INTERFACE_SEPARATOR) + parent_port = "" + if port.startswith(ETHERNET_PREFIX): + parent_port = port[:idx] + else: + assert port.startswith(SUBINTF_LAG_PREFIX) + parent_port = port[:idx] + return parent_port + def get_parent_port_index(self, port_name): + port_name = self.get_port_longname(port_name) if port_name.startswith(ETHERNET_PREFIX): idx = int(port_name[len(ETHERNET_PREFIX):]) else: @@ -94,7 +152,7 @@ def set_parent_port_oper_status(self, dvs, port_name, status): srv_idx = self.get_parent_port_index(port_name) // 4 dvs.servers[srv_idx].runcmd("ip link set dev eth0 " + status) else: - assert port_name.startswith(LAG_PREFIX) + assert port_name.startswith(SUBINTF_LAG_PREFIX) dvs.runcmd("bash -c 'echo " + ("1" if status == "up" else "0") + " > /sys/class/net/" + port_name + "/carrier'") time.sleep(1) @@ -105,7 +163,7 @@ def set_parent_port_admin_status(self, dvs, port_name, status): if port_name.startswith(ETHERNET_PREFIX): tbl_name = CFG_PORT_TABLE_NAME else: - assert port_name.startswith(LAG_PREFIX) + assert port_name.startswith(SUBINTF_LAG_PREFIX) tbl_name = CFG_LAG_TABLE_NAME self.config_db.create_entry(tbl_name, port_name, fvs) time.sleep(1) @@ -119,8 +177,28 @@ def set_parent_port_admin_status(self, dvs, port_name, status): def create_vrf(self, vrf_name): self.config_db.create_entry(CFG_VRF_TABLE_NAME, vrf_name, {"NULL": "NULL"}) + def is_short_name(self, port_name): + idx = port_name.find(VLAN_SUB_INTERFACE_SEPARATOR) + parent_port = port_name[:idx] + is_short = False + if parent_port.startswith("Eth"): + if parent_port.startswith(ETHERNET_PREFIX): + is_short = False + else: + is_short = True + elif parent_port.startswith("Po"): + if parent_port.startswith("PortChannel"): + is_short = False + else: + is_short = True + return is_short + def create_sub_port_intf_profile(self, sub_port_intf_name, vrf_name=None): fvs = {ADMIN_STATUS: "up"} + idx = sub_port_intf_name.find(VLAN_SUB_INTERFACE_SEPARATOR) + sub_port_idx = sub_port_intf_name[(idx+1):] + if self.is_short_name(sub_port_intf_name) == True: + fvs["vlan"] = sub_port_idx if vrf_name: fvs[VRF_NAME] = vrf_name @@ -154,8 +232,12 @@ def add_lag_members(self, lag, members): self.config_db.create_entry(CFG_LAG_MEMBER_TABLE_NAME, key, fvs) def create_sub_port_intf_profile_appl_db(self, sub_port_intf_name, admin_status, vrf_name=None): + substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) + parent_port = self.get_parent_port(sub_port_intf_name) + vlan_id = substrs[1] pairs = [ (ADMIN_STATUS, admin_status), + ("vlan", vlan_id), ("mtu", "0"), ] if vrf_name: @@ -210,7 +292,7 @@ def remove_parent_port_appl_db(self, port_name): if port_name in key: self.config_db.delete_entry("BUFFER_QUEUE", key) else: - assert port_name.startswith(LAG_PREFIX) + assert port_name.startswith(SUBINTF_LAG_PREFIX) tbl_name = APP_LAG_TABLE_NAME tbl = swsscommon.ProducerStateTable(self.app_db.db_connection, tbl_name) tbl._del(port_name) @@ -354,14 +436,14 @@ def _access_function(): def _test_sub_port_intf_creation(self, dvs, sub_port_intf_name, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) - parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vlan_id = substrs[1] if parent_port.startswith(ETHERNET_PREFIX): state_tbl_name = STATE_PORT_TABLE_NAME phy_ports = [parent_port] parent_port_oid = dvs.asicdb.portnamemap[parent_port] else: - assert parent_port.startswith(LAG_PREFIX) + assert parent_port.startswith(SUBINTF_LAG_PREFIX) state_tbl_name = STATE_LAG_TABLE_NAME phy_ports = self.LAG_MEMBERS_UNDER_TEST old_lag_oids = self.get_oids(ASIC_LAG_TABLE) @@ -370,7 +452,7 @@ def _test_sub_port_intf_creation(self, dvs, sub_port_intf_name, vrf_name=None): old_rif_oids = self.get_oids(ASIC_RIF_TABLE) self.set_parent_port_admin_status(dvs, parent_port, "up") - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): parent_port_oid = self.get_newly_created_oid(ASIC_LAG_TABLE, old_lag_oids) # Add lag members to test physical port host interface vlan tag attribute self.add_lag_members(parent_port, self.LAG_MEMBERS_UNDER_TEST) @@ -436,7 +518,7 @@ def _test_sub_port_intf_creation(self, dvs, sub_port_intf_name, vrf_name=None): self.remove_vrf(vrf_name) self.check_vrf_removal(vrf_oid) - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): # Remove lag members from lag parent port self.remove_lag_members(parent_port, self.LAG_MEMBERS_UNDER_TEST) self.asic_db.wait_for_n_keys(ASIC_LAG_MEMBER_TABLE, 0) @@ -456,7 +538,7 @@ def test_sub_port_intf_creation(self, dvs): def _test_sub_port_intf_add_ip_addrs(self, dvs, sub_port_intf_name, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) - parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vrf_oid = self.default_vrf_oid old_rif_oids = self.get_oids(ASIC_RIF_TABLE) @@ -515,7 +597,7 @@ def _test_sub_port_intf_add_ip_addrs(self, dvs, sub_port_intf_name, vrf_name=Non self.check_vrf_removal(vrf_oid) # Remove lag - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): self.remove_lag(parent_port) self.asic_db.wait_for_n_keys(ASIC_LAG_TABLE, 0) @@ -530,7 +612,7 @@ def test_sub_port_intf_add_ip_addrs(self, dvs): def _test_sub_port_intf_appl_db_proc_seq(self, dvs, sub_port_intf_name, admin_up, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) - parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vlan_id = substrs[1] vrf_oid = self.default_vrf_oid @@ -541,7 +623,7 @@ def _test_sub_port_intf_appl_db_proc_seq(self, dvs, sub_port_intf_name, admin_up if parent_port.startswith(ETHERNET_PREFIX): parent_port_oid = dvs.asicdb.portnamemap[parent_port] else: - assert parent_port.startswith(LAG_PREFIX) + assert parent_port.startswith(SUBINTF_LAG_PREFIX) parent_port_oid = self.get_newly_created_oid(ASIC_LAG_TABLE, old_lag_oids) if vrf_name: self.create_vrf(vrf_name) @@ -581,7 +663,7 @@ def _test_sub_port_intf_appl_db_proc_seq(self, dvs, sub_port_intf_name, admin_up self.check_vrf_removal(vrf_oid) # Remove lag - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): self.remove_lag(parent_port) self.check_lag_removal(parent_port_oid) @@ -603,6 +685,7 @@ def test_sub_port_intf_appl_db_proc_seq(self, dvs): def _test_sub_port_intf_admin_status_change(self, dvs, sub_port_intf_name, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vrf_oid = self.default_vrf_oid old_rif_oids = self.get_oids(ASIC_RIF_TABLE) @@ -691,7 +774,7 @@ def _test_sub_port_intf_admin_status_change(self, dvs, sub_port_intf_name, vrf_n self.check_vrf_removal(vrf_oid) # Remove lag - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): self.remove_lag(parent_port) self.asic_db.wait_for_n_keys(ASIC_LAG_TABLE, 0) @@ -707,6 +790,7 @@ def test_sub_port_intf_admin_status_change(self, dvs): def _test_sub_port_intf_remove_ip_addrs(self, dvs, sub_port_intf_name, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) old_rif_oids = self.get_oids(ASIC_RIF_TABLE) @@ -766,7 +850,7 @@ def _test_sub_port_intf_remove_ip_addrs(self, dvs, sub_port_intf_name, vrf_name= self.asic_db.wait_for_n_keys(ASIC_VIRTUAL_ROUTER_TABLE, 1) # Remove lag - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): self.remove_lag(parent_port) self.asic_db.wait_for_n_keys(ASIC_LAG_TABLE, 0) @@ -782,6 +866,7 @@ def test_sub_port_intf_remove_ip_addrs(self, dvs): def _test_sub_port_intf_removal(self, dvs, sub_port_intf_name, removal_seq_test=False, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vlan_id = substrs[1] if parent_port.startswith(ETHERNET_PREFIX): state_tbl_name = STATE_PORT_TABLE_NAME @@ -789,7 +874,7 @@ def _test_sub_port_intf_removal(self, dvs, sub_port_intf_name, removal_seq_test= parent_port_oid = dvs.asicdb.portnamemap[parent_port] asic_tbl_name = ASIC_PORT_TABLE else: - assert parent_port.startswith(LAG_PREFIX) + assert parent_port.startswith(SUBINTF_LAG_PREFIX) state_tbl_name = STATE_LAG_TABLE_NAME phy_ports = self.LAG_MEMBERS_UNDER_TEST old_lag_oids = self.get_oids(ASIC_LAG_TABLE) @@ -799,7 +884,7 @@ def _test_sub_port_intf_removal(self, dvs, sub_port_intf_name, removal_seq_test= old_rif_oids = self.get_oids(ASIC_RIF_TABLE) self.set_parent_port_admin_status(dvs, parent_port, "up") - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): parent_port_oid = self.get_newly_created_oid(ASIC_LAG_TABLE, old_lag_oids) if removal_seq_test == False: # Add lag members to test physical port host interface vlan tag attribute @@ -881,6 +966,10 @@ def _test_sub_port_intf_removal(self, dvs, sub_port_intf_name, removal_seq_test= "SAI_ROUTER_INTERFACE_ATTR_PORT_ID": parent_port_oid, } rif_oid = self.get_newly_created_oid(ASIC_RIF_TABLE, old_rif_oids) + #If subintf mtu deleted, it inherits from parent + if vrf_name == self.VRF_UNDER_TEST: + if parent_port.startswith(ETHERNET_PREFIX): + fv_dict["SAI_ROUTER_INTERFACE_ATTR_MTU"] = ETHERNET_PORT_DEFAULT_MTU self.check_sub_port_intf_fvs(self.asic_db, ASIC_RIF_TABLE, rif_oid, fv_dict) else: rif_oid = self.get_newly_created_oid(ASIC_RIF_TABLE, old_rif_oids) @@ -964,6 +1053,7 @@ def test_sub_port_intf_removal(self, dvs): def _test_sub_port_intf_mtu(self, dvs, sub_port_intf_name, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vrf_oid = self.default_vrf_oid old_rif_oids = self.get_oids(ASIC_RIF_TABLE) @@ -1007,7 +1097,7 @@ def _test_sub_port_intf_mtu(self, dvs, sub_port_intf_name, vrf_name=None): self.check_vrf_removal(vrf_oid) # Remove lag - if parent_port.startswith(LAG_PREFIX): + if parent_port.startswith(SUBINTF_LAG_PREFIX): self.remove_lag(parent_port) self.asic_db.wait_for_n_keys(ASIC_LAG_TABLE, 0) @@ -1126,13 +1216,14 @@ def check_nhg_members_on_parent_port_oper_status_change(self, dvs, parent_port_p def _test_sub_port_intf_nhg_accel(self, dvs, sub_port_intf_name, nhop_num=3, create_intf_on_parent_port=False, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vlan_id = substrs[1] assert len(vlan_id) == 2 if parent_port.startswith(ETHERNET_PREFIX): parent_port_prefix = ETHERNET_PREFIX else: - assert parent_port.startswith(LAG_PREFIX) + assert parent_port.startswith(SUBINTF_LAG_PREFIX) parent_port_prefix = LAG_PREFIX parent_port_idx_base = self.get_parent_port_index(parent_port) @@ -1254,13 +1345,14 @@ def _test_sub_port_intf_oper_down_with_pending_neigh_route_tasks(self, dvs, sub_ create_intf_on_parent_port=False, vrf_name=None): substrs = sub_port_intf_name.split(VLAN_SUB_INTERFACE_SEPARATOR) parent_port = substrs[0] + parent_port = self.get_parent_port(sub_port_intf_name) vlan_id = substrs[1] assert len(vlan_id) == 2 if parent_port.startswith(ETHERNET_PREFIX): parent_port_prefix = ETHERNET_PREFIX else: - assert parent_port.startswith(LAG_PREFIX) + assert parent_port.startswith(SUBINTF_LAG_PREFIX) parent_port_prefix = LAG_PREFIX parent_port_idx_base = self.get_parent_port_index(parent_port) From a851adca7c18a6883b57c32f58d6bb08fff6ea33 Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Tue, 23 Nov 2021 00:05:14 -0800 Subject: [PATCH 6/8] Addressing missed review comments --- cfgmgr/intfmgr.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 7e97f824a37..1567762ffac 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -307,6 +307,7 @@ void IntfMgr::addHostSubIntf(const string&intf, const string &subIntf, const str std::string IntfMgr::getIntfAdminStatus(const string &alias) { Table *portTable; + string admin = "down"; if (!alias.compare(0, strlen("Eth"), "Eth")) { portTable = &m_statePortTable; @@ -317,10 +318,9 @@ std::string IntfMgr::getIntfAdminStatus(const string &alias) } else { - return "down"; + return admin; } - string admin = "down"; vector temp; portTable->get(alias, temp); @@ -339,6 +339,7 @@ std::string IntfMgr::getIntfAdminStatus(const string &alias) std::string IntfMgr::getIntfMtu(const string &alias) { Table *portTable; + string mtu = "0"; if (!alias.compare(0, strlen("Eth"), "Eth")) { portTable = &m_statePortTable; @@ -349,11 +350,10 @@ std::string IntfMgr::getIntfMtu(const string &alias) } else { - return "0"; + return mtu; } vector temp; portTable->get(alias, temp); - string mtu = "0"; for (auto idx : temp) { const auto &field = fvField(idx); @@ -428,7 +428,9 @@ void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin) /* Avoid duplicate interface admin UP event. */ string curr_admin = m_subIntfList[intf].currAdminStatus; if (curr_admin == "up" && curr_admin == admin) + { continue; + } std::vector fvVector; string subintf_admin = setHostSubIntfAdminStatus(intf, m_subIntfList[intf].adminStatus, admin); m_subIntfList[intf].currAdminStatus = subintf_admin; @@ -624,9 +626,9 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, string vlanId; string parentAlias; size_t found = alias.find(VLAN_SUB_INTERFACE_SEPARATOR); - subIntf subIf(alias); if (found != string::npos) { + subIntf subIf(alias); // alias holds the complete sub interface name // while parentAlias holds the parent port name /*Check if subinterface is valid and sub interface name length is < 15(IFNAMSIZ)*/ @@ -760,6 +762,7 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, if (!parentAlias.empty()) { + subIntf subIf(alias); if (m_subIntfList.find(alias) == m_subIntfList.end()) { if (vlanId == "0" || vlanId.empty()) From 7e51fcf390d5471188369b6033658ddb9d83252c Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Wed, 24 Nov 2021 17:17:28 -0800 Subject: [PATCH 7/8] Fixing subport vs test script for subport under VNET --- tests/test_sub_port_intf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_sub_port_intf.py b/tests/test_sub_port_intf.py index a288829be7a..68b0207c498 100644 --- a/tests/test_sub_port_intf.py +++ b/tests/test_sub_port_intf.py @@ -1083,7 +1083,7 @@ def _test_sub_port_intf_removal(self, dvs, sub_port_intf_name, removal_seq_test= } rif_oid = self.get_newly_created_oid(ASIC_RIF_TABLE, old_rif_oids) #If subintf mtu deleted, it inherits from parent - if vrf_name == self.VRF_UNDER_TEST: + if vrf_name == self.VRF_UNDER_TEST or vrf_name == self.VNET_UNDER_TEST: if parent_port.startswith(ETHERNET_PREFIX): fv_dict["SAI_ROUTER_INTERFACE_ATTR_MTU"] = ETHERNET_PORT_DEFAULT_MTU self.check_sub_port_intf_fvs(self.asic_db, ASIC_RIF_TABLE, rif_oid, fv_dict) From 0612431434d8585ff0b66f91f144384e88a193a2 Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Wed, 24 Nov 2021 17:26:23 -0800 Subject: [PATCH 8/8] Fixing auto merge issue which introduced duplicate is_short_name routine --- tests/test_sub_port_intf.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/tests/test_sub_port_intf.py b/tests/test_sub_port_intf.py index 68b0207c498..748e680e2a1 100644 --- a/tests/test_sub_port_intf.py +++ b/tests/test_sub_port_intf.py @@ -228,22 +228,6 @@ def is_short_name(self, port_name): is_short = True return is_short - def is_short_name(self, port_name): - idx = port_name.find(VLAN_SUB_INTERFACE_SEPARATOR) - parent_port = port_name[:idx] - is_short = False - if parent_port.startswith("Eth"): - if parent_port.startswith(ETHERNET_PREFIX): - is_short = False - else: - is_short = True - elif parent_port.startswith("Po"): - if parent_port.startswith("PortChannel"): - is_short = False - else: - is_short = True - return is_short - def create_sub_port_intf_profile(self, sub_port_intf_name, vrf_name=None): fvs = {ADMIN_STATUS: "up"} idx = sub_port_intf_name.find(VLAN_SUB_INTERFACE_SEPARATOR)