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 <melwittt@gmail.com> Closes-Bug: #1784093 Change-Id: If897a0d721180152ebdceb7a0c23e8f283ce6d10 (cherry picked from commit85f8d033d2
) (cherry picked from commit870e5bcfb6
) (cherry picked from commitdc0ded897b
)
This commit is contained in:
parent
32c10fadb9
commit
22cd12dbb7
|
@ -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))
|
||||
|
|
|
@ -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)
|
|
@ -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')
|
||||
|
|
Loading…
Reference in New Issue