diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index 5696118ccf3f..485b1741a19c 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -918,6 +918,35 @@ class DbCommands(object): admin_context = context.get_admin_context() db.archive_deleted_rows(admin_context, max_rows) + @args('--delete', action='store_true', dest='delete', + help='If specified, automatically delete any records found where ' + 'instance_uuid is NULL.') + def null_instance_uuid_scan(self, delete=False): + """Lists and optionally deletes database records where + instance_uuid is NULL. + """ + hits = migration.db_null_instance_uuid_scan(delete) + records_found = False + for table_name, records in six.iteritems(hits): + # Don't print anything for 0 hits + if records: + records_found = True + if delete: + print(_("Deleted %(records)d records " + "from table '%(table_name)s'.") % + {'records': records, 'table_name': table_name}) + else: + print(_("There are %(records)d records in the " + "'%(table_name)s' table where the uuid or " + "instance_uuid column is NULL. Run this " + "command again with the --delete option after you " + "have backed up any necessary data.") % + {'records': records, 'table_name': table_name}) + # check to see if we didn't find anything + if not records_found: + print(_('There were no records found where ' + 'instance_uuid was NULL.')) + class AgentBuildCommands(object): """Class for managing agent builds.""" diff --git a/nova/db/migration.py b/nova/db/migration.py index 49ab83536422..c661a241cb56 100644 --- a/nova/db/migration.py +++ b/nova/db/migration.py @@ -37,3 +37,23 @@ def db_version(): def db_initial_version(): """The starting version for the database.""" return IMPL.db_initial_version() + + +def db_null_instance_uuid_scan(delete=False): + """Utility for scanning the database to look for NULL instance uuid rows. + + Scans the backing nova database to look for table entries where + instances.uuid or instance_uuid columns are NULL (except for the + fixed_ips table since that can contain NULL instance_uuid entries by + design). Dumps the tables that have NULL instance_uuid entries or + optionally deletes them based on usage. + + This tool is meant to be used in conjunction with the 267 database + migration script to detect and optionally cleanup NULL instance_uuid + records. + + :param delete: If true, delete NULL instance_uuid records found, else + just query to see if they exist for reporting. + :returns: dict of table name to number of hits for NULL instance_uuid rows. + """ + return IMPL.db_null_instance_uuid_scan(delete=delete) diff --git a/nova/db/sqlalchemy/migrate_repo/versions/267_instance_uuid_non_nullable.py b/nova/db/sqlalchemy/migrate_repo/versions/267_instance_uuid_non_nullable.py new file mode 100644 index 000000000000..440e20faeebd --- /dev/null +++ b/nova/db/sqlalchemy/migrate_repo/versions/267_instance_uuid_non_nullable.py @@ -0,0 +1,125 @@ +# Copyright 2014 IBM Corp. +# +# 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. + +from migrate import UniqueConstraint +from oslo.db.sqlalchemy import utils +from sqlalchemy import MetaData +from sqlalchemy.sql import null + +from nova import exception +from nova.i18n import _ +from nova.openstack.common import log as logging + +LOG = logging.getLogger(__name__) + +UC_NAME = 'uniq_instances0uuid' + + +def scan_for_null_records(table, col_name, check_fkeys): + """Queries the table looking for NULL instances of the given column. + + :param col_name: The name of the column to look for in the table. + :param check_fkeys: If True, check the table for foreign keys back to the + instances table and if not found, return. + :raises: exception.ValidationError: If any records are found. + """ + if col_name in table.columns: + # NOTE(mriedem): filter out tables that don't have a foreign key back + # to the instances table since they could have stale data even if + # instances.uuid wasn't NULL. + if check_fkeys: + fkey_found = False + fkeys = table.c[col_name].foreign_keys or [] + for fkey in fkeys: + if fkey.column.table.name == 'instances': + fkey_found = True + + if not fkey_found: + return + + records = len(list( + table.select().where(table.c[col_name] == null()).execute() + )) + if records: + msg = _("There are %(records)d records in the " + "'%(table_name)s' table where the uuid or " + "instance_uuid column is NULL. These must be " + "manually cleaned up before the migration will pass. " + "Consider running the " + "'nova-manage db null_instance_uuid_scan' command.") % ( + {'records': records, 'table_name': table.name}) + raise exception.ValidationError(detail=msg) + + +def process_null_records(meta, scan=True): + """Scans the database for null instance_uuid records for processing. + + :param meta: sqlalchemy.MetaData object, assumes tables are reflected. + :param scan: If True, does a query and fails the migration if NULL instance + uuid entries found. If False, makes instances.uuid + non-nullable. + """ + if scan: + for table in reversed(meta.sorted_tables): + # NOTE(mriedem): There is a periodic task in the network manager + # that calls nova.db.api.fixed_ip_disassociate_all_by_timeout which + # will set fixed_ips.instance_uuid to None by design, so we have to + # skip the fixed_ips table otherwise we'll wipeout the pool of + # fixed IPs. + if table.name not in ('fixed_ips', 'shadow_fixed_ips'): + scan_for_null_records(table, 'instance_uuid', check_fkeys=True) + + for table_name in ('instances', 'shadow_instances'): + table = meta.tables[table_name] + if scan: + scan_for_null_records(table, 'uuid', check_fkeys=False) + else: + # The record is gone so make the uuid column non-nullable. + table.columns.uuid.alter(nullable=False) + + +def upgrade(migrate_engine): + # NOTE(mriedem): We're going to load up all of the tables so we can find + # any with an instance_uuid column since those may be foreign keys back + # to the instances table and we want to cleanup those records first. We + # have to do this explicitly because the foreign keys in nova aren't + # defined with cascading deletes. + meta = MetaData(migrate_engine) + meta.reflect(migrate_engine) + # Scan the database first and fail if any NULL records found. + process_null_records(meta, scan=True) + # Now run the alter statements. + process_null_records(meta, scan=False) + # Create a unique constraint on instances.uuid for foreign keys. + instances = meta.tables['instances'] + UniqueConstraint('uuid', table=instances, name=UC_NAME).create() + + # NOTE(mriedem): We now have a unique index on instances.uuid from the + # 216_havana migration and a unique constraint on the same column, which + # is redundant but should not be a big performance penalty. We should + # clean this up in a later (separate) migration since it involves dropping + # any ForeignKeys on the instances.uuid column due to some index rename + # issues in older versions of MySQL. That is beyond the scope of this + # migration. + + +def downgrade(migrate_engine): + # drop the unique constraint on instances.uuid + UniqueConstraint('uuid', + table=utils.get_table(migrate_engine, 'instances'), + name=UC_NAME).drop() + # We can't bring the deleted records back but we can make uuid nullable. + for table_name in ('instances', 'shadow_instances'): + table = utils.get_table(migrate_engine, table_name) + table.columns.uuid.alter(nullable=True) diff --git a/nova/db/sqlalchemy/migration.py b/nova/db/sqlalchemy/migration.py index 388e6d2feea6..8df6445cfb0d 100644 --- a/nova/db/sqlalchemy/migration.py +++ b/nova/db/sqlalchemy/migration.py @@ -19,7 +19,9 @@ import os from migrate import exceptions as versioning_exceptions from migrate.versioning import api as versioning_api from migrate.versioning.repository import Repository +from oslo.db.sqlalchemy import utils as db_utils import sqlalchemy +from sqlalchemy.sql import null from nova.db.sqlalchemy import api as db_session from nova import exception @@ -70,6 +72,75 @@ def db_initial_version(): return INIT_VERSION +def _process_null_records(table, col_name, check_fkeys, delete=False): + """Queries the database and optionally deletes the NULL records. + + :param table: sqlalchemy.Table object. + :param col_name: The name of the column to check in the table. + :param check_fkeys: If True, check the table for foreign keys back to the + instances table and if not found, return. + :param delete: If true, run a delete operation on the table, else just + query for number of records that match the NULL column. + :returns: The number of records processed for the table and column. + """ + records = 0 + if col_name in table.columns: + # NOTE(mriedem): filter out tables that don't have a foreign key back + # to the instances table since they could have stale data even if + # instances.uuid wasn't NULL. + if check_fkeys: + fkey_found = False + fkeys = table.c[col_name].foreign_keys or [] + for fkey in fkeys: + if fkey.column.table.name == 'instances': + fkey_found = True + + if not fkey_found: + return 0 + + if delete: + records = table.delete().where( + table.c[col_name] == null() + ).execute().rowcount + else: + records = len(list( + table.select().where(table.c[col_name] == null()).execute() + )) + return records + + +def db_null_instance_uuid_scan(delete=False): + """Scans the database for NULL instance_uuid records. + + :param delete: If true, delete NULL instance_uuid records found, else + just query to see if they exist for reporting. + :returns: dict of table name to number of hits for NULL instance_uuid rows. + """ + engine = get_engine() + meta = sqlalchemy.MetaData(bind=engine) + # NOTE(mriedem): We're going to load up all of the tables so we can find + # any with an instance_uuid column since those may be foreign keys back + # to the instances table and we want to cleanup those records first. We + # have to do this explicitly because the foreign keys in nova aren't + # defined with cascading deletes. + meta.reflect(engine) + # Keep track of all of the tables that had hits in the query. + processed = {} + for table in reversed(meta.sorted_tables): + # Ignore the fixed_ips table by design. + if table.name not in ('fixed_ips', 'shadow_fixed_ips'): + processed[table.name] = _process_null_records( + table, 'instance_uuid', check_fkeys=True, delete=delete) + + # Now process the *instances tables. + for table_name in ('instances', 'shadow_instances'): + table = db_utils.get_table(engine, table_name) + processed[table.name] = _process_null_records( + table, 'uuid', check_fkeys=False, delete=delete) + + return processed + + def db_version_control(version=None): repository = _find_migrate_repo() versioning_api.version_control(get_engine(), repository, version) diff --git a/nova/db/sqlalchemy/models.py b/nova/db/sqlalchemy/models.py index 68611d66285c..f7d493ac110b 100644 --- a/nova/db/sqlalchemy/models.py +++ b/nova/db/sqlalchemy/models.py @@ -169,6 +169,7 @@ class Instance(BASE, NovaBase): 'host', 'node', 'deleted'), Index('instances_host_deleted_cleaned_idx', 'host', 'deleted', 'cleaned'), + schema.UniqueConstraint('uuid', name='uniq_instances0uuid'), ) injected_files = [] @@ -258,7 +259,7 @@ class Instance(BASE, NovaBase): os_type = Column(String(255)) architecture = Column(String(255)) vm_mode = Column(String(255)) - uuid = Column(String(36)) + uuid = Column(String(36), nullable=False) root_device_name = Column(String(255)) default_ephemeral_device = Column(String(255)) diff --git a/nova/tests/unit/db/test_migrations.py b/nova/tests/unit/db/test_migrations.py index 4cacbf7480ec..bf4cbc1f191f 100644 --- a/nova/tests/unit/db/test_migrations.py +++ b/nova/tests/unit/db/test_migrations.py @@ -43,12 +43,15 @@ from oslo.db.sqlalchemy import test_base from oslo.db.sqlalchemy import test_migrations from oslo.db.sqlalchemy import utils as oslodbutils import sqlalchemy +from sqlalchemy.engine import reflection import sqlalchemy.exc +from sqlalchemy.sql import null from nova.db import migration from nova.db.sqlalchemy import migrate_repo from nova.db.sqlalchemy import migration as sa_migration from nova.db.sqlalchemy import utils as db_utils +from nova import exception from nova.i18n import _ from nova import test from nova.tests.unit import conf_fixture @@ -442,6 +445,102 @@ class NovaMigrationsCheckers(test_migrations.WalkVersionsMixin): def _post_downgrade_266(self, engine): self.assertTableNotExists(engine, 'tags') + def _pre_upgrade_267(self, engine): + # Create a fixed_ips row with a null instance_uuid (if not already + # there) to make sure that's not deleted. + fixed_ips = oslodbutils.get_table(engine, 'fixed_ips') + fake_fixed_ip = {'id': 1} + fixed_ips.insert().execute(fake_fixed_ip) + # Create an instance record with a valid (non-null) UUID so we make + # sure we don't do something stupid and delete valid records. + instances = oslodbutils.get_table(engine, 'instances') + fake_instance = {'id': 1, 'uuid': 'fake-non-null-uuid'} + instances.insert().execute(fake_instance) + # Add a null instance_uuid entry for the volumes table + # since it doesn't have a foreign key back to the instances table. + volumes = oslodbutils.get_table(engine, 'volumes') + fake_volume = {'id': '9c3c317e-24db-4d57-9a6f-96e6d477c1da'} + volumes.insert().execute(fake_volume) + + def _check_267(self, engine, data): + # Make sure the column is non-nullable and the UC exists. + fixed_ips = oslodbutils.get_table(engine, 'fixed_ips') + self.assertTrue(fixed_ips.c.instance_uuid.nullable) + fixed_ip = fixed_ips.select(fixed_ips.c.id == 1).execute().first() + self.assertIsNone(fixed_ip.instance_uuid) + + instances = oslodbutils.get_table(engine, 'instances') + self.assertFalse(instances.c.uuid.nullable) + + inspector = reflection.Inspector.from_engine(engine) + constraints = inspector.get_unique_constraints('instances') + constraint_names = [constraint['name'] for constraint in constraints] + self.assertIn('uniq_instances0uuid', constraint_names) + + # Make sure the instances record with the valid uuid is still there. + instance = instances.select(instances.c.id == 1).execute().first() + self.assertIsNotNone(instance) + + # Check that the null entry in the volumes table is still there since + # we skipped tables that don't have FK's back to the instances table. + volumes = oslodbutils.get_table(engine, 'volumes') + self.assertTrue(volumes.c.instance_uuid.nullable) + volume = fixed_ips.select( + volumes.c.id == '9c3c317e-24db-4d57-9a6f-96e6d477c1da' + ).execute().first() + self.assertIsNone(volume.instance_uuid) + + def _post_downgrade_267(self, engine): + # Make sure the UC is gone and the column is nullable again. + instances = oslodbutils.get_table(engine, 'instances') + self.assertTrue(instances.c.uuid.nullable) + + inspector = reflection.Inspector.from_engine(engine) + constraints = inspector.get_unique_constraints('instances') + constraint_names = [constraint['name'] for constraint in constraints] + self.assertNotIn('uniq_instances0uuid', constraint_names) + + def test_migration_267(self): + # This is separate from test_walk_versions so we can test the case + # where there are non-null instance_uuid entries in the database which + # cause the 267 migration to fail. + engine = self.migrate_engine + self.migration_api.version_control( + engine, self.REPOSITORY, self.INIT_VERSION) + self.migration_api.upgrade(engine, self.REPOSITORY, 266) + # Create a consoles record with a null instance_uuid so + # we can test that the upgrade fails if that entry is found. + # NOTE(mriedem): We use the consoles table since that's the only table + # created in the 216 migration with a ForeignKey created on the + # instance_uuid table for sqlite. + consoles = oslodbutils.get_table(engine, 'consoles') + fake_console = {'id': 1} + consoles.insert().execute(fake_console) + + # NOTE(mriedem): We handle the 267 migration where we expect to + # hit a ValidationError on the consoles table to have + # a null instance_uuid entry + ex = self.assertRaises(exception.ValidationError, + self.migration_api.upgrade, + engine, self.REPOSITORY, 267) + + self.assertIn("There are 1 records in the " + "'consoles' table where the uuid or " + "instance_uuid column is NULL.", + ex.kwargs['detail']) + + # Remove the consoles entry with the null instance_uuid column. + rows = consoles.delete().where( + consoles.c['instance_uuid'] == null()).execute().rowcount + self.assertEqual(1, rows) + # Now run the 267 upgrade again. + self.migration_api.upgrade(engine, self.REPOSITORY, 267) + + # Make sure the consoles entry with the null instance_uuid + # was deleted. + console = consoles.select(consoles.c.id == 1).execute().first() + self.assertIsNone(console) + class TestNovaMigrationsSQLite(NovaMigrationsCheckers, test_base.DbTestCase): diff --git a/nova/tests/unit/db/test_sqlalchemy_migration.py b/nova/tests/unit/db/test_sqlalchemy_migration.py new file mode 100644 index 000000000000..da1e7dc770a2 --- /dev/null +++ b/nova/tests/unit/db/test_sqlalchemy_migration.py @@ -0,0 +1,85 @@ +# Copyright 2014 IBM Corp. +# +# 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. + +from migrate import UniqueConstraint +from oslo.db.sqlalchemy import utils as db_utils + +from nova.db.sqlalchemy import api as db_api +from nova.db.sqlalchemy import migration +from nova import test + + +class TestNullInstanceUuidScanDB(test.TestCase): + + # NOTE(mriedem): Copied from the 267 database migration. + def downgrade(self, migrate_engine): + UniqueConstraint('uuid', + table=db_utils.get_table(migrate_engine, 'instances'), + name='uniq_instances0uuid').drop() + for table_name in ('instances', 'shadow_instances'): + table = db_utils.get_table(migrate_engine, table_name) + table.columns.uuid.alter(nullable=True) + + def setUp(self): + super(TestNullInstanceUuidScanDB, self).setUp() + + self.engine = db_api.get_engine() + # When this test runs, we've already run the schema migration to make + # instances.uuid non-nullable, so we have to alter the table here + # so we can test against a real database. + self.downgrade(self.engine) + # Now create fake entries in the fixed_ips, consoles and + # instances table where (instance_)uuid is None for testing. + for table_name in ('fixed_ips', 'instances', 'consoles'): + table = db_utils.get_table(self.engine, table_name) + fake_record = {'id': 1} + table.insert().execute(fake_record) + + def test_db_null_instance_uuid_scan_readonly(self): + results = migration.db_null_instance_uuid_scan(delete=False) + self.assertEqual(1, results.get('instances')) + self.assertEqual(1, results.get('consoles')) + # The fixed_ips table should be ignored. + self.assertNotIn('fixed_ips', results) + # Now pick a random table with an instance_uuid column and show it's + # in the results but with 0 hits. + self.assertEqual(0, results.get('instance_info_caches')) + # Make sure nothing was deleted. + for table_name in ('fixed_ips', 'instances', 'consoles'): + table = db_utils.get_table(self.engine, table_name) + record = table.select(table.c.id == 1).execute().first() + self.assertIsNotNone(record) + + def test_db_null_instance_uuid_scan_delete(self): + results = migration.db_null_instance_uuid_scan(delete=True) + self.assertEqual(1, results.get('instances')) + self.assertEqual(1, results.get('consoles')) + # The fixed_ips table should be ignored. + self.assertNotIn('fixed_ips', results) + # Now pick a random table with an instance_uuid column and show it's + # in the results but with 0 hits. + self.assertEqual(0, results.get('instance_info_caches')) + # Make sure fixed_ips wasn't touched, but instances and instance_faults + # records were deleted. + fixed_ips = db_utils.get_table(self.engine, 'fixed_ips') + record = fixed_ips.select(fixed_ips.c.id == 1).execute().first() + self.assertIsNotNone(record) + + consoles = db_utils.get_table(self.engine, 'consoles') + record = consoles.select(consoles.c.id == 1).execute().first() + self.assertIsNone(record) + + instances = db_utils.get_table(self.engine, 'instances') + record = instances.select(instances.c.id == 1).execute().first() + self.assertIsNone(record) diff --git a/nova/tests/unit/test_nova_manage.py b/nova/tests/unit/test_nova_manage.py index 9ffaf66e816c..0f958d39d6eb 100644 --- a/nova/tests/unit/test_nova_manage.py +++ b/nova/tests/unit/test_nova_manage.py @@ -22,6 +22,7 @@ import mock from nova.cmd import manage from nova import context from nova import db +from nova.db import migration from nova import exception from nova.i18n import _ from nova import test @@ -336,6 +337,36 @@ class DBCommandsTestCase(test.TestCase): def test_archive_deleted_rows_negative(self): self.assertEqual(1, self.commands.archive_deleted_rows(-1)) + @mock.patch.object(migration, 'db_null_instance_uuid_scan', + return_value={'foo': 0}) + def test_null_instance_uuid_scan_no_records_found(self, mock_scan): + self.useFixture(fixtures.MonkeyPatch('sys.stdout', + StringIO.StringIO())) + self.commands.null_instance_uuid_scan() + self.assertIn("There were no records found", sys.stdout.getvalue()) + + @mock.patch.object(migration, 'db_null_instance_uuid_scan', + return_value={'foo': 1, 'bar': 0}) + def _test_null_instance_uuid_scan(self, mock_scan, delete): + self.useFixture(fixtures.MonkeyPatch('sys.stdout', + StringIO.StringIO())) + self.commands.null_instance_uuid_scan(delete) + output = sys.stdout.getvalue() + + if delete: + self.assertIn("Deleted 1 records from table 'foo'.", output) + self.assertNotIn("Deleted 0 records from table 'bar'.", output) + else: + self.assertIn("1 records in the 'foo' table", output) + self.assertNotIn("0 records in the 'bar' table", output) + self.assertNotIn("There were no records found", output) + + def test_null_instance_uuid_scan_readonly(self): + self._test_null_instance_uuid_scan(delete=False) + + def test_null_instance_uuid_scan_delete(self): + self._test_null_instance_uuid_scan(delete=True) + class ServiceCommandsTestCase(test.TestCase): def setUp(self):