> Hi Lorenzo, thanks for the patch. > > I have a couple of comments below.
Hi Mark, thx for the review. > > 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 yes, I think so > need to put out another new release of 25.09 to deal with this? I think this is technically possible, but I have not been able to trigger the problem with the current CI for logical router. Do you want me to split the patch and to post a dedicated fix for it? > > > + 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? it is because this is due to another problem that I spotted but it is not related to this patch (in fact even lflow IP for logical router has the same issue). It is just to remind to remove it as soon as the other problem is fixes. Do you want to me to add a comment for it? Regards, Lorenzo > > > + > > # 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
