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

Sure thing, It's in this branch
https://github.com/mkalcok/ovn/commit/8e1394dfbebc4f48255c77937015e07fbcbc5172
. It's based on your updated v5 branch with only one more commit that
adds the test. It won't pass either way because the branch is missing
advertisement of neighboring LBs. I have that change in another branch,
but for the purpose of investigating the crash, this should be good
enough. I can get it to crash about every other run. The tell-tell sign
is when the test hangs (because it waits on --wait=sb in some step).

Martin.

> 
> Thanks,
> Dumitru
> 
> > Martin.
> > 
> > > Regards,
> > > Dumitru
> > > 
> > > > ---
> > > > Regards,
> > > > Dumitru
> > > > 
> > > > 
> > > > > Thanks,
> > > > > Martin.
> > > > > 
> > > > > >  
> > > > > > > 
> > > > > > > Thanks again for the help,
> > > > > > > Martin.
> > > > > > > 
> > > > > > > > > Thanks for the feedback,
> > > > > > > > > Martin
> > > > > > > > > > 
> > > > > > > > > > Thanks a lot,
> > > > > > > > > > Felix
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > >  };
> > > > > > > > > > >  
> > > > > > > > > > >  /* Returns an independent clone of the provided
> > > > > > > > > > > parsed_route.
> > > > > > > > > > > The
> > > > > > > > > > > returned
> > > > > > > > > > > @@ -789,6 +790,7 @@ void parsed_route_add(const
> > > > > > > > > > > struct
> > > > > > > > > > > ovn_datapath
> > > > > > > > > > > *od,
> > > > > > > > > > >                        const struct sset
> > > > > > > > > > > *ecmp_selection_fields,
> > > > > > > > > > >                        enum route_source source,
> > > > > > > > > > >                        const struct ovsdb_idl_row
> > > > > > > > > > > *source_hint,
> > > > > > > > > > > +                      const struct ovn_port
> > > > > > > > > > > *tracked_port,
> > > > > > > > > > >                        struct hmap *routes);
> > > > > > > > > > >  
> > > > > > > > > > >  bool
> > > > > > > > > > > @@ -823,7 +825,14 @@ void
> > > > > > > > > > > route_policies_destroy(struct
> > > > > > > > > > > route_policies_data *);
> > > > > > > > > > >  void build_parsed_routes(const struct
> > > > > > > > > > > ovn_datapath
> > > > > > > > > > > *, const
> > > > > > > > > > > struct
> > > > > > > > > > > hmap *,
> > > > > > > > > > >                           const struct hmap *,
> > > > > > > > > > > struct
> > > > > > > > > > > hmap *,
> > > > > > > > > > > struct simap *,
> > > > > > > > > > >                           struct hmap *);
> > > > > > > > > > > -void build_lb_nat_parsed_routes(const struct
> > > > > > > > > > > ovn_datapath *,
> > > > > > > > > > > const
> > > > > > > > > > > struct lr_nat_record *, struct hmap *);
> > > > > > > > > > > +void build_nat_parsed_routes(const struct
> > > > > > > > > > > ovn_datapath *,
> > > > > > > > > > > +                             const struct
> > > > > > > > > > > lr_nat_record *,
> > > > > > > > > > > +                             const struct
> > > > > > > > > > > northd_data *,
> > > > > > > > > > > +                             struct hmap *);
> > > > > > > > > > > +void build_lb_parsed_routes(const struct
> > > > > > > > > > > ovn_datapath *,
> > > > > > > > > > > +                            const struct
> > > > > > > > > > > ovn_lb_ip_set *,
> > > > > > > > > > > +                            const struct
> > > > > > > > > > > northd_data
> > > > > > > > > > > *,
> > > > > > > > > > > +                            struct hmap *);
> > > > > > > > > > >  uint32_t get_route_table_id(struct simap *,
> > > > > > > > > > > const
> > > > > > > > > > > char *);
> > > > > > > > > > >  void routes_init(struct routes_data *);
> > > > > > > > > > >  void routes_destroy(struct routes_data *);
> > > > > > > > > > > --
> > > > > > > > > > > 2.43.0
> > > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > 
> > 
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to