On Thu, Aug 21, 2025 at 6:52 PM Lorenzo Bianconi via dev < [email protected]> 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. > > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > Changes in v2: > - Fix typos in optiontion name > - Add unit-test > --- > Hi Lorenzo, thank you for the patch, it needs a rebase and I have a couple of comments down below. > NEWS | 2 ++ > controller/garp_rarp.c | 29 ++++++++++++++++++++++-- > northd/northd.c | 5 +++++ > ovn-nb.xml | 9 ++++++++ > tests/ovn.at | 51 ++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/NEWS b/NEWS > index 0cce1790d..7f4dc2bc0 100644 > --- a/NEWS > +++ b/NEWS > @@ -41,6 +41,8 @@ Post v25.03.0 > - Added support for running tests from the 'check-kernel' system test > target > under retis by setting OVS_TEST_WITH_RETIS=yes. See the 'Testing' > section > of the documentation 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..678e9f211 100644 > --- a/controller/garp_rarp.c > +++ b/controller/garp_rarp.c > @@ -446,6 +446,29 @@ 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 char *peer_name = smap_get(&pb->options, "peer"); > + if (peer_name) { > + const struct sbrec_port_binding *peer = lport_lookup_by_name( > + sbrec_port_binding_by_name, peer_name); > Please use "lport_get_peer()", that should work for both DGP and GW routers. > + 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 +501,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 +512,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/northd/northd.c b/northd/northd.c > index 764575f21..5d5bf23b5 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -851,6 +851,11 @@ ovn_datapath_update_external_ids(struct ovn_datapath > *od) > smap_add_format(&ids, "mac_binding_age_threshold", > "%u", age_threshold); > } > + > + bool disable_garp_rarp = smap_get_bool(&od->nbr->options, > + "disable_garp_rarp", > false); > + smap_add_format(&ids, "disable_garp_rarp", > + disable_garp_rarp ? "true" : "false"); > } > > sbrec_datapath_binding_set_external_ids(od->sb, &ids); > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 4a7581807..ce460c1f3 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3249,6 +3249,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 18ce07e1a..5e54a3ddc 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -26303,6 +26303,57 @@ 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 > + > +AT_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]) > nit: Simple check is enough. > + > +check ovn-nbctl ls-add ls1 > +check ovn-nbctl lsp-add ls1 ln1 "" 101 > nit: Is VLAN tagging needed? > +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 > We should also check that it works for GW routers. > + > +check ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=1 > +check ovn-nbctl --wait=sb sync > + > +# Collect some traffic. > +sleep 5 > To speed it up there could be VIF that will send garp. So instead of sleeping a call to OVN_CHECK_PACKETS_UNIQ with the expected garp from VIF will work. + > +check ovn-appctl -t ovn-controller debug/pause > +OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) > = "xpaused"]) > There is a `sleep_controller` helper. > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/snoopvif-tx.pcap > > hv1/snoopvif-tx.packets > +AT_CHECK([grep -q > 'ffffffffffff0000000000118100006508060001080006040001000000000011c0a80101000000000000c0a80101' > hv1/snoopvif-tx.packets], [1]) > The packet should be constructed via "fmt_pkt". > + > +# GARP packet for lrp > +echo > "ffffffffffff0000000000118100006508060001080006040001000000000011c0a80101000000000000c0a80101" > > expected > Same here. > +reset_pcap_file snoopvif hv1/snoopvif > +check ovn-appctl -t ovn-controller debug/resume > +OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) > = "xrunning"]) > There is a "wake_up_controller" helper. > +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="false" > +check ovn-appctl -t ovn-controller inc-engine/recompute > Why do we need to recompute? Shouldn't this be reflected by I-P when the option changes? If it's not the case we should investigate why and at least file a bug. > +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 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
