From 1957f4d5a291dea4e15439a06c424e46605d5377 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Thu, 21 Dec 2017 15:44:38 -0500 Subject: [PATCH] Use volume shared_targets to lock during attach/detach The Cinder 3.48 API provides a shared_targets and service_uuid field in the volume resource which tells the caller whether or not it should lock operations on that volume. This change adds that logic to the DriverVolumeBlockDevice attach and detach flows by first trying to get the volume at the 3.48 microversion and if that's not available, it simply falls back to get the volume the same as before. If 3.48 is available and the volume storage backend uses shared targets, we synchronize the attach/detach operations using the volume service_uuid, which is based on the storage backend and is also configurable by the deployer on the Cinder side. This is a nice to have protection for "normal" volumes but is really needed for multiattach volumes to be detached safely. See Cinder blueprint add-shared-targets-flag-to-volume for details. Part of blueprint multi-attach-volume Depends-On: I3c07cd8458d55535a71626ffaa8ca50deb3ca3dd Change-Id: I5e96602184242fb9017c0434b445a3875f3b149a --- nova/tests/fixtures.py | 4 +- nova/tests/unit/compute/test_compute.py | 9 +-- nova/tests/unit/virt/test_block_device.py | 46 +++++++++++--- nova/virt/block_device.py | 74 ++++++++++++++++++++--- 4 files changed, 111 insertions(+), 22 deletions(-) diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 204a8894ef7a..380704a6cdf3 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -1325,7 +1325,7 @@ class CinderFixture(fixtures.Fixture): def setUp(self): super(CinderFixture, self).setUp() - def fake_get(self_api, context, volume_id): + def fake_get(self_api, context, volume_id, microversion=None): # Check for the special swap volumes. if volume_id in (CinderFixture.SWAP_OLD_VOL, CinderFixture.SWAP_ERR_OLD_VOL): @@ -1499,7 +1499,7 @@ class CinderFixtureNewAttachFlow(fixtures.Fixture): def setUp(self): super(CinderFixtureNewAttachFlow, self).setUp() - def fake_get(self_api, context, volume_id): + def fake_get(self_api, context, volume_id, microversion=None): # Check for the special swap volumes. if volume_id in (CinderFixture.SWAP_OLD_VOL, CinderFixture.SWAP_ERR_OLD_VOL): diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 44b795c826ea..66084d604900 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -4828,7 +4828,7 @@ class ComputeTestCase(BaseTestCase, bdm.create() # stub out volume attach - def fake_volume_get(self, context, volume_id): + def fake_volume_get(self, context, volume_id, microversion=None): return volume self.stub_out('nova.volume.cinder.API.get', fake_volume_get) @@ -10569,7 +10569,8 @@ class ComputeAPITestCase(BaseTestCase): @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume_and_instance') - def test_detach_volume_libvirt_is_down(self, mock_get): + @mock.patch.object(cinder.API, 'get', return_value={'id': uuids.volume_id}) + def test_detach_volume_libvirt_is_down(self, mock_get_vol, mock_get): # Ensure rollback during detach if libvirt goes down called = {} instance = self._create_fake_instance_obj() @@ -10618,7 +10619,7 @@ class ComputeAPITestCase(BaseTestCase): # Stub out fake_volume_get so cinder api does not raise exception # and manager gets to call bdm.destroy() - def fake_volume_get(self, context, volume_id): + def fake_volume_get(self, context, volume_id, microversion=None): return {'id': volume_id} self.stub_out('nova.volume.cinder.API.get', fake_volume_get) @@ -12494,7 +12495,7 @@ class EvacuateHostTestCase(BaseTestCase): db.block_device_mapping_create(self.context, values) - def fake_volume_get(self, context, volume): + def fake_volume_get(self, context, volume, microversion=None): return {'id': 'fake_volume_id'} self.stub_out("nova.volume.cinder.API.get", fake_volume_get) diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index 7f774a933c2f..cf14de3267a7 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -395,7 +395,7 @@ class TestDriverBlockDevice(test.NoDBTestCase): def test_call_wait_no_delete_volume(self): self._test_call_wait_func(False) - def test_volume_delete_attachment(self): + def test_volume_delete_attachment(self, include_shared_targets=False): attachment_id = uuids.attachment driver_bdm = self.driver_classes['volume'](self.volume_bdm) driver_bdm['attachment_id'] = attachment_id @@ -406,25 +406,40 @@ class TestDriverBlockDevice(test.NoDBTestCase): instance = fake_instance.fake_instance_obj(self.context, **instance_detail) connector = {'ip': 'fake_ip', 'host': 'fake_host'} + volume = {'id': driver_bdm.volume_id, + 'attach_status': 'attached', + 'status': 'in-use'} + if include_shared_targets: + volume['shared_targets'] = True + volume['service_uuid'] = uuids.service_uuid with test.nested( + mock.patch.object(driver_bdm, '_get_volume', return_value=volume), mock.patch.object(self.virt_driver, 'get_volume_connector', return_value=connector), + mock.patch('nova.utils.synchronized', + side_effect=lambda a: lambda f: lambda *args: f(*args)), mock.patch.object(self.volume_api, 'attachment_delete'), - ) as (_, vapi_attach_del): + ) as (mock_get_volume, mock_get_connector, mock_sync, vapi_attach_del): driver_bdm.detach(elevated_context, instance, self.volume_api, self.virt_driver, attachment_id=attachment_id) + if include_shared_targets: + mock_sync.assert_called_once_with((uuids.service_uuid)) vapi_attach_del.assert_called_once_with(elevated_context, attachment_id) + def test_volume_delete_attachment_with_shared_targets(self): + self.test_volume_delete_attachment(include_shared_targets=True) + def _test_volume_attach(self, driver_bdm, bdm_dict, fake_volume, fail_check_av_zone=False, driver_attach=False, fail_driver_attach=False, volume_attach=True, fail_volume_attach=False, - access_mode='rw', availability_zone=None): + access_mode='rw', availability_zone=None, + include_shared_targets=False): elevated_context = self.context.elevated() self.stubs.Set(self.context, 'elevated', lambda: elevated_context) @@ -440,8 +455,21 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'serial': fake_volume['id']} enc_data = {'fake': 'enc_data'} - self.volume_api.get(self.context, - fake_volume['id']).AndReturn(fake_volume) + if include_shared_targets: + fake_volume['shared_targets'] = True + fake_volume['service_uuid'] = uuids.service_uuid + self.volume_api.get( + self.context, fake_volume['id'], + microversion='3.48').AndReturn(fake_volume) + else: + # First call to get() fails because the API isn't new enough. + self.volume_api.get( + self.context, fake_volume['id'], microversion='3.48').AndRaise( + exception.CinderAPIVersionNotAvailable(version='3.48')) + # So we fallback to the old call. + self.volume_api.get(self.context, + fake_volume['id']).AndReturn(fake_volume) + if not fail_check_av_zone: self.volume_api.check_availability_zone(self.context, fake_volume, @@ -533,14 +561,15 @@ class TestDriverBlockDevice(test.NoDBTestCase): driver_bdm._bdm_obj.save().AndReturn(None) return instance, expected_conn_info - def test_volume_attach(self): + def test_volume_attach(self, include_shared_targets=False): test_bdm = self.driver_classes['volume']( self.volume_bdm) volume = {'id': 'fake-volume-id-1', 'attach_status': 'detached'} instance, expected_conn_info = self._test_volume_attach( - test_bdm, self.volume_bdm, volume) + test_bdm, self.volume_bdm, volume, + include_shared_targets=include_shared_targets) self.mox.ReplayAll() @@ -549,6 +578,9 @@ class TestDriverBlockDevice(test.NoDBTestCase): self.assertThat(test_bdm['connection_info'], matchers.DictMatches(expected_conn_info)) + def test_volume_attach_with_shared_targets(self): + self.test_volume_attach(include_shared_targets=True) + def test_volume_attach_ro(self): test_bdm = self.driver_classes['volume'](self.volume_bdm) volume = {'id': 'fake-volume-id-1', diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index 147d787c3dc6..21bb81c48de2 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -24,6 +24,7 @@ from oslo_utils import excutils from nova import block_device import nova.conf from nova import exception +from nova import utils CONF = nova.conf.CONF @@ -289,9 +290,23 @@ class DriverVolumeBlockDevice(DriverBlockDevice): instance=instance) volume_api.roll_detaching(context, volume_id) - def detach(self, context, instance, volume_api, virt_driver, - attachment_id=None, destroy_bdm=False): + @staticmethod + def _get_volume(context, volume_api, volume_id): + # First try to get the volume at microversion 3.48 so we can get the + # shared_targets parameter exposed in that version. If that API version + # is not available, we just fallback. + try: + return volume_api.get(context, volume_id, microversion='3.48') + except exception.CinderAPIVersionNotAvailable: + return volume_api.get(context, volume_id) + def _do_detach(self, context, instance, volume_api, virt_driver, + attachment_id=None, destroy_bdm=False): + """Private method that actually does the detach. + + This is separate from the detach() method so the caller can optionally + lock this call. + """ volume_id = self.volume_id # Only attempt to detach and disconnect from the volume if the instance @@ -358,6 +373,25 @@ class DriverVolumeBlockDevice(DriverBlockDevice): else: volume_api.attachment_delete(context, self['attachment_id']) + def detach(self, context, instance, volume_api, virt_driver, + attachment_id=None, destroy_bdm=False): + volume = self._get_volume(context, volume_api, self.volume_id) + # Check to see if we need to lock based on the shared_targets value. + # Default to False if the volume does not expose that value to maintain + # legacy behavior. + if volume.get('shared_targets', False): + # Lock the detach call using the provided service_uuid. + @utils.synchronized(volume['service_uuid']) + def _do_locked_detach(*args, **_kwargs): + self._do_detach(*args, **_kwargs) + + _do_locked_detach(context, instance, volume_api, virt_driver, + attachment_id, destroy_bdm) + else: + # We don't need to (or don't know if we need to) lock. + self._do_detach(context, instance, volume_api, virt_driver, + attachment_id, destroy_bdm) + def _legacy_volume_attach(self, context, volume, connector, instance, volume_api, virt_driver, do_driver_attach=False): @@ -500,15 +534,15 @@ class DriverVolumeBlockDevice(DriverBlockDevice): # Delete the attachment to mark the volume as "available". volume_api.attachment_delete(context, self['attachment_id']) - @update_db - def attach(self, context, instance, volume_api, virt_driver, - do_driver_attach=False, **kwargs): - volume = volume_api.get(context, self.volume_id) - volume_api.check_availability_zone(context, volume, - instance=instance) + def _do_attach(self, context, instance, volume, volume_api, virt_driver, + do_driver_attach): + """Private method that actually does the attach. + + This is separate from the attach() method so the caller can optionally + lock this call. + """ context = context.elevated() connector = virt_driver.get_volume_connector(instance) - if not self['attachment_id']: self._legacy_volume_attach(context, volume, connector, instance, volume_api, virt_driver, @@ -519,6 +553,28 @@ class DriverVolumeBlockDevice(DriverBlockDevice): self['attachment_id'], do_driver_attach) + @update_db + def attach(self, context, instance, volume_api, virt_driver, + do_driver_attach=False, **kwargs): + volume = self._get_volume(context, volume_api, self.volume_id) + volume_api.check_availability_zone(context, volume, + instance=instance) + # Check to see if we need to lock based on the shared_targets value. + # Default to False if the volume does not expose that value to maintain + # legacy behavior. + if volume.get('shared_targets', False): + # Lock the attach call using the provided service_uuid. + @utils.synchronized(volume['service_uuid']) + def _do_locked_attach(*args, **_kwargs): + self._do_attach(*args, **_kwargs) + + _do_locked_attach(context, instance, volume, volume_api, + virt_driver, do_driver_attach) + else: + # We don't need to (or don't know if we need to) lock. + self._do_attach(context, instance, volume, volume_api, + virt_driver, do_driver_attach) + @update_db def refresh_connection_info(self, context, instance, volume_api, virt_driver):