From 870e5bcfb6a5243a785b66353dda299286274e59 Mon Sep 17 00:00:00 2001 From: Mohammed Naser Date: Fri, 27 Jul 2018 21:09:22 -0400 Subject: [PATCH] Create request spec, build request and mappings in one transaction The transaction context is currently not shared when creating the RequestSpec, BuildRequest and InstanceMapping. Because of this, it is possible that the database ends in an inconsistent state due to the fact that one of these was created and the system crashed afterwards. This patch adds a function which handles the creation of all those resources in a single transaction. Co-Authored-By: melanie witt Closes-Bug: #1784093 Change-Id: If897a0d721180152ebdceb7a0c23e8f283ce6d10 (cherry picked from commit 85f8d033d27b31a6398529e0a25da74eae523b08) --- nova/compute/api.py | 26 ++++++++- nova/tests/functional/db/test_compute_api.py | 58 ++++++++++++++++++++ nova/tests/unit/compute/test_compute_api.py | 43 ++++++++++----- 3 files changed, 110 insertions(+), 17 deletions(-) create mode 100644 nova/tests/functional/db/test_compute_api.py diff --git a/nova/compute/api.py b/nova/compute/api.py index 2d9ca4a1aaea..6e58b8826dc8 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -56,6 +56,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 @@ -927,6 +928,20 @@ class API(base.Base): return (base_options, max_network_count, key_pair, security_groups, network_metadata) + @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, @@ -968,7 +983,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() # NOTE(stephenfin): The network_metadata field is not persisted # and is therefore set after 'create' is called. @@ -1001,7 +1015,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 @@ -1014,7 +1027,14 @@ class API(base.Base): inst_mapping.project_id = context.project_id inst_mapping.user_id = context.user_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..49fa10281a23 --- /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 oslo_utils.fixture import uuidsentinel as uuids + +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 + + +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 4ba51e1b6d53..714d1dd1cc59 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4684,6 +4684,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, @@ -4718,16 +4721,16 @@ 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('nova.compute.utils.check_num_instances_quota') @mock.patch.object(self.compute_api.security_group_api, 'ensure_default') @mock.patch.object(objects.RequestSpec, 'from_components') - @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_ensure_default, - mock_check_num_inst_quota, mock_inst_create): + def do_test(mock_req_spec_from_components, _mock_ensure_default, + mock_check_num_inst_quota, mock_inst_create, + mock_create_rs_br_im): min_count = 1 max_count = 2 @@ -4799,11 +4802,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, @@ -4814,8 +4820,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() @@ -4898,6 +4902,8 @@ class _ComputeAPIUnitTestMixIn(object): _mock_cinder_reserve_volume, _mock_cinder_check_availability_zone, _mock_cinder_get, _mock_get_min_ver_cells): + @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, @@ -4908,7 +4914,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 @@ -4975,9 +4982,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags, trusted_certs, False) # 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) @@ -5002,6 +5010,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, @@ -5012,7 +5022,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 @@ -5079,9 +5090,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags, trusted_certs, False) # 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) @@ -5092,6 +5104,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')