diff --git a/nova/compute/api.py b/nova/compute/api.py index 52d3d965743b..65b2b55687a4 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -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. diff --git a/nova/tests/functional/regressions/test_bug_1404867.py b/nova/tests/functional/regressions/test_bug_1404867.py index 5fc6466e0c17..49d85db52550 100644 --- a/nova/tests/functional/regressions/test_bug_1404867.py +++ b/nova/tests/functional/regressions/test_bug_1404867.py @@ -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)) diff --git a/nova/tests/functional/wsgi/test_servers.py b/nova/tests/functional/wsgi/test_servers.py index 8d406432198b..042c41c94e25 100644 --- a/nova/tests/functional/wsgi/test_servers.py +++ b/nova/tests/functional/wsgi/test_servers.py @@ -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 diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index f4d5c9b7ffa1..5fec6d7112e0 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -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({ diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index eeb448815c92..694d091cc916 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -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, diff --git a/nova/utils.py b/nova/utils.py index eac44964e10e..c683bcf25537 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -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