> Hi Lorenzo,
Hi Martin,
thx for the review.
>
> On Tue, 2025-07-29 at 16:46 +0200, Lorenzo Bianconi via dev wrote:
> > Introduce disable_arp_nd_rsp option in the Logical_Router table in
>
> Possible typo, I think you meant to name the option
> 'disable_garp_rarp'?
ack, I will fix it in v2.
>
> > order
> > to disable GARP/RARP announcements by all the peer ports of this
> > logical
> > router.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> > ---
> > NEWS | 3 +++
> > controller/garp_rarp.c | 29 +++++++++++++++++++++++++++--
> > northd/northd.c | 5 +++++
> > ovn-nb.xml | 9 +++++++++
> > 4 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 0cce1790d..62cda1b89 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -41,6 +41,9 @@ 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_arp_nd_rsp option to logical_router table in
> > order to
>
> Typo in the option name?
ditto.
>
> > + 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);
> > + 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..40a78deff 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_arp_nd_rsp"
>
> Same typo in the option key?
ditto.
>
> > + type='{"type": "boolean"}'>
> > + <p>
> > + If set to <code>true</code>, GARP and RARP announcements
> > are not
> > + sent when by all the VIF peer ports of this logical
>
> The "when" is probably a typo.
ack, I will fix it in v2.
>
> > router.
> > + The default value is <code>false</code>.
> > + </p>
> > + </column>
>
> Would it make sense to add note to this option saying that it can be
> overriden on per-port basis by the "disable_garp_rarp" option in the
> LSP table?
Actually this option overrides the on per-port basis one since if you set it to
true, it will disable the GARP announcements on all peer ports.
>
> > </group>
> > <group title="Common Columns">
> > <column name="external_ids">
>
> I think that this feature could also use a test. Perhaps by extending
> (or creating variation of) "Disabling RARP/GARP announcements" test in
> "tests/ovn.at"
ack, I will add it in v2.
Regards,
Lorenzo
>
> Otherwise the logic seems sound to me.
>
> Thank you,
> Martin.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
