On 7/19/25 3:54 AM, Lorenzo Bianconi via dev wrote:
This patch handles the logical switch creation incrementally
in the northd engine node. The dependent engine nodes - ls_stateful,
lflow and few others still fall back to full recompute, which
will be handled in separate patches.
Reported-at: https://issues.redhat.com/browse/FDP-754
Co-authored-by: Numan Siddique <num...@ovn.org>
Signed-off-by: Numan Siddique <num...@ovn.org>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
---
northd/en-lflow.c | 4 ++
northd/en-ls-stateful.c | 4 ++
northd/inc-proc-northd.c | 5 ++-
northd/northd.c | 84 ++++++++++++++++++++++++++++++++++++----
northd/northd.h | 15 +++++++
tests/ovn-northd.at | 68 ++++++++++++++++++++++++++++++++
6 files changed, 172 insertions(+), 8 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 63565ef80..539113556 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -135,6 +135,10 @@ lflow_northd_handler(struct engine_node *node,
return EN_UNHANDLED;
}
+ if (northd_has_lswitchs_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-ls-stateful.c b/northd/en-ls-stateful.c
index 7f07a6506..448ccc3c4 100644
--- a/northd/en-ls-stateful.c
+++ b/northd/en-ls-stateful.c
@@ -131,6 +131,10 @@ ls_stateful_northd_handler(struct engine_node *node, void
*data_)
return EN_UNHANDLED;
}
+ if (northd_has_lswitchs_in_tracked_data(&northd_data->trk_data)) {
+ return EN_UNHANDLED;
+ }
+
if (!northd_has_ls_lbs_in_tracked_data(&northd_data->trk_data) &&
!northd_has_ls_acls_in_tracked_data(&northd_data->trk_data)) {
return EN_HANDLED_UNCHANGED;
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 6bf8dde3f..b7be79946 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -249,7 +249,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_northd, &en_sb_meter, NULL);
engine_add_input(&en_northd, &en_sb_dns, NULL);
engine_add_input(&en_northd, &en_sb_ha_chassis_group, NULL);
- engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
@@ -278,6 +277,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(&en_northd, &en_nb_port_group,
northd_nb_port_group_handler);
+ /* No need for an explicit handler for the SB datapath and
+ * SB IP Multicast changes.*/
+ engine_add_input(&en_northd, &en_sb_ip_multicast, engine_noop_handler);
+
engine_add_input(&en_lr_nat, &en_northd, lr_nat_northd_handler);
engine_add_input(&en_lr_stateful, &en_northd, lr_stateful_northd_handler);
diff --git a/northd/northd.c b/northd/northd.c
index 0e990c19a..0ec18f516 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -811,8 +811,11 @@ ods_build_array_index(struct ovn_datapaths *datapaths)
* doesn't matter if they are different on every iteration. */
size_t index = 0;
- datapaths->array = xrealloc(datapaths->array,
- ods_size(datapaths) * sizeof *datapaths->array);
+ datapaths->n_array = ods_size(datapaths);
+ datapaths->n_allocated_array = datapaths->n_array + 10;
Later in this patch, I recommend using a vector instead of an open-coded
dynamic array. But, if for some reason the switch to a vector does not
work out, then please make the 10 here a named constant instead of being
hardcoded inline.
+ datapaths->array = xrealloc(
+ datapaths->array,
+ datapaths->n_allocated_array * sizeof *datapaths->array);
struct ovn_datapath *od;
HMAP_FOR_EACH (od, key_node, &datapaths->datapaths) {
@@ -822,6 +825,23 @@ ods_build_array_index(struct ovn_datapaths *datapaths)
}
}
+static void
+ods_assign_array_index(struct ovn_datapaths *datapaths,
+ struct ovn_datapath *od)
+{
+ ovs_assert(ods_size(datapaths) == (datapaths->n_array + 1));
+ od->index = datapaths->n_array;
+
+ if (datapaths->n_array == datapaths->n_allocated_array) {
+ datapaths->n_allocated_array += 10;
+ datapaths->array = xrealloc(
+ datapaths->array,
+ datapaths->n_allocated_array * sizeof *datapaths->array);
+ }
+ datapaths->array[datapaths->n_array++] = od;
+ od->datapaths = datapaths;
+}
+
/* Initializes 'ls_datapaths' to contain a "struct ovn_datapath" for every
* logical switch, and initializes 'lr_datapaths' to contain a
* "struct ovn_datapath" for every logical router.
@@ -4337,6 +4357,12 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
sset_destroy(&active_ha_chassis_grps);
}
+static void
+destroy_tracked_dps(struct tracked_dps *trk_dps)
+{
+ hmapx_clear(&trk_dps->crupdated);
+}
+
static void
destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports)
{
@@ -4373,6 +4399,7 @@ void
destroy_northd_data_tracked_changes(struct northd_data *nd)
{
struct northd_tracked_data *trk_changes = &nd->trk_data;
+ destroy_tracked_dps(&trk_changes->trk_switches);
destroy_tracked_ovn_ports(&trk_changes->trk_lsps);
destroy_tracked_lbs(&trk_changes->trk_lbs);
hmapx_clear(&trk_changes->trk_nat_lrs);
@@ -4386,6 +4413,7 @@ init_northd_tracked_data(struct northd_data *nd)
{
struct northd_tracked_data *trk_data = &nd->trk_data;
trk_data->type = NORTHD_TRACKED_NONE;
+ hmapx_init(&trk_data->trk_switches.crupdated);
hmapx_init(&trk_data->trk_lsps.created);
hmapx_init(&trk_data->trk_lsps.updated);
hmapx_init(&trk_data->trk_lsps.deleted);
@@ -4401,6 +4429,7 @@ destroy_northd_tracked_data(struct northd_data *nd)
{
struct northd_tracked_data *trk_data = &nd->trk_data;
trk_data->type = NORTHD_TRACKED_NONE;
+ hmapx_destroy(&trk_data->trk_switches.crupdated);
hmapx_destroy(&trk_data->trk_lsps.created);
hmapx_destroy(&trk_data->trk_lsps.updated);
hmapx_destroy(&trk_data->trk_lsps.deleted);
@@ -4701,7 +4730,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
for (size_t i = 0; i < changed_ls->n_ports; i++) {
if (nbrec_logical_switch_port_row_get_seqno(
- changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0) {
+ changed_ls->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0 ||
+ !ovn_port_find_in_datapath(od, changed_ls->ports[i])) {
ls_ports_changed = true;
break;
}
@@ -4871,18 +4901,54 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
const struct northd_input *ni,
struct northd_data *nd)
{
- const struct nbrec_logical_switch *changed_ls;
struct northd_tracked_data *trk_data = &nd->trk_data;
+ nd->trk_data.type = NORTHD_TRACKED_NONE;
- if (!hmapx_is_empty(&ni->synced_lses->new) ||
- !hmapx_is_empty(&ni->synced_lses->deleted)) {
+ if (!hmapx_is_empty(&ni->synced_lses->deleted)) {
goto fail;
}
struct hmapx_node *node;
+
+ HMAPX_FOR_EACH (node, &ni->synced_lses->new) {
+ const struct ovn_synced_logical_switch *synced = node->data;
+ const struct nbrec_logical_switch *new_ls = synced->nb;
+
+ /* If a logical switch is created with the below columns set,
+ * then we can't handle this yet. Goto fail. */
+ if (new_ls->copp || new_ls->n_dns_records ||
+ new_ls->n_forwarding_groups || new_ls->n_qos_rules) {
+ goto fail;
+ }
+
+ struct ovn_datapath *od = ovn_datapath_create(
+ &nd->ls_datapaths.datapaths, &new_ls->header_.uuid, new_ls,
+ NULL, synced->sb);
+
+ ods_assign_array_index(&nd->ls_datapaths, od);
+ init_ipam_info_for_datapath(od);
+ init_mcast_info_for_datapath(od);
+
+ /* Create SB:IP_Multicast for the logical switch. */
+ const struct sbrec_ip_multicast *ip_mcast =
+ sbrec_ip_multicast_insert(ovnsb_idl_txn);
+ store_mcast_info_for_switch_datapath(ip_mcast, od);
+
+ if (!ls_handle_lsp_changes(ovnsb_idl_txn, new_ls,
+ ni, nd, od, &trk_data->trk_lsps)) {
+ goto fail;
+ }
+
+ if (new_ls->n_acls) {
+ hmapx_add(&trk_data->ls_with_changed_acls, od);
+ }
+ hmapx_add(&trk_data->trk_switches.crupdated, od);
+ }
+
HMAPX_FOR_EACH (node, &ni->synced_lses->updated) {
const struct ovn_synced_logical_switch *synced = node->data;
- changed_ls = synced->nb;
+ const struct nbrec_logical_switch *changed_ls = synced->nb;
+
struct ovn_datapath *od = ovn_datapath_find_(
&nd->ls_datapaths.datapaths,
&changed_ls->header_.uuid);
@@ -4909,6 +4975,10 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
}
}
+ if (!hmapx_is_empty(&trk_data->trk_switches.crupdated)) {
+ trk_data->type |= NORTHD_TRACKED_SWITCHES;
+ }
+
if (!hmapx_is_empty(&trk_data->trk_lsps.created)
|| !hmapx_is_empty(&trk_data->trk_lsps.updated)
|| !hmapx_is_empty(&trk_data->trk_lsps.deleted)) {
diff --git a/northd/northd.h b/northd/northd.h
index 599a81d79..d0e7a1135 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -90,6 +90,8 @@ struct ovn_datapaths {
/* The array index of each element in 'datapaths'. */
struct ovn_datapath **array;
+ size_t n_array;
+ size_t n_allocated_array;
I think a good guideline to keep in mind is that if you are considering
making a static array dynamic, it's probably a good idea to convert it
to a vector instead of adding dynamic array mechanics on top. Many lines
of code in this patch could be replaced with calls to existing vector
functions instead.
};
static inline size_t
@@ -110,6 +112,11 @@ enum redirected_routing_protcol_flag_type {
REDIRECT_BFD = (1 << 1),
};
+struct tracked_dps {
+ /* Tracked created or updated datapaths. */
+ struct hmapx crupdated;
+};
+
struct tracked_ovn_ports {
/* tracked created ports.
* hmapx node data is 'struct ovn_port *' */
@@ -141,6 +148,7 @@ enum northd_tracked_data_type {
NORTHD_TRACKED_LR_NATS = (1 << 2),
NORTHD_TRACKED_LS_LBS = (1 << 3),
NORTHD_TRACKED_LS_ACLS = (1 << 4),
+ NORTHD_TRACKED_SWITCHES = (1 << 5),
};
/* Track what's changed in the northd engine node.
@@ -149,6 +157,7 @@ enum northd_tracked_data_type {
struct northd_tracked_data {
/* Indicates the type of data tracked. One or all of NORTHD_TRACKED_*. */
enum northd_tracked_data_type type;
+ struct tracked_dps trk_switches;
struct tracked_ovn_ports trk_lsps;
struct tracked_lbs trk_lbs;
@@ -953,6 +962,12 @@ northd_has_ls_acls_in_tracked_data(struct northd_tracked_data *trk_nd_changes)
return trk_nd_changes->type & NORTHD_TRACKED_LS_ACLS;
}
+static inline bool
+northd_has_lswitchs_in_tracked_data(struct northd_tracked_data *trk_nd_changes)
s/lswitchs/lswitches/
+{
+ return trk_nd_changes->type & NORTHD_TRACKED_SWITCHES;
+}
+
/* Returns 'true' if the IPv4 'addr' is on the same subnet with one of the
* IPs configured on the router port.
*/
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index d0843582a..305bd08e2 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -14656,6 +14656,74 @@ AT_CHECK([grep "lr_in_dnat" lr1flows | ovn_strip_lflows | grep
"30.0.0.1"], [0],
AT_CLEANUP
])
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Logical switch incremental processing])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.11
> +> +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+check ovn-nbctl --wait=sb sync
Since this is a test of just northd (the test declaration says "NO_HV"),
the lines after "ovn_start" up to this point are not necessary.
+
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb ls-add sw0
+check_engine_stats northd norecompute compute
+check_engine_stats ls_stateful norecompute compute
+check_engine_stats lflow recompute nocompute
+
+# For the below engine nodes, en_northd is input. So check
+# their engine status.
+check_engine_stats lr_stateful norecompute compute
+check_engine_stats route_policies norecompute compute
+check_engine_stats routes norecompute compute
+check_engine_stats bfd_sync norecompute compute
+check_engine_stats sync_to_sb_lb norecompute compute
+check_engine_stats sync_to_sb_pb norecompute compute
+
+CHECK_NO_CHANGE_AFTER_RECOMPUTE((1))
+
+# Update the logical switch.
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb set logical_switch sw0 other_config:foo=bar
+
+check_engine_stats northd recompute nocompute
+check_engine_stats ls_stateful recompute nocompute
+check_engine_stats lflow recompute nocompute
+
+# For the below engine nodes, en_northd is input. So check
+# their engine status.
+check_engine_stats lr_stateful recompute nocompute
+check_engine_stats route_policies recompute nocompute
+check_engine_stats routes recompute nocompute
+check_engine_stats bfd_sync recompute nocompute
+check_engine_stats sync_to_sb_lb recompute nocompute
+check_engine_stats sync_to_sb_pb recompute nocompute
+
+# Delete the logical switch
+check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats
+check ovn-nbctl --wait=sb ls-del sw0
+check_engine_stats northd recompute compute
+check_engine_stats ls_stateful recompute nocompute
+check_engine_stats lflow recompute nocompute
+
+# For the below engine nodes, en_northd is input. So check
+# their engine status.
+check_engine_stats lr_stateful recompute nocompute
+check_engine_stats route_policies recompute nocompute
+check_engine_stats routes recompute nocompute
+check_engine_stats bfd_sync recompute nocompute
+check_engine_stats sync_to_sb_lb recompute nocompute
+check_engine_stats sync_to_sb_pb recompute nocompute
+
+OVN_CLEANUP([hv1])
If you remove the lines I mentioned earlier in the test, then you'll
also need to remove this OVN_CLEANUP() line as well.
+AT_CLEANUP
+])
+
AT_SETUP([RBAC -- Recover builtin role and permissions])
ovn_start
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
