db: De-duplicate list of removed table columns

We have a policy of removing fields from SQLAlchemy models at least one
cycle before we remove the underlying database columns. This can result
in a discrepancy between the state that our newfangled database
migration tool, alembic, sees and what's actually in the database. We
were ignoring these removed fields (and one foreign key constraint) in
two different locations for both databases: as part of the alembic
configuration and as part of the tests we have to ensure our migrations
are in sync with our models (note: the tests actually use the alembic
mechanism to detect the changes [1]). De-duplicate these.

[1] sqlalchemy/alembic#724 (comment)

Change-Id: I978b4e44cf7f522a70cc5ca76e6d6f1a985d5469
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

nova/db/api/migrations/env.py

@@ -32,42 +32,21 @@
 
 
 def include_name(name, type_, parent_names):
+    """Determine which tables or columns to skip.
+
+    This is used when we decide to "delete" a table or column. In this
+    instance, we will remove the SQLAlchemy model or field but leave the
+    underlying database table or column in place for a number of releases
+    after. Once we're sure that there is no code running that contains
+    references to the old models, we can then remove the underlying table. In
+    the interim, we must track the discrepancy between models and actual
+    database data here.
+    """
+    if type_ == 'table':
+        return name not in models.REMOVED_TABLES
+
     if type_ == 'column':
-        # NOTE(stephenfin): This is a list of fields that have been removed
-        # from various SQLAlchemy models but which still exist in the
-        # underlying tables. Our upgrade policy dictates that we remove fields
-        # from models at least one cycle before we remove the column from the
-        # underlying table. Not doing so would prevent us from applying the
-        # new database schema before rolling out any of the new code since the
-        # old code could attempt to access data in the removed columns. Alembic
-        # identifies this temporary mismatch between the models and underlying
-        # tables and attempts to resolve it. Tell it instead to ignore these
-        # until we're ready to remove them ourselves.
-        return (parent_names['table_name'], name) not in {
-            ('build_requests', 'request_spec_id'),
-            ('build_requests', 'user_id'),
-            ('build_requests', 'display_name'),
-            ('build_requests', 'instance_metadata'),
-            ('build_requests', 'progress'),
-            ('build_requests', 'vm_state'),
-            ('build_requests', 'task_state'),
-            ('build_requests', 'image_ref'),
-            ('build_requests', 'access_ip_v4'),
-            ('build_requests', 'access_ip_v6'),
-            ('build_requests', 'info_cache'),
-            ('build_requests', 'security_groups'),
-            ('build_requests', 'config_drive'),
-            ('build_requests', 'key_name'),
-            ('build_requests', 'locked_by'),
-            ('build_requests', 'reservation_id'),
-            ('build_requests', 'launch_index'),
-            ('build_requests', 'hostname'),
-            ('build_requests', 'kernel_id'),
-            ('build_requests', 'ramdisk_id'),
-            ('build_requests', 'root_device_name'),
-            ('build_requests', 'user_data'),
-            ('resource_providers', 'can_host'),
-        }
+        return (parent_names['table_name'], name) not in models.REMOVED_COLUMNS
 
     return True
 

nova/db/api/models.py

@@ -24,6 +24,51 @@
 LOG = logging.getLogger(__name__)
 
 
+# NOTE(stephenfin): This is a list of fields that have been removed from
+# various SQLAlchemy models but which still exist in the underlying tables. Our
+# upgrade policy dictates that we remove fields from models at least one cycle
+# before we remove the column from the underlying table. Not doing so would
+# prevent us from applying the new database schema before rolling out any of
+# the new code since the old code could attempt to access data in the removed
+# columns. Alembic identifies this temporary mismatch between the models and
+# underlying tables and attempts to resolve it. Tell it instead to ignore these
+# until we're ready to remove them ourselves.
+REMOVED_COLUMNS = {
+    ('build_requests', 'request_spec_id'),
+    ('build_requests', 'user_id'),
+    ('build_requests', 'display_name'),
+    ('build_requests', 'instance_metadata'),
+    ('build_requests', 'progress'),
+    ('build_requests', 'vm_state'),
+    ('build_requests', 'task_state'),
+    ('build_requests', 'image_ref'),
+    ('build_requests', 'access_ip_v4'),
+    ('build_requests', 'access_ip_v6'),
+    ('build_requests', 'info_cache'),
+    ('build_requests', 'security_groups'),
+    ('build_requests', 'config_drive'),
+    ('build_requests', 'key_name'),
+    ('build_requests', 'locked_by'),
+    ('build_requests', 'reservation_id'),
+    ('build_requests', 'launch_index'),
+    ('build_requests', 'hostname'),
+    ('build_requests', 'kernel_id'),
+    ('build_requests', 'ramdisk_id'),
+    ('build_requests', 'root_device_name'),
+    ('build_requests', 'user_data'),
+    ('resource_providers', 'can_host'),
+}
+
+# NOTE(stephenfin): A list of foreign key constraints that were removed when
+# the column they were covering was removed.
+REMOVED_FKEYS = [
+    ('build_requests', ['request_spec_id']),
+]
+
+# NOTE(stephenfin): A list of entire models that have been removed.
+REMOVED_TABLES = []
+
+
 class _NovaAPIBase(models.ModelBase, models.TimestampMixin):
     pass
 

nova/db/main/migrations/env.py

@@ -32,27 +32,27 @@
 
 
 def include_name(name, type_, parent_names):
+    """Determine which tables or columns to skip.
+
+    This is used when we decide to "delete" a table or column. In this
+    instance, we will remove the SQLAlchemy model or field but leave the
+    underlying database table or column in place for a number of releases
+    after. Once we're sure that there is no code running that contains
+    references to the old models, we can then remove the underlying table. In
+    the interim, we must track the discrepancy between models and actual
+    database data here.
+    """
     if type_ == 'table':
         # NOTE(stephenfin): We don't have models corresponding to the various
         # shadow tables. Alembic doesn't like this. Tell Alembic to look the
         # other way. Good Alembic.
-        return not name.startswith('shadow_')
+        if name.startswith('shadow_'):
+            return False
+
+        return name not in models.REMOVED_TABLES
 
     if type_ == 'column':
-        # NOTE(stephenfin): This is a list of fields that have been removed
-        # from various SQLAlchemy models but which still exist in the
-        # underlying tables. Our upgrade policy dictates that we remove fields
-        # from models at least one cycle before we remove the column from the
-        # underlying table. Not doing so would prevent us from applying the
-        # new database schema before rolling out any of the new code since the
-        # old code could attempt to access data in the removed columns. Alembic
-        # identifies this temporary mismatch between the models and underlying
-        # tables and attempts to resolve it. Tell it instead to ignore these
-        # until we're ready to remove them ourselves.
-        return (parent_names['table_name'], name) not in {
-            ('instances', 'internal_id'),
-            ('instance_extra', 'vpmems'),
-        }
+        return (parent_names['table_name'], name) not in models.REMOVED_COLUMNS
 
     return True
 

nova/db/main/models.py

@@ -32,6 +32,27 @@
 
 CONF = cfg.CONF
 
+# NOTE(stephenfin): This is a list of fields that have been removed from
+# various SQLAlchemy models but which still exist in the underlying tables. Our
+# upgrade policy dictates that we remove fields from models at least one cycle
+# before we remove the column from the underlying table. Not doing so would
+# prevent us from applying the new database schema before rolling out any of
+# the new code since the old code could attempt to access data in the removed
+# columns. Alembic identifies this temporary mismatch between the models and
+# underlying tables and attempts to resolve it. Tell it instead to ignore these
+# until we're ready to remove them ourselves.
+REMOVED_COLUMNS = {
+    ('instances', 'internal_id'),
+    ('instance_extra', 'vpmems'),
+}
+
+# NOTE(stephenfin): A list of foreign key constraints that were removed when
+# the column they were covering was removed.
+REMOVED_FKEYS = []
+
+# NOTE(stephenfin): A list of entire models that have been removed.
+REMOVED_TABLES = []
+
 # we don't configure 'cls' since we have models that don't use the
 # TimestampMixin
 BASE = declarative.declarative_base()

nova/tests/unit/db/api/test_migrations.py

@@ -62,30 +62,17 @@ def include_object(self, object_, name, type_, reflected, compare_to):
             if name == 'migrate_version':
                 return False
 
+            # Define a whitelist of tables that will be removed from the DB in
+            # a later release and don't have a corresponding model anymore.
+
+            return name not in models.REMOVED_TABLES
+
         return True
 
     def filter_metadata_diff(self, diff):
         # Filter out diffs that shouldn't cause a sync failure.
         new_diff = []
 
-        # Define a whitelist of ForeignKeys that exist on the model but not in
-        # the database. They will be removed from the model at a later time.
-        fkey_whitelist = {'build_requests': ['request_spec_id']}
-
-        # Define a whitelist of columns that will be removed from the
-        # DB at a later release and aren't on a model anymore.
-
-        column_whitelist = {
-            'build_requests': [
-                'vm_state', 'instance_metadata',
-                'display_name', 'access_ip_v6', 'access_ip_v4', 'key_name',
-                'locked_by', 'image_ref', 'progress', 'request_spec_id',
-                'info_cache', 'user_id', 'task_state', 'security_groups',
-                'config_drive',
-            ],
-            'resource_providers': ['can_host'],
-        }
-
         for element in diff:
             if isinstance(element, list):
                 # modify_nullable is a list
@@ -97,18 +84,12 @@ def filter_metadata_diff(self, diff):
                     fkey = element[1]
                     tablename = fkey.table.name
                     column_keys = fkey.column_keys
-                    if (
-                        tablename in fkey_whitelist and
-                        column_keys == fkey_whitelist[tablename]
-                    ):
+                    if (tablename, column_keys) in models.REMOVED_FKEYS:
                         continue
                 elif element[0] == 'remove_column':
-                    tablename = element[2]
-                    column = element[3]
-                    if (
-                        tablename in column_whitelist and
-                        column.name in column_whitelist[tablename]
-                    ):
+                    table = element[2]
+                    column = element[3].name
+                    if (table, column) in models.REMOVED_COLUMNS:
                         continue
 
                 new_diff.append(element)

nova/tests/unit/db/main/test_migrations.py

@@ -80,32 +80,39 @@ def include_object(self, object_, name, type_, reflected, compare_to):
             if name == 'migrate_version' or name.startswith('shadow_'):
                 return False
 
-        return True
+            # Define a whitelist of tables that will be removed from the DB in
+            # a later release and don't have a corresponding model anymore.
 
-    def filter_metadata_diff(self, diff):
-        # Overriding the parent method to decide on certain attributes
-        # that maybe present in the DB but not in the models.py
-
-        def removed_column(element):
-            # Define a whitelist of columns that would be removed from the
-            # DB at a later release.
-            # NOTE(Luyao) The vpmems column was added to the schema in train,
-            # and removed from the model in train.
-            column_whitelist = {
-                'instances': ['internal_id'],
-                'instance_extra': ['vpmems'],
-            }
-
-            if element[0] != 'remove_column':
-                return False
+            return name not in models.REMOVED_TABLES
 
-            table_name, column = element[2], element[3]
-            return (
-                table_name in column_whitelist and
-                column.name in column_whitelist[table_name]
-            )
+        return True
 
-        return [element for element in diff if not removed_column(element)]
+    def filter_metadata_diff(self, diff):
+        # Filter out diffs that shouldn't cause a sync failure.
+        new_diff = []
+
+        for element in diff:
+            if isinstance(element, list):
+                # modify_nullable is a list
+                new_diff.append(element)
+            else:
+                # tuple with action as first element. Different actions have
+                # different tuple structures.
+                if element[0] == 'add_fk':
+                    fkey = element[1]
+                    tablename = fkey.table.name
+                    column_keys = fkey.column_keys
+                    if (tablename, column_keys) in models.REMOVED_FKEYS:
+                        continue
+                if element[0] == 'remove_column':
+                    table = element[2]
+                    column = element[3].name
+                    if (table, column) in models.REMOVED_COLUMNS:
+                        continue
+
+                new_diff.append(element)
+
+        return new_diff
 
 
 class TestModelsSyncSQLite(