From a4d7f70740b1c707a71ea50f9996c02bf434cf74 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 13 Oct 2021 17:58:33 +0100 Subject: [PATCH] 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] https://github.com/sqlalchemy/alembic/issues/724#issuecomment-672081357 Change-Id: I978b4e44cf7f522a70cc5ca76e6d6f1a985d5469 Signed-off-by: Stephen Finucane --- nova/db/api/migrations/env.py | 49 ++++++-------------- nova/db/api/models.py | 45 ++++++++++++++++++ nova/db/main/migrations/env.py | 30 ++++++------ nova/db/main/models.py | 21 +++++++++ nova/tests/unit/db/api/test_migrations.py | 37 ++++----------- nova/tests/unit/db/main/test_migrations.py | 53 ++++++++++++---------- 6 files changed, 134 insertions(+), 101 deletions(-) diff --git a/nova/db/api/migrations/env.py b/nova/db/api/migrations/env.py index 3f97e1a47c2..a74e135b262 100644 --- a/nova/db/api/migrations/env.py +++ b/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 diff --git a/nova/db/api/models.py b/nova/db/api/models.py index 47a384c0ac3..afb7709defd 100644 --- a/nova/db/api/models.py +++ b/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 diff --git a/nova/db/main/migrations/env.py b/nova/db/main/migrations/env.py index 7401e506469..1f593aa561d 100644 --- a/nova/db/main/migrations/env.py +++ b/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 diff --git a/nova/db/main/models.py b/nova/db/main/models.py index 7a97165fc6f..8c8031031ac 100644 --- a/nova/db/main/models.py +++ b/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() diff --git a/nova/tests/unit/db/api/test_migrations.py b/nova/tests/unit/db/api/test_migrations.py index f714cf1852d..0f51ad12a73 100644 --- a/nova/tests/unit/db/api/test_migrations.py +++ b/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) diff --git a/nova/tests/unit/db/main/test_migrations.py b/nova/tests/unit/db/main/test_migrations.py index c0da4cc66ae..0ca7a8bf77d 100644 --- a/nova/tests/unit/db/main/test_migrations.py +++ b/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(