os_net_config should map nics that are down if nic is in mapping file

Currently os-net_config will map nics from a user-supplied mapping file
only if the nic is active (operstate = up).  This can cause problems
if a nic is in a bond and one of the bond's nics has no carrier.
This fix will map the nic from the mapping file if the nic is defined
on the system, regardless of the operstate status.

The fix implements a new function to return a list of available nics
(no check of operstate) for use if a mapping file is supplied.  The
list of active nics must still be used in the default case when
numbering nics (no mapping file supplied).  There is also some
cleanup to check if a user-supplied mac is in the mapping file before
attempting to convert the mac to a nic.

Change-Id: Ia5d8c8b49b7ac0b51ee42a754f06e5e53587a5f6
Closes-Bug: 1679787

4 files changed, 103 insertions, 40 deletions

diff --git a/os_net_config/objects.py b/os_net_config/objects.py
index 944aecd..5d43777 100644
--- a/os_net_config/objects.py
+++ b/os_net_config/objects.py
@@ -85,37 +85,49 @@ def _mapped_nics(nic_mapping=None):
     if _MAPPED_NICS:
         return _MAPPED_NICS
     _MAPPED_NICS = {}
-    active_nics = utils.ordered_active_nics()
-    for nic_alias, nic_mapped in mapping.items():
-        if nic_mapped not in active_nics:
-            # The mapping is either invalid, or specifies a mac
-            is_mapping_valid = False
-            for active in active_nics:
-                try:
-                    active_mac = utils.interface_mac(active)
-                except IOError:
+
+    if mapping:
+        # If mapping file provided, nics need not be active
+        available_nics = utils.ordered_available_nics()
+        for nic_alias, nic_mapped in mapping.items():
+
+            if netaddr.valid_mac(nic_mapped):
+                # If 'nic' is actually a mac address, retrieve actual nic name
+                for nic in available_nics:
+                    try:
+                        mac = utils.interface_mac(nic)
+                    except IOError:
+                        continue
+                    if nic_mapped == mac:
+                        logger.debug("%s matches device %s" %
+                                     (nic_mapped, nic))
+                        nic_mapped = nic
+                        break
+                else:
+                    # The mac could not be found on this system
+                    logger.error('mac %s not found in available nics (%s)'
+                                 % (nic_mapped, ', '.join(available_nics)))
                     continue
-                if nic_mapped == active_mac:
-                    logger.debug("%s matches device %s" % (nic_mapped, active))
-                    nic_mapped = active
-                    is_mapping_valid = True
-                    break
-
-            if not is_mapping_valid:
-                # The mapping can't specify a non-active or non-existent nic
-                logger.warning('interface %s is not an active nic (%s)'
-                               % (nic_mapped, ', '.join(active_nics)))
+
+            elif nic_mapped not in available_nics:
+                # nic doesn't exist on this system
+                logger.error('nic %s not found in available nics (%s)'
+                             % (nic_mapped, ', '.join(available_nics)))
                 continue
 
-        # Duplicate mappings are not allowed
-        if nic_mapped in _MAPPED_NICS.values():
-            msg = ('interface %s already mapped, '
-                   'check mapping file for duplicates'
-                   % nic_mapped)
-            raise InvalidConfigException(msg)
+            # Duplicate mappings are not allowed
+            if nic_mapped in _MAPPED_NICS.values():
+                msg = ('interface %s already mapped, '
+                       'check mapping file for duplicates'
+                       % nic_mapped)
+                raise InvalidConfigException(msg)
+
+            _MAPPED_NICS[nic_alias] = nic_mapped
+            logger.info("%s in mapping file mapped to: %s"
+                        % (nic_alias, nic_mapped))
 
-        _MAPPED_NICS[nic_alias] = nic_mapped
-        logger.info("%s mapped to: %s" % (nic_alias, nic_mapped))
+    # nics not in mapping file must be active in order to be mapped
+    active_nics = utils.ordered_active_nics()
 
     # Add default numbered mappings, but do not overwrite existing entries
     for nic_mapped in set(active_nics).difference(set(_MAPPED_NICS.values())):
diff --git a/os_net_config/tests/test_objects.py b/os_net_config/tests/test_objects.py
index f5daf31..6f18d79 100644
--- a/os_net_config/tests/test_objects.py
+++ b/os_net_config/tests/test_objects.py
@@ -873,6 +873,12 @@ class TestNicMapping(base.TestCase):
             return nics
         self.stubs.Set(utils, 'ordered_active_nics', dummy_ordered_active_nics)
 
+    def _stub_available_nics(self, nics):
+        def dummy_ordered_available_nics():
+            return nics
+        self.stubs.Set(utils, 'ordered_available_nics',
+                       dummy_ordered_available_nics)
+
     def test_mapped_nics_default(self):
         self._stub_active_nics(['em1', 'em2'])
         expected = {'nic1': 'em1', 'nic2': 'em2'}
@@ -880,43 +886,56 @@ class TestNicMapping(base.TestCase):
 
     def test_mapped_nics_mapped(self):
         self._stub_active_nics(['em1', 'em2'])
+        self._stub_available_nics(['em1', 'em2'])
         mapping = {'nic1': 'em2', 'nic2': 'em1'}
         expected = {'nic1': 'em2', 'nic2': 'em1'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
     def test_mapped_nics_mapped_partial(self):
         self._stub_active_nics(['em1', 'em2', 'em3', 'em4'])
+        self._stub_available_nics(['em1', 'em2', 'em3', 'em4'])
         mapping = {'nic1': 'em2', 'nic2': 'em1'}
         expected = {'nic1': 'em2', 'nic2': 'em1', 'nic3': 'em3', 'nic4': 'em4'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
     def test_mapped_nics_mapped_partial_reordered(self):
         self._stub_active_nics(['em1', 'em2', 'em3', 'em4'])
+        self._stub_available_nics(['em1', 'em2', 'em3', 'em4'])
         mapping = {'nic1': 'em1', 'nic2': 'em3'}
         expected = {'nic1': 'em1', 'nic2': 'em3', 'nic4': 'em4'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
     def test_mapped_nics_mapped_unnumbered(self):
         self._stub_active_nics(['em1', 'em2', 'em3', 'em4'])
+        self._stub_available_nics(['em1', 'em2', 'em3', 'em4'])
         mapping = {'John': 'em1', 'Paul': 'em2', 'George': 'em3'}
         expected = {'John': 'em1', 'Paul': 'em2', 'George': 'em3',
                     'nic4': 'em4'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
     def test_mapped_nics_map_error_notactive(self):
-        self._stub_active_nics(['em1', 'em2'])
-        mapping = {'nic1': 'em3', 'nic2': 'em1'}
-        expected = {'nic2': 'em1'}
+        self._stub_active_nics(['em2'])
+        self._stub_available_nics(['em1', 'em2', 'em3'])
+        mapping = {'nic2': 'em1'}
+        expected = {'nic1': 'em2', 'nic2': 'em1'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
     def test_mapped_nics_map_error_duplicate(self):
         self._stub_active_nics(['em1', 'em2'])
+        self._stub_available_nics(['em1', 'em2'])
         mapping = {'nic1': 'em1', 'nic2': 'em1'}
         err = self.assertRaises(objects.InvalidConfigException,
                                 objects._mapped_nics, nic_mapping=mapping)
         expected = 'em1 already mapped, check mapping file for duplicates'
         self.assertIn(expected, six.text_type(err))
 
+    def test_mapped_nics_map_invalid_nic(self):
+        self._stub_active_nics(['em1'])
+        self._stub_available_nics(['em1', 'em2'])
+        mapping = {'nic1': 'em1', 'nic2': 'foo'}
+        expected = {'nic1': 'em1'}
+        self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
+
     def test_mapped_nics_map_mac(self):
         def dummy_interface_mac(name):
             mac_map = {'em1': '12:34:56:78:9a:bc',
@@ -924,10 +943,24 @@ class TestNicMapping(base.TestCase):
             return mac_map[name]
         self.stubs.Set(utils, 'interface_mac', dummy_interface_mac)
         self._stub_active_nics(['em1', 'em2'])
+        self._stub_available_nics(['em1', 'em2'])
         mapping = {'nic1': '12:34:56:de:f0:12', 'nic2': '12:34:56:78:9a:bc'}
         expected = {'nic1': 'em2', 'nic2': 'em1'}
         self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
 
+    def test_mapped_nics_map_invalid_mac(self):
+        def dummy_interface_mac(name):
+            mac_map = {'em1': '12:34:56:78:9a:bc',
+                       'em2': '12:34:56:de:f0:12'}
+            return mac_map[name]
+
+        self.stubs.Set(utils, 'interface_mac', dummy_interface_mac)
+        self._stub_active_nics(['em1', 'em2'])
+        self._stub_available_nics(['em1', 'em2'])
+        mapping = {'nic1': '12:34:56:de:f0:12', 'nic2': 'aa:bb:cc:dd:ee:ff'}
+        expected = {'nic1': 'em2'}
+        self.assertEqual(expected, objects._mapped_nics(nic_mapping=mapping))
+
     def test_mapped_nics_no_active(self):
         self._stub_active_nics([])
         expected = {}
diff --git a/os_net_config/tests/test_utils.py b/os_net_config/tests/test_utils.py
index 1885cbb..9e516b6 100644
--- a/os_net_config/tests/test_utils.py
+++ b/os_net_config/tests/test_utils.py
@@ -56,9 +56,9 @@ class TestUtils(base.TestCase):
         tmpdir = tempfile.mkdtemp()
         self.stubs.Set(utils, '_SYS_CLASS_NET', tmpdir)
 
-        def test_is_active_nic(interface_name):
+        def test_is_available_nic(interface_name, check_active):
             return True
-        self.stubs.Set(utils, '_is_active_nic', test_is_active_nic)
+        self.stubs.Set(utils, '_is_available_nic', test_is_available_nic)
 
         for nic in ['a1', 'em1', 'em2', 'eth2', 'z1',
                     'enp8s0', 'enp10s0', 'enp1s0f0']:
@@ -186,9 +186,9 @@ class TestUtils(base.TestCase):
         tmpdir = tempfile.mkdtemp()
         self.stubs.Set(utils, '_SYS_CLASS_NET', tmpdir)
 
-        def test_is_active_nic(interface_name):
+        def test_is_available_nic(interface_name, check_active):
             return True
-        self.stubs.Set(utils, '_is_active_nic', test_is_active_nic)
+        self.stubs.Set(utils, '_is_available_nic', test_is_available_nic)
 
         for nic in ['a1', 'em1', 'em2', 'eth2', 'z1',
                     'enp8s0', 'enp10s0', 'enp1s0f0']:
diff --git a/os_net_config/utils.py b/os_net_config/utils.py
index 98bfe99..84e719e 100644
--- a/os_net_config/utils.py
+++ b/os_net_config/utils.py
@@ -92,20 +92,30 @@ def interface_mac(name):
 
 
 def _is_active_nic(interface_name):
+    return _is_available_nic(interface_name, True)
+
+
+def _is_available_nic(interface_name, check_active=True):
     try:
         if interface_name == 'lo':
             return False
 
         device_dir = _SYS_CLASS_NET + '/%s/device' % interface_name
         has_device_dir = os.path.isdir(device_dir)
+        if not has_device_dir:
+            return False
 
         operstate = None
         with open(_SYS_CLASS_NET + '/%s/operstate' % interface_name, 'r') as f:
             operstate = f.read().rstrip().lower()
+        if check_active and operstate != 'up':
+            return False
 
         address = None
         with open(_SYS_CLASS_NET + '/%s/address' % interface_name, 'r') as f:
             address = f.read().rstrip()
+        if not address:
+            return False
 
         # If SR-IOV Virtual Functions (VF) are enabled in an interface, there
         # will be additional nics created for each VF. It has to be ignored in
@@ -114,12 +124,12 @@ def _is_active_nic(interface_name):
         # ignored.
         vf_path_check = _SYS_CLASS_NET + '/%s/device/physfn' % interface_name
         is_sriov_vf = os.path.isdir(vf_path_check)
-
-        if (has_device_dir and operstate == 'up' and address and
-                not is_sriov_vf):
-            return True
-        else:
+        if is_sriov_vf:
             return False
+
+        # nic is available
+        return True
+
     except IOError:
         return False
 
@@ -136,13 +146,21 @@ def _is_embedded_nic(nic):
     return False
 
 
+def ordered_available_nics():
+    return _ordered_nics(False)
+
+
 def ordered_active_nics():
+    return _ordered_nics(True)
+
+
+def _ordered_nics(check_active):
     embedded_nics = []
     nics = []
     logger.debug("Finding active nics")
     for name in glob.iglob(_SYS_CLASS_NET + '/*'):
         nic = name[(len(_SYS_CLASS_NET) + 1):]
-        if _is_active_nic(nic):
+        if _is_available_nic(nic, check_active):
             if _is_embedded_nic(nic):
                 logger.debug("%s is an embedded active nic" % nic)
                 embedded_nics.append(nic)