[portsorch] process only updated APP_DB fields when port is already c…

…reated (#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.
edge-core · Aug 8, 2024 · 3f8fbbb · 3f8fbbb
1 parent 3dec9b5
commit 3f8fbbb
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 18 deletions.
diff --git a/orchagent/port/porthlpr.cpp b/orchagent/port/porthlpr.cpp
@@ -1001,7 +1001,7 @@ bool PortHelper::parsePortConfig(PortConfig &port) const
         }
     }
 
-    return this->validatePortConfig(port);
+    return true;
 }
 
 bool PortHelper::validatePortConfig(PortConfig &port) const

diff --git a/orchagent/port/porthlpr.h b/orchagent/port/porthlpr.h
@@ -28,6 +28,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;
@@ -52,6 +53,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;
 };
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -3524,28 +3524,59 @@ void PortsOrch::doPortTask(Consumer &consumer)
         if (op == SET_COMMAND)
         {
             bool apply_port_speed_on_an_change_to_disabled = false;
-            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;
+                pCfg.fieldValueMap = fvMap;
+
+                if (!m_portHlpr.parsePortConfig(pCfg))
+                {
+                    return false;
+                }
 
-            if (!m_portHlpr.parsePortConfig(pCfg))
+                return true;
+            };
+
+            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
@@ -3661,6 +3692,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)
@@ -4242,6 +4276,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)
                 {

diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp
@@ -859,7 +859,6 @@ namespace portsorch_test
         std::deque<KeyOpFieldsValuesTuple> kfvSerdes = {{
             "Ethernet0",
             SET_COMMAND, {
-                { "admin_status", "up"              },
                 { "idriver"     , "0x6,0x6,0x6,0x6" }
             }
         }};
@@ -886,6 +885,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);