From ef330732fe8ddef3ebabcac20cdf3d48e0906b01 Mon Sep 17 00:00:00 2001 From: kannansel Date: Mon, 25 Mar 2024 13:36:28 -0500 Subject: [PATCH 1/2] Signed-off-by: kannansel Why ? Static lag support changes in SWSS module sonic-net/SONiC#1039 Patch explanation 1. static lag supported with option roundrobin. 2. tlm_teamd -> teamdctl changes to handle json dump for static lag. 3. test cases -> updated UT:- Test cases 1 Create static port channel with static flag pass pass 2 verify static has option flag true or false pass pass 3 Add static member see the portchannel is up pass pass 4 verify teamd is created with round-robin option by default pass pass 5 Remove last portchannel member check port channel down pass pass 6 Remove portchannel member check port channel still up pass pass 7 verify teamdctl config dump pass pass 8 verify teamdctl state dump pass pass 9 shutdown the portchannel check the kernel state pass pass 10 no shutdown the portchannel check the kernel state pass pass 11 "Check the show output matches the review comment root@sonic:~# show inter port Flags: A - active, I - inactive, Up - up, Dw - Down, N/A - not available, S - selected, D - deselected, * - not synced No. Team Dev Protocol Ports ----- ------------- ----------- ----------------------------------------- 01 PortChannel01 LACP(A)(Up) Ethernet16(S) 02 PortChannel02 NONE(A)(Up) Ethernet48(S) Ethernet64(S) Ethernet32(S) root@sonic:~# 12 teamnl is set to roundrobin pass pass 13 save and reload and verify portchannel is up pass pass 14 "docker restart teamd teamd stopped swss stopped syncd stopped swss started syncd started teamd started" pass pass 15 warm-reboot fails even without any port channel config fail 16 verify teamd settles doesnt hog cpu with 100% cpu usage pass 17 "trying with static port channel config on non supported branches port channel will be configured as LACP." pass Not Supported Options 1. Min links and 2. fall back are not supported --- cfgmgr/teammgr.cpp | 29 +++++++--- cfgmgr/teammgr.h | 2 +- tests/mock_tests/teammgrd/teammgr_ut.cpp | 20 ++++++- tests/test_portchannel.py | 69 ++++++++++++++++++++++++ tlm_teamd/values_store.cpp | 63 ++++++++++++++++++++++ tlm_teamd/values_store.h | 14 +++++ 6 files changed, 188 insertions(+), 9 deletions(-) diff --git a/cfgmgr/teammgr.cpp b/cfgmgr/teammgr.cpp index 36c9d134e1..e7919df024 100644 --- a/cfgmgr/teammgr.cpp +++ b/cfgmgr/teammgr.cpp @@ -253,6 +253,7 @@ void TeamMgr::doLagTask(Consumer &consumer) int min_links = 0; bool fallback = false; bool fast_rate = false; + bool static_lag = false; string admin_status = DEFAULT_ADMIN_STATUS_STR; string mtu = DEFAULT_MTU_STR; string learn_mode; @@ -301,11 +302,16 @@ void TeamMgr::doLagTask(Consumer &consumer) SWSS_LOG_INFO("Get fast_rate `%s`", fast_rate ? "true" : "false"); } + else if (fvField(i) == "static") + { + static_lag = true; + SWSS_LOG_INFO ("static %d", static_lag); + } } if (m_lagList.find(alias) == m_lagList.end()) { - if (addLag(alias, min_links, fallback, fast_rate) == task_need_retry) + if (addLag(alias, min_links, fallback, fast_rate, static_lag) == task_need_retry) { // If LAG creation fails, we need to clean up any potentially orphaned teamd processes removeLag(alias); @@ -562,7 +568,7 @@ bool TeamMgr::setLagLearnMode(const string &alias, const string &learn_mode) return true; } -task_process_status TeamMgr::addLag(const string &alias, int min_links, bool fallback, bool fast_rate) +task_process_status TeamMgr::addLag(const string &alias, int min_links, bool fallback, bool fast_rate, bool static_lag) { SWSS_LOG_ENTER(); @@ -603,11 +609,20 @@ task_process_status TeamMgr::addLag(const string &alias, int min_links, bool fal } } - conf << "'{\"device\":\"" << alias << "\"," - << "\"hwaddr\":\"" << mac_boot.to_string() << "\"," - << "\"runner\":{" - << "\"active\":true," - << "\"name\":\"lacp\""; + if (static_lag) { + conf << "'{\"device\":\"" << alias << "\"," + << "\"hwaddr\":\"" << mac_boot.to_string() << "\"," + << "\"runner\":{" + << "\"name\":\"roundrobin\""; + + } else { + + conf << "'{\"device\":\"" << alias << "\"," + << "\"hwaddr\":\"" << mac_boot.to_string() << "\"," + << "\"runner\":{" + << "\"active\":true," + << "\"name\":\"lacp\""; + } if (min_links != 0) { diff --git a/cfgmgr/teammgr.h b/cfgmgr/teammgr.h index 3c98f87dc5..b65f133763 100644 --- a/cfgmgr/teammgr.h +++ b/cfgmgr/teammgr.h @@ -41,7 +41,7 @@ class TeamMgr : public Orch void doLagMemberTask(Consumer &consumer); void doPortUpdateTask(Consumer &consumer); - task_process_status addLag(const std::string &alias, int min_links, bool fall_back, bool fast_rate); + task_process_status addLag(const std::string &alias, int min_links, bool fall_back, bool fast_rate, bool static_lag); bool removeLag(const std::string &alias); task_process_status addLagMember(const std::string &lag, const std::string &member); bool removeLagMember(const std::string &lag, const std::string &member); diff --git a/tests/mock_tests/teammgrd/teammgr_ut.cpp b/tests/mock_tests/teammgrd/teammgr_ut.cpp index 32f064f552..2743e45004 100644 --- a/tests/mock_tests/teammgrd/teammgr_ut.cpp +++ b/tests/mock_tests/teammgrd/teammgr_ut.cpp @@ -75,4 +75,22 @@ namespace teammgr_ut } ASSERT_EQ(kill_cmd_called, 1); } -} \ No newline at end of file + + TEST_F(TeamMgrTest, testProcessKilledAfterAddStaticFailure) + { + swss::TeamMgr teammgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_lag_tables); + swss::Table cfg_lag_table = swss::Table(m_config_db.get(), CFG_LAG_TABLE_NAME); + cfg_lag_table.set("PortChannel1", { { "admin_status", "up" }, + { "mtu", "9100" }, + { "static", "true" } }); + teammgr.addExistingData(&cfg_lag_table); + teammgr.doTask(); + int kill_cmd_called = 0; + for (auto cmd : mockCallArgs){ + if (cmd.find("kill -TERM 1234") != std::string::npos){ + kill_cmd_called++; + } + } + ASSERT_EQ(kill_cmd_called, 1); + } +} diff --git a/tests/test_portchannel.py b/tests/test_portchannel.py index 0a922e6936..53a3c44911 100644 --- a/tests/test_portchannel.py +++ b/tests/test_portchannel.py @@ -464,6 +464,75 @@ def test_portchannel_member_netdev_oper_status(self, dvs, testlog): # wait for port-channel deletion time.sleep(1) + + def test_static_portchannel_member_netdev_oper_status(self, dvs, testlog): + config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) + state_db = swsscommon.DBConnector(swsscommon.STATE_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"),("static": "true")]) + 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) + + # set netdev oper status + (exitcode, _) = dvs.runcmd("ip link set up dev Ethernet0") + assert exitcode == 0, "ip link set failed" + + (exitcode, _) = dvs.runcmd("ip link set up dev Ethernet4") + assert exitcode == 0, "ip link set failed" + + (exitcode, _) = dvs.runcmd("ip link set dev PortChannel111 carrier on") + assert exitcode == 0, "ip link set failed" + + # verify port-channel members netdev oper status + tbl = swsscommon.Table(state_db, "PORT_TABLE") + status, fvs = tbl.get("Ethernet0") + assert status is True + fvs = dict(fvs) + assert fvs['netdev_oper_status'] == 'up' + + status, fvs = tbl.get("Ethernet4") + assert status is True + fvs = dict(fvs) + assert fvs['netdev_oper_status'] == 'up' + + # wait for port-channel netdev creation + time.sleep(1) + + (exit_code, output) = dvs.runcmd("teamdctl " + "PortChannel111" + " state dump") + port_state_dump = json.loads(output) + runner_name = port_state_dump["setup"]["runner_name"] + assert runner_name == "roundrobin" + + + # 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(): diff --git a/tlm_teamd/values_store.cpp b/tlm_teamd/values_store.cpp index f883d22fd3..0b3ee5aeda 100644 --- a/tlm_teamd/values_store.cpp +++ b/tlm_teamd/values_store.cpp @@ -144,6 +144,47 @@ std::string ValuesStore::get_value(json_t * root, const std::string & path, Valu throw std::runtime_error("Reach the end of the ValuesStore::get_value. Path=" + path); } +/// +/// Extract static lag values for LAG with name lag_name, from the parsed json tree with root, to the temporary storage +/// @param lag_name a name of the LAG +/// @param root a pointer to the parsed json tree +/// @param storage a reference to the temporary storage +/// + +void ValuesStore::extract_values_static(const std::string & lag_name, json_t * root, HashOfRecords & storage) +{ + + const std::string key = "LAG_TABLE|" + lag_name; + Records lag_values; + + for (const auto & p: m_lag_static_paths) + { + const auto & path = p.first; + const auto & type = p.second; + const auto & value = get_value(root, path, type); + lag_values.emplace(path, value); + } + storage.emplace(key, lag_values); + + const auto & ports = get_ports(root); + for (const auto & port: ports) + { + const std::string key = "LAG_MEMBER_TABLE|" + lag_name + "|" + port; + Records member_values; + for (const auto & p: m_member_static_paths) + { + const auto & path = p.first; + const auto & type = p.second; + const std::string full_path = "ports." + port + "." + path; + const auto & value = get_value(root, full_path, type); + member_values.emplace(path, value); + } + storage.emplace(key, member_values); + } + + return; +} + /// /// Extract values for LAG with name lag_name, from the parsed json tree with root, to the temporary storage /// @param lag_name a name of the LAG @@ -155,6 +196,28 @@ void ValuesStore::extract_values(const std::string & lag_name, json_t * root, Ha const std::string key = "LAG_TABLE|" + lag_name; Records lag_values; + + + bool is_static = false; + + for (const auto & p: m_lag_static_paths) + { + const auto & path = p.first; + const auto & type = p.second; + if (path == "setup.runner_name") { + const auto & value = get_value(root, path, type); + if(value == "loadbalance" || + value == "roundrobin") + is_static = true; + break; + } + } + + if (is_static) { + extract_values_static(lag_name, root, storage); + return; + } + for (const auto & p: m_lag_paths) { const auto & path = p.first; diff --git a/tlm_teamd/values_store.h b/tlm_teamd/values_store.h index 7ad353d8ff..e6dbf880fb 100644 --- a/tlm_teamd/values_store.h +++ b/tlm_teamd/values_store.h @@ -41,6 +41,7 @@ class ValuesStore std::vector update_storage(const HashOfRecords & storage); void update_db(const HashOfRecords & storage, const std::vector & keys_to_refresh); void extract_values(const std::string & lag_name, json_t * root, HashOfRecords & storage); + void extract_values_static(const std::string & lag_name, json_t * root, HashOfRecords & storage); HashOfRecords m_storage; // our main storage const swss::DBConnector * m_db; @@ -70,4 +71,17 @@ class ValuesStore { "runner.selected", ValuesStore::json_type::boolean }, { "runner.state", ValuesStore::json_type::string }, }; + const std::vector> m_lag_static_paths = { + { "setup.runner_name", ValuesStore::json_type::string }, + { "setup.kernel_team_mode_name", ValuesStore::json_type::string }, + { "setup.pid", ValuesStore::json_type::integer }, + { "team_device.ifinfo.dev_addr", ValuesStore::json_type::string }, + { "team_device.ifinfo.ifindex", ValuesStore::json_type::integer }, + }; + const std::vector> m_member_static_paths = { + { "ifinfo.dev_addr", ValuesStore::json_type::string }, + { "ifinfo.ifindex", ValuesStore::json_type::integer }, + { "link.up", ValuesStore::json_type::boolean }, + { "link_watches.list.link_watch_0.up", ValuesStore::json_type::boolean }, + }; }; From 7e3fcff220c4b13d0fa02fcf41dd0b2a153cb18e Mon Sep 17 00:00:00 2001 From: kannansel Date: Tue, 26 Mar 2024 08:21:02 -0500 Subject: [PATCH 2/2] Signed-off-by: kannansel UT fixes --- tests/test_portchannel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_portchannel.py b/tests/test_portchannel.py index 53a3c44911..b3b5bf5873 100644 --- a/tests/test_portchannel.py +++ b/tests/test_portchannel.py @@ -472,7 +472,7 @@ def test_static_portchannel_member_netdev_oper_status(self, dvs, testlog): # create port-channel tbl = swsscommon.Table(config_db, "PORTCHANNEL") - fvs = swsscommon.FieldValuePairs([("admin_status", "up"),("mtu", "9100"),("oper_status", "up"),("static": "true")]) + fvs = swsscommon.FieldValuePairs([("admin_status", "up"),("mtu", "9100"),("oper_status", "up"),("static", "true")]) tbl.set("PortChannel111", fvs) # set port-channel oper status