Port configuration incremental update support

cfgmgr/Makefile.am

@@ -33,7 +33,7 @@ teammgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(CFLA
 teammgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(CFLAGS_ASAN)
 teammgrd_LDADD = $(LDFLAGS_ASAN) $(COMMON_LIBS) $(SAIMETA_LIBS)
 
-portmgrd_SOURCES = portmgrd.cpp portmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h
+portmgrd_SOURCES = portmgrd.cpp portmgr.cpp config_cache.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h
 portmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(CFLAGS_ASAN)
 portmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(CFLAGS_ASAN)
 portmgrd_LDADD = $(LDFLAGS_ASAN) $(COMMON_LIBS) $(SAIMETA_LIBS)

cfgmgr/config_cache.cpp

@@ -0,0 +1,58 @@
+#include "config_cache.h"
+
+using namespace swss;
+
+ConfigCache::ConfigCache(ConfigChangeCb configChangeCb):
+mConfigChangeCb(configChangeCb)
+{}
+
+void ConfigCache::config(const std::string& key, const std::string& field, const std::string& value, void *context)
+{
+    auto iter = mConfigData.find(key);
+    if (iter == mConfigData.end())
+    {
+        if (mConfigChangeCb(key, field, "", value, context))
+        {
+            mConfigData.emplace(key, ConfigEntry({{field, value}}));
+        }
+    }
+    else
+    {
+        auto entry_iter = iter->second.find(field);
+        if (entry_iter == iter->second.end())
+        {
+            if (mConfigChangeCb(key, field, "", value, context))
+            {
+                iter->second.emplace(field, value);
+            }
+        }
+        else
+        {
+            if (mConfigChangeCb(key, field, entry_iter->second, value, context))
+            {
+                entry_iter->second = value;
+            }
+        }
+    }
+}
+
+void ConfigCache::applyDefault(const std::string& key, const ConfigEntry &defaultConfig, void *context)
+{
+    auto &entry = mConfigData[key];
+    for (auto &fv : defaultConfig)
+    {
+        auto iter = entry.find(fv.first);
+        if (iter == entry.end())
+        {
+            if (mConfigChangeCb(key, fv.first, "", fv.second, context))
+            {
+                entry.emplace(fv.first, fv.second);
+            }
+        }
+    }
+}
+
+bool ConfigCache::exist(const std::string& key) const
+{
+    return mConfigData.find(key) != mConfigData.end();
+}

cfgmgr/config_cache.h

@@ -0,0 +1,27 @@
+#pragma once
+#include <map>
+#include <string>
+#include <functional>
+
+namespace swss {
+
+typedef std::function<bool(const std::string&, const std::string&, const std::string&, const std::string &, void *context)> ConfigChangeCb;
+typedef std::map<std::string, std::string> ConfigEntry;
+typedef std::map<std::string, ConfigEntry> ConfigData;
+
+class ConfigCache
+{
+public:
+
+    ConfigCache(ConfigChangeCb configChangeCb);
+
+    void config(const std::string& key, const std::string& field, const std::string& value, void *context = nullptr);
+    void applyDefault(const std::string& key, const ConfigEntry &defaultConfig, void *context = nullptr);
+    bool exist(const std::string& key) const;
+    void remove(const std::string &key) { mConfigData.erase(key); }
+private:
+    ConfigData mConfigData;
+    ConfigChangeCb mConfigChangeCb;
+};
+
+}

cfgmgr/portmgr.cpp

@@ -11,12 +11,18 @@
 using namespace std;
 using namespace swss;
 
+map<std::string, std::string> portDefaultConfig = {
+    {"mtu", DEFAULT_MTU_STR},
+    {"admin_status", DEFAULT_ADMIN_STATUS_STR}
+};
+
 PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) :
         Orch(cfgDb, tableNames),
         m_cfgPortTable(cfgDb, CFG_PORT_TABLE_NAME),
         m_cfgLagMemberTable(cfgDb, CFG_LAG_MEMBER_TABLE_NAME),
         m_statePortTable(stateDb, STATE_PORT_TABLE_NAME),
-        m_appPortTable(appDb, APP_PORT_TABLE_NAME)
+        m_appPortTable(appDb, APP_PORT_TABLE_NAME),
+        m_configCache(std::bind(&PortMgr::onPortConfigChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5))
 {
 }
 
@@ -31,29 +37,9 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)
 
     // Set the port MTU in application database to update both
     // the port MTU and possibly the port based router interface MTU
-    vector<FieldValueTuple> fvs;
-    FieldValueTuple fv("mtu", mtu);
-    fvs.push_back(fv);
-    m_appPortTable.set(alias, fvs);
-
-    return true;
+    return writeConfigToAppDb(alias, "mtu", mtu);
 }
 
-bool PortMgr::setPortTpid(const string &alias, const string &tpid)
-{
-    stringstream cmd;
-    string res;
-
-    // Set the port TPID in application database to update port TPID
-    vector<FieldValueTuple> fvs;
-    FieldValueTuple fv("tpid", tpid);
-    fvs.push_back(fv);
-    m_appPortTable.set(alias, fvs);
-
-    return true;
-}
-
-
 bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
 {
     stringstream cmd;
@@ -63,23 +49,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
     cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down");
     EXEC_WITH_ERROR_THROW(cmd.str(), res);
 
-    vector<FieldValueTuple> fvs;
-    FieldValueTuple fv("admin_status", (up ? "up" : "down"));
-    fvs.push_back(fv);
-    m_appPortTable.set(alias, fvs);
-
-    return true;
-}
-
-bool PortMgr::setPortLearnMode(const string &alias, const string &learn_mode)
-{
-    // Set the port MAC learn mode in application database
-    vector<FieldValueTuple> fvs;
-    FieldValueTuple fv("learn_mode", learn_mode);
-    fvs.push_back(fv);
-    m_appPortTable.set(alias, fvs);
-
-    return true;
+    return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down"));
 }
 
 bool PortMgr::isPortStateOk(const string &alias)
@@ -117,79 +87,91 @@ void PortMgr::doTask(Consumer &consumer)
 
         if (op == SET_COMMAND)
         {
-            if (!isPortStateOk(alias))
-            {
-                SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str());
-                it++;
-                continue;
-            }
-
-            string admin_status, mtu, learn_mode, tpid;
-
-            bool configured = (m_portList.find(alias) != m_portList.end());
+            /* portOk=true indicates that the port has been created in kernel.
+             * We should not call any ip command if portOk=false. However, it is
+             * valid to put port configuration to APP DB which will trigger port creation in kernel.
+             */
+            bool portOk = isPortStateOk(alias);
 
             /* If this is the first time we set port settings
              * assign default admin status and mtu
              */
-            if (!configured)
-            {
-                admin_status = DEFAULT_ADMIN_STATUS_STR;
-                mtu = DEFAULT_MTU_STR;
-
-                m_portList.insert(alias);
-            }
+            bool exist = m_configCache.exist(alias);
 
             for (auto i : kfvFieldsValues(t))
             {
-                if (fvField(i) == "mtu")
-                {
-                    mtu = fvValue(i);
-                }
-                else if (fvField(i) == "admin_status")
-                {
-                    admin_status = fvValue(i);
-                }
-                else if (fvField(i) == "learn_mode")
-                {
-                    learn_mode = fvValue(i);
-                }
-                else if (fvField(i) == "tpid")
-                {
-                    tpid = fvValue(i);
-                }
-            }
-
-            if (!mtu.empty())
-            {
-                setPortMtu(alias, mtu);
-                SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str());
-            }
-
-            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());
-            }
-
-            if (!learn_mode.empty())
-            {
-                setPortLearnMode(alias, learn_mode);
-                SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
+                m_configCache.config(alias, fvField(i), fvValue(i), (void *)&portOk);
             }
 
-            if (!tpid.empty())
+            if (!exist)
             {
-                setPortTpid(alias, tpid);
-                SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
+                // The port does not exist before, it means that it is the first time
+                // to configure this port, try to apply default configuration. Please
+                // not that applyDefault function will NOT override existing configuration
+                // with default value.
+                m_configCache.applyDefault(alias, portDefaultConfig, (void *)&portOk);
             }
         }
         else if (op == DEL_COMMAND)
         {
             SWSS_LOG_NOTICE("Delete Port: %s", alias.c_str());
             m_appPortTable.del(alias);
-            m_portList.erase(alias);
+            m_configCache.remove(alias);
         }
 
-        it = consumer.m_toSync.erase(it);
+        if (m_retryFields.empty())
+        {
+            it = consumer.m_toSync.erase(it);
+        }
+        else
+        {
+            /* There are some fields require retry due to set failure. This is usually because 
+             * port has not been created in kernel so that mtu and admin_status configuration 
+             * cannot be synced between kernel and  ASIC for now. In this case, we put the retry fields 
+             * back to m_toSync and wait for re-visit later.
+             */
+            it->second = KeyOpFieldsValuesTuple{alias, SET_COMMAND, m_retryFields};
+            ++it;
+            m_retryFields.clear();
+        }
     }
 }
+
+bool PortMgr::onPortConfigChanged(const std::string &alias, const std::string &field, const std::string &old_value, const std::string &new_value, void *context)
+{
+    bool portOk = *((bool *)context);
+    if (field == "mtu" && portOk)
+    {
+        setPortMtu(alias, new_value);
+    }
+    else if (field == "admin_status" && portOk)
+    {
+        setPortAdminStatus(alias, new_value == "up");
+    }
+    else
+    {
+        /* For mtu and admin_status, if portOk=false we still save it to APP DB which is 
+         * the same behavior as portsyncd before. 
+         */
+        writeConfigToAppDb(alias, field, new_value);
+    }
+
+    if (!portOk && (field == "mtu" || field == "admin_status"))
+    {
+        m_retryFields.emplace_back(field, new_value);
+        return false;
+    }
+
+    SWSS_LOG_NOTICE("Configure %s %s from %s to %s", alias.c_str(), field.c_str(), old_value.c_str(), new_value.c_str());
+    return true;
+}
+
+bool PortMgr::writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value)
+{
+    vector<FieldValueTuple> fvs;
+    FieldValueTuple fv(field, value);
+    fvs.push_back(fv);
+    m_appPortTable.set(alias, fvs);
+
+    return true;
+}

cfgmgr/portmgr.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "config_cache.h"
 #include "dbconnector.h"
 #include "orch.h"
 #include "producerstatetable.h"
@@ -26,13 +27,14 @@ class PortMgr : public Orch
     Table m_statePortTable;
     ProducerStateTable m_appPortTable;
 
-    std::set<std::string> m_portList;
+    ConfigCache m_configCache;
+    std::vector<FieldValueTuple> m_retryFields;
 
     void doTask(Consumer &consumer);
+    bool onPortConfigChanged(const std::string &alias, const std::string &field, const std::string &old_value, const std::string &new_value, void *context);
+    bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value);
     bool setPortMtu(const std::string &alias, const std::string &mtu);
-    bool setPortTpid(const std::string &alias, const std::string &tpid);
     bool setPortAdminStatus(const std::string &alias, const bool up);
-    bool setPortLearnMode(const std::string &alias, const std::string &learn_mode);
     bool isPortStateOk(const std::string &alias);
 };
 

orchagent/port.h

@@ -87,6 +87,12 @@ class Port
         UNKNOWN
     } ;
 
+    enum AutoNegMode {
+        AUTONEG_NOT_SET = -1,
+        AUTONEG_OFF = 0,
+        AUTONEG_ON = 1
+    };
+
     Port() {};
     Port(std::string alias, Type type) :
             m_alias(alias), m_type(type) {};
@@ -112,7 +118,7 @@ class Port
     uint32_t            m_mtu = DEFAULT_MTU;
     uint32_t            m_speed = 0;    // Mbps
     std::string         m_learn_mode = "hardware";
-    int                 m_autoneg = -1;  // -1 means not set, 0 = disabled, 1 = enabled
+    AutoNegMode         m_autoneg = Port::AutoNegMode::AUTONEG_NOT_SET;
     bool                m_admin_state_up = false;
     bool                m_init = false;
     bool                m_l3_vni = false;
@@ -148,7 +154,7 @@ class Port
     uint32_t  m_up_member_count = 0;
     uint32_t  m_maximum_headroom = 0;
     std::vector<uint32_t> m_adv_speeds;
-    sai_port_interface_type_t m_interface_type;
+    sai_port_interface_type_t m_interface_type = SAI_PORT_INTERFACE_TYPE_NONE;
     std::vector<uint32_t> m_adv_interface_types;
     bool      m_mpls = false;