portmgrd: prevent runtime failure in setting MTU on portchannel member (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)

Author: bradh352

cfgmgr/portmgr.cpp

@@ -7,6 +7,7 @@
 #include "exec.h"
 #include "shellcmd.h"
 #include <swss/redisutility.h>
+#include <unordered_map>
 
 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<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;
@@ -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<std::string, int> 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<FieldValueTuple> 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)

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<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);
 };
 
 }

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():