On Mon, Apr 7, 2025 at 11:26 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 4/7/25 5:22 PM, Numan Siddique wrote:
> > On Mon, Apr 7, 2025 at 9:15 AM Dumitru Ceara <dce...@redhat.com> wrote:
> >>
> >> On 4/5/25 6:38 AM, num...@ovn.org wrote:
> >>> From: Numan Siddique <num...@ovn.org>
> >>>
> >>
> >> Hi Numan,
> >>
> >> Thanks for the patch!
> >>
> >>> After the commit [1], OVN doesn't reply to the unicast ARP
> >>> requests for the known IPs. This patch adds a global option
> >>> "unicast_arp_responder" and if set to true, OVN will also
> >>> respond to the unicast ARP requests. This gives the option
> >>> for deployments which were relying on the older behavior.
> >>>
> >>
> >> I wonder if the new unicast_arp_responder option should be a router (or
> >> switch) option instead. Or maybe keep the global option but also add
> >> per router or switch ways of tuning this. Some deployments might not
> >> want this behavior globally.
> >
> > Thanks for the review. I think the option should be a switch one
> > since the arp responder flows
> > are added in the logical switch pipeline and arps are generally within
> > the same subnet boundary.
> > I'll address your comments and submit v2.
> >
>
> Hi Numan,
>
> Ack, that is, if we need this knob. Please see below.
>
> >>
> >>> However the main reason to add this option is to address
> >>> another issue. Consider the below topology where logical switches
> >>> sw0 and sw1 are connected lr0. lr0 is also connected to a
> >>> public switch which is not shown here.
> >>>
> >>> switch 8144b9c4-b38f-47db-a403-62283c8a5756 (sw0)
> >>> port sw0-port2
> >>> addresses: ["50:54:00:00:00:04 10.0.0.4"]
> >>> port sw0-port1
> >>> addresses: ["50:54:00:00:00:03 10.0.0.3"]
> >>> port sw0-lr0
> >>> type: router
> >>> addresses: ["00:00:00:00:ff:01"]
> >>> router-port: lr0-sw0
> >>>
> >>> switch 8f53c293-9486-49ff-9e62-98ea3408ee2e (sw1)
> >>> port sw1-port2
> >>> addresses: ["40:54:00:00:00:04 20.0.0.4"]
> >>> port sw1-lr0
> >>> type: router
> >>> addresses: ["00:00:00:00:ff:02"]
> >>> router-port: lr0-sw1
> >>> port sw1-port1
> >>> addresses: ["40:54:00:00:00:03 20.0.0.3"]
> >>>
> >>> ovn-nbctl lr-nat-list lr0
> >>> TYPE EXTERNAL_IP LOGICAL_IP EXTERNAL_MAC
> >>> LOGICAL_PORT
> >>> dnat_and_snat 172.168.0.110 10.0.0.3 50:54:00:00:00:03
> >>> sw0-port1
> >>> dnat_and_snat 172.168.0.120 20.0.0.3 40:54:00:00:00:04
> >>> sw1-port1
> >>>
> >>> Here the logical port's mac is also used as external mac in the NATs.
> >>>
> >>> ovn-northd adds the below logical flow in both the sw0 and sw1 pipeline
> >>>
> >>> table=28(ls_in_l2_lkup ), priority=75 ,
> >>> match=(eth.src == {.., 50:54:00:00:00:03, 40:54:00:00:00:04, ..}
> >>> && (arp.op == 1 || rarp.op == 3 || nd_ns)),
> >>> action=(outport = "_MC_flood_l2"; output;)
> >>>
> >>> This lflow is added to broadcast the self originated GARP packets.
> >>> Most likely to broadcast the GARPs generated by ovn-controller
> >>> and where the router MAC is used as source address [2].
> >>> Seems like this flow should have been added only on the logical switches
> >>> connected to the router if the IPs are reachable from it.
> >>>
> >>
> >> Just a note, the behavior before [2] was to always flood so I think this
> >> is not necessarily a problem introduced by [2] (32f5ebb06226
> >> ("ovn-northd: Limit ARP/ND broadcast domain whenever possible.")).
> >>
> >
> > The issue here is with the ARP unicast request packets generated by a
> > logical port's VIF.
> > If the logical port's mac is also used for its dnat_and_snat entry in
> > the router, then
> > any ARP request packet generated by a logical port VIF is flooded
> > instead of sending it to
> > the destination, which is wrong.
> >
> >
> >> The problem [2] was trying to avoid was determining whether a packet is
> >> GARP. There's no easy way to do that today without adding a per-OVN-IP
> >> flow which might be prohibitive from performance perspective.
> >>
> >
> > If I understand correctly [2] was added to flood the GARP packets
> > generated by ovn-controller right ?
> >
> > If so,
> > - can't we modify the flow added by [2] in the ls_in_l2_lkup stage
> > to also match on "flags.from_ctrl".
>
> I was thinking about that at some point but I wasn't sure if it would
> work fine for VLAN-backed networks.
>
> > Although we need to see if we preserve the flags when the packet
> > is cloned to enter the switch pipelines.
>
> We do preserve flags, don't we?
We don't actually. REG10 is used for flags.from_ctrl and we clear
that register when we clone
>
> > - Since ovn-controller generates GARPs with broadcast eth
> > destination, why can't we also match on "eth.dst ==
> > ff:ff:ff:ff:ff:ff:ff" ?
> >
>
> That's a good point, we should probably do that indeed. Would that
> avoid the need of the new knob?
>
I think so. Let me submit another patch which matches on eth.dst.
Regarding the unicast arp responder, let's put this patch on hold and
revisit it if required.
I'll check internally if we want unicast arp responders or not.
Thanks
Numan
> > If we can use one or both the above matches, then probably it's better
> > to fix this too. I'll submit another patch.
> >
>
> Looking forward to it!
>
> Thanks,
> Dumitru
>
> >
> >> Adding Ilya in CC because we had an offline discussion regarding the
> >> possibility of adding an OVS extension to allow OVN to match on "packet
> >> is GARP". That would definitely simplify OVN's life in this case.
> >>
> >>> When sw0-port1 sends a unicast ARP request to any IP in its subnet,
> >>> instead of tunneling the packet to the destination chassis, the packet
> >>> gets flooded in the L2 domain since the sw0-port1's mac matches this
> >>> flow.
> >>>
> >>
> >> We could change the per NAT ARP flooding flows and only flood if the ARP
> >> target is a NAT public IP that's in the network of a connected LRP.
> >> That means at most one flow per NAT which might be acceptable.
> >>
> >>> Until we fix this logical flow properly, this patch is sufficient to
> >>> address the unicast ARP request issue. This causes a flood of
> >>> ARP packets in the geneve network.
> >>>
> >>> [1] - c48ed1736a58("Do not reply on unicast arps for IPv4 targets.")
> >>> [2] - 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain whenever
> >>> possible.")
> >>>
> >>> Signed-off-by: Numan Siddique <num...@ovn.org>
> >>> ---
> >>> northd/en-global-config.c | 6 ++++++
> >>> northd/northd.c | 26 +++++++++++++++++---------
> >>> ovn-nb.xml | 9 +++++++++
> >>> tests/ovn-northd.at | 19 +++++++++++++++++--
> >>> 4 files changed, 49 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
> >>> index c103b137f6..d9b7dcb0a8 100644
> >>> --- a/northd/en-global-config.c
> >>> +++ b/northd/en-global-config.c
> >>> @@ -269,6 +269,12 @@ global_config_nb_global_handler(struct engine_node
> >>> *node, void *data)
> >>> return false;
> >>> }
> >>>
> >>> + /* Check if northd_internal_version has changed or not. */
> >>> + if (config_out_of_sync(&nb->options, &config_data->nb_options,
> >>> + "unicast_arp_responder", false)) {
> >>> + return false;
> >>> + }
> >>> +
> >>> if (check_nb_options_out_of_sync(nb, config_data, sampling_apps)) {
> >>> config_data->tracked_data.nb_options_changed = true;
> >>> }
> >>> diff --git a/northd/northd.c b/northd/northd.c
> >>> index 880ec92ac2..78a7125ce1 100644
> >>> --- a/northd/northd.c
> >>> +++ b/northd/northd.c
> >>> @@ -98,6 +98,9 @@ static bool vxlan_mode;
> >>>
> >>> static bool vxlan_ic_mode;
> >>>
> >>> +/* Respond to unicast ARP requests for known ips or not. */
> >>> +static bool unicast_arp_responder = false;
> >>> +
> >>> #define MAX_OVN_TAGS 4096
> >>>
> >>>
> >>> @@ -9515,15 +9518,16 @@ build_lswitch_arp_nd_responder_known_ips(struct
> >>> ovn_port *op,
> >>> for (size_t i = 0; i < op->n_lsp_addrs; i++) {
> >>> for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
> >>> ds_clear(match);
> >>> - /* Do not reply on unicast ARPs, forward them to the
> >>> target
> >>> - * to have ability to monitor target liveness via unicast
> >>> - * ARP requests.
> >>> - */
> >>> - ds_put_format(match,
> >>> - "arp.tpa == %s && "
> >>> - "arp.op == 1 && "
> >>> - "eth.dst == ff:ff:ff:ff:ff:ff",
> >>> - op->lsp_addrs[i].ipv4_addrs[j].addr_s);
> >>> + ds_put_format(match, "arp.tpa == %s && arp.op == 1",
> >>> + op->lsp_addrs[i].ipv4_addrs[j].addr_s);
> >>> +
> >>> + if (!unicast_arp_responder) {
> >>> + /* Do not reply on unicast ARPs, forward them to the
> >>> target
> >>> + * to have ability to monitor target liveness via
> >>> unicast
> >>> + * ARP requests.
> >>> + */
> >>> + ds_put_cstr(match, " && eth.dst ==
> >>> ff:ff:ff:ff:ff:ff");
> >>> + }
> >>> ds_clear(actions);
> >>> ds_put_format(actions,
> >>> "eth.dst = eth.src; "
> >>> @@ -18980,6 +18984,10 @@ ovnnb_db_run(struct northd_input *input_data,
> >>> use_common_zone = smap_get_bool(input_data->nb_options,
> >>> "use_common_zone",
> >>> false);
> >>>
> >>> + unicast_arp_responder = smap_get_bool(input_data->nb_options,
> >>> + "unicast_arp_responder",
> >>> + false);
> >>> +
> >>> vxlan_mode = is_vxlan_mode(input_data->nb_options,
> >>> input_data->sbrec_chassis_table);
> >>>
> >>> diff --git a/ovn-nb.xml b/ovn-nb.xml
> >>> index bf1f1628bd..7f8021667c 100644
> >>> --- a/ovn-nb.xml
> >>> +++ b/ovn-nb.xml
> >>> @@ -429,6 +429,15 @@
> >>> </p>
> >>> </column>
> >>>
> >>> + <column name="options" key="unicast_arp_responder" type='{"type":
> >>> "boolean"}'>
> >>> + <p>
> >>> + <code>ovn-northd</code> adds ARP responder flows for known IPs
> >>> for
> >>> + broadcast ARP request packets. If this option is set, it will
> >>> also
> >>> + respond to unicast ARP requests. By default this option is
> >>> set to
> >>> + <code>false</code>.
> >>> + </p>
> >>> + </column>
> >>> +
> >>
> >> I guess we also need a NEWS file entry for this new feature.
> >
> > Ack.
> >
> > Thanks
> > Numan
> >
> >>
> >>> <group title="Options for configuring interconnection route
> >>> advertisement">
> >>> <p>
> >>> These options control how routes are advertised between OVN
> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>> index e3a65e7a44..f0322a311c 100644
> >>> --- a/tests/ovn-northd.at
> >>> +++ b/tests/ovn-northd.at
> >>> @@ -9869,7 +9869,7 @@ AT_CLEANUP
> >>> ])
> >>>
> >>> OVN_FOR_EACH_NORTHD_NO_HV([
> >>> -AT_SETUP([check options:disable_arp_nd_rsp for LSP])
> >>> +AT_SETUP([ARP responder flows])
> >>> ovn_start NORTHD_TYPE
> >>> check ovn-nbctl ls-add S1
> >>> check ovn-nbctl --wait=sb lsp-add S1 S1-vm1
> >>> @@ -9886,7 +9886,22 @@ AT_CHECK([grep -e "ls_in_arp_rsp" S1flows |
> >>> ovn_strip_lflows], [0], [dnl
> >>> table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast &&
> >>> ip6.dst == ff02::1:ff00:10 && nd.target == fd00::10), action=(nd_na {
> >>> eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10;
> >>> nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output;
> >>> };)
> >>> ])
> >>>
> >>> -#Set the disable_arp_nd_rsp option and verify the flow
> >>> +check ovn-nbctl set NB_Global . options:unicast_arp_responder=true
> >>> +check ovn-nbctl --wait=sb sync
> >>> +
> >>> +ovn-sbctl dump-flows S1 > S1flows
> >>> +AT_CAPTURE_FILE([S1flows])
> >>> +
> >>> +AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl
> >>> + table=??(ls_in_arp_rsp ), priority=0 , match=(1),
> >>> action=(next;)
> >>> + table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa ==
> >>> 192.168.0.10 && arp.op == 1 && inport == "S1-vm1"), action=(next;)
> >>> + table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns_mcast &&
> >>> ip6.dst == ff02::1:ff00:10 && nd.target == fd00::10 && inport ==
> >>> "S1-vm1"), action=(next;)
> >>> + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa ==
> >>> 192.168.0.10 && arp.op == 1), action=(eth.dst = eth.src; eth.src =
> >>> 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha
> >>> = 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport =
> >>> inport; flags.loopback = 1; output;)
> >>> + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast &&
> >>> ip6.dst == ff02::1:ff00:10 && nd.target == fd00::10), action=(nd_na {
> >>> eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10;
> >>> nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output;
> >>> };)
> >>> +])
> >>> +
> >>> +
> >>> +# Set the disable_arp_nd_rsp option and verify the flow
> >>> check ovn-nbctl --wait=sb set logical_switch_port S1-vm1
> >>> options:disable_arp_nd_rsp=true
> >>>
> >>> ovn-sbctl dump-flows S1 > S1flows
> >>
> >> Thanks,
> >> Dumitru
> >>
> >>
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
