From 37d50923b9849455fa219837a0d9a8e11e952687 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Mon, 18 Nov 2013 23:32:19 +0000 Subject: [PATCH] Simplify ip allocation/recycling to relieve db pressure I found that multiple calls to delete_port can pile up on the _recycle_ip operation. This patch simplifies this operation. It reduces the _recycle_ip operation to a single row delete in the ip allocations table and doesn't touch the availability table. To acheive the recycling of ips in a pool, this code runs a more complex operation of rebuilding the availability table when it is exhausted. Only one API process will perform this more expensive operation and others waiting for allocation will immediately benefit. The amortized cost of this operation is much less than the cumulative cost of running the more expensive _recycle_ip operation for every port delete. IP allocation behaves a bit differently with this patch. Instead of giving out the first IP available in a pool, the entire pool will be allocated before wrapping around and recycling ip addresses that have been released. This is a desirable feature as it puts ip addresses in a sort of quarantine after they are released. It is easier to distinguish newly allocated ips from old ones. Change-Id: Ia55b66128de9986e075b0f87acc401d211cd91d3 Closes-Bug: #1252506 Closes-Bug: #1257815 --- neutron/db/db_base_plugin_v2.py | 141 ++++++---------- neutron/db/models_v2.py | 9 +- neutron/tests/unit/nicira/test_networkgw.py | 29 ---- neutron/tests/unit/test_db_plugin.py | 175 ++++++++++---------- 4 files changed, 144 insertions(+), 210 deletions(-) diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 0eaa237d72..f3a1df19f9 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -16,7 +16,6 @@ # under the License. import datetime -import itertools import random import netaddr @@ -325,92 +324,6 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, """Return an IP address to the pool of free IP's on the network subnet. """ - # Grab all allocation pools for the subnet - allocation_pools = (context.session.query( - models_v2.IPAllocationPool).filter_by(subnet_id=subnet_id). - options(orm.joinedload('available_ranges', innerjoin=True)). - with_lockmode('update')) - # If there are no available ranges the previous query will return no - # results as it uses an inner join to avoid errors with the postgresql - # backend (see lp bug 1215350). In this case IP allocation pools must - # be loaded with a different query, which does not require lock for - # update as the allocation pools for a subnet are immutable. - # The 2nd query will be executed only if the first yields no results - unlocked_allocation_pools = (context.session.query( - models_v2.IPAllocationPool).filter_by(subnet_id=subnet_id)) - - # Find the allocation pool for the IP to recycle - pool_id = None - - for allocation_pool in itertools.chain(allocation_pools, - unlocked_allocation_pools): - allocation_pool_range = netaddr.IPRange( - allocation_pool['first_ip'], allocation_pool['last_ip']) - if netaddr.IPAddress(ip_address) in allocation_pool_range: - pool_id = allocation_pool['id'] - break - if not pool_id: - NeutronDbPluginV2._delete_ip_allocation( - context, network_id, subnet_id, ip_address) - return - # Two requests will be done on the database. The first will be to - # search if an entry starts with ip_address + 1 (r1). The second - # will be to see if an entry ends with ip_address -1 (r2). - # If 1 of the above holds true then the specific entry will be - # modified. If both hold true then the two ranges will be merged. - # If there are no entries then a single entry will be added. - range_qry = context.session.query( - models_v2.IPAvailabilityRange).with_lockmode('update') - ip_first = str(netaddr.IPAddress(ip_address) + 1) - ip_last = str(netaddr.IPAddress(ip_address) - 1) - LOG.debug(_("Recycle %s"), ip_address) - try: - r1 = range_qry.filter_by(allocation_pool_id=pool_id, - first_ip=ip_first).one() - LOG.debug(_("Recycle: first match for %(first_ip)s-%(last_ip)s"), - {'first_ip': r1['first_ip'], 'last_ip': r1['last_ip']}) - except exc.NoResultFound: - r1 = [] - try: - r2 = range_qry.filter_by(allocation_pool_id=pool_id, - last_ip=ip_last).one() - LOG.debug(_("Recycle: last match for %(first_ip)s-%(last_ip)s"), - {'first_ip': r2['first_ip'], 'last_ip': r2['last_ip']}) - except exc.NoResultFound: - r2 = [] - - if r1 and r2: - # Merge the two ranges - ip_range = models_v2.IPAvailabilityRange( - allocation_pool_id=pool_id, - first_ip=r2['first_ip'], - last_ip=r1['last_ip']) - context.session.add(ip_range) - LOG.debug(_("Recycle: merged %(first_ip1)s-%(last_ip1)s and " - "%(first_ip2)s-%(last_ip2)s"), - {'first_ip1': r2['first_ip'], 'last_ip1': r2['last_ip'], - 'first_ip2': r1['first_ip'], 'last_ip2': r1['last_ip']}) - context.session.delete(r1) - context.session.delete(r2) - elif r1: - # Update the range with matched first IP - r1['first_ip'] = ip_address - LOG.debug(_("Recycle: updated first %(first_ip)s-%(last_ip)s"), - {'first_ip': r1['first_ip'], 'last_ip': r1['last_ip']}) - elif r2: - # Update the range with matched last IP - r2['last_ip'] = ip_address - LOG.debug(_("Recycle: updated last %(first_ip)s-%(last_ip)s"), - {'first_ip': r2['first_ip'], 'last_ip': r2['last_ip']}) - else: - # Create a new range - ip_range = models_v2.IPAvailabilityRange( - allocation_pool_id=pool_id, - first_ip=ip_address, - last_ip=ip_address) - context.session.add(ip_range) - LOG.debug(_("Recycle: created new %(first_ip)s-%(last_ip)s"), - {'first_ip': ip_address, 'last_ip': ip_address}) NeutronDbPluginV2._delete_ip_allocation(context, network_id, subnet_id, ip_address) @@ -449,6 +362,15 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, @staticmethod def _generate_ip(context, subnets): + try: + return NeutronDbPluginV2._try_generate_ip(context, subnets) + except q_exc.IpAddressGenerationFailure: + NeutronDbPluginV2._rebuild_availability_ranges(context, subnets) + + return NeutronDbPluginV2._try_generate_ip(context, subnets) + + @staticmethod + def _try_generate_ip(context, subnets): """Generate an IP address. The IP address will be generated from one of the subnets defined on @@ -481,6 +403,51 @@ class NeutronDbPluginV2(neutron_plugin_base_v2.NeutronPluginBaseV2, return {'ip_address': ip_address, 'subnet_id': subnet['id']} raise q_exc.IpAddressGenerationFailure(net_id=subnets[0]['network_id']) + @staticmethod + def _rebuild_availability_ranges(context, subnets): + ip_qry = context.session.query( + models_v2.IPAllocation).with_lockmode('update') + # PostgreSQL does not support select...for update with an outer join. + # No join is needed here. + pool_qry = context.session.query( + models_v2.IPAllocationPool).options( + orm.noload('available_ranges')).with_lockmode('update') + for subnet in sorted(subnets): + LOG.debug(_("Rebuilding availability ranges for subnet %s") + % subnet) + + # Create a set of all currently allocated addresses + ip_qry_results = ip_qry.filter_by(subnet_id=subnet['id']) + allocations = netaddr.IPSet([netaddr.IPAddress(i['ip_address']) + for i in ip_qry_results]) + + for pool in pool_qry.filter_by(subnet_id=subnet['id']): + # Create a set of all addresses in the pool + poolset = netaddr.IPSet(netaddr.iter_iprange(pool['first_ip'], + pool['last_ip'])) + + # Use set difference to find free addresses in the pool + available = poolset - allocations + + # Generator compacts an ip set into contiguous ranges + def ipset_to_ranges(ipset): + first, last = None, None + for cidr in ipset.iter_cidrs(): + if last and last + 1 != cidr.first: + yield netaddr.IPRange(first, last) + first = None + first, last = first if first else cidr.first, cidr.last + if first: + yield netaddr.IPRange(first, last) + + # Write the ranges to the db + for range in ipset_to_ranges(available): + available_range = models_v2.IPAvailabilityRange( + allocation_pool_id=pool['id'], + first_ip=str(netaddr.IPAddress(range.first)), + last_ip=str(netaddr.IPAddress(range.last))) + context.session.add(available_range) + @staticmethod def _allocate_specific_ip(context, subnet_id, ip_address): """Allocate a specific IP address on the subnet.""" diff --git a/neutron/db/models_v2.py b/neutron/db/models_v2.py index 992bd1374a..1d55ff7fdf 100644 --- a/neutron/db/models_v2.py +++ b/neutron/db/models_v2.py @@ -50,10 +50,11 @@ class IPAvailabilityRange(model_base.BASEV2): Allocation - first entry from the range will be allocated. If the first entry is equal to the last entry then this row will be deleted. - Recycling ips involves appending to existing ranges. This is - only done if the range is contiguous. If not, the first_ip will be - the same as the last_ip. When adjacent ips are recycled the ranges - will be merged. + Recycling ips involves reading the IPAllocationPool and IPAllocation tables + and inserting ranges representing available ips. This happens after the + final allocation is pulled from this table and a new ip allocation is + requested. Any contiguous ranges of available ips will be inserted as a + single range. """ allocation_pool_id = sa.Column(sa.String(36), diff --git a/neutron/tests/unit/nicira/test_networkgw.py b/neutron/tests/unit/nicira/test_networkgw.py index 2f60eb1338..19f66bd82d 100644 --- a/neutron/tests/unit/nicira/test_networkgw.py +++ b/neutron/tests/unit/nicira/test_networkgw.py @@ -521,35 +521,6 @@ class NetworkGatewayDbTestCase(test_db_plugin.NeutronDbPluginV2TestCase): 'vlan', 555, expected_status=exc.HTTPBadRequest.code) - def test_connect_network_does_not_waste_ips(self): - # Ensure address is immediately recycled - cfg.CONF.set_override('dhcp_lease_duration', -1) - with self._network_gateway() as gw: - with self.network() as net: - with self.subnet(network=net) as sub: - with self.port(subnet=sub) as port_1: - expected_ips = port_1['port']['fixed_ips'] - # port_1 has now been deleted - body = self._gateway_action('connect', - gw[self.resource]['id'], - net['network']['id'], - 'flat') - gw_port_id = body['connection_info']['port_id'] - gw_port_body = self._show('ports', gw_port_id) - self.assertEqual(gw[self.resource]['id'], - gw_port_body['port']['device_id']) - self.assertEqual([], gw_port_body['port']['fixed_ips']) - # Verify a new port gets same address as port_1 - # This will confirm the gateway port did not waste an ip - with self.port(subnet=sub) as port_2: - self.assertEqual(expected_ips, - port_2['port']['fixed_ips']) - # Clean up - otherwise delete will fail - self._gateway_action('disconnect', - gw[self.resource]['id'], - net['network']['id'], - 'flat') - def test_disconnect_network_ambiguous_returns_409(self): with self._network_gateway() as gw: with self.network() as net_1: diff --git a/neutron/tests/unit/test_db_plugin.py b/neutron/tests/unit/test_db_plugin.py index 665746f9c5..2e12ec491b 100644 --- a/neutron/tests/unit/test_db_plugin.py +++ b/neutron/tests/unit/test_db_plugin.py @@ -17,12 +17,9 @@ import contextlib import copy -import datetime import os -import random import mock -import netaddr from oslo.config import cfg from testtools import matchers import webob.exc @@ -42,7 +39,6 @@ from neutron.db import db_base_plugin_v2 from neutron.db import models_v2 from neutron.manager import NeutronManager from neutron.openstack.common import importutils -from neutron.openstack.common import timeutils from neutron.tests import base from neutron.tests.unit import test_extensions from neutron.tests.unit import testlib_api @@ -1246,9 +1242,9 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s data['port']['admin_state_up']) ips = res['port']['fixed_ips'] self.assertEqual(len(ips), 2) - self.assertEqual(ips[0]['ip_address'], '10.0.0.2') + self.assertEqual(ips[0]['ip_address'], '10.0.0.3') self.assertEqual(ips[0]['subnet_id'], subnet['subnet']['id']) - self.assertEqual(ips[1]['ip_address'], '10.0.0.3') + self.assertEqual(ips[1]['ip_address'], '10.0.0.4') self.assertEqual(ips[1]['subnet_id'], subnet['subnet']['id']) def test_requested_duplicate_mac(self): @@ -1603,39 +1599,6 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s for p in ports_to_delete: self._delete('ports', p['port']['id']) - def test_recycling(self): - # set expirations to past so that recycling is checked - reference = datetime.datetime(2012, 8, 13, 23, 11, 0) - cfg.CONF.set_override('dhcp_lease_duration', 0) - - with self.subnet(cidr='10.0.1.0/24') as subnet: - with self.port(subnet=subnet) as port: - with mock.patch.object(timeutils, 'utcnow') as mock_utcnow: - mock_utcnow.return_value = reference - ips = port['port']['fixed_ips'] - self.assertEqual(len(ips), 1) - self.assertEqual(ips[0]['ip_address'], '10.0.1.2') - self.assertEqual(ips[0]['subnet_id'], - subnet['subnet']['id']) - net_id = port['port']['network_id'] - ports = [] - for i in range(16 - 3): - res = self._create_port(self.fmt, net_id=net_id) - p = self.deserialize(self.fmt, res) - ports.append(p) - for i in range(16 - 3): - x = random.randrange(0, len(ports), 1) - p = ports.pop(x) - self._delete('ports', p['port']['id']) - res = self._create_port(self.fmt, net_id=net_id) - port = self.deserialize(self.fmt, res) - ips = port['port']['fixed_ips'] - self.assertEqual(len(ips), 1) - self.assertEqual(ips[0]['ip_address'], '10.0.1.3') - self.assertEqual(ips[0]['subnet_id'], - subnet['subnet']['id']) - self._delete('ports', port['port']['id']) - def test_invalid_admin_state(self): with self.network() as network: data = {'port': {'network_id': network['network']['id'], @@ -1671,57 +1634,6 @@ fixed_ips=ip_address%%3D%s&fixed_ips=ip_address%%3D%s&fixed_ips=subnet_id%%3D%s 120) self.assertTrue(log.mock_calls) - def _test_recycle_ip_address(self, ip_to_recycle, allocation_pools=None): - plugin = NeutronManager.get_plugin() - if not allocation_pools: - allocation_pools = [{"start": '10.0.0.10', - "end": '10.0.0.50'}] - with self.subnet(cidr='10.0.0.0/24', - allocation_pools=allocation_pools) as subnet: - network_id = subnet['subnet']['network_id'] - subnet_id = subnet['subnet']['id'] - fixed_ips = [{"subnet_id": subnet_id, - "ip_address": ip_to_recycle}] - with self.port(subnet=subnet, fixed_ips=fixed_ips) as port: - ctx = context.Context('', port['port']['tenant_id']) - ip_address = port['port']['fixed_ips'][0]['ip_address'] - plugin._recycle_ip(ctx, network_id, subnet_id, ip_address) - - q = ctx.session.query(models_v2.IPAllocation) - q = q.filter_by(subnet_id=subnet_id) - self.assertEqual(q.count(), 0) - # If the IP address is in the allocation pool also verify the - # address is returned to the availability range - for allocation_pool in allocation_pools: - allocation_pool_range = netaddr.IPRange( - allocation_pool['start'], allocation_pool['end']) - if netaddr.IPAddress(ip_to_recycle) in allocation_pool_range: - # Do not worry about no result found exception - pool = ctx.session.query( - models_v2.IPAllocationPool).filter_by( - subnet_id=subnet_id).one() - ip_av_range = ctx.session.query( - models_v2.IPAvailabilityRange).filter_by( - allocation_pool_id=pool['id']).first() - self.assertIsNotNone(ip_av_range) - self.assertIn(netaddr.IPAddress(ip_to_recycle), - netaddr.IPRange(ip_av_range['first_ip'], - ip_av_range['last_ip'])) - - def test_recycle_ip_address_outside_allocation_pool(self): - self._test_recycle_ip_address('10.0.0.100') - - def test_recycle_ip_address_in_allocation_pool(self): - self._test_recycle_ip_address('10.0.0.20') - - def test_recycle_ip_address_on_exhausted_allocation_pool(self): - # Perform the recycle ip address on a subnet with a single address - # in the pool to verify the corner case exposed by bug 1240353 - self._test_recycle_ip_address( - '10.0.0.20', - allocation_pools=[{'start': '10.0.0.20', - 'end': '10.0.0.20'}]) - def test_max_fixed_ips_exceeded(self): with self.subnet(gateway_ip='10.0.0.3', cidr='10.0.0.0/24') as subnet: @@ -3600,6 +3512,89 @@ class DbModelTestCase(base.BaseTestCase): self.assertEqual(actual_repr_output, final_exp) +class TestNeutronDbPluginV2(base.BaseTestCase): + """Unit Tests for NeutronDbPluginV2 IPAM Logic.""" + + def test_generate_ip(self): + with mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, + '_try_generate_ip') as generate: + with mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, + '_rebuild_availability_ranges') as rebuild: + + db_base_plugin_v2.NeutronDbPluginV2._generate_ip('c', 's') + + generate.assert_called_once_with('c', 's') + self.assertEqual(0, rebuild.call_count) + + def test_generate_ip_exhausted_pool(self): + with mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, + '_try_generate_ip') as generate: + with mock.patch.object(db_base_plugin_v2.NeutronDbPluginV2, + '_rebuild_availability_ranges') as rebuild: + + exception = q_exc.IpAddressGenerationFailure(net_id='n') + generate.side_effect = exception + + # I want the side_effect to throw an exception once but I + # didn't see a way to do this. So, let it throw twice and + # catch the second one. Check below to ensure that + # _try_generate_ip was called twice. + try: + db_base_plugin_v2.NeutronDbPluginV2._generate_ip('c', 's') + except q_exc.IpAddressGenerationFailure: + pass + + self.assertEqual(2, generate.call_count) + rebuild.assert_called_once_with('c', 's') + + def test_rebuild_availability_ranges(self): + pools = [{'id': 'a', + 'first_ip': '192.168.1.3', + 'last_ip': '192.168.1.10'}, + {'id': 'b', + 'first_ip': '192.168.1.100', + 'last_ip': '192.168.1.120'}] + + allocations = [{'ip_address': '192.168.1.3'}, + {'ip_address': '192.168.1.78'}, + {'ip_address': '192.168.1.7'}, + {'ip_address': '192.168.1.110'}, + {'ip_address': '192.168.1.11'}, + {'ip_address': '192.168.1.4'}, + {'ip_address': '192.168.1.111'}] + + ip_qry = mock.Mock() + ip_qry.with_lockmode.return_value = ip_qry + ip_qry.filter_by.return_value = allocations + + pool_qry = mock.Mock() + pool_qry.options.return_value = pool_qry + pool_qry.with_lockmode.return_value = pool_qry + pool_qry.filter_by.return_value = pools + + def return_queries_side_effect(*args, **kwargs): + if args[0] == models_v2.IPAllocation: + return ip_qry + if args[0] == models_v2.IPAllocationPool: + return pool_qry + + context = mock.Mock() + context.session.query.side_effect = return_queries_side_effect + subnets = [mock.MagicMock()] + + db_base_plugin_v2.NeutronDbPluginV2._rebuild_availability_ranges( + context, subnets) + + actual = [[args[0].allocation_pool_id, + args[0].first_ip, args[0].last_ip] + for _name, args, _kwargs in context.session.add.mock_calls] + + self.assertEqual([['a', '192.168.1.5', '192.168.1.6'], + ['a', '192.168.1.8', '192.168.1.10'], + ['b', '192.168.1.100', '192.168.1.109'], + ['b', '192.168.1.112', '192.168.1.120']], actual) + + class NeutronDbPluginV2AsMixinTestCase(base.BaseTestCase): """Tests for NeutronDbPluginV2 as Mixin.