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(