Improve "get_devices_with_ip" performance

As reported in LP#1896734, there is a limit in the size of information
that can be transmitted in one single message between an application
and the privsep daemon. The read socket buffer is limited in size;
a message exceeding this size will generate an exception.

In order to limit the amount of information to be sent, this patch
improves the performance of "get_devices_with_ip". In the previous
implementation, the whole list of network devices from a namespace
was retrieved. In some environments, the list of devices could be
so big that the list returned by "privileged.get_link_devices" can
exceed the read buffer size (as reported in the LP bug when the
OVS agent tries to retrieve the list of IP addresses in the system).

Now the function calls "privileged.get_ip_addresses", that returns
a much smaller list. This patch is also reducing the number of system
calls to just one; the previous implementation was retrieving first
the devices link information list (that method was returning a much
bigger blob) and then, per device, retrieving the IP address
information.

Change-Id: I97ada62484023b9833ed12afd68eb4c8d337fd1f
Related-Bug: #1896734
(cherry picked from commit a6cdf273c0)

neutron/agent/linux/ip_lib.py

@@ -1340,35 +1340,37 @@ def _parse_ip_address(pyroute2_address, device_name):
             'event': event}
 
 
-def _parse_link_device(namespace, device, **kwargs):
-    """Parse pytoute2 link device information
-
-    For each link device, the IP address information is retrieved and returned
-    in a dictionary.
-    IP address scope: http://linux-ip.net/html/tools-ip-address.html
-    """
-    retval = []
-    name = get_attr(device, 'IFLA_IFNAME')
-    ip_addresses = privileged.get_ip_addresses(namespace,
-                                               index=device['index'],
-                                               **kwargs)
-    for ip_address in ip_addresses:
-        retval.append(_parse_ip_address(ip_address, name))
-    return retval
-
-
 def get_devices_with_ip(namespace, name=None, **kwargs):
+    retval = []
     link_args = {}
     if name:
         link_args['ifname'] = name
     scope = kwargs.pop('scope', None)
     if scope:
         kwargs['scope'] = IP_ADDRESS_SCOPE_NAME[scope]
-    devices = privileged.get_link_devices(namespace, **link_args)
+
-    retval = []
+    if not link_args:
-    for parsed_ips in (_parse_link_device(namespace, device, **kwargs)
+        ip_addresses = privileged.get_ip_addresses(namespace, **kwargs)
-                       for device in devices):
+    else:
-        retval += parsed_ips
+        device = get_devices_info(namespace, **link_args)
+        if not device:
+            return retval
+        ip_addresses = privileged.get_ip_addresses(
+            namespace, index=device[0]['index'], **kwargs)
+
+    devices = {}  # {device index: name}
+    for ip_address in ip_addresses:
+        index = ip_address['index']
+        name = get_attr(ip_address, 'IFA_LABEL') or devices.get(index)
+        if not name:
+            device = get_devices_info(namespace, index=index)
+            if not device:
+                continue
+            name = device[0]['name']
+
+        retval.append(_parse_ip_address(ip_address, name))
+        devices[index] = name
+
     return retval
 
 

neutron/tests/functional/agent/linux/test_ip_lib.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 import collections
+import copy
 import itertools
 import signal
 
@@ -1013,3 +1014,83 @@ class IpAddrCommandTestCase(functional_base.BaseSudoTestCase):
             devices_filtered = self.device.addr.list(scope=scope)
             devices_cidr = {device['cidr'] for device in devices_filtered}
             self.assertIn(_ip, devices_cidr)
+
+
+class GetDevicesWithIpTestCase(functional_base.BaseSudoTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.namespace = self.useFixture(net_helpers.NamespaceFixture()).name
+        self.devices = []
+        self.num_devices = 5
+        self.num_devices_with_ip = 3
+        for idx in range(self.num_devices):
+            dev_name = 'test_device_%s' % idx
+            ip_lib.IPWrapper(self.namespace).add_dummy(dev_name)
+            device = ip_lib.IPDevice(dev_name, namespace=self.namespace)
+            device.link.set_up()
+            self.devices.append(device)
+
+        self.cidrs = [netaddr.IPNetwork('10.10.0.0/24'),
+                      netaddr.IPNetwork('10.20.0.0/24'),
+                      netaddr.IPNetwork('2001:db8:1234:1111::/64'),
+                      netaddr.IPNetwork('2001:db8:1234:2222::/64')]
+        for idx in range(self.num_devices_with_ip):
+            for cidr in self.cidrs:
+                self.devices[idx].addr.add(str(cidr.ip + idx) + '/' +
+                                           str(cidr.netmask.netmask_bits()))
+
+    @staticmethod
+    def _remove_loopback_interface(ip_addresses):
+        return [ipa for ipa in ip_addresses if
+                ipa['name'] != ip_lib.LOOPBACK_DEVNAME]
+
+    @staticmethod
+    def _remove_ipv6_scope_link(ip_addresses):
+        # Remove all IPv6 addresses with scope link (fe80::...).
+        return [ipa for ipa in ip_addresses if not (
+                ipa['scope'] == 'link' and utils.get_ip_version(ipa['cidr']))]
+
+    @staticmethod
+    def _pop_ip_address(ip_addresses, cidr):
+        for idx, ip_address in enumerate(copy.deepcopy(ip_addresses)):
+            if cidr == ip_address['cidr']:
+                ip_addresses.pop(idx)
+                return
+
+    def test_get_devices_with_ip(self):
+        ip_addresses = ip_lib.get_devices_with_ip(self.namespace)
+        ip_addresses = self._remove_loopback_interface(ip_addresses)
+        ip_addresses = self._remove_ipv6_scope_link(ip_addresses)
+        self.assertEqual(self.num_devices_with_ip * len(self.cidrs),
+                         len(ip_addresses))
+        for idx in range(self.num_devices_with_ip):
+            for cidr in self.cidrs:
+                cidr = (str(cidr.ip + idx) + '/' +
+                        str(cidr.netmask.netmask_bits()))
+                self._pop_ip_address(ip_addresses, cidr)
+
+        self.assertEqual(0, len(ip_addresses))
+
+    def test_get_devices_with_ip_name(self):
+        for idx in range(self.num_devices_with_ip):
+            dev_name = 'test_device_%s' % idx
+            ip_addresses = ip_lib.get_devices_with_ip(self.namespace,
+                                                      name=dev_name)
+            ip_addresses = self._remove_loopback_interface(ip_addresses)
+            ip_addresses = self._remove_ipv6_scope_link(ip_addresses)
+
+            for cidr in self.cidrs:
+                cidr = (str(cidr.ip + idx) + '/' +
+                        str(cidr.netmask.netmask_bits()))
+                self._pop_ip_address(ip_addresses, cidr)
+
+            self.assertEqual(0, len(ip_addresses))
+
+        for idx in range(self.num_devices_with_ip, self.num_devices):
+            dev_name = 'test_device_%s' % idx
+            ip_addresses = ip_lib.get_devices_with_ip(self.namespace,
+                                                      name=dev_name)
+            ip_addresses = self._remove_loopback_interface(ip_addresses)
+            ip_addresses = self._remove_ipv6_scope_link(ip_addresses)
+            self.assertEqual(0, len(ip_addresses))

neutron/tests/unit/agent/linux/test_ip_lib.py

@@ -24,7 +24,6 @@ from neutron_lib import exceptions
 from oslo_utils import netutils
 from oslo_utils import uuidutils
 import pyroute2
-from pyroute2.netlink.rtnl import ifaddrmsg
 from pyroute2.netlink.rtnl import ifinfmsg
 from pyroute2.netlink.rtnl import ndmsg
 from pyroute2 import NetlinkError
@@ -1603,39 +1602,6 @@ class ListIpRulesTestCase(base.BaseTestCase):
         self.assertEqual(reference, retval)
 
 
-class ParseLinkDeviceTestCase(base.BaseTestCase):
-
-    def setUp(self):
-        super(ParseLinkDeviceTestCase, self).setUp()
-        self._mock_get_ip_addresses = mock.patch.object(priv_lib,
-                                                        'get_ip_addresses')
-        self.mock_get_ip_addresses = self._mock_get_ip_addresses.start()
-        self.addCleanup(self._stop_mock)
-
-    def _stop_mock(self):
-        self._mock_get_ip_addresses.stop()
-
-    def test_parse_link_devices(self):
-        device = ({'index': 1, 'attrs': [['IFLA_IFNAME', 'int_name']]})
-        self.mock_get_ip_addresses.return_value = [
-            {'prefixlen': 24, 'scope': 200, 'event': 'RTM_NEWADDR', 'attrs': [
-                ['IFA_ADDRESS', '192.168.10.20'],
-                ['IFA_FLAGS', ifaddrmsg.IFA_F_PERMANENT]]},
-            {'prefixlen': 64, 'scope': 200, 'event': 'RTM_DELADDR', 'attrs': [
-                ['IFA_ADDRESS', '2001:db8::1'],
-                ['IFA_FLAGS', ifaddrmsg.IFA_F_PERMANENT]]}]
-
-        retval = ip_lib._parse_link_device('namespace', device)
-        expected = [{'scope': 'site', 'cidr': '192.168.10.20/24',
-                     'dynamic': False, 'dadfailed': False, 'name': 'int_name',
-                     'broadcast': None, 'tentative': False, 'event': 'added'},
-                    {'scope': 'site', 'cidr': '2001:db8::1/64',
-                     'dynamic': False, 'dadfailed': False, 'name': 'int_name',
-                     'broadcast': None, 'tentative': False,
-                     'event': 'removed'}]
-        self.assertEqual(expected, retval)
-
-
 class GetDevicesInfoTestCase(base.BaseTestCase):
 
     DEVICE_LO = {