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

@@ -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
 
 

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.")

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}