From 770e617af1da11fb48c22dfacf6192c03e4a358d Mon Sep 17 00:00:00 2001 From: Stepan Blyshchak <38952541+stepanblyschak@users.noreply.github.com> Date: Wed, 15 Jan 2020 22:10:45 +0200 Subject: [PATCH] [portsorch] process PortsOrch tables in specific order (#1108) * [portsorch] process PortsOrch tables in specific order * [portsorch] fix other tables not drained and make tableOrder constexpr * [portsorch_ut] fix test according to fix in portsorch Don't care if PORT_TABLE was not processed fully, we care about LAG, LAG_TABLE, VLAN and VLAN_TABLE. * [tests] fix ut after unsuccessful conflict resolution --- orchagent/orchdaemon.cpp | 23 +++-- orchagent/portsorch.cpp | 28 ++++++ orchagent/portsorch.h | 2 +- tests/mock_tests/portsorch_ut.cpp | 142 +++++++++++++++++++++++++++++- 4 files changed, 180 insertions(+), 15 deletions(-) diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index 756b8a35eb3d..8482c0ac850f 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -211,11 +211,11 @@ bool OrchDaemon::init() * The order of the orch list is important for state restore of warm start and * the queued processing in m_toSync map after gPortsOrch->allPortsReady() is set. * - * For the multiple consumers in ports_tables, tasks for LAG_TABLE is processed before VLAN_TABLE - * when iterating ConsumerMap. - * That is ensured implicitly by the order of map key, "LAG_TABLE" is smaller than "VLAN_TABLE" in lexicographic order. + * For the multiple consumers in Orchs, tasks in a table which name is smaller in lexicographic order are processed first + * when iterating ConsumerMap. This is ensured implicitly by the order of keys in ordered map. + * For cases when Orch has to process tables in specific order, like PortsOrch during warm start, it has to override Orch::doTask() */ - m_orchList = { gSwitchOrch, gCrmOrch, gBufferOrch, gPortsOrch, gIntfsOrch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, wm_orch, policer_orch, sflow_orch, debug_counter_orch}; + m_orchList = { gSwitchOrch, gCrmOrch, gPortsOrch, gBufferOrch, gIntfsOrch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, wm_orch, policer_orch, sflow_orch, debug_counter_orch}; bool initialize_dtel = false; if (platform == BFN_PLATFORM_SUBSTRING || platform == VS_PLATFORM_SUBSTRING) @@ -496,20 +496,17 @@ bool OrchDaemon::warmRestoreAndSyncUp() } /* - * Four iterations are needed. + * Three iterations are needed. * - * First iteration: switchorch, Port init/hostif create part of portorch. + * First iteration: switchorch, Port init/hostif create part of portorch, buffers configuration * - * Second iteratoin: gBufferOrch which requires port created, - * then port speed/mtu/fec_mode/pfc_asym/admin_status config. + * Second iteratoin: port speed/mtu/fec_mode/pfc_asym/admin_status config, + * other orch(s) which wait for port to become ready. * - * Third iteration: other orch(s) which wait for port init done. - * - * Fourth iteration: Drain remaining data that are out of order like LAG_MEMBER_TABLE and - * VLAN_MEMBER_TABLE since they were checked before LAG_TABLE and VLAN_TABLE within gPortsOrch. + * Third iteration: Drain remaining data that are out of order. */ - for (auto it = 0; it < 4; it++) + for (auto it = 0; it < 3; it++) { SWSS_LOG_DEBUG("The current iteration is %d", it); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index f5d616323c0a..ab9c37b371b3 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2556,6 +2556,34 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) } } +void PortsOrch::doTask() +{ + constexpr auto tableOrder = { + APP_PORT_TABLE_NAME, + APP_LAG_TABLE_NAME, + APP_LAG_MEMBER_TABLE_NAME, + APP_VLAN_TABLE_NAME, + APP_VLAN_MEMBER_TABLE_NAME, + }; + + for (auto tableName: tableOrder) + { + auto consumer = getExecutor(tableName); + consumer->drain(); + } + + // drain remaining tables + for (auto& it: m_consumerMap) + { + auto tableName = it.first; + auto consumer = it.second.get(); + if (find(tableOrder.begin(), tableOrder.end(), tableName) == tableOrder.end()) + { + consumer->drain(); + } + } +} + void PortsOrch::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 6f314d6bffe7..a1a99a94062e 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -135,9 +135,9 @@ class PortsOrch : public Orch, public Subject map m_port_ref_count; unordered_set m_pendingPortSet; - NotificationConsumer* m_portStatusNotificationConsumer; + void doTask() override; void doTask(Consumer &consumer); void doPortTask(Consumer &consumer); void doVlanTask(Consumer &consumer); diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 0bd88bc0fad5..0803c0300f83 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -383,7 +383,7 @@ namespace portsorch_test Port port; gPortsOrch->getPort("Ethernet0", port); - auto countersTable = make_shared(m_counters_db.get(), COUNTERS_TABLE); + auto countersTable = make_shared
(m_counters_db.get(), COUNTERS_TABLE); auto dropHandler = make_unique(port.m_port_id, port.m_queue_ids[3], 3, countersTable); // Create test buffer pool @@ -432,4 +432,144 @@ namespace portsorch_test ASSERT_TRUE(ts.empty()); // PG should be proceesed now ts.clear(); } + + /* + * 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. + * For objects like RIF which are created by a different Orch we know that they will wait until + * allPortsReady(), so we can guaranty they won't be created if PortsOrch can process ports, lags, + * vlans in single doTask(). + * If objects are created in PortsOrch, like bridge port, we will spy on SAI API to verify they are + * not called before create_lag_member. + * This is done like this because of limitation on Mellanox platform that does not allow to create objects + * on LAG before at least one LAG members is added in warm reboot. Later this will be fixed. + * + */ + 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); + Table vlanTable = Table(m_app_db.get(), APP_VLAN_TABLE_NAME); + Table vlanMemberTable = Table(m_app_db.get(), APP_VLAN_MEMBER_TABLE_NAME); + + // 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 = { CFG_BUFFER_POOL_TABLE_NAME, + CFG_BUFFER_PROFILE_TABLE_NAME, + CFG_BUFFER_QUEUE_TABLE_NAME, + CFG_BUFFER_PG_TABLE_NAME, + CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME, + CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME }; + + ASSERT_EQ(gBufferOrch, nullptr); + gBufferOrch = new BufferOrch(m_config_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. + */ + + // 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("PortChannel0001", + { + {"admin_status", "up"}, + {"mtu", "9100"} + } + ); + lagMemberTable.set( + std::string("PortChannel0001") + lagMemberTable.getTableNameSeparator() + ports.begin()->first, + { {"status", "enabled"} }); + vlanTable.set("Vlan5", + { + {"admin_status", "up"}, + {"mtu", "9100"} + } + ); + vlanMemberTable.set( + std::string("Vlan5") + vlanMemberTable.getTableNameSeparator() + std::string("PortChannel0001"), + { {"tagging_mode", "untagged"} } + ); + + // refill consumer + gPortsOrch->addExistingData(&portTable); + gPortsOrch->addExistingData(&lagTable); + gPortsOrch->addExistingData(&lagMemberTable); + gPortsOrch->addExistingData(&vlanTable); + gPortsOrch->addExistingData(&vlanMemberTable); + + // 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)); + + auto orig_bridge_api = sai_bridge_api; + sai_bridge_api = new sai_bridge_api_t(); + memcpy(sai_bridge_api, orig_bridge_api, sizeof(*sai_bridge_api)); + + bool bridgePortCalled = false; + bool bridgePortCalledBeforeLagMember = 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 { + if (bridgePortCalled) { + bridgePortCalledBeforeLagMember = true; + } + return orig_lag_api->create_lag_member(oid, swoid, count, attrs); + } + ); + + auto bridgeSpy = SpyOn(&sai_bridge_api->create_bridge_port); + bridgeSpy->callFake([&](sai_object_id_t *oid, sai_object_id_t swoid, uint32_t count, const sai_attribute_t * attrs) -> sai_status_t { + bridgePortCalled = true; + return orig_bridge_api->create_bridge_port(oid, swoid, count, attrs); + } + ); + + static_cast(gPortsOrch)->doTask(); + + vector ts; + + // check LAG, VLAN tasks were proceesed + // port table may require one more doTask iteration + for (auto tableName: { + APP_LAG_TABLE_NAME, + APP_LAG_MEMBER_TABLE_NAME, + APP_VLAN_TABLE_NAME, + APP_VLAN_MEMBER_TABLE_NAME}) + { + auto exec = gPortsOrch->getExecutor(tableName); + auto consumer = static_cast(exec); + ts.clear(); + consumer->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + + ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created + } + }