Merge "Prevent table and column alter and drop"

cinder/tests/unit/db/test_migrations.py

@@ -16,6 +16,9 @@ the test case runs a series of test cases to ensure that migrations work
 properly and that no data loss occurs if possible.
 """
 
+import functools
+from unittest import mock
+
 from alembic import command as alembic_api
 from alembic import script as alembic_script
 import fixtures
@@ -33,6 +36,52 @@ from cinder.db.sqlalchemy import models
 from cinder.tests import fixtures as cinder_fixtures
 
 
+def prevent_drop_alter(func):
+    """Decorator to prevent dropping or altering tables and columns.
+
+    With rolling upgrades we shouldn't blindly allow dropping or altering
+    tables and columns, since that can easily break them.
+
+    Dropping and altering should be done in a backward-compatible manner.  A
+    more detailed explanation is provided in Cinder's developer documentation.
+
+    To properly work, the first parameter of the decorated method must be a
+    class or instance with the DROP_ALTER_EXCEPTIONS and FORBIDDEN_METHODS
+    attribute, and the second parameter must be the version (legacy migrations)
+    or revision (alembic migrations).
+
+    Reviewers should be very careful when adding exceptions to
+    DROP_ALTER_EXCEPTIONS and make sure that in the previous release there was
+    nothing using that column, not even an ORM model (unless the whole ORM
+    model was not being used)
+    """
+
+    @functools.wraps(func)
+    def wrapper(self, revision, *args, **kwargs):
+        exceptions = getattr(self, 'DROP_ALTER_EXCEPTIONS', [])
+        do_ban = revision not in exceptions
+
+        patchers = []
+
+        if do_ban:
+            forbidden = getattr(self, 'FORBIDDEN_METHODS', [])
+            for path in forbidden:
+                txt = (f'Migration {revision}: Operation {path}() is not '
+                       'allowed in a DB migration')
+                patcher = mock.patch(path, autospec=True,
+                                     side_effect=Exception(txt))
+                patcher.start()
+                patchers.append(patcher)
+
+        try:
+            return func(self, revision, *args, **kwargs)
+        finally:
+            for patcher in patchers:
+                patcher.stop()
+
+    return wrapper
+
+
 class CinderModelsMigrationsSync(test_migrations.ModelsMigrationsSync):
     """Test sqlalchemy-migrate migrations."""
 
@@ -122,6 +171,25 @@ class MigrationsWalk(
     TIMEOUT_SCALING_FACTOR = 4
     BOOL_TYPE = sqlalchemy.types.BOOLEAN
 
+    # NOTE: List of migrations where we allow dropping/altring things.
+    # Reviewers: DO NOT ALLOW THINGS TO BE ADDED HERE WITHOUT CARE, and make
+    # sure that in the previous release there was nothing using that column,
+    # not even an ORM model (unless the whole ORM model was not being used)
+    # See prevent_drop_alter method docstring.
+    DROP_ALTER_EXCEPTIONS = [
+        # Drops and alters from initial migration have already been accepted
+        '921e1a36b076',
+        # Making shared_targets explicitly nullable (DB already allowed it)
+        'c92a3e68beed',
+        # Migration 89aa6f9639f9 doesn't fail because it's for a SQLAlquemy
+        # internal table, and we only check Cinder's tables.
+    ]
+    FORBIDDEN_METHODS = ('alembic.operations.Operations.alter_column',
+                         'alembic.operations.Operations.drop_column',
+                         'alembic.operations.Operations.drop_table',
+                         'alembic.operations.BatchOperations.alter_column',
+                         'alembic.operations.BatchOperations.drop_column')
+
     def setUp(self):
         super().setUp()
         self.engine = enginefacade.writer.get_engine()
@@ -129,6 +197,7 @@ class MigrationsWalk(
         self.config = migration._find_alembic_conf()
         self.init_version = '921e1a36b076'
 
+    @prevent_drop_alter
     def _migrate_up(self, revision, connection):
         check_method = getattr(self, f'_check_{revision}', None)
         if revision != self.init_version:  # no tests for the initial revision

doc/source/contributor/rolling.upgrades.rst

@@ -92,10 +92,10 @@ Dropping a column not referenced in SQLAlchemy code
 ...................................................
 
 When we want to remove a column that wasn't present in any SQLAlchemy model or
-it was in the model, but model was not referenced in any SQLAlchemy API
+it was in the model, but model was not referenced anywhere in our code (this
-function (this basically means that N-1 wasn't depending on the presence of
+basically means that N-1 wasn't depending on the presence of that column in the
-that column in the DB), then the situation is simple. We should be able to
+DB), then the situation is simple. We should be able to safely drop the column
-safely drop the column in N release.
+in N release.
 
 Removal of unnecessary column
 .............................