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
This commit is contained in:
Mohammed Naser
melanie witt
parent
b7a018f126
commit
85f8d033d2
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
@ -4714,6 +4714,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,
|
||||
|
|
@ -4748,16 +4751,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
|
||||
|
|
@ -4829,11 +4832,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,
|
||||
|
|
@ -4844,8 +4850,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()
|
||||
|
|
@ -4928,6 +4932,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,
|
||||
|
|
@ -4938,7 +4944,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
|
||||
|
|
@ -5005,9 +5012,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)
|
||||
|
|
@ -5032,6 +5040,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,
|
||||
|
|
@ -5042,7 +5052,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
|
||||
|
|
@ -5109,9 +5120,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)
|
||||
|
|
@ -5122,6 +5134,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