[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

@@ -212,11 +212,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)
@@ -510,20 +510,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
+    }
+
 }