On Thu, Jun 26, 2025 at 2:29 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for the patch. > I have one small comment below > Acked-by: Xavier Simonart <[email protected]> > Hi Xavier, thank you for the review. > > Thanks > Xavier > > > On Wed, Jun 25, 2025 at 3:58 PM Ales Musil via dev < > [email protected]> 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 <[email protected]> >> --- >> 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). > Good point, I will remove it in v2. > + >> + 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 >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
