From d250aae2b3751ba961433db3a3a28ba4635f370c Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Thu, 18 Aug 2016 13:14:35 -0400 Subject: [PATCH] Clean up instance mappings, build requests on quota failure During the boot process there is a point where failures, most often quota failures, cause the newly created instance to be deleted. In this situation the BuildRequest and InstanceMapping object that were just created should be deleted as well. If they are not then it's possible that an instance list will return the BuildRequest.instance as a regular instance though it should be deleted. Change-Id: Ic2dd3bb7db3ce563a358bed03adaa37ff12c30fd Partially-implements: bp add-buildrequest-ob --- nova/compute/api.py | 14 ++++++++++++++ nova/tests/unit/compute/test_compute_api.py | 16 ++++++++++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index d6b699ff18..aa44bff5d9 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -915,6 +915,8 @@ class API(base.Base): self.security_group_api.ensure_default(context) LOG.debug("Going to run %s instances...", num_instances) instances = [] + instance_mappings = [] + build_requests = [] try: for i in range(num_instances): # Create a uuid for the instance so we can store the @@ -948,6 +950,7 @@ class API(base.Base): project_id=instance.project_id, block_device_mappings=block_device_mapping) build_request.create() + build_requests.append(build_request) # 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. @@ -959,6 +962,7 @@ 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 @@ -1003,6 +1007,16 @@ class API(base.Base): instance.destroy() except exception.ObjectActionError: pass + for instance_mapping in instance_mappings: + try: + instance_mapping.destroy() + except exception.InstanceMappingNotFound: + pass + for build_request in build_requests: + try: + build_request.destroy() + except exception.BuildRequestNotFound: + pass finally: quotas.rollback() diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index faaaff3824..f5b4a6c373 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3550,8 +3550,8 @@ class _ComputeAPIUnitTestMixIn(object): @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.InstanceMapping, 'create') - def do_test(_mock_inst_mapping_create, mock_build_req, + @mock.patch.object(objects, 'InstanceMapping') + def do_test(mock_inst_mapping, mock_build_req, mock_req_spec_from_components, _mock_create_bdm, _mock_ensure_default, mock_inst, mock_check_num_inst_quota): quota_mock = mock.MagicMock() @@ -3567,6 +3567,8 @@ class _ComputeAPIUnitTestMixIn(object): mock_inst.side_effect = inst_mocks build_req_mocks = [mock.MagicMock() for i in range(max_count)] mock_build_req.side_effect = build_req_mocks + inst_map_mocks = [mock.MagicMock() for i in range(max_count)] + mock_inst_mapping.side_effect = inst_map_mocks ctxt = context.RequestContext('fake-user', 'fake-project') flavor = self._create_flavor() @@ -3616,12 +3618,18 @@ class _ComputeAPIUnitTestMixIn(object): shutdown_terminate, instance_group, check_server_group_quota, filter_properties, None) - # First instance is created and destroyed + # First instance, build_req, mapping is created and destroyed self.assertTrue(inst_mocks[0].create.called) self.assertTrue(inst_mocks[0].destroy.called) - # Second instance is not created nor destroyed + self.assertTrue(build_req_mocks[0].create.called) + self.assertTrue(build_req_mocks[0].destroy.called) + self.assertTrue(inst_map_mocks[0].create.called) + self.assertTrue(inst_map_mocks[0].destroy.called) + # Second instance, build_req, mapping is not created nor destroyed self.assertFalse(inst_mocks[1].create.called) self.assertFalse(inst_mocks[1].destroy.called) + self.assertFalse(build_req_mocks[1].destroy.called) + self.assertFalse(inst_map_mocks[1].destroy.called) do_test()