On Mon, Sep 22, 2025 at 6:54 PM Jacob Tanenbaum via dev < [email protected]> wrote:
> When a router is created or destroyed add the ability to en-lr-stateful > engine node to compute the new state of the lr_stateful table instead of > recalculating for the whole database. > > Acked-by: Lorenzo Bianconi <[email protected]> > Reported-by: Dumitru Ceara <[email protected]> > Reported-at: https://issues.redhat.com/browse/FDP-757 > Signed-off-by: Jacob Tanenbaum <[email protected]> > > ---- > Hi Jacob, thank you for the v4, I have a few comments down below. > > v4: added ack by Lorenzo. > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 50570b611..95c88ecea 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -146,6 +146,10 @@ lflow_northd_handler(struct engine_node *node, > return EN_UNHANDLED; > } > > + if (northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) { > + return EN_UNHANDLED; > + } > + > const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > > diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c > index 2b6c0d0ff..cfb8fd1a1 100644 > --- a/northd/en-lr-stateful.c > +++ b/northd/en-lr-stateful.c > @@ -93,6 +93,7 @@ en_lr_stateful_init(struct engine_node *node OVS_UNUSED, > struct ed_type_lr_stateful *data = xzalloc(sizeof *data); > lr_stateful_table_init(&data->table); > hmapx_init(&data->trk_data.crupdated); > + hmapx_init(&data->trk_data.deleted); > return data; > } > > @@ -102,6 +103,7 @@ en_lr_stateful_cleanup(void *data_) > struct ed_type_lr_stateful *data = data_; > lr_stateful_table_destroy(&data->table); > hmapx_destroy(&data->trk_data.crupdated); > + hmapx_destroy(&data->trk_data.deleted); > } > > void > @@ -110,6 +112,11 @@ en_lr_stateful_clear_tracked_data(void *data_) > struct ed_type_lr_stateful *data = data_; > > hmapx_clear(&data->trk_data.crupdated); > + struct hmapx_node *hmapx_node; > + HMAPX_FOR_EACH_SAFE (hmapx_node, &data->trk_data.deleted) { > + lr_stateful_record_destroy(hmapx_node->data); > + hmapx_delete(&data->trk_data.deleted, hmapx_node); > + } > data->trk_data.vip_nats_changed = false; > } > > @@ -132,11 +139,10 @@ en_lr_stateful_run(struct engine_node *node, void > *data_) > } > > enum engine_input_handler_result > -lr_stateful_northd_handler(struct engine_node *node, void *data > OVS_UNUSED) > +lr_stateful_northd_handler(struct engine_node *node, void *data_) > { > struct northd_data *northd_data = engine_get_input_data("northd", > node); > - if (!northd_has_tracked_data(&northd_data->trk_data) || > - northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) { > + if (!northd_has_tracked_data(&northd_data->trk_data)) { > return EN_UNHANDLED; > } > > @@ -144,10 +150,6 @@ lr_stateful_northd_handler(struct engine_node *node, > void *data OVS_UNUSED) > * See (lr_stateful_get_input_data()) > * 1. northd_data->lr_datapaths > * This data gets updated when a logical router is created or > deleted. > - * northd engine node presently falls back to full recompute when > - * this happens and so does this node. > - * Note: When we add I-P to the created/deleted logical routers, > we > - * need to revisit this handler. > * > * This node also accesses the router ports of the logical router > * (od->ports). When these logical router ports gets updated, > @@ -164,6 +166,19 @@ lr_stateful_northd_handler(struct engine_node *node, > void *data OVS_UNUSED) > * and (3) if any. > * > * */ > + if (northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) { > + struct ed_type_lr_stateful *data = data_; > + struct lr_stateful_table *table = &data->table; > + > + if (hmap_count(&northd_data->lr_datapaths.datapaths) > > + hmap_count(&table->entries)) { > + table->array = xrealloc(table->array, > + ods_size(&northd_data->lr_datapaths) * > + sizeof *table->array); > Same as the previous patch, we should keep the size aligned to ovn_datapaths vector. Otherwise we might be out of bounds. > + return EN_HANDLED_UPDATED; > Also is it updated when we just change the size of the array? There wasn't any data change, we will trigger other nodes with this. > + } > + } > + > return EN_HANDLED_UNCHANGED; > } > > @@ -348,6 +363,17 @@ lr_stateful_lr_nat_handler(struct engine_node *node, > void *data_) > struct ed_type_lr_stateful *data = data_; > struct hmapx_node *hmapx_node; > > + HMAPX_FOR_EACH (hmapx_node, &lr_nat_data->trk_data.deleted) { > + struct lr_nat_record *lr_nat_rec = hmapx_node->data; > + struct lr_stateful_record *lr_stateful_rec = > + lr_stateful_table_find_by_index_(&data->table, > + lr_nat_rec->lr_index); > We should be also careful here, find checks if the index is below the hmap size, that might not be the case when we have holes. The same applies to the previous patch. Maybe we should consider if there is any huge benefit of using the array and indexes and simply do the hmap lookup instead. ls_stateful does it via the hmap too. > + if (lr_stateful_rec) { > + hmap_remove(&data->table.entries, &lr_stateful_rec->key_node); > + hmapx_add(&data->trk_data.deleted, lr_stateful_rec); > + } > + } > + > HMAPX_FOR_EACH (hmapx_node, &lr_nat_data->trk_data.crupdated) { > const struct lr_nat_record *lrnat_rec = hmapx_node->data; > const struct ovn_datapath *od = > diff --git a/northd/en-lr-stateful.h b/northd/en-lr-stateful.h > index 75975c935..3c953d569 100644 > --- a/northd/en-lr-stateful.h > +++ b/northd/en-lr-stateful.h > @@ -107,6 +107,8 @@ struct lr_stateful_table { > struct lr_stateful_tracked_data { > /* Created or updated logical router with LB and/or NAT data. */ > struct hmapx crupdated; /* Stores 'struct lr_stateful_record'. */ > + /* Deleted logical router with LB and/or NAT data. */ > + struct hmapx deleted; > > /* Indicates if any router's NATs changed which were also LB vips > * or vice versa. */ > @@ -145,7 +147,9 @@ const struct lr_stateful_record > *lr_stateful_table_find_by_index( > static inline bool > lr_stateful_has_tracked_data(struct lr_stateful_tracked_data *trk_data) > { > - return !hmapx_is_empty(&trk_data->crupdated) || > trk_data->vip_nats_changed; > + return !hmapx_is_empty(&trk_data->crupdated) || > + !hmapx_is_empty(&trk_data->deleted) || > + trk_data->vip_nats_changed; > } > > static inline bool > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 3c358c232..4c8a6aca2 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12522,7 +12522,7 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > check ovn-nbctl --wait=sb lr-add lr0 > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > -check_engine_stats lr_stateful recompute nocompute > +check_engine_stats lr_stateful norecompute compute > check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > @@ -12533,7 +12533,7 @@ check ovn-nbctl --wait=sb lr-del lr0 > > check_engine_stats northd norecompute compute > check_engine_stats lr_nat norecompute compute > -check_engine_stats lr_stateful recompute nocompute > +check_engine_stats lr_stateful norecompute compute > check_engine_stats sync_to_sb_pb recompute nocompute > check_engine_stats sync_to_sb_lb norecompute compute > check_engine_stats lflow recompute nocompute > -- > 2.51.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
