Merge "Create Instance from BuildRequest if not in a cell"

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)