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/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 <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. 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
