On Wed, 2025-02-12 at 18:04 +0100, Dumitru Ceara wrote: > On Wednesday, February 12, 2025, <[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] 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]> > > > > > > --- > > > > > > 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 > > > > > > > > > > > > > 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 > > > > > > 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 > > > > > > 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 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? 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
