Merge lp:~mpontillo/maas/bug-1553617-bonds-with-no-parents into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Fix bug #1553617: rack registration fails when bond interfaces are present.

 * When parsing '/sbin/ip addr' and gathering driver information, ignore
   bonds with no backing interfaces.
 * After parsing '/sbin/ip addr', cross-reference data from
   /proc/net/bonding/* and update interface MACs with the original
   hardware MAC. (After a bond interface is added, the interface MAC is
   updated to be the same as the bond MAC, which was throwing off the data
   MAAS gathered.)

[Mike Pontillo (mpontillo)]

Updated this branch to fix the second issue blocking rack registration.

[Blake Rouse (blake-rouse)]

Looks good. Well sleuthed! Just one question on the filter on bonds that don't have any members.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/bug-1553617-bonds-with-no-parents into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://security.ubuntu.com/ubuntu xenial-security InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease [95.8 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main Sources [1,109 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,443 kB]
Fetched 2,648 kB in 0s (2,703 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind bind9 bind9utils build-essential 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 libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-bson python3-convoy python3-coverage python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-mock python3-netaddr python3-netifaces 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-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-1ubuntu1).
archdetect-deb is already the newest version (1.114ubuntu3).
authbind is already the newest version (2.1.1+nmu1).
bind9 is already the newest version (1:9.10.3.dfsg.P2-5).
bind9utils is already the newest version (1:9.10.3.dfsg.P2-5).
build-essential is already the newest version (12.1ubuntu2).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu2).
dh-apport is already the newest version (2.20-0ubuntu3).
dh-systemd is already the newest version (1.29ubuntu1).
distro-info is already the newest version (0.14build1).
dnsutils is already the newest version (1:9.10.3.dfsg.P2-5).
firefox is already the newest version (45.0+build2-0ubuntu1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.0-1).
isc-dhcp-common is already the newest version ...

=== modified file 'src/provisioningserver/utils/ipaddr.py'
--- src/provisioningserver/utils/ipaddr.py	2016-03-03 20:40:10 +0000
+++ src/provisioningserver/utils/ipaddr.py	2016-03-07 19:23:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Utility to parse 'ip addr [show]'.
@@ -221,7 +221,7 @@
 
 def get_interface_type(
         ifname, sys_class_net="/sys/class/net",
-        proc_net_vlan='/proc/net/vlan'):
+        proc_net="/proc/net"):
     """Heuristic to return the type of the given interface.
 
     The given interface must be able to be found in /sys/class/net/ifname.
@@ -270,7 +270,7 @@
         bond_dir = os.path.join(sys_path, 'bonding')
         if os.path.isdir(bond_dir):
             return 'ethernet.bond'
-        if os.path.isfile('%s/%s' % (proc_net_vlan, ifname)):
+        if os.path.isfile(os.path.join(proc_net, "vlan", ifname)):
             return 'ethernet.vlan'
         device_path = os.path.join(sys_path, 'device')
         if os.path.islink(device_path):
@@ -291,22 +291,88 @@
         return 'unknown-%d' % iftype
 
 
-def annotate_with_driver_information(interfaces):
+def _parse_proc_net_bonding(file):
+    """Parse the given file, which must be a path to a file in the format
+    that is used for file in `/proc/net/bonding/<interface>`.
+
+    Returns a dictionary mapping each interface name found in the file to
+    its original MAC address.
+    """
+    interfaces = {}
+    current_iface = None
+    with open(file) as f:
+        for line in f.readlines():
+            line = line.strip()
+            slave_iface = line.split("Slave Interface: ")
+            if len(slave_iface) == 2:
+                current_iface = slave_iface[1]
+            hw_addr = line.split("Permanent HW addr: ")
+            if len(hw_addr) == 2:
+                interfaces[current_iface] = hw_addr[1]
+    return interfaces
+
+
+def annotate_with_proc_net_bonding_original_macs(
+        interfaces, proc_net="/proc/net"):
+    """Repairs the MAC addresses of bond members in the specified structure.
+
+    Given the specified interfaces structure, uses the data in
+    `/proc/net/bonding/*` to determine if any of the interfaces
+    in the structure are bond members. If so, modifies their MAC address,
+    setting it back to the original hardware MAC. (When an interface is added
+    to a bond, its MAC address is set to the bond MAC, and subsequently
+    reported in commands like "ip addr".)
+    """
+    proc_net_bonding = os.path.join(proc_net, "bonding")
+    if os.path.isdir(proc_net_bonding):
+        bonds = os.listdir(proc_net_bonding)
+        for bond in bonds:
+            parent_macs = _parse_proc_net_bonding(
+                os.path.join(proc_net_bonding, bond))
+            for interface in parent_macs:
+                if interface in interfaces:
+                    interfaces[interface]['mac'] = parent_macs[interface]
+    return interfaces
+
+
+def annotate_with_driver_information(
+        interfaces, sys_class_net="/sys/class/net", proc_net="/proc/net"):
     """Determines driver information for each of the given interfaces.
 
     Annotates the given dictionary to update it with driver information
     (if found) for each interface.
+
+    Deletes bond interfaces if they are not configured.
+
+    :param interfaces: interfaces dictionary from `parse_ip_addr()`.
+    :param proc_net: path to /proc/net
+    :param sys_class_net: path to /sys/class/net
     """
+    interfaces = annotate_with_proc_net_bonding_original_macs(
+        interfaces, proc_net=proc_net)
+    bogus_interfaces = []
     for name in interfaces:
         iface = interfaces[name]
-        iftype = get_interface_type(name)
+        iftype = get_interface_type(
+            name, sys_class_net=sys_class_net, proc_net=proc_net)
         interfaces[name]['type'] = iftype
         if iftype == 'ethernet.bond':
-            iface['bonded_interfaces'] = get_bonded_interfaces(name)
+            bond_parents = get_bonded_interfaces(
+                name, sys_class_net=sys_class_net)
+            if len(bond_parents) > 0:
+                iface['bonded_interfaces'] = bond_parents
+            else:
+                # If we found a bond interface with no parents, just pretend
+                # it doesn't exist. The MAAS model assumes bonds must have
+                # backing interfaces.
+                bogus_interfaces.append(name)
         elif iftype == 'ethernet.vlan':
             iface['vid'] = get_vid_from_ifname(name)
         elif iftype == 'ethernet.bridge':
-            iface['bridged_interfaces'] = get_bridged_interfaces(name)
+            iface['bridged_interfaces'] = get_bridged_interfaces(
+                name, sys_class_net=sys_class_net)
+    for name in bogus_interfaces:
+        del interfaces[name]
     return interfaces
 
 

=== modified file 'src/provisioningserver/utils/tests/test_ipaddr.py'
--- src/provisioningserver/utils/tests/test_ipaddr.py	2016-03-04 00:56:41 +0000
+++ src/provisioningserver/utils/tests/test_ipaddr.py	2016-03-07 19:23:24 +0000
@@ -21,6 +21,7 @@
     _add_additional_interface_properties,
     _parse_interface_definition,
     annotate_with_driver_information,
+    annotate_with_proc_net_bonding_original_macs,
     get_bonded_interfaces,
     get_interface_type,
     get_ip_addr,
@@ -33,6 +34,7 @@
 from testtools.matchers import (
     Contains,
     Equals,
+    HasLength,
     Not,
 )
 
@@ -347,17 +349,19 @@
         self.assertThat(ip_link['ens11'], Not(Contains('inet')))
 
 
-class TestGetInterfaceType(MAASTestCase):
+class FakeSysProcTestCase(MAASTestCase):
 
     def setUp(self):
-        super(TestGetInterfaceType, self).setUp()
+        super().setUp()
         self.tmp_sys_net = mkdtemp('maas-unit-tests.sys-class-net')
-        self.tmp_proc_net_vlan = mkdtemp('maas-unit-tests.proc-net-vlan')
+        self.tmp_proc_net = mkdtemp('maas-unit-tests.proc-net')
+        os.mkdir(os.path.join(self.tmp_proc_net, "vlan"))
+        os.mkdir(os.path.join(self.tmp_proc_net, "bonding"))
 
     def tearDown(self):
-        super(TestGetInterfaceType, self).tearDown()
+        super().tearDown()
         rmtree(self.tmp_sys_net)
-        rmtree(self.tmp_proc_net_vlan)
+        rmtree(self.tmp_proc_net)
 
     def createInterfaceType(
             self, ifname, iftype, is_bridge=False, is_vlan=False,
@@ -371,7 +375,7 @@
         if is_bridge:
             os.mkdir(os.path.join(ifdir, 'bridge'))
         if is_vlan:
-            with open(os.path.join(self.tmp_proc_net_vlan, ifname), 'w'):
+            with open(os.path.join(self.tmp_proc_net, "vlan", ifname), 'w'):
                 pass  # Just touch.
         if is_bond:
             os.mkdir(os.path.join(ifdir, 'bonding'))
@@ -395,6 +399,9 @@
     def createEthernetInterface(self, ifname, **kwargs):
         self.createInterfaceType(ifname, 1, **kwargs)
 
+
+class TestGetInterfaceType(FakeSysProcTestCase):
+
     def test__identifies_missing_interface(self):
         self.assertThat(get_interface_type(
             'eth0', sys_class_net=self.tmp_sys_net),
@@ -405,7 +412,7 @@
         self.createEthernetInterface('br0', is_bridge=True)
         self.assertThat(get_interface_type(
             'br0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet.bridge')
         )
 
@@ -413,7 +420,7 @@
         self.createEthernetInterface('bond0', is_bond=True)
         self.assertThat(get_interface_type(
             'bond0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet.bond')
         )
 
@@ -429,7 +436,7 @@
         self.createEthernetInterface('vlan42', is_vlan=True)
         self.assertThat(get_interface_type(
             'vlan42', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet.vlan')
         )
 
@@ -437,7 +444,7 @@
         self.createEthernetInterface('eth0', is_physical=True)
         self.assertThat(get_interface_type(
             'eth0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet.physical')
         )
 
@@ -445,7 +452,7 @@
         self.createEthernetInterface('wlan0', is_wireless=True)
         self.assertThat(get_interface_type(
             'wlan0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet.wireless')
         )
 
@@ -453,7 +460,7 @@
         self.createEthernetInterface('eth1')
         self.assertThat(get_interface_type(
             'eth1', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ethernet')
         )
 
@@ -461,7 +468,7 @@
         self.createLoopbackInterface('lo')
         self.assertThat(get_interface_type(
             'lo', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('loopback')
         )
 
@@ -469,7 +476,7 @@
         self.createIpIpInterface('tun0')
         self.assertThat(get_interface_type(
             'tun0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('ipip')
         )
 
@@ -477,18 +484,16 @@
         self.createInterfaceType('avian0', 1149)
         self.assertThat(get_interface_type(
             'avian0', sys_class_net=self.tmp_sys_net,
-            proc_net_vlan=self.tmp_proc_net_vlan),
+            proc_net=self.tmp_proc_net),
             Equals('unknown-1149')
         )
 
 
-class TestAnnotateWithDriverInformation(MAASTestCase):
+class TestAnnotateWithDriverInformation(FakeSysProcTestCase):
 
     def test__populates_interface_type_for_each_interface(self):
         # Note: this is more of an end-to-end test, since we call
-        # "/sbin/ip addr" on the host running the tests. This is necessary
-        # because we don't have dependency injection for the directory names
-        # all the way through.
+        # "/sbin/ip addr" on the host running the tests.
         ip_addr_output = check_output(['/sbin/ip', 'addr'])
         interfaces = parse_ip_addr(ip_addr_output)
         interfaces_with_types = annotate_with_driver_information(interfaces)
@@ -502,6 +507,89 @@
             elif iface['type'] == 'ethernet.bridge':
                 self.expectThat(iface, Contains('bridged_interfaces'))
 
+    def test__ignores_bond_interfaces_with_no_parents(self):
+        interfaces = {
+            'eth0': {},
+            'eth1': {},
+            'bond0': {},
+            'bond1': {},
+        }
+        self.createEthernetInterface('eth0', is_physical=True)
+        self.createEthernetInterface('eth1', is_physical=True)
+        self.createEthernetInterface(
+            'bond0', is_bond=True, bonded_interfaces=['eth0, eth1'])
+        self.createEthernetInterface(
+            'bond1', is_bond=True, bonded_interfaces=[])
+        interfaces = annotate_with_driver_information(
+            interfaces, sys_class_net=self.tmp_sys_net,
+            proc_net=self.tmp_proc_net)
+        self.expectThat(interfaces, HasLength(3))
+        self.expectThat(interfaces, Contains('eth0'))
+        self.expectThat(interfaces, Contains('eth1'))
+        self.expectThat(interfaces, Contains('bond0'))
+
+    def test__finds_bond_members_original_mac_addresses(self):
+        testdata = dedent("""\
+            Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
+
+            Bonding Mode: fault-tolerance (active-backup)
+            Primary Slave: None
+            Currently Active Slave: ens11
+            MII Status: up
+            MII Polling Interval (ms): 100
+            Up Delay (ms): 200
+            Down Delay (ms): 0
+
+            Slave Interface: ens11
+            MII Status: up
+            Speed: Unknown
+            Duplex: Unknown
+            Link Failure Count: 0
+            Permanent HW addr: 52:54:00:ea:1c:fc
+            Slave queue ID: 0
+
+            Slave Interface: ens3
+            MII Status: up
+            Speed: Unknown
+            Duplex: Unknown
+            Link Failure Count: 0
+            Permanent HW addr: 52:54:00:13:0e:6f
+            Slave queue ID: 0
+            """)
+        proc_net_bonding_bond0 = os.path.join(
+            self.tmp_proc_net, "bonding", "bond0")
+        with open(proc_net_bonding_bond0, mode='w') as f:
+            f.write(testdata)
+        interfaces = {
+            "ens3": {"mac": "00:01:02:03:04:05"},
+            "ens11": {"mac": "01:02:03:04:05:06"},
+        }
+        annotate_with_proc_net_bonding_original_macs(
+            interfaces, proc_net=self.tmp_proc_net)
+        self.assertEqual(
+            {
+                "ens3": {"mac": "52:54:00:13:0e:6f"},
+                "ens11": {"mac": "52:54:00:ea:1c:fc"},
+            },
+            interfaces
+        )
+
+    def test__ignores_missing_proc_net_bonding(self):
+        os.rmdir(os.path.join(self.tmp_proc_net, "bonding"))
+        interfaces = {
+            "ens3": {"mac": "00:01:02:03:04:05"},
+            "ens11": {"mac": "01:02:03:04:05:06"},
+        }
+        annotate_with_proc_net_bonding_original_macs(
+            interfaces, proc_net=self.tmp_proc_net)
+        self.assertEqual(
+            {
+                "ens3": {"mac": "00:01:02:03:04:05"},
+                "ens11": {"mac": "01:02:03:04:05:06"},
+            },
+            interfaces
+        )
+
 
 class TestGetIPAddr(MAASTestCase):
     """Tests for `get_ip_addr`, `get_ip_addr_json`, `get_mac_addresses`."""