From ed4877b50793b3610ba8191179a441b95f2dcf8c Mon Sep 17 00:00:00 2001 From: Preetham Singh Date: Thu, 11 Nov 2021 22:38:11 -0800 Subject: [PATCH] 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 d55f6b9a52..fec8b2f96c 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 f50ca05aa6..d50d9ef177 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 f22e86e28e..65c6e3d36f 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 cde2c9168e..8fc43ca47f 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 d4f3627203..b52ffd7abd 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 687acf8b34..4d1d0a463f 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 c1ae7603fa..f9e4319e24 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 08df3ff9ec..4a2b351ee0 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 846d474a6c..661da4ee64 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 406953e312..deb5d84129 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 5fed8001a4..169edb1238 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 \