On 7/24/25 1:20 PM, Lorenzo Bianconi 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/en-multicast.c | 4 +
northd/inc-proc-northd.c | 5 +-
northd/northd.c | 173 +++++++++++++++++++++++++++++++++------
northd/northd.h | 18 ++++
tests/ovn-northd.at | 61 ++++++++++++++
7 files changed, 244 insertions(+), 25 deletions(-)
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 63565ef80..8dc31165d 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_lswitches_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)) {
s/lswitchs/lswitches/
+ 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/en-multicast.c b/northd/en-multicast.c
index d51db557e..669e6f73a 100644
--- a/northd/en-multicast.c
+++ b/northd/en-multicast.c
@@ -153,6 +153,10 @@ multicast_igmp_northd_handler(struct engine_node *node,
void *data OVS_UNUSED)
return EN_UNHANDLED;
}
+ if (hmapx_count(&northd_data->trk_data.trk_switches.deleted)) {
+ return EN_UNHANDLED;
+ }
+
/* This node uses the below data from the en_northd engine node.
* - northd_data->lr_datapaths
* - northd_data->ls_ports
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 e4125606a..0fa247576 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -520,23 +520,29 @@ destroy_ports_for_datapath(struct ovn_datapath *od)
hmap_destroy(&od->ports);
}
+static void
+ovn_datapath_remove(struct hmap *datapaths, struct ovn_datapath *od)
+{
+ /* Don't remove od->list. It is used within build_datapaths() as a
+ * private list and once we've exited that function it is not safe to
+ * use it. */
+ hmap_remove(datapaths, &od->key_node);
+ ovn_destroy_tnlids(&od->port_tnlids);
+ destroy_ipam_info(&od->ipam_info);
+ vector_destroy(&od->router_ports);
+ vector_destroy(&od->ls_peers);
+ vector_destroy(&od->localnet_ports);
+ vector_destroy(&od->l3dgw_ports);
+ destroy_mcast_info_for_datapath(od);
+ destroy_ports_for_datapath(od);
+ sset_destroy(&od->router_ips);
+}
> +> static void
ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od)
{
if (od) {
- /* Don't remove od->list. It is used within build_datapaths() as a
- * private list and once we've exited that function it is not safe to
- * use it. */
- hmap_remove(datapaths, &od->key_node);
- ovn_destroy_tnlids(&od->port_tnlids);
- destroy_ipam_info(&od->ipam_info);
- vector_destroy(&od->router_ports);
- vector_destroy(&od->ls_peers);
- vector_destroy(&od->localnet_ports);
- vector_destroy(&od->l3dgw_ports);
- destroy_mcast_info_for_datapath(od);
- destroy_ports_for_datapath(od);
- sset_destroy(&od->router_ips);
+ ovn_datapath_remove(datapaths, od);
free(od);
}
}
@@ -803,6 +809,7 @@ parse_dynamic_routing_redistribute(
return out;
}
+#define OVN_DPS_REALLOC_DELTA 10
static void
ods_build_array_index(struct ovn_datapaths *datapaths)
{
@@ -811,8 +818,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 + OVN_DPS_REALLOC_DELTA;
+ 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 +832,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 += OVN_DPS_REALLOC_DELTA;
+ 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.
@@ -4340,6 +4367,18 @@ 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);
+
+ struct hmapx_node *n;
+ HMAPX_FOR_EACH (n, &trk_dps->deleted) {
+ free(n->data);
+ }
+ hmapx_clear(&trk_dps->deleted);
+}
+
static void
destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports)
{
@@ -4377,6 +4416,7 @@ destroy_northd_data_tracked_changes(struct northd_data
*nd)
{
struct northd_tracked_data *trk_changes = &nd->trk_data;
destroy_tracked_ovn_ports(&trk_changes->trk_lsps);
+ destroy_tracked_dps(&trk_changes->trk_switches);
destroy_tracked_lbs(&trk_changes->trk_lbs);
hmapx_clear(&trk_changes->trk_nat_lrs);
hmapx_clear(&trk_changes->ls_with_changed_lbs);
@@ -4389,6 +4429,8 @@ 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_switches.deleted);
hmapx_init(&trk_data->trk_lsps.created);
hmapx_init(&trk_data->trk_lsps.updated);
hmapx_init(&trk_data->trk_lsps.deleted);
@@ -4404,7 +4446,12 @@ 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);
+ struct hmapx_node *n;
+ HMAPX_FOR_EACH (n, &trk_data->trk_switches.deleted) {
+ free(n->data);
+ }
hmapx_destroy(&trk_data->trk_lsps.updated);
hmapx_destroy(&trk_data->trk_lsps.deleted);
hmapx_destroy(&trk_data->trk_lbs.crupdated);
@@ -4698,13 +4745,15 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
struct ovn_datapath *od,
struct tracked_ovn_ports *trk_lsps)
{
- bool ls_ports_changed = false;
+ bool ls_deleted = nbrec_logical_switch_is_deleted(changed_ls);
+ bool ls_ports_changed = ls_deleted;
if (!nbrec_logical_switch_is_updated(changed_ls,
NBREC_LOGICAL_SWITCH_COL_PORTS)) {
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;
}
@@ -4791,7 +4840,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
od->tunnel_key, old_tunnel_key);
}
}
- op->visited = true;
+ op->visited = !ls_deleted;
}
/* Check for deleted ports */
@@ -4874,18 +4923,49 @@ 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)) {
- 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);
}
- struct hmapx_node *node;
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);
@@ -4912,6 +4992,51 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
}
}
+ HMAPX_FOR_EACH (node, &ni->synced_lses->deleted) {
+ const struct ovn_synced_logical_switch *synced = node->data;
+ const struct nbrec_logical_switch *deleted_ls = synced->nb;
+
+ struct ovn_datapath *od = ovn_datapath_find_(
+ &nd->ls_datapaths.datapaths,
+ &deleted_ls->header_.uuid);
+ if (!od) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+ VLOG_WARN_RL(&rl, "Internal error: a tracked updated LS doesn't "
+ "exist in ls_datapaths: "UUID_FMT,
+ UUID_ARGS(&deleted_ls->header_.uuid));
+ goto fail;
+ }
+
+ if (deleted_ls->copp || deleted_ls->n_dns_records ||
+ deleted_ls->n_forwarding_groups || deleted_ls->n_qos_rules ||
+ deleted_ls->n_load_balancer || deleted_ls->n_load_balancer_group) {
+ goto fail;
+ }
+
+ if (!ls_handle_lsp_changes(ovnsb_idl_txn, deleted_ls,
+ ni, nd, od, &trk_data->trk_lsps)) {
+ goto fail;
+ }
+
+ ovn_datapath_remove(&nd->ls_datapaths.datapaths, od);
ovn_datapath_remove() destroys most of od's data. If you're going to
present od to other engine nodes as a deleted datapath, they likely will
want access to od's data. It's probably better to delay destroying od's
data until the clear_tracked_data() callback. Instead, just call
hmap_remove() here to get od out of nd->ls_datapaths.datapaths.
+ nd->ls_datapaths.array[od->index] = NULL;
This creates a "hole" in the array of logical switches. Let's imagine
you add a new logical switch. Then you delete it. Then you add a new
logical switch. Then you delete it. You keep doing this over and over.
Each time a new logical switch is added, the array of logical switches
has to grow, because there is no logic to fill the hole created by
deleting the previous logical switch.
If you want to support deleting logical datapaths incrementally, you
need some way to keep track of which array indices are being used. A
bitmap (or dynamic bitmap) is a good structure for this purpose. This
way, when a switch is deleted, the bitmap can track that this particular
array index is no longer being used. Then when a new switch is created,
you can use the bitmap to find the lowest unused array index and insert
the switch there.
+ const struct sbrec_ip_multicast *ip_mcast =
+ ip_mcast_lookup(ni->sbrec_ip_mcast_by_dp, od->sb);
+ if (ip_mcast) {
+ sbrec_ip_multicast_delete(ip_mcast);
+ }
+
+ if (is_ls_acls_changed(deleted_ls)) {
+ hmapx_add(&trk_data->ls_with_changed_acls, od);
+ }
+ hmapx_add(&trk_data->trk_switches.deleted, od);
+ }
+
+ if (!hmapx_is_empty(&trk_data->trk_switches.crupdated) ||
+ !hmapx_is_empty(&trk_data->trk_switches.deleted)) {
+ 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..478756214 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;
};
static inline size_t
@@ -110,6 +112,13 @@ enum redirected_routing_protcol_flag_type {
REDIRECT_BFD = (1 << 1),
};
+struct tracked_dps {
+ /* Tracked created or updated datapaths. */
+ struct hmapx crupdated;
+ /* Tracked deleted datapaths. */
+ struct hmapx deleted;
+};
+
struct tracked_ovn_ports {
/* tracked created ports.
* hmapx node data is 'struct ovn_port *' */
@@ -141,6 +150,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 +159,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 +964,13 @@ 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_lswitches_in_tracked_data(
+ struct northd_tracked_data *trk_nd_changes)
+{
+ 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..22d11095a 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -14656,6 +14656,67 @@ 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
+
+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
+
+# Create a logical port
+check ovn-nbctl --wait=sb lsp-add sw0 lsp0
+
+# 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 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
+
+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
