On 01/09/2016 12:59, "Thadeu Lima de Souza Cascardo" <casca...@redhat.com>
wrote:
>On Thu, Sep 01, 2016 at 05:17:44PM +0000, Daniele Di Proietto wrote:
>> Let me try to sum up the problem further
>>
>> 1) Behavior on master, before commit 47bf118665a3("ofproto: Always set MTU
>> for new internal ports."):
>>
>> a) When an internal interface is added, or its MTU has changed we only
>> change its MTU if it is bigger than the bridge minimum
>> b) When a non internal, non tunnel port is added, we reconfigure every
>> internal interface to match exactly the bridge minimum, no matter what
>> c) If the bridge doesn't have physical ports (only tunnels and
>> internals), we consider the minimum mtu to be invalid and we don't change
>> the internal interfaces mtu
>>
>> I felt that there was an inconsistency between 1a and 1b. I thought 1c was
>> not very clean, because the current configuration depends on the previous
>> state (if there were physical ports at some point, we would lower the
>> internal interfaces mtu, but we couldn't restore it when they were removed)
>>
>> 2) Behavior on master after commit 47bf118665a3("ofproto: Always set MTU for
>> new internal ports."):
>>
>> a) When an internal interface is added, or its MTU has changed we
>> overwrite its MTU with the bridge minimum.
>> b) When a non internal, non tunnel port is added, we reconfigure every
>> internal interface to match exactly the bridge minimum, no matter what
>> c) If the bridge doesn't have physical ports (only tunnels and
>> internals), we consider the minimum mtu to be 1500. We therefore force
>> every internal interface to be 1500.
>>
>> Behavior b is the same. Behavior a is changed to match more closely
>> behavior b. Behavior c is changed completely. Now the current
>> configuration doesn't depend on the previous state.
>> When I made the change I didn't think that there were valid use cases for
>> allowing the user to configure internal interfaces MTU. The test suite
>> obviously proved me wrong.
>>
>> ---------
>>
>> Proposed solution:
>>
>> A) This patch.
>>
>> Since OVS is not able the control the MTU in case of tunnelling or MPLS,
>> stop doing it entirely. The systems that relied on the old behavior need to
>> be updated.
>>
>> B) We could consider an hybrid solution that keeps backwards compatibility
>> for tunnelling use cases (like our testsuite).
>>
>> 2a)
>> 2b or 1b)
>> 1c)
>>
>>
>>
>>
>> This would not solve the problem for MPLS. MPLS uses physical devices,
>> so the internal interface would be forced to match the physical interfaces
>> and this is not OK for MPLS (or double vlans). Also, this solution keeps
>> behavior 1c, which makes the mtu assignment "stateful".
>>
>> Other ideas?
>>
>> Thanks,
>>
>> Daniele
>
>
>How about we let the interfaces to have a less than minimum MTU? Would that
>solve the problem with tunnels/overlays?
>
>Then, we partially revert the patch, like below. This has fixed at least the
>VXLAN system test for me.
>
>Cascardo.
>
>diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>index 9d62b72..6383dc8 100644
>--- a/ofproto/ofproto.c
>+++ b/ofproto/ofproto.c
>@@ -2752,8 +2752,10 @@ update_mtu(struct ofproto *p, struct ofport *port)
> return;
> }
> if (ofport_is_internal(p, port)) {
>- if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
>- dev_mtu = p->min_mtu;
>+ if (dev_mtu > p->min_mtu) {
>+ if (!netdev_set_mtu(port->netdev, p->min_mtu)) {
>+ dev_mtu = p->min_mtu;
>+ }
> }
> port->mtu = dev_mtu;
> return;
Hi Cascardo,
thanks for your suggestions. While I agree that your approach fixes the
system tests, and it's more consistent, it still breaks compatibility for
some users.
I'm taking a look at a system where the controller adds 'eth1' (with MTU 1600)
to 'br0' and it expects 'br0' MTU to go to 1600.
At this point I think the best thing to do is to keep the old behavior, like
Jesse and Darrell suggested, but allow the user to override it with mtu_request.
Thanks,
Daniele
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
