[stable-only] Use a separate transaction for reading after race

As part of data migration code to ensure a Project/User record,
integrated placement creates a record if it does not exist. However,
during a race situation, the logic to read back a record written by
a racing request fails because a change written in a separate
transaction will not be reflected in the current transaction when the
isolation level is REPEATABLE_READ (the default):

  https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-levels.html

This changes the read after catching DBDuplicateEntry to occur in a
separate transaction so it can pick up the change written by the racing
parallel request.

Note: this is a stable-only change because the placement code
containing the bug was temporary data-transitioning code that no longer
exists in placement after Queens.

Closes-Bug: #1731668

Change-Id: Ic90beaaa9848a4f39f4223f31a55cd2c681959cd

2 files changed, 14 insertions, 5 deletions

diff --git a/nova/objects/resource_provider.py b/nova/objects/resource_provider.py
index 7ce5ec9685..903f90e924 100644
--- a/nova/objects/resource_provider.py
+++ b/nova/objects/resource_provider.py
@@ -1911,7 +1911,13 @@ def _ensure_lookup_table_entry(ctx, tbl, external_id):
         except db_exc.DBDuplicateEntry:
             # Another thread added it just before us, so just read the
             # internal ID that that thread created...
-            res = ctx.session.execute(sel).fetchall()
+            # NOTE(melwitt): We need to use a separate transaction to read
+            # back the external_id written by another thread. REPEATABLE_READ
+            # is the default isolation level for InnoDB, so reads from within
+            # the same transaction will be consistent in such a case.
+            # https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-levels.html
+            with db_api.api_context_manager.reader.independent.using(ctx):
+                res = ctx.session.execute(sel).fetchall()
 
     return res[0][0]
 
diff --git a/nova/tests/functional/db/test_resource_provider.py b/nova/tests/functional/db/test_resource_provider.py
index 460c504e24..0c4bf7ed03 100644
--- a/nova/tests/functional/db/test_resource_provider.py
+++ b/nova/tests/functional/db/test_resource_provider.py
@@ -19,10 +19,12 @@ from oslo_config import cfg
 from oslo_db import exception as db_exc
 from oslo_db.sqlalchemy import enginefacade
 from oslo_db.sqlalchemy import test_base
+from oslo_db.sqlalchemy import test_fixtures as db_fixtures
 import sqlalchemy as sa
 
 import nova
 from nova import context
+from nova.db.sqlalchemy import api as db_api
 from nova.db.sqlalchemy import api_models
 from nova.db.sqlalchemy import migration as sa_migration
 from nova import exception
@@ -3044,6 +3046,10 @@ class TestTransactionIsolation(test_base.MySQLOpportunisticTestCase):
         # to read/write to our test MySQL database.
         self.test_context_manager = enginefacade.transaction_context()
         self.test_context_manager.patch_factory(self.enginefacade)
+        # We need to patch our global API database transaction context manager
+        # in order to have it read/write to our test MySQL database too.
+        self.useFixture(db_fixtures.ReplaceEngineFacadeFixture(
+            db_api.api_context_manager, self.test_context_manager))
 
         self.ctx = context.RequestContext('fake-user', 'fake-project')
 
@@ -3077,9 +3083,6 @@ class TestTransactionIsolation(test_base.MySQLOpportunisticTestCase):
 
         # Now run the lookup table method to test the behavior of the scenario.
         with self.test_context_manager.writer.using(self.ctx):
-            # FIXME(melwitt): Because of the bug, we expect IndexError to be
-            # raised when we call _ensure_lookup_table_entry.
-            self.assertRaises(
-                IndexError, rp_obj._ensure_lookup_table_entry,
+            rp_obj._ensure_lookup_table_entry(
                 self.ctx, api_models.Project.__table__,
                 uuidsentinel.external_id)