From 9d868a3ee083f5b1d821f2c50d061d4491091af4 Mon Sep 17 00:00:00 2001 From: Prince Date: Tue, 21 Sep 2021 18:53:36 +0000 Subject: [PATCH 1/8] Cache route for single nexthop for faster retrieval --- orchagent/routeorch.cpp | 120 ++++++++++++++++++++++++++++++---------- orchagent/routeorch.h | 17 ++++++ 2 files changed, 108 insertions(+), 29 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index d54e205ded..176ff85462 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1317,47 +1317,87 @@ bool RouteOrch::removeNextHopGroup(const NextHopGroupKey &nexthops) return true; } +void RouteOrch::addNextHopRoute(const NextHopKey& nextHop, const RouteKey& routeKey) +{ + auto it = m_nextHops.find((nextHop)); + + if (it != m_nextHops.end()) + { + if (it->second.find(routeKey) != it->second.end()) + { + SWSS_LOG_INFO("Route already present in nh table %s", + routeKey.prefix.to_string().c_str()); + return; + } + + it->second.insert(routeKey); + } + else + { + set routes; + routes.insert(routeKey); + m_nextHops.insert(make_pair(nextHop, routes)); + } +} + +void RouteOrch::removeNextHopRoute(const NextHopKey& nextHop, const RouteKey& routeKey) +{ + auto it = m_nextHops.find((nextHop)); + + if (it != m_nextHops.end()) + { + if (it->second.find(routeKey) == it->second.end()) + { + SWSS_LOG_INFO("Route not present in nh table %s", + routeKey.prefix.to_string().c_str()); + return; + } + + it->second.erase(routeKey); + if (it->second.empty()) + { + m_nextHops.erase(nextHop); + } + } +} + bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRoutes) { numRoutes = 0; + auto it = m_nextHops.find((nextHop)); + + if (it == m_nextHops.end()) + { + SWSS_LOG_INFO("No routes found for NH %s", nextHop.ip_address.to_string().c_str()); + return true; + } + sai_route_entry_t route_entry; sai_attribute_t route_attr; sai_object_id_t next_hop_id; - for (auto rt_table : m_syncdRoutes) + auto rt = it->second.begin(); + while(rt != it->second.end()) { - for (auto rt_entry : rt_table.second) - { - // Skip routes with ecmp nexthops - if (rt_entry.second.getSize() > 1) - { - continue; - } + SWSS_LOG_INFO("Updating route %s", (*rt).prefix.to_string().c_str()); + next_hop_id = m_neighOrch->getNextHopId(nextHop); - if (rt_entry.second.contains(nextHop)) - { - SWSS_LOG_INFO("Updating route %s during nexthop status change", - rt_entry.first.to_string().c_str()); - next_hop_id = m_neighOrch->getNextHopId(nextHop); + route_entry.vr_id = (*rt).vrf_id; + route_entry.switch_id = gSwitchId; + copy(route_entry.destination, (*rt).prefix); - route_entry.vr_id = rt_table.first; - route_entry.switch_id = gSwitchId; - copy(route_entry.destination, rt_entry.first); + route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; + route_attr.value.oid = next_hop_id; - route_attr.id = SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID; - route_attr.value.oid = next_hop_id; - - sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_ERROR("Failed to update route %s, rv:%d", - rt_entry.first.to_string().c_str(), status); - return false; - } - - ++numRoutes; - } + sai_status_t status = sai_route_api->set_route_entry_attribute(&route_entry, &route_attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to update route %s, rv:%d", (*rt).prefix.to_string().c_str(), status); + return false; } + + ++numRoutes; + ++rt; } return true; @@ -1856,6 +1896,12 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey SWSS_LOG_NOTICE("Update overlay Nexthop %s", ol_nextHops.to_string().c_str()); m_bulkNhgReducedRefCnt.emplace(ol_nextHops, vrf_id); } + else if (ol_nextHops.getSize() == 1) + { + RouteKey r_key = { vrf_id, ipPrefix }; + auto nexthop = NextHopKey(ol_nextHops.to_string()); + removeNextHopRoute(nexthop, r_key); + } } if (blackhole) @@ -1878,6 +1924,16 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } + if (nextHops.getSize() == 1) + { + RouteKey r_key = { vrf_id, ipPrefix }; + auto nexthop = NextHopKey(nextHops.to_string()); + if (!nexthop.ip_address.isZero()) + { + addNextHopRoute(nexthop, r_key); + } + } + m_syncdRoutes[vrf_id][ipPrefix] = nextHops; notifyNextHopChangeObservers(vrf_id, ipPrefix, nextHops, true); @@ -2049,6 +2105,12 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx) m_neighOrch->removeMplsNextHop(nexthop); } } + else if (ol_nextHops.getSize() == 1) + { + RouteKey r_key = { vrf_id, ipPrefix }; + auto nexthop = NextHopKey(ol_nextHops.to_string()); + removeNextHopRoute(nexthop, r_key); + } } SWSS_LOG_INFO("Remove route %s with next hop(s) %s", diff --git a/orchagent/routeorch.h b/orchagent/routeorch.h index 20e79699d5..4f74db62dc 100644 --- a/orchagent/routeorch.h +++ b/orchagent/routeorch.h @@ -42,6 +42,18 @@ struct NextHopUpdate struct NextHopObserverEntry; +/* Route destination key for a nexthop */ +struct RouteKey +{ + sai_object_id_t vrf_id; + IpPrefix prefix; + + bool operator < (const RouteKey& rhs) const + { + return (vrf_id <= rhs.vrf_id && prefix < rhs.prefix); + } +}; + /* NextHopGroupTable: NextHopGroupKey, NextHopGroupEntry */ typedef std::map NextHopGroupTable; /* RouteTable: destination network, NextHopGroupKey */ @@ -56,6 +68,8 @@ typedef std::map LabelRouteTables; typedef std::pair Host; /* NextHopObserverTable: Host, next hop observer entry */ typedef std::map NextHopObserverTable; +/* Single Nexthop to Routemap */ +typedef std::map> NextHopRouteTable; struct NextHopObserverEntry { @@ -138,6 +152,8 @@ class RouteOrch : public Orch, public Subject bool addNextHopGroup(const NextHopGroupKey&); bool removeNextHopGroup(const NextHopGroupKey&); + void addNextHopRoute(const NextHopKey&, const RouteKey&); + void removeNextHopRoute(const NextHopKey&, const RouteKey&); bool updateNextHopRoutes(const NextHopKey&, uint32_t&); bool validnexthopinNextHopGroup(const NextHopKey&, uint32_t&); @@ -170,6 +186,7 @@ class RouteOrch : public Orch, public Subject RouteTables m_syncdRoutes; LabelRouteTables m_syncdLabelRoutes; NextHopGroupTable m_syncdNextHopGroups; + NextHopRouteTable m_nextHops; std::set> m_bulkNhgReducedRefCnt; /* m_bulkNhgReducedRefCnt: nexthop, vrf_id */ From 6fbf3908abba46f833340c0036f03fa288f482e0 Mon Sep 17 00:00:00 2001 From: Prince Date: Wed, 22 Sep 2021 01:42:16 +0000 Subject: [PATCH 2/8] VS test added to verify the set flow --- tests/test_mux.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/test_mux.py b/tests/test_mux.py index 84458a841d..c9aa578f79 100644 --- a/tests/test_mux.py +++ b/tests/test_mux.py @@ -298,6 +298,29 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route): self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) + # Check route set flow and changing nexthop + self.set_mux_state(appdb, "Ethernet4", "active") + + ps = swsscommon.ProducerStateTable(pdb.db_connection, "ROUTE_TABLE") + fvs = swsscommon.FieldValuePairs([("nexthop", self.SERV2_IPV4), ("ifname", "Vlan1000")]) + ps.set(rtprefix, fvs) + + # Check if route was propagated to ASIC DB + rtkeys = dvs_route.check_asicdb_route_entries([rtprefix]) + + # Change Mux status for Ethernet0 and expect no change to replaced route + self.set_mux_state(appdb, "Ethernet0", "standby") + self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) + + self.set_mux_state(appdb, "Ethernet4", "standby") + self.check_nexthop_in_asic_db(asicdb, rtkeys[0], True) + + # Delete the route + ps._del(rtprefix) + + self.set_mux_state(appdb, "Ethernet4", "active") + dvs_route.check_asicdb_deleted_route_entries([rtprefix]) + # Test ECMP routes self.set_mux_state(appdb, "Ethernet0", "active") From f49e855d303ecde4df3310b0bebeb356012e84a4 Mon Sep 17 00:00:00 2001 From: Prince Date: Wed, 22 Sep 2021 18:11:53 +0000 Subject: [PATCH 3/8] Address review comments --- orchagent/routeorch.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 176ff85462..227cde8ade 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1348,8 +1348,7 @@ void RouteOrch::removeNextHopRoute(const NextHopKey& nextHop, const RouteKey& ro { if (it->second.find(routeKey) == it->second.end()) { - SWSS_LOG_INFO("Route not present in nh table %s", - routeKey.prefix.to_string().c_str()); + SWSS_LOG_INFO("Route not present in nh table %s", routeKey.prefix.to_string().c_str()); return; } @@ -1359,6 +1358,10 @@ void RouteOrch::removeNextHopRoute(const NextHopKey& nextHop, const RouteKey& ro m_nextHops.erase(nextHop); } } + else + { + SWSS_LOG_INFO("Nexthop %s not found in nexthop table", nextHop.to_string().c_str()); + } } bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRoutes) @@ -1393,7 +1396,11 @@ bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRout if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to update route %s, rv:%d", (*rt).prefix.to_string().c_str(), status); - return false; + task_process_status handle_status = handleSaiCreateStatus(SAI_API_ROUTE, status); + if (handle_status != task_success) + { + return parseHandleSaiStatusFailure(handle_status); + } } ++numRoutes; From 60df4cd37d630132acacef85e995f32f47b86714 Mon Sep 17 00:00:00 2001 From: Prince Date: Wed, 22 Sep 2021 20:05:37 +0000 Subject: [PATCH 4/8] Fixed a typo --- orchagent/routeorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 227cde8ade..8a2de24c29 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1396,7 +1396,7 @@ bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRout if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to update route %s, rv:%d", (*rt).prefix.to_string().c_str(), status); - task_process_status handle_status = handleSaiCreateStatus(SAI_API_ROUTE, status); + task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTE, status); if (handle_status != task_success) { return parseHandleSaiStatusFailure(handle_status); From 54ee80cd756e7d067c2441eee938354d88ed1ab5 Mon Sep 17 00:00:00 2001 From: Prince Date: Fri, 24 Sep 2021 02:49:06 +0000 Subject: [PATCH 5/8] VS test to be added later --- tests/test_mux.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/tests/test_mux.py b/tests/test_mux.py index c9aa578f79..84458a841d 100644 --- a/tests/test_mux.py +++ b/tests/test_mux.py @@ -298,29 +298,6 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route): self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) - # Check route set flow and changing nexthop - self.set_mux_state(appdb, "Ethernet4", "active") - - ps = swsscommon.ProducerStateTable(pdb.db_connection, "ROUTE_TABLE") - fvs = swsscommon.FieldValuePairs([("nexthop", self.SERV2_IPV4), ("ifname", "Vlan1000")]) - ps.set(rtprefix, fvs) - - # Check if route was propagated to ASIC DB - rtkeys = dvs_route.check_asicdb_route_entries([rtprefix]) - - # Change Mux status for Ethernet0 and expect no change to replaced route - self.set_mux_state(appdb, "Ethernet0", "standby") - self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) - - self.set_mux_state(appdb, "Ethernet4", "standby") - self.check_nexthop_in_asic_db(asicdb, rtkeys[0], True) - - # Delete the route - ps._del(rtprefix) - - self.set_mux_state(appdb, "Ethernet4", "active") - dvs_route.check_asicdb_deleted_route_entries([rtprefix]) - # Test ECMP routes self.set_mux_state(appdb, "Ethernet0", "active") From 8b114b560cc2eb6bd2121619e14d907bbddcce31 Mon Sep 17 00:00:00 2001 From: Prince Date: Fri, 24 Sep 2021 05:01:28 +0000 Subject: [PATCH 6/8] Fixing an extra check --- orchagent/routeorch.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 8a2de24c29..04b1a83ffe 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -2111,11 +2111,8 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx) { m_neighOrch->removeMplsNextHop(nexthop); } - } - else if (ol_nextHops.getSize() == 1) - { + RouteKey r_key = { vrf_id, ipPrefix }; - auto nexthop = NextHopKey(ol_nextHops.to_string()); removeNextHopRoute(nexthop, r_key); } } From 5cdd859f67f3a4463f697c239a26a6bb55cf7aa5 Mon Sep 17 00:00:00 2001 From: Prince Date: Fri, 24 Sep 2021 15:24:36 +0000 Subject: [PATCH 7/8] Re-add VS tests --- tests/test_mux.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/test_mux.py b/tests/test_mux.py index 84458a841d..c9aa578f79 100644 --- a/tests/test_mux.py +++ b/tests/test_mux.py @@ -298,6 +298,29 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route): self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) + # Check route set flow and changing nexthop + self.set_mux_state(appdb, "Ethernet4", "active") + + ps = swsscommon.ProducerStateTable(pdb.db_connection, "ROUTE_TABLE") + fvs = swsscommon.FieldValuePairs([("nexthop", self.SERV2_IPV4), ("ifname", "Vlan1000")]) + ps.set(rtprefix, fvs) + + # Check if route was propagated to ASIC DB + rtkeys = dvs_route.check_asicdb_route_entries([rtprefix]) + + # Change Mux status for Ethernet0 and expect no change to replaced route + self.set_mux_state(appdb, "Ethernet0", "standby") + self.check_nexthop_in_asic_db(asicdb, rtkeys[0]) + + self.set_mux_state(appdb, "Ethernet4", "standby") + self.check_nexthop_in_asic_db(asicdb, rtkeys[0], True) + + # Delete the route + ps._del(rtprefix) + + self.set_mux_state(appdb, "Ethernet4", "active") + dvs_route.check_asicdb_deleted_route_entries([rtprefix]) + # Test ECMP routes self.set_mux_state(appdb, "Ethernet0", "active") From c66cc8691beec1a6f054ac59d3143fe3d44904f1 Mon Sep 17 00:00:00 2001 From: Prince Date: Fri, 24 Sep 2021 20:23:48 +0000 Subject: [PATCH 8/8] Minor fix to skip overlay case --- orchagent/routeorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp index 04b1a83ffe..9e9bbe9891 100644 --- a/orchagent/routeorch.cpp +++ b/orchagent/routeorch.cpp @@ -1931,7 +1931,7 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey ipPrefix.to_string().c_str(), nextHops.to_string().c_str()); } - if (nextHops.getSize() == 1) + if (nextHops.getSize() == 1 && !nextHops.is_overlay_nexthop()) { RouteKey r_key = { vrf_id, ipPrefix }; auto nexthop = NextHopKey(nextHops.to_string());