From a67d8af69685113011ffb377c9fcd4a05ae530a8 Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Tue, 20 Jul 2021 11:54:24 +0300 Subject: [PATCH] [202012][portsorch] fix errors when moving port from one lag to another. (#1819) In scenario that is executed in sonic-mgmt in test_po_update.py a portchannel member is deleted from one portchannel and added to another portchannel. It is possible that requests from teamsynd will arrive in different order This reordering happens because teamsyncd has single event handler/selectable TeamSync::TeamPortSync::onChange() per team device so when two of them are ready it is swss::Select implementation detail in which order they are going to be returned. This is a fundamental issue of Producer/ConsumerStateTable, thus orchagent must be aware of this and treat it as normal situation and figure out the right order and not crash or print an errors. Original PR https://github.com/Azure/sonic-swss/pull/1797 **What I did** Check if port is already a lag member beforehand. Added an UT to cover this scenario, this UT verifies that SAI API is not called in this case. Refactored portsorch_ut.cpp by moving out Orchs creation/deletion into SetUp()/TearDown() **Why I did it** To fix errors in log. **How I verified it** Ran test_po_update.py test. --- orchagent/portsorch.cpp | 25 +++- tests/mock_tests/portsorch_ut.cpp | 239 ++++++++++++++++++------------ 2 files changed, 163 insertions(+), 101 deletions(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 126073cdbf..109136b446 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -209,6 +209,12 @@ static char* hostif_vlan_tag[] = { [SAI_HOSTIF_VLAN_TAG_KEEP] = "SAI_HOSTIF_VLAN_TAG_KEEP", [SAI_HOSTIF_VLAN_TAG_ORIGINAL] = "SAI_HOSTIF_VLAN_TAG_ORIGINAL" }; + +static bool isValidPortTypeForLagMember(const Port& port) +{ + return port.m_type == Port::Type::PHY; +} + /* * Initialize PortsOrch * 0) If Gearbox is enabled, then initialize the external PHYs as defined in @@ -1938,7 +1944,7 @@ sai_status_t PortsOrch::removePort(sai_object_id_t port_id) Port port; - /* + /* * Make sure to bring down admin state. * SET would have replaced with DEL */ @@ -3189,6 +3195,15 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) continue; } + /* Fail if a port type is not a valid type for being a LAG member port. + * Erase invalid entry, no need to retry in this case. */ + if (!isValidPortTypeForLagMember(port)) + { + SWSS_LOG_ERROR("LAG member port has to be of type PHY or SYSTEM"); + it = consumer.m_toSync.erase(it); + continue; + } + /* Update a LAG member */ if (op == SET_COMMAND) { @@ -3201,8 +3216,12 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) if (lag.m_members.find(port_alias) == lag.m_members.end()) { - /* Assert the port doesn't belong to any LAG already */ - assert(!port.m_lag_id && !port.m_lag_member_id); + if (port.m_lag_member_id != SAI_NULL_OBJECT_ID) + { + SWSS_LOG_INFO("Port %s is already a LAG member", port.m_alias.c_str()); + it++; + continue; + } if (!addLagMember(lag, port, (status == "enabled"))) { diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 27c26f2ba3..d87edc9c0b 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -1,3 +1,7 @@ +#define private public // make Directory::m_values available to clean it. +#include "directory.h" +#undef private + #include "ut_helper.h" #include "mock_orchagent_main.h" #include "mock_table.h" @@ -33,17 +37,51 @@ namespace portsorch_test virtual void SetUp() override { ::testing_db::reset(); + + // Create dependencies ... + + const int portsorch_base_pri = 40; + + vector ports_tables = { + { APP_PORT_TABLE_NAME, portsorch_base_pri + 5 }, + { APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 }, + { APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri }, + { APP_LAG_TABLE_NAME, portsorch_base_pri + 4 }, + { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri } + }; + + ASSERT_EQ(gPortsOrch, nullptr); + + vector flex_counter_tables = { + CFG_FLEX_COUNTER_TABLE_NAME + }; + auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables); + gDirectory.set(flexCounterOrch); + + gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables); + vector buffer_tables = { APP_BUFFER_POOL_TABLE_NAME, + APP_BUFFER_PROFILE_TABLE_NAME, + APP_BUFFER_QUEUE_TABLE_NAME, + APP_BUFFER_PG_TABLE_NAME, + APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, + APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; + + ASSERT_EQ(gBufferOrch, nullptr); + gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables); } virtual void TearDown() override { ::testing_db::reset(); + delete gPortsOrch; gPortsOrch = nullptr; delete gBufferOrch; gBufferOrch = nullptr; - } + // clear orchs saved in directory + gDirectory.m_values.clear(); + } static void SetUpTestCase() { // Init switch and create dependencies @@ -89,6 +127,7 @@ namespace portsorch_test ut_helper::uninitSaiApi(); } + }; TEST_F(PortsOrchTest, PortReadinessColdBoot) @@ -129,28 +168,7 @@ namespace portsorch_test pgTableCfg.set(ossCfg.str(), { { "profile", "[BUFFER_PROFILE|test_profile]" } }); } - // Create dependencies ... - - const int portsorch_base_pri = 40; - - vector ports_tables = { - { APP_PORT_TABLE_NAME, portsorch_base_pri + 5 }, - { APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 }, - { APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri }, - { APP_LAG_TABLE_NAME, portsorch_base_pri + 4 }, - { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri } - }; - - ASSERT_EQ(gPortsOrch, nullptr); - - vector flex_counter_tables = { - CFG_FLEX_COUNTER_TABLE_NAME - }; - auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables); - - gDirectory.set(flexCounterOrch); - - gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables); + // Recreate buffer orch to read populated data vector buffer_tables = { APP_BUFFER_POOL_TABLE_NAME, APP_BUFFER_PROFILE_TABLE_NAME, APP_BUFFER_QUEUE_TABLE_NAME, @@ -158,7 +176,6 @@ namespace portsorch_test APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; - ASSERT_EQ(gBufferOrch, nullptr); gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables); // Populate pot table with SAI ports @@ -221,7 +238,6 @@ namespace portsorch_test TEST_F(PortsOrchTest, PortReadinessWarmBoot) { - Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); Table pgTable = Table(m_app_db.get(), APP_BUFFER_PG_TABLE_NAME); Table profileTable = Table(m_app_db.get(), APP_BUFFER_PROFILE_TABLE_NAME); @@ -266,30 +282,6 @@ namespace portsorch_test portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); portTable.set("PortInitDone", { { "lanes", "0" } }); - // Create dependencies ... - - const int portsorch_base_pri = 40; - - vector ports_tables = { - { APP_PORT_TABLE_NAME, portsorch_base_pri + 5 }, - { APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 }, - { APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri }, - { APP_LAG_TABLE_NAME, portsorch_base_pri + 4 }, - { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri } - }; - - ASSERT_EQ(gPortsOrch, nullptr); - gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables); - vector buffer_tables = { APP_BUFFER_POOL_TABLE_NAME, - APP_BUFFER_PROFILE_TABLE_NAME, - APP_BUFFER_QUEUE_TABLE_NAME, - APP_BUFFER_PG_TABLE_NAME, - APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, - APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; - - ASSERT_EQ(gBufferOrch, nullptr); - gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables); - // warm start, bake fill refill consumer gBufferOrch->bake(); @@ -336,30 +328,6 @@ namespace portsorch_test // Get SAI default ports to populate DB auto ports = ut_helper::getInitialSaiPorts(); - // Create dependencies ... - - const int portsorch_base_pri = 40; - - vector ports_tables = { - { APP_PORT_TABLE_NAME, portsorch_base_pri + 5 }, - { APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 }, - { APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri }, - { APP_LAG_TABLE_NAME, portsorch_base_pri + 4 }, - { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri } - }; - - ASSERT_EQ(gPortsOrch, nullptr); - gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables); - vector buffer_tables = { APP_BUFFER_POOL_TABLE_NAME, - APP_BUFFER_PROFILE_TABLE_NAME, - APP_BUFFER_QUEUE_TABLE_NAME, - APP_BUFFER_PG_TABLE_NAME, - APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, - APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; - - ASSERT_EQ(gBufferOrch, nullptr); - gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables); - // Populate port table with SAI ports for (const auto &it : ports) { @@ -458,6 +426,106 @@ namespace portsorch_test ts.clear(); } + /* This test checks that a LAG member validation happens on orchagent level + * and no SAI call is executed in case a port requested to be a LAG member + * is already a LAG member. + */ + TEST_F(PortsOrchTest, LagMemberDoesNotCallSAIApiWhenPortIsAlreadyALagMember) + { + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME); + Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); + + // Get SAI default ports to populate DB + auto ports = ut_helper::getInitialSaiPorts(); + + /* + * Next we will prepare some configuration data to be consumed by PortsOrch + * 32 Ports, 2 LAGs, 1 port is LAG member. + */ + + // Populate pot table with SAI ports + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + + // Set PortConfigDone + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { } }); + + lagTable.set("PortChannel999", + { + {"admin_status", "up"}, + {"mtu", "9100"} + } + ); + lagTable.set("PortChannel0001", + { + {"admin_status", "up"}, + {"mtu", "9100"} + } + ); + lagMemberTable.set( + std::string("PortChannel999") + lagMemberTable.getTableNameSeparator() + ports.begin()->first, + { {"status", "enabled"} }); + + // refill consumer + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagTable); + gPortsOrch->addExistingData(&lagMemberTable); + + static_cast(gPortsOrch)->doTask(); + + // check LAG, VLAN tasks were processed + // port table may require one more doTask iteration + for (auto tableName: {APP_LAG_TABLE_NAME, APP_LAG_MEMBER_TABLE_NAME}) + { + vector ts; + auto exec = gPortsOrch->getExecutor(tableName); + auto consumer = static_cast(exec); + ts.clear(); + consumer->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + + // Set first port as a LAG member while this port is still a member of different LAG. + lagMemberTable.set( + std::string("PortChannel0001") + lagMemberTable.getTableNameSeparator() + ports.begin()->first, + { {"status", "enabled"} }); + + // save original api since we will spy + auto orig_lag_api = sai_lag_api; + sai_lag_api = new sai_lag_api_t(); + memcpy(sai_lag_api, orig_lag_api, sizeof(*sai_lag_api)); + + bool lagMemberCreateCalled = false; + + auto lagSpy = SpyOn(&sai_lag_api->create_lag_member); + lagSpy->callFake([&](sai_object_id_t *oid, sai_object_id_t swoid, uint32_t count, const sai_attribute_t * attrs) -> sai_status_t + { + lagMemberCreateCalled = true; + return orig_lag_api->create_lag_member(oid, swoid, count, attrs); + } + ); + + gPortsOrch->addExistingData(&lagMemberTable); + + static_cast(gPortsOrch)->doTask(); + sai_lag_api = orig_lag_api; + + // verify there is a pending task to do. + vector ts; + auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME); + auto consumer = static_cast(exec); + ts.clear(); + consumer->dumpPendingTasks(ts); + ASSERT_FALSE(ts.empty()); + + // verify there was no SAI call executed. + ASSERT_FALSE(lagMemberCreateCalled); + } + /* * The scope of this test is to verify that LAG member is * added to a LAG before any other object on LAG is created, like RIF, bridge port in warm mode. @@ -472,7 +540,6 @@ namespace portsorch_test */ TEST_F(PortsOrchTest, LagMemberIsCreatedBeforeOtherObjectsAreCreatedOnLag) { - Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME); Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME); @@ -482,29 +549,6 @@ namespace portsorch_test // Get SAI default ports to populate DB auto ports = ut_helper::getInitialSaiPorts(); - // Create dependencies ... - const int portsorch_base_pri = 40; - - vector ports_tables = { - { APP_PORT_TABLE_NAME, portsorch_base_pri + 5 }, - { APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 }, - { APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri }, - { APP_LAG_TABLE_NAME, portsorch_base_pri + 4 }, - { APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri } - }; - - ASSERT_EQ(gPortsOrch, nullptr); - gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables); - vector buffer_tables = { APP_BUFFER_POOL_TABLE_NAME, - APP_BUFFER_PROFILE_TABLE_NAME, - APP_BUFFER_QUEUE_TABLE_NAME, - APP_BUFFER_PG_TABLE_NAME, - APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, - APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; - - ASSERT_EQ(gBufferOrch, nullptr); - gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables); - /* * Next we will prepare some configuration data to be consumed by PortsOrch * 32 Ports, 1 LAG, 1 port is LAG member and LAG is in Vlan. @@ -596,5 +640,4 @@ namespace portsorch_test ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created } - }