Return build_requests instead of instances

This patch is preparation step to move the instance creation to
the conductor. The goal is to move out the instance.create call from
the _provision_instances method.

The _provision_instances method return value is changed
from list of instances to the tuple containing a list of build_requests
and RequestSpec object. We also return the instance_mapping list so
that we can properly clean up residue if we fail to create in the caller.

Co-Authored-By: Dan Smith <dansmith@redhat.com>
Implements: bp cells-scheduling-interaction
Change-Id: I60abcd4f27dc877c4e420071be77c9fdb697ad99

nova/compute/api.py

@@ -939,9 +939,7 @@ class API(base.Base):
                 security_groups)
         self.security_group_api.ensure_default(context)
         LOG.debug("Going to run %s instances...", num_instances)
-        instances = []
+        instances_to_build = []
-        instance_mappings = []
-        build_requests = []
         try:
             for i in range(num_instances):
                 # Create a uuid for the instance so we can store the
@@ -971,12 +969,16 @@ class API(base.Base):
                     self._bdm_validate_set_size_and_instance(context,
                         instance, instance_type, block_device_mapping))
 
+                # NOTE(danms): BDMs are still not created, so we need to pass
+                # a clone and then reset them on our object after create so
+                # that they're still dirty for later in this process
                 build_request = objects.BuildRequest(context,
                         instance=instance, instance_uuid=instance.uuid,
                         project_id=instance.project_id,
-                        block_device_mappings=block_device_mapping)
+                        block_device_mappings=block_device_mapping.obj_clone())
                 build_request.create()
-                build_requests.append(build_request)
+                build_request.block_device_mappings = block_device_mapping
+
                 # Create an instance_mapping.  The null cell_mapping indicates
                 # that the instance doesn't yet exist in a cell, and lookups
                 # for it need to instead look for the RequestSpec.
@@ -988,17 +990,9 @@ class API(base.Base):
                 inst_mapping.project_id = context.project_id
                 inst_mapping.cell_mapping = None
                 inst_mapping.create()
-                instance_mappings.append(inst_mapping)
-                # TODO(alaski): Cast to conductor here which will call the
-                # scheduler and defer instance creation until the scheduler
-                # has picked a cell/host. Set the instance_mapping to the cell
-                # that the instance is scheduled to.
-                # NOTE(alaski): Instance and block device creation are going
-                # to move to the conductor.
-                instance.create()
-                instances.append(instance)
 
-                self._create_block_device_mapping(block_device_mapping)
+                instances_to_build.append(
+                    (req_spec, build_request, inst_mapping))
 
                 if instance_group:
                     if check_server_group_quota:
@@ -1021,29 +1015,22 @@ class API(base.Base):
                     # instance
                     instance_group.members.extend(members)
 
-                # send a state update notification for the initial create to
-                # show it going from non-existent to BUILDING
-                notifications.send_update_with_states(context, instance, None,
-                        vm_states.BUILDING, None, None, service="api")
-
         # In the case of any exceptions, attempt DB cleanup and rollback the
         # quota reservations.
         except Exception:
             with excutils.save_and_reraise_exception():
                 try:
-                    for instance in instances:
+                    for rs, br, im in instances_to_build:
                         try:
-                            instance.destroy()
+                            rs.destroy()
-                        except exception.ObjectActionError:
+                        except exception.RequestSpecNotFound:
                             pass
-                    for instance_mapping in instance_mappings:
                         try:
-                            instance_mapping.destroy()
+                            im.destroy()
                         except exception.InstanceMappingNotFound:
                             pass
-                    for build_request in build_requests:
                         try:
-                            build_request.destroy()
+                            br.destroy()
                         except exception.BuildRequestNotFound:
                             pass
                 finally:
@@ -1051,7 +1038,8 @@ class API(base.Base):
 
         # Commit the reservations
         quotas.commit()
-        return instances
+
+        return instances_to_build
 
     def _get_bdm_image_metadata(self, context, block_device_mapping,
                                 legacy_bdm=True):
@@ -1112,6 +1100,30 @@ class API(base.Base):
 
         return objects.InstanceGroup.get_by_uuid(context, group_hint)
 
+    def _safe_destroy_instance_residue(self, instances, instances_to_build):
+        """Delete residue left over from a failed instance build with
+           reckless abandon.
+
+        :param instances: List of Instance objects to destroy
+        :param instances_to_build: List of tuples, output from
+            _provision_instances, which is:
+             request_spec, build_request, instance_mapping
+        """
+        for instance in instances:
+            try:
+                instance.destroy()
+            except Exception as e:
+                LOG.debug('Failed to destroy instance residue: %s', e,
+                          instance=instance)
+        for to_destroy in instances_to_build:
+            for thing in to_destroy:
+                try:
+                    thing.destroy()
+                except Exception as e:
+                    LOG.debug(
+                        'Failed to destroy %s during residue cleanup: %s',
+                        thing, e)
+
     def _create_instance(self, context, instance_type,
                image_href, kernel_id, ramdisk_id,
                min_count, max_count,
@@ -1180,12 +1192,37 @@ class API(base.Base):
         instance_group = self._get_requested_instance_group(context,
                                    filter_properties)
 
-        instances = self._provision_instances(context, instance_type,
+        instances_to_build = self._provision_instances(context, instance_type,
                 min_count, max_count, base_options, boot_meta, security_groups,
                 block_device_mapping, shutdown_terminate,
                 instance_group, check_server_group_quota, filter_properties,
                 key_pair)
 
+        instances = []
+        build_requests = []
+        # TODO(alaski): Cast to conductor here which will call the
+        # scheduler and defer instance creation until the scheduler
+        # has picked a cell/host. Set the instance_mapping to the cell
+        # that the instance is scheduled to.
+        # NOTE(alaski): Instance and block device creation are going
+        # to move to the conductor.
+        try:
+            for rs, build_request, im in instances_to_build:
+                build_requests.append(build_request)
+                instance = build_request.get_new_instance(context)
+                instance.create()
+                instances.append(instance)
+                self._create_block_device_mapping(
+                    build_request.block_device_mappings)
+                # send a state update notification for the initial create to
+                # show it going from non-existent to BUILDING
+                notifications.send_update_with_states(context, instance, None,
+                        vm_states.BUILDING, None, None, service="api")
+        except Exception:
+            with excutils.save_and_reraise_exception():
+                self._safe_destroy_instance_residue(instances,
+                                                    instances_to_build)
+
         for instance in instances:
             self._record_action_start(context, instance,
                                       instance_actions.CREATE)

nova/objects/build_request.py

@@ -211,6 +211,21 @@ class BuildRequest(base.NovaObject):
         db_req = self._save_in_db(self._context, self.id, updates)
         self._from_db_object(self._context, self, db_req)
 
+    def get_new_instance(self, context):
+        # NOTE(danms): This is a hack to make sure that the returned
+        # instance has all dirty fields. There are probably better
+        # ways to do this, but they kinda involve o.vo internals
+        # so this is okay for the moment.
+        instance = objects.Instance(context)
+        for field in self.instance.obj_fields:
+            # NOTE(danms): Don't copy the defaulted tags field
+            # as instance.create() won't handle it properly.
+            if field == 'tags':
+                continue
+            if self.instance.obj_attr_is_set(field):
+                setattr(instance, field, getattr(self.instance, field))
+        return instance
+
 
 @base.NovaObjectRegistry.register
 class BuildRequestList(base.ObjectListBase, base.NovaObject):

nova/tests/unit/compute/test_compute.py

@@ -7748,6 +7748,7 @@ class ComputeAPITestCase(BaseTestCase):
                 id=0, cpuset=set([1, 2]), memory=512),
                    objects.InstanceNUMACell(
                 id=1, cpuset=set([3, 4]), memory=512)])
+        numa_topology.obj_reset_changes()
         numa_constraints_mock.return_value = numa_topology
 
         instances, resv_id = self.compute_api.create(self.context, inst_type,

nova/tests/unit/compute/test_compute_api.py

@@ -180,6 +180,7 @@ class _ComputeAPIUnitTestMixIn(object):
         # Make sure max_count is checked for None, as Python3 doesn't allow
         # comparison between NoneType and Integer, something that's allowed in
         # Python 2.
+        provision_instances.return_value = []
         get_image.return_value = (None, {})
         check_requested_networks.return_value = 1
 
@@ -3488,35 +3489,21 @@ class _ComputeAPIUnitTestMixIn(object):
         do_test()
 
     def test_provision_instances_creates_build_request(self):
+        @mock.patch.object(self.compute_api, 'volume_api')
         @mock.patch.object(self.compute_api, '_check_num_instances_quota')
-        @mock.patch.object(objects, 'Instance')
         @mock.patch.object(self.compute_api.security_group_api,
                 'ensure_default')
-        @mock.patch.object(self.compute_api,
-                           '_bdm_validate_set_size_and_instance')
-        @mock.patch.object(self.compute_api, '_create_block_device_mapping')
         @mock.patch.object(objects.RequestSpec, 'from_components')
-        @mock.patch.object(objects, 'BuildRequest')
+        @mock.patch.object(objects.BuildRequest, 'create')
         @mock.patch.object(objects.InstanceMapping, 'create')
         def do_test(_mock_inst_mapping_create, mock_build_req,
-                mock_req_spec_from_components, _mock_create_bdm,
+                mock_req_spec_from_components, _mock_ensure_default,
-                mock_bdm_validate, _mock_ensure_default, mock_inst,
+                mock_check_num_inst_quota, mock_volume):
-                mock_check_num_inst_quota):
-            quota_mock = mock.MagicMock()
 
             min_count = 1
             max_count = 2
+            quota_mock = mock.MagicMock()
             mock_check_num_inst_quota.return_value = (2, quota_mock)
-            req_spec_mock = mock.MagicMock()
-            mock_req_spec_from_components.return_value = req_spec_mock
-            inst_mocks = [mock.MagicMock() for i in range(max_count)]
-            for inst_mock in inst_mocks:
-                inst_mock.project_id = 'fake-project'
-            mock_inst.side_effect = inst_mocks
-            bdm_mocks = [mock.MagicMock() for i in range(max_count)]
-            mock_bdm_validate.side_effect = bdm_mocks
-            build_req_mocks = [mock.MagicMock() for i in range(max_count)]
-            mock_build_req.side_effect = build_req_mocks
 
             ctxt = context.RequestContext('fake-user', 'fake-project')
             flavor = self._create_flavor()
@@ -3542,7 +3529,7 @@ class _ComputeAPIUnitTestMixIn(object):
                             'numa_topology': None,
                             'pci_requests': None}
             security_groups = {}
-            block_device_mapping = objects.BlockDeviceMappingList(
+            block_device_mappings = objects.BlockDeviceMappingList(
                 objects=[objects.BlockDeviceMapping(
                     **fake_block_device.FakeDbBlockDeviceDict(
                     {
@@ -3559,32 +3546,20 @@ class _ComputeAPIUnitTestMixIn(object):
             filter_properties = {'scheduler_hints': None,
                     'instance_type': flavor}
 
-            instances = self.compute_api._provision_instances(ctxt, flavor,
+            instances_to_build = self.compute_api._provision_instances(
+                    ctxt, flavor,
                     min_count, max_count, base_options, boot_meta,
-                    security_groups, block_device_mapping, shutdown_terminate,
+                    security_groups, block_device_mappings, shutdown_terminate,
                     instance_group, check_server_group_quota,
                     filter_properties, None)
-            for instance in instances:
-                self.assertTrue(uuidutils.is_uuid_like(instance.uuid))
 
-            for inst_mock in inst_mocks:
+            for rs, br, im in instances_to_build:
-                inst_mock.create.assert_called_once_with()
+                self.assertIsInstance(br.instance, objects.Instance)
-
+                self.assertTrue(uuidutils.is_uuid_like(br.instance.uuid))
-            build_req_calls = [
+                self.assertEqual(base_options['project_id'],
-                    mock.call(ctxt,
+                                 br.instance.project_id)
-                              instance=instances[0],
+                self.assertEqual(1, br.block_device_mappings[0].id)
-                              instance_uuid=instances[0].uuid,
+                br.create.assert_called_with()
-                              project_id=instances[0].project_id,
-                              block_device_mappings=bdm_mocks[0]),
-                    mock.call(ctxt,
-                              instance=instances[1],
-                              instance_uuid=instances[1].uuid,
-                              project_id=instances[1].project_id,
-                              block_device_mappings=bdm_mocks[1]),
-                    ]
-            mock_build_req.assert_has_calls(build_req_calls)
-            for build_req_mock in build_req_mocks:
-                build_req_mock.create.assert_called_once_with()
 
         do_test()
 
@@ -3599,7 +3574,8 @@ class _ComputeAPIUnitTestMixIn(object):
                 new=mock.MagicMock())
         @mock.patch.object(objects.RequestSpec, 'from_components',
                 mock.MagicMock())
-        @mock.patch.object(objects, 'BuildRequest', new=mock.MagicMock())
+        @mock.patch.object(objects.BuildRequest, 'create',
+                new=mock.MagicMock())
         @mock.patch('nova.objects.InstanceMapping')
         def do_test(mock_inst_mapping, mock_check_num_inst_quota):
             quota_mock = mock.MagicMock()
@@ -3650,15 +3626,18 @@ class _ComputeAPIUnitTestMixIn(object):
             filter_properties = {'scheduler_hints': None,
                     'instance_type': flavor}
 
-            instances = self.compute_api._provision_instances(ctxt, flavor,
+            instances_to_build = (
+                self.compute_api._provision_instances(ctxt, flavor,
                     min_count, max_count, base_options, boot_meta,
                     security_groups, block_device_mapping, shutdown_terminate,
                     instance_group, check_server_group_quota,
-                    filter_properties, None)
+                    filter_properties, None))
-            self.assertTrue(uuidutils.is_uuid_like(instances[0].uuid))
+            rs, br, im = instances_to_build[0]
+            self.assertTrue(uuidutils.is_uuid_like(br.instance.uuid))
+            self.assertEqual(br.instance_uuid, im.instance_uuid)
 
-            self.assertEqual(instances[0].uuid,
+            self.assertEqual(br.instance.uuid,
-                    inst_mapping_mock.instance_uuid)
+                             inst_mapping_mock.instance_uuid)
             self.assertIsNone(inst_mapping_mock.cell_mapping)
             self.assertEqual(ctxt.project_id, inst_mapping_mock.project_id)
         do_test()
@@ -3744,8 +3723,6 @@ class _ComputeAPIUnitTestMixIn(object):
                               check_server_group_quota, filter_properties,
                               None)
             # First instance, build_req, mapping is created and destroyed
-            self.assertTrue(inst_mocks[0].create.called)
-            self.assertTrue(inst_mocks[0].destroy.called)
             self.assertTrue(build_req_mocks[0].create.called)
             self.assertTrue(build_req_mocks[0].destroy.called)
             self.assertTrue(inst_map_mocks[0].create.called)

nova/tests/unit/compute/test_compute_cells.py

@@ -492,7 +492,7 @@ class CellsConductorAPIRPCRedirect(test.NoDBTestCase):
         _get_image.return_value = (None, 'fake-image')
         _validate.return_value = ({}, 1, None, ['default'])
         _check_bdm.return_value = objects.BlockDeviceMappingList()
-        _provision.return_value = 'instances'
+        _provision.return_value = []
 
         self.compute_api.create(self.context, 'fake-flavor', 'fake-image')
 

nova/tests/unit/objects/test_build_request.py

@@ -133,6 +133,20 @@ class _TestBuildRequestObject(object):
         save_in_db.assert_called_once_with(self.context, req_obj.id,
                                            {'project_id': 'foo'})
 
+    def test_get_new_instance_show_changed_fields(self):
+        # Assert that we create a very dirty object from the cleaned one
+        # on build_request
+        fake_req = fake_build_request.fake_db_req()
+        fields = jsonutils.loads(fake_req['instance'])['nova_object.data']
+        build_request = objects.BuildRequest._from_db_object(
+            self.context, objects.BuildRequest(), fake_req)
+        self.assertEqual(0, len(build_request.instance.obj_what_changed()))
+        instance = build_request.get_new_instance(self.context)
+        for field in fields:
+            self.assertIn(field, instance.obj_what_changed())
+            self.assertEqual(getattr(build_request.instance, field),
+                             getattr(instance, field))
+
 
 class TestBuildRequestObject(test_objects._LocalTest,
                              _TestBuildRequestObject):