Skip to content

Commit

Permalink
[muxorch] Fixing bug with updateRoute and mux neighbors (sonic-net#3187)
Browse files Browse the repository at this point in the history
* [muxorch] Fixing bug with updateRoute and mux neighbors

mux neighbors that were not the configured mux ip were being treated as active.
bug was causing crash when neighbor was not the same as the mux configured neighbor
  • Loading branch information
Ndancejic authored Jun 8, 2024
1 parent 8f333b6 commit f497c4a
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 9 deletions.
16 changes: 7 additions & 9 deletions orchagent/muxorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1283,14 +1283,12 @@ void MuxOrch::updateRoute(const IpPrefix &pfx, bool add)
for (auto it = nextHops.begin(); it != nextHops.end(); it++)
{
NextHopKey nexthop = *it;
/* This will only work for configured MUX neighbors (most cases)
* TODO: add way to find MUX from neighbor
*/
MuxCable* cable = findMuxCableInSubnet(nexthop.ip_address);
auto standalone = standalone_tunnel_neighbors_.find(nexthop.ip_address);
NeighborEntry neighbor;
MacAddress mac;

gNeighOrch->getNeighborEntry(nexthop, neighbor, mac);

if ((cable == nullptr && standalone == standalone_tunnel_neighbors_.end()) ||
cable->isActive())
if (isNeighborActive(neighbor.ip_address, mac, neighbor.alias))
{
/* Here we pull from local nexthop ID because neighbor update occurs during state change
* before nexthopID is updated in neighorch. This ensures that if a neighbor is Active
Expand All @@ -1302,11 +1300,11 @@ void MuxOrch::updateRoute(const IpPrefix &pfx, bool add)
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set route entry %s to nexthop %s",
pfx.to_string().c_str(), nexthop.to_string().c_str());
pfx.to_string().c_str(), neighbor.to_string().c_str());
continue;
}
SWSS_LOG_NOTICE("setting route %s with nexthop %s %" PRIx64 "",
pfx.to_string().c_str(), nexthop.to_string().c_str(), next_hop_id);
pfx.to_string().c_str(), neighbor.to_string().c_str(), next_hop_id);
active_found = true;
break;
}
Expand Down
14 changes: 14 additions & 0 deletions tests/test_mux.py
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,8 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_
new_ipv6_nexthop = self.SERV3_IPV6
non_mux_ipv4 = "11.11.11.11"
non_mux_ipv6 = "2222::100"
mux_neighbor_ipv4 = "192.170.0.100"
mux_neighbor_ipv6 = "fc02:1000:100::100"
non_mux_mac = "00:aa:aa:aa:aa:aa"
mux_ports = ["Ethernet0", "Ethernet4"]
new_mux_port = "Ethernet8"
Expand All @@ -806,6 +808,8 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_
self.add_neighbor(dvs, new_ipv6_nexthop, new_mac)
self.add_neighbor(dvs, non_mux_ipv4, non_mux_mac)
self.add_neighbor(dvs, non_mux_ipv6, non_mux_mac)
self.add_neighbor(dvs, mux_neighbor_ipv4, macs[1])
self.add_neighbor(dvs, mux_neighbor_ipv6, macs[1])

for port in mux_ports:
self.set_mux_state(appdb, port, ACTIVE)
Expand All @@ -826,6 +830,14 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_
self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, non_mux_nexthop=non_mux_ipv4)
self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, non_mux_nexthop=non_mux_ipv6)

# Testing mux neighbors that do not match mux configured ip
self.add_route(dvs, route_ipv4, [self.SERV1_IPV4, mux_neighbor_ipv4])
self.add_route(dvs, route_ipv6, [self.SERV1_IPV6, mux_neighbor_ipv6])
self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route_ipv4, mux_ports, [self.SERV1_IPV4, mux_neighbor_ipv4])
self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route_ipv6, mux_ports, [self.SERV1_IPV6, mux_neighbor_ipv6])
self.del_route(dvs,route_ipv4)
self.del_route(dvs,route_ipv6)

# # These tests do not create route, so create beforehand:
self.add_route(dvs, route_ipv4, ipv4_nexthops)
self.add_route(dvs, route_ipv6, ipv6_nexthops)
Expand All @@ -850,6 +862,8 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_
self.del_neighbor(dvs, neighbor)
self.del_neighbor(dvs, new_ipv4_nexthop)
self.del_neighbor(dvs, new_ipv6_nexthop)
self.del_neighbor(dvs, mux_neighbor_ipv4)
self.del_neighbor(dvs, mux_neighbor_ipv6)

def create_and_test_NH_routes(self, appdb, asicdb, dvs, dvs_route, mac):
'''
Expand Down

0 comments on commit f497c4a

Please sign in to comment.