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

[portmgr] Fixed the orchagent crash due to late arrival of notif #2431

Merged
merged 6 commits into from
Sep 12, 2022
Merged

[portmgr] Fixed the orchagent crash due to late arrival of notif #2431

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9700711
Wait until all the attribs are recieved
vivekrnv Aug 31, 2022
28f2fc9
Merge branch 'sonic-net:master' into bulk_write_portmgr
vivekrnv Sep 1, 2022
eb7abf6
fixed the del after set race in portmgrd
vivekrnv Sep 6, 2022
a35480b
Merge branch 'bulk_write_portmgr' of github.com:vivekrnv/sonic-swss i…
vivekrnv Sep 6, 2022
e9c75e1
Removed sleep intervals in the test
vivekrnv Sep 7, 2022
6ea8be8
comment updated
vivekrnv Sep 7, 2022
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
60 changes: 47 additions & 13 deletions cfgmgr/portmgr.cpp
View file
Open in desktop
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 followed by a pending mtu SET notification
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 followed by a pending admin_status SET notification
vivekrnv marked this conversation as resolved.
Show resolved Hide resolved
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);
vivekrnv marked this conversation as resolved.
Show resolved Hide resolved
}

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;
vivekrnv marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions cfgmgr/portmgr.h
View file
Open in desktop
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
View file
Open in desktop
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())
Copy link
Collaborator

@prsunny prsunny Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does this impact VS swss initialization? Seems these two are mandatory on create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is why we are deferring until both of them are available. No, it doesn't. cfg used in VS has speed and lane_set defined, so shouldn't be a problem.

{
/*
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
4 changes: 4 additions & 0 deletions tests/port_dpb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,10 @@ def breakin(self, dvs, port_names):

for cp in child_ports:
assert(cp.exists_in_config_db() == False)
time.sleep(1)
vivekrnv marked this conversation as resolved.
Show resolved Hide resolved
for cp in child_ports:
assert(cp.exists_in_app_db() == False)
time.sleep(1)
for cp in child_ports:
assert(cp.exists_in_asic_db() == False)
#print "Verified child ports are deleted from all DBs"
Expand Down Expand Up @@ -275,9 +277,11 @@ def create_child_ports(self, dvs, p, num_child_ports):
assert(cp.exists_in_config_db() == True)
cp.verify_config_db()
#print "Config DB verification passed"
time.sleep(1)
for cp in child_ports:
assert(cp.exists_in_app_db() == True)
cp.verify_app_db()
time.sleep(1)
#print "APP DB verification passed"
for cp in child_ports:
assert(cp.exists_in_asic_db() == True)
Expand Down