On Fri, Jan 31, 2025 at 11:16:12AM +0100, Dumitru Ceara wrote: > 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]>
Hi Dumitru, thanks a lot, there will be no other changes on this patch. > > > +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
