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