On Wed, Aug 20, 2025 at 06:55:25PM +0200, 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/en-multicast.c | 4 + > northd/inc-proc-northd.c | 5 +- > northd/northd.c | 163 +++++++++++++++++++++++++++++++++++---- > northd/northd.h | 19 ++++- > tests/ovn-northd.at | 61 +++++++++++++++ > 7 files changed, 243 insertions(+), 17 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index f903f5e3a..50570b611 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -142,6 +142,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 895170089..1711d38e7 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_lswitches_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/en-multicast.c b/northd/en-multicast.c > index 6613bdaa9..54a40c1a1 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 bfb893a1a..016f91273 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -254,7 +254,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); > @@ -284,6 +283,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 7cbea7d57..ace5c2301 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -554,13 +554,12 @@ destroy_ports_for_datapath(struct ovn_datapath *od) > } > > static void > -ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) > +ovn_datapath_destroy(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); > @@ -837,13 +836,19 @@ parse_dynamic_routing_redistribute( > } > > static void > -ods_build_array_index(struct ovn_datapaths *datapaths) > +ods_build_dps_vector(struct ovn_datapaths *datapaths) > { > + size_t size = hmap_count(&datapaths->datapaths); > datapaths->dps = VECTOR_CAPACITY_INITIALIZER(struct ovn_datapath *, > - ods_size(datapaths)); > - datapaths->dps_index_map.map = bitmap_allocate(ods_size(datapaths)); > - datapaths->dps_index_map.capacity = ods_size(datapaths); > + size); > + datapaths->dps_index_map.map = bitmap_allocate(size); > + datapaths->dps_index_map.capacity = size; > +} > > +static void > +ods_build_array_index(struct ovn_datapaths *datapaths) > +{ > + ods_build_dps_vector(datapaths); > /* Assign unique sequential indexes to all datapaths. These are not > * visible outside of the northd loop, so, unlike the tunnel keys, it > * doesn't matter if they are different on every iteration. */ > @@ -859,6 +864,26 @@ ods_build_array_index(struct ovn_datapaths *datapaths) > } > } > > +static void > +ods_assign_array_index(struct ovn_datapaths *datapaths, > + struct ovn_datapath *od) > +{ > + dynamic_bitmap_realloc(&datapaths->dps_index_map, > + vector_len(&datapaths->dps) + 1); > + size_t index = bitmap_scan(datapaths->dps_index_map.map, 0, 0, > + datapaths->dps_index_map.capacity); > + if (index < vector_len(&datapaths->dps)) { > + /* We can reuse stale vector entries. */ > + vector_get(&datapaths->dps, index, struct ovn_datapath *) = od; > + } else { > + vector_insert(&datapaths->dps, index, &od); > + } > + bitmap_set1(datapaths->dps_index_map.map, index); > + datapaths->dps_index_map.n_elems++; > + od->datapaths = datapaths; > + od->index = index; > +} > + > /* 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. > @@ -3995,6 +4020,19 @@ 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_SAFE (n, &trk_dps->deleted) { > + ovn_datapath_destroy(n->data); > + hmapx_delete(&trk_dps->deleted, n); > + } > + hmapx_clear(&trk_dps->deleted); > +} > + > static void > destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports) > { > @@ -4037,6 +4075,7 @@ destroy_northd_data_tracked_changes(struct northd_data > *nd) > hmapx_clear(&trk_changes->ls_with_changed_lbs); > hmapx_clear(&trk_changes->ls_with_changed_acls); > hmapx_clear(&trk_changes->ls_with_changed_ipam); > + destroy_tracked_dps(&trk_changes->trk_switches); > trk_changes->type = NORTHD_TRACKED_NONE; > } > > @@ -4045,6 +4084,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); > @@ -4061,7 +4102,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); > @@ -4357,13 +4403,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; > } > @@ -4455,7 +4503,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > } else if (!strcmp(op->nbsp->type, "virtual")) { > ovs_list_push_back(&existing_virtual_ports, &op->list); > } > - op->visited = true; > + op->visited = !ls_deleted; > } > > /* Check for deleted ports */ > @@ -4577,19 +4625,55 @@ 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->new) && > + hmapx_is_empty(&ni->synced_lses->deleted) && > hmapx_is_empty(&ni->synced_lses->updated)) { > 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->sdp); > + > + 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); > @@ -4623,6 +4707,55 @@ 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; > + } > + > + hmap_remove(&nd->ls_datapaths.datapaths, &od->key_node); > + vector_get(&nd->ls_datapaths.dps, od->index, > + struct ovn_datapath *) = NULL; > + bitmap_set0(nd->ls_datapaths.dps_index_map.map, od->index); > + nd->ls_datapaths.dps_index_map.n_elems--; > + > + const struct sbrec_ip_multicast *ip_mcast = > + ip_mcast_lookup(ni->sbrec_ip_mcast_by_dp, od->sdp->sb_dp); > + 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)) { > @@ -18949,8 +19082,8 @@ static void > ovn_datapaths_destroy(struct ovn_datapaths *datapaths) > { > struct ovn_datapath *dp; > - HMAP_FOR_EACH_SAFE (dp, key_node, &datapaths->datapaths) { > - ovn_datapath_destroy(&datapaths->datapaths, dp); > + HMAP_FOR_EACH_POP (dp, key_node, &datapaths->datapaths) { > + ovn_datapath_destroy(dp); > } > hmap_destroy(&datapaths->datapaths); > > diff --git a/northd/northd.h b/northd/northd.h > index b71b13ca9..aaa2b0dab 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -22,6 +22,7 @@ > #include "lib/sset.h" > #include "northd/en-port-group.h" > #include "northd/ipam.h" > +#include "northd/lb.h" > #include "openvswitch/hmap.h" > #include "northd/lb.h" > #include "simap.h" > @@ -101,7 +102,7 @@ struct ovn_datapaths { > static inline size_t > ods_size(const struct ovn_datapaths *datapaths) > { > - return hmap_count(&datapaths->datapaths); > + return vector_len(&datapaths->dps); > } > > struct ovn_datapath * > @@ -116,6 +117,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 *' */ > @@ -147,6 +155,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. > @@ -155,6 +164,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; > > @@ -998,6 +1008,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 11bbb211d..9e0e80418 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -14544,6 +14544,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 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 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 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 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 > > -- > 2.50.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > One small nit, otherwise patch looks good to me.
Acked-by: Mairtin O'Loingsigh <moloi...@redhat.com> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
