On Tue, Feb 04, 2025 at 12:54:38PM +0100, Dumitru Ceara wrote:
> On 2/4/25 12:41 PM, Felix Huettner wrote:
> > 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 <[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. :)
> >
> > 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.
> >
>
> Are you worried about performance? I don't really think that's an issue
> here. However, I find it confusing that we're removing stuff from
> something that's supposed to be "input" data. I still think we need to
> pass the hmap as const * and clear it afterwards, in the caller. That's
> in line with what we'd have to do if we implemented actual incremental
> processing.
Hi Dumitru,
thanks a lot. That makes sense, i'll change it.
>
> >>
> >>>> +{
> >>>> + 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.
> >
>
> In that version the code was:
> + if (route_table_notify_run()) {
> + struct ed_type_route_table_notify *rtn =
> + engine_get_internal_data(&en_route_table_notify);
> + if (rtn) {
> + rtn->changed = true;
> + }
> + }
> +
>
> In this version the caller is:
> + struct ed_type_route_table_notify *rtn =
> + engine_get_internal_data(&en_route_table_notify);
> + route_table_notify_run(&rtn->changed)
>
> We could:
> + struct ed_type_route_table_notify *rtn =
> + engine_get_internal_data(&en_route_table_notify);
> + rtn->changed = route_table_notify_run()
>
> What do you think?
ah ok, then i just misunderstood it back then.
I'll change it to the new version, that looks good.
Thanks a lot,
Felix
>
> Thanks,
> Dumitru
>
> > 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
