From 81980146cbdcc42d2398f4e777e659fc59c3b55f Mon Sep 17 00:00:00 2001 From: Miro Tomaska Date: Fri, 2 Sep 2022 10:26:11 -0500 Subject: [PATCH] Add and delete multiple ip addresses in one priv call Created new add_ip_addresses privileged function which takes an iterable of cidrs and adds them in one privileged call. This is so we dont have to take on additional priv overhead when calling add_ip_address in a loop. For parity, performed the same change on the delete_ip_address function. Closes-Bug: #1987281 Partial-Bug: #1981113 Change-Id: Ib1278af20c3b3b057712453cb249aba34b684a21 --- neutron/agent/linux/ip_lib.py | 37 ++++++++-- neutron/agent/ovn/metadata/agent.py | 24 ++++--- neutron/common/utils.py | 23 ++++++- neutron/privileged/agent/linux/ip_lib.py | 53 +++++++++++++++ .../functional/agent/linux/test_ip_lib.py | 67 +++++++++++++++++-- .../unit/agent/ovn/metadata/test_agent.py | 12 ++-- 6 files changed, 189 insertions(+), 27 deletions(-) diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index f0f59124a40..3373395354e 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -536,9 +536,16 @@ class IpAddrCommand(IpDeviceCommandBase): add_ip_address(cidr, self.name, self._parent.namespace, scope, add_broadcast) + def add_multiple(self, cidrs, scope='global', add_broadcast=True): + add_ip_addresses(cidrs, self.name, self._parent.namespace, scope, + add_broadcast) + def delete(self, cidr): delete_ip_address(cidr, self.name, self._parent.namespace) + def delete_multiple(self, cidrs): + delete_ip_addresses(cidrs, self.name, self._parent.namespace) + def flush(self, ip_version): flush_ip_addresses(ip_version, self.name, self._parent.namespace) @@ -814,15 +821,27 @@ def add_ip_address(cidr, device, namespace=None, scope='global', """ net = netaddr.IPNetwork(cidr) broadcast = None - if add_broadcast and net.version == 4: - # NOTE(slaweq): in case if cidr is /32 net.broadcast is None so - # same IP address as cidr should be set as broadcast - broadcast = str(net.broadcast or net.ip) + if add_broadcast: + broadcast = common_utils.cidr_broadcast_address_alternative(cidr) privileged.add_ip_address( net.version, str(net.ip), net.prefixlen, device, namespace, scope, broadcast) +def add_ip_addresses(cidrs, device, namespace=None, scope='global', + add_broadcast=True): + """Add multiple IP addresses. + + :param cidrs: A list of IP addresses to add, in CIDR notation + :param device: Device name to use in adding address + :param namespace: The name of the namespace in which to add the address + :param scope: scope of address being added + :param add_broadcast: should broadcast address be added + """ + privileged.add_ip_addresses( + cidrs, device, namespace, scope, add_broadcast) + + def delete_ip_address(cidr, device, namespace=None): """Delete an IP address. @@ -835,6 +854,16 @@ def delete_ip_address(cidr, device, namespace=None): net.version, str(net.ip), net.prefixlen, device, namespace) +def delete_ip_addresses(cidrs, device, namespace=None): + """Delete multiple IP address. + + :param cidrs: A list of IP addresses to delete, in CIDR notation + :param device: Device name to use in deleting address + :param namespace: The name of the namespace in which to delete the address + """ + privileged.delete_ip_addresses(cidrs, device, namespace) + + def flush_ip_addresses(ip_version, device, namespace=None): """Flush all IP addresses. diff --git a/neutron/agent/ovn/metadata/agent.py b/neutron/agent/ovn/metadata/agent.py index a60ad26d964..e685b2b0955 100644 --- a/neutron/agent/ovn/metadata/agent.py +++ b/neutron/agent/ovn/metadata/agent.py @@ -483,16 +483,20 @@ class MetadataAgent(object): # Configure the IP addresses on the VETH pair and remove those # that we no longer need. current_cidrs = {dev['cidr'] for dev in dev_info} - for ipaddr in current_cidrs - metadata_port.ip_addresses: - ip2.addr.delete(ipaddr) - for ipaddr in metadata_port.ip_addresses - current_cidrs: - # NOTE(dalvarez): metadata only works on IPv4. We're doing this - # extra check here because it could be that the metadata port has - # an IPv6 address if there's an IPv6 subnet with SLAAC in its - # network. Neutron IPAM will autoallocate an IPv6 address for every - # port in the network. - if utils.get_ip_version(ipaddr) == 4: - ip2.addr.add(ipaddr) + cidrs_to_delete = list(current_cidrs - metadata_port.ip_addresses) + if cidrs_to_delete: + ip2.addr.delete_multiple(cidrs_to_delete) + + # NOTE(dalvarez): metadata only works on IPv4. We're doing this + # extra check here because it could be that the metadata port has + # an IPv6 address if there's an IPv6 subnet with SLAAC in its + # network. Neutron IPAM will autoallocate an IPv6 address for every + # port in the network. + ipv4_cidrs = [] + for cidr in metadata_port.ip_addresses - current_cidrs: + if utils.get_ip_version(cidr) == n_const.IP_VERSION_4: + ipv4_cidrs.append(cidr) + ip2.addr.add_multiple(ipv4_cidrs) # Check that this port is not attached to any other OVS bridge. This # can happen when the OVN bridge changes (for example, during a diff --git a/neutron/common/utils.py b/neutron/common/utils.py index 0f861338bb6..92921f648fc 100644 --- a/neutron/common/utils.py +++ b/neutron/common/utils.py @@ -311,14 +311,33 @@ def cidr_broadcast_address(cidr): return str(broadcast) +def cidr_broadcast_address_alternative(cidr): + """Wraps cidr_broadcast address and handles last cidr differently in + respect to netaddr + + :param cidr: (string, netaddr.IPNetwork, netaddr.IPAddress) either an ipv4 + or ipv6 cidr. + :returns: (string) broadcast address of the cidr, + IP of the cidr if IPv4 /32 CIDR, otherwise None + """ + broadcast = cidr_broadcast_address(cidr) + if broadcast: + return str(broadcast) + net = netaddr.IPNetwork(cidr) + if net.prefixlen == 32 and net.version == n_const.IP_VERSION_4: + return str(net.ip) + # NOTE(mtomaska): netaddr ver >= 0.9.0 will start returning None + # for IPv6 cidr /127 /128 + + def get_ip_version(ip_or_cidr): return netaddr.IPNetwork(ip_or_cidr).version def ip_version_from_int(ip_version_int): - if ip_version_int == 4: + if ip_version_int == n_const.IP_VERSION_4: return n_const.IPv4 - if ip_version_int == 6: + if ip_version_int == n_const.IP_VERSION_6: return n_const.IPv6 raise ValueError(_('Illegal IP version number')) diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index 7ad3af03349..71ace7e2586 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -14,6 +14,7 @@ import errno import os import socket +import netaddr from neutron_lib import constants from oslo_log import log as logging import pyroute2 @@ -27,6 +28,7 @@ from pyroute2 import netns # pylint: disable=no-name-in-module import tenacity from neutron._i18n import _ +from neutron.common import utils as common_utils from neutron import privileged from neutron.privileged.agent import linux as priv_linux @@ -272,6 +274,32 @@ def add_ip_address(ip_version, ip, prefixlen, device, namespace, scope, raise +@privileged.default.entrypoint +def add_ip_addresses(cidrs, device, namespace, scope, + add_broadcast=True): + for cidr in cidrs: + net = netaddr.IPNetwork(cidr) + ip = str(net.ip) + prefixlen = net.prefixlen + family = _IP_VERSION_FAMILY_MAP[net.version] + broadcast = None + if add_broadcast: + broadcast = common_utils.cidr_broadcast_address_alternative(cidr) + try: + _run_iproute_addr('add', + device, + namespace, + address=ip, + mask=prefixlen, + family=family, + broadcast=broadcast, + scope=get_scope_name(scope)) + except netlink_exceptions.NetlinkError as e: + if e.code == errno.EEXIST: + raise IpAddressAlreadyExists(ip=ip, device=device) + raise + + @privileged.default.entrypoint def delete_ip_address(ip_version, ip, prefixlen, device, namespace): family = _IP_VERSION_FAMILY_MAP[ip_version] @@ -292,6 +320,31 @@ def delete_ip_address(ip_version, ip, prefixlen, device, namespace): raise +@privileged.default.entrypoint +def delete_ip_addresses(cidrs, device, namespace): + for cidr in cidrs: + net = netaddr.IPNetwork(cidr) + ip = str(net.ip) + prefixlen = net.prefixlen + family = _IP_VERSION_FAMILY_MAP[net.version] + try: + _run_iproute_addr("delete", + device, + namespace, + address=ip, + mask=prefixlen, + family=family) + except netlink_exceptions.NetlinkError as e: + # when trying to delete a non-existent IP address, pyroute2 raises + # NetlinkError with code EADDRNOTAVAIL (99, 'Cannot assign + # requested address') + # this shouldn't raise an error + if e.code == errno.EADDRNOTAVAIL: + pass + else: + raise + + @privileged.default.entrypoint def flush_ip_addresses(ip_version, device, namespace): family = _IP_VERSION_FAMILY_MAP[ip_version] diff --git a/neutron/tests/functional/agent/linux/test_ip_lib.py b/neutron/tests/functional/agent/linux/test_ip_lib.py index 540d8a8dbd5..2e9f40aa0c1 100644 --- a/neutron/tests/functional/agent/linux/test_ip_lib.py +++ b/neutron/tests/functional/agent/linux/test_ip_lib.py @@ -613,6 +613,9 @@ class IpLibTestCase(IpLibTestFramework): cidr = netaddr.IPNetwork(ip_address['cidr']) self.assertNotEqual(ip_version, cidr.version) + def _get_cidrs_from_device(self, device_obj): + return [ip_info['cidr'] for ip_info in device_obj.addr.list()] + def test_add_ip_address(self): ip_addresses = [ (netaddr.IPNetwork("10.10.10.10/30"), "global", '10.10.10.11'), @@ -629,22 +632,78 @@ class IpLibTestCase(IpLibTestFramework): self.assertRaises(RuntimeError, device.addr.add, str(ip_address[0]), ip_address[1]) + def test_add_ip_addresses(self): + expected_cidrs = [ + "10.10.10.10/30", + "11.11.11.11/28", + "2801::1/120", + "fe80::/64" + ] + attr = self.generate_device_details(ip_cidrs=[]) + device = self.manage_device(attr) + + device.addr.add_multiple(expected_cidrs) + + self.assertListEqual( + expected_cidrs, + self._get_cidrs_from_device(device) + ) + def test_delete_ip_address(self): attr = self.generate_device_details() cidr = attr.ip_cidrs[0] device = self.manage_device(attr) - device_cidrs = [ip_info['cidr'] for ip_info in device.addr.list()] - self.assertIn(cidr, device_cidrs) + device_cidrs_before_delete = self._get_cidrs_from_device(device) + self.assertIn(cidr, device_cidrs_before_delete) device.addr.delete(cidr) - device_cidrs = [ip_info['cidr'] for ip_info in device.addr.list()] - self.assertNotIn(cidr, device_cidrs) + device_cidrs_after_delete = self._get_cidrs_from_device(device) + self.assertNotIn(cidr, device_cidrs_after_delete) # Try to delete not existing IP address, it should be just fine and # finish without any error raised device.addr.delete(cidr) + def test_delete_all_ip_addresses(self): + cidrs = [ + "10.10.10.10/30", + "11.11.11.11/28", + "2801::1/120", + "fe80::/64" + ] + attr = self.generate_device_details(ip_cidrs=cidrs) + device = self.manage_device(attr) + + device_cidrs_before_delete = self._get_cidrs_from_device(device) + self.assertCountEqual(cidrs, device_cidrs_before_delete) + + device.addr.delete_multiple(cidrs) + + self.assertEqual(0, len(device.addr.list())) + + def test_delete_some_ip_addresses(self): + cidrs = [ + "10.10.10.10/30", + "11.11.11.11/28", + "2801::1/120", + "fe80::/64" + ] + attr = self.generate_device_details(ip_cidrs=cidrs) + device = self.manage_device(attr) + + device_cidrs_before_delete = self._get_cidrs_from_device(device) + self.assertCountEqual(cidrs, device_cidrs_before_delete) + + # delete the last two cidrs + device.addr.delete_multiple(cidrs[-2:]) + + # confirm only remaining cidrs are present + self.assertCountEqual( + cidrs[:2], + self._get_cidrs_from_device(device) + ) + def test_flush_ip_addresses(self): ip_addresses = [ (netaddr.IPNetwork("10.10.10.10/30"), "global", '10.10.10.11'), diff --git a/neutron/tests/unit/agent/ovn/metadata/test_agent.py b/neutron/tests/unit/agent/ovn/metadata/test_agent.py index 041433a3758..31265eaa254 100644 --- a/neutron/tests/unit/agent/ovn/metadata/test_agent.py +++ b/neutron/tests/unit/agent/ovn/metadata/test_agent.py @@ -231,7 +231,8 @@ class TestMetadataAgent(base.BaseTestCase): mock.patch.object(ip_link, 'set_up') as link_set_up,\ mock.patch.object(ip_link, 'set_address') as link_set_addr,\ mock.patch.object(ip_addr, 'list', return_value=[]),\ - mock.patch.object(ip_addr, 'add') as ip_addr_add,\ + mock.patch.object( + ip_addr, 'add_multiple') as ip_addr_add_multiple,\ mock.patch.object( ip_wrap, 'add_veth', return_value=[ip_lib.IPDevice('ip1'), @@ -261,12 +262,9 @@ class TestMetadataAgent(base.BaseTestCase): 'br-int', 'veth_0') self.agent.ovs_idl.db_set.assert_called_once_with( 'Interface', 'veth_0', ('external_ids', {'iface-id': 'port'})) - # Check that the metadata port has the IP addresses properly - # configured and that IPv6 address has been skipped. - expected_calls = [mock.call('10.0.0.1/23'), - mock.call(n_const.METADATA_CIDR)] - self.assertEqual(sorted(expected_calls), - sorted(ip_addr_add.call_args_list)) + expected_call = [n_const.METADATA_CIDR, '10.0.0.1/23'] + self.assertCountEqual(expected_call, + ip_addr_add_multiple.call_args.args[0]) # Check that metadata proxy has been spawned spawn_mdp.assert_called_once_with( mock.ANY, 'namespace', 80, mock.ANY,