On 6/19/26 10:43 AM, Ales Musil via dev wrote:
> The multicast groups were updated as part of the lflow handler
> which is wrong. There is multicast group node which takes
> care of the group updates. Also, the updates in the lflow handler
> were partially wrong and not in line with the multicast group
> node.
>
> At the same time remove the lsp_can_receive_multicast() check that
> would prevent the lflow creation for new LSPs.
>
> Reported-at: https://redhat.atlassian.net/browse/FDP-4022
> Assisted-by: Claude Opus 4.6, Claude Code
> Fixes: b741cb7e299c ("northd: Incremental processing of VIF updates and
> deletions in 'lflow' node.")
> Fixes: 8e2d6fa14804 ("northd, tests: Network Function insertion logical flow
> programming.")
> Signed-off-by: Ales Musil <[email protected]>
> ---
Hi Ales,
Thanks for the patch!
> northd/en-multicast.c | 7 +++-
> northd/northd.c | 41 ------------------
> tests/ovn-northd.at | 96 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+), 43 deletions(-)
>
> diff --git a/northd/en-multicast.c b/northd/en-multicast.c
> index b2775516a..5148d8840 100644
> --- a/northd/en-multicast.c
> +++ b/northd/en-multicast.c
> @@ -153,7 +153,10 @@ 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)) {
> + struct tracked_ovn_ports *trk_lsps = &northd_data->trk_data.trk_lsps;
> + if (hmapx_count(&trk_lsps->created) ||
> + hmapx_count(&trk_lsps->updated) ||
> + hmapx_count(&trk_lsps->deleted)) {
> return EN_UNHANDLED;
This will cause the en_multicast_igmp node to recompute every time we
add/update/delete a logical switch port. Which will trigger the
lflow_multicast_igmp_handler incremental handler for the en_lflow node.
While that handler's work in theory isn't too heavy weight, it does
recompute all multicast related lflows for all switch ports in the NB.
Just to be sure, I'd like to hold on with accepting this change until we
manage to test it downstream in some of the scale testing setups we have
right now. I cc-ed Patryk in case he has some time to help with that.
Thanks,
Dumitru
> }
>
> @@ -171,7 +174,7 @@ multicast_igmp_northd_handler(struct engine_node *node,
> void *data OVS_UNUSED)
> * This node also accesses the router ports of the logical router
> * (od->ports). When these logical router ports gets updated,
> * en_northd engine recomputes and so does this node.
> - * Note: When we add I-P to handle switch/router port changes, we
> + * Note: When we add I-P to handle router port changes, we
> * need to revisit this handler.
> *
> * */
> diff --git a/northd/northd.c b/northd/northd.c
> index 30fdd9d39..aa332c449 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -20464,9 +20464,6 @@ lflow_handle_northd_port_changes(struct ovsdb_idl_txn
> *ovnsb_txn,
> if (!handled) {
> return false;
> }
> -
> - /* SB port_binding is not deleted, so don't update SB multicast
> - * groups. */
> }
>
> HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) {
> @@ -20474,20 +20471,6 @@ lflow_handle_northd_port_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> /* Make sure 'op' is an lsp and not lrp. */
> ovs_assert(op->nbsp);
>
> - if (!lsp_can_receive_multicast(op->nbsp)) {
> - continue;
> - }
> -
> - const struct sbrec_multicast_group *sbmc_flood =
> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> - MC_FLOOD, op->od->sdp->sb_dp);
> - const struct sbrec_multicast_group *sbmc_flood_l2 =
> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> - MC_FLOOD_L2, op->od->sdp->sb_dp);
> - const struct sbrec_multicast_group *sbmc_unknown =
> - mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> - MC_UNKNOWN, op->od->sdp->sb_dp);
> -
> struct ds match = DS_EMPTY_INITIALIZER;
> struct ds actions = DS_EMPTY_INITIALIZER;
> build_lswitch_and_lrouter_iterate_by_lsp(op, lflow_input->ls_ports,
> @@ -20522,30 +20505,6 @@ lflow_handle_northd_port_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> if (!handled) {
> return false;
> }
> -
> - /* Update SB multicast groups for the new port. */
> - if (!sbmc_flood) {
> - sbmc_flood = create_sb_multicast_group(ovnsb_txn,
> - op->od->sdp->sb_dp, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY);
> - }
> - sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb);
> -
> - if (!sbmc_flood_l2) {
> - sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
> - op->od->sdp->sb_dp, MC_FLOOD_L2,
> - OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
> - }
> - sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2, op->sb);
> -
> - if (op->has_unknown) {
> - if (!sbmc_unknown) {
> - sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
> - op->od->sdp->sb_dp, MC_UNKNOWN,
> - OVN_MCAST_UNKNOWN_TUNNEL_KEY);
> - }
> - sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> - op->sb);
> - }
> }
>
> return true;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 4ef2c8946..7d008a8fc 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12,6 +12,7 @@ m4_define([_DUMP_DB_TABLES], [
> ovn-sbctl list meter >> $1
> ovn-sbctl list meter_band >> $1
> ovn-sbctl list port_group >> $1
> + ovn-sbctl list multicast_group >> $1
> ovn-sbctl list bfd >> $1
> ovn-sbctl dump-flows > lflows_$1
> ])
> @@ -15854,6 +15855,101 @@ OVN_CLEANUP([hv1], [hv2])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Multicast group incremental processing])
> +ovn_start
> +
> +dnl Helper: verify that Multicast_Group 'mc_name' on datapath 'dp_name'
> +dnl contains exactly the listed ports.
> +dnl Usage: check_mc_group_ports MC_NAME DP_NAME PORT1 [PORT2 ...]
> +check_mc_group_ports() {
> + mc_group_name=$1; shift
> + mc_group_dp=$(fetch_column Datapath_Binding _uuid external_ids:name=$1);
> shift
> + mc_group_expected=""
> + for port_name; do
> + mc_group_expected="$mc_group_expected $(fetch_column Port_Binding
> _uuid logical_port=$port_name)"
> + done
> + mc_group_expected=$(echo $mc_group_expected | tr ' ' '\n' | sort | xargs)
> +
> + OVS_WAIT_UNTIL([
> + mc_group_found=$(fetch_column Multicast_Group ports
> name=$mc_group_name datapath=$mc_group_dp)
> + test "$mc_group_found" = "$mc_group_expected"
> + ])
> +}
> +
> +check ovn-nbctl --wait=sb ls-add ls0
> +check ovn-nbctl --wait=sb lsp-add-localnet-port ls0 ln0 physnet0
> +
> +dnl Baseline: localnet port only.
> +check_mc_group_ports _MC_flood ls0 ln0
> +check_mc_group_ports _MC_flood_l2 ls0 ln0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Add an enabled VIF.
> +check ovn-nbctl --wait=sb lsp-add ls0 lsp0
> +check_mc_group_ports _MC_flood ls0 ln0 lsp0
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Add a disabled VIF; it must NOT appear in multicast groups.
> +check ovn-nbctl --wait=sb \
> + lsp-add ls0 lsp1 \
> + -- set logical_switch_port lsp1 enabled=false
> +check_mc_group_ports _MC_flood ls0 ln0 lsp0
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp0
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Enable the disabled VIF.
> +check ovn-nbctl --wait=sb set logical_switch_port lsp1 enabled=true
> +check_mc_group_ports _MC_flood ls0 ln0 lsp0 lsp1
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp0 lsp1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Disable an enabled VIF.
> +check ovn-nbctl --wait=sb set logical_switch_port lsp0 enabled=false
> +check_mc_group_ports _MC_flood ls0 ln0 lsp1
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Re-enable it.
> +check ovn-nbctl --wait=sb set logical_switch_port lsp0 enabled=true
> +check_mc_group_ports _MC_flood ls0 ln0 lsp0 lsp1
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp0 lsp1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Delete an enabled VIF.
> +check ovn-nbctl --wait=sb lsp-del lsp0
> +check_mc_group_ports _MC_flood ls0 ln0 lsp1
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Add a router and connect it to ls0. The router LSP should appear
> +dnl in _MC_flood but NOT in _MC_flood_l2 (router ports are excluded
> +dnl from _MC_flood_l2).
> +check ovn-nbctl --wait=sb \
> + lr-add lr0 \
> + -- lrp-add lr0 lr0-ls0 00:00:00:00:01:00 10.0.0.254/24 \
> + -- lsp-add-router-port ls0 ls0-lr0 lr0-ls0
> +check_mc_group_ports _MC_flood ls0 ln0 lsp1 ls0-lr0
> +check_mc_group_ports _MC_flood_l2 ls0 ln0 lsp1
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +dnl Add a second switch connected to the router, with a localnet port
> +dnl and a VIF.
> +check ovn-nbctl --wait=sb \
> + ls-add ls1 \
> + -- lsp-add-localnet-port ls1 ln1 physnet1 \
> + -- lrp-add lr0 lr0-ls1 00:00:00:00:02:00 10.0.1.254/24 \
> + -- lsp-add-router-port ls1 ls1-lr0 lr0-ls1 \
> + -- lsp-add ls1 lsp2
> +check_mc_group_ports _MC_flood ls1 ln1 ls1-lr0 lsp2
> +check_mc_group_ports _MC_flood_l2 ls1 ln1 lsp2
> +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD_NO_HV([
> AT_SETUP([Transit router - remote ports])
> ovn_start
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
