On Thu, Jan 16, 2025 at 02:11:43PM +0100, Dumitru Ceara wrote:
> Hi Felix,
Hi Dumitru,
thanks for the review.
>
> 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 <felix.huettner@stackit.cloud>
> > ---
> > 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);
Yea, that is better :)
>
> > 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?
Yes, makes it nicer.
>
> > + }
> > + }
> > +
> > 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.
+1
>
> > +
> > 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.
Thanks for the fix above.
>
> 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?
I added logging for that.
>
> > + 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.
Yep, done
>
> > +
>
> 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 a lot,
Felix
>
> Thanks,
> Dumitru
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
