Hi Lorenzo, thanks for the patch. I have a couple of comments below.
On Mon, Nov 17, 2025 at 6:56 PM Lorenzo Bianconi via dev <[email protected]> wrote: > > Introduce logical-flows incremental processing support for > logical_switch creation/deletion. > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > northd/en-lflow.c | 13 ++-- > northd/en-ls-stateful.c | 6 +- > northd/lflow-mgr.c | 8 ++- > northd/northd.c | 141 ++++++++++++++++++++++++++++++++-------- > northd/northd.h | 8 ++- > tests/ovn-northd.at | 16 ++++- > 6 files changed, 152 insertions(+), 40 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 30bf7e35c..14a211bbb 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -145,10 +145,6 @@ 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; > > @@ -162,6 +158,13 @@ lflow_northd_handler(struct engine_node *node, > return EN_UNHANDLED; > } > > + if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn, > + &northd_data->trk_data.trk_switches, > + &lflow_input, > + lflow_data->lflow_table)) { > + return EN_UNHANDLED; > + } > + > if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, > &northd_data->trk_data.trk_lsps, > &lflow_input, > @@ -214,6 +217,7 @@ lflow_ls_stateful_handler(struct engine_node *node, void > *data) > return EN_UNHANDLED; > } > > + struct northd_data *northd_data = engine_get_input_data("northd", node); > const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > @@ -221,6 +225,7 @@ lflow_ls_stateful_handler(struct engine_node *node, void > *data) > lflow_get_input_data(node, &lflow_input); > if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, > &ls_sful_data->trk_data, > + > &northd_data->trk_data.trk_switches, > &lflow_input, > lflow_data->lflow_table)) { > return EN_UNHANDLED; > diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c > index a9a685504..0dea24aee 100644 > --- a/northd/en-ls-stateful.c > +++ b/northd/en-ls-stateful.c > @@ -161,8 +161,10 @@ ls_stateful_northd_handler(struct engine_node *node, > void *data_) > const struct ovn_datapath *od = hmapx_node->data; > > if (!ls_stateful_table_find_(&data->table, od->nbs)) { > - ls_stateful_record_create(&data->table, od, > - input_data.ls_port_groups); > + struct ls_stateful_record *ls_stateful_rec = > + ls_stateful_record_create(&data->table, od, > + input_data.ls_port_groups); > + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > } > } > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index d1fbc2196..f2eb2851c 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -745,6 +745,10 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash); > } > > + if (lrn->dpgrp_lflow) { > + dynamic_bitmap_realloc(&lrn->dpgrp_bitmap, dp_bitmap_len); > + } > + > if (!lrn->linked) { > if (lrn->dpgrp_lflow) { > ovs_assert(lrn->dpgrp_bitmap.capacity == dp_bitmap_len); > @@ -1277,7 +1281,9 @@ ovn_sb_insert_or_update_logical_dp_group( > BITMAP_FOR_EACH_1 (index, ods_size(datapaths), dpg_bitmap) { > struct ovn_datapath *od = vector_get(&datapaths->dps, index, > struct ovn_datapath *); > - sb[n++] = od->sdp->sb_dp; > + if (od) { Just to double-check, has this if statement been added because datapaths->dps can now have holes in it where ovn_datapaths do not exist? Is this also possible with logical routers? And if so, do we need to put out another new release of 25.09 to deal with this? > + sb[n++] = od->sdp->sb_dp; > + } > } > if (!dp_group) { > struct uuid dpg_uuid = uuid_random(); > diff --git a/northd/northd.c b/northd/northd.c > index cdf12ec86..c8f526660 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5729,10 +5729,13 @@ enum mirror_filter { > > static void > build_mirror_default_lflow(struct ovn_datapath *od, > - struct lflow_table *lflows) > + struct lflow_table *lflows, > + struct lflow_ref *lflow_ref) > { > - ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", NULL); > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", NULL); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", > + lflow_ref); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", > + lflow_ref); > } > > static void > @@ -18564,6 +18567,7 @@ struct lswitch_flow_build_info { > struct hmap *route_policies; > struct simap *route_tables; > const struct sbrec_acl_id_table *sbrec_acl_id_table; > + struct lflow_ref *igmp_lflow_ref; > }; > > /* Helper function to combine all lflow generation which is iterated by > @@ -18577,31 +18581,39 @@ build_lswitch_and_lrouter_iterate_by_ls(struct > ovn_datapath *od, > struct lswitch_flow_build_info *lsi) > { > ovs_assert(od->nbs); > - build_mirror_default_lflow(od, lsi->lflows); > + build_mirror_default_lflow(od, lsi->lflows, od->datapath_lflows); > build_lswitch_lflows_pre_acl_and_acl(od, lsi->lflows, > - lsi->meter_groups, NULL); > - build_network_function(od, lsi->lflows, lsi->ls_port_groups, NULL); > - build_fwd_group_lflows(od, lsi->lflows, NULL); > - build_lswitch_lflows_admission_control(od, lsi->lflows, NULL); > - build_lswitch_learn_fdb_od(od, lsi->lflows, NULL); > - build_lswitch_arp_nd_responder_default(od, lsi->lflows, NULL); > + lsi->meter_groups, > + od->datapath_lflows); > + build_network_function(od, lsi->lflows, lsi->ls_port_groups, > + od->datapath_lflows); > + build_fwd_group_lflows(od, lsi->lflows, od->datapath_lflows); > + build_lswitch_lflows_admission_control(od, lsi->lflows, > + od->datapath_lflows); > + build_lswitch_learn_fdb_od(od, lsi->lflows, od->datapath_lflows); > + build_lswitch_arp_nd_responder_default(od, lsi->lflows, > + od->datapath_lflows); > build_lswitch_dns_lookup_and_response(od, lsi->lflows, lsi->meter_groups, > - NULL); > - build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, NULL); > + od->datapath_lflows); > + build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, > od->datapath_lflows); > build_lswitch_destination_lookup_bmcast(od, lsi->lflows, &lsi->actions, > - lsi->meter_groups, NULL); > - build_lswitch_output_port_sec_od(od, lsi->lflows, NULL); > + lsi->meter_groups, > + od->datapath_lflows); > + build_lswitch_output_port_sec_od(od, lsi->lflows, od->datapath_lflows); > /* CT extraction flows are built with stateful flows, but default rule is > * always needed */ > ovn_lflow_add(lsi->lflows, od, S_SWITCH_IN_CT_EXTRACT, 0, "1", "next;", > - NULL); > - build_lswitch_lb_affinity_default_flows(od, lsi->lflows, NULL); > + od->datapath_lflows); > + build_lswitch_lb_affinity_default_flows(od, lsi->lflows, > + od->datapath_lflows); > if (od->has_evpn_vni) { > - build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, NULL); > + build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, > + od->datapath_lflows); > } else { > - build_lswitch_lflows_l2_unknown(od, lsi->lflows, NULL); > + build_lswitch_lflows_l2_unknown(od, lsi->lflows, > od->datapath_lflows); > } > - build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, NULL); > + build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, > + lsi->igmp_lflow_ref); > } > > /* Helper function to combine all lflow generation which is iterated by > @@ -18954,7 +18966,8 @@ build_lswitch_and_lrouter_flows( > const struct group_ecmp_route_data *route_data, > struct hmap *route_policies, > struct simap *route_tables, > - const struct sbrec_acl_id_table *sbrec_acl_id_table) > + const struct sbrec_acl_id_table *sbrec_acl_id_table, > + struct lflow_ref *igmp_lflow_ref) > { > > char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac); > @@ -18995,6 +19008,7 @@ build_lswitch_and_lrouter_flows( > lsiv[index].route_tables = route_tables; > lsiv[index].route_policies = route_policies; > lsiv[index].sbrec_acl_id_table = sbrec_acl_id_table; > + lsiv[index].igmp_lflow_ref = igmp_lflow_ref; > ds_init(&lsiv[index].match); > ds_init(&lsiv[index].actions); > > @@ -19045,6 +19059,7 @@ build_lswitch_and_lrouter_flows( > .match = DS_EMPTY_INITIALIZER, > .actions = DS_EMPTY_INITIALIZER, > .sbrec_acl_id_table = sbrec_acl_id_table, > + .igmp_lflow_ref = igmp_lflow_ref, > }; > > /* Combined build - all lflow generation from lswitch and lrouter > @@ -19216,7 +19231,8 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > input_data->route_data, > input_data->route_policies, > input_data->route_tables, > - input_data->sbrec_acl_id_table); > + input_data->sbrec_acl_id_table, > + input_data->igmp_lflow_ref); > build_igmp_lflows(input_data->igmp_groups, > &input_data->ls_datapaths->datapaths, > lflows, input_data->igmp_lflow_ref); > @@ -19336,6 +19352,59 @@ lflow_handle_northd_lr_changes(struct ovsdb_idl_txn > *ovnsb_txn, > return handled; > } > > +bool > +lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > + struct tracked_dps *tracked_ls, > + struct lflow_input *lflow_input, > + struct lflow_table *lflows) > +{ > + struct hmapx_node *hmapx_node; > + struct ovn_datapath *od; > + bool handled = true; > + > + HMAPX_FOR_EACH (hmapx_node, &tracked_ls->deleted) { > + od = hmapx_node->data; > + if (!lflow_ref_resync_flows( > + od->datapath_lflows, lflows, ovnsb_txn, > + lflow_input->ls_datapaths, > + lflow_input->lr_datapaths, > + lflow_input->ovn_internal_version_changed, > + lflow_input->sbrec_logical_flow_table, > + lflow_input->sbrec_logical_dp_group_table)) { > + return false; > + } > + } > + > + struct lswitch_flow_build_info lsi = { > + .meter_groups = lflow_input->meter_groups, > + .igmp_lflow_ref = lflow_input->igmp_lflow_ref, > + .ls_port_groups = lflow_input->ls_port_groups, > + .lflows = lflows, > + .match = DS_EMPTY_INITIALIZER, > + .actions = DS_EMPTY_INITIALIZER, > + }; > + > + HMAPX_FOR_EACH (hmapx_node, &tracked_ls->crupdated) { > + od = hmapx_node->data; > + build_lswitch_and_lrouter_iterate_by_ls(od, &lsi); > + handled = lflow_ref_sync_lflows( > + od->datapath_lflows, lflows, ovnsb_txn, > + lflow_input->ls_datapaths, > + lflow_input->lr_datapaths, > + lflow_input->ovn_internal_version_changed, > + lflow_input->sbrec_logical_flow_table, > + lflow_input->sbrec_logical_dp_group_table); > + if (!handled) { > + break; > + } > + } > + > + ds_destroy(&lsi.actions); > + ds_destroy(&lsi.match); > + > + return handled; > +} > + > bool > lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *trk_lsps, > @@ -19653,6 +19722,7 @@ exit: > bool > lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct ls_stateful_tracked_data *trk_data, > + struct tracked_dps *tracked_ls, > struct lflow_input *lflow_input, > struct lflow_table *lflows) > { > @@ -19695,13 +19765,28 @@ lflow_handle_ls_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { > struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; > - const struct ovn_datapath *od = > - ovn_datapaths_find_by_index(lflow_input->ls_datapaths, > - ls_stateful_rec->ls_index); > - ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, > - &ls_stateful_rec->nbs_uuid)); > - > - lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); > + const struct ovn_datapath *od = NULL; > + struct hmapx_node *n; > + HMAPX_FOR_EACH (n, &tracked_ls->deleted) { > + const struct ovn_datapath *iter = n->data; > + if (iter->index == ls_stateful_rec->ls_index) { > + od = iter; > + break; > + } > + } > + if (od) { > + ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, > + &ls_stateful_rec->nbs_uuid)); > + if (!lflow_ref_resync_flows( > + ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, > + lflow_input->ls_datapaths, > + lflow_input->lr_datapaths, > + lflow_input->ovn_internal_version_changed, > + lflow_input->sbrec_logical_flow_table, > + lflow_input->sbrec_logical_dp_group_table)) { > + return false; > + } > + } > } > > return true; > diff --git a/northd/northd.h b/northd/northd.h > index 32134d36e..2af238eea 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -473,8 +473,7 @@ struct ovn_datapath { > /* Map of ovn_port objects belonging to this datapath. > * This map doesn't include derived ports. */ > struct hmap ports; > - /* Reference to the lflows belonging to this datapath currently router > - * only lflows. */ > + /* Reference to the lflows belonging to this datapath. */ > struct lflow_ref *datapath_lflows; > }; > > @@ -941,6 +940,10 @@ bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn > *ovnsh_txn, > struct tracked_dps *, > struct lflow_input *, > struct lflow_table *lflows); > +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, > + struct tracked_dps *, > + struct lflow_input *, > + struct lflow_table *lflows); > bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *, > struct lflow_input *, > @@ -955,6 +958,7 @@ bool lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *, > struct lflow_table *lflows); > bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, > struct ls_stateful_tracked_data *, > + struct tracked_dps *, > struct lflow_input *, > struct lflow_table *lflows); > bool northd_handle_sb_port_binding_changes( > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index a88b71ca8..f40ac11ab 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2935,6 +2935,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check VXLAN mode disabling]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 Why are these FIXME lines here? > + > # Create a fake chassis with vxlan encap to implicitly enable VXLAN mode. > check_uuid ovn-sbctl \ > --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \ > @@ -2969,6 +2972,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check datapath tunnel ids exhaustion]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 > + > # Create a fake chassis with vxlan encap to lower MAX DP tunnel key to 2^12 > check_uuid ovn-sbctl \ > --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \ > @@ -3030,6 +3036,9 @@ OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([check LS/LR single line configuration]) > ovn_start > > +# FIXME > +OVS_CTL_TIMEOUT=120 > + > cmd="" > for i in {1..2048..1}; do > cmd="${cmd} -- ls-add lsw-${i}" > @@ -3119,11 +3128,12 @@ check_row_count sb:logical_dp_group 1 > echo "$sw2_sb_uuid" >> sw_sb_uuids > > AT_CHECK_UNQUOTED([ovn-sbctl --bare --columns _uuid,datapaths find > Logical_DP_Group dnl > - | grep -A1 $ls_dpg_uuid | tail -1 | tr ' ' '\n' | sort], > [0], [dnl > + | tail -1 | tr ' ' '\n' | sort], [0], [dnl > $(cat sw_sb_uuids | sort) > ]) > > dnl Add two routers and check that they are in their new separate group. > +ls_dpg_uuid=$(ovn-sbctl --bare --columns _uuid list Logical_DP_Group .) > check ovn-nbctl --wait=sb lr-add lr0 -- lr-add lr1 > lr0_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr0) > lr1_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=lr1) > @@ -14558,7 +14568,7 @@ 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 > +check_engine_stats lflow norecompute compute > > # For the below engine nodes, en_northd is input. So check > # their engine status. > @@ -14596,7 +14606,7 @@ 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 > +check_engine_stats lflow norecompute compute > > # For the below engine nodes, en_northd is input. So check > # their engine status. > -- > 2.51.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
