From 67984850228f6f26a72504b9f464a5fbcaac59e6 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Fri, 29 Jul 2016 10:10:48 -0600 Subject: [PATCH] Avoid IPAM driver reusing a session that has been rolled back With the in-tree pluggable IPAM driver, IPAM rollback tries to use the DB session after it has been rolled back due to an exception. This driver doesn't need roll back, so fix this by adding a method to the driver signalling that rollback shouldn't be attempted. Change-Id: Ic254789e58a8a51cd1aa943cb71de12410f4c0a7 Closes-Bug: #1603162 Related-Bug: #1516156 --- neutron/db/ipam_pluggable_backend.py | 19 ++++- neutron/ipam/driver.py | 12 ++++ neutron/ipam/drivers/neutrondb_ipam/driver.py | 3 + .../unit/db/test_ipam_pluggable_backend.py | 72 +++++++++++++++++++ 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/neutron/db/ipam_pluggable_backend.py b/neutron/db/ipam_pluggable_backend.py index 666ef6caa5d..ff600af0461 100644 --- a/neutron/db/ipam_pluggable_backend.py +++ b/neutron/db/ipam_pluggable_backend.py @@ -74,6 +74,9 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): ip) except Exception: with excutils.save_and_reraise_exception(): + if not ipam_driver.needs_rollback(): + return + LOG.debug("An exception occurred during IP deallocation.") if revert_on_fail and deallocated: LOG.debug("Reverting deallocation") @@ -124,6 +127,9 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): 'subnet_id': subnet_id}) except Exception: with excutils.save_and_reraise_exception(): + if not ipam_driver.needs_rollback(): + return + LOG.debug("An exception occurred during IP allocation.") if revert_on_fail and allocated: @@ -174,9 +180,12 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): except Exception: with excutils.save_and_reraise_exception(): if ips: + ipam_driver = driver.Pool.get_instance(None, context) + if not ipam_driver.needs_rollback(): + return + LOG.debug("An exception occurred during port creation. " "Reverting IP allocation") - ipam_driver = driver.Pool.get_instance(None, context) self._safe_rollback(self._ipam_deallocate_ips, context, ipam_driver, port_copy['port'], ips, revert_on_fail=False) @@ -333,8 +342,11 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): except Exception: with excutils.save_and_reraise_exception(): if 'fixed_ips' in new_port: - LOG.debug("An exception occurred during port update.") ipam_driver = driver.Pool.get_instance(None, context) + if not ipam_driver.needs_rollback(): + return + + LOG.debug("An exception occurred during port update.") if changes.add: LOG.debug("Reverting IP allocation.") self._safe_rollback(self._ipam_deallocate_ips, @@ -464,6 +476,9 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin): # IPAM part rolled back in exception handling # and subnet part is rolled back by transaction rollback. with excutils.save_and_reraise_exception(): + if not ipam_driver.needs_rollback(): + return + LOG.debug("An exception occurred during subnet creation. " "Reverting subnet allocation.") self._safe_rollback(self.delete_subnet, diff --git a/neutron/ipam/driver.py b/neutron/ipam/driver.py index 9519426746f..32aa049cc6d 100644 --- a/neutron/ipam/driver.py +++ b/neutron/ipam/driver.py @@ -119,6 +119,18 @@ class Pool(object): :raises: TODO(Carl) What sort of errors do we need to plan for? """ + def needs_rollback(self): + """Whether driver needs an explicit rollback when operations fail. + + A driver that (de)allocates resources in the same DB transaction passed + to it by Neutron will not want explicit rollback. A truly external IPAM + system would need to return True for sure. The default is True since + all drivers were assumed to be designed to need it from the start. + + :returns: True if driver needs to be called on rollback + """ + return True + @six.add_metaclass(abc.ABCMeta) class Subnet(object): diff --git a/neutron/ipam/drivers/neutrondb_ipam/driver.py b/neutron/ipam/drivers/neutrondb_ipam/driver.py index bd8f43ee96f..fdbfd1dd4b3 100644 --- a/neutron/ipam/drivers/neutrondb_ipam/driver.py +++ b/neutron/ipam/drivers/neutrondb_ipam/driver.py @@ -314,3 +314,6 @@ class NeutronDbPool(subnet_alloc.SubnetAllocator): "Neutron subnet %s does not exist"), subnet_id) raise n_exc.SubnetNotFound(subnet_id=subnet_id) + + def needs_rollback(self): + return False diff --git a/neutron/tests/unit/db/test_ipam_pluggable_backend.py b/neutron/tests/unit/db/test_ipam_pluggable_backend.py index abd0f7d1438..ffdcdcbcfb5 100644 --- a/neutron/tests/unit/db/test_ipam_pluggable_backend.py +++ b/neutron/tests/unit/db/test_ipam_pluggable_backend.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import copy import mock import netaddr from neutron_lib import constants @@ -779,3 +780,74 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase): mocks['ipam']._ipam_allocate_ips.assert_called_once_with( context, mocks['driver'], db_port, changes.remove, revert_on_fail=False) + + +class TestRollback(test_db_base.NeutronDbPluginV2TestCase): + def setUp(self): + cfg.CONF.set_override('ipam_driver', 'internal') + super(TestRollback, self).setUp() + + def test_ipam_rollback_not_broken_on_session_rollback(self): + """Triggers an error that calls rollback on session.""" + with self.network() as net: + with self.subnet(network=net, cidr='10.0.1.0/24') as subnet1: + with self.subnet(network=net, cidr='10.0.2.0/24') as subnet2: + pass + + # If this test fails and this method appears in the server side stack + # trace then IPAM rollback was likely tried using a session which had + # already been rolled back by the DB exception. + def rollback(func, *args, **kwargs): + func(*args, **kwargs) + + # Ensure DBDuplicate exception is raised in the context where IPAM + # rollback is triggered. It "breaks" the session because it triggers DB + # rollback. Inserting a flush in _store_ip_allocation does this. + orig = ipam_pluggable_backend.IpamPluggableBackend._store_ip_allocation + + def store(context, ip_address, *args, **kwargs): + try: + return orig(context, ip_address, *args, **kwargs) + finally: + context.session.flush() + + # Create a port to conflict with later. Simulates a race for addresses. + result = self._create_port( + self.fmt, + net_id=net['network']['id'], + fixed_ips=[{'subnet_id': subnet1['subnet']['id']}, + {'subnet_id': subnet2['subnet']['id']}]) + port = self.deserialize(self.fmt, result) + fixed_ips = port['port']['fixed_ips'] + + # Hands out the same 2nd IP to create conflict and trigger rollback + ips = [{'subnet_id': fixed_ips[0]['subnet_id'], + 'ip_address': fixed_ips[0]['ip_address']}, + {'subnet_id': fixed_ips[1]['subnet_id'], + 'ip_address': fixed_ips[1]['ip_address']}] + + def alloc(*args, **kwargs): + def increment_address(a): + a['ip_address'] = str(netaddr.IPAddress(a['ip_address']) + 1) + # Increment 1st address to return a free address on the first call + increment_address(ips[0]) + try: + return copy.deepcopy(ips) + finally: + # Increment 2nd address to return free address on the 2nd call + increment_address(ips[1]) + + Backend = ipam_pluggable_backend.IpamPluggableBackend + with mock.patch.object(Backend, '_store_ip_allocation', wraps=store),\ + mock.patch.object(Backend, '_safe_rollback', wraps=rollback),\ + mock.patch.object(Backend, '_allocate_ips_for_port', wraps=alloc): + # Create port with two addresses. The wrapper lets one succeed + # then simulates race for the second to trigger IPAM rollback. + response = self._create_port( + self.fmt, + net_id=net['network']['id'], + fixed_ips=[{'subnet_id': subnet1['subnet']['id']}, + {'subnet_id': subnet2['subnet']['id']}]) + + # When all goes well, retry kicks in and the operation is successful. + self.assertEqual(webob.exc.HTTPCreated.code, response.status_int)