From 097693aaa3f57011e38188a94687e02bc9b074ab Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Fri, 2 Nov 2018 17:36:41 -0400 Subject: [PATCH] Drop pre-cellsv2 compat in compute API.get() This removes the nova-osapi_compute service version compatibility check for cells v2 which was added in Newton. A "nova-status upgrade check" was added for that in Rocky: Ie2bc4616439352850cf29a9de7d33a06c8f7c2b8 And it is also causing issues with functional tests where instances are buried in cell0 and because we don't use the instance mapping, we fail to find the instance. As a result of this, compute API tests that rely on retrieving a real instance out of the database will also need to make sure an associated instance mapping record exists. Change-Id: I11083aa3c78bd8b6201558561457f3d65007a177 Closes-Bug: #1800472 --- nova/compute/api.py | 37 ++++++--------------- nova/tests/unit/compute/test_compute.py | 28 +++++++++++----- nova/tests/unit/compute/test_compute_api.py | 5 +-- 3 files changed, 31 insertions(+), 39 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index e52d5994b705..a43183d007f7 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2439,33 +2439,9 @@ class API(base.Base): def _get_instance(self, context, instance_uuid, expected_attrs, cell_down_support=False): - # Before service version 15 the BuildRequest is not cleaned up during - # a delete request so there is no reason to look it up here as we can't - # trust that it's not referencing a deleted instance. Also even if - # there is an instance mapping we don't need to honor it for older - # service versions. - service_version = objects.Service.get_minimum_version( - context, 'nova-osapi_compute') - # If we're on cellsv1, we also need to consult the top-level - # merged replica instead of the cell directly, so fall through - # here in that case as well. - if service_version < 15 or CONF.cells.enable: - # If not using cells v1, we need to log a warning about the API - # service version being less than 15 (that check was added in - # newton), which indicates there is some lingering data during the - # transition to cells v2 which could cause an InstanceNotFound - # here. The warning message is a sort of breadcrumb. - # This can all go away once we drop cells v1 and assert that all - # deployments have upgraded from a base cells v2 setup with - # mappings. - if not CONF.cells.enable: - LOG.warning('The nova-osapi_compute service version is from ' - 'before Ocata and may cause problems looking up ' - 'instances in a cells v2 setup. Check your ' - 'nova-api service configuration and cell ' - 'mappings. You may need to remove stale ' - 'nova-osapi_compute service records from the cell ' - 'database.') + # If we're on cellsv1, we need to consult the top-level + # merged replica instead of the cell directly. + if CONF.cells.enable: return objects.Instance.get_by_uuid(context, instance_uuid, expected_attrs=expected_attrs) inst_map = self._get_instance_map_or_none(context, instance_uuid) @@ -2491,6 +2467,13 @@ class API(base.Base): else: raise exception.InstanceNotFound(instance_id=instance_uuid) else: + # If we got here, we don't have an instance mapping, but we aren't + # sure why. The instance mapping might be missing because the + # upgrade is incomplete (map_instances wasn't run). Or because the + # instance was deleted and the DB was archived at which point the + # mapping is deleted. The former case is bad, but because of the + # latter case we can't really log any kind of warning/error here + # since it might be normal. raise exception.InstanceNotFound(instance_id=instance_uuid) return instance diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 24f301fe67ff..633e2efc21a2 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -284,9 +284,10 @@ class BaseTestCase(test.TestCase): return fake_instance.fake_instance_obj(None, **updates) def _create_fake_instance_obj(self, params=None, type_name='m1.tiny', - services=False, context=None): + services=False, ctxt=None): + ctxt = ctxt or self.context flavor = flavors.get_flavor_by_name(type_name) - inst = objects.Instance(context=context or self.context) + inst = objects.Instance(context=ctxt) inst.vm_state = vm_states.ACTIVE inst.task_state = None inst.power_state = power_state.RUNNING @@ -321,7 +322,18 @@ class BaseTestCase(test.TestCase): if services: _create_service_entries(self.context.elevated(), [['fake_zone', [inst.host]]]) - inst.create() + cell1 = self.cell_mappings[test.CELL1_NAME] + with context.target_cell(ctxt, cell1) as cctxt: + inst._context = cctxt + inst.create() + + # Create an instance mapping in cell1 so the API can get the instance. + inst_map = objects.InstanceMapping( + ctxt, + instance_uuid=inst.uuid, + project_id=inst.project_id, + cell_mapping=cell1) + inst_map.create() return inst @@ -6270,7 +6282,7 @@ class ComputeTestCase(BaseTestCase, c = context.get_admin_context() params = {'info_cache': objects.InstanceInfoCache( network_info=network_model.NetworkInfo([]))} - instance = self._create_fake_instance_obj(params=params, context=c) + instance = self._create_fake_instance_obj(params=params, ctxt=c) instance.host = self.compute.host dest = 'desthost' @@ -6326,7 +6338,7 @@ class ComputeTestCase(BaseTestCase, c = context.get_admin_context() params = {'info_cache': objects.InstanceInfoCache( network_info=network_model.NetworkInfo([]))} - instance = self._create_fake_instance_obj(params=params, context=c) + instance = self._create_fake_instance_obj(params=params, ctxt=c) instance.host = self.compute.host dest = 'desthost' @@ -6430,7 +6442,7 @@ class ComputeTestCase(BaseTestCase, 'host': srchost, 'state_description': 'migrating', 'state': power_state.PAUSED}, - context=c) + ctxt=c) instance.update({'task_state': task_states.MIGRATING, 'power_state': power_state.PAUSED}) @@ -6510,7 +6522,7 @@ class ComputeTestCase(BaseTestCase, 'node': srcnode, 'state_description': 'migrating', 'state': power_state.PAUSED}, - context=c) + ctxt=c) instance.update({'task_state': task_states.MIGRATING, 'power_state': power_state.PAUSED}) @@ -6561,7 +6573,7 @@ class ComputeTestCase(BaseTestCase, 'host': self.compute.host, 'state_description': 'migrating', 'state': power_state.PAUSED}, - context=c) + ctxt=c) instance.update({'task_state': task_states.MIGRATING, 'power_state': power_state.PAUSED}) instance.save() diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 1eed3f97eb3c..dca76b475fa9 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -5856,12 +5856,11 @@ class _ComputeAPIUnitTestMixIn(object): 'security_groups', 'info_cache']) - @mock.patch.object(objects.Service, 'get_minimum_version', return_value=15) @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid') @mock.patch.object(objects.Instance, 'get_by_uuid') def test_get_instance_not_in_cell(self, mock_get_inst, mock_get_build_req, - mock_get_inst_map, mock_get_min_service): + mock_get_inst_map): build_req_obj = fake_build_request.fake_req_obj(self.context) mock_get_inst_map.return_value = objects.InstanceMapping( cell_mapping=None) @@ -5881,8 +5880,6 @@ class _ComputeAPIUnitTestMixIn(object): expected_attrs=['metadata', 'system_metadata', 'security_groups', 'info_cache']) self.assertEqual(instance, inst_from_build_req) - mock_get_min_service.assert_called_once_with(self.context, - 'nova-osapi_compute') @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid')