> 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.
I am fine with it but since this array has not been introduced by this series I
guess we can do it with a follow-up patch. What do you think?
>
> > + 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.
ditto.
>
> > };
> > 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/
ack, I will fix it.
>
> > +{
> > + 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.
ack, I will fix it.
Regards,
Lorenzo
>
> > +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
