On 3/19/26 11:33 AM, Lorenzo Bianconi 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.
> Please note this is a patch specific for ovn branch-25.03.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1537
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
Hi Lorenzo,
Thanks for the backport patch! I have some comments that apply to all
backport patches for this feature, please see below.
> controller/pinctrl.c | 36 ++++++++++++++++-
> northd/northd.c | 5 +++
> ovn-nb.xml | 9 +++++
> tests/ovn.at | 93 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 141 insertions(+), 2 deletions(-)
>
This misses a NEWS file update.
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 0ccc7b1ee..3d440f31a 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -6489,6 +6489,28 @@ ip_mcast_querier_wait(long long int query_time)
> }
> }
>
> +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 = lport_get_l3gw_peer(pb, sbrec_port_binding_by_name);
> + }
> + if (peer && smap_get_bool(&peer->datapath->external_ids,
> + "disable_garp_rarp", false)) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> /* Get localnet vifs, local l3gw ports and ofport for localnet patch ports.
> */
> static void
> get_localnet_vifs_l3gwports(
> @@ -6536,6 +6558,11 @@ get_localnet_vifs_l3gwports(
> strcmp(iface_rec->link_state, "up")) {
> continue;
> }
> +
> + if (!garp_rarp_is_enabled(sbrec_port_binding_by_name, pb)) {
> + continue;
> + }
> +
In the original patch, on branches >= 25.09, we check
"garp_rarp_is_enabled()" later in the garp_rarp_run() loops over
localnet and local_l3gw_ports sets.
I think that's better. The reason is that with what you're proposing
here the semantics of the get_localnet_vifs_l3gwports() function changes
from "get all the localnet and l3gw ports" to "get all the localnet and
l3gw ports but only the l3gw ports that have garp_rarp enabled". That
seems overly complex.
Let's just add the check directly in the loops that "update
send_garp_rarp_data" in send_garp_rarp_prepare(), following the same
logic we have on the main branch.
> struct local_datapath *ld
> = get_local_datapath(local_datapaths,
> pb->datapath->tunnel_key);
> @@ -6564,8 +6591,9 @@ get_localnet_vifs_l3gwports(
> sbrec_port_binding_index_set_datapath(target, ld->datapath);
> SBREC_PORT_BINDING_FOR_EACH_EQUAL (pb, target,
> sbrec_port_binding_by_datapath) {
> - if ((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis)
> - || !strcmp(pb->type, "patch")) {
> + if (((!strcmp(pb->type, "l3gateway") && pb->chassis == chassis)
> + || !strcmp(pb->type, "patch")) &&
> + garp_rarp_is_enabled(sbrec_port_binding_by_name, pb)) {
> sset_add(local_l3gw_ports, pb->logical_port);
> }
> }
> @@ -6698,6 +6726,10 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> continue;
> }
>
> + if (!garp_rarp_is_enabled(sbrec_port_binding_by_name, pb)) {
> + continue;
> + }
> +
> if (pb->n_nat_addresses) {
> for (int i = 0; i < pb->n_nat_addresses; i++) {
> consider_nat_address(sbrec_port_binding_by_name,
> diff --git a/northd/northd.c b/northd/northd.c
> index 79ce1c996..96b950ada 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -837,6 +837,11 @@ ovn_datapath_update_external_ids(struct ovn_datapath *od)
> smap_add_format(&ids, "dynamic-routing-vrf-id", "%"PRId64,
> vrf_id);
> }
> +
> + 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 7c3edcedc..ab7f36fba 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -3290,6 +3290,15 @@ or
> for established sessions as we will commit everything in addition
> to already existing stateful NATs and LBs.
> </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">
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 7708de102..8b6752ca3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -43972,3 +43972,96 @@ AT_CHECK([ovs-ofctl dump-flows br-int
> table=$acl_in_eval | grep -q "tp_dst=80"],
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Disabling RARP/GARP announcements from Router options])
We should also backport the follow up fixes for the original patch,
i.e., we should backport the contents of:
f353dbd03cd7 ("tests: Mark fmt_pkt test with HAVE_SCAPY requirement.")
> +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 add-port br-int vif2 -- set Interface vif2
> external-ids:iface-id=lsp2
> +
> +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 lsp-add ls1 lsp1
> +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:12 192.168.1.2"
> +check ovn-nbctl --wait=hv sync
> +
> +check ovn-nbctl ls-add ls2
> +check ovn-nbctl lsp-add ls2 lsp2
> +check ovn-nbctl lsp-set-addresses lsp2 "00:00:00:00:00:13 10.0.0.2"
> +check ovn-nbctl --wait=hv sync
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="true"
> +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:11 192.168.1.1/24
> +check ovn-nbctl lrp-add lr1 lrp2 00:00:00:00:00:14 10.0.0.1/24
> +check ovn-nbctl lsp-add ls1 ls-lrp1 \
> + -- set Logical_Switch_Port ls-lrp1 type=router \
> + options:router-port=lrp1 addresses=\"00:00:00:00:00:11\"
> +check ovn-nbctl lsp-add ls2 ls-lrp2 \
> + -- set Logical_Switch_Port ls-lrp2 type=router \
> + options:router-port=lrp2 addresses=\"00:00:00:00:00:14\"
> +check ovn-nbctl lsp-set-options ls-lrp1 router-port=lrp1
> nat-addresses="router"
> +check ovn-nbctl lr-nat-add lr1 snat 192.168.1.10 10.0.0.0/24
> +check ovn-nbctl lrp-set-gateway-chassis lrp1 hv1
> +check ovn-nbctl --wait=hv sync
> +
> +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_nat=$(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.10',
> pdst='192.168.1.10')")
> +# 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])
> +AT_CHECK([grep -q "$garp_nat" hv1/snoopvif-tx.packets], [1])
> +
> +# GARP packet for lrp
> +echo $garp_lrp >> expected
> +echo $garp_nat >> expected
> +check ovn-nbctl --wait=hv set Logical_Router lr1
> options:disable_garp_rarp="false"
> +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> +
> +# Check for GW router
> +check ovn-nbctl lrp-del-gateway-chassis lrp1 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=hv sync
> +
> +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])
> +AT_CHECK([grep -q "$garp_nat" hv1/snoopvif-tx.packets], [1])
> +
> +echo $garp_lrp >> expected
> +echo $garp_nat >> expected
> +check ovn-nbctl set Logical_Router lr1 options:disable_garp_rarp="false"
> +check ovn-nbctl --wait=hv sync
> +OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], [expected])
> +
> +AT_CLEANUP
> +])
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
