On Mon, Feb 03, 2025 at 04:04:30PM +0100, Dumitru Ceara wrote:
> 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. :)
Hi Dumitru,
thanks a lot for the reviews.
>
> >> 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;
> > }
> > }
> > }
> >
Yes that makes it significantly more efficient. I'll add that.
> >> +
> >> +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.
The thing is that this hmap is just passed to here and afterwards needs
to be freed again. So i would leave this here, as we otherwise need to
have a separate loop freeing everything.
>
> >> +{
> >> + 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?
Yea, i'll add a *_destroy.
>
> >> +}
> >> 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.
That came from your comment at
https://patchwork.ozlabs.org/project/ovn/patch/ae5de92e1880da09e7d27770f0b0a3214f0a2985.1735830931.git.felix.huettner@stackit.cloud/
But maybe i misunderstood it?
"""
Maybe it's a bit cleaner if we pass &rtn->changed to
route_table_notify_run() instead of setting it here?
"""
But i can change it back if you want.
Thanks a lot,
Felix
>
> >> +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
