On 2/6/25 10:48 AM, Frode Nordahl wrote: > On Thu, Feb 6, 2025 at 12:24 AM Lorenzo Bianconi > <[email protected]> wrote: >> >>> For each vrf we use we open a netlink watcher. >>> This allows us to reconcile on changed route entries from outside >>> routing agents. >>> >>> Signed-off-by: Felix Huettner <[email protected]> >> >> Hi Felix, >> >> the patch seems fine to me. >> Acked-by: Lorenzo Bianconi <[email protected]> >> >> Regards, >> Lorenzo > > Thanks for the patch! > > I have a variable name change request and a request for a TODO (if you > agree) below. > > In the interest of time we can keep the current implementation, as I > don't expect anyone to add thousands of routers with dynamic-routing > on them, it will rather be aggregated in a few. However, I don't > understand why you need a watcher per VRF. Why not filter and route > the changes to their appropriate destinations in the callback function > that you provide? Please add this as a TODO if you agree.
Would this incremental work (still waiting for CI on it)? https://github.com/dceara/ovn/commit/5be1e32024f092d1ff2092f5c1f7c1dd6bc1b250 If so, would it be worth squashing this in v7? > > With those addressed: > Acked-by: Frode Nordahl <[email protected]> > >> >>> --- >>> v5->v6: >>> * addressed review comments >>> >>> controller/automake.mk | 7 +- >>> controller/ovn-controller.c | 51 ++++++- >>> controller/route-exchange.c | 6 +- >>> controller/route-exchange.h | 3 + >>> controller/route-table-notify-stub.c | 56 ++++++++ >>> controller/route-table-notify.c | 198 +++++++++++++++++++++++++++ >>> controller/route-table-notify.h | 44 ++++++ >>> tests/system-ovn.at | 4 - >>> 8 files changed, 360 insertions(+), 9 deletions(-) >>> create mode 100644 controller/route-table-notify-stub.c >>> create mode 100644 controller/route-table-notify.c >>> create mode 100644 controller/route-table-notify.h >>> >>> diff --git a/controller/automake.mk b/controller/automake.mk >>> index fa3083d2d..c789c313c 100644 >>> --- a/controller/automake.mk >>> +++ b/controller/automake.mk >>> @@ -55,6 +55,7 @@ controller_ovn_controller_SOURCES = \ >>> controller/ecmp-next-hop-monitor.h \ >>> controller/ecmp-next-hop-monitor.c \ >>> controller/route-exchange.h \ >>> + controller/route-table-notify.h \ >>> controller/route.h \ >>> controller/route.c >>> >>> @@ -62,10 +63,12 @@ if HAVE_NETLINK >>> controller_ovn_controller_SOURCES += \ >>> controller/route-exchange-netlink.h \ >>> controller/route-exchange-netlink.c \ >>> - controller/route-exchange.c >>> + controller/route-exchange.c \ >>> + controller/route-table-notify.c >>> else >>> controller_ovn_controller_SOURCES += \ >>> - controller/route-exchange-stub.c >>> + controller/route-exchange-stub.c \ >>> + controller/route-table-notify-stub.c >>> endif >>> >>> controller_ovn_controller_LDADD = lib/libovn.la >>> $(OVS_LIBDIR)/libopenvswitch.la >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 5d54fc04d..e819733e9 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -90,6 +90,7 @@ >>> #include "ovn-dns.h" >>> #include "route.h" >>> #include "route-exchange.h" >>> +#include "route-table-notify.h" >>> >>> VLOG_DEFINE_THIS_MODULE(main); >>> >>> @@ -5205,10 +5206,16 @@ en_route_exchange_run(struct engine_node *node, >>> void *data) >>> .sbrec_port_binding_by_name = sbrec_port_binding_by_name, >>> .announce_routes = &route_data->announce_routes, >>> }; >>> - struct route_exchange_ctx_out r_ctx_out = { >>> - }; >>> + struct route_exchange_ctx_out r_ctx_out = {}; >>> + >>> + hmap_init(&r_ctx_out.route_table_watches); >>> >>> route_exchange_run(&r_ctx_in, &r_ctx_out); >>> + route_table_notify_update_watches(&r_ctx_out.route_table_watches); >>> + >>> + route_table_watch_request_cleanup(&r_ctx_out.route_table_watches); >>> + hmap_destroy(&r_ctx_out.route_table_watches); >>> + >>> engine_set_node_state(node, EN_UPDATED); >>> } >>> >>> @@ -5226,6 +5233,38 @@ static void >>> en_route_exchange_cleanup(void *data OVS_UNUSED) >>> {} >>> >>> +struct ed_type_route_table_notify { >>> + /* For incremental processing this could be tracked per datapath in >>> + * the future. */ >>> + bool changed; >>> +}; >>> + >>> +static void >>> +en_route_table_notify_run(struct engine_node *node, void *data) >>> +{ >>> + struct ed_type_route_table_notify *rtn = data; >>> + if (rtn->changed) { >>> + engine_set_node_state(node, EN_UPDATED); >>> + } else { >>> + engine_set_node_state(node, EN_UNCHANGED); >>> + } >>> + rtn->changed = false; >>> +} >>> + >>> + >>> +static void * >>> +en_route_table_notify_init(struct engine_node *node OVS_UNUSED, >>> + struct engine_arg *arg OVS_UNUSED) >>> +{ >>> + struct ed_type_route_table_notify *rtn = xzalloc(sizeof *rtn); >>> + rtn->changed = true; >>> + return rtn; >>> +} >>> + >>> +static void >>> +en_route_table_notify_cleanup(void *data OVS_UNUSED) >>> +{} >>> + >>> /* Returns false if the northd internal version stored in SB_Global >>> * and ovn-controller internal version don't match. >>> */ >>> @@ -5525,6 +5564,7 @@ main(int argc, char *argv[]) >>> ENGINE_NODE(bfd_chassis, "bfd_chassis"); >>> ENGINE_NODE(dns_cache, "dns_cache"); >>> ENGINE_NODE(route, "route"); >>> + ENGINE_NODE(route_table_notify, "route_table_notify"); >>> ENGINE_NODE(route_exchange, "route_exchange"); >>> >>> #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); >>> @@ -5562,6 +5602,7 @@ main(int argc, char *argv[]) >>> engine_noop_handler); >>> engine_add_input(&en_route_exchange, &en_sb_port_binding, >>> engine_noop_handler); >>> + engine_add_input(&en_route_exchange, &en_route_table_notify, NULL); >>> >>> engine_add_input(&en_addr_sets, &en_sb_address_set, >>> addr_sets_sb_address_set_handler); >>> @@ -6080,6 +6121,10 @@ main(int argc, char *argv[]) >>> &transport_zones, >>> bridge_table); >>> >>> + struct ed_type_route_table_notify *rtn = >>> + engine_get_internal_data(&en_route_table_notify); >>> + rtn->changed = route_table_notify_run(); >>> + >>> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, >>> time_msec()); >>> >>> @@ -6362,6 +6407,7 @@ main(int argc, char *argv[]) >>> } >>> >>> binding_wait(); >>> + route_table_notify_wait(); >>> } >>> >>> unixctl_server_run(unixctl); >>> @@ -6516,6 +6562,7 @@ loop_done: >>> ovsrcu_exit(); >>> dns_resolve_destroy(); >>> route_exchange_destroy(); >>> + route_table_notify_destroy(); >>> >>> exit(retval); >>> } >>> diff --git a/controller/route-exchange.c b/controller/route-exchange.c >>> index a1976234b..89d07eeb7 100644 >>> --- a/controller/route-exchange.c >>> +++ b/controller/route-exchange.c >>> @@ -29,6 +29,7 @@ >>> #include "ha-chassis.h" >>> #include "local_data.h" >>> #include "route.h" >>> +#include "route-table-notify.h" >>> #include "route-exchange.h" >>> #include "route-exchange-netlink.h" >>> >>> @@ -167,7 +168,7 @@ sb_sync_learned_routes(const struct ovs_list >>> *learned_routes, >>> >>> void >>> route_exchange_run(struct route_exchange_ctx_in *r_ctx_in, >>> - struct route_exchange_ctx_out *r_ctx_out OVS_UNUSED) >>> + struct route_exchange_ctx_out *r_ctx_out) >>> { >>> struct sset old_maintained_vrfs = >>> SSET_INITIALIZER(&old_maintained_vrfs); >>> sset_swap(&_maintained_vrfs, &old_maintained_vrfs); >>> @@ -209,6 +210,9 @@ route_exchange_run(struct route_exchange_ctx_in >>> *r_ctx_in, >>> r_ctx_in->sbrec_port_binding_by_name, >>> r_ctx_in->sbrec_learned_route_by_datapath); >>> >>> + route_table_add_watch_request( >>> + &r_ctx_out->route_table_watches, table_id); >>> + >>> re_nl_learned_routes_destroy(&received_routes); >>> } >>> >>> diff --git a/controller/route-exchange.h b/controller/route-exchange.h >>> index 66d8f3945..1e1a09da3 100644 >>> --- a/controller/route-exchange.h >>> +++ b/controller/route-exchange.h >>> @@ -18,6 +18,8 @@ >>> #ifndef ROUTE_EXCHANGE_H >>> #define ROUTE_EXCHANGE_H 1 >>> >>> +#include "openvswitch/hmap.h" >>> + >>> struct route_exchange_ctx_in { >>> struct ovsdb_idl_txn *ovnsb_idl_txn; >>> struct ovsdb_idl_index *sbrec_port_binding_by_name; >>> @@ -27,6 +29,7 @@ struct route_exchange_ctx_in { >>> }; >>> >>> struct route_exchange_ctx_out { >>> + struct hmap route_table_watches; >>> }; >>> >>> void route_exchange_run(struct route_exchange_ctx_in *, >>> diff --git a/controller/route-table-notify-stub.c >>> b/controller/route-table-notify-stub.c >>> new file mode 100644 >>> index 000000000..f58897f03 >>> --- /dev/null >>> +++ b/controller/route-table-notify-stub.c >>> @@ -0,0 +1,56 @@ >>> +/* >>> + * 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 <stdbool.h> >>> + >>> +#include "openvswitch/compiler.h" >>> +#include "route-table-notify.h" >>> + >>> +bool >>> +route_table_notify_run(void) >>> +{ >>> + return false; >>> +} >>> + >>> +void >>> +route_table_notify_wait(void) >>> +{ >>> +} >>> + >>> +void >>> +route_table_add_watch_request(struct hmap *route_table_watches OVS_UNUSED, >>> + uint32_t table_id OVS_UNUSED) >>> +{ >>> +} >>> + >>> +void >>> +route_table_watch_request_cleanup(struct hmap *route_table_watches >>> OVS_UNUSED) >>> +{ >>> +} >>> + >>> +void >>> +route_table_notify_update_watches( >>> + const struct hmap *route_table_watches OVS_UNUSED) >>> +{ >>> +} >>> + >>> +void >>> +route_table_notify_destroy(void) >>> +{ >>> +} >>> + >>> diff --git a/controller/route-table-notify.c >>> b/controller/route-table-notify.c >>> new file mode 100644 >>> index 000000000..b79f50eba >>> --- /dev/null >>> +++ b/controller/route-table-notify.c >>> @@ -0,0 +1,198 @@ >>> +/* >>> + * 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 <linux/rtnetlink.h> >>> + >>> +#include "netlink-notifier.h" >>> +#include "openvswitch/vlog.h" >>> + >>> +#include "binding.h" >>> +#include "hash.h" >>> +#include "hmapx.h" >>> +#include "route-table.h" >>> +#include "route.h" >>> +#include "route-table-notify.h" >>> +#include "route-exchange-netlink.h" >>> + >>> +VLOG_DEFINE_THIS_MODULE(route_table_notify); >>> + >>> +struct route_table_watch_request { >>> + struct hmap_node node; >>> + uint32_t table_id; >>> +}; >>> + >>> +struct route_table_watch_entry { >>> + struct hmap_node node; >>> + uint32_t table_id; >>> + struct nln *nln; >>> + struct nln_notifier *route_notifier; >>> + struct nln_notifier *route6_notifier; >>> +}; >>> + >>> +static struct hmap watches = HMAP_INITIALIZER(&watches); >>> +static bool any_route_table_changed; >>> +static struct route_table_msg rtmsg; > > nit: As highlighted in > https://github.com/openvswitch/ovs/commit/910bc81e66ba98487469efb95997788d91c2853f, > the above variable name can be very confusing. Would you consider a > different name for it? > > -- > Frode Nordahl > >>> + >>> +static uint32_t >>> +route_table_notify_hash_watch(uint32_t table_id) >>> +{ >>> + return hash_add(0, table_id); >>> +} >>> + >>> +void >>> +route_table_add_watch_request(struct hmap *route_table_watches, >>> + uint32_t table_id) >>> +{ >>> + struct route_table_watch_request *wr = xzalloc(sizeof *wr); >>> + wr->table_id = table_id; >>> + hmap_insert(route_table_watches, &wr->node, >>> + route_table_notify_hash_watch(wr->table_id)); >>> +} >>> + >>> +void >>> +route_table_watch_request_cleanup(struct hmap *route_table_watches) >>> +{ >>> + struct route_table_watch_request *wr; >>> + HMAP_FOR_EACH_POP (wr, node, route_table_watches) { >>> + free(wr); >>> + } >>> +} >>> + >>> +static struct route_table_watch_entry * >>> +find_watch_entry(uint32_t table_id) >>> +{ >>> + struct route_table_watch_entry *we; >>> + uint32_t hash = route_table_notify_hash_watch(table_id); >>> + HMAP_FOR_EACH_WITH_HASH (we, node, hash, &watches) { >>> + if (table_id == we->table_id) { >>> + return we; >>> + } >>> + } >>> + return NULL; >>> +} >>> + >>> +static void >>> +route_table_change(const void *change_, void *aux OVS_UNUSED) >>> +{ >>> + const struct route_table_msg *change = change_; >>> + if (change && change->rd.rtm_protocol != RTPROT_OVN) { >>> + if (find_watch_entry(change->rd.rta_table_id)) { >>> + any_route_table_changed = true; >>> + } >>> + } >>> +} >>> + >>> +static void >>> +add_watch_entry(uint32_t table_id) >>> +{ >>> + struct route_table_watch_entry *we; >>> + uint32_t hash = route_table_notify_hash_watch(table_id); >>> + we = xzalloc(sizeof *we); >>> + we->table_id = table_id; >>> + VLOG_INFO("Registering new route table watcher for table %d.", >>> + table_id); >>> + we->nln = nln_create(NETLINK_ROUTE, route_table_parse, &rtmsg); >>> + >>> + we->route_notifier = >>> + nln_notifier_create(we->nln, RTNLGRP_IPV4_ROUTE, >>> + route_table_change, NULL); >>> + if (!we->route_notifier) { >>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >>> + VLOG_WARN_RL(&rl, "Failed to create ipv4 route table watcher for " >>> + "table %d. We will miss routing table updates.", >>> table_id); >>> + } >>> + >>> + we->route6_notifier = >>> + nln_notifier_create(we->nln, RTNLGRP_IPV6_ROUTE, >>> + route_table_change, NULL); >>> + if (!we->route6_notifier) { >>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >>> + VLOG_WARN_RL(&rl, "Failed to create ipv6 route table watcher for " >>> + "table %d. We will miss routing table updates.", >>> table_id); >>> + } >>> + >>> + hmap_insert(&watches, &we->node, hash); >>> +} >>> + >>> +static void >>> +remove_watch_entry(struct route_table_watch_entry *we) >>> +{ >>> + hmap_remove(&watches, &we->node); >>> + VLOG_INFO("Removing route table watcher for table %d.", we->table_id); >>> + nln_notifier_destroy(we->route_notifier); >>> + nln_notifier_destroy(we->route6_notifier); >>> + nln_destroy(we->nln); >>> + free(we); >>> +} >>> + >>> +bool >>> +route_table_notify_run(void) >>> +{ >>> + any_route_table_changed = false; >>> + >>> + struct route_table_watch_entry *we; >>> + HMAP_FOR_EACH (we, node, &watches) { >>> + nln_run(we->nln); >>> + } >>> + >>> + return any_route_table_changed; >>> +} >>> + >>> +void >>> +route_table_notify_wait(void) >>> +{ >>> + struct route_table_watch_entry *we; >>> + HMAP_FOR_EACH (we, node, &watches) { >>> + nln_wait(we->nln); >>> + } >>> +} >>> + >>> +void >>> +route_table_notify_update_watches(const struct hmap *route_table_watches) >>> +{ >>> + struct hmapx sync_watches = HMAPX_INITIALIZER(&sync_watches); >>> + struct route_table_watch_entry *we; >>> + HMAP_FOR_EACH (we, node, &watches) { >>> + hmapx_add(&sync_watches, we); >>> + } >>> + >>> + struct route_table_watch_request *wr; >>> + HMAP_FOR_EACH (wr, node, route_table_watches) { >>> + we = find_watch_entry(wr->table_id); >>> + if (we) { >>> + hmapx_find_and_delete(&sync_watches, we); >>> + } else { >>> + add_watch_entry(wr->table_id); >>> + } >>> + } >>> + >>> + struct hmapx_node *node; >>> + HMAPX_FOR_EACH (node, &sync_watches) { >>> + remove_watch_entry(node->data); >>> + } >>> +} >>> + >>> +void >>> +route_table_notify_destroy(void) >>> +{ >>> + struct route_table_watch_entry *we; >>> + HMAP_FOR_EACH_SAFE (we, node, &watches) { >>> + remove_watch_entry(we); >>> + } >>> +} >>> diff --git a/controller/route-table-notify.h >>> b/controller/route-table-notify.h >>> new file mode 100644 >>> index 000000000..a2bc05a49 >>> --- /dev/null >>> +++ b/controller/route-table-notify.h >>> @@ -0,0 +1,44 @@ >>> +/* >>> + * 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_TABLE_NOTIFY_H >>> +#define ROUTE_TABLE_NOTIFY_H 1 >>> + >>> +#include <stdbool.h> >>> +#include "openvswitch/hmap.h" >>> + >>> +/* Returns true if any route table has changed enough that we need >>> + * to learn new routes. */ >>> +bool route_table_notify_run(void); >>> +void route_table_notify_wait(void); >>> + >>> +/* Add a watch request to the hmap. The hmap should later be passed to >>> + * route_table_notify_update_watches*/ >>> +void route_table_add_watch_request(struct hmap *route_table_watches, >>> + uint32_t table_id); >>> + >>> +/* Cleanup all watch request in the provided hmap that where added using >>> + * route_table_add_watch_request. */ >>> +void route_table_watch_request_cleanup(struct hmap *route_table_watches); >>> + >>> +/* Updates the list of route table watches that are currently active. >>> + * hmap should contain struct route_table_watch_request */ >>> +void route_table_notify_update_watches(const struct hmap >>> *route_table_watches); >>> + >>> +/* Cleans up all route table watches. */ >>> +void route_table_notify_destroy(void); >>> + >>> +#endif /* ROUTE_TABLE_NOTIFY_H */ >>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >>> index a13ee4bbf..8b3238a96 100644 >>> --- a/tests/system-ovn.at >>> +++ b/tests/system-ovn.at >>> @@ -15677,8 +15677,6 @@ blackhole 198.51.100.0/24 proto 84 metric 1000]) >>> # Now we test route learning. >>> check_row_count Learned_Route 0 >>> check ip route add 233.252.0.0/24 via 192.168.10.10 dev lo onlink vrf >>> ovnvrf1337 >>> -# For now we trigger a recompute as route watching is not yet implemented. >>> -check ovn-appctl -t ovn-controller inc-engine/recompute >>> check ovn-nbctl --wait=hv sync >>> check_row_count Learned_Route 1 >>> lp=$(fetch_column port_binding _uuid logical_port=internet-phys) >>> @@ -15925,8 +15923,6 @@ blackhole 198.51.100.0/24 proto 84 metric 1000]) >>> # Now we test route learning. >>> check_row_count Learned_Route 0 >>> check ip route add 233.252.0.0/24 via 192.168.10.10 dev lo onlink vrf >>> ovnvrf1337 >>> -# For now we trigger a recompute as route watching is not yet implemented. >>> -check ovn-appctl -t ovn-controller inc-engine/recompute >>> check ovn-nbctl --wait=hv sync >>> check_row_count Learned_Route 1 >>> lp=$(fetch_column port_binding _uuid logical_port=internet-phys) >>> -- >>> 2.47.1 >>> >>> >>> _______________________________________________ >>> 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 > _______________________________________________ > 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
