Reduce window for allocate_fixed_ip / release_fixed_ip race in nova-net
There is a race during allocate_fixed_ip where a new instance, B, is
associated with a fixed IP, X, which was previously associated with
instance A that is being deallocated.
Between the time that instance A is associated with fixed IP X and the
time that it's VIF is allocated, and fip.allocated = True in the DB, the
dhcpagent callback hits release_fixed_ip for the fixed IP X from when
instance A was deallocating.
release_fixed_ip checks to see if the instance is allocated and if not
it disassociates the instance, which was associated with new instance B.
This leads to get_instance_nw_info() not building anything since there
are no fixed IPs associated with instance A in the database, so
eventually anything needing to do networking with instance A, like
assocating a floating IP, fails.
To narrow the race, we do the VIF allocation before associating the
fixed IP to the new instance. This does not completely fix the bug, but
it's a tactical change that we can backport to stable branches while
working on the longer-term fix which is going to involve network RPC API
changes to release_fixed_ip().
Note that test_vpn_allocate_fixed_ip_no_network_id is removed since it
no longer works and arguably was testing the DB API in the wrong place,
so a new test is added to test_db_api for the same coverage.
Partial-Bug: #1249065
Change-Id: I8cf5483982085da57ee470fa2753b0d0aebc12b3
(cherry picked from commit 3736c120cb)
This commit is contained in:
Matt Riedemann
parent
9f71bac7c1
commit
2b5fe5db0d
|
|
@ -875,6 +875,21 @@ class NetworkManager(manager.Manager):
|
|||
|
||||
try:
|
||||
if network['cidr']:
|
||||
|
||||
# NOTE(mriedem): allocate the vif before associating the
|
||||
# instance to reduce a race window where a previous instance
|
||||
# was associated with the fixed IP and has released it, because
|
||||
# release_fixed_ip will disassociate if allocated is False.
|
||||
# TODO(mriedem): fix the race in release_fixed_ip
|
||||
vif = objects.VirtualInterface.get_by_instance_and_network(
|
||||
context, instance_id, network['id'])
|
||||
if vif is None:
|
||||
LOG.debug('vif for network %(network)s is used up, '
|
||||
'trying to create new vif',
|
||||
{'network': network['id']}, instance=instance)
|
||||
vif = self._add_virtual_interface(context,
|
||||
instance_id, network['id'])
|
||||
|
||||
address = kwargs.get('address', None)
|
||||
if address:
|
||||
LOG.debug('Associating instance with specified fixed IP '
|
||||
|
|
@ -897,15 +912,6 @@ class NetworkManager(manager.Manager):
|
|||
context.elevated(), network['id'], instance_id)
|
||||
address = str(fip.address)
|
||||
|
||||
vif = objects.VirtualInterface.get_by_instance_and_network(
|
||||
context, instance_id, network['id'])
|
||||
if vif is None:
|
||||
LOG.debug('vif for network %(network)s is used up, '
|
||||
'trying to create new vif',
|
||||
{'network': network['id']}, instance=instance)
|
||||
vif = self._add_virtual_interface(context,
|
||||
instance_id, network['id'])
|
||||
|
||||
fip.allocated = True
|
||||
fip.virtual_interface_id = vif.id
|
||||
fip.save()
|
||||
|
|
@ -1930,6 +1936,23 @@ class VlanManager(RPCAllocateFixedIP, floating_ips.FloatingIP, NetworkManager):
|
|||
|
||||
LOG.debug('Allocate fixed ip on network %s', network['uuid'],
|
||||
instance_uuid=instance_id)
|
||||
|
||||
# NOTE(mriedem): allocate the vif before associating the
|
||||
# instance to reduce a race window where a previous instance
|
||||
# was associated with the fixed IP and has released it, because
|
||||
# release_fixed_ip will disassociate if allocated is False.
|
||||
# TODO(mriedem): fix the race in release_fixed_ip
|
||||
vif = objects.VirtualInterface.get_by_instance_and_network(
|
||||
context, instance_id, network['id'])
|
||||
if vif is None:
|
||||
LOG.debug('vif for network %(network)s and instance '
|
||||
'%(instance_id)s is used up, '
|
||||
'trying to create new vif',
|
||||
{'network': network['id'],
|
||||
'instance_id': instance_id})
|
||||
vif = self._add_virtual_interface(context,
|
||||
instance_id, network['id'])
|
||||
|
||||
if kwargs.get('vpn', None):
|
||||
address = network['vpn_private_address']
|
||||
fip = objects.FixedIP.associate(context, str(address),
|
||||
|
|
@ -1947,17 +1970,6 @@ class VlanManager(RPCAllocateFixedIP, floating_ips.FloatingIP, NetworkManager):
|
|||
instance_id)
|
||||
address = fip.address
|
||||
|
||||
vif = objects.VirtualInterface.get_by_instance_and_network(
|
||||
context, instance_id, network['id'])
|
||||
if vif is None:
|
||||
LOG.debug('vif for network %(network)s and instance '
|
||||
'%(instance_id)s is used up, '
|
||||
'trying to create new vif',
|
||||
{'network': network['id'],
|
||||
'instance_id': instance_id})
|
||||
vif = self._add_virtual_interface(context,
|
||||
instance_id, network['id'])
|
||||
|
||||
fip.allocated = True
|
||||
fip.virtual_interface_id = vif.id
|
||||
fip.save()
|
||||
|
|
|
|||
|
|
@ -4150,6 +4150,20 @@ class FixedIPTestCase(BaseInstanceTypeTestCase):
|
|||
self.ctxt, None, instance_uuid)
|
||||
self.assertEqual(1, mock_first.call_count)
|
||||
|
||||
def test_fixed_ip_associate_no_network_id_with_no_retries(self):
|
||||
# Tests that trying to associate an instance to a fixed IP on a network
|
||||
# but without specifying the network ID during associate will fail.
|
||||
instance_uuid = self._create_instance()
|
||||
network = db.network_create_safe(self.ctxt, {})
|
||||
address = self.create_fixed_ip(network_id=network['id'])
|
||||
|
||||
with mock.patch('sqlalchemy.orm.query.Query.first',
|
||||
return_value=None) as mock_first:
|
||||
self.assertRaises(exception.FixedIpNotFoundForNetwork,
|
||||
db.fixed_ip_associate,
|
||||
self.ctxt, address, instance_uuid)
|
||||
self.assertEqual(1, mock_first.call_count)
|
||||
|
||||
def test_fixed_ip_associate_pool_invalid_uuid(self):
|
||||
instance_uuid = '123'
|
||||
self.assertRaises(exception.InvalidUUID, db.fixed_ip_associate_pool,
|
||||
|
|
|
|||
|
|
@ -804,6 +804,29 @@ class FlatNetworkTestCase(test.TestCase):
|
|||
net['id'])
|
||||
self.assertEqual(fip.virtual_interface_id, vif.id)
|
||||
|
||||
@mock.patch('nova.objects.instance.Instance.get_by_uuid')
|
||||
@mock.patch.object(db, 'virtual_interface_get_by_instance_and_network',
|
||||
return_value=None)
|
||||
@mock.patch('nova.objects.fixed_ip.FixedIP')
|
||||
def test_allocate_fixed_ip_add_vif_fails(self, mock_fixedip,
|
||||
mock_get_vif, mock_instance_get):
|
||||
# Tests that we don't try to do anything with fixed IPs if
|
||||
# _add_virtual_interface fails.
|
||||
instance = fake_instance.fake_instance_obj(self.context)
|
||||
mock_instance_get.return_value = instance
|
||||
network = {'cidr': '24', 'id': 1,
|
||||
'uuid': '398399b3-f696-4859-8695-a6560e14cb02'}
|
||||
vif_error = exception.VirtualInterfaceMacAddressException()
|
||||
# mock out quotas because we don't care in this test
|
||||
with mock.patch.object(self.network, 'quotas_cls', objects.QuotasNoOp):
|
||||
with mock.patch.object(self.network, '_add_virtual_interface',
|
||||
side_effect=vif_error):
|
||||
self.assertRaises(
|
||||
exception.VirtualInterfaceMacAddressException,
|
||||
self.network.allocate_fixed_ip, self.context,
|
||||
'9d2ee1e3-ffad-4e5f-81ff-c96dd97b0ee0', network)
|
||||
self.assertFalse(mock_fixedip.called, str(mock_fixedip.mock_calls))
|
||||
|
||||
|
||||
class FlatDHCPNetworkTestCase(test.TestCase):
|
||||
|
||||
|
|
@ -907,18 +930,6 @@ class VlanNetworkTestCase(test.TestCase):
|
|||
self.network.allocate_fixed_ip(self.context, FAKEUUID, network,
|
||||
vpn=True)
|
||||
|
||||
def test_vpn_allocate_fixed_ip_no_network_id(self):
|
||||
network = dict(networks[0])
|
||||
network['vpn_private_address'] = '192.168.0.2'
|
||||
network['id'] = None
|
||||
instance = db.instance_create(self.context, {})
|
||||
self.assertRaises(exception.FixedIpNotFoundForNetwork,
|
||||
self.network.allocate_fixed_ip,
|
||||
self.context_admin,
|
||||
instance['uuid'],
|
||||
network,
|
||||
vpn=True)
|
||||
|
||||
def test_allocate_fixed_ip(self):
|
||||
self.stubs.Set(self.network,
|
||||
'_do_trigger_security_group_members_refresh_for_instance',
|
||||
|
|
@ -1030,6 +1041,22 @@ class VlanNetworkTestCase(test.TestCase):
|
|||
instance.uuid,
|
||||
1, reserved=True)
|
||||
|
||||
@mock.patch.object(db, 'virtual_interface_get_by_instance_and_network',
|
||||
return_value=None)
|
||||
@mock.patch('nova.objects.fixed_ip.FixedIP')
|
||||
def test_allocate_fixed_ip_add_vif_fails(self, mock_fixedip,
|
||||
mock_get_vif):
|
||||
# Tests that we don't try to do anything with fixed IPs if
|
||||
# _add_virtual_interface fails.
|
||||
vif_error = exception.VirtualInterfaceMacAddressException()
|
||||
with mock.patch.object(self.network, '_add_virtual_interface',
|
||||
side_effect=vif_error):
|
||||
self.assertRaises(exception.VirtualInterfaceMacAddressException,
|
||||
self.network.allocate_fixed_ip, self.context,
|
||||
'9d2ee1e3-ffad-4e5f-81ff-c96dd97b0ee0',
|
||||
networks[0])
|
||||
self.assertFalse(mock_fixedip.called, str(mock_fixedip.mock_calls))
|
||||
|
||||
def test_create_networks_too_big(self):
|
||||
self.assertRaises(ValueError, self.network.create_networks, None,
|
||||
num_networks=4094, vlan_start=1)
|
||||
|
|
|
|||
Loading…
Reference in New Issue