On Mon, Feb 03, 2025 at 12:15:36PM +0100, Dumitru Ceara wrote:
> On 1/29/25 12:15 PM, Felix Huettner via dev wrote:
> > This engine node determines the routes that the ovn-controller should
> > export.
> >
> > Co-Authored-By: Frode Nordahl <[email protected]>
> > Signed-off-by: Frode Nordahl <[email protected]>
> > Signed-off-by: Felix Huettner <[email protected]>
> > ---
>
> Hi Felix, Frode,
>
> Overall this patch looks correct to me. I have some minor comments for
> the next revision, please see below.
Hi Dumitru,
thanks for the review.
All changes will be part of the next version.
Thanks a lot,
Felix
>
> > v3->v4:
> > - addressed review comments.
> >
> > TODO.rst | 1 +
> > controller/automake.mk | 4 +-
> > controller/local_data.c | 7 +-
> > controller/local_data.h | 1 +
> > controller/ovn-controller.c | 191 ++++++++++++++++++++++++++++++++-
> > controller/route.c | 203 ++++++++++++++++++++++++++++++++++++
> > controller/route.h | 68 ++++++++++++
> > tests/automake.mk | 1 +
> > 8 files changed, 473 insertions(+), 3 deletions(-)
> > create mode 100644 controller/route.c
> > create mode 100644 controller/route.h
> >
> > diff --git a/TODO.rst b/TODO.rst
> > index 3426497a7..b3ab43232 100644
> > --- a/TODO.rst
> > +++ b/TODO.rst
> > @@ -97,6 +97,7 @@ OVN To-do List
> > * ovn-controller Incremental processing
> >
> > * Implement I-P for datapath groups.
> > + * Implement I-P for route exchange relevant ports.
> >
> > * ovn-northd parallel logical flow processing
> >
> > diff --git a/controller/automake.mk b/controller/automake.mk
> > index bb0bf2d33..a6a2c517a 100644
> > --- a/controller/automake.mk
> > +++ b/controller/automake.mk
> > @@ -51,7 +51,9 @@ controller_ovn_controller_SOURCES = \
> > controller/ct-zone.h \
> > controller/ct-zone.c \
> > controller/ovn-dns.c \
> > - controller/ovn-dns.h
> > + controller/ovn-dns.h \
> > + controller/route.h \
> > + controller/route.c
> >
> > controller_ovn_controller_LDADD = lib/libovn.la
> > $(OVS_LIBDIR)/libopenvswitch.la
> > man_MANS += controller/ovn-controller.8
> > diff --git a/controller/local_data.c b/controller/local_data.c
> > index 69a1b775f..4aee39d6b 100644
> > --- a/controller/local_data.c
> > +++ b/controller/local_data.c
> > @@ -414,14 +414,19 @@ tracked_datapath_lport_add(const struct
> > sbrec_port_binding *pb,
> > }
> >
> > void
> > -tracked_datapaths_destroy(struct hmap *tracked_datapaths)
> > +tracked_datapaths_clear(struct hmap *tracked_datapaths)
> > {
> > struct tracked_datapath *t_dp;
> > HMAP_FOR_EACH_POP (t_dp, node, tracked_datapaths) {
> > shash_destroy_free_data(&t_dp->lports);
> > free(t_dp);
> > }
> > +}
> >
> > +void
> > +tracked_datapaths_destroy(struct hmap *tracked_datapaths)
> > +{
> > + tracked_datapaths_clear(tracked_datapaths);
> > hmap_destroy(tracked_datapaths);
> > }
> >
> > diff --git a/controller/local_data.h b/controller/local_data.h
> > index ab8e789a5..1d60dada8 100644
> > --- a/controller/local_data.h
> > +++ b/controller/local_data.h
> > @@ -131,6 +131,7 @@ struct tracked_datapath *tracked_datapath_find(
> > void tracked_datapath_lport_add(const struct sbrec_port_binding *,
> > enum en_tracked_resource_type,
> > struct hmap *tracked_datapaths);
> > +void tracked_datapaths_clear(struct hmap *tracked_datapaths);
> > void tracked_datapaths_destroy(struct hmap *tracked_datapaths);
> >
> > /* Maps from a chassis to the OpenFlow port number of the tunnel that can
> > be
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 854e54854..6ffa283a7 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -88,6 +88,7 @@
> > #include "lib/dns-resolve.h"
> > #include "ct-zone.h"
> > #include "ovn-dns.h"
> > +#include "route.h"
> >
> > VLOG_DEFINE_THIS_MODULE(main);
> >
> > @@ -247,6 +248,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp);
> > struct ovsdb_idl_condition chprv = OVSDB_IDL_CONDITION_INIT(&chprv);
> > struct ovsdb_idl_condition tv = OVSDB_IDL_CONDITION_INIT(&tv);
> > + struct ovsdb_idl_condition ar = OVSDB_IDL_CONDITION_INIT(&ar);
> >
> > /* Always monitor all logical datapath groups. Otherwise, DPG updates
> > may
> > * be received *after* the lflows using it are seen by ovn-controller.
> > @@ -254,6 +256,11 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> > * avoid the unnecessarily extra wake-ups of ovn-controller. */
> > ovsdb_idl_condition_add_clause_true(&ldpg);
> >
> > + /* Always monitor advertised routes. Otherwise, once we claim a port on
> > + * startup we do not yet know the routes to advertise and might wrongly
> > + * delete already installed ones. */
> > + ovsdb_idl_condition_add_clause_true(&ar);
> > +
>
> This problem is just at startup if I understand correctly. We handled
> similar cases for other Southbound records in a different way. See the:
>
> if (chassis) {
> [...]
> } else {
> /* During initialization, we monitor all records in Chassis_Private so
> * that we don't try to recreate existing ones. */
> ovsdb_idl_condition_add_clause_true(&chprv);
> /* Also, to avoid traffic disruption (e.g., conntrack flushing for
> * zones that are used by OVN but not yet known due to the SB initial
> * contents not being available), monitor all port bindings
> * connected to gateways; they might be claimed as soon as the
> * chassis is available.
> */
> sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway");
> sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway");
> }
>
> Can't we do the same thing for advertised routes?
Sounds good since it also should just happen during startup.
>
> > if (monitor_all) {
> > ovsdb_idl_condition_add_clause_true(&pb);
> > ovsdb_idl_condition_add_clause_true(&lf);
> > @@ -381,6 +388,7 @@ out:;
> > sb_table_set_req_mon_condition(ovnsb_idl, igmp_group, &igmp),
> > sb_table_set_req_mon_condition(ovnsb_idl, chassis_private, &chprv),
> > sb_table_set_opt_mon_condition(ovnsb_idl, chassis_template_var,
> > &tv),
> > + sb_table_set_opt_mon_condition(ovnsb_idl, advertised_route, &ar),
> > };
> >
> > unsigned int expected_cond_seqno = 0;
> > @@ -400,6 +408,7 @@ out:;
> > ovsdb_idl_condition_destroy(&igmp);
> > ovsdb_idl_condition_destroy(&chprv);
> > ovsdb_idl_condition_destroy(&tv);
> > + ovsdb_idl_condition_destroy(&ar);
> > return expected_cond_seqno;
> > }
> >
> > @@ -865,7 +874,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> > SB_NODE(fdb, "fdb") \
> > SB_NODE(meter, "meter") \
> > SB_NODE(static_mac_binding, "static_mac_binding") \
> > - SB_NODE(chassis_template_var, "chassis_template_var")
> > + SB_NODE(chassis_template_var, "chassis_template_var") \
> > + SB_NODE(advertised_route, "advertised_route")
> >
> > enum sb_engine_node {
> > #define SB_NODE(NAME, NAME_STR) SB_##NAME,
> > @@ -4812,6 +4822,172 @@ pflow_lflow_output_sb_chassis_handler(struct
> > engine_node *node,
> > return true;
> > }
> >
> > +struct ed_type_route {
> > + struct ovsdb_idl *ovnsb_idl;
> > + /* Contains struct tracked_datapath entries for local datapaths
> > subject to
> > + * route exchange. */
> > + struct hmap tracked_route_datapaths;
> > + /* Contains struct advertise_datapath_entry */
> > + struct hmap announce_routes;
> > +};
> > +
> > +static void
> > +en_route_run(struct engine_node *node, void *data)
> > +{
> > + struct ed_type_route *re_data = data;
> > + route_cleanup(&re_data->announce_routes);
>
> Nit: For better "code grouping" I'd move the route_cleanup() call just
> before tracked_datapaths_clear() below.
>
> > +
> > + const struct ovsrec_open_vswitch_table *ovs_table =
> > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + ovs_assert(chassis_id);
> > +
> > + struct ovsdb_idl_index *sbrec_chassis_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_chassis", node),
> > + "name");
> > + const struct sbrec_chassis *chassis
> > + = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
> > + ovs_assert(chassis);
> > +
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_port_binding", node),
> > + "name");
> > + struct ed_type_runtime_data *rt_data =
> > + engine_get_input_data("runtime_data", node);
> > +
> > + struct route_ctx_in r_ctx_in = {
> > + .ovnsb_idl = re_data->ovnsb_idl,
> > + .sbrec_port_binding_by_name = sbrec_port_binding_by_name,
> > + .chassis = chassis,
> > + .active_tunnels = &rt_data->active_tunnels,
> > + .local_datapaths = &rt_data->local_datapaths,
> > + .local_lports = &rt_data->local_lports,
> > + };
> > +
> > + struct route_ctx_out r_ctx_out = {
> > + .tracked_re_datapaths = &re_data->tracked_route_datapaths,
> > + .announce_routes = &re_data->announce_routes,
> > + };
> > +
> > + tracked_datapaths_clear(r_ctx_out.tracked_re_datapaths);
> > +
>
> Nit: I'd remove this newline.
>
> > + route_run(&r_ctx_in, &r_ctx_out);
> > +
>
> Nit: this one too.
>
> > + engine_set_node_state(node, EN_UPDATED);
> > +}
> > +
> > +
> > +static void *
> > +en_route_init(struct engine_node *node OVS_UNUSED,
> > + struct engine_arg *arg)
> > +{
> > + struct ed_type_route *data = xzalloc(sizeof *data);
> > +
> > + hmap_init(&data->tracked_route_datapaths);
> > + hmap_init(&data->announce_routes);
> > + data->ovnsb_idl = arg->sb_idl;
> > +
> > + return data;
> > +}
> > +
> > +static void
> > +en_route_cleanup(void *data)
> > +{
> > + struct ed_type_route *re_data = data;
> > +
> > + tracked_datapaths_destroy(&re_data->tracked_route_datapaths);
> > + route_cleanup(&re_data->announce_routes);
> > + hmap_destroy(&re_data->announce_routes);
> > +}
> > +
> > +static bool
> > +route_runtime_data_handler(struct engine_node *node, void *data)
> > +{
> > + struct ed_type_route *re_data = data;
> > + struct ed_type_runtime_data *rt_data =
> > + engine_get_input_data("runtime_data", node);
> > +
> > + if (!rt_data->tracked) {
> > + return false;
> > + }
> > +
> > + struct tracked_datapath *t_dp;
> > + HMAP_FOR_EACH (t_dp, node, &rt_data->tracked_dp_bindings) {
> > + struct tracked_datapath *re_t_dp =
> > + tracked_datapath_find(&re_data->tracked_route_datapaths,
> > t_dp->dp);
> > +
> > + if (re_t_dp) {
> > + /* XXX: Until we get I-P support for route exchange we need to
> > + * request recompute. */
> > + return false;
> > + }
> > +
> > + struct shash_node *shash_node;
> > + SHASH_FOR_EACH (shash_node, &t_dp->lports) {
> > + struct tracked_lport *lport = shash_node->data;
> > + if (route_exchange_relevant_port(lport->pb)) {
> > + /* XXX: Until we get I-P support for route exchange we
> > need to
> > + * request recompute. */
> > + return false;
> > + }
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static bool
> > +route_sb_port_binding_data_handler(struct engine_node *node, void *data)
> > +{
> > + struct ed_type_route *re_data = data;
> > + const struct sbrec_port_binding_table *pb_table =
> > + EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
> > +
> > + const struct sbrec_port_binding *sbrec_pb;
> > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_pb, pb_table) {
> > + struct tracked_datapath *re_t_dp =
> > + tracked_datapath_find(&re_data->tracked_route_datapaths,
> > + sbrec_pb->datapath);
> > + if (re_t_dp) {
> > + /* XXX: Until we get I-P support for route exchange we need to
> > + * request recompute. */
> > + return false;
> > + }
> > +
> > + if (route_exchange_relevant_port(sbrec_pb)) {
> > + /* XXX: Until we get I-P support for route exchange we need to
> > + * request recompute. */
> > + return false;
> > + }
> > +
> > + }
> > + return true;
> > +}
> > +
> > +static bool
> > +route_sb_advertised_route_data_handler(struct engine_node *node, void
> > *data)
> > +{
> > + struct ed_type_route *re_data = data;
> > + const struct sbrec_advertised_route_table *advertised_route_table =
> > + EN_OVSDB_GET(engine_get_input("SB_advertised_route", node));
> > +
> > + const struct sbrec_advertised_route *sbrec_route;
> > + SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH_TRACKED (sbrec_route,
> > + advertised_route_table)
> > {
> > + struct tracked_datapath *re_t_dp =
> > + tracked_datapath_find(&re_data->tracked_route_datapaths,
> > + sbrec_route->datapath);
> > + if (re_t_dp) {
> > + /* XXX: Until we get I-P support for route exchange we need to
> > + * request recompute. */
> > + return false;
> > + }
> > + }
> > + return true;
> > +}
> > +
> > /* Returns false if the northd internal version stored in SB_Global
> > * and ovn-controller internal version don't match.
> > */
> > @@ -5103,6 +5279,7 @@ main(int argc, char *argv[])
> > ENGINE_NODE(mac_cache, "mac_cache");
> > ENGINE_NODE(bfd_chassis, "bfd_chassis");
> > ENGINE_NODE(dns_cache, "dns_cache");
> > + ENGINE_NODE(route, "route");
> >
> > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
> > SB_NODES
> > @@ -5125,6 +5302,15 @@ main(int argc, char *argv[])
> > engine_add_input(&en_lb_data, &en_runtime_data,
> > lb_data_runtime_data_handler);
> >
> > + engine_add_input(&en_route, &en_ovs_open_vswitch, NULL);
> > + engine_add_input(&en_route, &en_sb_chassis, NULL);
> > + engine_add_input(&en_route, &en_sb_port_binding,
> > + route_sb_port_binding_data_handler);
> > + engine_add_input(&en_route, &en_runtime_data,
> > + route_runtime_data_handler);
> > + engine_add_input(&en_route, &en_sb_advertised_route,
> > + route_sb_advertised_route_data_handler);
> > +
> > engine_add_input(&en_addr_sets, &en_sb_address_set,
> > addr_sets_sb_address_set_handler);
> > engine_add_input(&en_port_groups, &en_sb_port_group,
> > @@ -5310,6 +5496,9 @@ main(int argc, char *argv[])
> > controller_output_mac_cache_handler);
> > engine_add_input(&en_controller_output, &en_bfd_chassis,
> > controller_output_bfd_chassis_handler);
> > + /* This is just temporary until the route output is actually used. */
> > + engine_add_input(&en_controller_output, &en_route,
> > + controller_output_bfd_chassis_handler);
> >
> > struct engine_arg engine_arg = {
> > .sb_idl = ovnsb_idl_loop.idl,
> > diff --git a/controller/route.c b/controller/route.c
> > new file mode 100644
> > index 000000000..49e76ee73
> > --- /dev/null
> > +++ b/controller/route.c
> > @@ -0,0 +1,203 @@
> > +/*
> > + * Copyright (c) 2024, Canonical, Ltd.
> > + * Copyright (c) 2024, STACKIT GmbH & Co. KG
>
> Nit: 2025
>
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include <net/if.h>
> > +
> > +#include "openvswitch/hmap.h"
> > +#include "openvswitch/vlog.h"
> > +
> > +#include "lib/ovn-sb-idl.h"
> > +
> > +#include "binding.h"
> > +#include "ha-chassis.h"
> > +#include "local_data.h"
> > +#include "route.h"
> > +
> > +
>
> Nit: one newline too many.
>
> > +VLOG_DEFINE_THIS_MODULE(exchange);
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> > +
> > +/* While the linux kernel can handle 2^32 routing tables, only so many can
> > fit
> > + * in the corresponding VRF interface name. */
> > +#define MAX_TABLE_ID 1000000000
> > +
> > +bool
> > +route_exchange_relevant_port(const struct sbrec_port_binding *pb)
> > +{
> > + return pb && smap_get_bool(&pb->options, "dynamic-routing", false);
> > +}
> > +
> > +uint32_t
> > +advertise_route_hash(const struct in6_addr *dst, unsigned int plen)
> > +{
> > + uint32_t hash = hash_bytes(dst->s6_addr, 16, 0);
> > + return hash_int(plen, hash);
> > +}
> > +
> > +static const struct sbrec_port_binding*
> > +find_route_exchange_pb(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > + const struct sbrec_chassis *chassis,
> > + const struct sset *active_tunnels,
> > + const struct sbrec_port_binding *pb)
> > +{
> > + if (!pb) {
> > + return NULL;
> > + }
> > + if (route_exchange_relevant_port(pb)) {
> > + return pb;
> > + }
> > + const char *crp = smap_get(&pb->options, "chassis-redirect-port");
> > + if (!crp) {
> > + return NULL;
> > + }
> > + if (!lport_is_chassis_resident(sbrec_port_binding_by_name, chassis,
> > + active_tunnels, crp)) {
> > + return NULL;
> > + }
> > + const struct sbrec_port_binding *crpbp = lport_lookup_by_name(
> > + sbrec_port_binding_by_name, crp);
> > + if (route_exchange_relevant_port(crpbp)) {
> > + return crpbp;
> > + }
> > + return NULL;
> > +}
> > +
> > +static void
> > +advertise_datapath_cleanup(struct advertise_datapath_entry *ad)
> > +{
> > + struct advertise_route_entry *ar;
> > + HMAP_FOR_EACH_SAFE (ar, node, &ad->routes) {
> > + hmap_remove(&ad->routes, &ar->node);
> > + free(ar);
> > + }
> > + hmap_destroy(&ad->routes);
> > + sset_destroy(&ad->bound_ports);
> > + free(ad);
> > +}
> > +
> > +static struct advertise_datapath_entry*
> > +advertise_datapath_find(const struct hmap *datapaths,
> > + const struct sbrec_datapath_binding *db)
> > +{
> > + struct advertise_datapath_entry *ade;
> > + HMAP_FOR_EACH_WITH_HASH (ade, node, db->tunnel_key, datapaths) {
> > + if (ade->db == db) {
> > + return ade;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +void
> > +route_run(struct route_ctx_in *r_ctx_in,
> > + struct route_ctx_out *r_ctx_out)
> > +{
> > + struct advertise_datapath_entry *ad;
> > + const struct local_datapath *ld;
> > +
> > + HMAP_FOR_EACH (ld, hmap_node, r_ctx_in->local_datapaths) {
> > + if (!ld->n_peer_ports || ld->is_switch) {
> > + continue;
> > + }
> > +
> > + ad = xzalloc(sizeof(*ad));
> > + ad->db = ld->datapath;
> > + hmap_init(&ad->routes);
> > + sset_init(&ad->bound_ports);
> > +
> > + /* This is a LR datapath, find LRPs with route exchange options
> > + * that are bound locally. */
> > + for (size_t i = 0; i < ld->n_peer_ports; i++) {
> > + const struct sbrec_port_binding *local_peer
> > + = ld->peer_ports[i].local;
> > + const struct sbrec_port_binding *repb = find_route_exchange_pb(
> > + r_ctx_in->sbrec_port_binding_by_name,
> > + r_ctx_in->chassis,
> > + r_ctx_in->active_tunnels,
> > + local_peer);
> > + if (!repb) {
> > + continue;
> > + }
> > +
> > + ad->maintain_vrf |= smap_get_bool(
> > + &repb->options, "dynamic-routing-maintain-vrf", false);
> > + sset_add(&ad->bound_ports, local_peer->logical_port);
> > + }
> > +
> > + if (sset_is_empty(&ad->bound_ports)) {
> > + advertise_datapath_cleanup(ad);
> > + continue;
> > + }
> > + tracked_datapath_add(ld->datapath, TRACKED_RESOURCE_NEW,
> > + r_ctx_out->tracked_re_datapaths);
> > +
> > + /* While tunnel_key would most likely never be negative, the
> > compiler
> > + * has opinions if we don't check before using it in snprintf
> > below. */
> > + if (ld->datapath->tunnel_key < 0 ||
> > + ld->datapath->tunnel_key > MAX_TABLE_ID) {
>
> This seems unnecessary. The SB schema sets a maximum for the datapath
> binding tunnel_key ('"maxInteger": 16777215'). I guess we don't need
> MAX_TABLE_ID at all if we remove the check.
>
> > + VLOG_WARN_RL(&rl,
> > + "skip route sync for datapath "UUID_FMT", "
> > + "tunnel_key %"PRIi64" would make VRF interface
> > name "
> > + "overflow.",
> > + UUID_ARGS(&ld->datapath->header_.uuid),
> > + ld->datapath->tunnel_key);
> > + goto cleanup;
> > + }
> > +
> > + hmap_insert(r_ctx_out->announce_routes, &ad->node,
> > ad->db->tunnel_key);
> > + continue;
> > +
> > +cleanup:
> > + advertise_datapath_cleanup(ad);
> > + }
> > +
> > + const struct sbrec_advertised_route *route;
> > + SBREC_ADVERTISED_ROUTE_FOR_EACH (route, r_ctx_in->ovnsb_idl) {
>
> If you use the SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH() macro instead
> then you don't need to pass the IDL pointer. We do that in all places
> in ovn-controller; we pass a pointer to the table instead. See "struct
> binding_ctx_in.port_binding_table" for an example.
>
> > + struct in6_addr prefix;
> > + unsigned int plen;
> > + if (!ip46_parse_cidr(route->ip_prefix, &prefix, &plen)) {
> > + VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in route "
> > + UUID_FMT, route->ip_prefix,
> > + UUID_ARGS(&route->header_.uuid));
> > + continue;
> > + }
> > +
> > + ad = advertise_datapath_find(r_ctx_out->announce_routes,
> > + route->datapath);
> > + if (!ad) {
> > + continue;
> > + }
> > +
>
> Let's do the ip_prefix parsing here instead, only if the datapath is
> configured to advertise anything.
>
> > + struct advertise_route_entry *ar = xzalloc(sizeof(*ar));
>
> s/sizeof(*ar)/sizeof *ar/
>
> Also, xmalloc is fine. We're writing all fields immediately anyway.
>
> > + hmap_insert(&ad->routes, &ar->node,
> > + advertise_route_hash(&prefix, plen));
>
> Nit: I know it doesn't matter in this case but it feels weird to already
> insert a partially initialized struct into the hmap. Can we do the
> hmap_insert() two lines lower?
>
> > + ar->addr = prefix;
> > + ar->plen = plen;
> > + }
> > +}
> > +
> > +void
> > +route_cleanup(struct hmap *announce_routes)
> > +{
> > + struct advertise_datapath_entry *ad;
> > + HMAP_FOR_EACH_SAFE (ad, node, announce_routes) {
> > + hmap_remove(announce_routes, &ad->node);
>
> Nit: HMAP_FOR_EACH_POP is shorter.
>
> > + advertise_datapath_cleanup(ad);
> > + }
> > +}
> > diff --git a/controller/route.h b/controller/route.h
> > new file mode 100644
> > index 000000000..6fc5963a5
> > --- /dev/null
> > +++ b/controller/route.h
> > @@ -0,0 +1,68 @@
> > +/*
> > + * Copyright (c) 2024, Canonical, Ltd.
> > + * Copyright (c) 2024, STACKIT GmbH & Co. KG
>
> Nit: 2025
>
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef ROUTE_H
> > +#define ROUTE_H 1
> > +
> > +#include <stdbool.h>
> > +#include <netinet/in.h>
> > +#include "openvswitch/hmap.h"
> > +#include "sset.h"
> > +
> > +struct hmap;
> > +struct ovsdb_idl_index;
> > +struct sbrec_chassis;
> > +struct sbrec_port_binding;
> > +
> > +struct route_ctx_in {
> > + struct ovsdb_idl *ovnsb_idl;
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > + const struct sbrec_chassis *chassis;
> > + const struct sset *active_tunnels;
> > + const struct hmap *local_datapaths;
> > + const struct sset *local_lports;
> > +};
> > +
> > +struct route_ctx_out {
> > + struct hmap *tracked_re_datapaths;
> > + /* Contains struct advertise_datapath_entry */
> > + struct hmap *announce_routes;
> > +};
> > +
> > +struct advertise_datapath_entry {
> > + struct hmap_node node;
> > + const struct sbrec_datapath_binding *db;
> > + bool maintain_vrf;
> > + struct hmap routes;
> > +
> > + /* The name of the port bindings locally bound for this datapath and
> > + * running route exchange logic. */
> > + struct sset bound_ports;
> > +};
> > +
> > +struct advertise_route_entry {
> > + struct hmap_node node;
> > + struct in6_addr addr;
> > + unsigned int plen;
> > +};
> > +
> > +bool route_exchange_relevant_port(const struct sbrec_port_binding *pb);
> > +uint32_t advertise_route_hash(const struct in6_addr *dst, unsigned int
> > plen);
>
> 'advertise_route_hash()` can be static inside route.c. I don't think we
> use it anywhere else (I also checked the remaining patches in the series).
>
> > +void route_run(struct route_ctx_in *, struct route_ctx_out *);
> > +void route_cleanup(struct hmap *announce_routes);
> > +
> > +#endif /* ROUTE_H */
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index 3899c9e80..9244532fa 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -305,6 +305,7 @@ tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
> > controller/ovsport.$(OBJEXT) \
> > controller/patch.$(OBJEXT) \
> > controller/vif-plug.$(OBJEXT) \
> > + controller/route.$(OBJEXT) \
>
> Nit: Could you please move this one line higher? In this makefile
> (most) source files are listed in alphabetical order. We should
> probably see if we need to fix that up everywhere but in this specific
> case it looks a bit nicer if we don't break the ordering. :)
>
> > northd/ipam.$(OBJEXT)
> >
> > # Python tests.
>
> Thanks,
> Dumitru
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
