Skip to content

Commit

Permalink
[portmgr] Fixed the orchagent crash due to late arrival of notif (#2431)
Browse files Browse the repository at this point in the history
Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>
Bulk write to APP_DB i.e. alias, lanes, speed must be read through one notification by orchagent during create_port
Handled a race condition in portmgrd which tries to immediately apply a mtu/admin_status SET notif after a DEL causing it to crash
  • Loading branch information
vivekrnv authored Sep 12, 2022
1 parent b62c716 commit 43cc486
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 13 deletions.
60 changes: 47 additions & 13 deletions cfgmgr/portmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,55 @@ PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
bool PortMgr::setPortMtu(const string &alias, const string &mtu)
{
stringstream cmd;
string res;
string res, cmd_str;

// ip link set dev <port_name> mtu <mtu>
cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu);
EXEC_WITH_ERROR_THROW(cmd.str(), res);

// Set the port MTU in application database to update both
// the port MTU and possibly the port based router interface MTU
return writeConfigToAppDb(alias, "mtu", mtu);
cmd_str = cmd.str();
int ret = swss::exec(cmd_str, res);
if (!ret)
{
// Set the port MTU in application database to update both
// the port MTU and possibly the port based router interface MTU
return writeConfigToAppDb(alias, "mtu", mtu);
}
else if (!isPortStateOk(alias))
{
// Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notif
SWSS_LOG_WARN("Setting mtu to alias:%s netdev failed with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str());
return false;
}
else
{
throw runtime_error(cmd_str + " : " + res);
}
return true;
}

bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
{
stringstream cmd;
string res;
string res, cmd_str;

// ip link set dev <port_name> [up|down]
cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down");
EXEC_WITH_ERROR_THROW(cmd.str(), res);

return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down"));
cmd_str = cmd.str();
int ret = swss::exec(cmd_str, res);
if (!ret)
{
return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down"));
}
else if (!isPortStateOk(alias))
{
// Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notification
SWSS_LOG_WARN("Setting admin_status to alias:%s netdev failed with cmd%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str());
return false;
}
else
{
throw runtime_error(cmd_str + " : " + res);
}
return true;
}

bool PortMgr::isPortStateOk(const string &alias)
Expand Down Expand Up @@ -124,10 +152,9 @@ void PortMgr::doTask(Consumer &consumer)
}
}

for (auto &entry : field_values)
if (field_values.size())
{
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());
writeConfigToAppDb(alias, field_values);
}

if (!portOk)
Expand All @@ -136,6 +163,7 @@ void PortMgr::doTask(Consumer &consumer)

writeConfigToAppDb(alias, "mtu", mtu);
writeConfigToAppDb(alias, "admin_status", admin_status);
/* Retry setting these params after the netdev is created */
field_values.clear();
field_values.emplace_back("mtu", mtu);
field_values.emplace_back("admin_status", admin_status);
Expand Down Expand Up @@ -176,3 +204,9 @@ bool PortMgr::writeConfigToAppDb(const std::string &alias, const std::string &fi

return true;
}

bool PortMgr::writeConfigToAppDb(const std::string &alias, std::vector<FieldValueTuple> &field_values)
{
m_appPortTable.set(alias, field_values);
return true;
}
1 change: 1 addition & 0 deletions cfgmgr/portmgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class PortMgr : public Orch

void doTask(Consumer &consumer);
bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value);
bool writeConfigToAppDb(const std::string &alias, std::vector<FieldValueTuple> &field_values);
bool setPortMtu(const std::string &alias, const std::string &mtu);
bool setPortAdminStatus(const std::string &alias, const bool up);
bool isPortStateOk(const std::string &alias);
Expand Down
9 changes: 9 additions & 0 deletions orchagent/portsorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2615,6 +2615,15 @@ bool PortsOrch::addPort(const set<int> &lane_set, uint32_t speed, int an, string
{
SWSS_LOG_ENTER();

if (!speed || lane_set.empty())
{
/*
speed and lane list are mandatory attributes for the initial create_port call
This check is required because the incoming notifs may not be atomic
*/
return true;
}

vector<uint32_t> lanes(lane_set.begin(), lane_set.end());

sai_attribute_t attr;
Expand Down

0 comments on commit 43cc486

Please sign in to comment.