Hi Felix, On 1/2/25 4:19 PM, Felix Huettner via dev wrote: > for each vrf/network namespace we use we open a netlink watcher.
Nit: For > This allows us to reconcile on changed route entries from outside > routing agents. > > Signed-off-by: Felix Huettner <[email protected]> > --- > controller/automake.mk | 7 +- > controller/ovn-controller.c | 48 +++++++++ > controller/route-exchange-stub.c | 6 -- > controller/route-exchange.c | 8 +- > controller/route-exchange.h | 3 + > controller/route-table-notify-stub.c | 37 +++++++ > controller/route-table-notify.c | 148 +++++++++++++++++++++++++++ > controller/route-table-notify.h | 41 ++++++++ > tests/system-ovn.at | 4 - > 9 files changed, 289 insertions(+), 13 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 f90ab1f59..261b949cd 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); > > @@ -5081,9 +5082,13 @@ en_route_exchange_run(struct engine_node *node, void > *data) > > 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); > + Nit: the spacing seems a bit weird to me. I think I'd do: 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); > } > > @@ -5101,6 +5106,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)); 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. > */ > @@ -5403,6 +5440,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); > @@ -5440,6 +5478,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); > @@ -5957,6 +5996,14 @@ main(int argc, char *argv[]) > &transport_zones, > bridge_table); > > + if (route_table_notify_run()) { > + struct ed_type_route_table_notify *rtn = > + engine_get_internal_data(&en_route_table_notify); > + if (rtn) { engine_get_internal_data() always returns the node->data (regardless of the node's I-P state). So it should always be non-NULL here. > + rtn->changed = true; Maybe it's a bit cleaner if we pass &rtn->changed to route_table_notify_run() instead of setting it here? > + } > + } > + > stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME, > time_msec()); > > @@ -6232,6 +6279,7 @@ main(int argc, char *argv[]) > } > > binding_wait(); > + route_table_notify_wait(); > } > > unixctl_server_run(unixctl); > diff --git a/controller/route-exchange-stub.c > b/controller/route-exchange-stub.c > index 2ca644b06..7225e67a8 100644 > --- a/controller/route-exchange-stub.c > +++ b/controller/route-exchange-stub.c > @@ -19,12 +19,6 @@ > #include "openvswitch/compiler.h" > #include "route-exchange.h" > > -bool > -route_exchange_relevant_port(const struct sbrec_port_binding *pb OVS_UNUSED) > -{ > - return false; > -} > - This doesn't belong in this patch. > void > route_exchange_run(struct route_exchange_ctx_in *r_ctx_in OVS_UNUSED, > struct route_exchange_ctx_out *r_ctx_out OVS_UNUSED) > diff --git a/controller/route-exchange.c b/controller/route-exchange.c > index 77d5ce51d..a9eb4dbfe 100644 > --- a/controller/route-exchange.c > +++ b/controller/route-exchange.c > @@ -25,6 +25,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" > > @@ -187,7 +188,7 @@ sb_sync_learned_routes(const struct > sbrec_datapath_binding *datapath, > > 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); > @@ -228,6 +229,11 @@ route_exchange_run(struct route_exchange_ctx_in > *r_ctx_in, > r_ctx_in->sbrec_learned_route_by_datapath, > r_ctx_in->sbrec_port_binding_by_name); > > + struct route_table_watch_request *wr = xzalloc(sizeof(*wr)); sizeof *wr > + wr->table_id = ad->key; > + hmap_insert(&r_ctx_out->route_table_watches, &wr->node, > + route_table_notify_hash_watch(wr->table_id)); Nit: I'd make the route_table_watch_request creation a function in route-table-notify.[ch]. It encapsulates the functionality a bit better in my opinion. Then we don't need to export/inline the route_table_notify_hash_watch() function. > + > out: > re_nl_received_routes_destroy(&received_routes); > } > diff --git a/controller/route-exchange.h b/controller/route-exchange.h > index d51fba598..8617f9df2 100644 > --- a/controller/route-exchange.h > +++ b/controller/route-exchange.h > @@ -16,6 +16,7 @@ > #define ROUTE_EXCHANGE_H 1 > > #include <stdbool.h> > +#include "openvswitch/hmap.h" > > struct route_exchange_ctx_in { > /* We need the idl to check if a table exists. */ > @@ -28,6 +29,8 @@ struct route_exchange_ctx_in { > }; > > struct route_exchange_ctx_out { > + /* contains route_table_watch */ Nit: this comment is a bit superfluous. > + 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..d6de9852e > --- /dev/null > +++ b/controller/route-table-notify-stub.c > @@ -0,0 +1,37 @@ > +/* Missing copyright. > + * 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_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..dd8b4ffdb > --- /dev/null > +++ b/controller/route-table-notify.c > @@ -0,0 +1,148 @@ > +/* Missing copyright. > + * 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 "route-table.h" > +#include "route.h" > +#include "route-table-notify.h" > +#include "route-exchange-netlink.h" > + > + Nit: too many empty lines. > +VLOG_DEFINE_THIS_MODULE(route_table_notify); > + > +struct route_table_watch_entry { > + struct hmap_node node; > + uint32_t table_id; > + bool is_netns; Unused 'is_netns'. > + struct nln *nln; > + struct nln_notifier *route_notifier; > + struct nln_notifier *route6_notifier; > + /* used in update_watches to ensure we clean up */ > + bool stale; > +}; > + > +static struct hmap watches = HMAP_INITIALIZER(&watches); > +static bool any_route_table_changed = false; Nit: .bss section variables are always zeroed out. > +static struct route_table_msg rtmsg; > + > +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 struct route_table_msg *change OVS_UNUSED, > + void *aux OVS_UNUSED) This should be: static void route_table_change(const void *change_, void *aux OVS_UNUSED) { const struct route_table_msg *change = change_; [...] } to match the prototype expected by nln_notifier_create(). > +{ > + if (change && change->rd.rtm_protocol != RTPROT_OVN) { > + 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)); sizeof *we > + we->table_id = table_id; > + we->stale = false; > + VLOG_DBG("registering new route table watcher for table %d", > + table_id); Wouldn't it be useful to make this a VLOG_INFO? We should probably also log on watcher removal. > + we->nln = nln_create( NETLINK_ROUTE, route_table_parse, &rtmsg); Nit: spacing is wrong. > + > + we->route_notifier = > + nln_notifier_create(we->nln, RTNLGRP_IPV4_ROUTE, > + (nln_notify_func *) route_table_change, NULL); Without the cast we get: error: incompatible function pointer types passing 'void (const struct route_table_msg *, void *)' to parameter of type 'nln_notify_func *' (aka 'void (*)(const void *, void *)') [-Wincompatible-function-pointer-types] We should fix route_table_change to have the correct signature instead. Also, nln_notifier_create() can error out and will return NULL. It generates a VLOG_WARN but do we need additional logging/handling here too? > + we->route6_notifier = > + nln_notifier_create(we->nln, RTNLGRP_IPV6_ROUTE, > + (nln_notify_func *) route_table_change, NULL); > + hmap_insert(&watches, &we->node, hash); > +} > + > +static void > +remove_watch_entry(struct route_table_watch_entry *we) > +{ > + hmap_remove(&watches, &we->node); > + 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(struct hmap *route_table_watches) > +{ > + struct route_table_watch_entry *we; > + HMAP_FOR_EACH (we, node, &watches) { > + we->stale = true; > + } > + > + struct route_table_watch_request *wr; > + HMAP_FOR_EACH_SAFE (wr, node, route_table_watches) { > + we = find_watch_entry(wr->table_id); > + if (we) { > + we->stale = false; > + } else { > + add_watch_entry(wr->table_id); > + } > + hmap_remove(route_table_watches, &wr->node); > + free(wr); > + } > + > + HMAP_FOR_EACH_SAFE (we, node, &watches) { > + if (we->stale) { > + remove_watch_entry(we); > + } > + } We can avoid the need for the "stale" field if we: 1. create a temporary hmapx that stores all current 'watches'. 2. walk route_table_watches and lookup the current 'we': - if found: remove we from hmapx - else: add_watch_entry() 3. everything that's left in the hmapx is stale so we can call remove_watch_entry() for each of those. > + Nit: no need for empty line here. > +} > diff --git a/controller/route-table-notify.h b/controller/route-table-notify.h > new file mode 100644 > index 000000000..63100e283 > --- /dev/null > +++ b/controller/route-table-notify.h > @@ -0,0 +1,41 @@ > +/* Missing copyright. > + * 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" > +#include "hash.h" > + > +struct route_table_watch_request { > + struct hmap_node node; > + uint32_t table_id; > +}; > + > +static inline uint32_t > +route_table_notify_hash_watch(uint32_t table_id) > +{ > + return hash_add(0, table_id); > +} > + > +/* returns true if any route table has changed enough that we need to learn Nit: Returns > + * new routes. */ > +bool route_table_notify_run(void); > +void route_table_notify_wait(void); Nit: newline > +/* updates the list of route table watches that are currently active. Nit: Updates > + * 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 08a9dc418..0b4a240e6 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -14933,8 +14933,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=$(ovn-sbctl --bare --columns _uuid list port_binding internet-phys) > @@ -15184,8 +15182,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=$(ovn-sbctl --bare --columns _uuid list port_binding internet-phys) Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
