On 2/4/25 2:59 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, I have two small comments below. With those addressed feel free to add my ack to v7: Acked-by: Dumitru Ceara <[email protected]> > v5->v6: > * addressed review comments > 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 | 193 +++++++++++++++++++++++++++++++++++- > controller/route.c | 181 +++++++++++++++++++++++++++++++++ > controller/route.h | 67 +++++++++++++ > tests/automake.mk | 1 + > 8 files changed, 452 insertions(+), 3 deletions(-) > create mode 100644 controller/route.c > create mode 100644 controller/route.h > > diff --git a/TODO.rst b/TODO.rst > index 2e004b0b1..3d36f08bf 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 766e36382..9c13d48c8 100644 > --- a/controller/automake.mk > +++ b/controller/automake.mk > @@ -53,7 +53,9 @@ controller_ovn_controller_SOURCES = \ > controller/ovn-dns.c \ > controller/ovn-dns.h \ > controller/ecmp-next-hop-monitor.h \ > - controller/ecmp-next-hop-monitor.c > + controller/ecmp-next-hop-monitor.c \ > + 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 63c787bde..bd0ee4d66 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); > > @@ -250,6 +251,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > struct ovsdb_idl_condition chprv = OVSDB_IDL_CONDITION_INIT(&chprv); > struct ovsdb_idl_condition tv = OVSDB_IDL_CONDITION_INIT(&tv); > struct ovsdb_idl_condition nh = OVSDB_IDL_CONDITION_INIT(&nh); > + 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. > @@ -323,6 +325,11 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > */ > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l2gateway"); > sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "l3gateway"); > + > + /* Monitor all advertised routes during startup. 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); I don't remember if we agreed to add a TODO item for this but I still think we should add one. > } > > if (local_ifaces) { > @@ -359,6 +366,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, > sbrec_ip_multicast_add_clause_datapath(&ip_mcast, OVSDB_F_EQ, > uuid); > sbrec_ecmp_nexthop_add_clause_datapath(&nh, OVSDB_F_EQ, uuid); > + sbrec_advertised_route_add_clause_datapath(&ar, OVSDB_F_EQ, > uuid); > } > > /* Datapath groups are immutable, which means a new group record is > @@ -387,6 +395,7 @@ out:; > 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, ecmp_nexthop, &nh), > + sb_table_set_opt_mon_condition(ovnsb_idl, advertised_route, &ar), > }; > > unsigned int expected_cond_seqno = 0; > @@ -407,6 +416,7 @@ out:; > ovsdb_idl_condition_destroy(&chprv); > ovsdb_idl_condition_destroy(&tv); > ovsdb_idl_condition_destroy(&nh); > + ovsdb_idl_condition_destroy(&ar); > return expected_cond_seqno; > } > > @@ -947,7 +957,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, > @@ -4913,6 +4924,173 @@ 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; > + > + 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); > + > + const struct sbrec_advertised_route_table *advertised_route_table = > + EN_OVSDB_GET(engine_get_input("SB_advertised_route", node)); > + > + struct route_ctx_in r_ctx_in = { > + .advertised_route_table = advertised_route_table, > + .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, > + }; > + > + route_cleanup(&re_data->announce_routes); > + tracked_datapaths_clear(r_ctx_out.tracked_re_datapaths); > + route_run(&r_ctx_in, &r_ctx_out); > + 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. > */ > @@ -5204,6 +5382,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 > @@ -5226,6 +5405,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, > @@ -5411,6 +5599,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..dccebe35f > --- /dev/null > +++ b/controller/route.c > @@ -0,0 +1,181 @@ > +/* > + * Copyright (c) 2025, Canonical, Ltd. > + * Copyright (c) 2025, STACKIT GmbH & Co. KG > + * > + * 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" > + > +VLOG_DEFINE_THIS_MODULE(exchange); > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); > + > +bool > +route_exchange_relevant_port(const struct sbrec_port_binding *pb) > +{ > + return pb && smap_get_bool(&pb->options, "dynamic-routing", false); > +} > + > +static 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); > + > + hmap_insert(r_ctx_out->announce_routes, &ad->node, > ad->db->tunnel_key); > + } > + > + const struct sbrec_advertised_route *route; > + SBREC_ADVERTISED_ROUTE_TABLE_FOR_EACH (route, > + r_ctx_in->advertised_route_table) > { > + ad = advertise_datapath_find(r_ctx_out->announce_routes, > + route->datapath); > + if (!ad) { > + continue; > + } > + > + 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; > + } > + > + struct advertise_route_entry *ar = xmalloc(sizeof *ar); > + ar->addr = prefix; > + ar->plen = plen; > + hmap_insert(&ad->routes, &ar->node, > + advertise_route_hash(&prefix, plen)); > + } > +} > + > +void > +route_cleanup(struct hmap *announce_routes) > +{ > + struct advertise_datapath_entry *ad; > + HMAP_FOR_EACH_POP (ad, node, announce_routes) { > + advertise_datapath_cleanup(ad); > + } > +} > diff --git a/controller/route.h b/controller/route.h > new file mode 100644 > index 000000000..bd817e9c2 > --- /dev/null > +++ b/controller/route.h > @@ -0,0 +1,67 @@ > +/* > + * Copyright (c) 2025, Canonical, Ltd. > + * Copyright (c) 2025, STACKIT GmbH & Co. KG > + * > + * 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 { > + const struct sbrec_advertised_route_table *advertised_route_table; > + 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); Super nit: we don't need the argument name, "const struct sbrec_port_binding *" is probably enough. > +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..2c1f1ba27 100644 > --- a/tests/automake.mk > +++ b/tests/automake.mk > @@ -304,6 +304,7 @@ tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \ > controller/ofctrl-seqno.$(OBJEXT) \ > controller/ovsport.$(OBJEXT) \ > controller/patch.$(OBJEXT) \ > + controller/route.$(OBJEXT) \ > controller/vif-plug.$(OBJEXT) \ > northd/ipam.$(OBJEXT) > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
