diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 87da4e6f73ad..4ac36258b91b 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -266,6 +266,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 || port.m_type == Port::Type::SYSTEM); +} + /* * Initialize PortsOrch * 0) If Gearbox is enabled, then initialize the external PHYs as defined in @@ -1758,7 +1764,7 @@ void PortsOrch::getPortSupportedSpeeds(const std::string& alias, sai_object_id_t } // if our guess was wrong, retry with the correct value - speeds.resize(attr.value.u32list.count); + speeds.resize(attr.value.u32list.count); } if (status == SAI_STATUS_SUCCESS) @@ -2243,7 +2249,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 */ @@ -2812,7 +2818,7 @@ void PortsOrch::doPortTask(Consumer &consumer) it = consumer.m_toSync.erase(it); continue; } - + an = autoneg_mode_map[an_str]; if (an != p.m_autoneg) { @@ -2878,7 +2884,7 @@ void PortsOrch::doPortTask(Consumer &consumer) it++; continue; } - + SWSS_LOG_NOTICE("Set port %s speed from %u to %u", alias.c_str(), p.m_speed, speed); p.m_speed = speed; m_portList[alias] = p; @@ -2919,7 +2925,7 @@ void PortsOrch::doPortTask(Consumer &consumer) auto ori_adv_speeds = swss::join(',', p.m_adv_speeds.begin(), p.m_adv_speeds.end()); if (!setPortAdvSpeeds(p.m_port_id, adv_speeds)) { - + SWSS_LOG_ERROR("Failed to set port %s advertised speed from %s to %s", alias.c_str(), ori_adv_speeds.c_str(), adv_speeds_str.c_str()); @@ -3713,6 +3719,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; + } + if (table_name == CHASSIS_APP_LAG_MEMBER_TABLE_NAME) { int32_t lag_switch_id = lag.m_system_lag_info.switch_id; @@ -3746,8 +3761,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"))) { @@ -5567,7 +5586,7 @@ void PortsOrch::getPortSerdesVal(const std::string& val_str, } } -bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str, +bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str, std::vector &speed_values) { SWSS_LOG_ENTER(); @@ -5581,7 +5600,7 @@ bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str, std::string speed_str; std::istringstream iss(val_str); - try + try { while (std::getline(iss, speed_str, ',')) { @@ -5598,31 +5617,31 @@ bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str, return true; } -bool PortsOrch::getPortInterfaceTypeVal(const std::string &s, +bool PortsOrch::getPortInterfaceTypeVal(const std::string &s, sai_port_interface_type_t &interface_type) { SWSS_LOG_ENTER(); auto iter = interface_type_map_for_an.find(s); - if (iter != interface_type_map_for_an.end()) + if (iter != interface_type_map_for_an.end()) { interface_type = interface_type_map_for_an[s]; return true; } - else + else { const std::string &validInterfaceTypes = getValidInterfaceTypes(); - SWSS_LOG_ERROR("Failed to parse interface_type value %s, valid interface type includes: %s", + SWSS_LOG_ERROR("Failed to parse interface_type value %s, valid interface type includes: %s", s.c_str(), validInterfaceTypes.c_str()); return false; } } -bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str, +bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str, std::vector &type_values) { SWSS_LOG_ENTER(); - if (val_str == "all") + if (val_str == "all") { return true; } @@ -5637,7 +5656,7 @@ bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str, valid = getPortInterfaceTypeVal(type_str, interface_type); if (!valid) { const std::string &validInterfaceTypes = getValidInterfaceTypes(); - SWSS_LOG_ERROR("Failed to parse adv_interface_types value %s, valid interface type includes: %s", + SWSS_LOG_ERROR("Failed to parse adv_interface_types value %s, valid interface type includes: %s", val_str.c_str(), validInterfaceTypes.c_str()); return false; } diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 32b1d373ee8a..2c65a42f09ca 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" @@ -36,17 +40,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(), m_state_db.get(), ports_tables, m_chassis_app_db.get()); + 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 @@ -92,6 +130,7 @@ namespace portsorch_test ut_helper::uninitSaiApi(); } + }; TEST_F(PortsOrchTest, PortReadinessColdBoot) @@ -132,27 +171,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(), m_state_db.get(), ports_tables, m_chassis_app_db.get()); + // 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, @@ -160,7 +179,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 @@ -223,7 +241,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); @@ -268,30 +285,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(), m_state_db.get(), ports_tables, m_chassis_app_db.get()); - 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(); @@ -338,30 +331,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(), m_state_db.get(), ports_tables, m_chassis_app_db.get()); - 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) { @@ -460,6 +429,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. @@ -474,7 +543,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); @@ -484,29 +552,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(), m_state_db.get(), ports_tables, m_chassis_app_db.get()); - 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. @@ -598,5 +643,4 @@ namespace portsorch_test ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created } - }