From 3f8fbbbc0cd28729cff5c4cbee05f5ddd4c48d61 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Mon, 25 Mar 2024 22:20:47 +0200 Subject: [PATCH] [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. --- orchagent/port/porthlpr.cpp | 2 +- orchagent/port/porthlpr.h | 3 +- orchagent/portsorch.cpp | 69 ++++++++++++++++++++++++------- tests/mock_tests/portsorch_ut.cpp | 30 +++++++++++++- 4 files changed, 86 insertions(+), 18 deletions(-) diff --git a/orchagent/port/porthlpr.cpp b/orchagent/port/porthlpr.cpp index 64c05b2a..f6463b0a 100644 --- 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 index 4bcae7fc..6729a83a 100644 --- 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 index 033934cf..c37248bd 100755 --- 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 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 index fca4f34b..4e9cc3b0 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -859,7 +859,6 @@ namespace portsorch_test std::deque 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 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(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 taskList; gPortsOrch->dumpPendingTasks(taskList);