From df488c863e1f9a6656aac1cd9cb9b54c6160ad01 Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Fri, 1 Apr 2016 11:29:13 -0400 Subject: [PATCH] Put more into compute.api._populate_instance_for_create In order to start putting a serialized Instance into BuildRequest during the boot process there needs to be an easy way to get a populated Instance object. Much of the creation was happening in _populate_instance_for_create but some parts happened outside of that in create_db_entry_for_new_instance. Those parts have been moved into _populate_instance_for_create. create_db_entry_for_new_instance will be left around and used because the cellsv1 scheduler uses it but that method will not be used for cellsv2. Eventually it should be removed and the logic within moved out. Change-Id: I807c1518411bf535af12d3990c488cc0e5305b51 --- nova/compute/api.py | 73 +++++++++++-------- .../compute/test_block_device_mapping.py | 2 +- nova/tests/unit/compute/test_compute.py | 5 +- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 136d61004984..18d710f3e23e 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -571,7 +571,6 @@ class API(base.Base): instance.hostname = self._default_host_name(instance.uuid) else: instance.hostname = utils.sanitize_hostname(new_name) - instance.save() return instance def _check_config_drive(self, config_drive): @@ -960,6 +959,7 @@ class API(base.Base): context, instance_type, min_count, max_count) security_groups = self.security_group_api.populate_security_groups( security_groups) + self.security_group_api.ensure_default(context) LOG.debug("Going to run %s instances...", num_instances) instances = [] try: @@ -977,6 +977,16 @@ class API(base.Base): build_request = self._create_build_request(context, instance_uuid, base_options, req_spec, security_groups, num_instances, i) + + # Create an instance object, but do not store in db yet. + instance = objects.Instance(context=context) + instance.uuid = instance_uuid + instance.update(base_options) + instance = self.create_db_entry_for_new_instance(context, + instance_type, boot_meta, instance, security_groups, + block_device_mapping, num_instances, i, + shutdown_terminate, create_instance=False) + # 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. @@ -992,14 +1002,13 @@ class API(base.Base): # 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. - instance = objects.Instance(context=context) - instance.uuid = instance_uuid - instance.update(base_options) - instance = self.create_db_entry_for_new_instance( - context, instance_type, boot_meta, instance, - security_groups, block_device_mapping, - num_instances, i, shutdown_terminate) + # NOTE(alaski): Instance and block device creation are going + # to move to the conductor. + instance.create() instances.append(instance) + + self._create_block_device_mapping( + instance_type, instance.uuid, block_device_mapping) # The BuildRequest needs to be stored until the instance is in # an instance table. At that point it will never be used again # and should be deleted. As instance creation is pushed back, @@ -1396,7 +1405,8 @@ class API(base.Base): return "Server-%s" % instance_uuid def _populate_instance_for_create(self, context, instance, image, - index, security_groups, instance_type): + index, security_groups, instance_type, + num_instances, shutdown_terminate): """Build the beginning of a new instance.""" instance.launch_index = index @@ -1433,36 +1443,37 @@ class API(base.Base): instance.security_groups = security_groups + self._populate_instance_names(instance, num_instances) + instance.shutdown_terminate = shutdown_terminate + if num_instances > 1: + instance = self._apply_instance_name_template(context, instance, + index) + return instance - # NOTE(bcwaldon): No policy check since this is only used by scheduler and - # the compute api. That should probably be cleaned up, though. + # This method remains because cellsv1 uses it in the scheduler def create_db_entry_for_new_instance(self, context, instance_type, image, instance, security_group, block_device_mapping, num_instances, - index, shutdown_terminate=False): + index, shutdown_terminate=False, create_instance=True): """Create an entry in the DB for this new instance, including any related table updates (such as security group, etc). This is called by the scheduler after a location for the instance has been determined. + + :param create_instance: Determines if the instance is created here or + just populated for later creation. This is done so that this code + can be shared with cellsv1 which needs the instance creation to + happen here. It should be removed and this method cleaned up when + cellsv1 is a distant memory. """ self._populate_instance_for_create(context, instance, image, index, - security_group, instance_type) + security_group, instance_type, + num_instances, shutdown_terminate) - self._populate_instance_names(instance, num_instances) - - instance.shutdown_terminate = shutdown_terminate - - self.security_group_api.ensure_default(context) - instance.create() - - if num_instances > 1: - # NOTE(russellb) We wait until this spot to handle - # multi_instance_display_name_template, because we need - # the UUID from the instance. - instance = self._apply_instance_name_template(context, instance, - index) + if create_instance: + instance.create() # NOTE (ndipanov): This can now raise exceptions but the instance # has been created, so delete it and re-raise so @@ -1473,10 +1484,14 @@ class API(base.Base): except (exception.CinderConnectionFailed, exception.InvalidBDM, exception.InvalidVolume): with excutils.save_and_reraise_exception(): - instance.destroy() + if create_instance: + instance.destroy() - self._create_block_device_mapping( - instance_type, instance.uuid, block_device_mapping) + if create_instance: + # Because of the foreign key the bdm can't be created unless the + # instance is. + self._create_block_device_mapping( + instance_type, instance.uuid, block_device_mapping) return instance diff --git a/nova/tests/unit/api/openstack/compute/test_block_device_mapping.py b/nova/tests/unit/api/openstack/compute/test_block_device_mapping.py index ea0c28c21a2e..ae338e96998c 100644 --- a/nova/tests/unit/api/openstack/compute/test_block_device_mapping.py +++ b/nova/tests/unit/api/openstack/compute/test_block_device_mapping.py @@ -329,7 +329,7 @@ class BlockDeviceMappingTestV21(test.TestCase): self.assertRaises(exc.HTTPBadRequest, self._test_create, params) self.assertTrue(self.validation_fail_test_validate_called) - self.assertTrue(self.validation_fail_instance_destroy_called) + self.assertFalse(self.validation_fail_instance_destroy_called) self.validation_fail_test_validate_called = False self.validation_fail_instance_destroy_called = False diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 032fc62153c0..ab7cbe3ac851 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -7733,6 +7733,7 @@ class ComputeAPITestCase(BaseTestCase): def test_populate_instance_for_create(self): base_options = {'image_ref': self.fake_image['id'], 'system_metadata': {'fake': 'value'}, + 'display_name': 'foo', 'uuid': uuids.instance} instance = objects.Instance() instance.update(base_options) @@ -7743,7 +7744,9 @@ class ComputeAPITestCase(BaseTestCase): self.fake_image, 1, security_groups=objects.SecurityGroupList(), - instance_type=inst_type) + instance_type=inst_type, + num_instances=1, + shutdown_terminate=False) self.assertEqual(str(base_options['image_ref']), instance['system_metadata']['image_base_image_ref']) self.assertEqual(vm_states.BUILDING, instance['vm_state'])