On 1/31/25 4:05 PM, Dumitru Ceara wrote: > On 1/29/25 12:15 PM, Felix Huettner via dev 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 <felix.huettner@stackit.cloud> >> --- > > Hi Felix, > > This is not a full review, just some things I noticed while trying to > debug some test failures. >
This is a follow up which includes the rest of the review. :) >> controller/automake.mk | 7 +- >> controller/ovn-controller.c | 48 ++++++- >> controller/route-exchange.c | 6 +- >> controller/route-exchange.h | 3 + >> controller/route-table-notify-stub.c | 44 +++++++ >> controller/route-table-notify.c | 179 +++++++++++++++++++++++++++ >> controller/route-table-notify.h | 37 ++++++ >> tests/system-ovn.at | 4 - >> 8 files changed, 319 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 66aff8643..df24a674f 100644 >> --- a/controller/automake.mk >> +++ b/controller/automake.mk >> @@ -53,6 +53,7 @@ controller_ovn_controller_SOURCES = \ >> controller/ovn-dns.c \ >> controller/ovn-dns.h \ >> controller/route-exchange.h \ >> + controller/route-table-notify.h \ >> controller/route.h \ >> controller/route.c >> >> @@ -60,10 +61,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 612fccb42..70c3a5d1a 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); >> >> @@ -5104,10 +5105,14 @@ en_route_exchange_run(struct engine_node *node, void >> *data) >> .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); >> + >> + hmap_destroy(&r_ctx_out.route_table_watches); >> >> engine_set_node_state(node, EN_UPDATED); >> } >> @@ -5126,6 +5131,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. >> */ >> @@ -5425,6 +5462,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); >> @@ -5462,6 +5500,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); > > I know we for now we didn't really cover the route learning/advertising > incremental processing part but this dependency here will cause > en_route_exchange_run() to execute every time anything changes in the > host routing. > >> >> engine_add_input(&en_addr_sets, &en_sb_address_set, >> addr_sets_sb_address_set_handler); >> @@ -5979,6 +6018,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); >> + route_table_notify_run(&rtn->changed); >> + >> stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, >> time_msec()); >> >> @@ -6254,6 +6297,7 @@ main(int argc, char *argv[]) >> } >> >> binding_wait(); >> + route_table_notify_wait(); >> } >> >> unixctl_server_run(unixctl); >> diff --git a/controller/route-exchange.c b/controller/route-exchange.c >> index dc3e5657c..e7a85eecf 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" >> >> @@ -169,7 +170,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); >> @@ -211,6 +212,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 d23bb37a2..0eb0477ee 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..0a83a3e93 >> --- /dev/null >> +++ b/controller/route-table-notify-stub.c >> @@ -0,0 +1,44 @@ >> +/* >> + * Copyright (c) 2024, STACKIT GmbH & Co. KG > > 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 <stdbool.h> >> + >> +#include "openvswitch/compiler.h" >> +#include "route-table-notify.h" >> + >> +void >> +route_table_notify_run(bool *changed) >> +{ >> + *changed = 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_notify_update_watches(struct hmap *route_table_watches >> OVS_UNUSED) >> +{ >> +} >> + >> diff --git a/controller/route-table-notify.c >> b/controller/route-table-notify.c >> new file mode 100644 >> index 000000000..0d931e133 >> --- /dev/null >> +++ b/controller/route-table-notify.c >> @@ -0,0 +1,179 @@ >> +/* >> + * Copyright (c) 2024, STACKIT GmbH & Co. KG > > 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 <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; >> + >> +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)); >> +} >> + >> +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) { >> + any_route_table_changed = true; >> + } >> +} > > What do you think of this to avoid having to do en_route_exchange_run() > for each and every host route table change? > > 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) { >> + VLOG_WARN("Failed to create ipv4 route table watcher for " It's probably a good idea to rate limit this warning message. >> + "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) { >> + VLOG_WARN("Failed to create ipv6 route table watcher for " It's probably a good idea to rate limit this warning message. >> + "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); Nit: fits on one line. >> + nln_notifier_destroy(we->route_notifier); >> + nln_notifier_destroy(we->route6_notifier); >> + nln_destroy(we->nln); >> + free(we); >> +} >> + >> +void >> +route_table_notify_run(bool *changed) >> +{ >> + any_route_table_changed = false; >> + >> + struct route_table_watch_entry *we; >> + HMAP_FOR_EACH (we, node, &watches) { >> + nln_run(we->nln); >> + } >> + >> + *changed = 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); >> + } >> +} >> + >> +static struct route_table_watch_entry* > > Nit: missing space before '*'. > >> +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; >> +} >> + >> +void >> +route_table_notify_update_watches(struct hmap *route_table_watches) This should be 'const struct hmap *route_table_watches'. I know we remove from the map below but we don't have to. This is the "desired" state we want to reconcile the host to. >> +{ >> + 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_SAFE (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); >> + } >> + hmap_remove(route_table_watches, &wr->node); >> + free(wr); No need to remove here, so no need to free and that means HMAP_FOR_EACH() is enough. >> + } >> + >> + struct hmapx_node *node; >> + HMAPX_FOR_EACH (node, &sync_watches) { >> + remove_watch_entry(node->data); >> + } Kind of a nit because I assume exiting the process will probably cleanup fine but shouldn't we remove all watch entries on exit like we do with other resources? >> +} >> diff --git a/controller/route-table-notify.h >> b/controller/route-table-notify.h >> new file mode 100644 >> index 000000000..b7211f6e7 >> --- /dev/null >> +++ b/controller/route-table-notify.h >> @@ -0,0 +1,37 @@ >> +/* >> + * Copyright (c) 2024, STACKIT GmbH & Co. KG > > 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_TABLE_NOTIFY_H >> +#define ROUTE_TABLE_NOTIFY_H 1 >> + >> +#include <stdbool.h> >> +#include "openvswitch/hmap.h" >> + >> +/* Sets "changed" to true if any route table has changed enough that we need >> + * to learn new routes. */ >> +void route_table_notify_run(bool *changed); Why not just return true if any relevant table changed and false otherwise? It feels a bit weird to do that through a 'bool *' argument. >> +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); >> + >> +/* 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(struct hmap *route_table_watches); >> + >> +#endif /* ROUTE_TABLE_NOTIFY_H */ >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index ef43e242e..3f5672dad 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -15075,8 +15075,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) >> @@ -15324,8 +15322,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) > Thanks, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev