Merge lp:~mpontillo/maas/fix-parent-mtu-less-than-child--bug-1662948 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Consider child interface MTUs when deciding a parent interface's effective MTU.

[Mike Pontillo (mpontillo)]

Note: I moved all the MTU-related test cases into their own suite, InterfaceMTUTest, so that future developers working with MTU-related issues have an easy way to run all the interface-model-related tests.

[Lee Trager (ltrager)]

Should src/maasserver/forms/interface.py be updated to raise a validation error when this occurs, or should the parent MUTS always automatically supersede the child?

review: Needs Information

[Mike Pontillo (mpontillo)]

I think adding a ValidationError here would just make it hard on the user, since there are so many different ways to determine the effective MTU.

According to the bug reports on this issue (there were a couple duplicates, too), if you try to deploy a node where the parent MTU is less than its children, the deployment will fail and the interfaces will not come online.

[Lee Trager (ltrager)]

Sounds resonable.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/fix-parent-mtu-less-than-child--bug-1662948 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Get:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [102 kB]
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Sources [237 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [499 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/main Translation-en [200 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates/universe amd64 Packages [448 kB]
Fetched 1,690 kB in 0s (2,249 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql psmisc pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is al...

=== modified file 'src/maasserver/models/interface.py'
--- src/maasserver/models/interface.py	2017-03-24 22:52:14 +0000
+++ src/maasserver/models/interface.py	2017-03-28 03:35:50 +0000
@@ -597,6 +597,13 @@
             # MTU set and it is disconnected.
             from maasserver.models.vlan import DEFAULT_MTU
             mtu = DEFAULT_MTU
+        children = self.get_successors()
+        # Check if any child interface has a greater MTU. It is an invalid
+        # configuration for the parent MTU to be smaller. (LP: #1662948)
+        for child in children:
+            child_mtu = child.get_effective_mtu()
+            if mtu < child_mtu:
+                mtu = child_mtu
         return mtu
 
     def get_links(self):

=== modified file 'src/maasserver/models/tests/test_interface.py'
--- src/maasserver/models/tests/test_interface.py	2017-03-24 22:57:02 +0000
+++ src/maasserver/models/tests/test_interface.py	2017-03-28 03:35:50 +0000
@@ -913,27 +913,6 @@
             alloc_type=IPADDRESS_TYPE.AUTO, interface=nic1)
         self.assertTrue(nic1.is_configured())
 
-    def test_get_effective_mtu_returns_interface_mtu(self):
-        nic1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        nic_mtu = random.randint(552, 4096)
-        nic1.params = {
-            "mtu": nic_mtu
-        }
-        nic1.save()
-        self.assertEqual(nic_mtu, nic1.get_effective_mtu())
-
-    def test_get_effective_mtu_returns_vlan_mtu(self):
-        nic1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        vlan_mtu = random.randint(552, 4096)
-        nic1.vlan.mtu = vlan_mtu
-        nic1.vlan.save()
-        self.assertEqual(vlan_mtu, nic1.get_effective_mtu())
-
-    def test_get_effective_mtu_returns_default_mtu(self):
-        nic1 = factory.make_Interface(
-            INTERFACE_TYPE.PHYSICAL, disconnected=True)
-        self.assertEqual(DEFAULT_MTU, nic1.get_effective_mtu())
-
     def test_get_links_returns_links_for_each_type(self):
         interface = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         links = []
@@ -1372,6 +1351,80 @@
         self.assertFalse(reload_object(interface).enabled)
 
 
+class InterfaceMTUTest(MAASServerTestCase):
+
+    def test_get_effective_mtu_returns_default_mtu(self):
+        nic1 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, disconnected=True)
+        self.assertEqual(DEFAULT_MTU, nic1.get_effective_mtu())
+
+    def test_get_effective_mtu_returns_interface_mtu(self):
+        nic1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        nic_mtu = random.randint(552, 9100)
+        nic1.params = {
+            "mtu": nic_mtu
+        }
+        nic1.save()
+        self.assertEqual(nic_mtu, nic1.get_effective_mtu())
+
+    def test_get_effective_mtu_returns_vlan_mtu(self):
+        nic1 = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        vlan_mtu = random.randint(552, 9100)
+        nic1.vlan.mtu = vlan_mtu
+        nic1.vlan.save()
+        self.assertEqual(vlan_mtu, nic1.get_effective_mtu())
+
+    def test_get_effective_mtu_considers_jumbo_vlan_children(self):
+        fabric = factory.make_Fabric()
+        vlan = factory.make_VLAN(fabric=fabric)
+        eth0 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric.get_default_vlan())
+        eth0_vlan = factory.make_Interface(
+            iftype=INTERFACE_TYPE.VLAN, vlan=vlan, parents=[eth0])
+        vlan_mtu = random.randint(DEFAULT_MTU + 1, 9100)
+        eth0_vlan.vlan.mtu = vlan_mtu
+        eth0_vlan.vlan.save()
+        self.assertEqual(vlan_mtu, eth0.get_effective_mtu())
+
+    def test_get_effective_mtu_returns_highest_vlan_mtu(self):
+        fabric = factory.make_Fabric()
+        vlan1 = factory.make_VLAN(fabric=fabric)
+        vlan2 = factory.make_VLAN(fabric=fabric)
+        eth0 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=fabric.get_default_vlan())
+        eth0_vlan1 = factory.make_Interface(
+            iftype=INTERFACE_TYPE.VLAN, vlan=vlan1, parents=[eth0])
+        eth0_vlan2 = factory.make_Interface(
+            iftype=INTERFACE_TYPE.VLAN, vlan=vlan2, parents=[eth0])
+        eth0_vlan1.vlan.mtu = random.randint(1000, 1999)
+        eth0_vlan1.vlan.save()
+        eth0_vlan2.vlan.mtu = random.randint(2000, 2999)
+        eth0_vlan2.vlan.save()
+        self.assertEqual(eth0_vlan2.vlan.mtu, eth0.get_effective_mtu())
+
+    def test__creates_acquired_bridge_copies_mtu(self):
+        mtu = random.randint(600, 9100)
+        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
+        parent.params = {'mtu': mtu}
+        parent.save()
+        bridge = parent.create_acquired_bridge()
+        self.assertThat(bridge, MatchesStructure(
+            name=Equals("br-%s" % parent.get_name()),
+            mac_address=Equals(parent.mac_address),
+            node=Equals(parent.node),
+            vlan=Equals(parent.vlan),
+            enabled=Equals(True),
+            acquired=Equals(True),
+            params=MatchesDict({
+                "bridge_stp": Equals(False),
+                "bridge_fd": Equals(15),
+                "mtu": Equals(mtu),
+            })
+        ))
+        self.assertEquals(
+            [parent.id], [p.id for p in bridge.parents.all()])
+
+
 class VLANInterfaceTest(MAASServerTestCase):
 
     def test_vlan_has_generated_name(self):
@@ -2968,28 +3021,6 @@
         self.assertEquals(
             [parent.id], [p.id for p in bridge.parents.all()])
 
-    def test__creates_acquired_bridge_copies_mtu(self):
-        mtu = random.randint(600, 1000)
-        parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
-        parent.params = {'mtu': mtu}
-        parent.save()
-        bridge = parent.create_acquired_bridge()
-        self.assertThat(bridge, MatchesStructure(
-            name=Equals("br-%s" % parent.get_name()),
-            mac_address=Equals(parent.mac_address),
-            node=Equals(parent.node),
-            vlan=Equals(parent.vlan),
-            enabled=Equals(True),
-            acquired=Equals(True),
-            params=MatchesDict({
-                "bridge_stp": Equals(False),
-                "bridge_fd": Equals(15),
-                "mtu": Equals(mtu),
-            })
-        ))
-        self.assertEquals(
-            [parent.id], [p.id for p in bridge.parents.all()])
-
     def test__creates_acquired_bridge_moves_links_from_parent_to_bridge(self):
         parent = factory.make_Interface(INTERFACE_TYPE.PHYSICAL)
         auto_ip = factory.make_StaticIPAddress(