On Wed, Aug 27, 2025 at 08:30:35AM +0100, Mairtin O'Loingsigh via dev wrote: > 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, nit: Change updated to deleted > > + 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 > Sorry, I did not include nit in previous mail
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
