From b536fbba616d958e58b110c8f86ea569f8834d7c Mon Sep 17 00:00:00 2001
From: Sam Betts <sam@code-smash.net>
Date: Wed, 15 Feb 2017 16:31:33 +0000
Subject: [PATCH] [LLDP] Skip NICs that say they are ready but are unreadable.

While listening for LLDP packets, if one of the sockets marks itself as
ready to read then our code will try to read data from that socket, but
if something goes wrong while reading that data then it causes IPA to
raise out of the loop skipping any other of the other NICs which might
have worked. This patch adds code to catch and LOG any exception that is
raised while we are trying to read data from one of the sockets so that
we can proceed to process all the NICs.

Change-Id: I8546097f5ae23755a5fdb448902007a2d823b7bf
Closes-Bug: #1665025
---
 ironic_python_agent/netutils.py               | 15 +++++--
 .../tests/unit/test_netutils.py               | 45 +++++++++++++++++++
 ...t-are-not-plugged-in-29213f0a701a72e4.yaml |  7 +++
 3 files changed, 63 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/LLDP-ignore-NICs-that-are-not-plugged-in-29213f0a701a72e4.yaml

diff --git a/ironic_python_agent/netutils.py b/ironic_python_agent/netutils.py
index 45b85a5d7..5d38548d5 100644
--- a/ironic_python_agent/netutils.py
+++ b/ironic_python_agent/netutils.py
@@ -182,10 +182,17 @@ def _get_lldp_info(interfaces):
             # Create a copy of interfaces to avoid deleting while iterating.
             for index, interface in enumerate(list(interfaces)):
                 if s == interface[1]:
-                    LOG.info('Found LLDP info for interface: %s',
-                             interface[0])
-                    lldp_info[interface[0]] = (
-                        _receive_lldp_packets(s))
+                    try:
+                        lldp_info[interface[0]] = _receive_lldp_packets(s)
+                    except socket.error:
+                        LOG.exception('Socket for network interface %s said '
+                                      'that it was ready to read we were '
+                                      'unable to read from the socket while '
+                                      'trying to get LLDP packet. Skipping '
+                                      'this network interface.', interface[0])
+                    else:
+                        LOG.info('Found LLDP info for interface: %s',
+                                 interface[0])
                     # Remove interface from the list, only need one packet
                     del interfaces[index]
 
diff --git a/ironic_python_agent/tests/unit/test_netutils.py b/ironic_python_agent/tests/unit/test_netutils.py
index 4237b155b..b9b2a11e4 100644
--- a/ironic_python_agent/tests/unit/test_netutils.py
+++ b/ironic_python_agent/tests/unit/test_netutils.py
@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import binascii
+import socket
 
 import mock
 from oslo_config import cfg
@@ -89,6 +90,50 @@ class TestNetutils(base.IronicAgentTest):
         # 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
         self.assertEqual(6, fcntl_mock.call_count)
 
+    @mock.patch('fcntl.ioctl', autospec=True)
+    @mock.patch('select.select', autospec=True)
+    @mock.patch('socket.socket', autospec=socket_socket_sig)
+    def test_get_lldp_info_socket_recv_error(self, sock_mock, select_mock,
+                                             fcntl_mock):
+        expected_lldp = {
+            'eth1': [
+                (0, b''),
+                (1, b'\x04\x88Z\x92\xecTY'),
+                (2, b'\x05Ethernet1/18'),
+                (3, b'\x00x')]
+        }
+
+        interface_names = ['eth0', 'eth1']
+
+        sock1 = mock.Mock()
+        sock1.recv.side_effect = socket.error("BOOM")
+
+        sock2 = mock.Mock()
+        sock2.recv.return_value = FAKE_LLDP_PACKET
+        sock2.fileno.return_value = 5
+
+        sock_mock.side_effect = [sock1, sock2]
+
+        select_mock.side_effect = [
+            ([sock1], [], []),
+            ([sock2], [], [])
+        ]
+
+        lldp_info = netutils.get_lldp_info(interface_names)
+        self.assertEqual(expected_lldp, lldp_info)
+
+        sock1.bind.assert_called_with(('eth0', netutils.LLDP_ETHERTYPE))
+        sock2.bind.assert_called_with(('eth1', netutils.LLDP_ETHERTYPE))
+
+        sock1.recv.assert_called_with(1600)
+        sock2.recv.assert_called_with(1600)
+
+        self.assertEqual(1, sock1.close.call_count)
+        self.assertEqual(1, sock2.close.call_count)
+
+        # 2 interfaces, 2 calls to enter promiscuous mode, 1 to leave
+        self.assertEqual(6, fcntl_mock.call_count)
+
     @mock.patch('fcntl.ioctl', autospec=True)
     @mock.patch('select.select', autospec=True)
     @mock.patch('socket.socket', autospec=socket_socket_sig)
diff --git a/releasenotes/notes/LLDP-ignore-NICs-that-are-not-plugged-in-29213f0a701a72e4.yaml b/releasenotes/notes/LLDP-ignore-NICs-that-are-not-plugged-in-29213f0a701a72e4.yaml
new file mode 100644
index 000000000..1f54af9b5
--- /dev/null
+++ b/releasenotes/notes/LLDP-ignore-NICs-that-are-not-plugged-in-29213f0a701a72e4.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixes bug in LLDP discovery code which lead to no LLDP information being
+    discovered for any network interface if one network interface raised an
+    exception, for example if the network interface incorrectly indicates its
+    ready to read. `<https://bugs.launchpad.net/ironic-python-agent/+bug/1665025>`_