From 745a9ee911a067821214222c901ff274584e6373 Mon Sep 17 00:00:00 2001 From: Brad House Date: Tue, 17 Dec 2024 14:51:15 -0500 Subject: [PATCH] portmgrd: prevent runtime failure in setting MTU on portchannel member 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 8b99543fa274f083d81f6a07442b267741ceede9 ... 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) --- cfgmgr/portmgr.cpp | 64 ++++++++++++++++++++++++++++++++++++--- cfgmgr/portmgr.h | 3 +- tests/test_portchannel.py | 45 +++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 5 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index dcc71c6600..ea40bb5488 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -7,6 +7,7 @@ #include "exec.h" #include "shellcmd.h" #include +#include using namespace std; using namespace swss; @@ -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 &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 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 &lagMembers) { stringstream cmd; string res, cmd_str; @@ -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); @@ -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 lagMembers; + auto table = consumer.getTableName(); if (table == CFG_SEND_TO_INGRESS_PORT_TABLE_NAME) { @@ -156,6 +203,7 @@ void PortMgr::doTask(Consumer &consumer) bool portOk = isPortStateOk(alias); string admin_status, mtu; + bool isMtuSet = false; std::vector field_values; bool configured = (m_portList.find(alias) != m_portList.end()); @@ -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) @@ -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") { @@ -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 @@ -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) diff --git a/cfgmgr/portmgr.h b/cfgmgr/portmgr.h index 3d6f0365bf..a35795b07c 100644 --- a/cfgmgr/portmgr.h +++ b/cfgmgr/portmgr.h @@ -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 &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 &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 &lagMembers); }; } diff --git a/tests/test_portchannel.py b/tests/test_portchannel.py index 0a922e6936..0c6e8a5dcf 100644 --- a/tests/test_portchannel.py +++ b/tests/test_portchannel.py @@ -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():