On Thu, Dec 19, 2024 at 12:17:17PM +0100, Dumitru Ceara wrote: > On 12/19/24 9:43 AM, [email protected] wrote: > > On Wed, 2024-12-18 at 17:09 +0100, Felix Huettner wrote: > >> On Wed, Dec 18, 2024 at 04:30:32PM +0100, Dumitru Ceara wrote: > >>> On 12/18/24 3:50 PM, [email protected] wrote: > >>>> Hi Felix, > >>> > >>> Hi Martin, Felix, > >>> > >>>> I ran into an issue of advertising routes on gateway router ports > >>>> here, > >>>> I was wondering if I just misunderstood your intentions. > >>>> > >>>> On Tue, 2024-12-17 at 15:03 +0100, Felix Huettner via dev wrote: > >>>>> this engine node determines the routes that the ovn-controller > >>>>> should > >>>>> export. > >>>>> > >>>>> Signed-off-by: Felix Huettner <[email protected]> > >>>>> --- > >>>>> controller/automake.mk | 4 +- > >>>>> controller/ovn-controller.c | 191 > >>>>> +++++++++++++++++++++++++++++++++++- > >>>>> controller/route.c | 189 > >>>>> +++++++++++++++++++++++++++++++++++ > >>>>> controller/route.h | 73 ++++++++++++++ > >>>>> tests/automake.mk | 1 + > >>>>> 5 files changed, 456 insertions(+), 2 deletions(-) > >>>>> create mode 100644 controller/route.c > >>>>> create mode 100644 controller/route.h > >>>>> > >>>>> 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/ovn-controller.c b/controller/ovn- > >>>>> controller.c > >>>>> index 157def2a3..df96d77e3 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); > >>>>> > >>>>> @@ -864,7 +865,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, > >>>>> @@ -4804,6 +4806,175 @@ > >>>>> pflow_lflow_output_sb_chassis_handler(struct > >>>>> engine_node *node, > >>>>> return true; > >>>>> } > >>>>> > >>>>> +struct ed_type_route { > >>>>> + /* 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); > >>>>> + > >>>>> + 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 ovsdb_idl_index *sbrec_advertised_route_by_datapath > >>>>> = > >>>>> + engine_ovsdb_node_get_index( > >>>>> + engine_get_input("SB_advertised_route", node), > >>>>> + "datapath"); > >>>>> + > >>>>> + struct route_ctx_in r_ctx_in = { > >>>>> + .ovnsb_idl_txn = engine_get_context()->ovnsb_idl_txn, > >>>>> + .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, > >>>>> + .sbrec_advertised_route_by_datapath = > >>>>> + sbrec_advertised_route_by_datapath, > >>>>> + }; > >>>>> + > >>>>> + struct route_ctx_out r_ctx_out = { > >>>>> + .tracked_re_datapaths = &re_data- > >>>>>> tracked_route_datapaths, > >>>>> + .announce_routes = &re_data->announce_routes, > >>>>> + }; > >>>>> + > >>>>> + 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 OVS_UNUSED) > >>>>> +{ > >>>>> + struct ed_type_route *data = xzalloc(sizeof *data); > >>>>> + > >>>>> + hmap_init(&data->tracked_route_datapaths); > >>>>> + hmap_init(&data->announce_routes); > >>>>> + > >>>>> + 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) { > >>>>> + /* 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)) { > >>>>> + /* 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) { > >>>>> + /* Until we get I-P support for route exchange we > >>>>> need > >>>>> to request > >>>>> + * recompute. */ > >>>>> + return false; > >>>>> + } > >>>>> + > >>>>> + if (route_exchange_relevant_port(sbrec_pb)) { > >>>>> + /* 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) { > >>>>> + /* 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. > >>>>> */ > >>>>> @@ -5012,6 +5183,9 @@ main(int argc, char *argv[]) > >>>>> struct ovsdb_idl_index > >>>>> *sbrec_chassis_template_var_index_by_chassis > >>>>> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > >>>>> > >>>>> &sbrec_chassis_template_var_col_chassis); > >>>>> + struct ovsdb_idl_index > >>>>> *sbrec_advertised_route_index_by_datapath > >>>>> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, > >>>>> + > >>>>> &sbrec_advertised_route_col_datapath); > >>>>> > >>>>> ovsdb_idl_track_add_all(ovnsb_idl_loop.idl); > >>>>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, > >>>>> @@ -5095,6 +5269,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 > >>>>> @@ -5117,6 +5292,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, > >>>>> @@ -5302,6 +5486,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, > >>>>> @@ -5332,6 +5519,8 @@ main(int argc, char *argv[]) > >>>>> > >>>>> sbrec_static_mac_binding_by_datapath); > >>>>> engine_ovsdb_node_add_index(&en_sb_chassis_template_var, > >>>>> "chassis", > >>>>> > >>>>> sbrec_chassis_template_var_index_by_chassis); > >>>>> + engine_ovsdb_node_add_index(&en_sb_advertised_route, > >>>>> "datapath", > >>>>> + > >>>>> sbrec_advertised_route_index_by_datapath); > >>>>> > >>>>> engine_ovsdb_node_add_index(&en_ovs_flow_sample_collector_set, > >>>>> "id", > >>>>> > >>>>> ovsrec_flow_sample_collector_set_by_id); > >>>>> engine_ovsdb_node_add_index(&en_ovs_port, "qos", > >>>>> ovsrec_port_by_qos); > >>>>> diff --git a/controller/route.c b/controller/route.c > >>>>> new file mode 100644 > >>>>> index 000000000..fdff7573a > >>>>> --- /dev/null > >>>>> +++ b/controller/route.c > >>>>> @@ -0,0 +1,189 @@ > >>>>> +/* > >>>>> + * 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/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); > >>>>> + > >>>>> +/* 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_local_crp(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; > >>>>> + } > >>>>> + 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; > >>>>> + } > >>>>> + return lport_lookup_by_name(sbrec_port_binding_by_name, > >>>>> crp); > >>>>> +} > >>>>> + > >>>>> +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); > >>>>> +} > >>>>> + > >>>>> +void > >>>>> +route_run(struct route_ctx_in *r_ctx_in, > >>>>> + struct route_ctx_out *r_ctx_out) > >>>>> +{ > >>>>> + 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; > >>>>> + } > >>>>> + > >>>>> + bool relevant_datapath = false; > >>>>> + struct advertise_datapath_entry *ad = > >>>>> xzalloc(sizeof(*ad)); > >>>>> + ad->key = ld->datapath->tunnel_key; > >>>>> + 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 *sb_crp = > >>>>> find_local_crp( > >>>>> + r_ctx_in->sbrec_port_binding_by_name, > >>>>> + r_ctx_in->chassis, > >>>>> + r_ctx_in->active_tunnels, > >>>>> + local_peer); > >>>>> + if (!route_exchange_relevant_port(sb_crp)) { > >>>>> + continue; > >>>>> + } > >>>>> + > >>>>> + ad->maintain_vrf |= smap_get_bool(&sb_crp- > >>>>>> options, > >>>>> + "maintain-vrf", > >>>>> false); > >>>>> + ad->use_netns |= smap_get_bool(&sb_crp->options, > >>>>> + "use-netns", false); > >>>>> + relevant_datapath = true; > >>>>> + sset_add(&ad->bound_ports, local_peer- > >>>>>> logical_port); > >>>>> + } > >>>>> + > >>>>> + if (!relevant_datapath) { > >>>>> + advertise_datapath_cleanup(ad); > >> > >> Hi Martin, Hi Dumitru, > >> > >>>> > >>>> In the for-loop above, we loop only through the chassis redirect > >>>> ports > >>>> (find_local_crp), which means that "relevant_datapath=true" is > >>>> never > >>>> set for gateway routers. It's bit hard to discern GW router ports > >>>> just > >>> > >>> +1 I think this needs to be fixed. Gateway routers won't have > >>> chassis-redirect ports. And, at least for ovn-kubernetes, that's > >>> exactly the use case we might want BGP integration for. > >> > >> Thats a very good point. I only tested it with chassis-redirect > >> ports. > >>> > >>>> from the data in SB port bindings, but I was wondering if we > >>>> could > >>>> perhaps pass more options from NB LRP to SB PB and decide only > >>>> based on > >>>> those options (and locality). Right now only "dynamic-routing" > >>>> option > >>>> is copied from LRP options to PB options, we could also pass > >>>> "dynamic- > >>>> routing-connected", "dynamic-routing-static (and in the future > >>>> work > >>>> "redistribute-nat") and decide based on those. > >>> > >>> I'm not sure I understand why we'd propagate > >>> "dynamic-routing-connected/static" to the Southbound port binding. > >>> IIUC > >>> those options control what routes to be advertised for a given > >>> router > >>> (or port). And routes to be advertised are written in the > >>> SB.Advertised_Route table by ovn-northd. Why would ovn-controller > >>> care > >>> about those options? > >> > >> My current spontanious idea would be to set an option on all relevant > >> Port_Bindings based on LRs with "dynamic-routing" and if the LRP > >> would > >> be relevant for dynamic routing. > >> What i understood for this a dynamic-routing relevant LRP is one > >> where: > >> > >> "LRP is connected to a LS with a localnet LSP" && > >> ("LRP has ha_chassis_group/gateway_chassis set" || "LR has chassis > >> set") > >> > >> Would that be appropriate from your perspective or would that still > >> miss > >> something? > >> > >> Then the ovn-controller just checks the option on the Port_Binding > >> and > >> if the port is local. > >> > >> Thanks a lot > >> Felix > > > > Given that northd already associates route with the right datapath, > > wouldn't it be sufficient to cycle through all routes associated with > > the DP and "do stuff" if DP is local? I found some other examples [0] > > of functions within ovn-controller that get the "local_datapaths"[1] > > from "runtime data". > > > > That was my initial thought too. And I think that's fine but I didn't > try it out yet. > > I'm still not sure why we'd need to add more information to the port > binding. But maybe I misunderstood Felix's intention above.
Hi Martin, Hi Dumitru, i wanted to stay on iterating over the Port_Bindings because they can contain information not stored easily in the Datapath. E.g. the filtering of learning routes by ifname. I found a way to (from my perspective) nicely do this. I will try to post this later today as a v2. It includes only smaller changes to to code here as well as a testcase for this scenario. Thanks a lot Felix > > > [0]https://github.com/ovn-org/ovn/blob/ebe5d70122ce0f74067858f5cb19276c852a81da/controller/ovn-controller.c#L5753 > > [1] > > https://github.com/ovn-org/ovn/blob/ebe5d70122ce0f74067858f5cb19276c852a81da/controller/local_data.h#L44 > > > > Martin. > > > >> > >>> > >>> Regards, > >>> Dumitru > >>> > >>>> > >>>> Martin. > >>>> > >>>>> + 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) { > >>>>> + 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; > >>>>> + } > >>>>> + > >>>>> + if (ad->maintain_vrf && ad->use_netns) { > >>>>> + VLOG_WARN_RL(&rl, > >>>>> + "For Datapath %"PRIu64" both > >>>>> maintain-vrf > >>>>> and " > >>>>> + "use-netns are set, this will never > >>>>> work", > >>>>> + ld->datapath->tunnel_key); > >>>>> + goto cleanup; > >>>>> + } > >>>>> + > >>>>> + struct sbrec_advertised_route *route_filter = > >>>>> + sbrec_advertised_route_index_init_row( > >>>>> + r_ctx_in->sbrec_advertised_route_by_datapath); > >>>>> + > >>>>> sbrec_advertised_route_index_set_datapath(route_filter, ld- > >>>>>> datapath); > >>>>> + struct sbrec_advertised_route *route; > >>>>> + SBREC_ADVERTISED_ROUTE_FOR_EACH_EQUAL (route, > >>>>> route_filter, > >>>>> + r_ctx_in->sbrec_advertised_route_by_datapath) > >>>>> { > >>>>> + 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 = > >>>>> xzalloc(sizeof(*ar)); > >>>>> + hmap_insert(&ad->routes, &ar->node, > >>>>> + advertise_route_hash(&prefix, plen)); > >>>>> + ar->addr = prefix; > >>>>> + ar->plen = plen; > >>>>> + } > >>>>> + > >>>>> sbrec_advertised_route_index_destroy_row(route_filter); > >>>>> + > >>>>> + hmap_insert(r_ctx_out->announce_routes, &ad->node, ad- > >>>>>> key); > >>>>> + continue; > >>>>> + > >>>>> +cleanup: > >>>>> + advertise_datapath_cleanup(ad); > >>>>> + } > >>>>> +} > >>>>> + > >>>>> +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); > >>>>> + advertise_datapath_cleanup(ad); > >>>>> + } > >>>>> +} > >>>>> diff --git a/controller/route.h b/controller/route.h > >>>>> new file mode 100644 > >>>>> index 000000000..2a54cf3e3 > >>>>> --- /dev/null > >>>>> +++ b/controller/route.h > >>>>> @@ -0,0 +1,73 @@ > >>>>> +/* > >>>>> + * 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 sset; > >>>>> + > >>>>> +struct route_ctx_in { > >>>>> + struct ovsdb_idl_txn *ovnsb_idl_txn; > >>>>> + struct ovsdb_idl_index *sbrec_port_binding_by_name; > >>>>> + const struct sbrec_chassis *chassis; > >>>>> + const struct sset *active_tunnels; > >>>>> + struct hmap *local_datapaths; > >>>>> + const struct sset *local_lports; > >>>>> + struct ovsdb_idl_index > >>>>> *sbrec_advertised_route_by_datapath; > >>>>> +}; > >>>>> + > >>>>> +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; > >>>>> + /* tunnel_key of the datapath */ > >>>>> + int64_t key; > >>>>> + const struct sbrec_datapath_binding *db; > >>>>> + bool maintain_vrf; > >>>>> + bool use_netns; > >>>>> + 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; > >>>>> + /* used by the route-exchange module to determine if the > >>>>> route > >>>>> is > >>>>> + * already installed */ > >>>>> + bool installed; > >>>>> +}; > >>>>> + > >>>>> +bool route_exchange_relevant_port(const struct > >>>>> sbrec_port_binding > >>>>> *pb); > >>>>> +uint32_t advertise_route_hash(const struct in6_addr *dst, > >>>>> unsigned > >>>>> int plen); > >>>>> +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) \ > >>>>> northd/ipam.$(OBJEXT) > >>>>> > >>>>> # Python tests. > >>>> > >>>> _______________________________________________ > >>>> dev mailing list > >>>> [email protected] > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
