Merge lp:~hopem/charm-helpers/allow-list-nics-return-all into lp:charm-helpers

[Edward Hope-Morley]

[Liam Young (gnuoy)]

Please could you update the unit tests so that the macs of IP_LINE_ETH0 and IP_LINE_ETH0_VLAN match and add a test for list_nics(['eth'], include_vlans=False) ?

As a nitpick I think it's cleaner parse the output of ip when using '-o' so that all entries are on one line but that's how it was before so I won't block on it.

review: Needs Fixing

[lp:~hopem/charm-helpers/allow-list-nics-return-all]

432. By Edward Hope-Morley on 2015-08-18

[hopem,r=]

Add support to core.host.list_nics() to allow for listing
all nics i.e. unfiltered. Ensure that neutron resolve_ports
only resolves physical interfaces.

433. By Edward Hope-Morley on 2015-08-18

Add support for identiying bonded nics and resolving
their master.

434. By Edward Hope-Morley on 2015-08-18

ensure master is bond

435. By Edward Hope-Morley on 2015-08-19

added multimac unit test

436. By Edward Hope-Morley on 2015-08-19

more unit test

437. By Edward Hope-Morley on 2015-08-19

*fix* py3 unit test error

438. By Edward Hope-Morley on 2015-08-19

fix unit test

Unmerged revisions

=== modified file 'charmhelpers/contrib/openstack/context.py'
--- charmhelpers/contrib/openstack/context.py	2015-07-29 14:21:45 +0000
+++ charmhelpers/contrib/openstack/context.py	2015-08-18 11:36:40 +0000
@@ -50,6 +50,7 @@
 from charmhelpers.core.strutils import bool_from_string
 
 from charmhelpers.core.host import (
+    is_phy_iface,
     list_nics,
     get_nic_hwaddr,
     mkdir,
@@ -923,7 +924,6 @@
 
 
 class NeutronPortContext(OSContextGenerator):
-    NIC_PREFIXES = ['eth', 'bond']
 
     def resolve_ports(self, ports):
         """Resolve NICs not yet bound to bridge(s)
@@ -935,12 +935,14 @@
 
         hwaddr_to_nic = {}
         hwaddr_to_ip = {}
-        for nic in list_nics(self.NIC_PREFIXES):
-            hwaddr = get_nic_hwaddr(nic)
-            hwaddr_to_nic[hwaddr] = nic
-            addresses = get_ipv4_addr(nic, fatal=False)
-            addresses += get_ipv6_addr(iface=nic, fatal=False)
-            hwaddr_to_ip[hwaddr] = addresses
+        for nic in list_nics():
+            # Ignore virtual interfaces
+            if is_phy_iface(nic):
+                hwaddr = get_nic_hwaddr(nic)
+                hwaddr_to_nic[hwaddr] = nic
+                addresses = get_ipv4_addr(nic, fatal=False)
+                addresses += get_ipv6_addr(iface=nic, fatal=False)
+                hwaddr_to_ip[hwaddr] = addresses
 
         resolved = []
         mac_regex = re.compile(r'([0-9A-F]{2}[:-]){5}([0-9A-F]{2})', re.I)
@@ -961,7 +963,8 @@
                 # trust it to be the real external network).
                 resolved.append(entry)
 
-        return resolved
+        # Ensure no duplicates
+        return list(set(resolved))
 
 
 class OSConfigFlagContext(OSContextGenerator):
@@ -1280,15 +1283,19 @@
     def __call__(self):
         ports = config('data-port')
         if ports:
+            # Map of {port/mac:bridge}
             portmap = parse_data_port_mappings(ports)
-            ports = portmap.values()
+            ports = portmap.keys()
+            # Resolve provided ports or mac addresses and filter out those
+            # already attached to a bridge.
             resolved = self.resolve_ports(ports)
+            # FIXME: is this necessary?
             normalized = {get_nic_hwaddr(port): port for port in resolved
                           if port not in ports}
             normalized.update({port: port for port in resolved
                                if port in ports})
             if resolved:
-                return {bridge: normalized[port] for bridge, port in
+                return {bridge: normalized[port] for port, bridge in
                         six.iteritems(portmap) if port in normalized.keys()}
 
         return None
 
=== modified file 'charmhelpers/contrib/openstack/neutron.py'
--- charmhelpers/contrib/openstack/neutron.py	2015-06-10 11:17:45 +0000
+++ charmhelpers/contrib/openstack/neutron.py	2015-08-18 11:36:40 +0000
@@ -255,17 +255,30 @@
         return 'neutron'
 
 
-def parse_mappings(mappings):
+def parse_mappings(mappings, key_rvalue=False):
+    """By default mappings are lvalue keyed.
+
+    If key_rvalue is True, the mapping will be reversed to allow multiple
+    configs for the same lvalue.
+    """
     parsed = {}
     if mappings:
         mappings = mappings.split()
         for m in mappings:
             p = m.partition(':')
-            key = p[0].strip()
-            if p[1]:
-                parsed[key] = p[2].strip()
+
+            if key_rvalue:
+                key_index = 2
+                val_index = 0
+                # if there is no rvalue skip to next
+                if not p[1]:
+                    continue
             else:
-                parsed[key] = ''
+                key_index = 0
+                val_index = 2
+
+            key = p[key_index].strip()
+            parsed[key] = p[val_index].strip()
 
     return parsed
 
@@ -283,25 +296,25 @@
 def parse_data_port_mappings(mappings, default_bridge='br-data'):
     """Parse data port mappings.
 
-    Mappings must be a space-delimited list of bridge:port mappings.
+    Mappings must be a space-delimited list of port:bridge mappings.
 
-    Returns dict of the form {bridge:port}.
+    Returns dict of the form {port:bridge} where port may be an mac address or
+    interface name.
     """
-    _mappings = parse_mappings(mappings)
+
+    # NOTE(dosaboy): we use rvalue for key to allow multiple values to be
+    # proposed for <port> since it may be a mac address which will differ
+    # across units this allowing first-known-good to be chosen.
+    _mappings = parse_mappings(mappings, key_rvalue=True)
     if not _mappings or list(_mappings.values()) == ['']:
         if not mappings:
             return {}
 
         # For backwards-compatibility we need to support port-only provided in
         # config.
-        _mappings = {default_bridge: mappings.split()[0]}
-
-    bridges = _mappings.keys()
-    ports = _mappings.values()
-    if len(set(bridges)) != len(bridges):
-        raise Exception("It is not allowed to have more than one port "
-                        "configured on the same bridge")
-
+        _mappings = {mappings.split()[0]: default_bridge}
+
+    ports = _mappings.keys()
     if len(set(ports)) != len(ports):
         raise Exception("It is not allowed to have the same port configured "
                         "on more than one bridge")
 
=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2015-08-17 11:15:38 +0000
+++ charmhelpers/core/host.py	2015-08-18 11:36:40 +0000
@@ -417,25 +417,59 @@
     return(''.join(random_chars))
 
 
-def list_nics(nic_type):
+def is_phy_iface(interface):
+    """Returns True if interface is not virtual, otherwise False."""
+    if interface:
+        sys_net = '/sys/class/net'
+        if os.path.isdir(sys_net):
+            for iface in glob.glob(os.path.join(sys_net, '*')):
+                if '/virtual/' in os.path.realpath(iface):
+                    continue
+
+                if interface == os.path.basename(iface):
+                    return True
+
+    return False
+
+
+def list_nics(nic_type=None):
     '''Return a list of nics of given type(s)'''
     if isinstance(nic_type, six.string_types):
         int_types = [nic_type]
     else:
         int_types = nic_type
+
     interfaces = []
-    for int_type in int_types:
-        cmd = ['ip', 'addr', 'show', 'label', int_type + '*']
+    if nic_type:
+        for int_type in int_types:
+            cmd = ['ip', 'addr', 'show', 'label', int_type + '*']
+            ip_output = subprocess.check_output(cmd).decode('UTF-8')
+            ip_output = ip_output.split('\n')
+            ip_output = (line for line in ip_output if line)
+            for line in ip_output:
+                if line.split()[1].startswith(int_type):
+                    matched = re.search('.*: (' + int_type +
+                                        r'[0-9]+\.[0-9]+)@.*', line)
+                    if matched:
+                        iface = matched.groups()[0]
+                    else:
+                        iface = line.split()[1].replace(":", "")
+
+                    if iface not in interfaces:
+                        interfaces.append(iface)
+    else:
+        cmd = ['ip', 'a']
         ip_output = subprocess.check_output(cmd).decode('UTF-8').split('\n')
-        ip_output = (line for line in ip_output if line)
+        ip_output = (line.strip() for line in ip_output if line)
+
+        key = re.compile('^[0-9]+:\s+(.+):')
         for line in ip_output:
-            if line.split()[1].startswith(int_type):
-                matched = re.search('.*: (' + int_type + r'[0-9]+\.[0-9]+)@.*', line)
-                if matched:
-                    interface = matched.groups()[0]
-                else:
-                    interface = line.split()[1].replace(":", "")
-                interfaces.append(interface)
+            matched = re.search(key, line)
+            if matched:
+                iface = matched.group(1)
+                iface = iface.partition("@")[0]
+                if iface not in interfaces:
+                    interfaces.append(iface)
 
     return interfaces
 
 
=== modified file 'tests/contrib/openstack/test_neutron_utils.py'
--- tests/contrib/openstack/test_neutron_utils.py	2015-05-02 22:27:26 +0000
+++ tests/contrib/openstack/test_neutron_utils.py	2015-08-18 11:36:40 +0000
@@ -181,13 +181,13 @@
         ret = neutron.parse_data_port_mappings(None)
         self.assertEqual(ret, {})
         ret = neutron.parse_data_port_mappings('br0:eth0')
-        self.assertEqual(ret, {'br0': 'eth0'})
+        self.assertEqual(ret, {'eth0': 'br0'})
         # Back-compat test
         ret = neutron.parse_data_port_mappings('eth0', default_bridge='br0')
-        self.assertEqual(ret, {'br0': 'eth0'})
+        self.assertEqual(ret, {'eth0': 'br0'})
         # Multiple mappings
         ret = neutron.parse_data_port_mappings('br0:eth0 br1:eth1')
-        self.assertEqual(ret, {'br0': 'eth0', 'br1': 'eth1'})
+        self.assertEqual(ret, {'eth0': 'br0', 'eth1': 'br1'})
 
     def test_parse_vlan_range_mappings(self):
         ret = neutron.parse_vlan_range_mappings(None)
 
=== modified file 'tests/contrib/openstack/test_os_contexts.py'
--- tests/contrib/openstack/test_os_contexts.py	2015-07-29 14:21:45 +0000
+++ tests/contrib/openstack/test_os_contexts.py	2015-08-18 11:36:40 +0000
@@ -2425,6 +2425,8 @@
         self.assertEquals(context.ExternalPortContext()(),
                           {'ext_port': 'eth1010'})
 
+    @patch('charmhelpers.contrib.openstack.context.is_phy_iface',
+           lambda arg: True)
     @patch('charmhelpers.contrib.openstack.context.get_nic_hwaddr')
     @patch('charmhelpers.contrib.openstack.context.list_nics')
     @patch('charmhelpers.contrib.openstack.context.get_ipv6_addr')
@@ -2452,6 +2454,8 @@
 
         self.assertEquals(context.ExternalPortContext()(), {})
 
+    @patch('charmhelpers.contrib.openstack.context.is_phy_iface',
+           lambda arg: True)
     @patch('charmhelpers.contrib.openstack.context.get_nic_hwaddr')
     @patch('charmhelpers.contrib.openstack.context.list_nics')
     @patch('charmhelpers.contrib.openstack.context.get_ipv6_addr')
@@ -2484,8 +2488,9 @@
            'resolve_ports')
     def test_data_port_eth(self, mock_resolve):
         self.config.side_effect = fake_config({'data-port':
-                                               'phybr1:eth1010'})
-        mock_resolve.side_effect = lambda ports: ports
+                                               'phybr1:eth1010 '
+                                               'phybr1:eth1011'})
+        mock_resolve.side_effect = lambda ports: ['eth1010']
         self.assertEquals(context.DataPortContext()(),
                           {'phybr1': 'eth1010'})
 
 
=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2015-08-17 11:15:38 +0000
+++ tests/core/test_host.py	2015-08-18 11:36:40 +0000
@@ -35,6 +35,11 @@
     link/ether e4:11:5b:ab:a7:3c brd ff:ff:ff:ff:ff:ff
 """
 
+IP_LINE_ETH100 = b"""
+2: eth100: <BROADCAST,MULTICAST,SLAVE,UP,LOWER_UP> mtu 1500 qdisc mq master bond0 state UP qlen 1000
+    link/ether e4:11:5b:ab:a7:3d brd ff:ff:ff:ff:ff:ff
+"""
+
 IP_LINE_ETH0_VLAN = b"""
 6: eth0.10@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default
     link/ether 08:00:27:16:b9:5f brd ff:ff:ff:ff:ff:ff
@@ -47,7 +52,7 @@
 
 IP_LINE_HWADDR = b"""2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000\    link/ether e4:11:5b:ab:a7:3c brd ff:ff:ff:ff:ff:ff"""
 
-IP_LINES = IP_LINE_ETH0 + IP_LINE_ETH1 + IP_LINE_ETH0_VLAN
+IP_LINES = IP_LINE_ETH0 + IP_LINE_ETH1 + IP_LINE_ETH0_VLAN + IP_LINE_ETH100
 
 IP_LINE_BONDS = b"""
 6: bond0.10@bond0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default
@@ -962,13 +967,36 @@
         pw2 = host.pwgen(10)
         self.assertNotEqual(pw, pw2, 'Duplicated password')
 
+    @patch.object(host, 'glob')
+    @patch('os.path.realpath')
+    @patch('os.path.isdir')
+    def test_is_phy_iface(self, mock_isdir, mock_realpath, mock_glob):
+        mock_isdir.return_value = True
+        mock_glob.glob.return_value = ['/sys/class/net/eth0',
+                                       '/sys/class/net/veth0']
+
+        def fake_realpath(soft):
+            if soft.endswith('/eth0'):
+                hard = \
+                    '/sys/devices/pci0000:00/0000:00:1c.4/0000:02:00.1/net/eth0'
+            else:
+                hard = '/sys/devices/virtual/net/veth0'
+
+            return hard
+
+        mock_realpath.side_effect = fake_realpath
+        self.assertTrue(host.is_phy_iface('eth0'))
+        self.assertFalse(host.is_phy_iface('veth0'))
+
     @patch('subprocess.check_output')
     def test_list_nics(self, check_output):
         check_output.return_value = IP_LINES
+        nics = host.list_nics()
+        self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100'])
         nics = host.list_nics('eth')
-        self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10'])
+        self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100'])
         nics = host.list_nics(['eth'])
-        self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10'])
+        self.assertEqual(nics, ['eth0', 'eth1', 'eth0.10', 'eth100'])
 
     @patch('subprocess.check_output')
     def test_list_nics_with_bonds(self, check_output):