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 <[email protected]>
>> ---
>
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
