[portsorch] process only updated APP_DB fields when port is already created (#3025)

* [portsorch] process only updated APP_DB fields when port is already created

What I did

Fixing an issue when setting some port attribute in APPL_DB triggers serdes parameters to be re-programmed with port toggling. Made portsorch to handle only those attributes that were pushed to APPL_DB, so that serdes programming happens only by xcvrd's request to do so.

Author: stepanblyschak

orchagent/port/porthlpr.cpp

@@ -910,7 +910,7 @@ bool PortHelper::parsePortConfig(PortConfig &port) const
         }
     }
 
-    return this->validatePortConfig(port);
+    return true;
 }
 
 bool PortHelper::validatePortConfig(PortConfig &port) const

orchagent/port/porthlpr.h

@@ -26,6 +26,7 @@ class PortHelper final
     std::string getAdminStatusStr(const PortConfig &port) const;
 
     bool parsePortConfig(PortConfig &port) const;
+    bool validatePortConfig(PortConfig &port) const;
 
 private:
     std::string getFieldValueStr(const PortConfig &port, const std::string &field) const;
@@ -50,6 +51,4 @@ class PortHelper final
     bool parsePortRole(PortConfig &port, const std::string &field, const std::string &value) const;
     bool parsePortAdminStatus(PortConfig &port, const std::string &field, const std::string &value) const;
     bool parsePortDescription(PortConfig &port, const std::string &field, const std::string &value) const;
-
-    bool validatePortConfig(PortConfig &port) const;
 };

orchagent/portsorch.cpp

@@ -3287,28 +3287,59 @@ void PortsOrch::doPortTask(Consumer &consumer)
 
         if (op == SET_COMMAND)
         {
-            auto &fvMap = m_portConfigMap[key];
-
-            for (const auto &cit : kfvFieldsValues(keyOpFieldsValues))
+            auto parsePortFvs = [&](auto& fvMap) -> bool
             {
-                auto fieldName = fvField(cit);
-                auto fieldValue = fvValue(cit);
+                for (const auto &cit : kfvFieldsValues(keyOpFieldsValues))
+                {
+                    auto fieldName = fvField(cit);
+                    auto fieldValue = fvValue(cit);
 
-                SWSS_LOG_INFO("FIELD: %s, VALUE: %s", fieldName.c_str(), fieldValue.c_str());
+                    SWSS_LOG_INFO("FIELD: %s, VALUE: %s", fieldName.c_str(), fieldValue.c_str());
 
-                fvMap[fieldName] = fieldValue;
-            }
+                    fvMap[fieldName] = fieldValue;
+                }
+
+                pCfg.fieldValueMap = fvMap;
+
+                if (!m_portHlpr.parsePortConfig(pCfg))
+                {
+                    return false;
+                }
 
-            pCfg.fieldValueMap = fvMap;
+                return true;
+            };
 
-            if (!m_portHlpr.parsePortConfig(pCfg))
+            if (m_portList.find(key) == m_portList.end())
             {
-                it = taskMap.erase(it);
-                continue;
+                // Aggregate configuration while the port is not created.
+                auto &fvMap = m_portConfigMap[key];
+
+                if (!parsePortFvs(fvMap))
+                {
+                    it = taskMap.erase(it);
+                    continue;
+                }
+
+                if (!m_portHlpr.validatePortConfig(pCfg))
+                {
+                    it = taskMap.erase(it);
+                    continue;
+                }
+
+                /* Collect information about all received ports */
+                m_lanesAliasSpeedMap[pCfg.lanes.value] = pCfg;
             }
+            else
+            {
+                // Port is already created, gather updated field-values.
+                std::unordered_map<std::string, std::string> fvMap;
 
-            /* Collect information about all received ports */
-            m_lanesAliasSpeedMap[pCfg.lanes.value] = pCfg;
+                if (!parsePortFvs(fvMap))
+                {
+                    it = taskMap.erase(it);
+                    continue;
+                }
+            }
 
             // TODO:
             // Fix the issue below
@@ -3424,6 +3455,9 @@ void PortsOrch::doPortTask(Consumer &consumer)
                 PortSerdesAttrMap_t serdes_attr;
                 getPortSerdesAttr(serdes_attr, pCfg);
 
+                // Saved configured admin status
+                bool admin_status = p.m_admin_state_up;
+
                 if (pCfg.autoneg.is_set)
                 {
                     if (!p.m_an_cfg || p.m_autoneg != pCfg.autoneg.value)
@@ -3975,6 +4009,13 @@ void PortsOrch::doPortTask(Consumer &consumer)
                 /* create host_tx_ready field in state-db */
                 initHostTxReadyState(p);
 
+                // Restore admin status if the port was brought down
+                if (admin_status != p.m_admin_state_up)
+                {
+                    pCfg.admin_status.is_set = true;
+                    pCfg.admin_status.value = admin_status;
+                }
+
                 /* Last step set port admin status */
                 if (pCfg.admin_status.is_set)
                 {

tests/mock_tests/portsorch_ut.cpp

@@ -882,7 +882,6 @@ namespace portsorch_test
         std::deque<KeyOpFieldsValuesTuple> kfvSerdes = {{
             "Ethernet0",
             SET_COMMAND, {
-                { "admin_status", "up"              },
                 { "idriver"     , "0x6,0x6,0x6,0x6" }
             }
         }};
@@ -909,6 +908,35 @@ namespace portsorch_test
         ASSERT_EQ(_sai_set_admin_state_down_count, ++current_sai_api_call_count);
         ASSERT_EQ(_sai_set_admin_state_up_count, current_sai_api_call_count);
 
+        // Configure non-serdes attribute that does not trigger admin state change
+        std::deque<KeyOpFieldsValuesTuple> kfvMtu = {{
+            "Ethernet0",
+            SET_COMMAND, {
+                { "mtu", "1234" },
+            }
+        }};
+
+        // Refill consumer
+        consumer->addToSync(kfvMtu);
+
+        _hook_sai_port_api();
+        current_sai_api_call_count = _sai_set_admin_state_down_count;
+
+        // Apply configuration
+        static_cast<Orch*>(gPortsOrch)->doTask();
+
+        _unhook_sai_port_api();
+
+        ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", p));
+        ASSERT_TRUE(p.m_admin_state_up);
+
+        // Verify mtu is set
+        ASSERT_EQ(p.m_mtu, 1234);
+
+        // Verify no admin-disable then admin-enable
+        ASSERT_EQ(_sai_set_admin_state_down_count, current_sai_api_call_count);
+        ASSERT_EQ(_sai_set_admin_state_up_count, current_sai_api_call_count);
+
         // Dump pending tasks
         std::vector<std::string> taskList;
         gPortsOrch->dumpPendingTasks(taskList);