On Wed, Mar 11, 2026 at 10:33 AM Lorenzo Bianconi < [email protected]> wrote:
> Introduce logical-flows incremental processing support for > logical_switch creation/deletion. > > Reported-at: https://issues.redhat.com/browse/FDP-2025 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Hi Lorenzo, thank you for the v5, I'm afraid there is a similar problem we had with the DP groups and LR I-P, see down below. > northd/en-lflow.c | 10 +++-- > northd/en-multicast.c | 3 +- > northd/lflow-mgr.c | 4 ++ > northd/northd.c | 97 +++++++++++++++++++++++++++++++++++-------- > northd/northd.h | 7 +++- > tests/ovn-northd.at | 7 ++-- > 6 files changed, 101 insertions(+), 27 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index d4351edb9..7432d5278 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -149,9 +149,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; > - } > nit: Extra empty space. > > const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > @@ -166,6 +163,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, > diff --git a/northd/en-multicast.c b/northd/en-multicast.c > index a7dfd71c4..97b12f6a3 100644 > --- a/northd/en-multicast.c > +++ b/northd/en-multicast.c > @@ -153,7 +153,8 @@ 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)) { > + if (hmapx_count(&northd_data->trk_data.trk_switches.crupdated) || > + hmapx_count(&northd_data->trk_data.trk_switches.deleted)) { > return EN_UNHANDLED; > } > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index b236c13c6..52320eb78 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -780,6 +780,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); > diff --git a/northd/northd.c b/northd/northd.c > index 0da15629e..a5315e26b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5954,10 +5954,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 > @@ -19237,29 +19240,36 @@ 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); > Is this intentionally left with NULL? > } > @@ -20036,6 +20046,57 @@ 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; > + > + 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->dps, > + 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, > + .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; > + lflow_ref_unlink_lflows(od->datapath_lflows); > + build_lswitch_and_lrouter_iterate_by_ls(od, &lsi); > + } > We should put a comment explaining why we do it in two passes over the hmapx. > + > + ds_destroy(&lsi.actions); > + ds_destroy(&lsi.match); > + > + HMAPX_FOR_EACH (hmapx_node, &tracked_ls->crupdated) { > + od = hmapx_node->data; > + if (!lflow_ref_sync_lflows( > + od->datapath_lflows, lflows, ovnsb_txn, > lflow_input->dps, > + lflow_input->ovn_internal_version_changed, > + lflow_input->sbrec_logical_flow_table, > + lflow_input->sbrec_logical_dp_group_table)) { > + return false; > + } > + } > + > + return true; > +} > + > bool > lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > struct tracked_ovn_ports *trk_lsps, > diff --git a/northd/northd.h b/northd/northd.h > index 41190110b..6b4c575fa 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -474,8 +474,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; > }; > > @@ -949,6 +948,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 *, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index fd049d096..bb0f0eaa4 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -3148,11 +3148,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 .) Well this isn't right. This means that the dp group was recreated during I-P. Actually checking the DB logs it seems to be the case, we remove the old DP group, create new one and update a lot of lflow references to DP group. This is very problematic it should be solved before proceeding. > 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) > @@ -14958,7 +14959,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. > @@ -14996,7 +14997,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.53.0 > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
