diff --git a/nova/compute/api.py b/nova/compute/api.py index 43055f567538..bc3cc6df95a5 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -57,6 +57,7 @@ from nova.consoleauth import rpcapi as consoleauth_rpcapi from nova import context as nova_context from nova import crypto from nova.db import base +from nova.db.sqlalchemy import api as db_api from nova import exception from nova import exception_wrapper from nova import hooks @@ -886,6 +887,20 @@ class API(base.Base): # by the network quotas return base_options, max_network_count, key_pair, security_groups + @staticmethod + @db_api.api_context_manager.writer + def _create_reqspec_buildreq_instmapping(context, rs, br, im): + """Create the request spec, build request, and instance mapping in a + single database transaction. + + The RequestContext must be passed in to this method so that the + database transaction context manager decorator will nest properly and + include each create() into the same transaction context. + """ + rs.create() + br.create() + im.create() + def _provision_instances(self, context, instance_type, min_count, max_count, base_options, boot_meta, security_groups, block_device_mapping, shutdown_terminate, @@ -915,7 +930,6 @@ class API(base.Base): # spec as this is how the conductor knows how many were in this # batch. req_spec.num_instances = num_instances - req_spec.create() # Create an instance object, but do not store in db yet. instance = objects.Instance(context=context) @@ -939,7 +953,6 @@ class API(base.Base): project_id=instance.project_id, block_device_mappings=block_device_mapping, tags=instance_tags) - build_request.create() # Create an instance_mapping. The null cell_mapping indicates # that the instance doesn't yet exist in a cell, and lookups @@ -951,7 +964,14 @@ class API(base.Base): inst_mapping.instance_uuid = instance_uuid inst_mapping.project_id = context.project_id inst_mapping.cell_mapping = None - inst_mapping.create() + + # Create the request spec, build request, and instance mapping + # records in a single transaction so that if a DBError is + # raised from any of them, all INSERTs will be rolled back and + # no orphaned records will be left behind. + self._create_reqspec_buildreq_instmapping(context, req_spec, + build_request, + inst_mapping) instances_to_build.append( (req_spec, build_request, inst_mapping)) diff --git a/nova/tests/functional/db/test_compute_api.py b/nova/tests/functional/db/test_compute_api.py new file mode 100644 index 000000000000..5c5264e81621 --- /dev/null +++ b/nova/tests/functional/db/test_compute_api.py @@ -0,0 +1,58 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from nova.compute import api as compute_api +from nova import context as nova_context +from nova import exception +from nova import objects +from nova import test +from nova.tests import fixtures as nova_fixtures +import nova.tests.uuidsentinel as uuids + + +class ComputeAPITestCase(test.NoDBTestCase): + USES_DB_SELF = True + + def setUp(self): + super(ComputeAPITestCase, self).setUp() + self.useFixture(nova_fixtures.Database(database='api')) + + @mock.patch('nova.objects.instance_mapping.InstanceMapping.create') + def test_reqspec_buildreq_instmapping_single_transaction(self, + mock_create): + # Simulate a DBError during an INSERT by raising an exception from the + # InstanceMapping.create method. + mock_create.side_effect = test.TestingException('oops') + + ctxt = nova_context.RequestContext('fake-user', 'fake-project') + rs = objects.RequestSpec(context=ctxt, instance_uuid=uuids.inst) + # project_id and instance cannot be None + br = objects.BuildRequest(context=ctxt, instance_uuid=uuids.inst, + project_id=ctxt.project_id, + instance=objects.Instance()) + im = objects.InstanceMapping(context=ctxt, instance_uuid=uuids.inst) + + self.assertRaises( + test.TestingException, + compute_api.API._create_reqspec_buildreq_instmapping, ctxt, rs, br, + im) + + # Since the instance mapping failed to INSERT, we should not have + # written a request spec record or a build request record. + self.assertRaises( + exception.RequestSpecNotFound, + objects.RequestSpec.get_by_instance_uuid, ctxt, uuids.inst) + self.assertRaises( + exception.BuildRequestNotFound, + objects.BuildRequest.get_by_instance_uuid, ctxt, uuids.inst) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index bf1c02c6526e..67eaed3f6a3e 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4414,6 +4414,9 @@ class _ComputeAPIUnitTestMixIn(object): mock_br, mock_rs): fake_keypair = objects.KeyPair(name='test') + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping', + new=mock.MagicMock()) @mock.patch('nova.compute.utils.check_num_instances_quota') @mock.patch.object(self.compute_api, 'security_group_api') @mock.patch.object(self.compute_api, @@ -4446,6 +4449,8 @@ class _ComputeAPIUnitTestMixIn(object): do_test() def test_provision_instances_creates_build_request(self): + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping') @mock.patch.object(objects.Instance, 'create') @mock.patch.object(self.compute_api, 'volume_api') @mock.patch('nova.compute.utils.check_num_instances_quota') @@ -4461,7 +4466,8 @@ class _ComputeAPIUnitTestMixIn(object): def do_test(mock_get_min_ver, mock_get_min_ver_cells, _mock_inst_mapping_create, mock_build_req, mock_req_spec_from_components, _mock_ensure_default, - mock_check_num_inst_quota, mock_volume, mock_inst_create): + mock_check_num_inst_quota, mock_volume, mock_inst_create, + mock_create_rs_br_im): min_count = 1 max_count = 2 @@ -4524,11 +4530,14 @@ class _ComputeAPIUnitTestMixIn(object): br.instance.project_id) self.assertEqual(1, br.block_device_mappings[0].id) self.assertEqual(br.instance.uuid, br.tags[0].resource_id) - br.create.assert_called_with() + mock_create_rs_br_im.assert_any_call(ctxt, rs, br, im) do_test() def test_provision_instances_creates_instance_mapping(self): + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping', + new=mock.MagicMock()) @mock.patch('nova.compute.utils.check_num_instances_quota') @mock.patch.object(objects.Instance, 'create', new=mock.MagicMock()) @mock.patch.object(self.compute_api.security_group_api, @@ -4539,8 +4548,6 @@ class _ComputeAPIUnitTestMixIn(object): new=mock.MagicMock()) @mock.patch.object(objects.RequestSpec, 'from_components', 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): inst_mapping_mock = mock.MagicMock() @@ -4619,6 +4626,8 @@ class _ComputeAPIUnitTestMixIn(object): _mock_cinder_reserve_volume, _mock_cinder_check_availability_zone, _mock_cinder_get, _mock_get_min_ver_cells, _mock_get_min_ver): + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping') @mock.patch('nova.compute.utils.check_num_instances_quota') @mock.patch.object(objects, 'Instance') @mock.patch.object(self.compute_api.security_group_api, @@ -4629,7 +4638,8 @@ class _ComputeAPIUnitTestMixIn(object): @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): + _mock_ensure_default, mock_inst, mock_check_num_inst_quota, + mock_create_rs_br_im): min_count = 1 max_count = 2 @@ -4694,9 +4704,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags) # First instance, build_req, mapping is created and destroyed - self.assertTrue(build_req_mocks[0].create.called) + mock_create_rs_br_im.assert_called_once_with(ctxt, req_spec_mock, + build_req_mocks[0], + inst_map_mocks[0]) 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) @@ -4721,6 +4732,8 @@ class _ComputeAPIUnitTestMixIn(object): _mock_bdm, _mock_cinder_attach_create, _mock_cinder_check_availability_zone, _mock_cinder_get, _mock_get_min_ver_cells, _mock_get_min_ver): + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping') @mock.patch('nova.compute.utils.check_num_instances_quota') @mock.patch.object(objects, 'Instance') @mock.patch.object(self.compute_api.security_group_api, @@ -4731,7 +4744,8 @@ class _ComputeAPIUnitTestMixIn(object): @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): + _mock_ensure_default, mock_inst, mock_check_num_inst_quota, + mock_create_rs_br_im): min_count = 1 max_count = 2 @@ -4796,9 +4810,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags) # First instance, build_req, mapping is created and destroyed - self.assertTrue(build_req_mocks[0].create.called) + mock_create_rs_br_im.assert_called_once_with(ctxt, req_spec_mock, + build_req_mocks[0], + inst_map_mocks[0]) 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) @@ -4809,6 +4824,9 @@ class _ComputeAPIUnitTestMixIn(object): do_test() def test_provision_instances_creates_reqspec_with_secgroups(self): + @mock.patch.object(self.compute_api, + '_create_reqspec_buildreq_instmapping', + new=mock.MagicMock()) @mock.patch('nova.compute.utils.check_num_instances_quota') @mock.patch.object(self.compute_api, 'security_group_api') @mock.patch.object(compute_api, 'objects')