Don't nest alembic transactions

SqlAlchemy 2.0 has gotten a lot more picky about transactions. In
addition to needing to explicitly set up transactions using SqlAlchemy
2.0 it seems alembic's get_current_revision() call cannot be in the same
transaction as the alembic migrate/stamp calls with MySQL 8.0.

In particular the get_current_revision call seems to get a
SHARED_HIGH_PRIO lock on the alembic_version table. This prevents the
migrate/stamp calls from creating the alembic_version table as this
requires an EXCLUSIVE lock. The SHARED_HIGH_PRIO lock appears to be in
place as long as the get_current_revision transaction is active. To fix
this we simplify our migration tooling and put get_current_revision in a
transaction block of its own. The rest of our migrate function calls
into functions that will setup new transactions and it doesn't need to
be in this block.

Change-Id: Ic71ddf1968610784cef72c4634ccec3a18855a0e

1 files changed, 22 insertions, 18 deletions

diff --git a/zuul/driver/sql/sqlconnection.py b/zuul/driver/sql/sqlconnection.py
index b89653bba..2d5c39ec3 100644
--- a/zuul/driver/sql/sqlconnection.py
+++ b/zuul/driver/sql/sqlconnection.py
@@ -308,27 +308,31 @@ class SQLConnection(BaseConnection):
 
     def _migrate(self, revision='head'):
         """Perform the alembic migrations for this connection"""
+        # Note that this method needs to be called with an external lock held.
+        # The reason for this is we retrieve the alembic version and run the
+        # alembic migrations in different database transactions which opens
+        # us to races without an external lock.
         with self.engine.begin() as conn:
             context = alembic.migration.MigrationContext.configure(conn)
             current_rev = context.get_current_revision()
-            self.log.debug('Current migration revision: %s' % current_rev)
-
-            config = alembic.config.Config()
-            config.set_main_option("script_location",
-                                   "zuul:driver/sql/alembic")
-            config.set_main_option("sqlalchemy.url",
-                                   self.connection_config.get('dburi').
-                                   replace('%', '%%'))
-
-            # Alembic lets us add arbitrary data in the tag argument. We can
-            # leverage that to tell the upgrade scripts about the table prefix.
-            tag = {'table_prefix': self.table_prefix}
-
-            if current_rev is None and not self.force_migrations:
-                self.metadata.create_all(self.engine)
-                alembic.command.stamp(config, revision, tag=tag)
-            else:
-                alembic.command.upgrade(config, revision, tag=tag)
+        self.log.debug('Current migration revision: %s' % current_rev)
+
+        config = alembic.config.Config()
+        config.set_main_option("script_location",
+                               "zuul:driver/sql/alembic")
+        config.set_main_option("sqlalchemy.url",
+                               self.connection_config.get('dburi').
+                               replace('%', '%%'))
+
+        # Alembic lets us add arbitrary data in the tag argument. We can
+        # leverage that to tell the upgrade scripts about the table prefix.
+        tag = {'table_prefix': self.table_prefix}
+
+        if current_rev is None and not self.force_migrations:
+            self.metadata.create_all(self.engine)
+            alembic.command.stamp(config, revision, tag=tag)
+        else:
+            alembic.command.upgrade(config, revision, tag=tag)
 
     def onLoad(self, zk_client, component_registry=None):
         safe_connection = quote_plus(self.connection_name)