Merge "Create request spec, build request and mappings in one transaction"

Author: Zuul

nova/compute/api.py

@@ -56,6 +56,7 @@
 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 @@ def _validate_and_build_base_options(self, context, instance_type,
         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 @@ def _provision_instances(self, context, instance_type, min_count,
                 # 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 @@ def _provision_instances(self, context, instance_type, min_count,
                         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 @@ def _provision_instances(self, context, instance_type, min_count,
                 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))

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)

nova/tests/unit/compute/test_compute_api.py

@@ -4714,6 +4714,9 @@ def test_provision_instances_with_keypair(self, mock_im, mock_instance,
                                               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,
@@ -4748,16 +4751,16 @@ def do_test(mock_cbdm, mock_bdm_v, mock_cdb, mock_sg, mock_cniq):
         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
@@ -4829,11 +4832,14 @@ def do_test(_mock_inst_mapping_create, mock_build_req,
                                  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,
@@ -4844,8 +4850,6 @@ def test_provision_instances_creates_instance_mapping(self):
                 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()
@@ -4928,6 +4932,8 @@ def test_provision_instances_cleans_up_when_volume_invalid(self,
             _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,
@@ -4938,7 +4944,8 @@ def test_provision_instances_cleans_up_when_volume_invalid(self,
         @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
@@ -5005,9 +5012,10 @@ def do_test(mock_inst_mapping, mock_build_req,
                               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)
@@ -5032,6 +5040,8 @@ def test_provision_instances_cleans_up_when_volume_invalid_new_flow(self,
             _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,
@@ -5042,7 +5052,8 @@ def test_provision_instances_cleans_up_when_volume_invalid_new_flow(self,
         @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
@@ -5109,9 +5120,10 @@ def do_test(mock_inst_mapping, mock_build_req,
                               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)
@@ -5122,6 +5134,9 @@ def do_test(mock_inst_mapping, mock_build_req,
         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')