Merge "Add nova-status upgrade check for request spec migrations"

doc/source/cli/nova-status.rst

@@ -115,6 +115,8 @@ Upgrade
     across all cell mappings which might cause issues when querying instances
     depending on how the **nova-api** service is configured.
     See https://bugs.launchpad.net/nova/+bug/1759316 for details.
+  * Checks that existing instances have been migrated to have a matching
+    request spec in the API DB.
 
 See Also
 ========

nova/cmd/manage.py

@@ -386,6 +386,10 @@ class DbCommands(object):
     # finished.
     online_migrations = (
         # Added in Newton
+        # TODO(mriedem): Remove this in Stein along with the compatibility
+        # code in the api and conductor services; the nova-status upgrade check
+        # added in Rocky is the tool operators can use to make sure they have
+        # completed this migration.
         request_spec.migrate_instances_add_request_spec,
         # Added in Newton
         keypair_obj.migrate_keypairs_to_api_db,

nova/cmd/status.py

@@ -514,6 +514,66 @@ class UpgradeCommands(object):
             return UpgradeCheckResult(UpgradeCheckCode.WARNING, msg)
         return UpgradeCheckResult(UpgradeCheckCode.SUCCESS)
 
+    def _check_request_spec_migration(self):
+        """Checks to make sure request spec migrations are complete.
+
+        Iterates all cells checking to see that non-deleted instances have
+        a matching request spec in the API database. This is necessary in order
+        to drop the migrate_instances_add_request_spec online data migration
+        and accompanying compatibility code found through nova-api and
+        nova-conductor.
+        """
+        meta = MetaData(bind=db_session.get_api_engine())
+        cell_mappings = Table('cell_mappings', meta, autoload=True)
+        mappings = cell_mappings.select().execute().fetchall()
+
+        if not mappings:
+            # There are no cell mappings so we can't determine this, just
+            # return a warning. The cellsv2 check would have already failed
+            # on this.
+            msg = (_('Unable to determine request spec migrations without '
+                     'cell mappings.'))
+            return UpgradeCheckResult(UpgradeCheckCode.WARNING, msg)
+
+        request_specs = Table('request_specs', meta, autoload=True)
+        ctxt = nova_context.get_admin_context()
+        incomplete_cells = []  # list of cell mapping uuids
+        for mapping in mappings:
+            with nova_context.target_cell(ctxt, mapping) as cctxt:
+                # Get all instance uuids for non-deleted instances in this
+                # cell.
+                meta = MetaData(bind=db_session.get_engine(context=cctxt))
+                instances = Table('instances', meta, autoload=True)
+                instance_records = (
+                    select([instances.c.uuid]).select_from(instances).where(
+                        instances.c.deleted == 0
+                    ).execute().fetchall())
+                # For each instance in the list, verify that it has a matching
+                # request spec in the API DB.
+                for inst in instance_records:
+                    spec_id = (
+                        select([request_specs.c.id]).select_from(
+                            request_specs).where(
+                            request_specs.c.instance_uuid == inst['uuid']
+                        ).execute().scalar())
+                    if spec_id is None:
+                        # This cell does not have all of its instances
+                        # migrated for request specs so track it and move on.
+                        incomplete_cells.append(mapping['uuid'])
+                        break
+
+        # It's a failure if there are any unmigrated instances at this point
+        # because we are planning to drop the online data migration routine and
+        # compatibility code in Stein.
+        if incomplete_cells:
+            msg = (_("The following cells have instances which do not have "
+                     "matching request_specs in the API database: %s Run "
+                     "'nova-manage db online_data_migrations' on each cell "
+                     "to create the missing request specs.") %
+                   ', '.join(incomplete_cells))
+            return UpgradeCheckResult(UpgradeCheckCode.FAILURE, msg)
+        return UpgradeCheckResult(UpgradeCheckCode.SUCCESS)
+
     # The format of the check functions is to return an UpgradeCheckResult
     # object with the appropriate UpgradeCheckCode and details set. If the
     # check hits warnings or failures then those should be stored in the
@@ -531,7 +591,9 @@ class UpgradeCommands(object):
         # Added in Rocky (but also useful going back to Pike)
         (_('Ironic Flavor Migration'), _check_ironic_flavor_migration),
         # Added in Rocky (but is backportable to Ocata)
-        (_('API Service Version'), _check_api_service_version)
+        (_('API Service Version'), _check_api_service_version),
+        # Added in Rocky
+        (_('Request Spec Migration'), _check_request_spec_migration),
     )
 
     def _get_details(self, upgrade_check_result):

nova/tests/unit/cmd/test_status.py

@@ -33,6 +33,7 @@ from nova import context
 # NOTE(mriedem): We only use objects as a convenience to populate the database
 # in the tests, we don't use them in the actual CLI.
 from nova import objects
+from nova.objects import request_spec as reqspec_obj
 from nova import rc_fields as fields
 from nova import test
 from nova.tests import fixtures as nova_fixtures
@@ -916,3 +917,97 @@ class TestUpgradeCheckAPIServiceVersion(test.NoDBTestCase):
 
         result = self.cmd._check_api_service_version()
         self.assertEqual(status.UpgradeCheckCode.SUCCESS, result.code)
+
+
+class TestUpgradeCheckRequestSpecMigration(test.NoDBTestCase):
+    """Tests for the nova-status upgrade check for request spec migration."""
+
+    # We'll setup the database ourselves because we need to use cells fixtures
+    # for multiple cell mappings.
+    USES_DB_SELF = True
+
+    # This will create three cell mappings: cell0, cell1 (default) and cell2
+    NUMBER_OF_CELLS = 2
+
+    def setUp(self):
+        super(TestUpgradeCheckRequestSpecMigration, self).setUp()
+        self.output = StringIO()
+        self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output))
+        # We always need the API DB to be setup.
+        self.useFixture(nova_fixtures.Database(database='api'))
+        self.cmd = status.UpgradeCommands()
+
+    @staticmethod
+    def _create_instance_in_cell(ctxt, cell, is_deleted=False,
+                                 create_request_spec=False):
+        with context.target_cell(ctxt, cell) as cctxt:
+            inst = objects.Instance(
+                context=cctxt,
+                uuid=uuidutils.generate_uuid())
+            inst.create()
+
+            if is_deleted:
+                inst.destroy()
+
+        if create_request_spec:
+            # Fake out some fields in the Instance so we don't lazy-load them.
+            inst.flavor = objects.Flavor()
+            inst.numa_topology = None
+            inst.system_metadata = {}
+            inst.pci_requests = None
+            inst.project_id = 'fake-project'
+            inst.user_id = 'fake-user'
+            reqspec_obj._create_minimal_request_spec(ctxt, inst)
+
+        return inst
+
+    def test_fresh_install_no_cell_mappings(self):
+        """Tests the scenario where we don't have any cell mappings (no cells
+        v2 setup yet) so we don't know what state we're in and we return a
+        warning.
+        """
+        result = self.cmd._check_request_spec_migration()
+        self.assertEqual(status.UpgradeCheckCode.WARNING, result.code)
+        self.assertIn('Unable to determine request spec migrations without '
+                      'cell mappings.', result.details)
+
+    def test_deleted_instance_one_cell_migrated_other_success(self):
+        """Tests the scenario that we have two cells, one has only a single
+        deleted instance in it and the other has a single already-migrated
+        instance in it, so the overall result is success.
+        """
+        self._setup_cells()
+        ctxt = context.get_admin_context()
+
+        # Create a deleted instance in cell1.
+        self._create_instance_in_cell(
+            ctxt, self.cell_mappings['cell1'], is_deleted=True)
+
+        # Create a migrated instance in cell2.
+        self._create_instance_in_cell(
+            ctxt, self.cell_mappings['cell2'], create_request_spec=True)
+
+        result = self.cmd._check_request_spec_migration()
+        self.assertEqual(status.UpgradeCheckCode.SUCCESS, result.code)
+
+    def test_unmigrated_request_spec_instances(self):
+        """Tests the scenario that we have a migrated instance in cell1 and
+        an unmigrated instance in cell2 so the check fails.
+        """
+        self._setup_cells()
+        ctxt = context.get_admin_context()
+
+        # Create a migrated instance in cell1.
+        self._create_instance_in_cell(
+            ctxt, self.cell_mappings['cell1'], create_request_spec=True)
+
+        # Create an unmigrated instance in cell2.
+        self._create_instance_in_cell(ctxt, self.cell_mappings['cell2'])
+
+        result = self.cmd._check_request_spec_migration()
+        self.assertEqual(status.UpgradeCheckCode.FAILURE, result.code)
+        self.assertIn("The following cells have instances which do not have "
+                      "matching request_specs in the API database: %s Run "
+                      "'nova-manage db online_data_migrations' on each cell "
+                      "to create the missing request specs." %
+                      self.cell_mappings['cell2'].uuid, result.details)

releasenotes/notes/nova-status-check-requestspec-migration-2a3b50b98fff9324.yaml

@@ -0,0 +1,6 @@
+---
+upgrade:
+  - |
+    A new check is added to the ``nova-status upgrade check`` CLI to make sure
+    request spec online migrations have been run per-cell. Missing request spec
+    compatibility code is planned to be removed in the Stein release.