> On Thu, Aug 28, 2025 at 11:17 AM Lorenzo Bianconi via dev < > [email protected]> wrote: >
Hi Jacob, thx for the review. > > This patch handles the logical switch creation and deletion 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. > > > > Acked-by: Mairtin O'Loingsigh <[email protected]> > > Reported-at: https://issues.redhat.com/browse/FDP-754 > > Co-authored-by: Numan Siddique <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > 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 | 158 +++++++++++++++++++++++++++++++++++---- > > northd/northd.h | 21 +++++- > > tests/ovn-northd.at | 61 +++++++++++++++ > > 7 files changed, 240 insertions(+), 17 deletions(-) > > [...] > > /* 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. > > @@ -3999,6 +4022,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); > > > this is more of a nit if we call hmapx_delete() on every entry then we > don't have to call hmapx_clear() ack, I will fix it in v8. > > > > > +} > > + > > static void > > destroy_tracked_ovn_ports(struct tracked_ovn_ports *trk_ovn_ports) > > { > > @@ -4041,6 +4077,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; > > } > > > > @@ -4049,6 +4086,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); > > @@ -4065,7 +4104,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); > > + } > > > > Why is there a difference when clearing the trk_switches here and > in destroy_northd_data_tracked_changes? Now that I look at them, is there > really a need for both of them? Why is it not required to use > HMAP_FOR_EACH_SAFE and ovn_datapath_destroy? From some of the testing I ack, right. We need to run ovn_datapath_destroy() here too. I will fix it in v8. > have been doing it seems like this is called after > destroy_northd_data_tracked_changes anyway. we need to keep destroy_northd_data_tracked_changes() since it is run in en_northd_clear_tracked_data() (e.g. in engine_init_run()). Agree? > > There is a memory leak here if you don't also call hmapx_destroy() on > &trk_data->trk_switches.deleted right, we need to call hmapx_destroy() on trk_switches.deleted here. > > hmapx_destroy(&trk_data->trk_lsps.updated); > > hmapx_destroy(&trk_data->trk_lsps.deleted); > > hmapx_destroy(&trk_data->trk_lbs.crupdated); > > @@ -4361,13 +4405,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, [...] > > > > + 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 deleted 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); > > > > This only works because our testing uses one switch and deletes one at a > time. There is an issue further in database processing when > ovn_datapaths_find_by_index() is called because of ovs_assert(od_index <= > hmap_count(&ovn_datapaths->datapaths));. The scenario being you have sw0, > sw1, sw2, sw3 all with the corresponding index if you delete more than one > in a transaction "ovn-nbctl ls-del sw0 -- ls-del sw1" when rebuilding the > lflows and calling ovn_datapaths_find_by_index() since we don't update the > index of each datapath then hmap_count will be 2 but the index of sw3 will > still be 3. ack, right. Now the index vector is 'sparse'. Since we actually never remove items in ovn_datapaths->dps at runtime (we just mark elements as 'unused') we need to substitute: ovs_assert(od_index <= hmap_count(&ovn_datapaths->datapaths)); with ovs_assert(od_index <= vector_len(&ovn_datapaths->dps)); Agree? Regards, Lorenzo > > > > + vector_get(&nd->ls_datapaths.dps, od->index, > > + struct ovn_datapath *) = NULL; > > + dynamic_bitmap_set0(&nd->ls_datapaths.dps_index_map, od->index); > > + > > + 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)) { > > @@ -18958,8 +19088,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 bb12ec781..1fbc7a802 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 "simap.h" > > #include "ovs-thread.h" > > @@ -100,7 +101,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 * > > @@ -115,6 +116,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 *' */ > > @@ -146,6 +154,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. > > @@ -154,6 +163,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; > > > > @@ -376,7 +386,7 @@ struct ovn_datapath { > > struct uuid key; /* (nbs/nbr)->header_.uuid. */ > > > > size_t index; /* A unique index across all datapaths. > > - * Datapath indexes are sequential and start from > > zero. */ > > + * Datapath indexes start from zero. */ > > > > struct ovn_datapaths *datapaths; /* The collection of datapaths that > > contains this datapath. */ > > @@ -997,6 +1007,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 b2b9f092c..1b5b92ab9 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -14561,6 +14561,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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
