Hi Han, I have one super tiny nit below, but other than that,
Acked-by: Mark Michelson <[email protected]> On Mon, Jan 12, 2026 at 11:39 AM Han Zhou <[email protected]> wrote: > > When a remote chassis has multiple encap IPs, the original code only > created ARP reply and ND NA ingress flows for the first tunnel found. > This caused responses arriving on other tunnels to miss the ingress > pipeline, failing to learn MAC bindings. > > Fix by iterating over all tunnels in the ingress loop and creating flows > for each tunnel that belongs to a remote chassis. The egress flood flow > still uses only one tunnel per chassis to avoid duplicate traffic. > > Fixes: 7c3f7f415f1d ("northd, controller: Flood ARP and NA packet on transit > router.") > Assisted-by: Cursor, with model: claude-4.5-opus-high > Signed-off-by: Han Zhou <[email protected]> > --- > controller/physical.c | 56 +++++++++++++++++++++++++++-------- > tests/ovn-controller.at | 65 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 109 insertions(+), 12 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index b9c60c8ab13c..103fd4d9c4ff 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -3098,6 +3098,9 @@ physical_eval_remote_chassis_flows(const struct > physical_ctx *ctx, > > ofpbuf_clear(egress_ofpacts); > > + /* For egress flooding, we only need one tunnel per remote chassis. > + * Using chassis_tunnel_find() which returns the first tunnel is > sufficient > + * since we just need to reach the remote chassis once per flood. */ > const struct sbrec_chassis *chassis; > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, ctx->chassis_table) { > if (!smap_get_bool(&chassis->other_config, "is-remote", false)) { > @@ -3125,7 +3128,46 @@ physical_eval_remote_chassis_flows(const struct > physical_ctx *ctx, > > ofpact_put_OUTPUT(egress_ofpacts)->port = tun->ofport; > prev = tun; > + } > + > + if (egress_ofpacts->size > 0) { > + match_init_catchall(&match); > + /* Match if the packet wasn't already received from tunnel. > + * This prevents from looping it back to the tunnel again. */ > + match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0, 0, > + MLF_RX_FROM_TUNNEL); > + ofctrl_add_flow(flow_table, OFTABLE_FLOOD_REMOTE_CHASSIS, 100, 0, > + &match, egress_ofpacts, hc_uuid); > + } > > + /* For ingress, we need to handle ALL tunnels to remote chassis because > + * ARP replies and ND NA responses could arrive on any tunnel. A remote > + * chassis may have multiple tunnels (e.g., multiple encap IPs). */ > + struct chassis_tunnel *tun; > + HMAP_FOR_EACH (tun, hmap_node, ctx->chassis_tunnels) { > + char *chassis_name = NULL; > + if (!encaps_tunnel_id_parse(tun->chassis_id, &chassis_name, > + NULL, NULL)) { > + continue; > + } > + > + /* Look up the chassis record and check if it's a remote chassis. */ > + const struct sbrec_chassis *remote_chassis = > + chassis_lookup_by_name(ctx->sbrec_chassis_by_name, chassis_name); > + free(chassis_name); > + > + if (!remote_chassis || > + !smap_get_bool(&remote_chassis->other_config, > + "is-remote", false)) { > + continue; > + } > + > + /* Do not create flows for Geneve if the TLV negotiation is not > + * finished. > + */ > + if (tun->type == GENEVE && !ctx->mff_ovn_geneve) { > + continue; > + } Nit: This if check is much cheaper compared to allocating a chassis_name, looking up a chassis, and looking up chassis options. It's probably worthwhile to put this at the top of the loop instead. > > ofpbuf_clear(&ingress_ofpacts); > put_load(1, MFF_LOG_FLAGS, MLF_RX_FROM_TUNNEL_BIT, 1, > @@ -3147,7 +3189,7 @@ physical_eval_remote_chassis_flows(const struct > physical_ctx *ctx, > ctx->mff_ovn_geneve); > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, > - chassis->header_.uuid.parts[0], > + remote_chassis->header_.uuid.parts[0], > &match, &ingress_ofpacts, hc_uuid); > > /* Add match on ND NA coming from remote chassis. */ > @@ -3161,20 +3203,10 @@ physical_eval_remote_chassis_flows(const struct > physical_ctx *ctx, > ctx->mff_ovn_geneve); > > ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 120, > - chassis->header_.uuid.parts[0], > + remote_chassis->header_.uuid.parts[0], > &match, &ingress_ofpacts, hc_uuid); > } > > - if (egress_ofpacts->size > 0) { > - match_init_catchall(&match); > - /* Match if the packet wasn't already received from tunnel. > - * This prevents from looping it back to the tunnel again. */ > - match_set_reg_masked(&match, MFF_LOG_FLAGS - MFF_REG0, 0, > - MLF_RX_FROM_TUNNEL); > - ofctrl_add_flow(flow_table, OFTABLE_FLOOD_REMOTE_CHASSIS, 100, 0, > - &match, egress_ofpacts, hc_uuid); > - } > - > ofpbuf_uninit(&ingress_ofpacts); > } > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index d7cc06d006a9..33bae69e17bf 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3672,6 +3672,71 @@ AT_CHECK_UNQUOTED([grep "cookie=$hv3_cookie," > phy_to_log_flows], [0], [dnl > OVN_CLEANUP([hv1]) > AT_CLEANUP > > +AT_SETUP([Remote chassis flood flows - multiple tunnels per chassis]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.11 24 geneve > + > +check ovs-vsctl set open . external_ids:ovn-is-interconn=true > + > +# Add a remote chassis with TWO encap IPs (two tunnels). > +# This tests that ingress flows are created for ALL tunnels to a remote > +# chassis, not just the first one found. > +check ovn-sbctl chassis-add hv2 geneve 192.168.0.12 > +check_uuid ovn-sbctl --id=@encap create Encap type=geneve ip=192.168.0.13 \ > + -- add Chassis hv2 encaps @encap > +check ovn-sbctl set chassis hv2 other_config:is-remote=true > + > +check ovn-nbctl --wait=hv sync > + > +# Wait for both tunnels to be created by checking ovn-chassis-id > external_ids. > +# Each tunnel port should have ovn-chassis-id=hv2@<remote_ip>%<local_ip>. > +ovs-vsctl list port > +ovs-vsctl list interface > +OVS_WAIT_UNTIL([ > + test "$(ovs-vsctl --bare --columns external_ids list port | \ > + grep -c 'ovn-chassis-id=hv2@')" -eq 2 > +]) > + > +chassis_cookie() { > + ovn-debug uuid-to-cookie $(fetch_column chassis _uuid name=$1) > +} > + > +ovs-ofctl dump-flows --names --no-stats br-int table=OFTABLE_PHY_TO_LOG > > phy_to_log_flows > +ovs-ofctl dump-flows --names --no-stats br-int > table=OFTABLE_FLOOD_REMOTE_CHASSIS > flood_flows > + > +# Check that egress flood flow outputs to only ONE tunnel (first found). > +# We should have exactly one output action to hv2. > +AT_CHECK([grep -c 'output:"ovn-hv2' flood_flows], [0], [dnl > +1 > +]) > + > +# Check that ingress flows exist for BOTH tunnels to hv2. > +# Each tunnel should have ARP and ND NA ingress flows. > +hv2_cookie="$(chassis_cookie hv2)" > + > +# Count total ingress flows with hv2's cookie - should be 4 (2 tunnels x 2 > flows each) > +AT_CHECK([grep -c "cookie=$hv2_cookie," phy_to_log_flows], [0], [dnl > +4 > +]) > + > +# Verify we have ARP flows for both tunnel ports > +AT_CHECK([grep "cookie=$hv2_cookie," phy_to_log_flows | grep -c > "arp,.*in_port=\"ovn-hv2"], [0], [dnl > +2 > +]) > + > +# Verify we have ND NA flows for both tunnel ports > +AT_CHECK([grep "cookie=$hv2_cookie," phy_to_log_flows | grep -c > "icmp6,.*in_port=\"ovn-hv2"], [0], [dnl > +2 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > + > AT_SETUP([ovn-cntroller exit - Cleanup]) > ovn_start > > -- > 2.38.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
