Re: [ovs-dev] [PATCH ovn] northd: add router broadcast option to logical switch
On Fri, Feb 24, 2023 at 6:04 AM Felix Hüttner via dev
wrote:
>
> Assume the following setup:
>
> ++
> | Logical Router |
> | lr001 +-+
> ++ |
>|
> ++ |
> | Logical Router | | ++ +--+
> | lr002 +-+-+ Logical Switch +-+ Phyiscal Network |
> ++ | | ls-ext | | |
>| ++ +--+
> ... |
>|
> ++ |
> | Logical Router | |
> | lr300 +-+
> ++
>
> If a arp request for the ip of lr001 on ls-ext is now received it is
> only forwarded to that individual logical router.
>
> If we however now receive a arp request for an ip not used by any of
> lr001-lr300 we try to flood the arp request to all logical ports on ls-ext.
> With around 300 routers this causes the arp request to be dropped after
> some routers as we hit the 4096 resubmit limit.
>
> In the most cases forwarding the arp requests to the logical routers is
> pointless as we already know all of their ip addresses and they will
> therefor not be able to answer the arp requests anyway.
> Only if someone sends garps this is not the case. Then the request would
> need to be flooded to all logical routers.
>
> We can therefor not generally send these arp requests to MC_FLOOD_L2 as
> this would break garps. As we can also not detect garps we need to leave
> the solution to our users.
>
> To do this we introduce the other_config `broadcast-arps-to-all-routers`
> on logical switches (which is per default true). If set to false we add
> a logical flow that forwards arp requests where we do not know a
> specific target logical switch port to MC_FLOOD_L2, thereby bypassing
> all logical routers.
>
> Note that the testcase is quite flaky in the ci (as it takes verry long)
> but runs well locally. I'm unsure how to best proceed there.
Thanks for the patch and sorry for the delay in reviews.
I don't see any harm in supporting this option and the patch overall
looks ok to me.
Regarding the test case flakiness, I don't think we want to introduce
a flaky test.
I'd suggest the following
- Either find a solution to fix the flakiness in the CI or
- Drop this test case and add a test case in ovn-northd.at which
makes sure that when this
option is set, ovn-northd generates the required logical flow and
when the option is cleared
it clears the logical flow. This should be sufficient IMO to
test this patch.
Also please add a NEWS entry for this new option.
Thanks
Numan
>
> Signed-off-by: Felix Huettner
> ---
> northd/northd.c | 7 +++
> northd/ovn-northd.8.xml | 7 +++
> ovn-nb.xml | 12 +
> tests/ovn.at| 114
> 4 files changed, 140 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index c366b545e..6aff04cc5 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8964,6 +8964,13 @@ build_lswitch_destination_lookup_bmcast(struct
> ovn_datapath *od,
> }
> }
>
> +
> +if (!smap_get_bool(>nbs->other_config,
> "broadcast-arps-to-all-routers", true)) {
> +ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
> +"eth.mcast && (arp.op == 1 || nd_ns)",
> +"outport = \""MC_FLOOD_L2"\"; output;");
> +}
> +
> ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
>"outport = \""MC_FLOOD"\"; output;");
> }
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 2eab2c4ae..033841383 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1873,6 +1873,13 @@ output;
> non-router logical ports.
>
>
> +
> +A priority-72 flow that outputs all ARP requests and ND packets with
> +an Ethernet broadcast or multicast eth.dst to the
> +MC_FLOOD_L2 multicast group if
> +other_config:broadcast-arps-to-all-routers=true.
> +
> +
>
> A priority-70 flow that outputs all packets with an Ethernet
> broadcast
> or multicast eth.dst to the MC_FLOOD
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 8d56d0c6e..a41d5b11f 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -729,6 +729,18 @@
> localnet ports, fabric traffic that belongs to other tagged networks
> may
> be passed through such a port.
>
> +
> + + type='{"type": "boolean"}'>
> +Determines whether arp requests and ipv6 neighbor solicitations
> should
> +be send to all routers and other switchports (default) or if it
> should
> +only be send to switchports where the ip/mac address is unknown.
> +Setting this to false can significantly reduce the load if the
> logical
> +switch can receive arp requests for ips
Re: [ovs-dev] [PATCH ovn] northd: add router broadcast option to logical switch
Bleep bloop. Greetings Felix Hüttner, I am a robot and I have tried out your
patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Author Felix Hüttner needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors
or committers: Felix Huettner
WARNING: Line is 92 characters long (recommended limit is 79)
#69 FILE: northd/northd.c:8968:
if (!smap_get_bool(>nbs->other_config,
"broadcast-arps-to-all-routers", true)) {
Lines checked: 247, Warnings: 2, Errors: 1
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: add router broadcast option to logical switch
Assume the following setup:
++
| Logical Router |
| lr001 +-+
++ |
|
++ |
| Logical Router | | ++ +--+
| lr002 +-+-+ Logical Switch +-+ Phyiscal Network |
++ | | ls-ext | | |
| ++ +--+
... |
|
++ |
| Logical Router | |
| lr300 +-+
++
If a arp request for the ip of lr001 on ls-ext is now received it is
only forwarded to that individual logical router.
If we however now receive a arp request for an ip not used by any of
lr001-lr300 we try to flood the arp request to all logical ports on ls-ext.
With around 300 routers this causes the arp request to be dropped after
some routers as we hit the 4096 resubmit limit.
In the most cases forwarding the arp requests to the logical routers is
pointless as we already know all of their ip addresses and they will
therefor not be able to answer the arp requests anyway.
Only if someone sends garps this is not the case. Then the request would
need to be flooded to all logical routers.
We can therefor not generally send these arp requests to MC_FLOOD_L2 as
this would break garps. As we can also not detect garps we need to leave
the solution to our users.
To do this we introduce the other_config `broadcast-arps-to-all-routers`
on logical switches (which is per default true). If set to false we add
a logical flow that forwards arp requests where we do not know a
specific target logical switch port to MC_FLOOD_L2, thereby bypassing
all logical routers.
Note that the testcase is quite flaky in the ci (as it takes verry long)
but runs well locally. I'm unsure how to best proceed there.
Signed-off-by: Felix Huettner
---
northd/northd.c | 7 +++
northd/ovn-northd.8.xml | 7 +++
ovn-nb.xml | 12 +
tests/ovn.at| 114
4 files changed, 140 insertions(+)
diff --git a/northd/northd.c b/northd/northd.c
index c366b545e..6aff04cc5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8964,6 +8964,13 @@ build_lswitch_destination_lookup_bmcast(struct
ovn_datapath *od,
}
}
+
+if (!smap_get_bool(>nbs->other_config,
"broadcast-arps-to-all-routers", true)) {
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
+"eth.mcast && (arp.op == 1 || nd_ns)",
+"outport = \""MC_FLOOD_L2"\"; output;");
+}
+
ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
"outport = \""MC_FLOOD"\"; output;");
}
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 2eab2c4ae..033841383 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1873,6 +1873,13 @@ output;
non-router logical ports.
+
+A priority-72 flow that outputs all ARP requests and ND packets with
+an Ethernet broadcast or multicast eth.dst to the
+MC_FLOOD_L2 multicast group if
+other_config:broadcast-arps-to-all-routers=true.
+
+
A priority-70 flow that outputs all packets with an Ethernet broadcast
or multicast eth.dst to the MC_FLOOD
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 8d56d0c6e..a41d5b11f 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -729,6 +729,18 @@
localnet ports, fabric traffic that belongs to other tagged networks
may
be passed through such a port.
+
+
+Determines whether arp requests and ipv6 neighbor solicitations should
+be send to all routers and other switchports (default) or if it should
+only be send to switchports where the ip/mac address is unknown.
+Setting this to false can significantly reduce the load if the logical
+switch can receive arp requests for ips it does not know about.
+However setting this to false also means that garps are no longer
+forwarded to all routers and therefor the mac bindings of the routers
+are no longer updated.
+
diff --git a/tests/ovn.at b/tests/ovn.at
index dc5c5df3f..dfef5dacc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5048,6 +5048,120 @@ OVN_CLEANUP([hv1], [hv2], [hv3])
AT_CLEANUP
])
+# 1 hypervisor, 1 logical switch with 1 logical port, 300 logical router
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([1 HVs, 1 LS, 2 lports/LS, 300 LR])
+AT_KEYWORDS([slowtest])
+ovn_start
+
+# Logical network:
+#
+# One logical switch ls1.
+# 300 logical routers lr001 - lr299. All connected to ls1
+# with one subnet:
+#lrp001 on ls1 for subnet 192.168.0.1/20
+#lrp002 on ls1 for subnet 192.168.0.2/20
+#...
+#lrp101 on ls1 for subnet 192.168.1.1/20
+#...
+#lrp299 on ls1 for subnet 192.168.2.99/20
+#
+# also 2 VIF on the first hypervisor.
+#
