On Thu, 2025-02-13 at 00:00 +0100, Dumitru Ceara wrote: > On 2/12/25 9:38 PM, Dumitru Ceara wrote: > > On 2/12/25 8:05 PM, [email protected] wrote: > > > On Wed, 2025-02-12 at 18:04 +0100, Dumitru Ceara wrote: > > > > On Wednesday, February 12, 2025, <[email protected] > > > > <mailto:[email protected]>> wrote: > > > > > Hi Dumitru, thanks for the feedback and suggestions > > > > > > > > > > On Wed, 2025-02-12 at 16:25 +0100, Dumitru Ceara wrote: > > > > > > > > > > > > Hi Martin, Felix, > > > > > > > > > > > > On 2/12/25 4:04 PM, [email protected] > > > > > <mailto:[email protected]> wrote: > > > > > > > On Wed, 2025-02-12 at 15:20 +0100, Felix Huettner wrote: > > > > > > > > On Wed, Feb 12, 2025 at 02:43:09AM +0100, Martin Kalcok > > > > > > > > wrote: > > > > > > > > > This is a continuation of previous commit. It's > > > > > > > > > separate > > > > > > > > > for ease of review. It will be squshed together for > > > > > > > > > the > > > > > > > > > final version. > > > > > > > > > > > > > > > > Hi Martin, > > > > > > > > > > > > > > > > i took a look at the changes and i'll add my findings > > > > > > > > below. > > > > > > > > > > > > > > > > Note that i squashed part 3 and 4 together locally for > > > > > > > > reviewing. > > > > > > > > I'll > > > > > > > > try to put my comments to the appropriate patch, but i > > > > > > > > am already > > > > > > > > sorry > > > > > > > > for when i mess that up :) > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Martin Kalcok > > > > > > > > > <[email protected] > > > > > <mailto:[email protected]>> > > > > > > > > > --- > > > > > > > > > northd/en-advertised-route-sync.c | 20 ++- > > > > > > > > > northd/en-learned-route-sync.c | 3 +- > > > > > > > > > northd/northd.c | 217 > > > > > > > > > ++++++++++++++++++++++++++++-- > > > > > > > > > northd/northd.h | 11 +- > > > > > > > > > 4 files changed, 231 insertions(+), 20 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/northd/en-advertised-route-sync.c > > > > > > > > > b/northd/en- > > > > > > > > > advertised-route-sync.c > > > > > > > > > index 9d4fb308d..e81c86afa 100644 > > > > > > > > > --- a/northd/en-advertised-route-sync.c > > > > > > > > > +++ b/northd/en-advertised-route-sync.c > > > > > > > > > @@ -25,7 +25,6 @@ > > > > > > > > > #include "openvswitch/hmap.h" > > > > > > > > > #include "ovn-util.h" > > > > > > > > > > > > > > > > > > - > > > > > > > > > static void > > > > > > > > > advertised_route_table_sync( > > > > > > > > > struct ovsdb_idl_txn *ovnsb_txn, > > > > > > > > > @@ -205,9 +204,13 @@ en_dynamic_routes_run(struct > > > > > > > > > engine_node > > > > > > > > > *node, void *data) > > > > > > > > > if (!od->dynamic_routing) { > > > > > > > > > continue; > > > > > > > > > } > > > > > > > > > - build_lb_nat_parsed_routes(od, > > > > > > > > > lr_stateful_rec- > > > > > > > > > > lrnat_rec, > > > > > > > > > - > > > > > > > > > &dynamic_routes_data- > > > > > > > > > > parsed_routes); > > > > > > > > > + build_nat_parsed_routes(od, lr_stateful_rec- > > > > > > > > > > lrnat_rec, > > > > > > > > > + northd_data, > > > > > > > > > + > > > > > > > > > &dynamic_routes_data- > > > > > > > > > > parsed_routes); > > > > > > > > > > > > > > > > > > + build_lb_parsed_routes(od, lr_stateful_rec- > > > > > > > > > >lb_ips, > > > > > > > > > + northd_data, > > > > > > > > > + &dynamic_routes_data- > > > > > > > > > > parsed_routes); > > > > > > > > > } > > > > > > > > > engine_set_node_state(node, EN_UPDATED); > > > > > > > > > } > > > > > > > > > @@ -442,10 +445,19 @@ > > > > > > > > > advertised_route_table_sync_route_add( > > > > > > > > > if (route->source == ROUTE_SOURCE_NAT && (drr & > > > > > > > > > DRRM_NAT) > > > > > > > > > == > > > > > > > > > 0) { > > > > > > > > > return; > > > > > > > > > } > > > > > > > > > + if (route->source == ROUTE_SOURCE_LB && (drr & > > > > > > > > > DRRM_LB) == > > > > > > > > > 0) > > > > > > > > > { > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > > > > > > > > > > + /* XXX: I'm not sure if normalize prefix is the > > > > > > > > > best call > > > > > > > > > here. It doesn't > > > > > > > > > + * include "/plen" for host routes, so they get > > > > > > > > > announced > > > > > > > > > without it. */ > > > > > > > > > > > > > > > > This is unfortunate but i don't think it would cause > > > > > > > > issues. In > > > > > > > > route.c > > > > > > > > we parse this value using ip46_parse_cidr and this will > > > > > > > > deduce it > > > > > > > > is > > > > > > > > a > > > > > > > > host route if there is no "/.*". > > > > > > > > > > > > > > > > Also this would probably also happen for now if we > > > > > > > > would specify > > > > > > > > a > > > > > > > > host > > > > > > > > route in a Logical_Router_Static_Route. > > > > > > > > > > > > > > You are right, this doesn't seem to be a big issue and > > > > > > > the route is > > > > > > > propagated to the VRF. I just thought that I'd comment on > > > > > > > it, since > > > > > > > we > > > > > > > do have access to route->plen here. > > > > > > > > > > > > > > > > > > > > > > > > char *ip_prefix = normalize_v46_prefix(&route- > > > > > > > > > >prefix, > > > > > > > > > route- > > > > > > > > > > plen); > > > > > > > > > + const struct sbrec_port_binding *tracked_port = > > > > > > > > > NULL; > > > > > > > > > + if (route->tracked_port) { > > > > > > > > > + tracked_port = route->tracked_port->sb; > > > > > > > > > + } > > > > > > > > > ar_add_entry(sync_routes, route->od->sb, route- > > > > > > > > > >out_port- > > > > > > > > > > sb, > > > > > > > > > - ip_prefix, NULL); > > > > > > > > > + ip_prefix, tracked_port); > > > > > > > > > } > > > > > > > > > > > > > > > > > > static void > > > > > > > > > diff --git a/northd/en-learned-route-sync.c > > > > > > > > > b/northd/en- > > > > > > > > > learned- > > > > > > > > > route-sync.c > > > > > > > > > index 406f1551f..4e87b3265 100644 > > > > > > > > > --- a/northd/en-learned-route-sync.c > > > > > > > > > +++ b/northd/en-learned-route-sync.c > > > > > > > > > @@ -181,7 +181,8 @@ > > > > > > > > > parse_route_from_sbrec_route(struct hmap > > > > > > > > > *parsed_routes_out, > > > > > > > > > > > > > > > > > > parsed_route_add(od, nexthop, &prefix, plen, > > > > > > > > > false, > > > > > > > > > lrp_addr_s, > > > > > > > > > out_port, 0, false, false, > > > > > > > > > NULL, > > > > > > > > > - ROUTE_SOURCE_LEARNED, &route- > > > > > > > > > >header_, > > > > > > > > > parsed_routes_out); > > > > > > > > > + ROUTE_SOURCE_LEARNED, &route- > > > > > > > > > >header_, > > > > > > > > > NULL, > > > > > > > > > + parsed_routes_out); > > > > > > > > > } > > > > > > > > > > > > > > > > > > static void > > > > > > > > > diff --git a/northd/northd.c b/northd/northd.c > > > > > > > > > index 4b5708059..c0953028a 100644 > > > > > > > > > --- a/northd/northd.c > > > > > > > > > +++ b/northd/northd.c > > > > > > > > > @@ -10992,6 +10992,7 @@ parsed_route_init(const > > > > > > > > > struct > > > > > > > > > ovn_datapath > > > > > > > > > *od, > > > > > > > > > bool ecmp_symmetric_reply, > > > > > > > > > const struct sset > > > > > > > > > *ecmp_selection_fields, > > > > > > > > > enum route_source source, > > > > > > > > > + const struct ovn_port > > > > > > > > > *tracked_port, > > > > > > > > > const struct ovsdb_idl_row > > > > > > > > > *source_hint) > > > > > > > > > { > > > > > > > > > > > > > > > > > > @@ -11007,6 +11008,7 @@ parsed_route_init(const > > > > > > > > > struct > > > > > > > > > ovn_datapath > > > > > > > > > *od, > > > > > > > > > new_pr->is_discard_route = is_discard_route; > > > > > > > > > new_pr->lrp_addr_s = > > > > > > > > > nullable_xstrdup(lrp_addr_s); > > > > > > > > > new_pr->out_port = out_port; > > > > > > > > > + new_pr->tracked_port = tracked_port; > > > > > > > > > new_pr->source = source; > > > > > > > > > if (ecmp_selection_fields) { > > > > > > > > > sset_clone(&new_pr->ecmp_selection_fields, > > > > > > > > > ecmp_selection_fields); > > > > > > > > > @@ -11030,7 +11032,7 @@ parsed_route_clone(const > > > > > > > > > struct > > > > > > > > > parsed_route *pr) > > > > > > > > > pr->od, nexthop, pr->prefix, pr->plen, pr- > > > > > > > > > > is_discard_route, > > > > > > > > > pr->lrp_addr_s, pr->out_port, pr- > > > > > > > > > >route_table_id, pr- > > > > > > > > > > is_src_route, > > > > > > > > > pr->ecmp_symmetric_reply, &pr- > > > > > > > > > >ecmp_selection_fields, > > > > > > > > > pr- > > > > > > > > > > source, > > > > > > > > > - pr->source_hint); > > > > > > > > > + pr->tracked_port, pr->source_hint); > > > > > > > > > > > > > > > > > > new_pr->hash = pr->hash; > > > > > > > > > return new_pr; > > > > > > > > > @@ -11069,13 +11071,14 @@ parsed_route_add(const > > > > > > > > > struct > > > > > > > > > ovn_datapath *od, > > > > > > > > > const struct sset > > > > > > > > > *ecmp_selection_fields, > > > > > > > > > enum route_source source, > > > > > > > > > const struct ovsdb_idl_row > > > > > > > > > *source_hint, > > > > > > > > > + const struct ovn_port > > > > > > > > > *tracked_port, > > > > > > > > > struct hmap *routes) > > > > > > > > > { > > > > > > > > > > > > > > > > > > struct parsed_route *new_pr = parsed_route_init( > > > > > > > > > od, nexthop, *prefix, plen, > > > > > > > > > is_discard_route, > > > > > > > > > lrp_addr_s, > > > > > > > > > out_port, > > > > > > > > > route_table_id, is_src_route, > > > > > > > > > ecmp_symmetric_reply, > > > > > > > > > - ecmp_selection_fields, source, source_hint); > > > > > > > > > + ecmp_selection_fields, source, tracked_port, > > > > > > > > > source_hint); > > > > > > > > > > > > > > > > > > new_pr->hash = route_hash(new_pr); > > > > > > > > > > > > > > > > > > @@ -11212,7 +11215,7 @@ > > > > > > > > > parsed_routes_add_static(const struct > > > > > > > > > ovn_datapath *od, > > > > > > > > > parsed_route_add(od, nexthop, &prefix, plen, > > > > > > > > > is_discard_route, > > > > > > > > > lrp_addr_s, > > > > > > > > > out_port, route_table_id, > > > > > > > > > is_src_route, > > > > > > > > > ecmp_symmetric_reply, > > > > > > > > > &ecmp_selection_fields, > > > > > > > > > source, > > > > > > > > > - &route->header_, routes); > > > > > > > > > + &route->header_, NULL, routes); > > > > > > > > > sset_destroy(&ecmp_selection_fields); > > > > > > > > > } > > > > > > > > > > > > > > > > > > @@ -11230,7 +11233,7 @@ > > > > > > > > > parsed_routes_add_connected(const > > > > > > > > > struct > > > > > > > > > ovn_datapath *od, > > > > > > > > > false, addr->addr_s, op, > > > > > > > > > 0, false, > > > > > > > > > false, NULL, > > > > > > > > > ROUTE_SOURCE_CONNECTED, > > > > > > > > > - &op->nbrp->header_, > > > > > > > > > routes); > > > > > > > > > + &op->nbrp->header_, NULL, > > > > > > > > > routes); > > > > > > > > > } > > > > > > > > > > > > > > > > > > for (size_t i = 0; i < op- > > > > > > > > > >lrp_networks.n_ipv6_addrs; i++) > > > > > > > > > { > > > > > > > > > @@ -11242,7 +11245,7 @@ > > > > > > > > > parsed_routes_add_connected(const > > > > > > > > > struct > > > > > > > > > ovn_datapath *od, > > > > > > > > > false, addr->addr_s, op, > > > > > > > > > 0, false, > > > > > > > > > false, NULL, > > > > > > > > > ROUTE_SOURCE_CONNECTED, > > > > > > > > > - &op->nbrp->header_, > > > > > > > > > routes); > > > > > > > > > + &op->nbrp->header_, NULL, > > > > > > > > > routes); > > > > > > > > > } > > > > > > > > > } > > > > > > > > > > > > > > > > > > @@ -11280,10 +11283,153 @@ build_parsed_routes(const > > > > > > > > > struct > > > > > > > > > ovn_datapath *od, const struct hmap *lr_ports, > > > > > > > > > } > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static int > > > > > > > > > +lrouter_check_nat_entry(const struct ovn_datapath > > > > > > > > > *od, > > > > > > > > > + const struct ovn_nat > > > > > > > > > *nat_entry, > > > > > > > > > + const struct hmap *lr_ports, > > > > > > > > > ovs_be32 > > > > > > > > > *mask, > > > > > > > > > + bool *is_v6, int *cidr_bits, > > > > > > > > > struct > > > > > > > > > eth_addr *mac, > > > > > > > > > + bool *distributed, struct > > > > > > > > > ovn_port > > > > > > > > > **nat_l3dgw_port); > > > > > > > > > + > > > > > > > > > +static const struct ovn_port * > > > > > > > > > +get_nat_route_tracked_port(const struct ovn_port > > > > > > > > > *op, > > > > > > > > > + const struct ovn_nat > > > > > > > > > *nat, > > > > > > > > > + const struct northd_data > > > > > > > > > *northd_data) > > > > > > > > > +{ > > > > > > > > > + /* Ports on GW routers don't need tracked_port > > > > > > > > > because all > > > > > > > > > resources > > > > > > > > > + * are located on the same chassis.*/ > > > > > > > > > + if (op->od->is_gw_router) { > > > > > > > > > + return NULL; > > > > > > > > > + } > > > > > > > > > + struct eth_addr mac = eth_addr_broadcast; > > > > > > > > > + bool is_v6, distributed_nat; > > > > > > > > > + ovs_be32 mask; > > > > > > > > > + int cidr_bits; > > > > > > > > > + struct ovn_port *l3dgw_port; > > > > > > > > > + > > > > > > > > > + if (lrouter_check_nat_entry(op->od, nat, > > > > > > > > > &northd_data- > > > > > > > > > > lr_ports, &mask, &is_v6, > > > > > > > > > + &cidr_bits, > > > > > > > > > + &mac, > > > > > > > > > &distributed_nat, > > > > > > > > > &l3dgw_port) < 0) { > > > > > > > > > + return NULL; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + /* For distributed NAT rules, we find ovn_port > > > > > > > > > with name > > > > > > > > > that > > > > > > > > > matches > > > > > > > > > + * logical_port value of the NAT entry, and use > > > > > > > > > that as > > > > > > > > > tracked_port. */ > > > > > > > > > + if (distributed_nat) { > > > > > > > > > + return ovn_port_find(&northd_data->ls_ports, > > > > > > > > > nat->nb- > > > > > > > > > > logical_port); > > > > > > > > > + /* For centralized NAT rules, we use designated > > > > > > > > > DGP as a > > > > > > > > > tracked port. */ > > > > > > > > > + } else { > > > > > > > > > + return l3dgw_port; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + return NULL; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +static void > > > > > > > > > +lport_addrs_to_parsed_routes(const struct ovn_port > > > > > > > > > *out_port, > > > > > > > > > + const struct ovn_port > > > > > > > > > *tracked_port, > > > > > > > > > + const struct > > > > > > > > > lport_addresses > > > > > > > > > *laddrs, > > > > > > > > > + enum route_source > > > > > > > > > route_type, > > > > > > > > > + struct hmap *routes) > > > > > > > > > +{ > > > > > > > > > + for (int i = 0; i < laddrs->n_ipv4_addrs; i++) { > > > > > > > > > + struct ipv4_netaddr *addr = &laddrs- > > > > > > > > > >ipv4_addrs[i]; > > > > > > > > > + struct in6_addr prefix; > > > > > > > > > + ip46_parse(addr->network_s, &prefix); > > > > > > > > > + parsed_route_add(out_port->od, NULL, > > > > > > > > > &prefix, addr- > > > > > > > > > > plen, > > > > > > > > > + false, addr->addr_s, > > > > > > > > > out_port, > > > > > > > > > + 0, false, > > > > > > > > > + false, NULL, route_type, > > > > > > > > > + &out_port->nbrp->header_, > > > > > > > > > tracked_port, > > > > > > > > > routes); > > > > > > > > > + } > > > > > > > > > + for (int i = 0; i < laddrs->n_ipv6_addrs; i++) { > > > > > > > > > + struct ipv6_netaddr *addr = &laddrs- > > > > > > > > > >ipv6_addrs[i]; > > > > > > > > > + parsed_route_add(out_port->od, NULL, &addr- > > > > > > > > > >addr, > > > > > > > > > addr- > > > > > > > > > > plen, > > > > > > > > > + false, addr->addr_s, > > > > > > > > > out_port, > > > > > > > > > + 0, false, > > > > > > > > > + false, NULL, route_type, > > > > > > > > > + &out_port->nbrp->header_, > > > > > > > > > tracked_port, > > > > > > > > > routes); > > > > > > > > > + } > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +/* XXX: This function is, in nature, very similar to > > > > > > > > > how > > > > > > > > > + * publish_host_routes_lrp looks up neighboring host > > > > > > > > > routes. I > > > > > > > > > really wanted > > > > > > > > > + * to reuse it, but it's designed to work with > > > > > > > > > already > > > > > > > > > existing > > > > > > > > > parsed_routes > > > > > > > > > + * when creating Advertised_Route records. And that > > > > > > > > > doesn't > > > > > > > > > work > > > > > > > > > in following > > > > > > > > > + * scenario: > > > > > > > > > + * > > > > > > > > > + * R1 --- ls_int --- R2 > > > > > > > > > + * > > > > > > > > > + * If R1 and R2 don't have IPv4/6 configured on > > > > > > > > > their LRPs > > > > > > > > > plugged > > > > > > > > > to > > > > > > > > > + * ls_int, and you set "connected-as-host", no > > > > > > > > > parsed_routes > > > > > > > > > will > > > > > > > > > be generated > > > > > > > > > + * (makes sense). But as a consequence, > > > > > > > > > publish_host_routes_lrp is > > > > > > > > > never > > > > > > > > > + * executed. > > > > > > > > > > > > > > > > there is a parsed_route generated for the link local > > > > > > > > address but > > > > > > > > we > > > > > > > > have > > > > > > > > been filtering it out before processing it further. > > > > > > > > However in the case of connected-as-host i see no > > > > > > > > reason to keep > > > > > > > > this > > > > > > > > limitation. > > > > > > > > > > > > > > > > I have built a small patch on top of current main to > > > > > > > > share this. > > > > > > > > Not sure if that is exactly what you are looking for. > > > > > > > > https://github.com/felixhuettner/ovn/ > > > > > commit/5326af4e0fb0b054a7f023ed8d15ae5b2969928a > > > > > <https://github.com/ > > > > > felixhuettner/ovn/commit/5326af4e0fb0b054a7f023ed8d15ae5b2969 > > > > > 928a> > > > > > > > > > > > > > > > > > > > > > > Oh right, the filtering happens all the way down during > > > > > > > the > > > > > > > "publish_*_route" phase. > > > > > > > In the meantime I came up with a way to publish both > > > > > > > NAT/LBs that > > > > > > > doesn't seem to be too costly. I'll take your change for > > > > > > > a spin, > > > > > > > but if > > > > > > > your version work (and it looks like it does), the only > > > > > > > benefit of > > > > > > > keeping my approach would be more granular control over > > > > > > > what gets > > > > > > > advertised (NATs, LBs or NATs+LBS+LRP_IPs). > > > > > > > > > > > > > > > > > > > Actually, I don't think we need this (especially if it > > > > > > parses NAT/LBs > > > > > > again). I think we already (almost) have all the NAT > > > > > > information in > > > > > > the > > > > > > lr_stateful_record data (see below, [suggestion]). > > > > > > > > > > > > > > > + * We would very much like this scenario to work. > > > > > > > > > i.e. no > > > > > > > > > explicit > > > > > > > > > IP > > > > > > > > > + * configuration on ls_int, but NATs/LBs on R2 are > > > > > > > > > reachable > > > > > > > > > from > > > > > > > > > R1 over > > > > > > > > > + * R2s IPv6 LLA. This function aims to solve it by > > > > > > > > > generating > > > > > > > > > + * advertised_routes for NATs that are on op's > > > > > > > > > neighbors > > > > > > > > > (either > > > > > > > > > directly > > > > > > > > > + * connected LRs or LRs connected to same LS as op). > > > > > > > > > I tried > > > > > > > > > to > > > > > > > > > keep the > > > > > > > > > + * overhead to minimum. > > > > > > > > > + */ > > > > > > > > > +static void > > > > > > > > > +build_connected_nat_routes(const struct ovn_port > > > > > > > > > *op, struct > > > > > > > > > hmap > > > > > > > > > *routes) > > > > > > > > > +{ > > > > > > > > > + if (!op->peer) { > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > + struct ovn_datapath *peer_od = op->peer->od; > > > > > > > > > + if (!peer_od->nbs && !peer_od->nbr) { > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + const struct ovn_port *peer_port = NULL; > > > > > > > > > + /* This is directly connected LR peer. */ > > > > > > > > > + if (peer_od->nbr) { > > > > > > > > > + peer_port = op->peer; > > > > > > > > > + size_t n_nats = 0; > > > > > > > > > + char **nats = NULL; > > > > > > > > > + nats = get_nat_addresses(peer_port, &n_nats, > > > > > > > > > false, > > > > > > > > > false, > > > > > > > > > NULL, true); > > > > > > > > > + for (size_t i = 0; i < n_nats; i++) { > > > > > > > > > + /* XXX: This block should be probably > > > > > > > > > squshed to a > > > > > > > > > function. It's > > > > > > > > > + * bit repetitive with the one bellow. > > > > > > > > > */ > > > > > > > > > + struct lport_addresses laddrs; > > > > > > > > > + int ofs = 0; > > > > > > > > > + extract_addresses(nats[i], &laddrs, > > > > > > > > > &ofs); > > > > > > > > > + lport_addrs_to_parsed_routes(op, > > > > > > > > > peer_port, > > > > > > > > > &laddrs, > > > > > > > > > ROUTE_SOURCE_NAT, routes); > > > > > > > > > + free(nats[i]); > > > > > > > > > + destroy_lport_addresses(&laddrs); > > > > > > > > > + } > > > > > > > > > + free(nats); > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + /* This peer is LSP, we need to check all > > > > > > > > > connected router > > > > > > > > > ports for NAT.*/ > > > > > > > > > + for (size_t i = 0; i < peer_od->n_router_ports; > > > > > > > > > i++) { > > > > > > > > > + peer_port = peer_od->router_ports[i]->peer; > > > > > > > > > + if (peer_port == op) { > > > > > > > > > + /* no need to check NAT addresses on > > > > > > > > > ovn_port that > > > > > > > > > initiated this > > > > > > > > > + * function.*/ > > > > > > > > > + continue; > > > > > > > > > + } > > > > > > > > > + size_t n_nats = 0; > > > > > > > > > + char **nats = NULL; > > > > > > > > > + nats = get_nat_addresses(peer_port, &n_nats, > > > > > > > > > false, > > > > > > > > > false, > > > > > > > > > NULL, true); > > > > > > > > > + for (size_t j = 0; j < n_nats; j++) { > > > > > > > > > + struct lport_addresses laddrs; > > > > > > > > > + int ofs = 0; > > > > > > > > > + extract_addresses(nats[j], &laddrs, > > > > > > > > > &ofs); > > > > > > > > > + lport_addrs_to_parsed_routes(op, > > > > > > > > > peer_port, > > > > > > > > > &laddrs, > > > > > > > > > ROUTE_SOURCE_NAT, routes); > > > > > > > > > + free(nats[j]); > > > > > > > > > + destroy_lport_addresses(&laddrs); > > > > > > > > > + } > > > > > > > > > + free(nats); > > > > > > > > > + } > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > void > > > > > > > > > -build_lb_nat_parsed_routes(const struct ovn_datapath > > > > > > > > > *od, > > > > > > > > > - const struct > > > > > > > > > lr_nat_record *lr_nat, > > > > > > > > > - struct hmap *routes) > > > > > > > > > +build_nat_parsed_routes(const struct ovn_datapath > > > > > > > > > *od, > > > > > > > > > + const struct lr_nat_record > > > > > > > > > *lr_nat, > > > > > > > > > + const struct northd_data > > > > > > > > > *northd_data, > > > > > > > > > + struct hmap *routes) > > > > > > > > > { > > > > > > > > > const struct ovn_port *op; > > > > > > > > > HMAP_FOR_EACH (op, dp_node, &od->ports) { > > > > > > > > > @@ -11291,19 +11437,62 @@ > > > > > > > > > build_lb_nat_parsed_routes(const > > > > > > > > > struct > > > > > > > > > ovn_datapath *od, > > > > > > > > > if (!(drrm & DRRM_NAT)) { > > > > > > > > > continue; > > > > > > > > > } > > > > > > > > > - /* I'm thinking of extending parsed_route > > > > > > > > > struct with > > > > > > > > > "tracked_port". > > > > > > > > > - * Since we are already parsing/iterating > > > > > > > > > NATs here, > > > > > > > > > it > > > > > > > > > feels more > > > > > > > > > - * efficinet to figure out the tracked_port > > > > > > > > > here, > > > > > > > > > rather > > > > > > > > > than > > > > > > > > > - * re-parsing NATs down the line in the > > > > > > > > > advertised_route_table_sync > > > > > > > > > - * function before calling "ar_add_entry".*/ > > > > > > > > > + > > > > > > > > > for (size_t i = 0; i < lr_nat- > > > > > > > > > >n_nat_entries; i++) { > > > > > > > > > const struct ovn_nat *nat = &lr_nat- > > > > > > > > > > nat_entries[i]; > > > > > > > > > int plen = nat_entry_is_v6(nat) ? 128 : > > > > > > > > > 32; > > > > > > > > > struct in6_addr prefix; > > > > > > > > > ip46_parse(nat->nb->external_ip, > > > > > > > > > &prefix); > > > > > > > > > + const struct ovn_port *tracked_port = > > > > > > > > > + get_nat_route_tracked_port(op, nat, > > > > > > > > > northd_data); > > > > > > > > > parsed_route_add(od, NULL, &prefix, > > > > > > > > > plen, false, > > > > > > > > > nat->nb->external_ip, > > > > > > > > > op, 0, > > > > > > > > > false, > > > > > > > > > false, > > > > > > > > > - NULL, ROUTE_SOURCE_NAT, > > > > > > > > > &op- > > > > > > > > > > nbrp- > > > > > > > > > > header_, routes); > > > > > > > > > + NULL, ROUTE_SOURCE_NAT, > > > > > > > > > &op- > > > > > > > > > > nbrp- > > > > > > > > > > header_, > > > > > > > > > + tracked_port, routes); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + /* XXX: This could be made optional by > > > > > > > > > adding "nat- > > > > > > > > > connected" option > > > > > > > > > + * to dynamic-routing-redistribute. Similar > > > > > > > > > to how > > > > > > > > > connected and > > > > > > > > > + * connected-as-host work.*/ > > > > > > > > > + build_connected_nat_routes(op, routes); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +void > > > > > > > > > +build_lb_parsed_routes(const struct ovn_datapath > > > > > > > > > *od, > > > > > > > > > + const struct ovn_lb_ip_set > > > > > > > > > *lb_ips, > > > > > > > > > + const struct northd_data > > > > > > > > > *northd_data, > > > > > > > > > > > > > > > > This seems to be unused. > > > > > > > > > > > > > > Yeah, just something I expected to use for LB tracked > > > > > > > port lookup. > > > > > > > It > > > > > > > won't be there in next version. > > > > > > > > > > > > > > > > > > > I actually think this is used in en_dynamic_routes_run() > > > > > > and, to be > > > > > > honest, to me it seems like the right approach. Please see > > > > > > below in > > > > > > the > > > > > Oh, I was just referring to the northd_data argument in this > > > > > function > > > > > that turned out to be unused. > > > > > > > > > > > [suggestion] section. > > > > > > > > > > > > > > > > > > > > > > > + struct hmap *routes) > > > > > > > > > +{ > > > > > > > > > + const struct ovn_port *op; > > > > > > > > > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > > > > > > > > > + enum dynamic_routing_redistribute_mode drrm > > > > > > > > > = op- > > > > > > > > > > dynamic_routing_redistribute; > > > > > > > > > + if (!(drrm & DRRM_LB)) { > > > > > > > > > + continue; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + const char *ip_address; > > > > > > > > > + SSET_FOR_EACH (ip_address, &lb_ips->ips_v4) > > > > > > > > > { > > > > > > > > > + struct in6_addr prefix; > > > > > > > > > + ip46_parse(ip_address, &prefix); > > > > > > > > > + /* XXX: Need to figure out tracked_port > > > > > > > > > for LB > > > > > > > > > without > > > > > > > > > re-parsing > > > > > > > > > + * ovn_datapath. lr_stateful_record > > > > > > > > > doens't have > > > > > > > > > as > > > > > > > > > much data > > > > > > > > > + * about LBs as it does about NATs. */ > > > > > > > > > + const struct ovn_port *tracked_port = > > > > > > > > > NULL; > > > > > > > > > + parsed_route_add(od, NULL, &prefix, 32, > > > > > > > > > false, > > > > > > > > > + ip_address, op, 0, > > > > > > > > > false, false, > > > > > > > > > + NULL, ROUTE_SOURCE_LB, > > > > > > > > > &op->nbrp- > > > > > > > > > > header_, > > > > > > > > > + tracked_port, routes); > > > > > > > > > + } > > > > > > > > > + SSET_FOR_EACH (ip_address, &lb_ips->ips_v6) > > > > > > > > > { > > > > > > > > > + struct in6_addr prefix; > > > > > > > > > + ip46_parse(ip_address, &prefix); > > > > > > > > > + const struct ovn_port *tracked_port = > > > > > > > > > NULL; > > > > > > > > > + parsed_route_add(od, NULL, &prefix, 128, > > > > > > > > > false, > > > > > > > > > + ip_address, op, 0, > > > > > > > > > false, false, > > > > > > > > > + NULL, ROUTE_SOURCE_LB, > > > > > > > > > &op->nbrp- > > > > > > > > > > header_, > > > > > > > > > + tracked_port, routes); > > > > > > > > > } > > > > > > > > > } > > > > > > > > > > > > > > > > > > diff --git a/northd/northd.h b/northd/northd.h > > > > > > > > > index 95f2fe010..5c0f7bc52 100644 > > > > > > > > > --- a/northd/northd.h > > > > > > > > > +++ b/northd/northd.h > > > > > > > > > @@ -761,6 +761,7 @@ struct parsed_route { > > > > > > > > > const struct ovsdb_idl_row *source_hint; > > > > > > > > > char *lrp_addr_s; > > > > > > > > > const struct ovn_port *out_port; > > > > > > > > > + const struct ovn_port *tracked_port; /* May be > > > > > > > > > NULL. */ > > > > > > > > > > > > > > > > I am not sure if adding this here makes sense. > > > > > > > > It seems to me that in the case that tracked_port is > > > > > > > > set then > > > > > > > > most > > > > > > > > other > > > > > > > > fields have empty/default values. > > > > > > > > Therefore maybe it makes sense to create a separate > > > > > > > > struct just > > > > > > > > with > > > > > > > > the > > > > > > > > needed fields (datapath, port, prefix and > > > > > > > > tracked_port). > > > > > > > > > > > > > > I see one big benefit of keeping only parsed_route > > > > > > > struct, and that > > > > > > > is > > > > > > > the fact that many existing useful functions already work > > > > > > > with > > > > > > > parsed_route. It keeps the syncing to SB relatively > > > > > > > unified. > > > > > > > > > > > > > > > > > > > I guess we could do this as follow up. For now, me > > > > > > personally, I'm > > > > > > OK > > > > > > with it being in parsed_route. Martin, could you please add > > > > > > a > > > > > > TODO/XXX > > > > > > comment here? > > > > > > > > > > > > Also, let's consolidate all TODOs this series adds into the > > > > > > "* > > > > > > Dynamic > > > > > > Routing" section Felix started in TODO.rst. > > > > > > > > > > Will do. > > > > > > > > > > > > > > > > > [suggestion] > > > > > > I wanted to try to reduce all the NAT parsing this version > > > > > > still has > > > > > > and > > > > > > ended up with: > > > > > > > > > > > > https://github.com/dceara/ovn/tree/refs/heads/tmp-bgp-lb-nat > > > > > > - > > > > > advertise-v5 > > > > > <https://github.com/dceara/ovn/tree/refs/heads/tmp-bgp- > > > > > lb-nat-advertise-v5> > > > > > > > > > > > > The first patch refactors how NATs are parsed in the > > > > > > incremental > > > > > > processing engine in ovn-northd. We can preparse all the > > > > > > information > > > > > > that's needed for both lflow generation and dynamic > > > > > > routes. It's > > > > > > still > > > > > > WIP because I need to test this thoroughly. > > > > > > > > > > > > In the second patch I used the already parsed NAT > > > > > > information (for > > > > > > LBs > > > > > > it's even simpler because we only process LBs on chassis > > > > > > where the > > > > > > distributed gateway ports are bound. It essentially > > > > > > implements the > > > > > > approach I tried to describe here: > > > > > > > > > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2025- > > > > > February/420789.html > > > > > <https://mail.openvswitch.org/pipermail/ovs- > > > > > dev/2025-February/420789.html> > > > > > > > > > > > > I tried it out locally and in my tests the NAT > > > > > > advertisements seem to > > > > > > work fine. However, Martin, I didn't adapt the system > > > > > > tests you had > > > > > > in > > > > > > v3 so I didn't run those. > > > > > > > > > > > > In any case, let me know what you guys think. If this > > > > > > looks OK to > > > > > > you, > > > > > > maybe we can try to integrate it in the next iteration of > > > > > > the > > > > > > patchset. > > > > > > > > > > > > Regards, > > > > > > Dumitru > > > > > > > > > > > > > > > > Your suggestions look good to me. The NAT is greatly > > > > > simplified by your > > > > > addition of l3dgw_port and the approach to LB tracked port is > > > > > the same > > > > > as I took today. > > > > > I now also have a function for advertising LBs on neighboring > > > > > routers, > > > > > and it looks to me equally cheap to me (only using > > > > > lr_stateful_rec- > > > > > > lb_ips). I'll merge my today's work with your suggestions > > > > > > and post new > > > > > version ASAP. > > > > > > > > > > > > > Ok, but please be aware: the first patch in my branch (the nat > > > > refactoring) is likely a bit buggy (for some reason distributed > > > > nats > > > > are not always marked as such). I need to look into it but I > > > > don’t > > > > think i’ll be able to do that today. > > > > > > > > Regards, > > > > Dumitru > > > > > > Ok, I'll try to look into that as well. > > I figured out the problem and while at it I cleaned up the NAT > parsing > and checking more. I pushed the new version to: > > https://github.com/dceara/ovn/tree/refs/heads/tmp-bgp-lb-nat-advertise-v5 > > where the refactor commit is: > https://github.com/dceara/ovn/commit/18e19bcc70b362da7beb32efcb58955f818c1184 > > > > I also noticed a discrepancy between NAT/LB tracked ports on GW > > > routers. > > > NAT doesn't set any tracked_port but LB sets itself (the port > > > advertising) as a track_port. Is it necessary for LBs to set > > > tracked_port on GW routers since the resources are all on the > > > same chassis? > > > > > > > You're probably right, we don't need to set tracked_port when > > advertising routes on GW routers in general. > > > > This slightly hacky diff on top of the branch I shared earlierwould > > fix that I guess: > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 9362624d63..9795c849f6 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -11421,7 +11421,7 @@ build_lb_parsed_routes(const struct > > ovn_datapath *od, > > * > > * Advertise the LB IPs via all 'op' if this is a gateway > > router or > > * throuh all DGPs of this distributed router otherwise. > > */ > > - struct ovn_port *op_ = CONST_CAST(struct ovn_port *, op); > > + struct ovn_port *op_ = NULL; > > size_t n_tracked_ports = !od->is_gw_router ? od- > > >n_l3dgw_ports : 1; > > struct ovn_port **tracked_ports = !od->is_gw_router > > ? od->l3dgw_ports > > I also included this incremental change in the last version of the > branch I linked above. > > Again, if all this looks good to you, feel free to use it in v6.
Thanks, I will add it. In the meantime I'm also fighting with segfaults that appeared when I was adding tests. Tests add "stuff" to the OVN in big batches and It crashes fairly consistently (though not always). This didn't occur in my manual testing. Sometimes the crashes are silent, but from time to time, there are transaction errors complaining about referencing non-existing DPs or PBs (both for logical ports and tracked ports). I don't presume that your patch will fix that, since it happens with pure LBs too. I wonder if it's enough to have dependency on lr_stateful. Is it possible that we need to depend on something else? Like "en_sync_to_sb_pb"? Martin. > Regards, > Dumitru > > > --- > > Regards, > > Dumitru > > > > > > > Thanks, > > > Martin. > > > > > > > > > > > > > > > > > Thanks again for the help, > > > > > Martin. > > > > > > > > > > > > Thanks for the feedback, > > > > > > > Martin > > > > > > > > > > > > > > > > Thanks a lot, > > > > > > > > Felix > > > > > > > > > > > > > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > /* Returns an independent clone of the provided > > > > > > > > > parsed_route. > > > > > > > > > The > > > > > > > > > returned > > > > > > > > > @@ -789,6 +790,7 @@ void parsed_route_add(const > > > > > > > > > struct > > > > > > > > > ovn_datapath > > > > > > > > > *od, > > > > > > > > > const struct sset > > > > > > > > > *ecmp_selection_fields, > > > > > > > > > enum route_source source, > > > > > > > > > const struct ovsdb_idl_row > > > > > > > > > *source_hint, > > > > > > > > > + const struct ovn_port > > > > > > > > > *tracked_port, > > > > > > > > > struct hmap *routes); > > > > > > > > > > > > > > > > > > bool > > > > > > > > > @@ -823,7 +825,14 @@ void > > > > > > > > > route_policies_destroy(struct > > > > > > > > > route_policies_data *); > > > > > > > > > void build_parsed_routes(const struct ovn_datapath > > > > > > > > > *, const > > > > > > > > > struct > > > > > > > > > hmap *, > > > > > > > > > const struct hmap *, struct > > > > > > > > > hmap *, > > > > > > > > > struct simap *, > > > > > > > > > struct hmap *); > > > > > > > > > -void build_lb_nat_parsed_routes(const struct > > > > > > > > > ovn_datapath *, > > > > > > > > > const > > > > > > > > > struct lr_nat_record *, struct hmap *); > > > > > > > > > +void build_nat_parsed_routes(const struct > > > > > > > > > ovn_datapath *, > > > > > > > > > + const struct > > > > > > > > > lr_nat_record *, > > > > > > > > > + const struct > > > > > > > > > northd_data *, > > > > > > > > > + struct hmap *); > > > > > > > > > +void build_lb_parsed_routes(const struct > > > > > > > > > ovn_datapath *, > > > > > > > > > + const struct > > > > > > > > > ovn_lb_ip_set *, > > > > > > > > > + const struct northd_data > > > > > > > > > *, > > > > > > > > > + struct hmap *); > > > > > > > > > uint32_t get_route_table_id(struct simap *, const > > > > > > > > > char *); > > > > > > > > > void routes_init(struct routes_data *); > > > > > > > > > void routes_destroy(struct routes_data *); > > > > > > > > > -- > > > > > > > > > 2.43.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
