Create Instance from BuildRequest if not in a cell

This is preliminary work towards moving instance creation until after
scheduling. In order to do that we still need the ability to honor an
instance show request. This does a lookup to determine if an instance is
in a cell and if not it synthesizes an instance from the BuildRequest
object that has been stored.

If the instance is not mapped then the deployment has not migrated to
their first cell yet and the instance lookup proceeds as usual.

Until instance creation is moved to after scheduling this is mostly a
noop.

Honoring the instance list command will come later.

Change-Id: Ib984c30543acb3ca9cb95fb53d44d9ded0f5a5c8
Partially-implements: bp add-buildrequest-obj
Partially-implements: bp cells-scheduling-interaction

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)

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

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)
 

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

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

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,

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)