diff --git a/nova/compute/api.py b/nova/compute/api.py index a8def4a6fd79..073c841ec4db 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -102,6 +102,7 @@ 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 # FIXME(danms): Keep a global cache of the cells we find the # first time we look. This needs to be refreshed on a timer or @@ -1331,7 +1332,7 @@ class API(base.Base): if (min_compute_version >= BFV_RESERVE_MIN_COMPUTE_VERSION): volume = self._check_attach_and_reserve_volume( - context, volume_id, instance) + context, volume_id, instance, bdm) else: # NOTE(ildikov): This call is here only for backward # compatibility can be removed after Ocata EOL. @@ -3653,12 +3654,45 @@ class API(base.Base): except exception.VolumeBDMNotFound: pass - def _check_attach_and_reserve_volume(self, context, volume_id, instance): + def _check_attach_and_reserve_volume(self, context, volume_id, instance, + bdm): volume = self.volume_api.get(context, volume_id) self.volume_api.check_availability_zone(context, volume, instance=instance) - self.volume_api.reserve_volume(context, volume_id) - + if 'id' in instance: + # This is a volume attach to an existing instance, so + # we only care about the cell the instance is in. + min_compute_version = objects.Service.get_minimum_version( + context, 'nova-compute') + else: + # 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']) + if min_compute_version >= CINDER_V3_ATTACH_MIN_COMPUTE_VERSION: + # Attempt a new style volume attachment, but fallback to old-style + # in case Cinder API 3.44 isn't available. + try: + attachment_id = self.volume_api.attachment_create( + context, volume_id, instance.uuid)['id'] + bdm.attachment_id = attachment_id + # NOTE(ildikov): In case of boot from volume the BDM at this + # point is not yet created in a cell database, so we can't + # call save(). When attaching a volume to an existing + # instance, the instance is already in a cell and the BDM has + # been created in that same cell so updating here in that case + # is "ok". + if bdm.obj_attr_is_set('id'): + bdm.save() + except exception.CinderAPIVersionNotAvailable: + LOG.debug('The available Cinder microversion is not high ' + 'enough to create new style volume attachment.') + self.volume_api.reserve_volume(context, volume_id) + else: + LOG.debug('The compute service version is not high enough to ' + 'create a new style volume attachment.') + self.volume_api.reserve_volume(context, volume_id) return volume def _attach_volume(self, context, instance, volume_id, device, @@ -3672,7 +3706,8 @@ class API(base.Base): context, instance, device, volume_id, disk_bus=disk_bus, device_type=device_type, tag=tag) try: - self._check_attach_and_reserve_volume(context, volume_id, instance) + self._check_attach_and_reserve_volume(context, volume_id, instance, + volume_bdm) self._record_action_start( context, instance, instance_actions.ATTACH_VOLUME) self.compute_rpcapi.attach_volume(context, instance, volume_bdm) @@ -3695,21 +3730,31 @@ class API(base.Base): instance will be unshelved. """ @wrap_instance_event(prefix='api') - def attach_volume(self, context, v_id, instance, dev): - self.volume_api.attach(context, - v_id, - instance.uuid, - dev) + def attach_volume(self, context, v_id, instance, dev, attachment_id): + if attachment_id: + # Normally we wouldn't complete an attachment without a host + # connector, but we do this to make the volume status change + # to "in-use" to maintain the API semantics with the old flow. + # When unshelving the instance, the compute service will deal + # with this disconnected attachment. + self.volume_api.attachment_complete(context, attachment_id) + else: + self.volume_api.attach(context, + v_id, + instance.uuid, + dev) volume_bdm = self._create_volume_bdm( context, instance, device, volume_id, disk_bus=disk_bus, device_type=device_type, is_local_creation=True) try: - self._check_attach_and_reserve_volume(context, volume_id, instance) + self._check_attach_and_reserve_volume(context, volume_id, instance, + volume_bdm) self._record_action_start( context, instance, instance_actions.ATTACH_VOLUME) - attach_volume(self, context, volume_id, instance, device) + attach_volume(self, context, volume_id, instance, device, + volume_bdm.attachment_id) except Exception: with excutils.save_and_reraise_exception(): volume_bdm.destroy() @@ -3731,6 +3776,28 @@ class API(base.Base): if device and not block_device.match_device(device): raise exception.InvalidDevicePath(path=device) + # Check to see if the computes in this cell can support new-style + # volume attachments. + min_compute_version = objects.Service.get_minimum_version( + context, 'nova-compute') + if min_compute_version >= CINDER_V3_ATTACH_MIN_COMPUTE_VERSION: + try: + # Check to see if Cinder is new enough to create new-style + # attachments. + cinder.is_microversion_supported(context, '3.44') + except exception.CinderAPIVersionNotAvailable: + pass + else: + # Make sure the volume isn't already attached to this instance + # because based on the above checks, we'll use the new style + # attachment flow in _check_attach_and_reserve_volume and + # Cinder will allow multiple attachments between the same + # volume and instance but the old flow API semantics don't + # allow that so we enforce it here. + self._check_volume_already_attached_to_instance(context, + instance, + volume_id) + is_shelved_offloaded = instance.vm_state == vm_states.SHELVED_OFFLOADED if is_shelved_offloaded: if tag: diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 6d76561aaea4..596bc1b1b9cf 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -3024,8 +3024,23 @@ class ComputeManager(manager.Manager): def detach_block_devices(context, bdms): for bdm in bdms: if bdm.is_volume: + # NOTE (ildikov): Having the attachment_id set in the BDM + # means that it's the new Cinder attach/detach flow + # (available from v3.44). In that case we explicitly + # attach and detach the volumes through attachment level + # operations. In this scenario _detach_volume will delete + # the existing attachment which would make the volume + # status change to 'in-use' if we don't pre-create another + # empty attachment before deleting the old one. + attachment_id = None + if bdm.attachment_id: + attachment_id = self.volume_api.attachment_create( + context, bdm['volume_id'], instance.uuid)['id'] self._detach_volume(context, bdm, instance, destroy_bdm=False) + if attachment_id: + bdm.attachment_id = attachment_id + bdm.save() files = self._decode_files(injected_files) @@ -4216,8 +4231,22 @@ class ComputeManager(manager.Manager): for bdm in bdms: if bdm.is_volume: if bdm.attachment_id: + # NOTE(jdg): So here's the thing, the idea behind the new + # attach API's was to have a new code fork/path that we + # followed, we're not going to do that so we have to do + # some extra work in here to make it *behave* just like the + # old code. Cinder doesn't allow disconnect/reconnect (you + # just delete the attachment and get a new one) + # attachments in the new attach code so we have to do + # a delete and create without a connector (reserve), + # in other words, beware + attachment_id = self.volume_api.attachment_create( + context, bdm.volume_id, instance.uuid)['id'] self.volume_api.attachment_delete(context, bdm.attachment_id) + bdm.attachment_id = attachment_id + bdm.save() + else: if connector is None: connector = self.driver.get_volume_connector(instance) @@ -5136,7 +5165,11 @@ class ComputeManager(manager.Manager): {'volume_id': bdm.volume_id, 'mountpoint': bdm['mount_device']}, instance=instance) - self.volume_api.unreserve_volume(context, bdm.volume_id) + if bdm['attachment_id']: + self.volume_api.attachment_delete(context, + bdm['attachment_id']) + else: + self.volume_api.unreserve_volume(context, bdm.volume_id) compute_utils.notify_about_volume_attach_detach( context, instance, self.host, action=fields.NotificationAction.VOLUME_ATTACH, @@ -5309,6 +5342,8 @@ class ComputeManager(manager.Manager): instance=instance) self.driver.swap_volume(old_cinfo, new_cinfo, instance, mountpoint, resize_to) + if new_attachment_id: + self.volume_api.attachment_complete(context, new_attachment_id) LOG.debug("swap_volume: Driver volume swap returned, new " "connection_info is now : %(new_cinfo)s", {'new_cinfo': new_cinfo}) diff --git a/nova/objects/service.py b/nova/objects/service.py index 6f47bb0a52db..a39b340ce5c8 100644 --- a/nova/objects/service.py +++ b/nova/objects/service.py @@ -31,7 +31,7 @@ LOG = logging.getLogger(__name__) # NOTE(danms): This is the global service version counter -SERVICE_VERSION = 23 +SERVICE_VERSION = 24 # NOTE(danms): This is our SERVICE_VERSION history. The idea is that any @@ -115,6 +115,8 @@ SERVICE_VERSION_HISTORY = ( # Version 23: Compute hosts allow pre-creation of the migration object # for cold migration. {'compute_rpc': '4.18'}, + # Version 24: Add support for Cinder v3 attach/detach API. + {'compute_rpc': '4.18'}, ) diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index ee3e8885c180..ffb76ebe1bf0 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -32,6 +32,7 @@ from oslo_config import cfg import oslo_messaging as messaging from oslo_messaging import conffixture as messaging_conffixture from oslo_privsep import daemon as privsep_daemon +from oslo_utils import uuidutils from requests import adapters from wsgi_intercept import interceptor @@ -1459,6 +1460,179 @@ class CinderFixture(fixtures.Fixture): fake_unreserve_volume) +# TODO(mriedem): We can probably pull some of the common parts from the +# CinderFixture into a common mixin class for things like the variables +# and fake_get. +class CinderFixtureNewAttachFlow(fixtures.Fixture): + """A fixture to volume operations with the new Cinder attach/detach API""" + + # the default project_id in OSAPIFixtures + tenant_id = '6f70656e737461636b20342065766572' + + SWAP_OLD_VOL = 'a07f71dc-8151-4e7d-a0cc-cd24a3f11113' + SWAP_NEW_VOL = '227cc671-f30b-4488-96fd-7d0bf13648d8' + SWAP_ERR_OLD_VOL = '828419fa-3efb-4533-b458-4267ca5fe9b1' + SWAP_ERR_NEW_VOL = '9c6d9c2d-7a8f-4c80-938d-3bf062b8d489' + SWAP_ERR_ATTACH_ID = '4a3cd440-b9c2-11e1-afa6-0800200c9a66' + + # This represents a bootable image-backed volume to test + # boot-from-volume scenarios. + IMAGE_BACKED_VOL = '6ca404f3-d844-4169-bb96-bc792f37de98' + + def __init__(self, test): + super(CinderFixtureNewAttachFlow, self).__init__() + self.test = test + self.swap_error = False + self.swap_volume_instance_uuid = None + self.swap_volume_instance_error_uuid = None + self.attachment_error_id = None + # This is a map of instance UUIDs mapped to a list of volume IDs. + # This map gets updated on attach/detach operations. + self.attachments = collections.defaultdict(list) + + def setUp(self): + super(CinderFixtureNewAttachFlow, self).setUp() + + def fake_get(self_api, context, volume_id): + # Check for the special swap volumes. + if volume_id in (CinderFixture.SWAP_OLD_VOL, + CinderFixture.SWAP_ERR_OLD_VOL): + volume = { + 'status': 'available', + 'display_name': 'TEST1', + 'attach_status': 'detached', + 'id': volume_id, + 'size': 1 + } + if ((self.swap_volume_instance_uuid and + volume_id == CinderFixture.SWAP_OLD_VOL) or + (self.swap_volume_instance_error_uuid and + volume_id == CinderFixture.SWAP_ERR_OLD_VOL)): + instance_uuid = (self.swap_volume_instance_uuid + if volume_id == CinderFixture.SWAP_OLD_VOL + else self.swap_volume_instance_error_uuid) + + volume.update({ + 'status': 'in-use', + 'attachments': { + instance_uuid: { + 'mountpoint': '/dev/vdb', + 'attachment_id': volume_id + } + }, + 'attach_status': 'attached' + }) + return volume + + # Check to see if the volume is attached. + for instance_uuid, volumes in self.attachments.items(): + if volume_id in volumes: + # The volume is attached. + volume = { + 'status': 'in-use', + 'display_name': volume_id, + 'attach_status': 'attached', + 'id': volume_id, + 'size': 1, + 'attachments': { + instance_uuid: { + 'attachment_id': volume_id, + 'mountpoint': '/dev/vdb' + } + } + } + break + else: + # This is a test that does not care about the actual details. + volume = { + 'status': 'available', + 'display_name': 'TEST2', + 'attach_status': 'detached', + 'id': volume_id, + 'size': 1 + } + + # update the status based on existing attachments + has_attachment = any( + [volume['id'] in attachments + for attachments in self.attachments.values()]) + volume['status'] = 'attached' if has_attachment else 'detached' + + # Check for our special image-backed volume. + if volume_id == self.IMAGE_BACKED_VOL: + # Make it a bootable volume. + volume['bootable'] = True + # Add the image_id metadata. + volume['volume_image_metadata'] = { + # There would normally be more image metadata in here... + 'image_id': '155d900f-4e14-4e4c-a73d-069cbf4541e6' + } + + return volume + + def fake_migrate_volume_completion(self, context, old_volume_id, + new_volume_id, error): + return {'save_volume_id': new_volume_id} + + def fake_attachment_create(_self, context, volume_id, instance_uuid, + connector=None): + attachment_id = uuidutils.generate_uuid() + if self.attachment_error_id is not None: + attachment_id = self.attachment_error_id + attachment = {'id': attachment_id, 'connection_info': {'data': {}}} + self.attachments['instance_uuid'].append(instance_uuid) + self.attachments[instance_uuid].append(volume_id) + + return attachment + + def fake_attachment_delete(_self, context, attachment_id): + instance_uuid = self.attachments['instance_uuid'][0] + del self.attachments[instance_uuid][0] + del self.attachments['instance_uuid'][0] + if attachment_id == CinderFixtureNewAttachFlow.SWAP_ERR_ATTACH_ID: + self.swap_error = True + + def fake_attachment_update(_self, context, attachment_id, connector): + attachment_ref = {'driver_volume_type': 'fake_type', + 'id': attachment_id, + 'connection_info': {'data': + {'foo': 'bar', + 'target_lun': '1'}}} + if attachment_id == CinderFixtureNewAttachFlow.SWAP_ERR_ATTACH_ID: + attachment_ref = {'connection_info': ()} + return attachment_ref + + def fake_attachment_get(_self, context, attachment_id): + attachment_ref = {'driver_volume_type': 'fake_type', + 'id': attachment_id, + 'connection_info': {'data': + {'foo': 'bar', + 'target_lun': '1'}}} + return attachment_ref + + self.test.stub_out('nova.volume.cinder.API.attachment_create', + fake_attachment_create) + self.test.stub_out('nova.volume.cinder.API.attachment_delete', + fake_attachment_delete) + self.test.stub_out('nova.volume.cinder.API.attachment_update', + fake_attachment_update) + self.test.stub_out('nova.volume.cinder.API.attachment_complete', + lambda *args, **kwargs: None) + self.test.stub_out('nova.volume.cinder.API.attachment_get', + fake_attachment_get) + self.test.stub_out('nova.volume.cinder.API.begin_detaching', + lambda *args, **kwargs: None) + self.test.stub_out('nova.volume.cinder.API.get', + fake_get) + self.test.stub_out( + 'nova.volume.cinder.API.migrate_volume_completion', + fake_migrate_volume_completion) + self.test.stub_out('nova.volume.cinder.API.roll_detaching', + lambda *args, **kwargs: None) + self.test.stub_out('nova.volume.cinder.is_microversion_supported', + lambda *args, **kwargs: None) + + class PlacementApiClient(object): def __init__(self, placement_fixture): self.fixture = placement_fixture diff --git a/nova/tests/functional/api_sample_tests/test_volumes.py b/nova/tests/functional/api_sample_tests/test_volumes.py index 36d962f8df2b..ca6166ccdf62 100644 --- a/nova/tests/functional/api_sample_tests/test_volumes.py +++ b/nova/tests/functional/api_sample_tests/test_volumes.py @@ -15,6 +15,7 @@ import datetime +from nova.compute import api as compute_api from nova import context from nova import objects from nova.tests import fixtures @@ -24,6 +25,9 @@ from nova.tests.unit.api.openstack import fakes from nova.tests.unit import fake_block_device from nova.tests.unit import fake_instance +COMPUTE_VERSION_OLD_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1 + class SnapshotsSampleJsonTests(api_sample_base.ApiSampleTestBaseV21): sample_dir = "os-volumes" @@ -238,6 +242,8 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): self.stub_out('nova.compute.api.API.get', fake_compute_api_get) def test_attach_volume_to_server(self): + self.stub_out('nova.objects.Service.get_minimum_version', + lambda *a, **k: COMPUTE_VERSION_OLD_ATTACH_FLOW) self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) self.stub_out('nova.volume.cinder.API.reserve_volume', lambda *a, **k: None) @@ -265,6 +271,38 @@ class VolumeAttachmentsSample(test_servers.ServersSampleBase): self._verify_response('attach-volume-to-server-resp', subs, response, 200) + def test_attach_volume_to_server_new_flow(self): + self.stub_out('nova.volume.cinder.is_microversion_supported', + lambda *a, **k: None) + self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) + self.stub_out('nova.volume.cinder.API.attachment_create', + lambda *a, **k: {'id': 'fake_id'}) + self.stub_out('nova.objects.BlockDeviceMapping.save', + lambda *a, **k: None) + device_name = '/dev/vdd' + bdm = objects.BlockDeviceMapping() + bdm['device_name'] = device_name + self.stub_out( + 'nova.compute.manager.ComputeManager.reserve_block_device_name', + lambda *a, **k: bdm) + self.stub_out( + 'nova.compute.manager.ComputeManager.attach_volume', + lambda *a, **k: None) + + volume = fakes.stub_volume_get(None, context.get_admin_context(), + 'a26887c6-c47b-4654-abb5-dfadf7d3f803') + subs = { + 'volume_id': volume['id'], + 'device': device_name + } + server_id = self._post_server() + response = self._do_post('servers/%s/os-volume_attachments' + % server_id, + 'attach-volume-to-server-req', subs) + + self._verify_response('attach-volume-to-server-resp', subs, + response, 200) + def test_list_volume_attachments(self): server_id = self._post_server() self._stub_db_bdms_get_all_by_instance(server_id) @@ -327,7 +365,7 @@ class VolumeAttachmentsSampleV249(test_servers.ServersSampleBase): def setUp(self): super(VolumeAttachmentsSampleV249, self).setUp() - self.useFixture(fixtures.CinderFixture(self)) + self.useFixture(fixtures.CinderFixtureNewAttachFlow(self)) def test_attach_volume_to_server(self): device_name = '/dev/sdb' @@ -347,3 +385,35 @@ class VolumeAttachmentsSampleV249(test_servers.ServersSampleBase): self._verify_response('attach-volume-to-server-resp', subs, response, 200) + + +class VolumeAttachmentsSampleV249OldCinderFlow(test_servers.ServersSampleBase): + + sample_dir = "os-volumes" + microversion = '2.49' + scenarios = [('v2_49', {'api_major_version': 'v2.1'})] + + def setUp(self): + super(VolumeAttachmentsSampleV249OldCinderFlow, self).setUp() + self.useFixture(fixtures.CinderFixture(self)) + + def test_attach_volume_to_server(self): + device_name = '/dev/sdb' + bdm = objects.BlockDeviceMapping() + bdm['device_name'] = device_name + volume = fakes.stub_volume_get(None, context.get_admin_context(), + 'a26887c6-c47b-4654-abb5-dfadf7d3f803') + self.stub_out('nova.objects.Service.get_minimum_version', + lambda *a, **k: COMPUTE_VERSION_OLD_ATTACH_FLOW) + subs = { + 'volume_id': volume['id'], + 'device': device_name, + 'tag': 'foo', + } + server_id = self._post_server() + response = self._do_post('servers/%s/os-volume_attachments' + % server_id, + 'attach-volume-to-server-req', subs) + + self._verify_response('attach-volume-to-server-resp', subs, + response, 200) diff --git a/nova/tests/functional/notification_sample_tests/test_instance.py b/nova/tests/functional/notification_sample_tests/test_instance.py index e5340f74c5ad..2c5fc100b2af 100644 --- a/nova/tests/functional/notification_sample_tests/test_instance.py +++ b/nova/tests/functional/notification_sample_tests/test_instance.py @@ -14,6 +14,7 @@ import time import mock +from nova.compute import api as compute_api from nova import context from nova import exception from nova.tests import fixtures @@ -23,6 +24,9 @@ from nova.tests.functional.notification_sample_tests \ from nova.tests.unit import fake_notifier from nova.virt import fake +COMPUTE_VERSION_OLD_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1 + class TestInstanceNotificationSampleWithMultipleCompute( notification_sample_base.NotificationSampleTestBase): @@ -34,7 +38,7 @@ class TestInstanceNotificationSampleWithMultipleCompute( super(TestInstanceNotificationSampleWithMultipleCompute, self).setUp() self.neutron = fixtures.NeutronFixture(self) self.useFixture(self.neutron) - self.cinder = fixtures.CinderFixture(self) + self.cinder = fixtures.CinderFixtureNewAttachFlow(self) self.useFixture(self.cinder) self.useFixture(fixtures.AllServicesCurrent()) @@ -177,6 +181,26 @@ class TestInstanceNotificationSampleWithMultipleCompute( actual=fake_notifier.VERSIONED_NOTIFICATIONS[5]) +class TestInstanceNotificationSampleWithMultipleComputeOldAttachFlow( + TestInstanceNotificationSampleWithMultipleCompute): + + def setUp(self): + self.flags(compute_driver='fake.FakeLiveMigrateDriver') + self.flags(use_neutron=True) + self.flags(bdms_in_notifications='True', group='notifications') + super(TestInstanceNotificationSampleWithMultipleCompute, self).setUp() + self.neutron = fixtures.NeutronFixture(self) + self.useFixture(self.neutron) + self.cinder = fixtures.CinderFixture(self) + self.useFixture(self.cinder) + + patcher = self.mock_min_service_version = \ + mock.patch('nova.objects.Service.get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) + self.mock_min_service_version = patcher.start() + self.addCleanup(patcher.stop) + + class TestInstanceNotificationSample( notification_sample_base.NotificationSampleTestBase): @@ -186,7 +210,7 @@ class TestInstanceNotificationSample( super(TestInstanceNotificationSample, self).setUp() self.neutron = fixtures.NeutronFixture(self) self.useFixture(self.neutron) - self.cinder = fixtures.CinderFixture(self) + self.cinder = fixtures.CinderFixtureNewAttachFlow(self) self.useFixture(self.cinder) def _wait_until_swap_volume(self, server, volume_id): @@ -1067,12 +1091,17 @@ class TestInstanceNotificationSample( 'uuid': server['id']}, actual=fake_notifier.VERSIONED_NOTIFICATIONS[6]) - def test_volume_swap_server_with_error(self): + def _do_setup_server_and_error_flag(self): server = self._boot_a_server( extra_params={'networks': [{'port': self.neutron.port_1['id']}]}) - self._attach_volume_to_server(server, self.cinder.SWAP_ERR_OLD_VOL) - self.cinder.swap_volume_instance_error_uuid = server['id'] + + self.cinder.attachment_error_id = self.cinder.SWAP_ERR_ATTACH_ID + + return server + + def test_volume_swap_server_with_error(self): + server = self._do_setup_server_and_error_flag() self._volume_swap_server(server, self.cinder.SWAP_ERR_OLD_VOL, self.cinder.SWAP_ERR_NEW_VOL) @@ -1212,8 +1241,7 @@ class TestInstanceNotificationSample( # Leave instance in normal, active state self.api.post_server_action(server['id'], {'restore': {}}) - @mock.patch('nova.volume.cinder.API.attach') - def _test_attach_volume_error(self, server, mock_attach): + def _do_test_attach_volume_error(self, server, mock_attach): def attach_volume(*args, **kwargs): raise exception.CinderConnectionFailed( reason="Connection timed out") @@ -1267,6 +1295,10 @@ class TestInstanceNotificationSample( 'uuid': server['id']}, actual=fake_notifier.VERSIONED_NOTIFICATIONS[1]) + @mock.patch('nova.volume.cinder.API.attachment_update') + def _test_attach_volume_error(self, server, mock_attach): + self._do_test_attach_volume_error(server, mock_attach) + def _test_interface_attach_and_detach(self, server): post = { 'interfaceAttachment': { @@ -1340,3 +1372,35 @@ class TestInstanceNotificationSample( 'reservation_id': server['reservation_id'], 'uuid': server['id']}, actual=fake_notifier.VERSIONED_NOTIFICATIONS[1]) + + +class TestInstanceNotificationSampleOldAttachFlow( + TestInstanceNotificationSample): + + def setUp(self): + self.flags(use_neutron=True) + self.flags(bdms_in_notifications='True', group='notifications') + super(TestInstanceNotificationSample, self).setUp() + self.neutron = fixtures.NeutronFixture(self) + self.useFixture(self.neutron) + self.cinder = fixtures.CinderFixture(self) + self.useFixture(self.cinder) + + patcher = self.mock_min_service_version = \ + mock.patch('nova.objects.Service.get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) + self.mock_min_service_version = patcher.start() + self.addCleanup(patcher.stop) + + def _do_setup_server_and_error_flag(self): + server = self._boot_a_server( + extra_params={'networks': [{'port': self.neutron.port_1['id']}]}) + self._attach_volume_to_server(server, self.cinder.SWAP_ERR_OLD_VOL) + + self.cinder.swap_volume_instance_error_uuid = server['id'] + + return server + + @mock.patch('nova.volume.cinder.API.attach') + def _test_attach_volume_error(self, server, mock_attach): + self._do_test_attach_volume_error(server, mock_attach) diff --git a/nova/tests/functional/regressions/test_bug_1675570.py b/nova/tests/functional/regressions/test_bug_1675570.py index b07546f7e83f..9ce6f68c485b 100644 --- a/nova/tests/functional/regressions/test_bug_1675570.py +++ b/nova/tests/functional/regressions/test_bug_1675570.py @@ -14,8 +14,10 @@ import time +import mock from oslo_log import log as logging +from nova.compute import api as compute_api from nova import test from nova.tests import fixtures as nova_fixtures from nova.tests.functional.api import client @@ -25,6 +27,9 @@ from nova.tests.unit import policy_fixture LOG = logging.getLogger(__name__) +COMPUTE_VERSION_OLD_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1 + class TestLocalDeleteAttachedVolumes(test.TestCase): """Test local delete in the API of a server with a volume attached. @@ -44,7 +49,8 @@ class TestLocalDeleteAttachedVolumes(test.TestCase): super(TestLocalDeleteAttachedVolumes, self).setUp() self.useFixture(policy_fixture.RealPolicyFixture()) # We need the CinderFixture to stub out the volume API. - self.cinder = self.useFixture(nova_fixtures.CinderFixture(self)) + self.cinder = self.useFixture( + nova_fixtures.CinderFixtureNewAttachFlow(self)) # The NeutronFixture is needed to stub out validate_networks in API. self.useFixture(nova_fixtures.NeutronFixture(self)) # Use the PlacementFixture to avoid annoying warnings in the logs. @@ -124,7 +130,7 @@ class TestLocalDeleteAttachedVolumes(test.TestCase): 'server %s. Currently attached volumes: %s' % (volume_id, server_id, attached_vols)) - def test_local_delete_with_volume_attached(self): + def test_local_delete_with_volume_attached(self, mock_version_get=None): LOG.info('Creating server and waiting for it to be ACTIVE.') server = dict( name='local-delete-volume-attach-test', @@ -167,3 +173,12 @@ class TestLocalDeleteAttachedVolumes(test.TestCase): volume_id, server_id) # Now that the bug is fixed, assert the volume was detached. self.assertNotIn(volume_id, self.cinder.attachments[server_id]) + + +@mock.patch('nova.objects.Service.get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) +class TestLocalDeleteAttachedVolumesOldFlow(TestLocalDeleteAttachedVolumes): + + def setUp(self): + super(TestLocalDeleteAttachedVolumesOldFlow, self).setUp() + self.cinder = self.useFixture(nova_fixtures.CinderFixture(self)) diff --git a/nova/tests/functional/regressions/test_bug_1732947.py b/nova/tests/functional/regressions/test_bug_1732947.py index 5dcdf6e57f5e..be8583434b0c 100644 --- a/nova/tests/functional/regressions/test_bug_1732947.py +++ b/nova/tests/functional/regressions/test_bug_1732947.py @@ -34,7 +34,7 @@ class RebuildVolumeBackedSameImage(integrated_helpers._IntegratedTestBase, def setUp(self): super(RebuildVolumeBackedSameImage, self).setUp() # We are creating a volume-backed server so we need the CinderFixture. - self.useFixture(nova_fixtures.CinderFixture(self)) + self.useFixture(nova_fixtures.CinderFixtureNewAttachFlow(self)) def _setup_scheduler_service(self): # Add the IsolatedHostsFilter to the list of enabled filters since it diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index aa02bfd5124f..9e0ea6262f19 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -988,11 +988,16 @@ class ServerTestV220(ServersTestBase): server_id = found_server['id'] # Test attach volume - with test.nested(mock.patch.object(compute_api.API, + with test.nested(mock.patch.object(volume.cinder, + 'is_microversion_supported'), + mock.patch.object(compute_api.API, '_check_attach_and_reserve_volume'), mock.patch.object(rpcapi.ComputeAPI, - 'attach_volume')) as (mock_reserve, + 'attach_volume')) as (mock_cinder_mv, + mock_reserve, mock_attach): + mock_cinder_mv.side_effect = \ + exception.CinderAPIVersionNotAvailable(version='3.44') volume_attachment = {"volumeAttachment": {"volumeId": "5d721593-f033-4f6d-ab6f-b5b067e61bc4"}} self.api.api_post( @@ -1028,10 +1033,15 @@ class ServerTestV220(ServersTestBase): server_id = found_server['id'] # Test attach volume - with test.nested(mock.patch.object(compute_api.API, + with test.nested(mock.patch.object(volume.cinder, + 'is_microversion_supported'), + mock.patch.object(compute_api.API, '_check_attach_and_reserve_volume'), mock.patch.object(volume.cinder.API, - 'attach')) as (mock_reserve, mock_vol): + 'attach')) as (mock_cinder_mv, + mock_reserve, mock_vol): + mock_cinder_mv.side_effect = \ + exception.CinderAPIVersionNotAvailable(version='3.44') volume_attachment = {"volumeAttachment": {"volumeId": "5d721593-f033-4f6d-ab6f-b5b067e61bc4"}} attach_response = self.api.api_post( @@ -1060,6 +1070,58 @@ class ServerTestV220(ServersTestBase): self._delete_server(server_id) + def test_attach_detach_vol_to_shelved_offloaded_server_new_flow(self): + self.flags(shelved_offload_time=0) + found_server = self._shelve_server() + self.assertEqual('SHELVED_OFFLOADED', found_server['status']) + server_id = found_server['id'] + fake_bdms = self._get_fake_bdms(self.ctxt) + + # Test attach volume + self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) + with test.nested(mock.patch.object(volume.cinder, + 'is_microversion_supported'), + mock.patch.object(compute_api.API, + '_check_volume_already_attached_to_instance'), + mock.patch.object(volume.cinder.API, + 'check_availability_zone'), + mock.patch.object(volume.cinder.API, + 'attachment_create'), + mock.patch.object(volume.cinder.API, + 'attachment_complete') + ) as (mock_cinder_mv, mock_check_vol_attached, + mock_check_av_zone, mock_attach_create, + mock_attachment_complete): + mock_attach_create.return_value = {'id': 'fake_id'} + volume_attachment = {"volumeAttachment": {"volumeId": + "5d721593-f033-4f6d-ab6f-b5b067e61bc4"}} + attach_response = self.api.api_post( + '/servers/%s/os-volume_attachments' % (server_id), + volume_attachment).body['volumeAttachment'] + self.assertTrue(mock_attach_create.called) + mock_attachment_complete.assert_called_once_with( + mock.ANY, 'fake_id') + self.assertIsNone(attach_response['device']) + + # Test detach volume + self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get) + with test.nested(mock.patch.object(volume.cinder.API, + 'begin_detaching'), + mock.patch.object(objects.BlockDeviceMappingList, + 'get_by_instance_uuid'), + mock.patch.object(compute_api.API, + '_local_cleanup_bdm_volumes') + ) as (mock_check, mock_get_bdms, mock_clean_vols): + + mock_get_bdms.return_value = fake_bdms + attachment_id = mock_get_bdms.return_value[0]['volume_id'] + self.api.api_delete('/servers/%s/os-volume_attachments/%s' % + (server_id, attachment_id)) + self.assertTrue(mock_check.called) + self.assertTrue(mock_clean_vols.called) + + self._delete_server(server_id) + class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase, integrated_helpers.InstanceHelperMixin): @@ -1230,7 +1292,7 @@ class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase, different image than what is in the root disk of the root volume will result in a 400 BadRequest error. """ - self.useFixture(nova_fixtures.CinderFixture(self)) + self.useFixture(nova_fixtures.CinderFixtureNewAttachFlow(self)) # First create our server as normal. server_req_body = { # There is no imageRef because this is boot from volume. @@ -1242,7 +1304,8 @@ class ServerRebuildTestCase(integrated_helpers._IntegratedTestBase, 'networks': 'none', 'block_device_mapping_v2': [{ 'boot_index': 0, - 'uuid': nova_fixtures.CinderFixture.IMAGE_BACKED_VOL, + 'uuid': + nova_fixtures.CinderFixtureNewAttachFlow.IMAGE_BACKED_VOL, 'source_type': 'volume', 'destination_type': 'volume' }] diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 220a873c26cb..f57703a7788d 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -104,6 +104,11 @@ FAKE_IMAGE_REF = uuids.image_ref NODENAME = 'fakenode1' NODENAME2 = 'fakenode2' +COMPUTE_VERSION_NEW_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION +COMPUTE_VERSION_OLD_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1 + def fake_not_implemented(*args, **kwargs): raise NotImplementedError() @@ -446,6 +451,43 @@ class ComputeVolumeTestCase(BaseTestCase): mock_event.assert_called_once_with( self.context, 'compute_attach_volume', instance.uuid) + @mock.patch('nova.context.RequestContext.elevated') + @mock.patch('nova.compute.utils.notify_about_volume_attach_detach') + def test_attach_volume_raises_new_flow(self, mock_notify, mock_elevate): + mock_elevate.return_value = self.context + + attachment_id = uuids.attachment_id + fake_bdm = objects.BlockDeviceMapping(**self.fake_volume) + fake_bdm.attachment_id = attachment_id + instance = self._create_fake_instance_obj() + expected_exception = test.TestingException() + + def fake_attach(*args, **kwargs): + raise expected_exception + + with test.nested( + mock.patch.object(driver_block_device.DriverVolumeBlockDevice, + 'attach'), + mock.patch.object(cinder.API, 'attachment_delete'), + mock.patch.object(objects.BlockDeviceMapping, + 'destroy') + ) as (mock_attach, mock_attachment_delete, mock_destroy): + mock_attach.side_effect = fake_attach + self.assertRaises( + test.TestingException, self.compute.attach_volume, + self.context, instance, fake_bdm) + self.assertTrue(mock_attachment_delete.called) + self.assertTrue(mock_destroy.called) + mock_notify.assert_has_calls([ + mock.call(self.context, instance, 'fake-mini', + action='volume_attach', phase='start', + volume_id=uuids.volume_id), + mock.call(self.context, instance, 'fake-mini', + action='volume_attach', phase='error', + volume_id=uuids.volume_id, + exception=expected_exception), + ]) + @mock.patch.object(compute_utils, 'EventReporter') def test_detach_volume_api_raises(self, mock_event): fake_bdm = objects.BlockDeviceMapping(**self.fake_volume) @@ -569,8 +611,6 @@ class ComputeVolumeTestCase(BaseTestCase): self.assertEqual(1, attempts) def test_boot_volume_serial(self): - self.stub_out('nova.volume.cinder.API.check_attach', - lambda *a, **kw: None) with ( mock.patch.object(objects.BlockDeviceMapping, 'save') ) as mock_save: @@ -585,6 +625,7 @@ class ComputeVolumeTestCase(BaseTestCase): 'device_name': '/dev/vdb', 'volume_size': 55, 'delete_on_termination': False, + 'attachment_id': None, })] bdms = block_device_obj.block_device_make_list_from_dicts( self.context, block_device_mapping) @@ -10062,14 +10103,16 @@ class ComputeAPITestCase(BaseTestCase): fake_volume = {'id': 'fake-volume-id'} with test.nested( + mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW), mock.patch.object(cinder.API, 'get', return_value=fake_volume), mock.patch.object(cinder.API, 'check_availability_zone'), mock.patch.object(cinder.API, 'reserve_volume'), mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name', return_value=bdm), mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - ) as (mock_get, mock_check_availability_zone, mock_reserve_vol, - mock_reserve_bdm, mock_attach): + ) as (mock_get_version, mock_get, mock_check_availability_zone, + mock_reserve_vol, mock_reserve_bdm, mock_attach): self.compute_api.attach_volume( self.context, instance, 'fake-volume-id', @@ -10089,20 +10132,185 @@ class ComputeAPITestCase(BaseTestCase): self.assertEqual(a[2].device_name, '/dev/vdb') self.assertEqual(a[2].volume_id, uuids.volume_id) + def test_attach_volume_new_flow(self): + fake_bdm = fake_block_device.FakeDbBlockDeviceDict( + {'source_type': 'volume', 'destination_type': 'volume', + 'volume_id': uuids.volume_id, 'device_name': '/dev/vdb'}) + bdm = block_device_obj.BlockDeviceMapping()._from_db_object( + self.context, + block_device_obj.BlockDeviceMapping(), + fake_bdm) + instance = self._create_fake_instance_obj() + instance.id = 42 + fake_volume = {'id': 'fake-volume-id'} + + with test.nested( + mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW), + mock.patch.object(cinder.API, 'get', return_value=fake_volume), + mock.patch.object(cinder, 'is_microversion_supported'), + mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance'), + mock.patch.object(cinder.API, 'check_availability_zone'), + mock.patch.object(cinder.API, 'attachment_create', + return_value={'id': uuids.attachment_id}), + mock.patch.object(objects.BlockDeviceMapping, 'save'), + mock.patch.object(compute_rpcapi.ComputeAPI, + 'reserve_block_device_name', return_value=bdm), + mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + ) as (mock_get_version, mock_get, mock_mv_supported, mock_no_bdm, + mock_check_availability_zone, mock_attachment_create, + mock_bdm_save, mock_reserve_bdm, mock_attach): + mock_no_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='fake-volume-id') + self.compute_api.attach_volume( + self.context, instance, 'fake-volume-id', + '/dev/vdb', 'ide', 'cdrom') + + mock_reserve_bdm.assert_called_once_with( + self.context, instance, '/dev/vdb', 'fake-volume-id', + disk_bus='ide', device_type='cdrom', tag=None) + self.assertEqual(mock_get.call_args, + mock.call(self.context, 'fake-volume-id')) + self.assertEqual(mock_check_availability_zone.call_args, + mock.call( + self.context, fake_volume, instance=instance)) + mock_attachment_create.assert_called_once_with(self.context, + 'fake-volume-id', + instance.uuid) + a, kw = mock_attach.call_args + self.assertEqual(a[2].device_name, '/dev/vdb') + self.assertEqual(a[2].volume_id, uuids.volume_id) + + def test_attach_volume_new_flow_microversion_not_available(self): + fake_bdm = fake_block_device.FakeDbBlockDeviceDict( + {'source_type': 'volume', 'destination_type': 'volume', + 'volume_id': uuids.volume_id, 'device_name': '/dev/vdb'}) + bdm = block_device_obj.BlockDeviceMapping()._from_db_object( + self.context, + block_device_obj.BlockDeviceMapping(), + fake_bdm) + instance = self._create_fake_instance_obj() + instance.id = 42 + fake_volume = {'id': 'fake-volume-id'} + + with test.nested( + mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW), + mock.patch.object(cinder.API, 'get', return_value=fake_volume), + mock.patch.object(cinder, 'is_microversion_supported'), + mock.patch.object(cinder.API, 'check_availability_zone'), + mock.patch.object(cinder.API, 'attachment_create'), + mock.patch.object(cinder.API, 'reserve_volume'), + mock.patch.object(compute_rpcapi.ComputeAPI, + 'reserve_block_device_name', return_value=bdm), + mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + ) as (mock_get_version, mock_get, mock_mv_supported, + mock_check_availability_zone, mock_attachment_create, + mock_reserve_vol, mock_reserve_bdm, mock_attach): + mock_mv_supported.side_effect = \ + exception.CinderAPIVersionNotAvailable(version='3.44') + mock_attachment_create.side_effect = \ + exception.CinderAPIVersionNotAvailable(version='3.44') + self.compute_api.attach_volume( + self.context, instance, 'fake-volume-id', + '/dev/vdb', 'ide', 'cdrom') + + mock_reserve_bdm.assert_called_once_with( + self.context, instance, '/dev/vdb', 'fake-volume-id', + disk_bus='ide', device_type='cdrom', tag=None) + self.assertEqual(mock_get.call_args, + mock.call(self.context, 'fake-volume-id')) + self.assertEqual(mock_check_availability_zone.call_args, + mock.call( + self.context, fake_volume, instance=instance)) + mock_attachment_create.assert_called_once_with(self.context, + 'fake-volume-id', + instance.uuid) + mock_reserve_vol.assert_called_once_with( + self.context, 'fake-volume-id') + a, kw = mock_attach.call_args + self.assertEqual(a[2].device_name, '/dev/vdb') + self.assertEqual(a[2].volume_id, uuids.volume_id) + + def test_attach_volume_no_device_new_flow(self): + fake_bdm = fake_block_device.FakeDbBlockDeviceDict( + {'source_type': 'volume', 'destination_type': 'volume', + 'device_name': '/dev/vdb', 'volume_id': uuids.volume_id}) + bdm = block_device_obj.BlockDeviceMapping()._from_db_object( + self.context, + block_device_obj.BlockDeviceMapping(), + fake_bdm) + instance = self._create_fake_instance_obj() + instance.id = 42 + fake_volume = {'id': 'fake-volume-id'} + + with test.nested( + mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW), + mock.patch.object(cinder.API, 'get', return_value=fake_volume), + mock.patch.object(cinder, 'is_microversion_supported'), + mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance', + side_effect=exception.VolumeBDMNotFound), + mock.patch.object(cinder.API, 'check_availability_zone'), + mock.patch.object(cinder.API, 'attachment_create', + return_value={'id': uuids.attachment_id}), + mock.patch.object(objects.BlockDeviceMapping, 'save'), + mock.patch.object(compute_rpcapi.ComputeAPI, + 'reserve_block_device_name', return_value=bdm), + mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + ) as (mock_get_version, mock_get, mock_mv_supported, mock_no_bdm, + mock_check_availability_zone, mock_attachment_create, + mock_bdm_save, mock_reserve_bdm, mock_attach): + mock_no_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='fake-volume-id') + + self.compute_api.attach_volume( + self.context, instance, 'fake-volume-id', + device=None) + + mock_reserve_bdm.assert_called_once_with( + self.context, instance, None, 'fake-volume-id', + disk_bus=None, device_type=None, tag=None) + self.assertEqual(mock_get.call_args, + mock.call(self.context, 'fake-volume-id')) + self.assertEqual(mock_check_availability_zone.call_args, + mock.call( + self.context, fake_volume, instance=instance)) + mock_attachment_create.assert_called_once_with(self.context, + 'fake-volume-id', + instance.uuid) + a, kw = mock_attach.call_args + self.assertEqual(a[2].volume_id, uuids.volume_id) + def test_attach_volume_shelved_offloaded(self): instance = self._create_fake_instance_obj() + fake_bdm = objects.BlockDeviceMapping( + **fake_block_device.FakeDbBlockDeviceDict( + {'volume_id': 'fake-volume-id', + 'source_type': 'volume', + 'destination_type': 'volume', + 'device_type': 'cdrom', + 'disk_bus': 'ide', + 'instance_uuid': instance.uuid})) with test.nested( + mock.patch.object(compute_api.API, + '_create_volume_bdm', + return_value=fake_bdm), mock.patch.object(compute_api.API, '_check_attach_and_reserve_volume'), mock.patch.object(cinder.API, 'attach'), mock.patch.object(compute_utils, 'EventReporter') - ) as (mock_attach_and_reserve, mock_attach, mock_event): + ) as (mock_bdm_create, mock_attach_and_reserve, mock_attach, + mock_event): self.compute_api._attach_volume_shelved_offloaded( self.context, instance, 'fake-volume-id', '/dev/vdb', 'ide', 'cdrom') mock_attach_and_reserve.assert_called_once_with(self.context, 'fake-volume-id', - instance) + instance, + fake_bdm) mock_attach.assert_called_once_with(self.context, 'fake-volume-id', instance.uuid, @@ -10112,10 +10320,46 @@ class ComputeAPITestCase(BaseTestCase): ) self.assertTrue(mock_attach.called) + def test_attach_volume_shelved_offloaded_new_flow(self): + instance = self._create_fake_instance_obj() + fake_bdm = objects.BlockDeviceMapping( + **fake_block_device.FakeDbBlockDeviceDict( + {'volume_id': 'fake-volume-id', + 'source_type': 'volume', + 'destination_type': 'volume', + 'device_type': 'cdrom', + 'disk_bus': 'ide', + 'instance_uuid': instance.uuid})) + + def fake_check_attach_and_reserve(*args, **kwargs): + fake_bdm.attachment_id = uuids.attachment_id + + with test.nested( + mock.patch.object(compute_api.API, + '_create_volume_bdm', + return_value=fake_bdm), + mock.patch.object(compute_api.API, + '_check_attach_and_reserve_volume', + side_effect=fake_check_attach_and_reserve), + mock.patch.object(cinder.API, 'attachment_complete') + ) as (mock_bdm_create, mock_attach_and_reserve, mock_attach_complete): + self.compute_api._attach_volume_shelved_offloaded( + self.context, instance, 'fake-volume-id', + '/dev/vdb', 'ide', 'cdrom') + mock_attach_and_reserve.assert_called_once_with(self.context, + 'fake-volume-id', + instance, + fake_bdm) + mock_attach_complete.assert_called_once_with( + self.context, uuids.attachment_id) + def test_attach_volume_no_device(self): called = {} + def fake_mv_supported(*args, **kwargs): + raise exception.CinderAPIVersionNotAvailable(version='3.44') + def fake_check_availability_zone(*args, **kwargs): called['fake_check_availability_zone'] = True @@ -10137,6 +10381,8 @@ class ComputeAPITestCase(BaseTestCase): return bdm self.stub_out('nova.volume.cinder.API.get', fake_volume_get) + self.stub_out('nova.volume.cinder.is_microversion_supported', + fake_mv_supported) self.stub_out('nova.volume.cinder.API.check_availability_zone', fake_check_availability_zone) self.stub_out('nova.volume.cinder.API.reserve_volume', @@ -10192,8 +10438,9 @@ class ComputeAPITestCase(BaseTestCase): mock_begin_detaching, mock_event, mock_record): - mock_block_dev.return_value = [block_device_obj.BlockDeviceMapping( - context=context)] + fake_bdm = block_device_obj.BlockDeviceMapping(context=context) + fake_bdm.attachment_id = None + mock_block_dev.return_value = fake_bdm instance = self._create_fake_instance_obj() volume = {'id': 1, 'attach_status': 'fake'} self.compute_api._detach_volume_shelved_offloaded(self.context, @@ -10208,6 +10455,27 @@ class ComputeAPITestCase(BaseTestCase): instance.uuid) self.assertTrue(mock_local_cleanup.called) + @mock.patch.object(nova.volume.cinder.API, 'begin_detaching') + @mock.patch.object(compute_api.API, '_local_cleanup_bdm_volumes') + @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume_id') + def test_detach_volume_shelved_offloaded_new_flow(self, + mock_block_dev, + mock_local_cleanup, + mock_begin_detaching): + + fake_bdm = block_device_obj.BlockDeviceMapping(context=context) + fake_bdm.attachment_id = uuids.attachment_id + fake_bdm.volume_id = 1 + mock_block_dev.return_value = fake_bdm + instance = self._create_fake_instance_obj() + volume = {'id': 1, 'attach_status': 'fake'} + self.compute_api._detach_volume_shelved_offloaded(self.context, + instance, + volume) + mock_begin_detaching.assert_called_once_with(self.context, + volume['id']) + self.assertTrue(mock_local_cleanup.called) + @mock.patch.object(nova.volume.cinder.API, 'begin_detaching', side_effect=exception.InvalidInput(reason='error')) def test_detach_invalid_volume(self, mock_begin_detaching): diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 6f78b6196e83..59bc87a17a34 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -65,6 +65,10 @@ SHELVED_IMAGE = 'fake-shelved-image' SHELVED_IMAGE_NOT_FOUND = 'fake-shelved-image-notfound' SHELVED_IMAGE_NOT_AUTHORIZED = 'fake-shelved-image-not-authorized' SHELVED_IMAGE_EXCEPTION = 'fake-shelved-image-exception' +COMPUTE_VERSION_NEW_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION +COMPUTE_VERSION_OLD_ATTACH_FLOW = \ + compute_api.CINDER_V3_ATTACH_MIN_COMPUTE_VERSION - 1 @ddt.ddt @@ -383,8 +387,11 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - def test_attach_volume(self, mock_attach, mock_reserve, mock_record): + def test_attach_volume(self, mock_attach, mock_get_min_ver, mock_reserve, + mock_record): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -410,9 +417,11 @@ class _ComputeAPIUnitTestMixIn(object): @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - def test_tagged_volume_attach(self, mock_attach, mock_reserve, - mock_record): + def test_tagged_volume_attach(self, mock_attach, mock_get_min_ver, + mock_reserve, mock_record): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -440,7 +449,87 @@ class _ComputeAPIUnitTestMixIn(object): mock_record.assert_called_once_with( self.context, instance, instance_actions.ATTACH_VOLUME) - def test_attach_volume_shelved_instance(self): + @mock.patch.object(compute_api.API, '_record_action_start') + @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch('nova.volume.cinder.is_microversion_supported') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + def test_attach_volume_new_flow(self, mock_attach, mock_bdm, + mock_cinder_mv_supported, mock_get_min_ver, + mock_reserve, mock_record): + mock_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='fake-volume-id') + instance = self._create_instance_obj() + volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', + None, None, None, None, None) + + fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping) + mock_reserve.return_value = fake_bdm + + mock_volume_api = mock.patch.object(self.compute_api, 'volume_api', + mock.MagicMock(spec=cinder.API)) + + with mock_volume_api as mock_v_api: + mock_v_api.get.return_value = volume + mock_v_api.attachment_create.return_value = \ + {'id': uuids.attachment_id} + self.compute_api.attach_volume( + self.context, instance, volume['id']) + mock_v_api.check_availability_zone.assert_called_once_with( + self.context, volume, instance=instance) + mock_v_api.attachment_create.assert_called_once_with(self.context, + volume['id'], + instance.uuid) + mock_attach.assert_called_once_with(self.context, + instance, fake_bdm) + + @mock.patch.object(compute_api.API, '_record_action_start') + @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch('nova.volume.cinder.is_microversion_supported') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + def test_tagged_volume_attach_new_flow(self, mock_attach, mock_bdm, + mock_cinder_mv_supported, + mock_get_min_ver, + mock_reserve, mock_record): + mock_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='fake-volume-id') + instance = self._create_instance_obj() + volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', + None, None, None, None, None) + + fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping) + mock_reserve.return_value = fake_bdm + + mock_volume_api = mock.patch.object(self.compute_api, 'volume_api', + mock.MagicMock(spec=cinder.API)) + + with mock_volume_api as mock_v_api: + mock_v_api.get.return_value = volume + mock_v_api.attachment_create.return_value = \ + {'id': uuids.attachment_id} + self.compute_api.attach_volume( + self.context, instance, volume['id'], tag='foo') + mock_reserve.assert_called_once_with(self.context, instance, None, + volume['id'], + device_type=None, + disk_bus=None, tag='foo') + mock_v_api.check_availability_zone.assert_called_once_with( + self.context, volume, instance=instance) + mock_v_api.attachment_create.assert_called_once_with( + self.context, volume['id'], instance.uuid) + mock_attach.assert_called_once_with(self.context, + instance, fake_bdm) + + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) + def test_attach_volume_shelved_instance(self, mock_get_min_ver): instance = self._create_instance_obj() instance.vm_state = vm_states.SHELVED_OFFLOADED volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', @@ -450,8 +539,11 @@ class _ComputeAPIUnitTestMixIn(object): instance, volume['id'], tag='foo') @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - def test_attach_volume_reserve_fails(self, mock_attach, mock_reserve): + def test_attach_volume_reserve_fails(self, mock_attach, + mock_get_min_ver, mock_reserve): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -475,6 +567,42 @@ class _ComputeAPIUnitTestMixIn(object): self.assertEqual(0, mock_attach.call_count) fake_bdm.destroy.assert_called_once_with() + @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch('nova.volume.cinder.is_microversion_supported') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') + def test_attach_volume_attachment_create_fails(self, mock_attach, mock_bdm, + mock_cinder_mv_supported, + mock_get_min_ver, + mock_reserve): + mock_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='fake-volume-id') + instance = self._create_instance_obj() + volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', + None, None, None, None, None) + + fake_bdm = mock.MagicMock(spec=objects.BlockDeviceMapping) + mock_reserve.return_value = fake_bdm + + mock_volume_api = mock.patch.object(self.compute_api, 'volume_api', + mock.MagicMock(spec=cinder.API)) + + with mock_volume_api as mock_v_api: + mock_v_api.get.return_value = volume + mock_v_api.attachment_create.side_effect = test.TestingException() + self.assertRaises(test.TestingException, + self.compute_api.attach_volume, + self.context, instance, volume['id']) + mock_v_api.check_availability_zone.assert_called_once_with( + self.context, volume, instance=instance) + mock_v_api.attachment_create.assert_called_once_with( + self.context, volume['id'], instance.uuid) + self.assertEqual(0, mock_attach.call_count) + fake_bdm.destroy.assert_called_once_with() + def test_suspend(self): # Ensure instance can be suspended. instance = self._create_instance_obj() @@ -3786,6 +3914,43 @@ class _ComputeAPIUnitTestMixIn(object): self.context, instance, instance_type, bdms) + @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): + # 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() + instance_type = self._create_flavor() + volume_id = 'e856840e-9f5b-4894-8bde-58c6e29ac1e8' + volume_info = {'status': 'error', + 'attach_status': 'detached', + 'id': volume_id} + mock_get.return_value = volume_info + bdms = [objects.BlockDeviceMapping( + **fake_block_device.FakeDbBlockDeviceDict( + { + 'boot_index': 0, + 'volume_id': volume_id, + 'source_type': 'volume', + 'destination_type': 'volume', + 'device_name': 'vda', + }))] + + self.assertRaises(exception.InvalidVolume, + self.compute_api._validate_bdm, + self.context, + instance, instance_type, bdms) + + mock_get.assert_called_once_with(self.context, volume_id) + mock_attach_create.assert_called_once_with( + self.context, volume_id, instance.uuid) + def _test_provision_instances_with_cinder_error(self, expected_exception): @mock.patch('nova.compute.utils.check_num_instances_quota') @@ -3864,6 +4029,8 @@ class _ComputeAPIUnitTestMixIn(object): @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') @mock.patch.object(cinder.API, 'check_availability_zone') @mock.patch.object(cinder.API, 'reserve_volume', @@ -3872,10 +4039,25 @@ class _ComputeAPIUnitTestMixIn(object): mock_cinder_check_av_zone, mock_reserve_volume, mock_get, + mock_get_min_ver_cells, mock_get_min_ver): self._test_provision_instances_with_cinder_error( expected_exception=exception.InvalidVolume) + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(objects.service, 'get_minimum_version_all_cells', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(cinder.API, 'get') + @mock.patch.object(cinder.API, 'check_availability_zone') + @mock.patch.object(cinder.API, 'attachment_create', + side_effect=exception.InvalidInput(reason='error')) + def test_provision_instances_with_error_volume_new_flow(self, + mock_cinder_check_av_zone, mock_attach_create, mock_get, + mock_get_min_ver_cells, mock_get_min_ver): + self._test_provision_instances_with_cinder_error( + expected_exception=exception.InvalidVolume) + @mock.patch('nova.objects.RequestSpec.from_components') @mock.patch('nova.objects.BuildRequest') @mock.patch('nova.objects.Instance') @@ -3924,12 +4106,14 @@ class _ComputeAPIUnitTestMixIn(object): @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_inst_mapping_create, - mock_build_req, mock_req_spec_from_components, - _mock_ensure_default, mock_check_num_inst_quota, - mock_volume, mock_inst_create): + 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): min_count = 1 max_count = 2 @@ -4075,6 +4259,8 @@ class _ComputeAPIUnitTestMixIn(object): @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') @mock.patch.object(cinder.API, 'check_availability_zone',) @mock.patch.object(cinder.API, 'reserve_volume', @@ -4082,7 +4268,108 @@ 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): + _mock_get_min_ver_cells, _mock_get_min_ver): + @mock.patch('nova.compute.utils.check_num_instances_quota') + @mock.patch.object(objects, 'Instance') + @mock.patch.object(self.compute_api.security_group_api, + 'ensure_default') + @mock.patch.object(self.compute_api, '_create_block_device_mapping') + @mock.patch.object(objects.RequestSpec, 'from_components') + @mock.patch.object(objects, 'BuildRequest') + @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): + + min_count = 1 + max_count = 2 + mock_check_num_inst_quota.return_value = 2 + req_spec_mock = mock.MagicMock() + mock_req_spec_from_components.return_value = req_spec_mock + inst_mocks = [mock.MagicMock() for i in range(max_count)] + for inst_mock in inst_mocks: + inst_mock.project_id = 'fake-project' + mock_inst.side_effect = inst_mocks + build_req_mocks = [mock.MagicMock() for i in range(max_count)] + mock_build_req.side_effect = build_req_mocks + inst_map_mocks = [mock.MagicMock() for i in range(max_count)] + mock_inst_mapping.side_effect = inst_map_mocks + + ctxt = context.RequestContext('fake-user', 'fake-project') + flavor = self._create_flavor() + boot_meta = { + 'id': 'fake-image-id', + 'properties': {'mappings': []}, + 'status': 'fake-status', + 'location': 'far-away'} + base_options = {'image_ref': 'fake-ref', + 'display_name': 'fake-name', + 'project_id': 'fake-project', + 'availability_zone': None, + 'metadata': {}, + 'access_ip_v4': None, + 'access_ip_v6': None, + 'config_drive': None, + 'key_name': None, + 'reservation_id': None, + 'kernel_id': None, + 'ramdisk_id': None, + 'root_device_name': None, + 'user_data': None, + 'numa_topology': None, + 'pci_requests': None} + security_groups = {} + block_device_mapping = objects.BlockDeviceMappingList( + objects=[objects.BlockDeviceMapping( + **fake_block_device.FakeDbBlockDeviceDict( + { + 'id': 1, + 'volume_id': 1, + 'source_type': 'volume', + 'destination_type': 'volume', + 'device_name': 'vda', + 'boot_index': 0, + }))]) + shutdown_terminate = True + instance_group = None + check_server_group_quota = False + filter_properties = {'scheduler_hints': None, + 'instance_type': flavor} + tags = objects.TagList() + self.assertRaises(exception.InvalidVolume, + self.compute_api._provision_instances, ctxt, + flavor, min_count, max_count, base_options, + boot_meta, security_groups, block_device_mapping, + shutdown_terminate, instance_group, + 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) + 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) + self.assertFalse(inst_mocks[1].destroy.called) + self.assertFalse(build_req_mocks[1].destroy.called) + self.assertFalse(inst_map_mocks[1].destroy.called) + + do_test() + + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(objects.service, 'get_minimum_version_all_cells', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(cinder.API, 'get') + @mock.patch.object(cinder.API, 'check_availability_zone',) + @mock.patch.object(cinder.API, 'attachment_create', + side_effect=[{'id': uuids.attachment_id}, + exception.InvalidInput(reason='error')]) + @mock.patch.object(objects.BlockDeviceMapping, 'save') + 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('nova.compute.utils.check_num_instances_quota') @mock.patch.object(objects, 'Instance') @mock.patch.object(self.compute_api.security_group_api, @@ -5496,7 +5783,9 @@ class ComputeAPIAPICellUnitTestCase(Cellsv1DeprecatedTestMixIn, self.assertIsNone(result, None) @mock.patch.object(compute_cells_api.ComputeCellsAPI, '_call_to_cells') - def test_attach_volume(self, mock_attach): + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) + def test_attach_volume(self, mock_get_min_ver, mock_attach): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -5513,7 +5802,54 @@ class ComputeAPIAPICellUnitTestCase(Cellsv1DeprecatedTestMixIn, 'attach_volume', volume['id'], None, None, None) - def test_tagged_volume_attach(self): + @mock.patch.object(compute_cells_api.ComputeCellsAPI, '_call_to_cells') + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(cinder, 'is_microversion_supported') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_attach_volume_new_flow(self, mock_no_bdm, + mock_cinder_mv_supported, + mock_get_min_ver, mock_attach): + mock_no_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='test-vol') + instance = self._create_instance_obj() + volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', + None, None, None, None, None) + + mock_volume_api = mock.patch.object(self.compute_api, 'volume_api', + mock.MagicMock(spec=cinder.API)) + + with mock_volume_api as mock_v_api: + mock_v_api.get.return_value = volume + self.compute_api.attach_volume( + self.context, instance, volume['id']) + mock_v_api.check_availability_zone.assert_called_once_with( + self.context, volume, instance=instance) + mock_attach.assert_called_once_with(self.context, instance, + 'attach_volume', volume['id'], + None, None, None) + + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_OLD_ATTACH_FLOW) + def test_tagged_volume_attach(self, mock_get_min_ver): + instance = self._create_instance_obj() + volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', + None, None, None, None, None) + self.assertRaises(exception.VolumeTaggedAttachNotSupported, + self.compute_api.attach_volume, self.context, + instance, volume['id'], tag='foo') + + @mock.patch.object(objects.Service, 'get_minimum_version', + return_value=COMPUTE_VERSION_NEW_ATTACH_FLOW) + @mock.patch.object(cinder, 'is_microversion_supported') + @mock.patch.object(objects.BlockDeviceMapping, + 'get_by_volume_and_instance') + def test_tagged_volume_attach_new_flow(self, mock_no_bdm, + mock_cinder_mv_supported, + mock_get_min_ver): + mock_no_bdm.side_effect = exception.VolumeBDMNotFound( + volume_id='test-vol') instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -5527,6 +5863,9 @@ class ComputeAPIAPICellUnitTestCase(Cellsv1DeprecatedTestMixIn, def test_attach_volume_reserve_fails(self): self.skipTest("Reserve is never done in the API cell.") + def test_attach_volume_attachment_create_fails(self): + self.skipTest("Reserve is never done in the API cell.") + def test_check_requested_networks_no_requested_networks(self): # The API cell just returns the number of instances passed in since the # actual validation happens in the child (compute) cell. diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 189f74d6b3f1..44bce903a63a 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -2049,11 +2049,13 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): @mock.patch('nova.volume.cinder.API.get') @mock.patch('nova.volume.cinder.API.attachment_update') @mock.patch('nova.volume.cinder.API.attachment_delete') + @mock.patch('nova.volume.cinder.API.attachment_complete') @mock.patch('nova.volume.cinder.API.migrate_volume_completion', return_value={'save_volume_id': uuids.old_volume_id}) def test_swap_volume_with_new_attachment_id_cinder_migrate_true( - self, migrate_volume_completion, attachment_delete, - attachment_update, get_volume, get_bdm, notify_about_volume_swap): + self, migrate_volume_completion, attachment_complete, + attachment_delete, attachment_update, get_volume, get_bdm, + notify_about_volume_swap): """Tests a swap volume operation with a new style volume attachment passed in from the compute API, and the case that Cinder initiated the swap volume because of a volume retype situation. This is a happy @@ -2091,6 +2093,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): # We updated the new attachment with the host connector. attachment_update.assert_called_once_with( self.context, uuids.new_attachment_id, mock.sentinel.connector) + # We tell Cinder that the new volume is connected + attachment_complete.assert_called_once_with( + self.context, uuids.new_attachment_id) # After a successful swap volume, we deleted the old attachment. attachment_delete.assert_called_once_with( self.context, uuids.old_attachment_id) @@ -2115,10 +2120,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): @mock.patch('nova.volume.cinder.API.get') @mock.patch('nova.volume.cinder.API.attachment_update') @mock.patch('nova.volume.cinder.API.attachment_delete') + @mock.patch('nova.volume.cinder.API.attachment_complete') @mock.patch('nova.volume.cinder.API.migrate_volume_completion') def test_swap_volume_with_new_attachment_id_cinder_migrate_false( - self, migrate_volume_completion, attachment_delete, - attachment_update, get_volume, get_bdm, notify_about_volume_swap): + self, migrate_volume_completion, attachment_complete, + attachment_delete, attachment_update, get_volume, get_bdm, + notify_about_volume_swap): """Tests a swap volume operation with a new style volume attachment passed in from the compute API, and the case that Cinder did not initiate the swap volume. This is a happy path test. Since it is not a @@ -2156,6 +2163,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): # We updated the new attachment with the host connector. attachment_update.assert_called_once_with( self.context, uuids.new_attachment_id, mock.sentinel.connector) + # We tell Cinder that the new volume is connected + attachment_complete.assert_called_once_with( + self.context, uuids.new_attachment_id) # After a successful swap volume, we deleted the old attachment. attachment_delete.assert_called_once_with( self.context, uuids.old_attachment_id) @@ -4152,8 +4162,13 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): mock_log.assert_not_called() @mock.patch('nova.volume.cinder.API.attachment_delete') + @mock.patch('nova.volume.cinder.API.attachment_create', + return_value={'id': uuids.attachment_id}) + @mock.patch('nova.objects.BlockDeviceMapping.save') @mock.patch('nova.volume.cinder.API.terminate_connection') def test_terminate_volume_connections(self, mock_term_conn, + mock_bdm_save, + mock_attach_create, mock_attach_delete): """Tests _terminate_volume_connections with cinder v2 style, cinder v3.44 style, and non-volume BDMs. @@ -4174,21 +4189,25 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): objects.BlockDeviceMapping(volume_id=None, destination_type='local') ]) + instance = fake_instance.fake_instance_obj( + self.context, vm_state=vm_states.ACTIVE) fake_connector = mock.sentinel.fake_connector with mock.patch.object(self.compute.driver, 'get_volume_connector', return_value=fake_connector) as connector_mock: self.compute._terminate_volume_connections( - self.context, mock.sentinel.instance, bdms) + self.context, instance, bdms) # assert we called terminate_connection twice (once per old volume bdm) mock_term_conn.assert_has_calls([ mock.call(self.context, uuids.v2_volume_id_1, fake_connector), mock.call(self.context, uuids.v2_volume_id_2, fake_connector) ]) # assert we only build the connector once - connector_mock.assert_called_once_with(mock.sentinel.instance) + connector_mock.assert_called_once_with(instance) # assert we called delete_attachment once for the single new volume bdm mock_attach_delete.assert_called_once_with( self.context, uuids.attach_id) + mock_attach_create.assert_called_once_with( + self.context, uuids.v3_volume_id, instance.uuid) def test_instance_soft_delete_notification(self): inst_obj = fake_instance.fake_instance_obj(self.context, diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 7d054ae302d4..0ae2a3d7fe4e 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -79,6 +79,38 @@ def _check_microversion(url, microversion): raise exception.CinderAPIVersionNotAvailable(version=microversion) +def _get_cinderclient_parameters(context): + global _SESSION + + if not _SESSION: + _SESSION = ks_loading.load_session_from_conf_options( + CONF, nova.conf.cinder.cinder_group.name) + + url = None + + auth = service_auth.get_auth_plugin(context) + service_type, service_name, interface = CONF.cinder.catalog_info.split(':') + + service_parameters = {'service_type': service_type, + 'service_name': service_name, + 'interface': interface, + 'region_name': CONF.cinder.os_region_name} + + if CONF.cinder.endpoint_template: + url = CONF.cinder.endpoint_template % context.to_dict() + else: + url = _SESSION.get_endpoint(auth, **service_parameters) + + return auth, service_parameters, url + + +def is_microversion_supported(context, microversion): + + _, _, url = _get_cinderclient_parameters(context) + + _check_microversion(url, microversion) + + def cinderclient(context, microversion=None, skip_version_check=False): """Constructs a cinder client object for making API requests. @@ -92,28 +124,12 @@ def cinderclient(context, microversion=None, skip_version_check=False): is used directly. This should only be used if a previous check for the same microversion was successful. """ - global _SESSION - if not _SESSION: - _SESSION = ks_loading.load_session_from_conf_options( - CONF, nova.conf.cinder.cinder_group.name) - - url = None endpoint_override = None - - auth = service_auth.get_auth_plugin(context) - service_type, service_name, interface = CONF.cinder.catalog_info.split(':') - - service_parameters = {'service_type': service_type, - 'service_name': service_name, - 'interface': interface, - 'region_name': CONF.cinder.os_region_name} + auth, service_parameters, url = _get_cinderclient_parameters(context) if CONF.cinder.endpoint_template: - url = CONF.cinder.endpoint_template % context.to_dict() endpoint_override = url - else: - url = _SESSION.get_endpoint(auth, **service_parameters) # TODO(jamielennox): This should be using proper version discovery from # the cinder service rather than just inspecting the URL for certain string diff --git a/releasenotes/notes/bp-cinder-new-attach-apis-eca854e27a255e3e.yaml b/releasenotes/notes/bp-cinder-new-attach-apis-eca854e27a255e3e.yaml new file mode 100644 index 000000000000..ab5f3f6ec4e2 --- /dev/null +++ b/releasenotes/notes/bp-cinder-new-attach-apis-eca854e27a255e3e.yaml @@ -0,0 +1,25 @@ +--- +other: + - | + Nova will now internally use a new flow for new volume attachments when: + + * All *nova-compute* services are upgraded + * The *block-storage* 3.44 API microversion is available + + This change should be transparent to end users and does not affect existing + volume attachments. Also, this does not affect how new volumes are created + and attached during boot-from-volume when the + ``block_device_mapping_v2.source_type`` is ``blank``, ``image`` or + ``snapshot`` and the ``block_device_mapping_v2.destination_type`` is + ``volume``. + + The motivation behind these changes are: + + * Track volume attachment state in the block storage service rather than + the compute service (separation of duties, simplicity, etc) + * Removal of technical debt from the compute service long-term + * Enable a foundation on which to build support for multi-attach volumes + + More details can be found in the spec: + + https://specs.openstack.org/openstack/nova-specs/specs/queens/approved/cinder-new-attach-apis.html \ No newline at end of file