On Fri, Feb 07, 2025 at 03:50:45PM +0100, Dumitru Ceara wrote:
> On 2/6/25 3:19 PM, Felix Huettner via dev wrote:
> > Here we expand the previous routes-sync engine node to not only
> > advertise routes to the southbound table, but also learn received routes
> > from this table.
> > 
> > These routes are then passed to the same logic that connected and static
> > routes are using for flow generation.
> > However we prioritize these routes lower than connected or static routes
> > as information in cluster (for the same prefix length) should always be
> > more correct then learned routes.
> > This is also consistent with the default behaviour of phyiscal routers.
> > 
> > Acked-by: Dumitru Ceara <[email protected]>
> > Signed-off-by: Felix Huettner <[email protected]>
> > ---
> 
> Hi Felix,
> 
> > v6->v7:
> >   * addressed review comments
> > v5->v6:
> >   * addressed review comments
> >   * changed option to "dynamic-routing-redistribute"
> > v4->v5: skipped
> > v2->v3:
> >   * A lot of minor review comments.
> >   * Support learning routes over other address families
> > 
> >  NEWS                           |   4 +
> >  TODO.rst                       |   5 +
> >  lib/stopwatch-names.h          |   1 +
> >  northd/automake.mk             |   2 +
> >  northd/en-ecmp-nexthop.c       |  16 +-
> >  northd/en-learned-route-sync.c | 208 +++++++++++++++++++++++
> >  northd/en-learned-route-sync.h |  32 ++++
> >  northd/en-lflow.c              |   5 +-
> >  northd/inc-proc-northd.c       |  14 +-
> >  northd/northd.c                | 301 ++++++++++++++++++++-------------
> >  northd/northd.h                |  38 ++++-
> >  northd/ovn-northd.c            |   1 +
> >  tests/ovn-northd.at            | 253 +++++++++++++++++++++++----
> >  13 files changed, 724 insertions(+), 156 deletions(-)
> >  create mode 100644 northd/en-learned-route-sync.c
> >  create mode 100644 northd/en-learned-route-sync.h
> > 
> 
> [...]
> 
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 06314730d..1fa0d6365 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -718,6 +718,8 @@ enum route_source {
> >      ROUTE_SOURCE_CONNECTED,
> >      /* The route is derived from a northbound static route entry. */
> >      ROUTE_SOURCE_STATIC,
> > +    /* The route is dynamically learned by an ovn-controller. */
> > +    ROUTE_SOURCE_LEARNED,
> >  };
> >  
> >  struct parsed_route {
> > @@ -728,17 +730,51 @@ struct parsed_route {
> >      bool is_src_route;
> >      uint32_t route_table_id;
> >      uint32_t hash;
> > -    const struct nbrec_logical_router_static_route *route;
> >      bool ecmp_symmetric_reply;
> >      bool is_discard_route;
> >      const struct ovn_datapath *od;
> >      bool stale;
> >      struct sset ecmp_selection_fields;
> >      enum route_source source;
> > +    const struct ovsdb_idl_row *source_hint;
> >      char *lrp_addr_s;
> >      const struct ovn_port *out_port;
> >  };
> >  
> > +/* Returns an independent clone of the provided parsed_route. The returned
> > + * parsed_route will need to be freed using parsed_route_free. */
> > +struct parsed_route *parsed_route_clone(const struct parsed_route *);
> > +
> > +/* Returns the hash to be used for a hmap of parsed_routes that are 
> > provided
> > + * to the lflow engine node. */
> > +size_t parsed_route_hash(const struct parsed_route *);
> > +void parsed_route_free(struct parsed_route *);
> > +
> > +/* Adds a parsed_route to the provided routes hmap if it is not already
> > + * in there.
> > + * Takes ownership of the provided nexthop. All other parameters are 
> > cloned.
> > + * Elements of the routes hmap need to be freed using parsed_route_free. */
> 
> Thanks for documenting these functions!  However, the comments should go
> in the C file.
> 
> With that addressed, my ack stands:
> Acked-by: Dumitru Ceara <[email protected]>

Thanks for pointing that out. wil be fixed.

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

Reply via email to