From e39a02db9899ee8bc11685c4dceb105563c9fb55 Mon Sep 17 00:00:00 2001 From: pkholkin Date: Wed, 10 Dec 2014 13:32:09 +0400 Subject: [PATCH] Replace select-for-update in fixed_ip_associate_pool MySQL Galera does not support the write-intent locks that SELECT FOR UPDATE requires. In this patch SELECT FOR UPDATE in 'fixed_ip_associate_pool' function was replaced with SELECT and UPDATE. The problems with MySQL Galera and algorithm of replacing SELECT FOR UPDATE relate to spec https://review.openstack.org/#/c/135296 Also this patch fixes incorrect network assigning because previously the relationship 'network' was updated instead of column 'network_id'. Change-Id: I403da67a8d55aa30c9abcc491f71918cd2b452dd --- nova/db/sqlalchemy/api.py | 34 +++++++++++++++------ nova/exception.py | 4 +++ nova/tests/unit/db/test_db_api.py | 49 +++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 9 deletions(-) diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 210353dd6b1c..d5f153539da2 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -32,6 +32,7 @@ from oslo.db.sqlalchemy import session as db_session from oslo.db.sqlalchemy import utils as sqlalchemyutils from oslo.utils import excutils from oslo.utils import timeutils +import retrying import six from sqlalchemy import and_ from sqlalchemy import Boolean @@ -1104,6 +1105,9 @@ def fixed_ip_associate(context, address, instance_uuid, network_id=None, @require_admin_context +@_retry_on_deadlock +@retrying.retry(stop_max_attempt_number=5, retry_on_exception= + lambda exc: isinstance(exc, exception.FixedIpAssociateFailed)) def fixed_ip_associate_pool(context, network_id, instance_uuid=None, host=None): if instance_uuid and not uuidutils.is_uuid_like(instance_uuid): @@ -1119,22 +1123,34 @@ def fixed_ip_associate_pool(context, network_id, instance_uuid=None, filter_by(reserved=False).\ filter_by(instance_uuid=None).\ filter_by(host=None).\ - with_lockmode('update').\ first() - # NOTE(vish): if with_lockmode isn't supported, as in sqlite, - # then this has concurrency issues + if not fixed_ip_ref: raise exception.NoMoreFixedIps(net=network_id) + params = {} if fixed_ip_ref['network_id'] is None: - fixed_ip_ref['network'] = network_id - + params['network_id'] = network_id if instance_uuid: - fixed_ip_ref['instance_uuid'] = instance_uuid - + params['instance_uuid'] = instance_uuid if host: - fixed_ip_ref['host'] = host - session.add(fixed_ip_ref) + params['host'] = host + + rows_updated = model_query(context, models.FixedIp, session=session, + read_deleted="no").\ + filter_by(id=fixed_ip_ref['id']).\ + filter_by(network_id=fixed_ip_ref['network_id']).\ + filter_by(reserved=False).\ + filter_by(instance_uuid=None).\ + filter_by(host=None).\ + filter_by(address=fixed_ip_ref['address']).\ + update(params, synchronize_session='evaluate') + + if not rows_updated: + LOG.debug('The row was updated in a concurrent transaction, ' + 'we will fetch another row') + raise exception.FixedIpAssociateFailed(net=network_id) + return fixed_ip_ref diff --git a/nova/exception.py b/nova/exception.py index b917d3a13933..827120279e80 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -782,6 +782,10 @@ class FixedIpNotFoundForNetwork(FixedIpNotFound): "network (%(network_uuid)s).") +class FixedIpAssociateFailed(NovaException): + msg_fmt = _("Fixed IP associate failed for network: %(net)s.") + + class FixedIpAlreadyInUse(NovaException): msg_fmt = _("Fixed IP address %(address)s is already in use on instance " "%(instance_uuid)s.") diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py index 3f86b5287c9d..a79a4f13381e 100644 --- a/nova/tests/unit/db/test_db_api.py +++ b/nova/tests/unit/db/test_db_api.py @@ -3947,6 +3947,55 @@ class FixedIPTestCase(BaseInstanceTypeTestCase): fixed_ip = db.fixed_ip_get_by_address(self.ctxt, address) self.assertEqual(fixed_ip['instance_uuid'], instance_uuid) + def test_fixed_ip_associate_pool_succeeds_fip_ref_network_id_is_none(self): + instance_uuid = self._create_instance() + network = db.network_create_safe(self.ctxt, {}) + + self.create_fixed_ip(network_id=None) + fixed_ip = db.fixed_ip_associate_pool(self.ctxt, + network['id'], instance_uuid) + self.assertEqual(instance_uuid, fixed_ip['instance_uuid']) + self.assertEqual(network['id'], fixed_ip['network_id']) + + def test_fixed_ip_associate_pool_succeeds_retry(self): + instance_uuid = self._create_instance() + network = db.network_create_safe(self.ctxt, {}) + + address = self.create_fixed_ip(network_id=network['id']) + + def fake_first(): + if mock_first.call_count == 1: + return {'network_id': network['id'], 'address': 'invalid', + 'instance_uuid': None, 'host': None, 'id': 1} + else: + return {'network_id': network['id'], 'address': address, + 'instance_uuid': None, 'host': None, 'id': 1} + + with mock.patch('sqlalchemy.orm.query.Query.first', + side_effect=fake_first) as mock_first: + db.fixed_ip_associate_pool(self.ctxt, network['id'], instance_uuid) + self.assertEqual(2, mock_first.call_count) + + fixed_ip = db.fixed_ip_get_by_address(self.ctxt, address) + self.assertEqual(instance_uuid, fixed_ip['instance_uuid']) + + def test_fixed_ip_associate_pool_retry_limit_exceeded(self): + instance_uuid = self._create_instance() + network = db.network_create_safe(self.ctxt, {}) + + self.create_fixed_ip(network_id=network['id']) + + def fake_first(): + return {'network_id': network['id'], 'address': 'invalid', + 'instance_uuid': None, 'host': None, 'id': 1} + + with mock.patch('sqlalchemy.orm.query.Query.first', + side_effect=fake_first) as mock_first: + self.assertRaises(exception.FixedIpAssociateFailed, + db.fixed_ip_associate_pool, self.ctxt, + network['id'], instance_uuid) + self.assertEqual(5, mock_first.call_count) + def test_fixed_ip_create_same_address(self): address = '192.168.1.5' params = {'address': address}