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