From 6d70d6adf9c902f0a9f1735ef90d992cbc0dcb46 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Fri, 21 Jul 2017 13:31:57 +0200 Subject: [PATCH] Implement new attach Cinder flow This change integrates support of the Cinder 3.44 volume attachment API. The patch bumps the compute service version to check whether all the compute nodes are upgraded to the version that can handle attach and detach with the new flow. To enable the new flow we also need the 3.44 or higher microversion from Cinder. We check that in the API and if it's not available we fall back to the old attach/detach flow. Co-Authored-By: Ildiko Vancsa Partially Implements: blueprint cinder-new-attach-apis Change-Id: Ifc01dbf98545104c998ab96f65ff8623a6db0f28 --- nova/compute/api.py | 91 ++++- nova/compute/manager.py | 37 +- nova/objects/service.py | 4 +- nova/tests/fixtures.py | 174 +++++++++ .../api_sample_tests/test_volumes.py | 72 +++- .../test_instance.py | 78 +++- .../regressions/test_bug_1675570.py | 19 +- .../regressions/test_bug_1732947.py | 2 +- nova/tests/functional/test_servers.py | 75 +++- nova/tests/unit/compute/test_compute.py | 284 +++++++++++++- nova/tests/unit/compute/test_compute_api.py | 363 +++++++++++++++++- nova/tests/unit/compute/test_compute_mgr.py | 31 +- nova/volume/cinder.py | 50 ++- ...nder-new-attach-apis-eca854e27a255e3e.yaml | 25 ++ 14 files changed, 1231 insertions(+), 74 deletions(-) create mode 100644 releasenotes/notes/bp-cinder-new-attach-apis-eca854e27a255e3e.yaml 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