Remove old check_attach version check in API

In Pike, I3a3caa4c566ecc132aa2699f8c7e5987bbcc863a added
a version check for computes during boot from volume
to determine if the computes were running Ocata and would
call the "check_attach" routine which was removed in that
same change.

Since the API doesn't support computes that old at this point
we can remove that version compat checking code. Related tests
are updated appropriately including several tests for _validate_bdm
that were incorrectly passing because of the mock on
Service.get_minimum_version when in fact instances going through
_validate_bdm won't have their instance.id attribute set.

Finally, the really old "check_attach" comment in the
get_image_metadata_from_volume() method is updated for context.

Change-Id: Iaffbb019fef7779e7fa44306aacca954512b6970
This commit is contained in:
Matt Riedemann 2018-08-02 13:14:48 -04:00
parent 6522ea3ecf
commit fc5839b959
6 changed files with 48 additions and 138 deletions

View File

@ -102,7 +102,6 @@ AGGREGATE_ACTION_UPDATE = 'Update'
AGGREGATE_ACTION_UPDATE_META = 'UpdateMeta'
AGGREGATE_ACTION_DELETE = 'Delete'
AGGREGATE_ACTION_ADD = 'Add'
BFV_RESERVE_MIN_COMPUTE_VERSION = 17
CINDER_V3_ATTACH_MIN_COMPUTE_VERSION = 24
MIN_COMPUTE_MULTIATTACH = 27
MIN_COMPUTE_TRUSTED_CERTS = 31
@ -1400,28 +1399,10 @@ class API(base.Base):
"destination_type 'volume' need to have a non-zero "
"size specified"))
elif volume_id is not None:
# The instance is being created and we don't know which
# cell it's going to land in, so check all cells.
min_compute_version = \
objects.service.get_minimum_version_all_cells(
context, ['nova-compute'])
try:
# NOTE(ildikov): The boot from volume operation did not
# reserve the volume before Pike and as the older computes
# are running 'check_attach' which will fail if the volume
# is in 'attaching' state; if the compute service version
# is not high enough we will just perform the old check as
# opposed to reserving the volume here.
volume = self.volume_api.get(context, volume_id)
if (min_compute_version >=
BFV_RESERVE_MIN_COMPUTE_VERSION):
self._check_attach_and_reserve_volume(
context, volume, instance, bdm,
supports_multiattach)
else:
# NOTE(ildikov): This call is here only for backward
# compatibility can be removed after Ocata EOL.
self._check_attach(context, volume, instance)
self._check_attach_and_reserve_volume(
context, volume, instance, bdm, supports_multiattach)
bdm.volume_size = volume.get('size')
# NOTE(mnaser): If we end up reserving the volume, it will
@ -1478,20 +1459,6 @@ class API(base.Base):
if num_local > max_local:
raise exception.InvalidBDMLocalsLimit()
def _check_attach(self, context, volume, instance):
# TODO(ildikov): This check_attach code is kept only for backward
# compatibility and should be removed after Ocata EOL.
if volume['status'] != 'available':
msg = _("volume '%(vol)s' status must be 'available'. Currently "
"in '%(status)s'") % {'vol': volume['id'],
'status': volume['status']}
raise exception.InvalidVolume(reason=msg)
if volume['attach_status'] == 'attached':
msg = _("volume %s already attached") % volume['id']
raise exception.InvalidVolume(reason=msg)
self.volume_api.check_availability_zone(context, volume,
instance=instance)
def _populate_instance_names(self, instance, num_instances, index):
"""Populate instance display_name and hostname.

View File

@ -61,7 +61,8 @@ class DeleteWithReservedVolumes(integrated_helpers._IntegratedTestBase,
return self._wait_for_state_change(self.api, server, 'ERROR')
@mock.patch('nova.objects.service.get_minimum_version_all_cells',
return_value=compute_api.BFV_RESERVE_MIN_COMPUTE_VERSION)
return_value=
compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1)
def test_delete_with_reserved_volumes(self, mock_version_get=None):
self.cinder = self.useFixture(nova_fixtures.CinderFixture(self))

View File

@ -248,8 +248,9 @@ class ServersPreSchedulingTestCase(test.TestCase,
self.assertEqual(0, len(list_resp))
@mock.patch('nova.objects.service.get_minimum_version_all_cells',
return_value=compute_api.BFV_RESERVE_MIN_COMPUTE_VERSION)
def test_bfv_delete_build_request_pre_scheduling_ocata(self, mock_get):
return_value=
compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1)
def test_bfv_delete_build_request_pre_scheduling_old_flow(self, mock_get):
cinder = self.useFixture(nova_fixtures.CinderFixture(self))
volume_id = nova_fixtures.CinderFixture.IMAGE_BACKED_VOL

View File

@ -977,9 +977,7 @@ class ComputeVolumeTestCase(BaseTestCase):
for expected, got in zip(expected_result, preped_bdm):
self.assertThat(expected, matchers.IsSubDictOf(got))
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
def test_validate_bdm(self, mock_get_min_ver):
def test_validate_bdm(self):
def fake_get(self, context, res_id):
return {'id': res_id, 'size': 4, 'multiattach': False}
@ -1205,52 +1203,13 @@ class ComputeVolumeTestCase(BaseTestCase):
self.context, self.instance,
instance_type, all_mappings)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=16)
@mock.patch.object(nova.volume.cinder.API, 'get')
@mock.patch.object(cinder.API, 'check_availability_zone')
def test_validate_bdm_volume_old_compute(self, mock_check_av_zone,
mock_get, mock_get_min_ver):
instance = self._create_fake_instance_obj()
instance_type = {'ephemeral_gb': 2}
volume_id = uuids.volume_id
mappings = [
fake_block_device.FakeDbBlockDeviceDict({
'device_name': '/dev/sda1',
'source_type': 'volume',
'destination_type': 'volume',
'device_type': 'disk',
'volume_id': volume_id,
'guest_format': None,
'boot_index': 0,
}, anon=True)
]
mappings = block_device_obj.block_device_make_list_from_dicts(
self.context, mappings)
volume = {'id': volume_id,
'size': 4,
'status': 'available',
'attach_status': 'detached',
'multiattach': False}
mock_get.return_value = volume
self.compute_api._validate_bdm(self.context, instance,
instance_type, mappings)
self.assertEqual(4, mappings[0].volume_size)
mock_check_av_zone.assert_called_once_with(self.context, volume,
instance=instance)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
@mock.patch.object(cinder.API, 'get')
@mock.patch.object(cinder.API, 'check_availability_zone')
@mock.patch.object(cinder.API, 'reserve_volume',
side_effect=exception.InvalidVolume(reason='error'))
def test_validate_bdm_media_service_invalid_volume(self, mock_reserve_vol,
mock_check_av_zone,
mock_get,
mock_get_min_ver):
mock_get):
volume_id = uuids.volume_id
instance_type = {'swap': 1, 'ephemeral_gb': 1}
bdms = [fake_block_device.FakeDbBlockDeviceDict({
@ -1296,11 +1255,8 @@ class ComputeVolumeTestCase(BaseTestCase):
instance_type, bdms)
mock_get.assert_called_with(self.context, volume_id)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
@mock.patch.object(cinder.API, 'get')
def test_validate_bdm_media_service_volume_not_found(self, mock_get,
mock_get_min_ver):
def test_validate_bdm_media_service_volume_not_found(self, mock_get):
volume_id = uuids.volume_id
instance_type = {'swap': 1, 'ephemeral_gb': 1}
bdms = [fake_block_device.FakeDbBlockDeviceDict({
@ -1322,15 +1278,12 @@ class ComputeVolumeTestCase(BaseTestCase):
self.context, self.instance,
instance_type, bdms)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(cinder.API, 'get')
@mock.patch.object(cinder.API, 'check_availability_zone')
@mock.patch.object(cinder.API, 'reserve_volume')
def test_validate_bdm_media_service_valid(self, mock_reserve_vol,
mock_check_av_zone,
mock_get,
mock_get_min_ver):
mock_get):
volume_id = uuids.volume_id
instance_type = {'swap': 1, 'ephemeral_gb': 1}
bdms = [fake_block_device.FakeDbBlockDeviceDict({

View File

@ -4269,17 +4269,17 @@ class _ComputeAPIUnitTestMixIn(object):
bdms, legacy_bdm=True)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
return_value=
compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1)
@mock.patch.object(cinder.API, 'get')
@mock.patch.object(cinder.API, 'reserve_volume')
def test_validate_bdm_returns_attachment_id(self, mock_reserve_volume,
mock_get, mock_get_min_ver,
mock_get,
mock_get_min_ver_all):
# Tests that bdm validation *always* returns an attachment_id even if
# it's None.
instance = self._create_instance_obj()
del instance.id
instance_type = self._create_flavor()
volume_id = 'e856840e-9f5b-4894-8bde-58c6e29ac1e8'
volume_info = {'status': 'available',
@ -4309,19 +4309,19 @@ class _ComputeAPIUnitTestMixIn(object):
self.context, volume_id)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
return_value=
compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1)
@mock.patch.object(cinder.API, 'get')
@mock.patch.object(cinder.API, 'reserve_volume',
side_effect=exception.InvalidInput(reason='error'))
def test_validate_bdm_with_error_volume(self, mock_reserve_volume,
mock_get, mock_get_min_ver,
mock_get,
mock_get_min_ver_all):
# Tests that an InvalidInput exception raised from
# volume_api.reserve_volume due to the volume status not being
# 'available' results in _validate_bdm re-raising InvalidVolume.
instance = self._create_instance_obj()
del instance.id
instance_type = self._create_flavor()
volume_id = 'e856840e-9f5b-4894-8bde-58c6e29ac1e8'
volume_info = {'status': 'error',
@ -4348,14 +4348,11 @@ class _ComputeAPIUnitTestMixIn(object):
mock_reserve_volume.assert_called_once_with(
self.context, volume_id)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(cinder.API, 'get_snapshot',
side_effect=exception.CinderConnectionFailed(reason='error'))
@mock.patch.object(cinder.API, 'get',
side_effect=exception.CinderConnectionFailed(reason='error'))
def test_validate_bdm_with_cinder_down(self, mock_get, mock_get_snapshot,
mock_get_min_ver):
def test_validate_bdm_with_cinder_down(self, mock_get, mock_get_snapshot):
instance = self._create_instance_obj()
instance_type = self._create_flavor()
bdm = [objects.BlockDeviceMapping(
@ -4389,19 +4386,17 @@ class _ComputeAPIUnitTestMixIn(object):
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW)
@mock.patch.object(cinder.API, 'get')
@mock.patch.object(cinder.API, 'attachment_create',
side_effect=exception.InvalidInput(reason='error'))
def test_validate_bdm_with_error_volume_new_flow(self, mock_attach_create,
mock_get,
mock_get_min_ver,
mock_get_min_ver_all):
# Tests that an InvalidInput exception raised from
# volume_api.attachment_create due to the volume status not being
# 'available' results in _validate_bdm re-raising InvalidVolume.
instance = self._create_instance_obj()
del instance.id
instance_type = self._create_flavor()
volume_id = 'e856840e-9f5b-4894-8bde-58c6e29ac1e8'
volume_info = {'status': 'error',
@ -4508,19 +4503,12 @@ class _ComputeAPIUnitTestMixIn(object):
do_test()
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(cinder.API, 'get',
side_effect=exception.CinderConnectionFailed(reason='error'))
def test_provision_instances_with_cinder_down(self, mock_get,
mock_get_min_ver):
def test_provision_instances_with_cinder_down(self, mock_get):
self._test_provision_instances_with_cinder_error(
expected_exception=exception.CinderConnectionFailed)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(cinder.API, 'get',
return_value={'id': '1', 'multiattach': False})
@mock.patch.object(cinder.API, 'check_availability_zone')
@ -4529,9 +4517,7 @@ class _ComputeAPIUnitTestMixIn(object):
def test_provision_instances_with_error_volume(self,
mock_cinder_check_av_zone,
mock_reserve_volume,
mock_get,
mock_get_min_ver_cells,
mock_get_min_ver):
mock_get):
self._test_provision_instances_with_cinder_error(
expected_exception=exception.InvalidVolume)
@ -4593,21 +4579,15 @@ class _ComputeAPIUnitTestMixIn(object):
def test_provision_instances_creates_build_request(self):
@mock.patch.object(objects.Instance, 'create')
@mock.patch.object(self.compute_api, 'volume_api')
@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')
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
def do_test(mock_get_min_ver, mock_get_min_ver_cells,
_mock_inst_mapping_create, mock_build_req,
def do_test(_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_inst_create):
min_count = 1
max_count = 2
@ -4648,7 +4628,6 @@ class _ComputeAPIUnitTestMixIn(object):
'device_name': 'vda',
'boot_index': 0,
}))])
mock_volume.get.return_value = {'id': '1', 'multiattach': False}
instance_tags = objects.TagList(objects=[objects.Tag(tag='tag')])
shutdown_terminate = True
trusted_certs = None
@ -4657,13 +4636,20 @@ class _ComputeAPIUnitTestMixIn(object):
filter_properties = {'scheduler_hints': None,
'instance_type': flavor}
instances_to_build = self.compute_api._provision_instances(
ctxt, flavor,
min_count, max_count, base_options, boot_meta,
security_groups, block_device_mappings, shutdown_terminate,
instance_group, check_server_group_quota,
filter_properties, None, instance_tags, trusted_certs,
False)
with mock.patch.object(
self.compute_api,
'_bdm_validate_set_size_and_instance',
return_value=block_device_mappings) as validate_bdm:
instances_to_build = self.compute_api._provision_instances(
ctxt, flavor,
min_count, max_count, base_options, boot_meta,
security_groups, block_device_mappings,
shutdown_terminate, instance_group,
check_server_group_quota, filter_properties, None,
instance_tags, trusted_certs, False)
validate_bdm.assert_has_calls([mock.call(
ctxt, test.MatchType(objects.Instance), flavor,
block_device_mappings, False)] * max_count)
for rs, br, im in instances_to_build:
self.assertIsInstance(br.instance, objects.Instance)
@ -4756,10 +4742,9 @@ class _ComputeAPIUnitTestMixIn(object):
self.assertEqual(ctxt.project_id, inst_mapping_mock.project_id)
do_test()
@mock.patch.object(objects.Service, 'get_minimum_version',
return_value=17)
@mock.patch.object(objects.service, 'get_minimum_version_all_cells',
return_value=17)
return_value=
compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1)
@mock.patch.object(cinder.API, 'get',
return_value={'id': '1', 'multiattach': False})
@mock.patch.object(cinder.API, 'check_availability_zone',)
@ -4768,7 +4753,7 @@ class _ComputeAPIUnitTestMixIn(object):
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_get_min_ver):
_mock_get_min_ver_cells):
@mock.patch('nova.compute.utils.check_num_instances_quota')
@mock.patch.object(objects, 'Instance')
@mock.patch.object(self.compute_api.security_group_api,

View File

@ -953,9 +953,12 @@ def get_image_metadata_from_volume(volume):
val = properties.pop(attr, None)
if attr in ('min_ram', 'min_disk'):
image_meta[attr] = int(val or 0)
# NOTE(yjiang5): Always set the image status as 'active'
# and depends on followed volume_api.check_attach() to
# verify it. This hack should be harmless with that check.
# NOTE(mriedem): Set the status to 'active' as a really old hack
# from when this method was in the compute API class and is
# needed for _check_requested_image which makes sure the image
# is 'active'. For volume-backed servers, if the volume is not
# available because the image backing the volume is not active,
# then the compute API trying to reserve the volume should fail.
image_meta['status'] = 'active'
return image_meta