Hi Jacob,
I think this patch is not approaching the problem correctly. When a new
logical router appears, en_northd should place the new ovn_datapath into
its tracked data. Then en_lr_nat_northd_handler() will detect the new
logical router and create a new lr_nat_record to correspond with the new
logical router. Then en_lr_stateful_lr_nat_handler() will detect the new
lr_nat_record and create a new lr_stateful_record. The
en_lr_stateful_northd_handler() should not be creating new
lr_stateful_records since en_lr_stateful_lr_nat_handler() should be
doing that. Since en_lr_stateful_northd_handler() runs before
en_lr_stateful_lr_nat_handler(), there's a chance that trying to find an
lr_nat_record for a new logical router will fail since the record has
not been created yet.
That's not to say that en_lr_stateful doesn't require any changes. It
definitely does. The biggest change (which this patch addresses) is that
the lr_stateful_table->array needs to be resized to the size of the
northd->lr_datapaths array. One approach would be to perform this
reallocation in the en_lr_stateful_northd_handler() as you've done here.
Then when en_lr_stateful_lr_nat_handler() runs, there will be no
problems inserting the new lr_stateful_record into the
lr_stateful_table->array. However, it makes more sense to me to perform
the reallocation closer to when the array is going to be used. So it
probably makes more sense to reallocate the array in
en_lr_stateful_lr_nat_handler().
On 7/25/25 5:27 PM, Jacob Tanenbaum via dev wrote:
en_northd engine nodes provides the created or updated logical routers
in its tracked data and en_lr_stateful node handles those changes.
This is built on Mark Michelson's patch series located here
https://patchwork.ozlabs.org/project/ovn/list/?series=465828
this is required because the datapath refactor was required in order to
accomplish incremental processing of the logical routers
also built with the first two patches of this series by Lorenzo Bianconi
located https://patchwork.ozlabs.org/project/ovn/list/?series=466579.
He brought in some changes required to dynamically allocate the sizes of
the datapaths array.
Reported-by: Dumitru Ceara <dce...@redhat.com>
Reported-at: https://issues.redhat.com/browse/FDP-757
Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com>
diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
index 3b1dec0b1..95a18e8ed 100644
--- a/northd/en-lr-stateful.c
+++ b/northd/en-lr-stateful.c
@@ -132,24 +132,38 @@ 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)) {
return EN_UNHANDLED;
}
if (northd_has_lr_new_in_tracked_data(&northd_data->trk_data)) {
- return EN_UNHANDLED;
+ struct lr_stateful_input input_data = lr_stateful_get_input_data(node);
+ struct ed_type_lr_stateful *data = data_;
+
+ struct lr_stateful_table *table = &data->table;
+ const struct ovn_datapaths *lr_datapaths = input_data.lr_datapaths;
+
+ table->array = xrealloc(table->array,
+ ods_size(lr_datapaths) * sizeof *table->array);
+
+ struct hmapx_node *hmapx_node;
+ HMAPX_FOR_EACH (hmapx_node, &northd_data->trk_data.trk_new_lrs) {
+ struct ovn_datapath *od = hmapx_node->data;
+ const struct lr_nat_record *lrnat_rec =
+ lr_nat_table_find_by_index(input_data.lr_nats, od->index);
+ lr_stateful_record_create(table, lrnat_rec, od,
+ input_data.lb_datapaths_map,
+ input_data.lbgrp_datapaths_map);
+ }
+ return EN_HANDLED_UPDATED;
}
/* This node uses the below data from the en_northd engine node.
* 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.
Nit: The northd node still recomputes when a logical router is deleted.
*
* This node also accesses the router ports of the logical router
* (od->ports). When these logical router ports gets updated,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index ff00bedf1..18ac13a92 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -12598,7 +12598,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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
