diff --git a/nova/compute/api.py b/nova/compute/api.py index a6b9c45b3982..d6b699ff1869 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -2107,6 +2107,72 @@ class API(base.Base): self.compute_rpcapi.trigger_crash_dump(context, instance) + def _get_instance_map_or_none(self, context, instance_uuid): + try: + inst_map = objects.InstanceMapping.get_by_instance_uuid( + context, instance_uuid) + except exception.InstanceMappingNotFound: + # InstanceMapping should always be found generally. This exception + # may be raised if a deployment has partially migrated the nova-api + # services. + inst_map = None + return inst_map + + def _get_instance(self, context, instance_uuid, expected_attrs): + # 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-api') + if service_version < 15: + return objects.Instance.get_by_uuid(context, instance_uuid, + expected_attrs=expected_attrs) + inst_map = self._get_instance_map_or_none(context, instance_uuid) + if inst_map and (inst_map.cell_mapping is not None): + with nova_context.target_cell(context, inst_map.cell_mapping): + instance = objects.Instance.get_by_uuid( + context, instance_uuid, expected_attrs=expected_attrs) + elif inst_map and (inst_map.cell_mapping is None): + # This means the instance has not been scheduled and put in + # a cell yet. For now it also may mean that the deployer + # has not created their cell(s) yet. + try: + build_req = objects.BuildRequest.get_by_instance_uuid( + context, instance_uuid) + instance = build_req.instance + except exception.BuildRequestNotFound: + # Instance was mapped and the BuildRequest was deleted + # while fetching. Try again. + inst_map = self._get_instance_map_or_none(context, + instance_uuid) + if inst_map and (inst_map.cell_mapping is not None): + with nova_context.target_cell(context, + inst_map.cell_mapping): + instance = objects.Instance.get_by_uuid( + context, instance_uuid, + expected_attrs=expected_attrs) + else: + # If BuildRequest is not found but inst_map.cell_mapping + # does not point at a cell then cell migration has not + # happened yet. This will be a failure case later. + # TODO(alaski): Make this a failure case after we put in + # a block that requires migrating to cellsv2. + instance = objects.Instance.get_by_uuid( + context, instance_uuid, expected_attrs=expected_attrs) + else: + # This should not happen if we made it past the service_version + # check above. But it does not need to be an exception yet. + LOG.warning(_LW('No instance_mapping found for instance %s. This ' + 'should not be happening and will lead to errors ' + 'in the future. Please open a bug at ' + 'https://bugs.launchpad.net/nova'), instance_uuid) + instance = objects.Instance.get_by_uuid( + context, instance_uuid, expected_attrs=expected_attrs) + + return instance + def get(self, context, instance_id, expected_attrs=None): """Get a single instance with the given instance_id.""" if not expected_attrs: @@ -2118,8 +2184,9 @@ class API(base.Base): if uuidutils.is_uuid_like(instance_id): LOG.debug("Fetching instance by UUID", instance_uuid=instance_id) - instance = objects.Instance.get_by_uuid( - context, instance_id, expected_attrs=expected_attrs) + + instance = self._get_instance(context, instance_id, + expected_attrs) else: LOG.debug("Failed to fetch instance by id %s", instance_id) raise exception.InstanceNotFound(instance_id=instance_id) diff --git a/nova/objects/build_request.py b/nova/objects/build_request.py index 141740dac81c..6477d0ea4797 100644 --- a/nova/objects/build_request.py +++ b/nova/objects/build_request.py @@ -78,6 +78,26 @@ class BuildRequest(base.NovaObject): LOG.exception(_LE('Could not deserialize instance in ' 'BuildRequest')) raise exception.BuildRequestNotFound(uuid=self.instance_uuid) + # NOTE(alaski): Set some fields on instance that are needed by the api, + # not lazy-loadable, and don't change. + self.instance.deleted = 0 + self.instance.disable_terminate = False + self.instance.terminated_at = None + self.instance.host = None + self.instance.node = None + self.instance.launched_at = None + self.instance.launched_on = None + self.instance.cell_name = None + # The fields above are not set until the instance is in a cell at + # which point this BuildRequest will be gone. locked_by could + # potentially be set by an update so it should not be overwritten. + if not self.instance.obj_attr_is_set('locked_by'): + self.instance.locked_by = None + # created_at/updated_at are not on the serialized instance because it + # was never persisted. + self.instance.created_at = self.created_at + self.instance.updated_at = self.updated_at + self.instance.tags = objects.TagList([]) def _load_block_device_mappings(self, db_bdms): # 'db_bdms' is a serialized BlockDeviceMappingList object. If it's None @@ -102,13 +122,17 @@ class BuildRequest(base.NovaObject): req.instance_uuid = db_req['instance_uuid'] for key in req.fields: - if isinstance(req.fields[key], fields.ObjectField): + if key == 'instance': + continue + elif isinstance(req.fields[key], fields.ObjectField): try: getattr(req, '_load_%s' % key)(db_req[key]) except AttributeError: LOG.exception(_LE('No load handler for %s'), key) else: setattr(req, key, db_req[key]) + # Load instance last because other fields on req may be referenced + req._load_instance(db_req['instance']) req.obj_reset_changes() req._context = context return req diff --git a/nova/tests/functional/db/test_build_request.py b/nova/tests/functional/db/test_build_request.py index 3eadf5cd75de..3cddb072eaee 100644 --- a/nova/tests/functional/db/test_build_request.py +++ b/nova/tests/functional/db/test_build_request.py @@ -10,9 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import functools - -from oslo_serialization import jsonutils from oslo_utils import uuidutils from nova import context @@ -22,7 +19,6 @@ from nova.objects import build_request from nova import test from nova.tests import fixtures from nova.tests.unit import fake_build_request -from nova.tests.unit.objects import test_objects class BuildRequestTestCase(test.NoDBTestCase): @@ -53,52 +49,21 @@ class BuildRequestTestCase(test.NoDBTestCase): self.instance_uuid) def test_get_by_uuid(self): - req = self._create_req() - db_req = self.build_req_obj._get_by_instance_uuid_from_db(self.context, - self.instance_uuid) - - obj_comp = functools.partial(objects.base.obj_equal_prims, - ignore=['deleted', 'deleted_at', 'created_at', - 'updated_at']) - - def date_comp(db_val, obj_val): - # We have this separate comparison method because compare_obj below - # assumes that db datetimes are tz unaware. That's normally true - # but not when they're part of a serialized object and not a - # dedicated datetime column. - self.assertEqual(db_val.replace(tzinfo=None), - obj_val.replace(tzinfo=None)) + expected_req = self._create_req() + req_obj = self.build_req_obj.get_by_instance_uuid(self.context, + self.instance_uuid) for key in self.build_req_obj.fields.keys(): - expected = getattr(req, key) - db_value = db_req[key] - if key in ['created_at', 'updated_at']: - # Objects store tz aware datetimes but the db does not. - expected = expected.replace(tzinfo=None) - elif key == 'instance': - db_instance = objects.Instance.obj_from_primitive( - jsonutils.loads(db_value)) - test_objects.compare_obj(self, expected, db_instance, - # These objects are not loaded in the test instance - allow_missing=['pci_requests', 'numa_topology', - 'pci_devices', 'security_groups', 'info_cache', - 'ec2_ids', 'migration_context', 'metadata', - 'vcpu_model', 'services', 'system_metadata', - 'tags', 'fault', 'device_metadata'], - comparators={'flavor': obj_comp, - 'created_at': date_comp, - 'keypairs': obj_comp}) + expected = getattr(expected_req, key) + db_value = getattr(req_obj, key) + if key == 'instance': + objects.base.obj_equal_prims(expected, db_value) continue elif key == 'block_device_mappings': - db_bdms = objects.BlockDeviceMappingList.obj_from_primitive( - jsonutils.loads(db_value)) - self.assertEqual(1, len(db_bdms)) + self.assertEqual(1, len(db_value)) # Can't compare list objects directly, just compare the single # item they contain. - test_objects.compare_obj(self, expected[0], db_bdms[0], - allow_missing=['instance'], - comparators={'created_at': date_comp, - 'updated_at': date_comp}) + objects.base.obj_equal_prims(expected[0], db_value[0]) continue self.assertEqual(expected, db_value) diff --git a/nova/tests/functional/wsgi/test_servers.py b/nova/tests/functional/wsgi/test_servers.py index 12e71aeb1615..828a5bcb31e1 100644 --- a/nova/tests/functional/wsgi/test_servers.py +++ b/nova/tests/functional/wsgi/test_servers.py @@ -44,6 +44,62 @@ class ServersPreSchedulingTestCase(test.TestCase): self.api = api_fixture.api self.api.microversion = 'latest' + def test_instance_from_buildrequest(self): + self.useFixture(nova_fixtures.AllServicesCurrent()) + image_ref = fake_image.get_valid_image_id() + body = { + 'server': { + 'name': 'foo', + 'imageRef': image_ref, + 'flavorRef': '1', + 'networks': 'none', + } + } + create_resp = self.api.api_post('servers', body) + get_resp = self.api.api_get('servers/%s' % + create_resp.body['server']['id']) + + server = get_resp.body['server'] + # Validate a few things + self.assertEqual('foo', server['name']) + self.assertEqual(image_ref, server['image']['id']) + self.assertEqual('1', server['flavor']['id']) + self.assertEqual('', server['hostId']) + self.assertIsNone(None, server['OS-SRV-USG:launched_at']) + self.assertIsNone(None, server['OS-SRV-USG:terminated_at']) + self.assertFalse(server['locked']) + self.assertEqual([], server['tags']) + self.assertEqual('scheduling', server['OS-EXT-STS:task_state']) + self.assertEqual('building', server['OS-EXT-STS:vm_state']) + self.assertEqual('BUILD', server['status']) + + def test_instance_from_buildrequest_old_service(self): + image_ref = fake_image.get_valid_image_id() + body = { + 'server': { + 'name': 'foo', + 'imageRef': image_ref, + 'flavorRef': '1', + 'networks': 'none', + } + } + create_resp = self.api.api_post('servers', body) + get_resp = self.api.api_get('servers/%s' % + create_resp.body['server']['id']) + server = get_resp.body['server'] + # Just validate some basics + self.assertEqual('foo', server['name']) + self.assertEqual(image_ref, server['image']['id']) + self.assertEqual('1', server['flavor']['id']) + self.assertEqual('', server['hostId']) + self.assertIsNone(None, server['OS-SRV-USG:launched_at']) + self.assertIsNone(None, server['OS-SRV-USG:terminated_at']) + self.assertFalse(server['locked']) + self.assertEqual([], server['tags']) + self.assertEqual('scheduling', server['OS-EXT-STS:task_state']) + self.assertEqual('building', server['OS-EXT-STS:vm_state']) + self.assertEqual('BUILD', server['status']) + def test_delete_instance_from_buildrequest(self): self.useFixture(nova_fixtures.AllServicesCurrent()) image_ref = fake_image.get_valid_image_id() diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 7cddc7abe04f..faaaff382430 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -47,6 +47,7 @@ from nova import quota from nova import test from nova.tests import fixtures from nova.tests.unit import fake_block_device +from nova.tests.unit import fake_build_request from nova.tests.unit import fake_instance from nova.tests.unit import fake_volume from nova.tests.unit.image import fake as fake_image @@ -1161,6 +1162,8 @@ class _ComputeAPIUnitTestMixIn(object): quotas = quotas_obj.Quotas(self.context) updates = {'progress': 0, 'task_state': task_states.DELETING} + self.mox.StubOutWithMock(objects.BuildRequest, + 'get_by_instance_uuid') self.mox.StubOutWithMock(inst, 'save') self.mox.StubOutWithMock(objects.BlockDeviceMappingList, 'get_by_instance_uuid') @@ -4069,6 +4072,158 @@ class _ComputeAPIUnitTestMixIn(object): self.context, requested_networks, 5) self.assertEqual(4, count) + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid', + side_effect=exception.InstanceMappingNotFound(uuid='fake')) + @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid') + @mock.patch.object(objects.Instance, 'get_by_uuid') + def test_get_instance_no_mapping(self, mock_get_inst, mock_get_build_req, + mock_get_inst_map): + + self.useFixture(fixtures.AllServicesCurrent()) + # Just check that an InstanceMappingNotFound causes the instance to + # get looked up normally. + self.compute_api.get(self.context, uuids.inst_uuid) + mock_get_build_req.assert_not_called() + mock_get_inst_map.assert_called_once_with(self.context, + uuids.inst_uuid) + mock_get_inst.assert_called_once_with(self.context, uuids.inst_uuid, + expected_attrs=[ + 'metadata', + 'system_metadata', + 'security_groups', + 'info_cache']) + + @mock.patch.object(objects.InstanceMapping, 'get_by_instance_uuid') + @mock.patch.object(objects.BuildRequest, 'get_by_instance_uuid') + def test_get_instance_not_in_cell(self, mock_get_build_req, + mock_get_inst_map): + self.useFixture(fixtures.AllServicesCurrent()) + build_req_obj = fake_build_request.fake_req_obj(self.context) + mock_get_inst_map.return_value = objects.InstanceMapping( + cell_mapping=None) + mock_get_build_req.return_value = build_req_obj + + instance = build_req_obj.instance + inst_from_build_req = self.compute_api.get(self.context, instance.uuid) + mock_get_inst_map.assert_called_once_with(self.context, instance.uuid) + mock_get_build_req.assert_called_once_with(self.context, instance.uuid) + self.assertEqual(instance, inst_from_build_req) + + @mock.patch.object(context, 'target_cell') + @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_buildreq_deleted_inst_in_cell( + self, mock_get_inst, mock_get_build_req, mock_get_inst_map, + mock_target_cell): + # This test checks the following scenario: + # The instance is not mapped to a cell, so it should be retrieved from + # a BuildRequest object. However the BuildRequest does not exist + # because the instance was put in a cell and mapped while while + # attempting to get the BuildRequest. So pull the instance from the + # cell. + self.useFixture(fixtures.AllServicesCurrent()) + build_req_obj = fake_build_request.fake_req_obj(self.context) + instance = build_req_obj.instance + inst_map = objects.InstanceMapping(cell_mapping=objects.CellMapping()) + + mock_get_inst_map.side_effect = [ + objects.InstanceMapping(cell_mapping=None), inst_map] + mock_get_build_req.side_effect = exception.BuildRequestNotFound( + uuid=instance.uuid) + mock_get_inst.return_value = instance + + inst_from_get = self.compute_api.get(self.context, instance.uuid) + + inst_map_calls = [mock.call(self.context, instance.uuid), + mock.call(self.context, instance.uuid)] + mock_get_inst_map.assert_has_calls(inst_map_calls) + self.assertEqual(2, mock_get_inst_map.call_count) + mock_get_build_req.assert_called_once_with(self.context, instance.uuid) + mock_target_cell.assert_called_once_with(self.context, + inst_map.cell_mapping) + mock_get_inst.assert_called_once_with(self.context, instance.uuid, + expected_attrs=[ + 'metadata', + 'system_metadata', + 'security_groups', + 'info_cache']) + self.assertEqual(instance, inst_from_get) + + @mock.patch.object(context, 'target_cell') + @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_buildreq_deleted_inst_still_not_in_cell( + self, mock_get_inst, mock_get_build_req, mock_get_inst_map, + mock_target_cell): + # This test checks the following scenario: + # The instance is not mapped to a cell, so it should be retrieved from + # a BuildRequest object. However the BuildRequest does not exist which + # means it should now be possible to find the instance in a cell db. + # But the instance is not mapped which means the cellsv2 migration has + # not occurred in this scenario, so the instance is pulled from the + # configured Nova db. + + # TODO(alaski): The tested case will eventually be an error condition. + # But until we force cellsv2 migrations we need this to work. + self.useFixture(fixtures.AllServicesCurrent()) + build_req_obj = fake_build_request.fake_req_obj(self.context) + instance = build_req_obj.instance + + mock_get_inst_map.side_effect = [ + objects.InstanceMapping(cell_mapping=None), + objects.InstanceMapping(cell_mapping=None)] + mock_get_build_req.side_effect = exception.BuildRequestNotFound( + uuid=instance.uuid) + mock_get_inst.return_value = instance + + inst_from_get = self.compute_api.get(self.context, instance.uuid) + + inst_map_calls = [mock.call(self.context, instance.uuid), + mock.call(self.context, instance.uuid)] + mock_get_inst_map.assert_has_calls(inst_map_calls) + self.assertEqual(2, mock_get_inst_map.call_count) + mock_get_build_req.assert_called_once_with(self.context, instance.uuid) + mock_target_cell.assert_not_called() + mock_get_inst.assert_called_once_with(self.context, instance.uuid, + expected_attrs=[ + 'metadata', + 'system_metadata', + 'security_groups', + 'info_cache']) + self.assertEqual(instance, inst_from_get) + + @mock.patch.object(context, 'target_cell') + @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_in_cell(self, mock_get_inst, mock_get_build_req, + mock_get_inst_map, mock_target_cell): + self.useFixture(fixtures.AllServicesCurrent()) + # This just checks that the instance is looked up normally and not + # synthesized from a BuildRequest object. Verification of pulling the + # instance from the proper cell will be added when that capability is. + instance = self._create_instance_obj() + build_req_obj = fake_build_request.fake_req_obj(self.context) + inst_map = objects.InstanceMapping(cell_mapping=objects.CellMapping()) + mock_get_inst_map.return_value = inst_map + mock_get_build_req.return_value = build_req_obj + mock_get_inst.return_value = instance + + returned_inst = self.compute_api.get(self.context, instance.uuid) + mock_get_build_req.assert_not_called() + mock_get_inst_map.assert_called_once_with(self.context, instance.uuid) + self.assertEqual(instance, returned_inst) + mock_target_cell.assert_called_once_with(self.context, + inst_map.cell_mapping) + mock_get_inst.assert_called_once_with(self.context, instance.uuid, + expected_attrs=[ + 'metadata', + 'system_metadata', + 'security_groups', + 'info_cache']) + class ComputeAPIUnitTestCase(_ComputeAPIUnitTestMixIn, test.NoDBTestCase): def setUp(self): diff --git a/nova/tests/unit/fake_build_request.py b/nova/tests/unit/fake_build_request.py index 8c391f2eee44..6bdf06c0d74b 100644 --- a/nova/tests/unit/fake_build_request.py +++ b/nova/tests/unit/fake_build_request.py @@ -28,6 +28,8 @@ def fake_db_req(**updates): instance_uuid = uuidutils.generate_uuid() instance = fake_instance.fake_instance_obj(ctxt, objects.Instance, uuid=instance_uuid) + # This will always be set this way for an instance at build time + instance.host = None block_devices = objects.BlockDeviceMappingList( objects=[fake_block_device.fake_bdm_object( context, diff --git a/nova/tests/unit/objects/test_build_request.py b/nova/tests/unit/objects/test_build_request.py index 661d53952e05..f1ac65928374 100644 --- a/nova/tests/unit/objects/test_build_request.py +++ b/nova/tests/unit/objects/test_build_request.py @@ -64,6 +64,22 @@ class _TestBuildRequestObject(object): build_request.BuildRequest.get_by_instance_uuid, self.context, fake_req['instance_uuid']) + @mock.patch.object(build_request.BuildRequest, + '_get_by_instance_uuid_from_db') + def test_get_by_instance_uuid_do_not_override_locked_by(self, get_by_uuid): + fake_req = fake_build_request.fake_db_req() + instance = fake_instance.fake_instance_obj(self.context, + objects.Instance, uuid=fake_req['instance_uuid']) + instance.locked_by = 'admin' + fake_req['instance'] = jsonutils.dumps(instance.obj_to_primitive()) + get_by_uuid.return_value = fake_req + + req_obj = build_request.BuildRequest.get_by_instance_uuid(self.context, + fake_req['instance_uuid']) + + self.assertIsInstance(req_obj.instance, objects.Instance) + self.assertEqual('admin', req_obj.instance.locked_by) + def test_create(self): fake_req = fake_build_request.fake_db_req() req_obj = fake_build_request.fake_req_obj(self.context, fake_req)