From c9ae6aa982ee913465832fc01d8f21d60d66df4b Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Thu, 2 Mar 2023 01:29:45 +0800 Subject: [PATCH] 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 --- orchagent/intfsorch.cpp | 7 + orchagent/intfsorch.h | 2 + tests/mock_tests/Makefile.am | 1 + tests/mock_tests/intfsorch_ut.cpp | 330 +++++++++++++++++++++ tests/mock_tests/orchdaemon_ut.cpp | 2 +- tests/mock_tests/test_failure_handling.cpp | 9 + 6 files changed, 350 insertions(+), 1 deletion(-) create mode 100644 tests/mock_tests/intfsorch_ut.cpp diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index adc2aac9f480..29ea4102bbca 100644 --- a/orchagent/intfsorch.cpp +++ b/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; } diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 6b4942897efa..ea15ada14b32 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -92,6 +92,8 @@ class IntfsOrch : public Orch unique_ptr m_flexCounterTable; unique_ptr m_flexCounterGroupTable; + std::set m_removingIntfses; + std::string getRifFlexCounterTableKey(std::string s); bool addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackAction); diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 8adda0be11a3..ed996b098cc9 100644 --- a/tests/mock_tests/Makefile.am +++ b/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 \ diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp new file mode 100644 index 000000000000..60041520bd0a --- /dev/null +++ b/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 +#include + + + +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 m_app_db; + shared_ptr m_config_db; + shared_ptr m_state_db; + shared_ptr 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 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("APPL_DB", 0); + m_config_db = make_shared("CONFIG_DB", 0); + m_state_db = make_shared("STATE_DB", 0); + m_chassis_app_db = make_shared("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 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 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 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 vnet_tables = { + APP_VNET_RT_TABLE_NAME, + APP_VNET_RT_TUNNEL_TABLE_NAME + }; + + vector 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 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 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 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 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 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 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 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(gPortsOrch)->doTask(); + portTable.set("PortInitDone", { { "lanes", "0" } }); + gPortsOrch->addExistingData(&portTable); + static_cast(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 entries; + entries.push_back({"Ethernet0", "SET", { {"mtu", "9100"}}}); + auto consumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + auto current_create_count = create_rif_count; + static_cast(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(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + auto current_remove_count = remove_rif_count; + static_cast(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(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + consumer->addToSync(entries); + current_create_count = create_rif_count; + static_cast(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(gIntfsOrch)->doTask(); + ASSERT_EQ(current_create_count + 1, create_rif_count); + ASSERT_EQ(current_remove_count + 1, remove_rif_count); + } +} \ No newline at end of file diff --git a/tests/mock_tests/orchdaemon_ut.cpp b/tests/mock_tests/orchdaemon_ut.cpp index ab3e4a226cb6..3a2d2169f885 100644 --- a/tests/mock_tests/orchdaemon_ut.cpp +++ b/tests/mock_tests/orchdaemon_ut.cpp @@ -39,7 +39,7 @@ namespace orchdaemon_test ~OrchDaemonTest() { - + sai_switch_api = nullptr; }; }; diff --git a/tests/mock_tests/test_failure_handling.cpp b/tests/mock_tests/test_failure_handling.cpp index be53e0fd5f68..7381f4015ee2 100644 --- a/tests/mock_tests/test_failure_handling.cpp +++ b/tests/mock_tests/test_failure_handling.cpp @@ -28,8 +28,15 @@ namespace saifailure_test void _hook_sai_switch_api() { + map profile = { + { "SAI_VS_SWITCH_TYPE", "SAI_VS_SWITCH_TYPE_BCM56850" }, + { "KV_DEVICE_MAC_ADDRESS", "20:03:04:05:06:00" } + }; + + ut_helper::initSaiApi(profile); ut_sai_switch_api = *sai_switch_api; pold_sai_switch_api = sai_switch_api; + ut_sai_switch_api.set_switch_attribute = _ut_stub_sai_set_switch_attribute; sai_switch_api = &ut_sai_switch_api; } @@ -37,6 +44,7 @@ namespace saifailure_test void _unhook_sai_switch_api() { sai_switch_api = pold_sai_switch_api; + ut_helper::uninitSaiApi(); } TEST_F(SaiFailureTest, handleSaiFailure) @@ -44,6 +52,7 @@ namespace saifailure_test _hook_sai_switch_api(); _sai_syncd_notifications_count = (uint32_t*)mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); + _sai_syncd_notification_event = (int32_t*)mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); *_sai_syncd_notifications_count = 0;