From e73f828057f8ec3d0693f1c498e59600caf1eeb6 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Wed, 11 Jul 2018 12:51:11 -0400 Subject: [PATCH] Add nova-status upgrade check for request spec migrations We can't easily add a blocker db sync migration to make sure the migrate_instances_add_request_spec online data migration has been run since we have to iterate both cells (for instances) and the API DB (for request specs) and that's not something we should do during a db sync call. But we want to eventually drop the online data migration and the accompanying compat code found in the api and conductor services. This adds a nova-status upgrade check for missing request specs and fails if any existing non-deleted instances are found which don't have a matching request spec. Related to blueprint request-spec-use-by-compute Change-Id: I1fb63765f0b0e8f35d6a66dccf9d12cc20e9c661 --- doc/source/cli/nova-status.rst | 2 + nova/cmd/manage.py | 4 + nova/cmd/status.py | 64 ++++++++++++- nova/tests/unit/cmd/test_status.py | 95 +++++++++++++++++++ ...equestspec-migration-2a3b50b98fff9324.yaml | 6 ++ 5 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/nova-status-check-requestspec-migration-2a3b50b98fff9324.yaml diff --git a/doc/source/cli/nova-status.rst b/doc/source/cli/nova-status.rst index 6ca9bb2a425f..47c2f6f2f3c2 100644 --- a/doc/source/cli/nova-status.rst +++ b/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 ======== diff --git a/nova/cmd/manage.py b/nova/cmd/manage.py index c696393f0ff8..5605e22c356a 100644 --- a/nova/cmd/manage.py +++ b/nova/cmd/manage.py @@ -384,6 +384,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, diff --git a/nova/cmd/status.py b/nova/cmd/status.py index 523e5a1e3dc7..831e11f9edd0 100644 --- a/nova/cmd/status.py +++ b/nova/cmd/status.py @@ -513,6 +513,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 @@ -530,7 +590,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): diff --git a/nova/tests/unit/cmd/test_status.py b/nova/tests/unit/cmd/test_status.py index 97e12db4432a..c47191be187f 100644 --- a/nova/tests/unit/cmd/test_status.py +++ b/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) diff --git a/releasenotes/notes/nova-status-check-requestspec-migration-2a3b50b98fff9324.yaml b/releasenotes/notes/nova-status-check-requestspec-migration-2a3b50b98fff9324.yaml new file mode 100644 index 000000000000..c66a91c75fed --- /dev/null +++ b/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.