Fix issue: there is no retry while creating a RIF which is in removing state (#2679)

* Fix issue: there is no retry while creating a RIF which is in removing state

Author: Junchao-Mellanox

orchagent/intfsorch.cpp

@@ -470,6 +470,11 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre
 {
     SWSS_LOG_ENTER();
 
+    if (m_removingIntfses.find(alias) != m_removingIntfses.end())
+    {
+        return false;
+    }
+
     Port port;
     gPortsOrch->getPort(alias, port);
 
@@ -1076,10 +1081,12 @@ void IntfsOrch::doTask(Consumer &consumer)
             {
                 if (removeIntf(alias, port.m_vr_id, ip_prefix_in_key ? &ip_prefix : nullptr))
                 {
+                    m_removingIntfses.erase(alias);
                     it = consumer.m_toSync.erase(it);
                 }
                 else
                 {
+                    m_removingIntfses.insert(alias);
                     it++;
                     continue;
                 }

orchagent/intfsorch.h

@@ -92,6 +92,8 @@ class IntfsOrch : public Orch
     unique_ptr<ProducerTable> m_flexCounterTable;
     unique_ptr<ProducerTable> m_flexCounterGroupTable;
 
+    std::set<std::string> m_removingIntfses;
+
     std::string getRifFlexCounterTableKey(std::string s);
 
     bool addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackAction);

tests/mock_tests/Makefile.am

@@ -49,6 +49,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 swssnet_ut.cpp \
                 flowcounterrouteorch_ut.cpp \
                 orchdaemon_ut.cpp \
+                intfsorch_ut.cpp \
                 warmrestartassist_ut.cpp \
                 test_failure_handling.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \

tests/mock_tests/intfsorch_ut.cpp

@@ -0,0 +1,330 @@
+#define private public // make Directory::m_values available to clean it.
+#include "directory.h"
+#undef private
+#include "gtest/gtest.h"
+#include "ut_helper.h"
+#include "mock_orchagent_main.h"
+#include "mock_table.h"
+#include <memory>
+#include <vector>
+
+
+
+namespace intfsorch_test
+{
+    using namespace std;
+
+    int create_rif_count = 0;
+    int remove_rif_count = 0;
+    sai_router_interface_api_t *pold_sai_rif_api;
+    sai_router_interface_api_t ut_sai_rif_api;
+
+    sai_status_t _ut_create_router_interface(
+            _Out_ sai_object_id_t *router_interface_id,
+            _In_ sai_object_id_t switch_id,
+            _In_ uint32_t attr_count,
+            _In_ const sai_attribute_t *attr_list)
+    {
+        ++create_rif_count;
+        return SAI_STATUS_SUCCESS;
+    }
+
+    sai_status_t _ut_remove_router_interface(
+            _In_ sai_object_id_t router_interface_id)
+    {
+        ++remove_rif_count;
+        return SAI_STATUS_SUCCESS;
+    }
+
+    struct IntfsOrchTest : public ::testing::Test
+    {
+        shared_ptr<swss::DBConnector> m_app_db;
+        shared_ptr<swss::DBConnector> m_config_db;
+        shared_ptr<swss::DBConnector> m_state_db;
+        shared_ptr<swss::DBConnector> m_chassis_app_db;
+
+        //sai_router_interface_api_t *old_sai_rif_api_ptr;
+
+        //sai_create_router_interface_fn old_create_rif;
+        //sai_remove_router_interface_fn old_remove_rif;
+        void SetUp() override
+        {
+            map<string, string> profile = {
+                { "SAI_VS_SWITCH_TYPE", "SAI_VS_SWITCH_TYPE_BCM56850" },
+                { "KV_DEVICE_MAC_ADDRESS", "20:03:04:05:06:00" }
+            };
+
+            ut_helper::initSaiApi(profile);
+            pold_sai_rif_api = sai_router_intfs_api;
+            ut_sai_rif_api = *sai_router_intfs_api;
+            sai_router_intfs_api = &ut_sai_rif_api;
+
+            sai_router_intfs_api->create_router_interface = _ut_create_router_interface;
+            sai_router_intfs_api->remove_router_interface = _ut_remove_router_interface;
+
+            m_app_db = make_shared<swss::DBConnector>("APPL_DB", 0);
+            m_config_db = make_shared<swss::DBConnector>("CONFIG_DB", 0);
+            m_state_db = make_shared<swss::DBConnector>("STATE_DB", 0);
+            m_chassis_app_db = make_shared<swss::DBConnector>("CHASSIS_APP_DB", 0);
+
+            sai_attribute_t attr;
+
+            attr.id = SAI_SWITCH_ATTR_INIT_SWITCH;
+            attr.value.booldata = true;
+
+            auto status = sai_switch_api->create_switch(&gSwitchId, 1, &attr);
+            ASSERT_EQ(status, SAI_STATUS_SUCCESS);
+
+            // Get switch source MAC address
+            attr.id = SAI_SWITCH_ATTR_SRC_MAC_ADDRESS;
+            status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr);
+
+            ASSERT_EQ(status, SAI_STATUS_SUCCESS);
+
+            gMacAddress = attr.value.mac;
+
+            attr.id = SAI_SWITCH_ATTR_DEFAULT_VIRTUAL_ROUTER_ID;
+            status = sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr);
+
+            ASSERT_EQ(status, SAI_STATUS_SUCCESS);
+
+            gVirtualRouterId = attr.value.oid;
+
+
+            ASSERT_EQ(gCrmOrch, nullptr);
+            gCrmOrch = new CrmOrch(m_config_db.get(), CFG_CRM_TABLE_NAME);
+
+            TableConnector stateDbSwitchTable(m_state_db.get(), "SWITCH_CAPABILITY");
+            TableConnector conf_asic_sensors(m_config_db.get(), CFG_ASIC_SENSORS_TABLE_NAME);
+            TableConnector app_switch_table(m_app_db.get(),  APP_SWITCH_TABLE_NAME);
+
+            vector<TableConnector> switch_tables = {
+                conf_asic_sensors,
+                app_switch_table
+            };
+
+            ASSERT_EQ(gSwitchOrch, nullptr);
+            gSwitchOrch = new SwitchOrch(m_app_db.get(), switch_tables, stateDbSwitchTable);
+
+            // Create dependencies ...
+            TableConnector stateDbBfdSessionTable(m_state_db.get(), STATE_BFD_SESSION_TABLE_NAME);
+            gBfdOrch = new BfdOrch(m_app_db.get(), APP_BFD_SESSION_TABLE_NAME, stateDbBfdSessionTable);
+
+            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 }
+            };
+
+            vector<string> flex_counter_tables = {
+                CFG_FLEX_COUNTER_TABLE_NAME
+            };
+            auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables);
+            gDirectory.set(flexCounterOrch);
+
+            ASSERT_EQ(gPortsOrch, nullptr);
+            gPortsOrch = new PortsOrch(m_app_db.get(), m_state_db.get(), ports_tables, m_chassis_app_db.get());
+
+            vector<string> vnet_tables = {
+                APP_VNET_RT_TABLE_NAME,
+                APP_VNET_RT_TUNNEL_TABLE_NAME
+            };
+
+            vector<string> cfg_vnet_tables = {
+                CFG_VNET_RT_TABLE_NAME,
+                CFG_VNET_RT_TUNNEL_TABLE_NAME
+            };
+
+            auto* vnet_orch = new VNetOrch(m_app_db.get(), APP_VNET_TABLE_NAME);
+            gDirectory.set(vnet_orch);
+            auto* cfg_vnet_rt_orch = new VNetCfgRouteOrch(m_config_db.get(), m_app_db.get(), cfg_vnet_tables);
+            gDirectory.set(cfg_vnet_rt_orch);
+            auto* vnet_rt_orch = new VNetRouteOrch(m_app_db.get(), vnet_tables, vnet_orch);
+            gDirectory.set(vnet_rt_orch);
+            ASSERT_EQ(gVrfOrch, nullptr);
+            gVrfOrch = new VRFOrch(m_app_db.get(), APP_VRF_TABLE_NAME, m_state_db.get(), STATE_VRF_OBJECT_TABLE_NAME);
+            gDirectory.set(gVrfOrch);
+
+            ASSERT_EQ(gIntfsOrch, nullptr);
+            gIntfsOrch = new IntfsOrch(m_app_db.get(), APP_INTF_TABLE_NAME, gVrfOrch, m_chassis_app_db.get());
+
+            const int fdborch_pri = 20;
+
+            vector<table_name_with_pri_t> app_fdb_tables = {
+                { APP_FDB_TABLE_NAME,        FdbOrch::fdborch_pri},
+                { APP_VXLAN_FDB_TABLE_NAME,  FdbOrch::fdborch_pri},
+                { APP_MCLAG_FDB_TABLE_NAME,  fdborch_pri}
+            };
+
+            TableConnector stateDbFdb(m_state_db.get(), STATE_FDB_TABLE_NAME);
+            TableConnector stateMclagDbFdb(m_state_db.get(), STATE_MCLAG_REMOTE_FDB_TABLE_NAME);
+            ASSERT_EQ(gFdbOrch, nullptr);
+            gFdbOrch = new FdbOrch(m_app_db.get(), app_fdb_tables, stateDbFdb, stateMclagDbFdb, gPortsOrch);
+
+            ASSERT_EQ(gNeighOrch, nullptr);
+            gNeighOrch = new NeighOrch(m_app_db.get(), APP_NEIGH_TABLE_NAME, gIntfsOrch, gFdbOrch, gPortsOrch, m_chassis_app_db.get());
+
+            auto* tunnel_decap_orch = new TunnelDecapOrch(m_app_db.get(), APP_TUNNEL_DECAP_TABLE_NAME);
+            vector<string> mux_tables = {
+                CFG_MUX_CABLE_TABLE_NAME,
+                CFG_PEER_SWITCH_TABLE_NAME
+            };
+            auto* mux_orch = new MuxOrch(m_config_db.get(), mux_tables, tunnel_decap_orch, gNeighOrch, gFdbOrch);
+            gDirectory.set(mux_orch);
+
+            ASSERT_EQ(gFgNhgOrch, nullptr);
+            const int fgnhgorch_pri = 15;
+
+            vector<table_name_with_pri_t> fgnhg_tables = {
+                { CFG_FG_NHG,                 fgnhgorch_pri },
+                { CFG_FG_NHG_PREFIX,          fgnhgorch_pri },
+                { CFG_FG_NHG_MEMBER,          fgnhgorch_pri }
+            };
+            gFgNhgOrch = new FgNhgOrch(m_config_db.get(), m_app_db.get(), m_state_db.get(), fgnhg_tables, gNeighOrch, gIntfsOrch, gVrfOrch);
+
+            ASSERT_EQ(gSrv6Orch, nullptr);
+            vector<string> srv6_tables = {
+                APP_SRV6_SID_LIST_TABLE_NAME,
+                APP_SRV6_MY_SID_TABLE_NAME
+            };
+            gSrv6Orch = new Srv6Orch(m_app_db.get(), srv6_tables, gSwitchOrch, gVrfOrch, gNeighOrch);
+
+            // Start FlowCounterRouteOrch
+            static const  vector<string> route_pattern_tables = {
+                CFG_FLOW_COUNTER_ROUTE_PATTERN_TABLE_NAME,
+            };
+            gFlowCounterRouteOrch = new FlowCounterRouteOrch(m_config_db.get(), route_pattern_tables);
+
+            ASSERT_EQ(gRouteOrch, nullptr);
+            const int routeorch_pri = 5;
+            vector<table_name_with_pri_t> route_tables = {
+                { APP_ROUTE_TABLE_NAME,        routeorch_pri },
+                { APP_LABEL_ROUTE_TABLE_NAME,  routeorch_pri }
+            };
+            gRouteOrch = new RouteOrch(m_app_db.get(), route_tables, gSwitchOrch, gNeighOrch, gIntfsOrch, gVrfOrch, gFgNhgOrch, gSrv6Orch);
+            gNhgOrch = new NhgOrch(m_app_db.get(), APP_NEXTHOP_GROUP_TABLE_NAME);
+
+            // Recreate buffer orch to read populated data
+            vector<string> 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 };
+
+            gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);
+
+            Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
+
+            // Get SAI default ports to populate DB
+            auto ports = ut_helper::getInitialSaiPorts();
+
+            // 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()) } });
+            gPortsOrch->addExistingData(&portTable);
+            static_cast<Orch *>(gPortsOrch)->doTask();
+            portTable.set("PortInitDone", { { "lanes", "0" } });
+            gPortsOrch->addExistingData(&portTable);
+            static_cast<Orch *>(gPortsOrch)->doTask();
+        }
+
+        void TearDown() override
+        {
+            gDirectory.m_values.clear();
+
+            delete gCrmOrch;
+            gCrmOrch = nullptr;
+
+            delete gSwitchOrch;
+            gSwitchOrch = nullptr;
+
+            delete gBfdOrch;
+            gBfdOrch = nullptr;
+
+            delete gNeighOrch;
+            gNeighOrch = nullptr;
+
+            delete gFdbOrch;
+            gFdbOrch = nullptr;
+
+            delete gPortsOrch;
+            gPortsOrch = nullptr;
+
+            delete gIntfsOrch;
+            gIntfsOrch = nullptr;
+
+            delete gFgNhgOrch;
+            gFgNhgOrch = nullptr;
+
+            delete gSrv6Orch;
+            gSrv6Orch = nullptr;
+
+            delete gRouteOrch;
+            gRouteOrch = nullptr;
+
+            delete gNhgOrch;
+            gNhgOrch = nullptr;
+
+            delete gBufferOrch;
+            gBufferOrch = nullptr;
+
+            delete gVrfOrch;
+            gVrfOrch = nullptr;
+
+            delete gFlowCounterRouteOrch;
+            gFlowCounterRouteOrch = nullptr;
+
+            sai_router_intfs_api = pold_sai_rif_api;
+            ut_helper::uninitSaiApi();
+        }
+    };
+
+    TEST_F(IntfsOrchTest, IntfsOrchDeleteCreateRetry)
+    {
+        // create a interface
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        entries.push_back({"Ethernet0", "SET", { {"mtu", "9100"}}});
+        auto consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        auto current_create_count = create_rif_count;
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        ASSERT_EQ(current_create_count + 1, create_rif_count);
+
+        // create dependency to the interface
+        gIntfsOrch->increaseRouterIntfsRefCount("Ethernet0");
+
+        // delete the interface, expect retry because dependency exists
+        entries.clear();
+        entries.push_back({"Ethernet0", "DEL", { {} }});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        auto current_remove_count = remove_rif_count;
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        ASSERT_EQ(current_remove_count, remove_rif_count);
+
+        // create the interface again, expect retry because interface is in removing
+        entries.clear();
+        entries.push_back({"Ethernet0", "SET", { {"mtu", "9100"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        current_create_count = create_rif_count;
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        ASSERT_EQ(current_create_count, create_rif_count);
+
+        // remove the dependency, expect delete and create a new one
+        gIntfsOrch->decreaseRouterIntfsRefCount("Ethernet0");
+        current_create_count = create_rif_count;
+        current_remove_count = remove_rif_count;
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        ASSERT_EQ(current_create_count + 1, create_rif_count);
+        ASSERT_EQ(current_remove_count + 1, remove_rif_count);
+    }
+}

tests/mock_tests/orchdaemon_ut.cpp

@@ -39,7 +39,7 @@ namespace orchdaemon_test
 
             ~OrchDaemonTest()
             {
-
+                sai_switch_api = nullptr;
             };
     };