On 12/18/25 2:50 PM, Lucas Vargas Dias wrote: > Hi Dumitru, Hi Lucas,
> thanks for the review. > > Em sex., 12 de dez. de 2025 às 12:29, Dumitru Ceara <[email protected]> > escreveu: > >> On 12/5/25 7:20 PM, Lucas Vargas Dias via dev wrote: >>> Learned routes must have the nexthop reachable. So, if logical router >>> has two ports in differents subnets, route must be learned in just >>> one which is the port in the same subnet from nexthop. >>> >>> Signed-off-by: Lucas Vargas Dias <[email protected]> >>> --- >> >> Hi Lucas, >> >> Thanks for the patch! >> >> I'm not sure I understand the problem though, please see below. >> >>> northd/en-learned-route-sync.c | 44 ++++++++++++++++++++ >>> tests/ovn-northd.at | 14 ++++++- >>> tests/system-ovn.at | 74 +++++++++++++++++----------------- >>> 3 files changed, 93 insertions(+), 39 deletions(-) >>> >>> diff --git a/northd/en-learned-route-sync.c >> b/northd/en-learned-route-sync.c >>> index f22aaa664..4c9de8651 100644 >>> --- a/northd/en-learned-route-sync.c >>> +++ b/northd/en-learned-route-sync.c >>> @@ -227,6 +227,50 @@ routes_table_sync( >>> sbrec_learned_route_delete(sb_route); >>> continue; >>> } >>> + bool is_same_subnet = false; >>> + for (size_t i = 0; !is_same_subnet && >>> + i < sb_route->logical_port->n_mac; >>> + i++) { >>> + struct lport_addresses logical_port_addrs; >>> + if (!extract_lsp_addresses(sb_route->logical_port->mac[i], >>> + &logical_port_addrs)) { >>> + destroy_lport_addresses(&logical_port_addrs); >>> + continue; >>> + } >>> + ovs_be32 neigh_prefix_v4; >>> + struct in6_addr neigh_prefix_v6; >>> + >>> + if (ip_parse(sb_route->nexthop, &neigh_prefix_v4)) { >>> + for (size_t j = 0; j < logical_port_addrs.n_ipv4_addrs; >>> + j++) { >>> + struct ipv4_netaddr address = >>> + logical_port_addrs.ipv4_addrs[j]; >>> + if (address.network == >>> + (neigh_prefix_v4 & address.mask)) { >>> + is_same_subnet = true; >>> + break; >>> + } >>> + } >>> + } else if (ipv6_parse(sb_route->nexthop, &neigh_prefix_v6)) >> { >>> + for (size_t j = 0; j < logical_port_addrs.n_ipv6_addrs; >> j++) { >>> + struct ipv6_netaddr address = >>> + logical_port_addrs.ipv6_addrs[j]; >>> + struct in6_addr neigh_prefix = >>> + ipv6_addr_bitand(&neigh_prefix_v6, >> &address.mask); >>> + if (ipv6_addr_equals(&address.network, >> &neigh_prefix)) { >>> + is_same_subnet = true; >>> + break; >>> + } >>> + } >>> + } >>> + destroy_lport_addresses(&logical_port_addrs); >>> + } >>> + >>> + >>> + if (!is_same_subnet) { >>> + sbrec_learned_route_delete(sb_route); >>> + continue; >>> + } >>> parse_route_from_sbrec_route(parsed_routes_out, lr_ports, >>> &lr_datapaths->datapaths, >>> sb_route); >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 864854c56..0d1d06196 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -15578,7 +15578,7 @@ AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | >> ovn_strip_lflows], [0], [dnl >>> ]) >>> >>> # Learn a route to 2001:db8:2::/64 via 2001:db8:ffff::20 learned on >> lr0-sw1. >>> -# This is not reachable so will not produce a lflow. >>> +# This is not reachable so will not produce a lflow. Also, it'll be >> removed by northd >> >> As the comment used to say, while the route is learned, ovn-northd >> doesn't actually produce logical flows for it because the next hop is >> not reachable on any network configured on lr0-sw1. >> >> So yes, the route is in SB.Learned_Route, but it doesn't affect routing >> in any way. It's exactly the same as configuring a NB Static_Route with >> a wrong next hop. >> >> Moreover, your code removes the Learned_Route in northd. But >> Learned_Routes are fully managed (created & deleted) by ovn-controller. >> >> So next time ovn-controller runs it will just relearn the route >> (re-create the SB.Learned_Route record). So this only creates >> unnecessary churn as far as I understand. >> >> Or is there a use case I missed? >> >> > You're right, it's more appropriate to add the code in ovn controller > and northd will not generate flows. However, I would like to avoid the > garbage on the southbound. > I'm still not sure what "garbage" we're avoiding. The router learns a "dynamic" route from a routing protocol (like any traditional router). Then there's some logic (in northd in our case) that decides whether that route should be used for actual traffic forwarding (i.e., is installed in the forwarding table in traditional routers). If the port it's learnt on doesn't have an IP in a subnet that includes the next hop, northd doesn't "install" the route (as logical flows). Also, I don't think you can move the check to ovn-controller. We already check for LRP subnets that match the next hop in ovn-northd. We don't want to duplicate that logic to ovn-controller IMO. Regards, Dumitru > > Regards, > Lucas > > >> Regards, >> Dumitru >> >> >>> check_uuid ovn-sbctl create Learned_Route \ >>> datapath=$datapath \ >>> logical_port=$sw1 \ >>> @@ -15586,7 +15586,9 @@ check_uuid ovn-sbctl create Learned_Route \ >>> nexthop=\"2001:db8:ffff::20\" >>> check ovn-nbctl --wait=sb sync >>> check_row_count Advertised_Route 4 >>> -check_row_count Learned_Route 2 >>> +check_row_count Learned_Route 1 >>> +check_row_count Learned_Route 0 logical_port=$sw1 >> ip_prefix=\"2001:db8:2::/64\" >>> + >>> ovn-sbctl dump-flows lr0 > lr0flows >>> AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | ovn_strip_lflows], [0], >> [dnl >>> table=??(lr_in_ip_routing ), priority=0 , match=(1), >> action=(drop;) >>> @@ -15605,6 +15607,14 @@ AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | >> ovn_strip_lflows], [0], [dnl >>> # active. >>> check ovn-nbctl --wait=sb set Logical_Router_Port lr0-sw1 \ >>> networks="\"2001:db8::1/64\" \"2001:db8:ffff::1/64\"" >>> + >>> +# Learn a route to 2001:db8:2::/64 via 2001:db8:ffff::20 learned on >> lr0-sw1. >>> +# Northd doesn not remove now because lrp have address in same subnet >> from nexthop >>> +check_uuid ovn-sbctl create Learned_Route \ >>> + datapath=$datapath \ >>> + logical_port=$sw1 \ >>> + ip_prefix=\"2001:db8:2::/64\" \ >>> + nexthop=\"2001:db8:ffff::20\" >>> check_row_count Advertised_Route 5 >>> check_row_count Learned_Route 2 >>> ovn-sbctl dump-flows lr0 > lr0flows >>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>> index 1cbbdfa58..6e87fb266 100644 >>> --- a/tests/system-ovn.at >>> +++ b/tests/system-ovn.at >>> @@ -15404,10 +15404,10 @@ wait_for_ports_up mylearninglsp >>> check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ >>> options:dynamic-routing-port-name=mylearninglsp >>> >>> -check ip route add 233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 30 proto zebra >>> -check ip route add 233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 40 proto zebra >>> +check ip route add 233.253.0.0/24 via 192.168.10.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 30 proto zebra >>> +check ip route add 233.253.0.0/24 via 192.168.10.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 40 proto zebra >>> check ovn-nbctl --wait=hv sync >>> -check_row_count Learned_Route 1 ip_prefix=233.253.0.0/24 >> nexthop=192.168.20.20 >>> +check_row_count Learned_Route 1 ip_prefix=233.253.0.0/24 >> nexthop=192.168.10.20 >>> >>> # Stopping the ovn-controller will clean up the route entries created >> by it. >>> # We first need to unset dynamic-routing-maintain-vrf as otherwise it >> will >>> @@ -15417,8 +15417,8 @@ check ovn-nbctl --wait=hv set >> Logical_Router_Port internet-phys \ >>> OVN_CLEANUP_CONTROLLER([hv1]) >>> OVN_ROUTE_EQUAL([ovnvrf1337], [dnl >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Starting it again will add the routes again. >>> start_daemon ovn-controller >>> @@ -15431,8 +15431,8 @@ blackhole 192.0.2.10 proto ovn metric 100 >>> blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Changing the vrf name will switch to the new one. >>> # The old vrf will be removed. >>> @@ -15449,8 +15449,8 @@ blackhole 192.0.2.10 proto ovn metric 100 >>> blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Stopping with --restart will not touch the routes. >>> OVN_CONTROLLER_EXIT([],[--restart]) >>> @@ -15462,8 +15462,8 @@ blackhole 192.0.2.10 proto ovn metric 100 >>> blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # When we now stop the ovn-controller it will remove the VRF. >>> start_daemon ovn-controller >>> @@ -15494,8 +15494,8 @@ blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 192.0.2.21 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Bind "vip" port locally and check the virtual IP is added in the VRF. >>> NS_EXEC([vif4], [arping -U -c 1 -w 2 -I vif4 192.0.2.30]) >>> @@ -15511,8 +15511,8 @@ blackhole 192.0.2.21 proto ovn metric 100 >>> blackhole 192.0.2.30 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> check ovn-sbctl clear Port_Binding vip virtual-parent >>> OVN_ROUTE_EQUAL([ovnvrf1338], [dnl >>> @@ -15524,8 +15524,8 @@ blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 192.0.2.21 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Remove the backoff period, so we can bind it right away. >>> check ovn-sbctl remove Port_Binding vip options vport-backoff >>> @@ -15543,8 +15543,8 @@ blackhole 192.0.2.21 proto ovn metric 100 >>> blackhole 192.0.2.30 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Simulate "vip" bound to a different chassis. >>> check ovn-sbctl clear Port_Binding vip virtual-parent >>> @@ -15559,8 +15559,8 @@ blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 192.0.2.21 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Check with dynamic-routing-redistribute-local-only=false. >>> check ovn-nbctl --wait=hv set logical_router_port internet-public \ >>> @@ -15576,8 +15576,8 @@ blackhole 192.0.2.22 proto ovn metric 1000 >>> blackhole 192.0.2.30 proto ovn metric 1000 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Remove the backoff period, so we can bind it right away. >>> check ovn-sbctl remove Port_Binding vip options vport-backoff >>> @@ -15596,8 +15596,8 @@ blackhole 192.0.2.22 proto ovn metric 1000 >>> blackhole 192.0.2.30 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> OVN_CLEANUP_CONTROLLER([hv1]) >>> AT_CHECK([ip vrf | grep -q ovnvrf1338], [1], []) >>> @@ -15857,7 +15857,7 @@ check ip route add 233.253.0.0/24 via >> 192.168.10.10 dev lo onlink vrf ovnvrf1337 >>> check ovn-nbctl --wait=hv sync >>> # With a Gateway Router all LRPs are locally bound, and without explicit >>> # mapping/filtering they will all learn the route. >>> -check_row_count Learned_Route 2 >>> +check_row_count Learned_Route 1 >>> lp=$(fetch_column port_binding _uuid logical_port=internet-phys) >>> check_row_count Learned_Route 1 logical_port=$lp ip_prefix= >> 233.252.0.0/24 nexthop=192.168.10.10 >>> >>> @@ -15892,10 +15892,10 @@ wait_for_ports_up mylearninglsp >>> check ovn-nbctl --wait=hv set Logical_Router_Port internet-phys \ >>> options:dynamic-routing-port-name=mylearninglsp >>> >>> -check ip route add 233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 30 proto zebra >>> -check ip route add 233.253.0.0/24 via 192.168.20.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 40 proto zebra >>> +check ip route add 233.253.0.0/24 via 192.168.10.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 30 proto zebra >>> +check ip route add 233.253.0.0/24 via 192.168.10.20 dev hv1-mll onlink >> vrf ovnvrf1337 metric 40 proto zebra >>> check ovn-nbctl --wait=hv sync >>> -check_row_count Learned_Route 1 ip_prefix=233.253.0.0/24 >> nexthop=192.168.20.20 >>> +check_row_count Learned_Route 1 ip_prefix=233.253.0.0/24 >> nexthop=192.168.10.20 >>> >>> # Stopping the ovn-controller will clean up the route entries created >> by it. >>> # We first need to unset dynamic-routing-maintain-vrf as otherwise it >> will >>> @@ -15905,8 +15905,8 @@ check ovn-nbctl --wait=hv set >> Logical_Router_Port internet-phys \ >>> OVN_CLEANUP_CONTROLLER([hv1]) >>> OVN_ROUTE_EQUAL([ovnvrf1337], [dnl >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Starting it again will add the routes again. >>> start_daemon ovn-controller >>> @@ -15919,8 +15919,8 @@ blackhole 192.0.2.10 proto ovn metric 100 >>> blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Stopping with --restart will not touch the routes. >>> OVN_CONTROLLER_EXIT([],[--restart]) >>> @@ -15932,8 +15932,8 @@ blackhole 192.0.2.10 proto ovn metric 100 >>> blackhole 192.0.2.20 proto ovn metric 100 >>> blackhole 198.51.100.0/24 proto ovn metric 1000 >>> 233.252.0.0/24 via 192.168.10.10 dev lo proto zebra onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 30 >> onlink >>> -233.253.0.0/24 via 192.168.20.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 30 >> onlink >>> +233.253.0.0/24 via 192.168.10.20 dev hv1-mll proto zebra metric 40 >> onlink]) >>> >>> # Now we set maintain-vrf again and stop the ovn-controller. >>> # It will then remove the VRF. >>> @@ -16925,13 +16925,13 @@ check_row_count Learned_Route 1 >>> AS_BOX([No dynamic-routing-port-name: routes learned on lrp1 and lrp2]) >>> check ovn-nbctl --wait=hv \ >>> remove logical_router_port lrp2 options dynamic-routing-port-name >>> -check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \ >>> +check_row_count Learned_Route 0 ip_prefix=3.3.3.0/24 \ >>> nexthop=2.2.2.2 \ >>> logical_port=$lrp1 >>> check_row_count Learned_Route 1 ip_prefix=3.3.3.0/24 \ >>> nexthop=2.2.2.2 \ >>> logical_port=$lrp2 >>> -check_row_count Learned_Route 2 >>> +check_row_count Learned_Route 1 >>> >>> OVN_CLEANUP_CONTROLLER([hv1]) >>> >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
