Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Routed subinterface enhancements #1907

Merged
merged 7 commits into from
Nov 23, 2021
Merged

Routed subinterface enhancements #1907

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e0c8af
Add support for long name and short name routed subinterfaces.
preetham-singh Sep 15, 2021
ed4877b
Addressing review comments:
preetham-singh Nov 12, 2021
3653f38
Merge branch 'master' into subintf
preetham-singh Nov 12, 2021
7d84b06
Addressing left over comments
preetham-singh Nov 12, 2021
386a243
Fixing compilation issue
preetham-singh Nov 16, 2021
e38a62c
UT Fixes and subport Pytest script update
preetham-singh Nov 18, 2021
a851adc
Addressing missed review comments
preetham-singh Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
233 changes: 220 additions & 13 deletions cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -22,6 +24,7 @@ using namespace swss;
#define VRF_MGMT "mgmt"

#define LOOPBACK_DEFAULT_MTU_STR "65536"
#define PORT_MTU_DEFAULT 9100
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use this as being used in other mgr files - #define DEFAULT_MTU_STR "9100"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Changed as part of next commit.


IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) :
Orch(cfgDb, tableNames),
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need the lines 43 to 52?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPL_DB & STATE_DB tables in orch is not subscribed for runtime table change events rather only consumer object(only for get operations). Hence creating intfmgrd(from Orch) object with APPL_DB & STATE_DB tables, intfmgrd does not get events at runtime for APPL_DB table changes.
Hence we need to explicitly register with subscriberStateTable in constructor for APPL_DB and STATE_DB tables to get runtime table add/del/update events.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preetham-singh, APPL_DB object is intended only for one consumer. There are other implications if there are multiple subscribers. We can discuss if you want more info. It cannot be subscribed here as orchagent is the consumer for this table. As I mentioned, this is a config change that you are interested in. So suggest to subscribe for config_db table rather than any other tables.

Copy link

@hasan-brcm hasan-brcm Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prsunny, this is not config change instead the event when portmgr actually sets the mtu and admin state on the interface in kernel. Otherwise, intfmgr tries to set the mtu or admin state on subinterface netdev in kernel and it fails if parent netdev is having lower mtu or is admin-down, respectively. The fix is that portmgr populates mtu/admin fields in the port table in state db, and intfmgr updates the kernel subintf netdevice afterwards. Similar fix is required for LAG.

I agree it will be better to discuss and conclude.

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();
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/getPortAdminStatus/getIntfAdminStatus?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

{
Table *portTable;
if (!alias.compare(0, strlen("Eth"), "Eth"))
{
portTable = &m_statePortTable;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if its a good idea to read this each time from the DBs for parent mtu/admin_status. Do you think its better to cache? @venkatmahalingam , @preetham-singh , any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we can get admin status from cache.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we planning to get the admin_status from cache?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan to do the caching as another PR

}
else if (!alias.compare(0, strlen("Po"), "Po"))
{
portTable = &m_appLagTable;
}
else
{
return "down";
}

string admin = "down";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please have this as the beginning of the function and return admin variable instead of repeating "down" key word

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as part of next commit

vector<FieldValueTuple> 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace port with intf here as well in the func. name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

{
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<FieldValueTuple> temp;
portTable->get(alias, temp);
string mtu = "0";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.. please assign at the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as part of next commit

for (auto idx : temp)
{
const auto &field = fvField(idx);
const auto &value = fvValue(idx);
if (field == "mtu")
{
mtu = value;
}
}
if (mtu.empty())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use { and at other places added in PR

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<FieldValueTuple> 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*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rephrase the comments and fix typos. Also, align the comments.

Copy link
Contributor Author

@preetham-singh preetham-singh Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as part of next commit.

string curr_admin = m_subIntfList[intf].currAdminStatus;
if (curr_admin == "up" && curr_admin == admin)
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have this in braces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as part of next commit

std::vector<FieldValueTuple> 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)
Expand Down Expand Up @@ -459,13 +600,26 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
string vlanId;
string parentAlias;
size_t found = alias.find(VLAN_SUB_INTERFACE_SEPARATOR);
subIntf subIf(alias);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should create subIntf if found is false

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this comment is not addressed

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 = "";
Expand Down Expand Up @@ -515,6 +669,10 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
{
ipv6_link_local_mode = value;
}
else if (field == "vlan")
{
vlanId = value;
}
}

if (op == SET_COMMAND)
Expand Down Expand Up @@ -580,6 +738,11 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& 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);
Expand All @@ -590,25 +753,33 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& 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())
Expand All @@ -619,13 +790,20 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& 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);
Expand Down Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not subscribe for events from APP_DB here for a table which is already subscribed by Orchagent. I think what you want here is the config_db entry for both Port and LAG

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per offline discussion, moving to STATE_PORT_TABLE and STATE_LAG_TABLE for parent interface and and mtu change events. Updated same as part of next commit.

{
doPortTableTask(kfvKey(t), kfvFieldsValues(t), kfvOp(t));
}
else
{
vector<string> keys = tokenize(kfvKey(t), config_db_key_delimiter);
const vector<FieldValueTuple>& data = kfvFieldsValues(t);
string op = kfvOp(t);
Expand Down Expand Up @@ -827,6 +1010,7 @@ void IntfMgr::doTask(Consumer &consumer)
{
SWSS_LOG_ERROR("Invalid key %s", kfvKey(t).c_str());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix alignment


it = consumer.m_toSync.erase(it);
}
Expand All @@ -836,3 +1020,26 @@ void IntfMgr::doTask(Consumer &consumer)
setWarmReplayDoneState();
}
}

void IntfMgr::doPortTableTask(const string& key, vector<FieldValueTuple> 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);
}
}
}
}
Loading