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
This commit is contained in:
parent
c8021143fd
commit
6798485022
|
@ -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,
|
||||
|
|
|
@ -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):
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
Loading…
Reference in New Issue