From e86f5aeb182476ff4e0ae855fa3bdb97abf19763 Mon Sep 17 00:00:00 2001 From: Brad House Date: Tue, 17 Dec 2024 14:51:15 -0500 Subject: [PATCH 1/5] 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 dcc71c6600d..ea40bb54882 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 3d6f0365bff..a35795b07c6 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 b9f7a994ddd..5fcbbf4cc1d 100644 --- a/tests/test_portchannel.py +++ b/tests/test_portchannel.py @@ -470,6 +470,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(): From 828b565c4ca1b642a3cec5c9b91fb5e296086901 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 28 Mar 2025 08:39:19 -0400 Subject: [PATCH 2/5] Revert "portmgrd: prevent runtime failure in setting MTU on portchannel member" This reverts commit e86f5aeb182476ff4e0ae855fa3bdb97abf19763. @prsunny won't approve doing this the right way --- cfgmgr/portmgr.cpp | 64 +++------------------------------------ cfgmgr/portmgr.h | 3 +- tests/test_portchannel.py | 45 --------------------------- 3 files changed, 5 insertions(+), 107 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index ea40bb54882..dcc71c6600d 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -7,7 +7,6 @@ #include "exec.h" #include "shellcmd.h" #include -#include using namespace std; using namespace swss; @@ -23,40 +22,7 @@ PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c { } -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) +bool PortMgr::setPortMtu(const string &alias, const string &mtu) { stringstream cmd; string res, cmd_str; @@ -77,12 +43,6 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu, std::unordered_ 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); @@ -168,17 +128,10 @@ 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) { @@ -203,7 +156,6 @@ 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()); @@ -215,6 +167,7 @@ void PortMgr::doTask(Consumer &consumer) { admin_status = DEFAULT_ADMIN_STATUS_STR; mtu = DEFAULT_MTU_STR; + m_portList.insert(alias); } else if (!portOk) @@ -228,10 +181,6 @@ 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") { @@ -243,11 +192,6 @@ 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 @@ -277,14 +221,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()) { - SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str()); setPortAdminStatus(alias, admin_status == "up"); + SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str()); } } else if (op == DEL_COMMAND) diff --git a/cfgmgr/portmgr.h b/cfgmgr/portmgr.h index a35795b07c6..3d6f0365bff 100644 --- a/cfgmgr/portmgr.h +++ b/cfgmgr/portmgr.h @@ -34,10 +34,9 @@ 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, std::unordered_map &lagMembers); + 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); - bool isLagMember(const std::string &alias, std::unordered_map &lagMembers); }; } diff --git a/tests/test_portchannel.py b/tests/test_portchannel.py index 5fcbbf4cc1d..b9f7a994ddd 100644 --- a/tests/test_portchannel.py +++ b/tests/test_portchannel.py @@ -470,51 +470,6 @@ 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(): From 5cd5d27c542ee53ddbb048bd22f40898e911de6f Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 28 Mar 2025 08:46:29 -0400 Subject: [PATCH 3/5] a fix that @prsunny might accept --- cfgmgr/portmgr.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index dcc71c6600d..8e663da9c60 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -37,17 +37,18 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) // 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; + + // Failure happens on PortChannels during system startup. PortChannel enslaves + // members before a default MTU on the port (set in this file, not via the config!). + // Therefore this error is always emitted on startup for portchannel members. + // In theory we shouldn't log in this case, the correct fix is to detect the + // port is part of a portchannel and not even try this but that is rejected for + // possible performance implications. + // + // This can also 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; } bool PortMgr::setPortAdminStatus(const string &alias, const bool up) From 8dbf4f805c110516e555c88c722a8e859129bf03 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 28 Mar 2025 14:55:29 -0400 Subject: [PATCH 4/5] Revert "a fix that @prsunny might accept" This reverts commit 5cd5d27c542ee53ddbb048bd22f40898e911de6f. Nope, @prsunny didn't accept. --- cfgmgr/portmgr.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index 8e663da9c60..dcc71c6600d 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -37,18 +37,17 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) // the port MTU and possibly the port based router interface MTU return writeConfigToAppDb(alias, "mtu", mtu); } - - // Failure happens on PortChannels during system startup. PortChannel enslaves - // members before a default MTU on the port (set in this file, not via the config!). - // Therefore this error is always emitted on startup for portchannel members. - // In theory we shouldn't log in this case, the correct fix is to detect the - // port is part of a portchannel and not even try this but that is rejected for - // possible performance implications. - // - // This can also 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 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) From 194ba54c42003e966a5604cbe27ea9f58876bc65 Mon Sep 17 00:00:00 2001 From: Brad House Date: Fri, 28 Mar 2025 14:59:48 -0400 Subject: [PATCH 5/5] still not acceptable. Try Again. --- cfgmgr/portmgr.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index dcc71c6600d..e3ac0e8590d 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -45,7 +45,14 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) } else { - throw runtime_error(cmd_str + " : " + res); + // This failure can happen on PortChannels during system startup. A PortChannel enslaves + // members before a default MTU is set on the port (set in this file, not via the config!). + // Therefore this error is always emitted on startup for portchannel members. + // In theory we shouldn't log in this case, the correct fix is to detect the + // port is part of a portchannel and not even try this but that is rejected for + // possible performance implications. + SWSS_LOG_WARN("Setting mtu to alias:%s netdev failed (isPortStateOk=true) with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); + return false; } return true; }