From 558196ac46f857070637cc03c5e88df2b37cbb0d Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 20 Aug 2021 13:47:33 +0100 Subject: [PATCH] db: Add migration to resolve shadow table discrepancies As part of the migration to alembic, a number of mistakes were noted in the migrations. These were all a result of doing things differently for the shadow table compared to the main table. Resolve some of these in a new migration to both address the bugs and test out the alembic machinery. Note that we don't actually resolve all of the mismatches. A large number of these mismatches were caused by the addition of 'nullable=False' to a column in a main table but not the equivalent shadow table. With the benefit of hindsight, this is probably wise and possibly even intended. If we were to apply the new constraint to the shadow table also, we'd need a data migration to populate any remaining NULL fields in the shadow tables. This data migration could be very costly and may not even possible as we don't necessarily have all available context required to do this population. As a result, the wording around these differences is changed to indicate that we will likely never address these "bugs". Change-Id: Icdc683d534a4fd35af399561253888abb9193c7d Signed-off-by: Stephen Finucane --- ...16f1fbcab42b_resolve_shadow_table_diffs.py | 62 +++++++++++++++++++ .../versions/8f2f1571d55b_initial_version.py | 24 ++++--- nova/tests/unit/db/main/test_migrations.py | 31 ++++++++++ 3 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 nova/db/main/migrations/versions/16f1fbcab42b_resolve_shadow_table_diffs.py diff --git a/nova/db/main/migrations/versions/16f1fbcab42b_resolve_shadow_table_diffs.py b/nova/db/main/migrations/versions/16f1fbcab42b_resolve_shadow_table_diffs.py new file mode 100644 index 000000000000..d01edae2e813 --- /dev/null +++ b/nova/db/main/migrations/versions/16f1fbcab42b_resolve_shadow_table_diffs.py @@ -0,0 +1,62 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Resolve shadow table diffs + +Revision ID: 16f1fbcab42b +Revises: 8f2f1571d55b +Create Date: 2021-08-20 13:26:30.204633 +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '16f1fbcab42b' +down_revision = '8f2f1571d55b' +branch_labels = None +depends_on = None + + +def upgrade(): + bind = op.get_bind() + + # 244_increase_user_id_length_volume_usage_cache; the length in the + # corresponding shadow table was not increased + + with op.batch_alter_table('shadow_volume_usage_cache') as batch_op: + batch_op.alter_column( + 'user_id', + type_=sa.String(64), + existing_type=sa.String(36), + ) + + # 252_add_instance_extra_table; we shouldn't have created an index for the + # shadow table + + op.drop_index('shadow_instance_extra_idx', 'shadow_instance_extra') + + # 373_migration_uuid; we shouldn't have created an index for the shadow + # table + + op.drop_index('shadow_migrations_uuid', 'shadow_migrations') + + # 298_mysql_extra_specs_binary_collation; we changed the collation on the + # main table but not the shadow table + + if bind.engine.name == 'mysql': + op.execute( + 'ALTER TABLE shadow_instance_type_extra_specs ' + 'CONVERT TO CHARACTER SET utf8 ' + 'COLLATE utf8_bin' + ) diff --git a/nova/db/main/migrations/versions/8f2f1571d55b_initial_version.py b/nova/db/main/migrations/versions/8f2f1571d55b_initial_version.py index 7432b70af263..43020d385b09 100644 --- a/nova/db/main/migrations/versions/8f2f1571d55b_initial_version.py +++ b/nova/db/main/migrations/versions/8f2f1571d55b_initial_version.py @@ -89,17 +89,20 @@ def _create_shadow_tables(connection): ) column_copy = sa.Column(column.name, enum) - # TODO(stephenfin): Fix these various bugs in a follow-up + # NOTE(stephenfin): There were some bugs in the squashed + # sqlalchemy-migrate migrations which we need to account for here # 244_increase_user_id_length_volume_usage_cache; this # alteration should apply to shadow tables also + # (fixed in migration 16f1fbcab42b) if table_name == 'volume_usage_cache' and column.name == 'user_id': - # nullable should be True + # column type should be String(64) column_copy = sa.Column('user_id', sa.String(36)) - # 247_nullable_mismatch; these alterations should apply to shadow - # tables also + # 247_nullable_mismatch; these alterations were not applied to + # shadow tables + # (wontfix since there could be null entries in the database still) if table_name == 'quota_usages' and column.name == 'resources': # nullable should be False @@ -124,8 +127,9 @@ def _create_shadow_tables(connection): # nullable should be False column_copy = sa.Column('dev_type', sa.String(8)) - # 280_add_nullable_false_to_keypairs_name; this should apply to the - # shadow table also + # 280_add_nullable_false_to_keypairs_name; these alterations were + # not applied to the shadow tables + # (wontfix since there could be null entries in the database still) if table_name == 'key_pairs' and column.name == 'name': # nullable should be False @@ -163,10 +167,9 @@ def _create_shadow_tables(connection): 'shadow_' + table_name, meta, *columns, mysql_engine='InnoDB', ) - # TODO(stephenfin): Fix these various bugs in a follow-up - # 252_add_instance_extra_table; we don't create indexes for shadow tables # in general and these should be removed + # (fixed in migration 16f1fbcab42b) op.create_index( 'shadow_instance_extra_idx', @@ -174,6 +177,7 @@ def _create_shadow_tables(connection): ['instance_uuid']) # 373_migration_uuid; we should't create indexes for shadow tables + # (fixed in migration 16f1fbcab42b) op.create_index( 'shadow_migrations_uuid', 'shadow_migrations', ['uuid'], unique=True) @@ -1603,10 +1607,12 @@ def upgrade(): _create_shadow_tables(bind) - # TODO(stephenfin): Fix these various bugs in a follow-up + # NOTE(stephenfin): There were some bugs in the squashed sqlalchemy-migrate + # migrations which we need to account for here # 298_mysql_extra_specs_binary_collation; we should update the shadow table # also + # (fixed in migration 16f1fbcab42b) if bind.engine.name == 'mysql': # Use binary collation for extra specs table diff --git a/nova/tests/unit/db/main/test_migrations.py b/nova/tests/unit/db/main/test_migrations.py index 959d477a330e..c0da4cc66ae5 100644 --- a/nova/tests/unit/db/main/test_migrations.py +++ b/nova/tests/unit/db/main/test_migrations.py @@ -33,6 +33,7 @@ import mock from oslo_db.sqlalchemy import enginefacade from oslo_db.sqlalchemy import test_fixtures from oslo_db.sqlalchemy import test_migrations +from oslo_db.sqlalchemy import utils as oslodbutils from oslo_log.fixture import logging_error as log_fixture from oslo_log import log as logging from oslotest import base @@ -202,6 +203,18 @@ class NovaMigrationsWalk( self.config = migration._find_alembic_conf('main') self.init_version = migration.ALEMBIC_INIT_VERSION['main'] + def assertIndexExists(self, connection, table_name, index): + self.assertTrue( + oslodbutils.index_exists(connection, table_name, index), + 'Index %s on table %s should not exist' % (index, table_name), + ) + + def assertIndexNotExists(self, connection, table_name, index): + self.assertFalse( + oslodbutils.index_exists(connection, table_name, index), + 'Index %s on table %s should not exist' % (index, table_name), + ) + def _migrate_up(self, connection, revision): if revision == self.init_version: # no tests for the initial revision alembic_api.upgrade(self.config, revision) @@ -224,6 +237,24 @@ class NovaMigrationsWalk( if post_upgrade: post_upgrade(connection) + def _pre_upgrade_16f1fbcab42b(self, connection): + self.assertIndexExists( + connection, 'shadow_instance_extra', 'shadow_instance_extra_idx', + ) + self.assertIndexExists( + connection, 'shadow_migrations', 'shadow_migrations_uuid', + ) + + def _check_16f1fbcab42b(self, connection): + self.assertIndexNotExists( + connection, 'shadow_instance_extra', 'shadow_instance_extra_idx', + ) + self.assertIndexNotExists( + connection, 'shadow_migrations', 'shadow_migrations_uuid', + ) + + # no check for the MySQL-specific change + def test_single_base_revision(self): """Ensure we only have a single base revision.