| Message ID | 20160831215253.75678-1-diproiettod@vmware.com |
|---|---|
| State | Rejected |
| Headers | show |
On Wed, Aug 31, 2016 at 2:52 PM, Daniele Di Proietto <diproiettod@vmware.com > wrote: > Open vSwitch controls the MTU of internal ports and sets it to the > minimum of physical ports MTU on the same bridge. > > Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") > made this more consistent. Now the MTU is always set, even when the > bridge doesn't have any physical ports (e.g. when it has only an > internal device and a tunnel). > > This latest change broke the system testsuite. Some tests need to > lower internal devices MTU because they use tunnels and they want to > work with a 1500 bytes underlay. > > There are other valid usecases where the user/controller needs to > configure the internal devices MTU (like mpls push actions, double vlans > or any overlay) > and there's no way for Open vSwitch to know what the > appropriate MTU should be. > > This is not a review. But, IMO, the above statement is correct. > Since in the general case Open vSwitch is not able to configure a > reasonable MTU for internal devices, this commit removes the feature > entirely. > > Now the user/controller is entirely responsible for configuring the MTU > of internal ports. Other hybrid solutions are possible (like not > touching the internal interfaces MTU, unless there's a physical device), > but they make the current MTU dependent on the previous state of the > system (if there was at some point a physical device the MTU would be > touched, but it wouldn't be possible to restore it). > > This change breaks compatibility with previous versions on Open vSwitch. > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > --- > I realize that it's not nice to introduce a backwards incompatible > change, especially so late in the release. I considered other possible > solutions, but they all introduce undesirable behavior. If it's not > acceptable to break compatibility, I can get away with alternative > approaches. > --- > NEWS | 4 ++ > debian/changelog | 4 ++ > ofproto/ofproto-provider.h | 2 - > ofproto/ofproto.c | 101 ------------------------------ > --------------- > tests/ofproto-dpif.at | 16 +------ > vswitchd/vswitch.xml | 10 ++--- > 6 files changed, 13 insertions(+), 124 deletions(-) > > diff --git a/NEWS b/NEWS > index 1439151..590a7b9 100644 > --- a/NEWS > +++ b/NEWS > @@ -123,6 +123,10 @@ v2.6.0 - xx xxx xxxx > disable it in OpenSSL. > - Add 'mtu_request' column to the Interface table. It can be used to > configure the MTU of non-internal ports. > + - Open vSwitch no longer automatically configures the internal > interfaces > + MTU to match the rest of the bridge. Please use external tools (or > + better, the 'mtu_request' column) to appropriately configure the MTU > on > + internal ports. > > > v2.5.0 - 26 Feb 2016 > diff --git a/debian/changelog b/debian/changelog > index d73e636..9958ef9 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -117,6 +117,10 @@ openvswitch (2.6.0-1) unstable; urgency=low > disable it in OpenSSL. > - Add 'mtu_request' column to the Interface table. It can be used to > configure the MTU of non-internal ports. > + - Open vSwitch no longer automatically configures the internal > interfaces > + MTU to match the rest of the bridge. Please use external tools (or > + better, the 'mtu_request' column) to appropriately configure the MTU > on > + internal ports. > > -- Open vSwitch team <dev@openvswitch.org> Mon, 15 Aug 2016 19:53:13 > -0700 > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 7f7813e..9f2c408 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -115,8 +115,6 @@ struct ofproto { > /* OpenFlow connections. */ > struct connmgr *connmgr; > > - int min_mtu; /* Current MTU of non-internal ports. > */ > - > /* Groups. */ > struct cmap groups; /* Contains "struct ofgroup"s. */ > uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each > type. */ > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 9d62b72..9fc87de 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -175,8 +175,6 @@ static void learned_cookies_flush(struct ofproto *, > struct ovs_list *dead_cookie > /* ofport. */ > static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); > static void ofport_destroy(struct ofport *, bool del); > -static inline bool ofport_is_internal(const struct ofproto *, > - const struct ofport *); > > static int update_port(struct ofproto *, const char *devname); > static int init_ports(struct ofproto *); > @@ -273,16 +271,12 @@ static void calc_duration(long long int start, long > long int now, > static uint64_t pick_datapath_id(const struct ofproto *); > static uint64_t pick_fallback_dpid(void); > static void ofproto_destroy__(struct ofproto *); > -static void update_mtu(struct ofproto *, struct ofport *); > -static void update_mtu_ofproto(struct ofproto *); > static void meter_delete(struct ofproto *, uint32_t first, uint32_t last); > static void meter_insert_rule(struct rule *); > > /* unixctl. */ > static void ofproto_unixctl_init(void); > > -static int find_min_mtu(struct ofproto *p); > - > /* All registered ofproto classes, in probe order. */ > static const struct ofproto_class **ofproto_classes; > static size_t n_ofproto_classes; > @@ -516,7 +510,6 @@ ofproto_create(const char *datapath_name, const char > *datapath_type, > hmap_init(&ofproto->learned_cookies); > ovs_list_init(&ofproto->expirable); > ofproto->connmgr = connmgr_create(ofproto, datapath_name, > datapath_name); > - ofproto->min_mtu = find_min_mtu(ofproto); > cmap_init(&ofproto->groups); > ovs_mutex_unlock(&ofproto_mutex); > ofproto->ogf.types = 0xf; > @@ -2392,8 +2385,6 @@ ofport_install(struct ofproto *p, > hash_ofp_port(ofport->ofp_port)); > shash_add(&p->port_by_name, netdev_name, ofport); > > - update_mtu(p, ofport); > - > /* Let the ofproto_class initialize its private data. */ > error = p->ofproto_class->port_construct(ofport); > if (error) { > @@ -2417,15 +2408,9 @@ error: > static void > ofport_remove(struct ofport *ofport) > { > - struct ofproto *p = ofport->ofproto; > - bool is_internal = ofport_is_internal(p, ofport); > - > connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp, > OFPPR_DELETE); > ofport_destroy(ofport, true); > - if (!is_internal) { > - update_mtu_ofproto(p); > - } > } > > /* If 'ofproto' contains an ofport named 'name', removes it from > 'ofproto' and > @@ -2621,8 +2606,6 @@ update_port(struct ofproto *ofproto, const char > *name) > ofport_modified(port, &pp); > } > > - update_mtu(ofproto, port); > - > /* Install the newly opened netdev in case it has changed. > * Don't close the old netdev yet in case port_modified has to > * remove a retained reference to it.*/ > @@ -2702,90 +2685,6 @@ init_ports(struct ofproto *p) > > return 0; > } > - > -static inline bool > -ofport_is_internal(const struct ofproto *p, const struct ofport *port) > -{ > - return !strcmp(netdev_get_type(port->netdev), > - ofproto_port_open_type(p->type, "internal")); > -} > - > -/* Find the minimum MTU of all non-datapath devices attached to 'p'. > - * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */ > -static int > -find_min_mtu(struct ofproto *p) > -{ > - struct ofport *ofport; > - int mtu = 0; > - > - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { > - struct netdev *netdev = ofport->netdev; > - int dev_mtu; > - > - /* Skip any internal ports, since that's what we're trying to > - * set. */ > - if (ofport_is_internal(p, ofport)) { > - continue; > - } > - > - if (netdev_get_mtu(netdev, &dev_mtu)) { > - continue; > - } > - if (!mtu || dev_mtu < mtu) { > - mtu = dev_mtu; > - } > - } > - > - return mtu ? mtu: ETH_PAYLOAD_MAX; > -} > - > -/* Update MTU of all datapath devices on 'p' to the minimum of the > - * non-datapath ports in event of 'port' added or changed. */ > -static void > -update_mtu(struct ofproto *p, struct ofport *port) > -{ > - struct netdev *netdev = port->netdev; > - int dev_mtu; > - > - if (netdev_get_mtu(netdev, &dev_mtu)) { > - port->mtu = 0; > - return; > - } > - if (ofport_is_internal(p, port)) { > - if (!netdev_set_mtu(port->netdev, p->min_mtu)) { > - dev_mtu = p->min_mtu; > - } > - port->mtu = dev_mtu; > - return; > - } > - > - port->mtu = dev_mtu; > - /* For non-internal port find new min mtu. */ > - update_mtu_ofproto(p); > -} > - > -static void > -update_mtu_ofproto(struct ofproto *p) > -{ > - /* For non-internal port find new min mtu. */ > - struct ofport *ofport; > - int old_min = p->min_mtu; > - > - p->min_mtu = find_min_mtu(p); > - if (p->min_mtu == old_min) { > - return; > - } > - > - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { > - struct netdev *netdev = ofport->netdev; > - > - if (ofport_is_internal(p, ofport)) { > - if (!netdev_set_mtu(netdev, p->min_mtu)) { > - ofport->mtu = p->min_mtu; > - } > - } > - } > -} > > static void > ofproto_rule_destroy__(struct rule *rule) > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 8e5fde6..250d99a 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -8870,10 +8870,7 @@ AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], > [dnl > > add_of_ports br0 1 > > -# Check that initial MTU is 1500 for 'br0' and 'p1'. > -AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], [dnl > -1500 > -]) > +# Check that initial MTU is 1500 for 'p1'. > AT_CHECK([ovs-vsctl get Interface p1 mtu], [0], [dnl > 1500 > ]) > @@ -8883,23 +8880,12 @@ AT_CHECK([ovs-vsctl set Interface p1 > mtu_request=1600]) > > # Check that the new MTU is applied > AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p1 mtu=1600]) > -# The internal port 'br0' should have the same MTU value as p1, becase > it's > -# the new bridge minimum. > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) > > AT_CHECK([ovs-vsctl del-port br0 p1]) > > -# When 'p1' is deleted, the internal port should return to the default MTU > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1500]) > - > # New port with 'mtu_request' in the same transaction. > AT_CHECK([ovs-vsctl add-port br0 p2 -- set int p2 type=dummy > mtu_request=1600]) > AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1600]) > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) > - > -# New internal port. The MTU should be updated even for a newly added > port. > -AT_CHECK([ovs-vsctl add-port br0 int1 -- set int int1 type=internal]) > -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface int1 mtu=1600]) > > OVS_VSWITCHD_STOP > AT_CLEANUP > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 69b5592..684df55 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2388,11 +2388,9 @@ > </p> > > <p> > - A client may change a non-internal interface MTU by filling in > - <ref column="mtu_request"/>. Internal interfaces MTU, instead, > is set > - by Open vSwitch to the minimum of non-internal interfaces MTU in > the > - bridge. In any case, Open vSwitch then reports in <ref > column="mtu"/> > - the currently configured value. > + A client may change an interface MTU by filling in > + <ref column="mtu_request"/>. Open vSwitch then reports in > + <ref column="mtu"/> the currently configured value. > </p> > > <column name="mtu"> > @@ -2411,7 +2409,7 @@ > type='{"type": "integer", "minInteger": 1}'> > <p> > Requested MTU (Maximum Transmission Unit) for the interface. A > client > - can fill this column to change the MTU of a non-internal > interface. > + can fill this column to change the MTU of an interface. > </p> > </column> > > -- > 2.9.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
On 31 August 2016 at 14:52, Daniele Di Proietto <diproiettod@vmware.com> wrote: > Open vSwitch controls the MTU of internal ports and sets it to the > minimum of physical ports MTU on the same bridge. > > Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") > made this more consistent. Now the MTU is always set, even when the > bridge doesn't have any physical ports (e.g. when it has only an > internal device and a tunnel). > > This latest change broke the system testsuite. Some tests need to > lower internal devices MTU because they use tunnels and they want to > work with a 1500 bytes underlay. > > There are other valid usecases where the user/controller needs to > configure the internal devices MTU (like mpls push actions, double vlans > or any overlay) and there's no way for Open vSwitch to know what the > appropriate MTU should be. > > Since in the general case Open vSwitch is not able to configure a > reasonable MTU for internal devices, this commit removes the feature > entirely. > > Now the user/controller is entirely responsible for configuring the MTU > of internal ports. Other hybrid solutions are possible (like not > touching the internal interfaces MTU, unless there's a physical device), > but they make the current MTU dependent on the previous state of the > system (if there was at some point a physical device the MTU would be > touched, but it wouldn't be possible to restore it). > > This change breaks compatibility with previous versions on Open vSwitch. > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> So I guess that prior to this patch, there is some sort of contract that the MTU of the internal interfaces are entirely managed by OVS, and it's set to the minimum of all non-internal devices, then the MTU of all other devices is managed by the user/controller. The trouble with this is cases where, for instance, the user wants to attach the local networking stack to some kind of overlay (tunnel, mpls, QinQ,...) and the user either cannot, or does not want to, change the underlay MTU to allow encapsulating full 1500B frames. In such a case, including the OVS testsuite today (which admittedly can be taught to be smarter), the current behaviour prohibits the user from providing that more detailed information for the bridge interface. Maybe it works in /some/ cases, but OVS will override the MTU if any sort of configuration is changed on that bridge. On the flip side, if a user wants to, for instance, configure OVS to provide switching from a physical NIC that supports jumbo frames, then today they simply set the MTU of the physical device and OVS takes care of the internal device. With this patch, the user/controller would be responsible for changing the internal device MTU as well. One thought I have is that it seems like maybe the cases that OVS is implicitly figuring out the MTU today are cases where the user already has to figure out MTU for another device, so maybe forcing them to also configure the internal port's MTU is not a huge burden. Is this the case? The other thought I have is that maybe users are more familiar with doing "ip link set dev breth0 mtu 1600" rather than using OVSDB to set up the MTU. I suspect that even if this is the case, it's simpler if OVS doesn't have any implicit logic to reconfigure the bridge device MTU rather than potentially override the user configuration. If the user wants to do things this way, they can, but it just won't persist. If they want it to persist, they can set the configuration in OVSDB.
On Wed, Aug 31, 2016 at 6:48 PM, Joe Stringer <joe@ovn.org> wrote: > On 31 August 2016 at 14:52, Daniele Di Proietto <diproiettod@vmware.com> > wrote: > > Open vSwitch controls the MTU of internal ports and sets it to the > > minimum of physical ports MTU on the same bridge. > > > > Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") > > made this more consistent. Now the MTU is always set, even when the > > bridge doesn't have any physical ports (e.g. when it has only an > > internal device and a tunnel). > > > > This latest change broke the system testsuite. Some tests need to > > lower internal devices MTU because they use tunnels and they want to > > work with a 1500 bytes underlay. > > > > There are other valid usecases where the user/controller needs to > > configure the internal devices MTU (like mpls push actions, double vlans > > or any overlay) and there's no way for Open vSwitch to know what the > > appropriate MTU should be. > > > > Since in the general case Open vSwitch is not able to configure a > > reasonable MTU for internal devices, this commit removes the feature > > entirely. > > > > Now the user/controller is entirely responsible for configuring the MTU > > of internal ports. Other hybrid solutions are possible (like not > > touching the internal interfaces MTU, unless there's a physical device), > > but they make the current MTU dependent on the previous state of the > > system (if there was at some point a physical device the MTU would be > > touched, but it wouldn't be possible to restore it). > > > > This change breaks compatibility with previous versions on Open vSwitch. > > > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > > So I guess that prior to this patch, there is some sort of contract > that the MTU of the internal interfaces are entirely managed by OVS, > and it's set to the minimum of all non-internal devices, then the MTU > of all other devices is managed by the user/controller. The trouble > with this is cases where, for instance, the user wants to attach the > local networking stack to some kind of overlay (tunnel, mpls, > QinQ,...) and the user either cannot, or does not want to, change the > underlay MTU to allow encapsulating full 1500B frames. In such a case, > including the OVS testsuite today (which admittedly can be taught to > be smarter), the current behaviour prohibits the user from providing > that more detailed information for the bridge interface. Maybe it > works in /some/ cases, but OVS will override the MTU if any sort of > configuration is changed on that bridge. > ip and mpls tunnels that traverse multiple transport nodes have another issue; PMTU could help in many cases, but not all. Even just local to the source transport node, figuring out the path taken, which can change requires added infra to get it right and optimal in an automatic, general and timely manner. I am not sure the added complexity is worth it, in the case of ovs. If OVS is in the business of controlling non-internal interface MTUs, as it is with the addition of the mtu_request column, then it seems useful to be able to also "manually" control the mtu of internal interfaces, when needed, as this patch proposes. > > On the flip side, if a user wants to, for instance, configure OVS to > provide switching from a physical NIC that supports jumbo frames, then > today they simply set the MTU of the physical device and OVS takes > care of the internal device. With this patch, the user/controller > would be responsible for changing the internal device MTU as well. > > One thought I have is that it seems like maybe the cases that OVS is > implicitly figuring out the MTU today are cases where the user already > has to figure out MTU for another device, so maybe forcing them to > also configure the internal port's MTU is not a huge burden. Is this > the case? > Dealing with internal interface MTU configuration seems like some added burden, including users understanding the internal interfaces relationships. I thought that the main benefit of the present auto-configuration of MTU for internal interfaces is simplicity. I am not sure completely removing the default behavior of auto-configuring internal interface MTUs to the minimum of non-internal interfaces is the way to go, even though it is not optimal in all cases; it seems like there might be some simplicity benefit for some users. Being able to override this default behavior, as proposed with this patch seems useful. > > The other thought I have is that maybe users are more familiar with > doing "ip link set dev breth0 mtu 1600" rather than using OVSDB to set > up the MTU. I suspect that even if this is the case, it's simpler if > OVS doesn't have any implicit logic to reconfigure the bridge device > MTU rather than potentially override the user configuration. If the > user wants to do things this way, they can, but it just won't persist. > If they want it to persist, they can set the configuration in OVSDB. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Hi Joe, Thanks for your comments replies inline On 31/08/2016 18:48, "Joe Stringer" <joe@ovn.org> wrote: >On 31 August 2016 at 14:52, Daniele Di Proietto <diproiettod@vmware.com> wrote: >> Open vSwitch controls the MTU of internal ports and sets it to the >> minimum of physical ports MTU on the same bridge. >> >> Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") >> made this more consistent. Now the MTU is always set, even when the >> bridge doesn't have any physical ports (e.g. when it has only an >> internal device and a tunnel). >> >> This latest change broke the system testsuite. Some tests need to >> lower internal devices MTU because they use tunnels and they want to >> work with a 1500 bytes underlay. >> >> There are other valid usecases where the user/controller needs to >> configure the internal devices MTU (like mpls push actions, double vlans >> or any overlay) and there's no way for Open vSwitch to know what the >> appropriate MTU should be. >> >> Since in the general case Open vSwitch is not able to configure a >> reasonable MTU for internal devices, this commit removes the feature >> entirely. >> >> Now the user/controller is entirely responsible for configuring the MTU >> of internal ports. Other hybrid solutions are possible (like not >> touching the internal interfaces MTU, unless there's a physical device), >> but they make the current MTU dependent on the previous state of the >> system (if there was at some point a physical device the MTU would be >> touched, but it wouldn't be possible to restore it). >> >> This change breaks compatibility with previous versions on Open vSwitch. >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >So I guess that prior to this patch, there is some sort of contract >that the MTU of the internal interfaces are entirely managed by OVS, >and it's set to the minimum of all non-internal devices, then the MTU >of all other devices is managed by the user/controller. The trouble >with this is cases where, for instance, the user wants to attach the >local networking stack to some kind of overlay (tunnel, mpls, >QinQ,...) and the user either cannot, or does not want to, change the >underlay MTU to allow encapsulating full 1500B frames. In such a case, >including the OVS testsuite today (which admittedly can be taught to >be smarter), the current behaviour prohibits the user from providing >that more detailed information for the bridge interface. Maybe it >works in /some/ cases, but OVS will override the MTU if any sort of >configuration is changed on that bridge. That's correct >On the flip side, if a user wants to, for instance, configure OVS to >provide switching from a physical NIC that supports jumbo frames, then >today they simply set the MTU of the physical device and OVS takes >care of the internal device. With this patch, the user/controller >would be responsible for changing the internal device MTU as well. Yes >One thought I have is that it seems like maybe the cases that OVS is >implicitly figuring out the MTU today are cases where the user already >has to figure out MTU for another device, so maybe forcing them to >also configure the internal port's MTU is not a huge burden. Is this >the case? I think so >The other thought I have is that maybe users are more familiar with >doing "ip link set dev breth0 mtu 1600" rather than using OVSDB to set >up the MTU. I suspect that even if this is the case, it's simpler if >OVS doesn't have any implicit logic to reconfigure the bridge device >MTU rather than potentially override the user configuration. If the >user wants to do things this way, they can, but it just won't persist. >If they want it to persist, they can set the configuration in OVSDB. That's exactly what's happening after this patch.
Hi Darrel, thanks for your comments Replies inline On 31/08/2016 21:02, "Darrell Ball" <dlu998@gmail.com> wrote: > > >On Wed, Aug 31, 2016 at 6:48 PM, Joe Stringer ><joe@ovn.org> wrote: > >On 31 August 2016 at 14:52, Daniele Di Proietto <diproiettod@vmware.com> wrote: >> Open vSwitch controls the MTU of internal ports and sets it to the >> minimum of physical ports MTU on the same bridge. >> >> Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") >> made this more consistent. Now the MTU is always set, even when the >> bridge doesn't have any physical ports (e.g. when it has only an >> internal device and a tunnel). >> >> This latest change broke the system testsuite. Some tests need to >> lower internal devices MTU because they use tunnels and they want to >> work with a 1500 bytes underlay. >> >> There are other valid usecases where the user/controller needs to >> configure the internal devices MTU (like mpls push actions, double vlans >> or any overlay) and there's no way for Open vSwitch to know what the >> appropriate MTU should be. >> >> Since in the general case Open vSwitch is not able to configure a >> reasonable MTU for internal devices, this commit removes the feature >> entirely. >> >> Now the user/controller is entirely responsible for configuring the MTU >> of internal ports. Other hybrid solutions are possible (like not >> touching the internal interfaces MTU, unless there's a physical device), >> but they make the current MTU dependent on the previous state of the >> system (if there was at some point a physical device the MTU would be >> touched, but it wouldn't be possible to restore it). >> >> This change breaks compatibility with previous versions on Open vSwitch. >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > > > >So I guess that prior to this patch, there is some sort of contract >that the MTU of the internal interfaces are entirely managed by OVS, >and it's set to the minimum of all non-internal devices, then the MTU >of all other devices is managed by the user/controller. The trouble >with this is cases where, for instance, the user wants to attach the >local networking stack to some kind of overlay (tunnel, mpls, >QinQ,...) and the user either cannot, or does not want to, change the >underlay MTU to allow encapsulating full 1500B frames. In such a case, >including the OVS testsuite today (which admittedly can be taught to >be smarter), the current behaviour prohibits the user from providing >that more detailed information for the bridge interface. Maybe it >works in /some/ cases, but OVS will override the MTU if any sort of >configuration is changed on that bridge. > > > > >ip and mpls tunnels that traverse multiple transport nodes have another issue; >PMTU could help in many cases, but not all. >Even just local to the source transport node, figuring out the path taken, which >can change requires added infra to get it right and optimal in an automatic, general >and timely manner. I am not sure the added complexity is worth it, >in the case of ovs. > > >If OVS is in the business of controlling non-internal interface MTUs, as it >is with the addition of the mtu_request column, then it seems useful to be able >to also "manually" control the mtu of internal interfaces, when needed, as this >patch proposes. > > > > > >On the flip side, if a user wants to, for instance, configure OVS to >provide switching from a physical NIC that supports jumbo frames, then >today they simply set the MTU of the physical device and OVS takes >care of the internal device. With this patch, the user/controller >would be responsible for changing the internal device MTU as well. > >One thought I have is that it seems like maybe the cases that OVS is >implicitly figuring out the MTU today are cases where the user already >has to figure out MTU for another device, so maybe forcing them to >also configure the internal port's MTU is not a huge burden. Is this >the case? > > > > >Dealing with internal interface MTU configuration seems like some added >burden, including users understanding the internal interfaces relationships. >I thought that the main benefit of the present auto-configuration >of MTU for internal interfaces is simplicity. > > >I am not sure completely removing the default behavior of auto-configuring >internal interface MTUs to the minimum of non-internal interfaces is the >way to go, even though it is not optimal in all cases; it seems like there >might be some simplicity benefit for some users. >Being able to override this default behavior, as proposed with this patch >seems useful. This patch completely removes the default behavior. I think it's the cleanest solution, even though I'm worried about compatibility. >The other thought I have is that maybe users are more familiar with >doing "ip link set dev breth0 mtu 1600" rather than using OVSDB to set >up the MTU. I suspect that even if this is the case, it's simpler if >OVS doesn't have any implicit logic to reconfigure the bridge device >MTU rather than potentially override the user configuration. If the >user wants to do things this way, they can, but it just won't persist. >If they want it to persist, they can set the configuration in OVSDB. >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev > > > > > > > >
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
On 31/08/2016 21:02, "Darrell Ball" <dlu998@gmail.com> wrote:
>
>
>On Wed, Aug 31, 2016 at 6:48 PM, Joe Stringer
><joe@ovn.org> wrote:
>
>On 31 August 2016 at 14:52, Daniele Di Proietto <diproiettod@vmware.com> wrote:
>> Open vSwitch controls the MTU of internal ports and sets it to the
>> minimum of physical ports MTU on the same bridge.
>>
>> Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.")
>> made this more consistent. Now the MTU is always set, even when the
>> bridge doesn't have any physical ports (e.g. when it has only an
>> internal device and a tunnel).
>>
>> This latest change broke the system testsuite. Some tests need to
>> lower internal devices MTU because they use tunnels and they want to
>> work with a 1500 bytes underlay.
>>
>> There are other valid usecases where the user/controller needs to
>> configure the internal devices MTU (like mpls push actions, double vlans
>> or any overlay) and there's no way for Open vSwitch to know what the
>> appropriate MTU should be.
>>
>> Since in the general case Open vSwitch is not able to configure a
>> reasonable MTU for internal devices, this commit removes the feature
>> entirely.
>>
>> Now the user/controller is entirely responsible for configuring the MTU
>> of internal ports. Other hybrid solutions are possible (like not
>> touching the internal interfaces MTU, unless there's a physical device),
>> but they make the current MTU dependent on the previous state of the
>> system (if there was at some point a physical device the MTU would be
>> touched, but it wouldn't be possible to restore it).
>>
>> This change breaks compatibility with previous versions on Open vSwitch.
>>
>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
>
>
>
>So I guess that prior to this patch, there is some sort of contract
>that the MTU of the internal interfaces are entirely managed by OVS,
>and it's set to the minimum of all non-internal devices, then the MTU
>of all other devices is managed by the user/controller. The trouble
>with this is cases where, for instance, the user wants to attach the
>local networking stack to some kind of overlay (tunnel, mpls,
>QinQ,...) and the user either cannot, or does not want to, change the
>underlay MTU to allow encapsulating full 1500B frames. In such a case,
>including the OVS testsuite today (which admittedly can be taught to
>be smarter), the current behaviour prohibits the user from providing
>that more detailed information for the bridge interface. Maybe it
>works in /some/ cases, but OVS will override the MTU if any sort of
>configuration is changed on that bridge.
>
>
>
>
>ip and mpls tunnels that traverse multiple transport nodes have another issue;
>PMTU could help in many cases, but not all.
>Even just local to the source transport node, figuring out the path taken, which
>can change requires added infra to get it right and optimal in an automatic, general
>and timely manner. I am not sure the added complexity is worth it,
>in the case of ovs.
>
>
>If OVS is in the business of controlling non-internal interface MTUs, as it
>is with the addition of the mtu_request column, then it seems useful to be able
>to also "manually" control the mtu of internal interfaces, when needed, as this
>patch proposes.
>
>
>
>
>
>On the flip side, if a user wants to, for instance, configure OVS to
>provide switching from a physical NIC that supports jumbo frames, then
>today they simply set the MTU of the physical device and OVS takes
>care of the internal device. With this patch, the user/controller
>would be responsible for changing the internal device MTU as well.
>
>One thought I have is that it seems like maybe the cases that OVS is
>implicitly figuring out the MTU today are cases where the user already
>has to figure out MTU for another device, so maybe forcing them to
>also configure the internal port's MTU is not a huge burden. Is this
>the case?
>
>
>
>
>Dealing with internal interface MTU configuration seems like some added
>burden, including users understanding the internal interfaces relationships.
>I thought that the main benefit of the present auto-configuration
>of MTU for internal interfaces is simplicity.
>
>
>I am not sure completely removing the default behavior of auto-configuring
>internal interface MTUs to the minimum of non-internal interfaces is the
>way to go, even though it is not optimal in all cases; it seems like there
>might be some simplicity benefit for some users.
>Being able to override this default behavior, as proposed with this patch
>seems useful.
>
>
>
>
>
>The other thought I have is that maybe users are more familiar with
>doing "ip link set dev breth0 mtu 1600" rather than using OVSDB to set
>up the MTU. I suspect that even if this is the case, it's simpler if
>OVS doesn't have any implicit logic to reconfigure the bridge device
>MTU rather than potentially override the user configuration. If the
>user wants to do things this way, they can, but it just won't persist.
>If they want it to persist, they can set the configuration in OVSDB.
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
>
>
>
>
>
>
>
>
On Thu, Sep 1, 2016 at 10:17 AM, Daniele Di Proietto <diproiettod@vmware.com> 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. This is not the first time that this has come up, for example, a couple months back: http://openvswitch.org/pipermail/dev/2016-June/073190.html The existing behavior is a legacy of trying to behave more similarly to the Linux bridge a long time ago when users were trying to easily swap between it and OVS. However, in practice the two aren't that similar since OVS allows arbitrary partitioning of bridges through flows in ways that could effectively make the pieces into separate networks with different MTUs, imposing tags, etc. At the time, the bridge didn't even have native support for VLANs, so none of this could have been an issue. If we could go back, I wish that we hadn't carried this behavior into OVS. For people adjusting MTUs, it probably wouldn't have been a big deal to adjust the OVS bridge MTU as well - the same as what is necessary for VMs. However, I think that simply removing it at this point will break some existing setups that I'm pretty sure exist in the real world. If you have just a simple setup with an OVS bridge and a physical Ethernet device with a non-default MTU, you are probably relying on this automatic MTU adjustment. So overall, I would be happy to move to the new model but I think this patch is probably too aggressive in switching for the time being. I think we need to keep the existing behavior as the default with a switch and then maybe add some big deprecation warnings so we can eventually change the default.
On 01/09/2016 11:07, "Jesse Gross" <jesse@kernel.org> wrote: >On Thu, Sep 1, 2016 at 10:17 AM, Daniele Di Proietto ><diproiettod@vmware.com> 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. > >This is not the first time that this has come up, for example, a >couple months back: >http://openvswitch.org/pipermail/dev/2016-June/073190.html > >The existing behavior is a legacy of trying to behave more similarly >to the Linux bridge a long time ago when users were trying to easily >swap between it and OVS. However, in practice the two aren't that >similar since OVS allows arbitrary partitioning of bridges through >flows in ways that could effectively make the pieces into separate >networks with different MTUs, imposing tags, etc. At the time, the >bridge didn't even have native support for VLANs, so none of this >could have been an issue. > >If we could go back, I wish that we hadn't carried this behavior into >OVS. For people adjusting MTUs, it probably wouldn't have been a big >deal to adjust the OVS bridge MTU as well - the same as what is >necessary for VMs. However, I think that simply removing it at this >point will break some existing setups that I'm pretty sure exist in >the real world. If you have just a simple setup with an OVS bridge and >a physical Ethernet device with a non-default MTU, you are probably >relying on this automatic MTU adjustment. Thanks for the detailed explanation Jesse, it makes more sense now. >So overall, I would be happy to move to the new model but I think this >patch is probably too aggressive in switching for the time being. I >think we need to keep the existing behavior as the default with a >switch and then maybe add some big deprecation warnings so we can >eventually change the default. You're right, it's probably not a good idea to change this behavior now. Darrell suggested another approach, which at this point I think it's the best way to go. We keep the old behavior, even if it was a little inconsistent, for backwards compatibility. Since in this release we also introduced 'mtu_request', we use that to override the old behavior for internal ports. Users that are really interested in configuring their internal port MTU can use that instead. I don't like that mtu_request behaves differently than standard linux tools, but I think this is a good balance between compatibility and flexibility. I've sent a series here: http://openvswitch.org/pipermail/dev/2016-September/079078.html http://openvswitch.org/pipermail/dev/2016-September/079079.html Thanks, Daniele
diff --git a/NEWS b/NEWS index 1439151..590a7b9 100644 --- a/NEWS +++ b/NEWS @@ -123,6 +123,10 @@ v2.6.0 - xx xxx xxxx disable it in OpenSSL. - Add 'mtu_request' column to the Interface table. It can be used to configure the MTU of non-internal ports. + - Open vSwitch no longer automatically configures the internal interfaces + MTU to match the rest of the bridge. Please use external tools (or + better, the 'mtu_request' column) to appropriately configure the MTU on + internal ports. v2.5.0 - 26 Feb 2016 diff --git a/debian/changelog b/debian/changelog index d73e636..9958ef9 100644 --- a/debian/changelog +++ b/debian/changelog @@ -117,6 +117,10 @@ openvswitch (2.6.0-1) unstable; urgency=low disable it in OpenSSL. - Add 'mtu_request' column to the Interface table. It can be used to configure the MTU of non-internal ports. + - Open vSwitch no longer automatically configures the internal interfaces + MTU to match the rest of the bridge. Please use external tools (or + better, the 'mtu_request' column) to appropriately configure the MTU on + internal ports. -- Open vSwitch team <dev@openvswitch.org> Mon, 15 Aug 2016 19:53:13 -0700 diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 7f7813e..9f2c408 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -115,8 +115,6 @@ struct ofproto { /* OpenFlow connections. */ struct connmgr *connmgr; - int min_mtu; /* Current MTU of non-internal ports. */ - /* Groups. */ struct cmap groups; /* Contains "struct ofgroup"s. */ uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each type. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 9d62b72..9fc87de 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -175,8 +175,6 @@ static void learned_cookies_flush(struct ofproto *, struct ovs_list *dead_cookie /* ofport. */ static void ofport_destroy__(struct ofport *) OVS_EXCLUDED(ofproto_mutex); static void ofport_destroy(struct ofport *, bool del); -static inline bool ofport_is_internal(const struct ofproto *, - const struct ofport *); static int update_port(struct ofproto *, const char *devname); static int init_ports(struct ofproto *); @@ -273,16 +271,12 @@ static void calc_duration(long long int start, long long int now, static uint64_t pick_datapath_id(const struct ofproto *); static uint64_t pick_fallback_dpid(void); static void ofproto_destroy__(struct ofproto *); -static void update_mtu(struct ofproto *, struct ofport *); -static void update_mtu_ofproto(struct ofproto *); static void meter_delete(struct ofproto *, uint32_t first, uint32_t last); static void meter_insert_rule(struct rule *); /* unixctl. */ static void ofproto_unixctl_init(void); -static int find_min_mtu(struct ofproto *p); - /* All registered ofproto classes, in probe order. */ static const struct ofproto_class **ofproto_classes; static size_t n_ofproto_classes; @@ -516,7 +510,6 @@ ofproto_create(const char *datapath_name, const char *datapath_type, hmap_init(&ofproto->learned_cookies); ovs_list_init(&ofproto->expirable); ofproto->connmgr = connmgr_create(ofproto, datapath_name, datapath_name); - ofproto->min_mtu = find_min_mtu(ofproto); cmap_init(&ofproto->groups); ovs_mutex_unlock(&ofproto_mutex); ofproto->ogf.types = 0xf; @@ -2392,8 +2385,6 @@ ofport_install(struct ofproto *p, hash_ofp_port(ofport->ofp_port)); shash_add(&p->port_by_name, netdev_name, ofport); - update_mtu(p, ofport); - /* Let the ofproto_class initialize its private data. */ error = p->ofproto_class->port_construct(ofport); if (error) { @@ -2417,15 +2408,9 @@ error: static void ofport_remove(struct ofport *ofport) { - struct ofproto *p = ofport->ofproto; - bool is_internal = ofport_is_internal(p, ofport); - connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp, OFPPR_DELETE); ofport_destroy(ofport, true); - if (!is_internal) { - update_mtu_ofproto(p); - } } /* If 'ofproto' contains an ofport named 'name', removes it from 'ofproto' and @@ -2621,8 +2606,6 @@ update_port(struct ofproto *ofproto, const char *name) ofport_modified(port, &pp); } - update_mtu(ofproto, port); - /* Install the newly opened netdev in case it has changed. * Don't close the old netdev yet in case port_modified has to * remove a retained reference to it.*/ @@ -2702,90 +2685,6 @@ init_ports(struct ofproto *p) return 0; } - -static inline bool -ofport_is_internal(const struct ofproto *p, const struct ofport *port) -{ - return !strcmp(netdev_get_type(port->netdev), - ofproto_port_open_type(p->type, "internal")); -} - -/* Find the minimum MTU of all non-datapath devices attached to 'p'. - * Returns ETH_PAYLOAD_MAX or the minimum of the ports. */ -static int -find_min_mtu(struct ofproto *p) -{ - struct ofport *ofport; - int mtu = 0; - - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { - struct netdev *netdev = ofport->netdev; - int dev_mtu; - - /* Skip any internal ports, since that's what we're trying to - * set. */ - if (ofport_is_internal(p, ofport)) { - continue; - } - - if (netdev_get_mtu(netdev, &dev_mtu)) { - continue; - } - if (!mtu || dev_mtu < mtu) { - mtu = dev_mtu; - } - } - - return mtu ? mtu: ETH_PAYLOAD_MAX; -} - -/* Update MTU of all datapath devices on 'p' to the minimum of the - * non-datapath ports in event of 'port' added or changed. */ -static void -update_mtu(struct ofproto *p, struct ofport *port) -{ - struct netdev *netdev = port->netdev; - int dev_mtu; - - if (netdev_get_mtu(netdev, &dev_mtu)) { - port->mtu = 0; - return; - } - if (ofport_is_internal(p, port)) { - if (!netdev_set_mtu(port->netdev, p->min_mtu)) { - dev_mtu = p->min_mtu; - } - port->mtu = dev_mtu; - return; - } - - port->mtu = dev_mtu; - /* For non-internal port find new min mtu. */ - update_mtu_ofproto(p); -} - -static void -update_mtu_ofproto(struct ofproto *p) -{ - /* For non-internal port find new min mtu. */ - struct ofport *ofport; - int old_min = p->min_mtu; - - p->min_mtu = find_min_mtu(p); - if (p->min_mtu == old_min) { - return; - } - - HMAP_FOR_EACH (ofport, hmap_node, &p->ports) { - struct netdev *netdev = ofport->netdev; - - if (ofport_is_internal(p, ofport)) { - if (!netdev_set_mtu(netdev, p->min_mtu)) { - ofport->mtu = p->min_mtu; - } - } - } -} static void ofproto_rule_destroy__(struct rule *rule) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 8e5fde6..250d99a 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -8870,10 +8870,7 @@ AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], [dnl add_of_ports br0 1 -# Check that initial MTU is 1500 for 'br0' and 'p1'. -AT_CHECK([ovs-vsctl get Interface br0 mtu], [0], [dnl -1500 -]) +# Check that initial MTU is 1500 for 'p1'. AT_CHECK([ovs-vsctl get Interface p1 mtu], [0], [dnl 1500 ]) @@ -8883,23 +8880,12 @@ AT_CHECK([ovs-vsctl set Interface p1 mtu_request=1600]) # Check that the new MTU is applied AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p1 mtu=1600]) -# The internal port 'br0' should have the same MTU value as p1, becase it's -# the new bridge minimum. -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) AT_CHECK([ovs-vsctl del-port br0 p1]) -# When 'p1' is deleted, the internal port should return to the default MTU -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1500]) - # New port with 'mtu_request' in the same transaction. AT_CHECK([ovs-vsctl add-port br0 p2 -- set int p2 type=dummy mtu_request=1600]) AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface p2 mtu=1600]) -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface br0 mtu=1600]) - -# New internal port. The MTU should be updated even for a newly added port. -AT_CHECK([ovs-vsctl add-port br0 int1 -- set int int1 type=internal]) -AT_CHECK([ovs-vsctl --timeout=10 wait-until Interface int1 mtu=1600]) OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 69b5592..684df55 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2388,11 +2388,9 @@ </p> <p> - A client may change a non-internal interface MTU by filling in - <ref column="mtu_request"/>. Internal interfaces MTU, instead, is set - by Open vSwitch to the minimum of non-internal interfaces MTU in the - bridge. In any case, Open vSwitch then reports in <ref column="mtu"/> - the currently configured value. + A client may change an interface MTU by filling in + <ref column="mtu_request"/>. Open vSwitch then reports in + <ref column="mtu"/> the currently configured value. </p> <column name="mtu"> @@ -2411,7 +2409,7 @@ type='{"type": "integer", "minInteger": 1}'> <p> Requested MTU (Maximum Transmission Unit) for the interface. A client - can fill this column to change the MTU of a non-internal interface. + can fill this column to change the MTU of an interface. </p> </column>
Open vSwitch controls the MTU of internal ports and sets it to the minimum of physical ports MTU on the same bridge. Commit 47bf118665a3("ofproto: Always set MTU for new internal ports.") made this more consistent. Now the MTU is always set, even when the bridge doesn't have any physical ports (e.g. when it has only an internal device and a tunnel). This latest change broke the system testsuite. Some tests need to lower internal devices MTU because they use tunnels and they want to work with a 1500 bytes underlay. There are other valid usecases where the user/controller needs to configure the internal devices MTU (like mpls push actions, double vlans or any overlay) and there's no way for Open vSwitch to know what the appropriate MTU should be. Since in the general case Open vSwitch is not able to configure a reasonable MTU for internal devices, this commit removes the feature entirely. Now the user/controller is entirely responsible for configuring the MTU of internal ports. Other hybrid solutions are possible (like not touching the internal interfaces MTU, unless there's a physical device), but they make the current MTU dependent on the previous state of the system (if there was at some point a physical device the MTU would be touched, but it wouldn't be possible to restore it). This change breaks compatibility with previous versions on Open vSwitch. Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- I realize that it's not nice to introduce a backwards incompatible change, especially so late in the release. I considered other possible solutions, but they all introduce undesirable behavior. If it's not acceptable to break compatibility, I can get away with alternative approaches. --- NEWS | 4 ++ debian/changelog | 4 ++ ofproto/ofproto-provider.h | 2 - ofproto/ofproto.c | 101 --------------------------------------------- tests/ofproto-dpif.at | 16 +------ vswitchd/vswitch.xml | 10 ++--- 6 files changed, 13 insertions(+), 124 deletions(-)