Hi Ales
Thanks for the patch.
I have one small comment below
Acked-by: Xavier Simonart <xsimo...@redhat.com>
Thanks
Xavier
On Wed, Jun 25, 2025 at 3:58 PM Ales Musil via dev <ovs-dev@openvswitch.org>
wrote:
> There wasn't any check if SB is writable when we tried to commit
> learned routes into SB, this would lead to assert and crash. In
> order to prevent that track the SB state and add additional handler
> that will trigger recompute of the route_exchange node if there are
> SB changes pending and the SB is writable again.
>
> Fixes: 866a5014ae45 ("controller: Support learning routes.")
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> controller/ovn-controller.c | 27 ++++++++++++++++++++++++++-
> controller/route-exchange.c | 10 ++++++++--
> controller/route-exchange.h | 1 +
> 3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index cd568de01..7c606906e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5308,6 +5308,9 @@ route_sb_advertised_route_data_handler(struct
> engine_node *node, void *data)
> struct ed_type_route_exchange {
> /* We need the idl to check if the Learned_Route table exists. */
> struct ovsdb_idl *sb_idl;
> + /* Set to true when SB is readonly and we have routes that need
> + * to be inserted into SB. */
> + bool sb_changes_pending;
> };
>
> static enum engine_node_state
> @@ -5341,7 +5344,9 @@ en_route_exchange_run(struct engine_node *node, void
> *data)
> .sbrec_port_binding_by_name = sbrec_port_binding_by_name,
> .announce_routes = &route_data->announce_routes,
> };
> - struct route_exchange_ctx_out r_ctx_out = {};
> + struct route_exchange_ctx_out r_ctx_out = {
> + .sb_changes_pending = false,
> + };
>
> hmap_init(&r_ctx_out.route_table_watches);
>
> @@ -5351,9 +5356,27 @@ en_route_exchange_run(struct engine_node *node,
> void *data)
> route_table_watch_request_cleanup(&r_ctx_out.route_table_watches);
> hmap_destroy(&r_ctx_out.route_table_watches);
>
> + re->sb_changes_pending = r_ctx_out.sb_changes_pending;
> +
> return EN_UPDATED;
> }
>
> +static enum engine_input_handler_result
> +route_exchange_sb_ro_handler(struct engine_node *node OVS_UNUSED, void
> *data)
> +{
> + /* Return right away if the SB is read only. */
> + if (!engine_get_context()->ovnsb_idl_txn) {
> + return EN_HANDLED_UNCHANGED;
> + }
>
I do not think that this is needed - sb_ro node is only updated when sb is
writable,
so this handler only runs when sb is writable (we have a txn).
> +
> + struct ed_type_route_exchange *re = data;
> + if (re->sb_changes_pending) {
> + return EN_UNHANDLED;
> + }
> +
> + return EN_HANDLED_UNCHANGED;
> +}
> +
>
> static void *
> en_route_exchange_init(struct engine_node *node OVS_UNUSED,
> @@ -5966,6 +5989,8 @@ main(int argc, char *argv[])
> engine_noop_handler);
> engine_add_input(&en_route_exchange, &en_route_table_notify, NULL);
> engine_add_input(&en_route_exchange, &en_route_exchange_status, NULL);
> + engine_add_input(&en_route_exchange, &en_sb_ro,
> + route_exchange_sb_ro_handler);
>
> engine_add_input(&en_addr_sets, &en_sb_address_set,
> addr_sets_sb_address_set_handler);
> diff --git a/controller/route-exchange.c b/controller/route-exchange.c
> index 564d87b53..eef58e801 100644
> --- a/controller/route-exchange.c
> +++ b/controller/route-exchange.c
> @@ -137,7 +137,8 @@ sb_sync_learned_routes(const struct vector
> *learned_routes,
> const struct smap *bound_ports,
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> - struct ovsdb_idl_index
> *sbrec_learned_route_by_datapath)
> + struct ovsdb_idl_index
> *sbrec_learned_route_by_datapath,
> + bool *sb_changes_pending)
> {
> struct hmap sync_routes = HMAP_INITIALIZER(&sync_routes);
> const struct sbrec_learned_route *sb_route;
> @@ -186,6 +187,10 @@ sb_sync_learned_routes(const struct vector
> *learned_routes,
> hmap_remove(&sync_routes, &route_e->hmap_node);
> free(route_e);
> } else {
> + if (!ovnsb_idl_txn) {
> + *sb_changes_pending = true;
> + continue;
> + }
> sb_route = sbrec_learned_route_insert(ovnsb_idl_txn);
> sbrec_learned_route_set_datapath(sb_route, datapath);
> sbrec_learned_route_set_logical_port(sb_route,
> logical_port);
> @@ -268,7 +273,8 @@ route_exchange_run(const struct route_exchange_ctx_in
> *r_ctx_in,
> sb_sync_learned_routes(&received_routes, ad->db,
> &ad->bound_ports, r_ctx_in->ovnsb_idl_txn,
> r_ctx_in->sbrec_port_binding_by_name,
> - r_ctx_in->sbrec_learned_route_by_datapath);
> + r_ctx_in->sbrec_learned_route_by_datapath,
> + &r_ctx_out->sb_changes_pending);
>
> route_table_add_watch_request(&r_ctx_out->route_table_watches,
> table_id);
> diff --git a/controller/route-exchange.h b/controller/route-exchange.h
> index 87cac5808..e3791c331 100644
> --- a/controller/route-exchange.h
> +++ b/controller/route-exchange.h
> @@ -31,6 +31,7 @@ struct route_exchange_ctx_in {
>
> struct route_exchange_ctx_out {
> struct hmap route_table_watches;
> + bool sb_changes_pending;
> };
>
> void route_exchange_run(const struct route_exchange_ctx_in *,
> --
> 2.49.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
