From dc0ded897b5e5df89166c318f8bed3dbb6553fce 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 Conflicts: nova/tests/unit/compute/test_compute_api.py NOTE(melwitt): The conflict is because change Iaffbb019fef7779e7fa44306aacca954512b6970 is not in Rocky. The difference in the test is because change I7f5f08691ca3f73073c66c29dddb996fb2c2b266 is not in Rocky. Change-Id: If897a0d721180152ebdceb7a0c23e8f283ce6d10 (cherry picked from commit 85f8d033d27b31a6398529e0a25da74eae523b08) (cherry picked from commit 870e5bcfb6a5243a785b66353dda299286274e59) --- nova/compute/api.py | 26 ++++++++- nova/tests/functional/db/test_compute_api.py | 58 ++++++++++++++++++++ nova/tests/unit/compute/test_compute_api.py | 38 +++++++++---- 3 files changed, 109 insertions(+), 13 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 22dddaa38053..d0e105244c80 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 @@ -858,6 +859,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, @@ -897,7 +912,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. @@ -930,7 +944,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 @@ -942,7 +955,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 21ba2ccb8f1d..486b0c477c46 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -4599,6 +4599,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, @@ -4632,6 +4635,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') @@ -4647,7 +4652,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 @@ -4711,11 +4717,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, @@ -4726,8 +4735,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() @@ -4807,6 +4814,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, @@ -4817,7 +4826,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 @@ -4883,9 +4893,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags, trusted_certs) # 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) @@ -4910,6 +4921,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, @@ -4920,7 +4933,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 @@ -4986,9 +5000,10 @@ class _ComputeAPIUnitTestMixIn(object): check_server_group_quota, filter_properties, None, tags, trusted_certs) # 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) @@ -4999,6 +5014,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')