> On Thu, Aug 28, 2025 at 4:14 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
>
> > Introduce disable_garp_rarp option in the Logical_Router table in order
> > to disable GARP/RARP announcements by all the peer ports of this logical
> > router.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-1537
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> > ---
> > Changes in v3:
> > - Trigger garp_rarp_run if datapath sb table change
> > - Improve unit-test
> > - cosmetics
> > ---
> >
>
> Hi Lorenzo,
>
> thank you for the v3, I have a few comments down below.
Hi Ales,
thx for the review.
>
>
> > NEWS | 2 +
> > controller/garp_rarp.c | 26 ++++++++++-
> > controller/ovn-controller.c | 1 +
> > northd/en-datapath-logical-router.c | 5 ++
> > ovn-nb.xml | 9 ++++
> > tests/ovn.at | 72 +++++++++++++++++++++++++++++
> > 6 files changed, 113 insertions(+), 2 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 932e173af..f21a7e235 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -66,6 +66,8 @@ OVN v25.09.0 - xxx xx xxxx
> > - ovn-sbctl:
> > * Added --filter option that allows filtering the command output. See
> > ovn-sbctl(8) for more details.
> > + - Added disable_garp_rarp option to logical_router table in order to
> > disable
> > + GARP/RARP announcements by all the peer ports of this logical router.
> >
> > OVN v25.03.0 - 07 Mar 2025
> > --------------------------
> > diff --git a/controller/garp_rarp.c b/controller/garp_rarp.c
> > index 8eccf10fe..940c38e13 100644
> > --- a/controller/garp_rarp.c
> > +++ b/controller/garp_rarp.c
> > @@ -446,6 +446,26 @@ garp_rarp_clear(struct garp_rarp_ctx_in *r_ctx_in)
> > sset_clear(&r_ctx_in->data->local_lports);
> > }
> >
> > +static bool
> > +garp_rarp_is_enabled(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > + const struct sbrec_port_binding *pb)
> > +{
> > + if (smap_get_bool(&pb->options, "disable_garp_rarp", false)) {
> > + return false;
> > + }
> > +
> > + /* Check if GARP probing is disabled on the peer logical router. */
> > + const struct sbrec_port_binding *peer = lport_get_peer(
> > + pb, sbrec_port_binding_by_name);
> > + if (peer && peer->datapath &&
> > + smap_get_bool(&peer->datapath->external_ids,
> > + "disable_garp_rarp", false)) {
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > void
> > garp_rarp_run(struct garp_rarp_ctx_in *r_ctx_in)
> > {
> > @@ -478,7 +498,8 @@ garp_rarp_run(struct garp_rarp_ctx_in *r_ctx_in)
> > SSET_FOR_EACH (iface_id, &localnet_vifs) {
> > const struct sbrec_port_binding *pb = lport_lookup_by_name(
> > r_ctx_in->sbrec_port_binding_by_name, iface_id);
> > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp",
> > false)) {
> > + if (pb &&
> > + garp_rarp_is_enabled(r_ctx_in->sbrec_port_binding_by_name,
> > pb)) {
> > send_garp_rarp_update(r_ctx_in, pb, &nat_addresses);
> > }
> > }
> > @@ -488,7 +509,8 @@ garp_rarp_run(struct garp_rarp_ctx_in *r_ctx_in)
> > SSET_FOR_EACH (gw_port, &local_l3gw_ports) {
> > const struct sbrec_port_binding *pb = lport_lookup_by_name(
> > r_ctx_in->sbrec_port_binding_by_name, gw_port);
> > - if (pb && !smap_get_bool(&pb->options, "disable_garp_rarp",
> > false)) {
> > + if (pb &&
> > + garp_rarp_is_enabled(r_ctx_in->sbrec_port_binding_by_name,
> > pb)) {
> > send_garp_rarp_update(r_ctx_in, pb, &nat_addresses);
> > }
> > }
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 6396fa898..61a79b308 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -6864,6 +6864,7 @@ main(int argc, char *argv[])
> > dns_cache_sb_dns_handler);
> >
> > engine_add_input(&en_garp_rarp, &en_ovs_open_vswitch, NULL);
> > + engine_add_input(&en_garp_rarp, &en_sb_datapath_binding, NULL);
> >
>
> This will unfortunately cause a lot of unrelated recomputes which could
> be pretty costly, we had an issue with this node in the past when it
> caused a lot of forced recomputes. I think the simplest handler
> which will minimize the impact should check:
> 1) Datapath is local.
> 2) Datapath is a logical router.
> 3) And the column external_ids has changed.
>
> This will prevent a lot of unnecessary
> recomputes.
ack, fine. Do you think it is ok to just return EN_UNHANDLED if the conditions
above are met? Or do you prefer something more precise?
>
> engine_add_input(&en_garp_rarp, &en_sb_chassis, NULL);
>
> engine_add_input(&en_garp_rarp, &en_sb_port_binding,
> > garp_rarp_sb_port_binding_handler);
> > diff --git a/northd/en-datapath-logical-router.c
> > b/northd/en-datapath-logical-router.c
> > index 22687e849..f6f49422f 100644
> > --- a/northd/en-datapath-logical-router.c
> > +++ b/northd/en-datapath-logical-router.c
> > @@ -83,6 +83,11 @@ gather_external_ids(const struct nbrec_logical_router
> > *nbr,
> > "%u", age_threshold);
> > }
> >
> > + bool disable_garp_rarp = smap_get_bool(&nbr->options,
> > "disable_garp_rarp",
> > + false);
> > + smap_add_format(external_ids, "disable_garp_rarp",
> > + disable_garp_rarp ? "true" : "false");
> > +
> > /* For backwards-compatibility, also store the NB UUID in
> > * external-ids:logical-router. This is useful if ovn-controller
> > * has not updated and expects this to be where to find the
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index b7b5b5c40..034500e00 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -3333,6 +3333,15 @@ or
> > routes in <code>ovn-ic</code> daemon.
> > </p>
> > </column>
> > +
> > + <column name="options" key="disable_garp_rarp"
> > + type='{"type": "boolean"}'>
> > + <p>
> > + If set to <code>true</code>, GARP and RARP announcements are not
> > + sent by all the VIF peer ports of this logical router.
> > + The default value is <code>false</code>.
> > + </p>
> > + </column>
> > </group>
> > <group title="Common Columns">
> > <column name="external_ids">
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 292ca0dae..cd5bdfeef 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26310,6 +26310,78 @@ OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> >
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([Disabling RARP/GARP announcements from Router options])
> > +ovn_start
> > +
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +
> > +check ovs-vsctl add-port br-phys snoopvif -- set Interface snoopvif
> > options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap
> > +check ovs-vsctl add-port br-int vif1 -- set Interface vif1
> > external-ids:iface-id=lsp1
> > +
> > +check ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=1
> > +
> > +check ovn-nbctl ls-add ls1
> > +check ovn-nbctl lsp-add ls1 ln1
> > +check ovn-nbctl lsp-set-addresses ln1 unknown
> > +check ovn-nbctl lsp-set-type ln1 localnet
> > +check ovn-nbctl lsp-set-options ln1 network_name=phys
> > +
> > +check ovn-nbctl lr-add lr1
> > +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="true"
> > +check ovn-nbctl lrp-add lr1 lrp 00:00:00:00:00:11 192.168.1.1/24
> > +check ovn-nbctl lsp-add ls1 ls-lrp \
> > + -- set Logical_Switch_Port ls-lrp type=router \
> > + options:router-port=lrp addresses=\"00:00:00:00:00:11\"
> > +check ovn-nbctl lrp-set-gateway-chassis lrp hv1
> > +
> > +check ovn-nbctl lsp-add ls1 lsp1
> > +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:12 192.168.1.2"
> >
>
> Sorry I didn't mention that earlier, but we should also define a NAT
> on the lr1 and make sure the NAT isn't advertised as well when the
> option is set to true. That's the main use case for the RFE.
>
> nit: Missing ovn-nbctl --wait=hv sync.
ack, I will add it.
>
>
> > +wait_for_ports_up
> > +
> > +garp_lrp=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> > src='00:00:00:00:00:11')/ \
> > + ARP(hwsrc='00:00:00:00:00:11', psrc='192.168.1.1',
> > pdst='192.168.1.1')")
> > +garp_vif=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff',
> > src='00:00:00:00:00:12')/ \
> > + ARP(hwsrc='00:00:00:00:00:12', psrc='192.168.1.2',
> > pdst='192.168.1.2')")
> > +# GARP packet for vif
> > +echo $garp_vif > expected
> > +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap >
> > hv1/snoopvif-tx.packets
> > +AT_CHECK([grep -q "$garp_lrp" hv1/snoopvif-tx.packets], [1])
> > +
> > +# GARP packet for lrp
> > +echo $garp_lrp >> expected
> > +check ovn-nbctl --wait=sb set Logical_Router lr1
> > options:disable_garp_rarp="false"
> >
>
> nit: The wait target should be hv instead of sb.
ack, I will fix it.
>
>
> > +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> > +
> > +# Check for GW router
> > +check ovn-nbctl lrp-del-gateway-chassis lrp hv1
> > +check ovn-nbctl set Logical_Router lr1 options:chassis="hv1"
> > +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="true"
> > +check ovn-nbctl --wait=sb sync
> >
>
> nit: Same here, s/sb/hv/
ack, I will fix it.
>
>
> > +
> > +sleep_controller hv1
> > +reset_pcap_file snoopvif hv1/snoopvif
> > +wake_up_controller hv1
> > +
> > +echo $garp_vif > expected
> > +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap >
> > hv1/snoopvif-tx.packets
> > +AT_CHECK([grep -q "$garp_lrp" hv1/snoopvif-tx.packets], [1])
> > +
> > +echo $garp_lrp >> expected
> > +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="false"
> >
>
> nit: Missing --wait=hv
ack, I will fix it.
Regards,
Lorenzo
>
>
> > +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> > +
> > +AT_CLEANUP
> > +])
> > +
> > OVN_FOR_EACH_NORTHD([
> > AT_SETUP([Stateless Floating IP])
> > ovn_start
> > --
> > 2.50.1
> >
> >
> Thanks,
> Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
