Skip to content

Commit

Permalink
portmgrd: prevent runtime failure in setting MTU on portchannel membe…
Browse files Browse the repository at this point in the history 
…r (PR sonic-net#3432)

Prevent setting the PORT MTU on PortChannel members as it will likely fail
and cause portmgrd to exit (The PortChannel itself is where the MTU gets set,
not the PORT).  The current code is setting a default value for an MTU (9100)
even when its a PortChannel member, so this patch prevents that default value
from being set.  Also if a user were to incorrectly specify an MTU on a Port
that is a member of the port channel via `config_db.json` this too would bring
down portmgrd, so catch that and just emit a warning instead.

The YANG model does NOT support checking for this.

In order to not add much overhead for large port count systems, we are also
lazily caching portchannel members and only using that cache on a new port
being brought up or on failure to set an MTU.

The current code *always* attempts to set an MTU on the PORT by setting a
default here:
https://github.com/sonic-net/sonic-swss/blob/c20902f3195b5bf8a941045e131aa1b863b69fd0/cfgmgr/portmgr.cpp#L163-L172
Then applies it here:
https://github.com/sonic-net/sonic-swss/blob/c20902f3195b5bf8a941045e131aa1b863b69fd0/cfgmgr/portmgr.cpp#L222-L226

So it isn't crashing because the user configured the MTU in the PORT config,
but rather because it is done by default when the port is created.   (But it
also would crash if a user set an MTU on a port which is bad since YANG doesn't
do anything to prevent this).

**NOTE**: this only appears to crash on a freshly loaded config at boot, if you
take an existing running configuration and modify it to add a portchannel it
works since the PORT is already provisioned so the default MTU setting path
isn't taken in the above referenced code.

This regression was caused by 8b99543 ... but
just reverting that patch isn't the right solution.  The startup logic does not
appear to be proper as it tries to set a default MTU regardless if its valid
to do so for the port or not.

Logs show this issue which is a critical failure causing the entire switch to
go down:
```
2024 Dec 17 19:26:20.964259 sw1 INFO swss#supervisord: portmgrd RTNETLINK answers: Operation not permitted
2024 Dec 17 19:26:20.965353 sw1 ERR swss#portmgrd: :- main: Runtime error: /sbin/ip link set dev "Ethernet0" mtu "9100" :
2024 Dec 17 19:26:20.967020 sw1 INFO swss#supervisord 2024-12-17 19:26:20,966 WARN exited: portmgrd (exit status 255; not expected)
```

Signed-off-by: Brad House (@bradh352)
  • Loading branch information
@bradh352
bradh352 committed Jan 17, 2025
1 parent 6b41306 commit bf4408c
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 5 deletions.
64 changes: 60 additions & 4 deletions cfgmgr/portmgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "exec.h"
#include "shellcmd.h"
#include <swss/redisutility.h>
#include <unordered_map>

using namespace std;
using namespace swss;
Expand All @@ -22,7 +23,40 @@ PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c
{
}

bool PortMgr::setPortMtu(const string &alias, const string &mtu)
bool PortMgr::isLagMember(const std::string &alias, std::unordered_map<std::string, int> &lagMembers)
{
/* Lag members are lazily loaded on the first call to isLagMember and cached
* within a variable inside of doTask() for future calls.
*/
if (lagMembers.empty())
{
vector<string> keys;
m_cfgLagMemberTable.getKeys(keys);
for (auto key: keys)
{
auto tokens = tokenize(key, config_db_key_delimiter);
std::string member = tokens[1];
if (!member.empty()) {
lagMembers[member] = 1;
}
}

/* placeholder to state we already read lagmembers even though there are
* none */
if (lagMembers.empty()) {
lagMembers["none"] = 1;
}
}

if (lagMembers.find(alias) != lagMembers.end())
{
return true;
}

return false;
}

bool PortMgr::setPortMtu(const string &alias, const string &mtu, std::unordered_map<std::string, int> &lagMembers)
{
stringstream cmd;
string res, cmd_str;
Expand All @@ -43,6 +77,12 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
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 if (isLagMember(alias, lagMembers))
{
// Catch user improperly specified an MTU on the PortChannel
SWSS_LOG_WARN("Setting mtu to alias:%s which is a member of a PortChannel is invalid", alias.c_str());
return false;
}
else
{
throw runtime_error(cmd_str + " : " + res);
Expand Down Expand Up @@ -128,10 +168,17 @@ void PortMgr::doSendToIngressPortTask(Consumer &consumer)

}


void PortMgr::doTask(Consumer &consumer)
{
SWSS_LOG_ENTER();

/* Variable to lazily cache lag members upon first call into isLagMember(). We
* don't want to always query for lag members if not needed, and we also don't
* want to query it for each call to isLagMember() which may be on every port.
*/
std::unordered_map<std::string, int> lagMembers;

auto table = consumer.getTableName();
if (table == CFG_SEND_TO_INGRESS_PORT_TABLE_NAME)
{
Expand All @@ -156,6 +203,7 @@ void PortMgr::doTask(Consumer &consumer)
bool portOk = isPortStateOk(alias);

string admin_status, mtu;
bool isMtuSet = false;
std::vector<FieldValueTuple> field_values;

bool configured = (m_portList.find(alias) != m_portList.end());
Expand All @@ -167,7 +215,6 @@ void PortMgr::doTask(Consumer &consumer)
{
admin_status = DEFAULT_ADMIN_STATUS_STR;
mtu = DEFAULT_MTU_STR;

m_portList.insert(alias);
}
else if (!portOk)
Expand All @@ -181,6 +228,10 @@ void PortMgr::doTask(Consumer &consumer)
if (fvField(i) == "mtu")
{
mtu = fvValue(i);
/* mtu read might be "", so we can't just use .empty() to
* know if its set. There's test cases that depend on this
* logic ... */
isMtuSet = true;
}
else if (fvField(i) == "admin_status")
{
Expand All @@ -192,6 +243,11 @@ void PortMgr::doTask(Consumer &consumer)
}
}

/* Clear default MTU if LAG member as this will otherwise fail. */
if (!isMtuSet && isLagMember(alias, lagMembers)) {
mtu = "";
}

if (!portOk)
{
// Port configuration is handled by the orchagent. If the configuration is written to the APP DB using
Expand Down Expand Up @@ -221,14 +277,14 @@ void PortMgr::doTask(Consumer &consumer)

if (!mtu.empty())
{
setPortMtu(alias, mtu);
SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str());
setPortMtu(alias, mtu, lagMembers);
}

if (!admin_status.empty())
{
setPortAdminStatus(alias, admin_status == "up");
SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str());
setPortAdminStatus(alias, admin_status == "up");
}
}
else if (op == DEL_COMMAND)
Expand Down
3 changes: 2 additions & 1 deletion cfgmgr/portmgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ class PortMgr : public Orch
void doSendToIngressPortTask(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 setPortMtu(const std::string &alias, const std::string &mtu, std::unordered_map<std::string, int> &lagMembers);
bool setPortAdminStatus(const std::string &alias, const bool up);
bool isPortStateOk(const std::string &alias);
bool isLagMember(const std::string &alias, std::unordered_map<std::string, int> &lagMembers);
};

}
45 changes: 45 additions & 0 deletions tests/test_portchannel.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,51 @@ def test_portchannel_member_netdev_oper_status(self, dvs, testlog):
# wait for port-channel deletion
time.sleep(1)

# Make sure if a PortChannel member port tries to set an MTU that it is
# ignored and does not cause a runtime error.
def test_portchannel_member_mtu(self, dvs, testlog):
config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)
app_db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)

# create port-channel
tbl = swsscommon.Table(config_db, "PORTCHANNEL")
fvs = swsscommon.FieldValuePairs([("admin_status", "up"),("mtu", "9100"),("oper_status", "up")])
tbl.set("PortChannel111", fvs)

# set port-channel oper status
tbl = swsscommon.ProducerStateTable(app_db, "LAG_TABLE")
fvs = swsscommon.FieldValuePairs([("admin_status", "up"),("mtu", "9100"),("oper_status", "up")])
tbl.set("PortChannel111", fvs)

# add members to port-channel
tbl = swsscommon.Table(config_db, "PORTCHANNEL_MEMBER")
fvs = swsscommon.FieldValuePairs([("NULL", "NULL")])
tbl.set("PortChannel111|Ethernet0", fvs)
tbl.set("PortChannel111|Ethernet4", fvs)

# wait for port-channel netdev creation
time.sleep(1)

tbl = swsscommon.Table(config_db, "PORT")
fvs = swsscommon.FieldValuePairs([("mtu", "9100")])
tbl.set("Ethernet0", fvs)

# wait for attempted configuration to be applied
time.sleep(1)

# remove port-channel members
tbl = swsscommon.Table(config_db, "PORTCHANNEL_MEMBER")
tbl._del("PortChannel111|Ethernet0")
tbl._del("PortChannel111|Ethernet4")

# remove port-channel
tbl = swsscommon.Table(config_db, "PORTCHANNEL")
tbl._del("PortChannel111")

# wait for port-channel deletion
time.sleep(1)


# Add Dummy always-pass test at end as workaroud
# for issue when Flaky fail on final test it invokes module tear-down before retrying
def test_nonflaky_dummy():
Expand Down

0 comments on commit bf4408c

Please sign in to comment.