On 1/21/25 4:47 PM, Felix Huettner via dev wrote: > Learned routes must be bound to a lrp on which the routes where learned. > In case the lrp is deleted for whatever reason no ovn-controller would > clean these routes up, therefor we do this in northd. > > Signed-off-by: Felix Huettner <[email protected]> > --- > v2->v3: > * A lot of minor review comments. > > northd/en-learned-route-sync.c | 8 +++++++- > northd/northd.c | 34 ---------------------------------- > northd/northd.h | 34 ++++++++++++++++++++++++++++++++++ > tests/ovn-northd.at | 16 ++++++++++++++++ > 4 files changed, 57 insertions(+), 35 deletions(-) > > diff --git a/northd/en-learned-route-sync.c b/northd/en-learned-route-sync.c > index 048b807ed..a144b4487 100644 > --- a/northd/en-learned-route-sync.c > +++ b/northd/en-learned-route-sync.c > @@ -197,7 +197,13 @@ routes_table_sync( > const struct parsed_route *route; > > const struct sbrec_learned_route *sb_route; > - SBREC_LEARNED_ROUTE_TABLE_FOR_EACH (sb_route, sbrec_learned_route_table) > { > + SBREC_LEARNED_ROUTE_TABLE_FOR_EACH_SAFE (sb_route, > + sbrec_learned_route_table) { > + if (!ovn_port_find(lr_ports, > + sb_route->logical_port->logical_port)) { > + 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/northd/northd.c b/northd/northd.c > index 0eff92125..baffa7c90 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -1297,34 +1297,6 @@ ovn_port_destroy(struct hmap *ports, struct ovn_port > *port) > } > } > > -/* Returns the ovn_port that matches 'name'. If 'prefer_bound' is true and > - * multiple ports share the same name, gives precendence to ports bound to > - * an ovn_datapath. > - */ > -static struct ovn_port * > -ovn_port_find__(const struct hmap *ports, const char *name, > - bool prefer_bound) > -{ > - struct ovn_port *matched_op = NULL; > - struct ovn_port *op; > - > - HMAP_FOR_EACH_WITH_HASH (op, key_node, hash_string(name, 0), ports) { > - if (!strcmp(op->key, name)) { > - matched_op = op; > - if (!prefer_bound || op->od) { > - return op; > - } > - } > - } > - return matched_op; > -} > - > -static struct ovn_port * > -ovn_port_find(const struct hmap *ports, const char *name) > -{ > - return ovn_port_find__(ports, name, false); > -} > - > static bool > lsp_is_clone_to_unknown(const struct nbrec_logical_switch_port *nbsp) > { > @@ -1339,12 +1311,6 @@ lsp_is_clone_to_unknown(const struct > nbrec_logical_switch_port *nbsp) > return false; > } > > -static struct ovn_port * > -ovn_port_find_bound(const struct hmap *ports, const char *name) > -{ > - return ovn_port_find__(ports, name, true); > -} > - > /* Returns true if the logical switch port 'enabled' column is empty or > * set to true. Otherwise, returns false. */ > static bool > diff --git a/northd/northd.h b/northd/northd.h > index bef7d3778..6d0e7998a 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -913,4 +913,38 @@ is_vxlan_mode(const struct smap *nb_options, > > uint32_t get_ovn_max_dp_key_local(bool _vxlan_mode); > > +/* Returns the ovn_port that matches 'name'. If 'prefer_bound' is true and > + * multiple ports share the same name, gives precendence to ports bound to > + * an ovn_datapath. > + */ > +static struct ovn_port * > +ovn_port_find__(const struct hmap *ports, const char *name, > + bool prefer_bound) > +{ > + struct ovn_port *matched_op = NULL; > + struct ovn_port *op; > + > + HMAP_FOR_EACH_WITH_HASH (op, key_node, hash_string(name, 0), ports) { > + if (!strcmp(op->key, name)) { > + matched_op = op; > + if (!prefer_bound || op->od) { > + return op; > + } > + } > + } > + return matched_op; > +} > + > +static inline struct ovn_port * > +ovn_port_find(const struct hmap *ports, const char *name) > +{ > + return ovn_port_find__(ports, name, false); > +} > + > +static inline struct ovn_port * > +ovn_port_find_bound(const struct hmap *ports, const char *name) > +{ > + return ovn_port_find__(ports, name, true); > +} > + > #endif /* NORTHD_H */ > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 885e6996a..0f146ee7f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -14661,6 +14661,22 @@ AT_CHECK([grep -w "lr_in_ip_routing" lr0flows | > ovn_strip_lflows], [0], [dnl > table=??(lr_in_ip_routing ), priority=518 , match=(ip6.dst == > 2001:db8:ffff::/64), action=(ip.ttl--; reg8[[0..15]] = 0; xxreg0 = ip6.dst; > xxreg1 = 2001:db8:ffff::1; eth.src = 00:00:00:00:ff:02; outport = "lr0-sw1"; > flags.loopback = 1; reg9[[9]] = 0; next;) > ]) > > +# deleting lr0-sw1 will remove the flows and also the learned route
Nit: Deleting lr0-sw1 will remove the flows and also the learned route. The rest looks good to me. If this is the only change that you'll make in v5 feel free to add my ack: Acked-by: Dumitru Ceara <[email protected]> > +check ovn-nbctl --wait=sb lrp-del lr0-sw1 > +check_row_count Advertised_Route 2 > +check_row_count Learned_Route 2 > +check_row_count Learned_Route 2 logical_port=$sw0 > +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;) > + table=??(lr_in_ip_routing ), priority=10550, match=(nd_rs || nd_ra), > action=(drop;) > + table=??(lr_in_ip_routing ), priority=194 , match=(reg7 == 0 && ip4.dst > == 172.16.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 10.0.0.11; > reg5 = 10.0.0.1; eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; > flags.loopback = 1; reg9[[9]] = 1; next;) > + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && ip4.dst > == 192.168.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 10.0.0.10; > reg5 = 10.0.0.1; eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; > flags.loopback = 1; reg9[[9]] = 1; next;) > + table=??(lr_in_ip_routing ), priority=198 , match=(ip4.dst == > 10.0.0.0/24), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = ip4.dst; reg5 = > 10.0.0.1; eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; flags.loopback = > 1; reg9[[9]] = 1; next;) > + table=??(lr_in_ip_routing ), priority=514 , match=(reg7 == 0 && ip6.dst > == 2001:db8:3::/64), action=(ip.ttl--; reg8[[0..15]] = 0; reg0 = 10.0.0.20; > reg5 = 10.0.0.1; eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; > flags.loopback = 1; reg9[[9]] = 1; next;) > + table=??(lr_in_ip_routing ), priority=518 , match=(inport == "lr0-sw0" > && ip6.dst == fe80::/64), action=(ip.ttl--; reg8[[0..15]] = 0; xxreg0 = > ip6.dst; xxreg1 = fe80::200:ff:fe00:ff01; eth.src = 00:00:00:00:ff:01; > outport = "lr0-sw0"; flags.loopback = 1; reg9[[9]] = 0; next;) > +]) > + > AT_CLEANUP > ]) > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
