Enforce unique instance uuid in data model

This adds the database migration that checks for records
where instance_uuid is NULL and fails if found until the
'nova-manage db null_instance_uuid_scan' command is run
and the records are manually deleted.

Unit tests are added for the migration to make sure the
fixed_ips table is left alone and the UniqueConstraint is
created on the instances table.

DocImpact: New 'nova-manage db null_instance_uuid_scan'
           command.

UpgradeImpact: The new database migration script.

Implements blueprint enforce-unique-instance-uuid-in-db

Change-Id: I768c36b140c7e9f437907d4dfe175e1410ca44b0

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."""

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)

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)

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)

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))

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):

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)

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):