Hi Lorenzo,
On 4/16/25 1:07 PM, Lorenzo Bianconi wrote:
> Avoid recirculating IGMP/MLD packets more than one time from stage
> ls_out_pre_lb in the egress pipeline to ovn table 37 in order to avoid
> packet looping for ovn-ic deployment.
>
> Reported-at: https://issues.redhat.com/browse/FDP-58
> Acked-by: Mohammad Heib <mh...@redhat.com>
I assume this ack is a leftover from the old attempt (that got reverted
in the meantime). I removed it, I hope that's fine.
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> ---
> controller/pinctrl.c | 2 ++
> include/ovn/logical-fields.h | 3 +++
> lib/logical-fields.c | 5 +++++
> northd/en-multicast.c | 2 +-
> northd/northd.c | 11 +++++++----
> tests/ovn-ic.at | 32 +++++++++++++++++++++++++++++++-
> 6 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index bdb619b4d..5e24d97c8 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -727,6 +727,8 @@ pinctrl_forward_pkt(struct rconn *swconn, int64_t dp_key,
> put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
> put_load(in_port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
> put_load(out_port_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
> + /* Avoid re-injecting packet already consumed. */
> + put_load(1, MFF_LOG_FLAGS, MLF_IGMP_IGMP_SNOOP_INJECT_BIT, 1, &ofpacts);
>
> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
> resubmit->in_port = OFPP_CONTROLLER;
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index 17154d38b..653145ddd 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -103,6 +103,7 @@ enum mff_log_flags_bits {
> MLF_FROM_CTRL_BIT = 19,
> MLF_UNSNAT_NEW_BIT = 20,
> MLF_UNSNAT_NOT_TRACKED_BIT = 21,
> + MLF_IGMP_IGMP_SNOOP_INJECT_BIT = 22,
> MLF_NETWORK_ID_START_BIT = 28,
> MLF_NETWORK_ID_END_BIT = 31,
> };
> @@ -169,6 +170,8 @@ enum mff_log_flags {
> /* Indicate that the packet didn't go through unSNAT. */
> MLF_UNSNAT_NOT_TRACKED = (1 << MLF_UNSNAT_NOT_TRACKED_BIT),
>
I added a comment here:
/* Indicate that this is an IGMP packet reinjected by ovn-controller. */
> + MLF_IGMP_IGMP_SNOOP = (1 << MLF_IGMP_IGMP_SNOOP_INJECT_BIT),
> +
> /* Assign network ID to packet to choose correct network for snat when
> * lb_force_snat_ip=router_ip. */
> MLF_NETWORK_ID = (OVN_MAX_NETWORK_ID << MLF_NETWORK_ID_START_BIT),
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index db2d08ada..a74e18e0f 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -154,6 +154,11 @@ ovn_init_symtab(struct shash *symtab)
> snprintf(flags_str, sizeof flags_str, "flags[%d]", MLF_FROM_CTRL_BIT);
> expr_symtab_add_subfield(symtab, "flags.from_ctrl", NULL, flags_str);
>
> + snprintf(flags_str, sizeof flags_str, "flags[%d]",
> + MLF_IGMP_IGMP_SNOOP_INJECT_BIT);
> + expr_symtab_add_subfield(symtab, "flags.igmp_loopback", NULL,
> + flags_str);
Nit: this fits on one line.
> +
> /* Connection tracking state. */
> expr_symtab_add_field_scoped(symtab, "ct_mark", MFF_CT_MARK, NULL, false,
> WR_CT_COMMIT);
> diff --git a/northd/en-multicast.c b/northd/en-multicast.c
> index 7f3a6077c..657e20b39 100644
> --- a/northd/en-multicast.c
> +++ b/northd/en-multicast.c
> @@ -625,7 +625,7 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group
> *sb_igmp_group,
> }
>
> /* If this is already a flood port skip it for the group. */
> - if (port->mcast_info.flood) {
> + if (port->mcast_info.flood || port->mcast_info.flood_reports) {
> continue;
> }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 1f9340e55..5416244cc 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -6125,7 +6125,8 @@ build_interconn_mcast_snoop_flows(struct ovn_datapath
> *od,
> continue;
> }
> /* Punt IGMP traffic to controller. */
> - char *match = xasprintf("inport == %s && igmp", op->json_key);
> + char *match = xasprintf("inport == %s && igmp && "
> + "flags.igmp_loopback == 0", op->json_key);
> ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
> "clone { igmp; }; next;",
> copp_meter_get(COPP_IGMP, od->nbs->copp,
> @@ -6134,7 +6135,8 @@ build_interconn_mcast_snoop_flows(struct ovn_datapath
> *od,
> free(match);
>
> /* Punt MLD traffic to controller. */
> - match = xasprintf("inport == %s && (mldv1 || mldv2)", op->json_key);
> + match = xasprintf("inport == %s && (mldv1 || mldv2) && "
> + "flags.igmp_loopback == 0", op->json_key);
> ovn_lflow_metered(lflows, od, S_SWITCH_OUT_PRE_LB, 120, match,
> "clone { igmp; }; next;",
> copp_meter_get(COPP_IGMP, od->nbs->copp,
> @@ -9994,14 +9996,15 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
> ds_put_cstr(actions, "igmp;");
> /* Punt IGMP traffic to controller. */
> ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> - "igmp", ds_cstr(actions),
> + "flags.igmp_loopback == 0 && igmp",
> ds_cstr(actions),
> copp_meter_get(COPP_IGMP, od->nbs->copp,
> meter_groups),
> lflow_ref);
>
> /* Punt MLD traffic to controller. */
> ovn_lflow_metered(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> - "mldv1 || mldv2", ds_cstr(actions),
> + "flags.igmp_loopback == 0 && (mldv1 || mldv2)",
> + ds_cstr(actions),
> copp_meter_get(COPP_IGMP, od->nbs->copp,
> meter_groups),
> lflow_ref);
> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
> index c8276189c..401a71580 100644
> --- a/tests/ovn-ic.at
> +++ b/tests/ovn-ic.at
> @@ -2043,6 +2043,10 @@ check ovs-vsctl -- add-port br-int hv2-vif2 \
> -- set interface hv2-vif2 external-ids:iface-id=lsp3 \
> options:tx_pcap=hv2/vif2-tx.pcap \
> options:rxq_pcap=hv2/vif2-rx.pcap
> +check ovs-vsctl -- add-port br-int hv2-vif3 \
> + -- set interface hv2-vif3 external-ids:iface-id=lsp4 \
> + options:tx_pcap=hv2/vif3-tx.pcap \
> + options:rxq_pcap=hv2/vif3-rx.pcap
> check ovs-vsctl set open . external-ids:ovn-is-interconn=true
>
> AT_CHECK([ovn-ic-nbctl --wait=sb create Transit_Switch name=ts], [0],
> [ignore])
> @@ -2081,6 +2085,8 @@ check ovn-nbctl lsp-add ts ts-lr2 \
> -- lsp-set-addresses ts-lr2 router \
> -- lsp-set-type ts-lr2 router \
> -- lsp-set-options ts-lr2 router-port=lr2-ts
> +check ovn-nbctl lsp-add ts lsp4 \
> + -- lsp-set-addresses lsp4 unknown
>
> check ovn-nbctl lr-add lr3 \
> -- lrp-add lr3 lr3-ts 00:00:00:02:00:02 42.42.42.3/24 4242::3/64 \
> @@ -2165,6 +2171,18 @@ send_mld_v2_report hv2-vif2 hv2 \
> ff0adeadbeef00000000000000000001 04 c0e4 \
> /dev/null
>
> +# Inject IGMP Join for 239.0.1.68 on LSP4.
> +send_igmp_v3_report hv2-vif3 hv2 \
> + 000000000001 $(ip_to_hex 10 0 0 42) f9cf \
> + $(ip_to_hex 239 0 1 68) 04 e9b9 \
> + /dev/null
> +
> +# Inject MLD Join for ff0a:dead:beef::1 on LSP3.
> +send_mld_v2_report hv2-vif3 hv2 \
> + 000000000001 10000000000000000000000000000042 \
> + ff0adeadbeef00000000000000000001 04 c0a3 \
> + /dev/null
> +
> # Check that the IGMP and MLD groups are learned on both AZs (on the LS
> # and TS).
> ovn_as az1
> @@ -2181,6 +2199,7 @@ check ovn-nbctl --wait=hv sync
> # to lsp1 and lsp3.
> > expected_az1
> > expected_az2
> +> expected_az2-lsp4
> send_ip_multicast_pkt hv2-vif1 hv2 \
> 000000000001 01005e000144 \
> $(ip_to_hex 44 44 44 2) $(ip_to_hex 239 0 1 68) 1e 20 7c6b 11 \
> @@ -2193,6 +2212,10 @@ store_ip_multicast_pkt \
> 000000020200 01005e000144 \
> $(ip_to_hex 44 44 44 2) $(ip_to_hex 239 0 1 68) 1e 1e 7e6b 11 \
> e518e518000aed350000 expected_az2
> +store_ip_multicast_pkt \
> + 000000020001 01005e000144 \
> + $(ip_to_hex 44 44 44 2) $(ip_to_hex 239 0 1 68) 1e 1f 7d6b 11 \
> + e518e518000aed350000 expected_az2-lsp4
>
> send_ip6_multicast_pkt hv2-vif1 hv2 \
> 000000000001 333300000001 \
> @@ -2211,10 +2234,17 @@ store_ip6_multicast_pkt \
> 000e 3e 11 \
> 93407a69000e2b4e61736461640a \
> expected_az2
> +store_ip6_multicast_pkt \
> + 000000020001 333300000001 \
> + 00100000000000000000000000000042 ff0adeadbeef00000000000000000001 \
> + 000e 3f 11 \
> + 93407a69000e2b4e61736461640a \
> + expected_az2-lsp4
>
> OVS_WAIT_UNTIL(
> [check_packets 'hv1/vif1-tx.pcap expected_az1' \
> - 'hv2/vif2-tx.pcap expected_az2'],
> + 'hv2/vif2-tx.pcap expected_az2' \
> + 'hv2/vif3-tx.pcap expected_az2-lsp4'],
> [$at_diff -F'^---' exp rcv])
>
> OVN_CLEANUP_SBOX([hv1], ["/IGMP Querier enabled without a valid IPv4/d
With the minor comments I had above addressed I went ahead and pushed
this patch to main.
Even though it's a bug fix, I chose not to backport it because:
1. it introduces new logical fields and we'd need to be careful handling
those during minor upgrades
2. as far as I know there's no user configuring OVN IC and IGMP in this
specific way yet.
If there's a need to backport this in the future we can revisit it and
maybe add a feature flag to be checked by ovn-northd in case of minor
upgrades that first upgrade ovn-northd.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
