[portsorch] process PortsOrch tables in specific order (sonic-net#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

@@ -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);
 

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();

orchagent/portsorch.h

@@ -135,9 +135,9 @@ class PortsOrch : public Orch, public Subject
  map<string, uint32_t> m_port_ref_count;
  unordered_set<string> m_pendingPortSet;
 
-
  NotificationConsumer* m_portStatusNotificationConsumer;
 
+ void doTask() override;
  void doTask(Consumer &consumer);
  void doPortTask(Consumer &consumer);
  void doVlanTask(Consumer &consumer);

tests/mock_tests/portsorch_ut.cpp

@@ -383,7 +383,7 @@ namespace portsorch_test
  Port port;
  gPortsOrch->getPort("Ethernet0", port);
 
- auto countersTable = make_shared<Table>(m_counters_db.get(), COUNTERS_TABLE); 
+ auto countersTable = make_shared<Table>(m_counters_db.get(), COUNTERS_TABLE);
  auto dropHandler = make_unique<PfcWdZeroBufferHandler>(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<table_name_with_pri_t> 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<string> 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_API_LAG, SAI_OBJECT_TYPE_LAG_MEMBER>(&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_API_BRIDGE, SAI_OBJECT_TYPE_BRIDGE_PORT>(&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<Orch *>(gPortsOrch)->doTask();
+
+ vector<string> 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<Consumer*>(exec);
+ ts.clear();
+ consumer->dumpPendingTasks(ts);
+ ASSERT_TRUE(ts.empty());
+ }
+
+ ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created
+ }
+
 }