From cb3a6bfe1abde63c7e20524ed99b569d4a68d35e Mon Sep 17 00:00:00 2001 From: Krisztian Gacsal Date: Thu, 18 Jun 2015 15:08:20 +0200 Subject: [PATCH] Pass attachment_id to Cinder when detach a volume Cinder needs the attachment_id to properly identify which attachment of a volume to detach. This patch adapts the Cinder driver to pass the required information. The attachment_id is necessary for volumes that enable multiple attachments in order to terminate the connection properly. The attachment_id is retrieved in the API layer where available and sent through RPC, the RPC version is bumped to 4.7. Also the translation functions are modified to retrieve all necessary information for the volume info coming from Cinder including the multiple attachments. The Nova API returns the volume info in the same format as earlier. Co-Authored-By: Ildiko Vancsa Partially-implements: blueprint multi-attach-volume Change-Id: I3cdc49924acbdd21d1e6678a3bb4cf7de7f1db1a --- .../compute/legacy_v2/contrib/volumes.py | 16 ++++- nova/api/openstack/compute/volumes.py | 16 ++++- nova/compute/api.py | 23 +++--- nova/compute/cells_api.py | 2 +- nova/compute/manager.py | 15 ++-- nova/compute/rpcapi.py | 11 ++- nova/objects/service.py | 4 +- .../api_sample_tests/test_volumes.py | 11 ++- .../api/openstack/compute/test_volumes.py | 5 +- nova/tests/unit/api/openstack/fakes.py | 9 ++- nova/tests/unit/compute/test_compute.py | 71 ++++++++++++++----- nova/tests/unit/compute/test_compute_api.py | 19 +++-- nova/tests/unit/compute/test_compute_mgr.py | 12 ++-- nova/tests/unit/compute/test_rpcapi.py | 26 ++++++- nova/tests/unit/fake_volume.py | 22 ++++-- nova/tests/unit/test_cinder.py | 68 ++++++++++++++++-- nova/tests/unit/volume/test_cinder.py | 39 ++++++++-- nova/volume/cinder.py | 57 +++++++++++++-- 18 files changed, 340 insertions(+), 86 deletions(-) diff --git a/nova/api/openstack/compute/legacy_v2/contrib/volumes.py b/nova/api/openstack/compute/legacy_v2/contrib/volumes.py index 52e46738f6e8..6b0e9966e2b5 100644 --- a/nova/api/openstack/compute/legacy_v2/contrib/volumes.py +++ b/nova/api/openstack/compute/legacy_v2/contrib/volumes.py @@ -59,9 +59,21 @@ def _translate_volume_summary_view(context, vol): d['createdAt'] = vol['created_at'] if vol['attach_status'] == 'attached': + # NOTE(ildikov): The attachments field in the volume info that + # Cinder sends is converted to an OrderedDict with the + # instance_uuid as key to make it easier for the multiattach + # feature to check the required information. Multiattach will + # be enable in the Nova API in Newton. + # The format looks like the following: + # attachments = {'instance_uuid': { + # 'attachment_id': 'attachment_uuid', + # 'mountpoint': '/dev/sda/ + # } + # } + attachment = vol['attachments'].items()[0] d['attachments'] = [_translate_attachment_detail_view(vol['id'], - vol['instance_uuid'], - vol['mountpoint'])] + attachment[0], + attachment[1].get('mountpoint'))] else: d['attachments'] = [{}] diff --git a/nova/api/openstack/compute/volumes.py b/nova/api/openstack/compute/volumes.py index 3db1106e1f1d..2d5e41774d6d 100644 --- a/nova/api/openstack/compute/volumes.py +++ b/nova/api/openstack/compute/volumes.py @@ -55,9 +55,21 @@ def _translate_volume_summary_view(context, vol): d['createdAt'] = vol['created_at'] if vol['attach_status'] == 'attached': + # NOTE(ildikov): The attachments field in the volume info that + # Cinder sends is converted to an OrderedDict with the + # instance_uuid as key to make it easier for the multiattach + # feature to check the required information. Multiattach will + # be enable in the Nova API in Newton. + # The format looks like the following: + # attachments = {'instance_uuid': { + # 'attachment_id': 'attachment_uuid', + # 'mountpoint': '/dev/sda/ + # } + # } + attachment = vol['attachments'].items()[0] d['attachments'] = [_translate_attachment_detail_view(vol['id'], - vol['instance_uuid'], - vol['mountpoint'])] + attachment[0], + attachment[1].get('mountpoint'))] else: d['attachments'] = [{}] diff --git a/nova/compute/api.py b/nova/compute/api.py index acdce3c33749..b7b0a0dba4a2 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1764,7 +1764,8 @@ class API(base.Base): self.volume_api.terminate_connection(context, bdm.volume_id, connector) - self.volume_api.detach(elevated, bdm.volume_id) + self.volume_api.detach(elevated, bdm.volume_id, + instance.uuid) if bdm.delete_on_termination: self.volume_api.delete(context, bdm.volume_id) except Exception as exc: @@ -3016,10 +3017,14 @@ class API(base.Base): This method is separated to make it easier for cells version to override. """ - self.volume_api.check_detach(context, volume) + self.volume_api.check_detach(context, volume, instance=instance) self.volume_api.begin_detaching(context, volume['id']) + attachments = volume.get('attachments', {}) + attachment_id = None + if attachments and instance.uuid in attachments: + attachment_id = attachments[instance.uuid]['attachment_id'] self.compute_rpcapi.detach_volume(context, instance=instance, - volume_id=volume['id']) + volume_id=volume['id'], attachment_id=attachment_id) @wrap_check_policy @check_instance_lock @@ -3028,13 +3033,7 @@ class API(base.Base): vm_states.SOFT_DELETED]) def detach_volume(self, context, instance, volume): """Detach a volume from an instance.""" - if volume['attach_status'] == 'detached': - msg = _("Volume must be attached in order to detach.") - raise exception.InvalidVolume(reason=msg) - # The caller likely got the instance from volume['instance_uuid'] - # in the first place, but let's sanity check. - if volume['instance_uuid'] != instance.uuid: - raise exception.VolumeUnattached(volume_id=volume['id']) + self._detach_volume(context, instance, volume) @wrap_check_policy @@ -3046,9 +3045,9 @@ class API(base.Base): """Swap volume attached to an instance.""" if old_volume['attach_status'] == 'detached': raise exception.VolumeUnattached(volume_id=old_volume['id']) - # The caller likely got the instance from volume['instance_uuid'] + # The caller likely got the instance from volume['attachments'] # in the first place, but let's sanity check. - if old_volume['instance_uuid'] != instance.uuid: + if not old_volume.get('attachments', {}).get(instance.uuid): msg = _("Old volume is attached to a different instance.") raise exception.InvalidVolume(reason=msg) if new_volume['attach_status'] == 'attached': diff --git a/nova/compute/cells_api.py b/nova/compute/cells_api.py index 7381a14f8b83..50ad4826bce9 100644 --- a/nova/compute/cells_api.py +++ b/nova/compute/cells_api.py @@ -429,7 +429,7 @@ class ComputeCellsAPI(compute_api.API): @check_instance_cell def _detach_volume(self, context, instance, volume): """Detach a volume from an instance.""" - self.volume_api.check_detach(context, volume) + self.volume_api.check_detach(context, volume, instance=instance) self._cast_to_cells(context, instance, 'detach_volume', volume) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 5b205821a581..cffb6dfc06aa 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -674,7 +674,7 @@ class ComputeVirtAPI(virtapi.VirtAPI): class ComputeManager(manager.Manager): """Manages the running instances from creation to destruction.""" - target = messaging.Target(version='4.6') + target = messaging.Target(version='4.7') # How long to wait in seconds before re-issuing a shutdown # signal to an instance during power off. The overall @@ -2326,7 +2326,7 @@ class ComputeManager(manager.Manager): self.volume_api.terminate_connection(context, bdm.volume_id, connector) - self.volume_api.detach(context, bdm.volume_id) + self.volume_api.detach(context, bdm.volume_id, instance.uuid) except exception.DiskNotFound as exc: LOG.debug('Ignoring DiskNotFound: %s', exc, instance=instance) @@ -4737,7 +4737,8 @@ class ComputeManager(manager.Manager): context=context, instance=instance) self.volume_api.roll_detaching(context, volume_id) - def _detach_volume(self, context, volume_id, instance, destroy_bdm=True): + def _detach_volume(self, context, volume_id, instance, destroy_bdm=True, + attachment_id=None): """Detach a volume from an instance. :param context: security context @@ -4790,14 +4791,16 @@ class ComputeManager(manager.Manager): info = dict(volume_id=volume_id) self._notify_about_instance_usage( context, instance, "volume.detach", extra_usage_info=info) - self.volume_api.detach(context.elevated(), volume_id) + self.volume_api.detach(context.elevated(), volume_id, instance.uuid, + attachment_id) @wrap_exception() @wrap_instance_fault - def detach_volume(self, context, volume_id, instance): + def detach_volume(self, context, volume_id, instance, attachment_id=None): """Detach a volume from an instance.""" - self._detach_volume(context, volume_id, instance) + self._detach_volume(context, volume_id, instance, + attachment_id=attachment_id) def _init_volume_connection(self, context, new_volume_id, old_volume_id, connector, instance, bdm): diff --git a/nova/compute/rpcapi.py b/nova/compute/rpcapi.py index 0260fc9b3d06..b9cadb512511 100644 --- a/nova/compute/rpcapi.py +++ b/nova/compute/rpcapi.py @@ -319,6 +319,7 @@ class ComputeAPI(object): * ... - Remove refresh_security_group_members() * ... - Remove refresh_security_group_rules() * 4.6 - Add trigger_crash_dump() + * 4.7 - Add attachment_id argument to detach_volume() ''' VERSION_ALIASES = { @@ -467,12 +468,16 @@ class ComputeAPI(object): cctxt.cast(ctxt, 'detach_interface', instance=instance, port_id=port_id) - def detach_volume(self, ctxt, instance, volume_id): - version = '4.0' + def detach_volume(self, ctxt, instance, volume_id, attachment_id=None): + extra = {'attachment_id': attachment_id} + version = '4.7' + if not self.client.can_send_version(version): + version = '4.0' + extra.pop('attachment_id') cctxt = self.client.prepare(server=_compute_host(None, instance), version=version) cctxt.cast(ctxt, 'detach_volume', - instance=instance, volume_id=volume_id) + instance=instance, volume_id=volume_id, **extra) def finish_resize(self, ctxt, instance, migration, image, disk_info, host, reservations=None): diff --git a/nova/objects/service.py b/nova/objects/service.py index 6ea5cc8f58c1..e24e54f4a584 100644 --- a/nova/objects/service.py +++ b/nova/objects/service.py @@ -28,7 +28,7 @@ LOG = logging.getLogger(__name__) # NOTE(danms): This is the global service version counter -SERVICE_VERSION = 4 +SERVICE_VERSION = 5 # NOTE(danms): This is our SERVICE_VERSION history. The idea is that any @@ -60,6 +60,8 @@ SERVICE_VERSION_HISTORY = ( {'compute_rpc': '4.6'}, # Version 4: Add PciDevice.parent_addr (data migration needed) {'compute_rpc': '4.6'}, + # Version 5: Add attachment_id kwarg to detach_volume() + {'compute_rpc': '4.7'}, ) diff --git a/nova/tests/functional/api_sample_tests/test_volumes.py b/nova/tests/functional/api_sample_tests/test_volumes.py index 26a33db1f79c..8eba7b643971 100644 --- a/nova/tests/functional/api_sample_tests/test_volumes.py +++ b/nova/tests/functional/api_sample_tests/test_volumes.py @@ -111,8 +111,6 @@ class VolumesSampleJsonTest(test_servers.ServersSampleBase): 'id': id, 'size': size, 'availability_zone': 'zone1:host1', - 'instance_uuid': '3912f2b4-c5ba-4aec-9165-872876fe202e', - 'mountpoint': '/', 'status': 'in-use', 'attach_status': 'attached', 'name': 'vol name', @@ -122,7 +120,14 @@ class VolumesSampleJsonTest(test_servers.ServersSampleBase): 'snapshot_id': None, 'volume_type_id': 'fakevoltype', 'volume_metadata': [], - 'volume_type': {'name': 'Backup'} + 'volume_type': {'name': 'Backup'}, + 'multiattach': False, + 'attachments': {'3912f2b4-c5ba-4aec-9165-872876fe202e': + {'mountpoint': '/', + 'attachment_id': + 'a26887c6-c47b-4654-abb5-dfadf7d3f803' + } + } } return volume diff --git a/nova/tests/unit/api/openstack/compute/test_volumes.py b/nova/tests/unit/api/openstack/compute/test_volumes.py index 19da82b324f5..0672e88991a7 100644 --- a/nova/tests/unit/api/openstack/compute/test_volumes.py +++ b/nova/tests/unit/api/openstack/compute/test_volumes.py @@ -60,7 +60,10 @@ def fake_get_instance(self, context, instance_id, want_objects=False, def fake_get_volume(self, context, id): - return {'id': 'woot'} + return {'id': FAKE_UUID_A, + 'status': 'available', + 'attach_status': 'detached' + } def fake_attach_volume(self, context, instance, volume_id, device): diff --git a/nova/tests/unit/api/openstack/fakes.py b/nova/tests/unit/api/openstack/fakes.py index e7dabc08b522..549edb8f9b30 100644 --- a/nova/tests/unit/api/openstack/fakes.py +++ b/nova/tests/unit/api/openstack/fakes.py @@ -581,8 +581,6 @@ def stub_volume(id, **kwargs): 'host': 'fakehost', 'size': 1, 'availability_zone': 'fakeaz', - 'instance_uuid': 'fakeuuid', - 'mountpoint': '/', 'status': 'fakestatus', 'attach_status': 'attached', 'name': 'vol name', @@ -592,7 +590,12 @@ def stub_volume(id, **kwargs): 'snapshot_id': None, 'volume_type_id': 'fakevoltype', 'volume_metadata': [], - 'volume_type': {'name': 'vol_type_name'}} + 'volume_type': {'name': 'vol_type_name'}, + 'multiattach': True, + 'attachments': {'fakeuuid': {'mountpoint': '/'}, + 'fakeuuid2': {'mountpoint': '/dev/sdb'} + } + } volume.update(kwargs) return volume diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 04fae9fb46ea..adbf9669ecee 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -367,7 +367,7 @@ class ComputeVolumeTestCase(BaseTestCase): self.context, objects.Instance(), fake_instance.fake_db_instance()) self.stubs.Set(self.compute.volume_api, 'get', lambda *a, **kw: - {'id': 'fake', 'size': 4, + {'id': uuids.volume_id, 'size': 4, 'attach_status': 'detached'}) self.stubs.Set(self.compute.driver, 'get_volume_connector', lambda *a, **kw: None) @@ -436,7 +436,7 @@ class ComputeVolumeTestCase(BaseTestCase): mock_get.return_value = fake_bdm self.assertRaises( test.TestingException, self.compute.detach_volume, - self.context, 'fake', instance) + self.context, 'fake', instance, 'fake_id') mock_internal_detach.assert_called_once_with(self.context, instance, fake_bdm) @@ -748,6 +748,7 @@ class ComputeVolumeTestCase(BaseTestCase): self.mox.StubOutWithMock(self.compute.driver, 'block_stats') self.mox.StubOutWithMock(self.compute, '_get_host_volume_bdms') self.mox.StubOutWithMock(self.compute.driver, 'get_all_volume_usage') + self.mox.StubOutWithMock(self.compute.driver, 'instance_exists') # The following methods will be called objects.BlockDeviceMapping.get_by_volume_and_instance( @@ -767,6 +768,8 @@ class ComputeVolumeTestCase(BaseTestCase): 'wr_bytes': 5, 'instance': instance}]) + self.compute.driver.instance_exists(mox.IgnoreArg()).AndReturn(True) + self.mox.ReplayAll() def fake_get_volume_encryption_metadata(self, context, volume_id): @@ -1081,10 +1084,19 @@ class ComputeVolumeTestCase(BaseTestCase): ) for status, attach_status in status_values: - def fake_volume_get(self, ctxt, volume_id): - return {'id': volume_id, - 'status': status, - 'attach_status': attach_status} + if attach_status == 'attached': + def fake_volume_get(self, ctxt, volume_id): + return {'id': volume_id, + 'status': status, + 'attach_status': attach_status, + 'multiattach': False, + 'attachments': {}} + else: + def fake_volume_get(self, ctxt, volume_id): + return {'id': volume_id, + 'status': status, + 'attach_status': attach_status, + 'multiattach': False} self.stubs.Set(cinder.API, 'get', fake_volume_get) self.assertRaises(exception.InvalidVolume, self.compute_api._validate_bdm, @@ -1106,7 +1118,8 @@ class ComputeVolumeTestCase(BaseTestCase): def fake_volume_get_ok(self, context, volume_id): return {'id': volume_id, 'status': 'available', - 'attach_status': 'detached'} + 'attach_status': 'detached', + 'multiattach': False} self.stubs.Set(cinder.API, 'get', fake_volume_get_ok) self.compute_api._validate_bdm(self.context, self.instance, @@ -1873,12 +1886,18 @@ class ComputeTestCase(BaseTestCase): pass def fake_volume_get(self, context, volume_id): - return {'id': volume_id} + return {'id': volume_id, + 'attach_status': 'attached', + 'attachments': {instance.uuid: { + 'attachment_id': 'abc123' + } + } + } def fake_terminate_connection(self, context, volume_id, connector): pass - def fake_detach(self, context, volume_id): + def fake_detach(self, context, volume_id, instance_uuid): pass bdms = [] @@ -9670,8 +9689,7 @@ class ComputeAPITestCase(BaseTestCase): called = {} instance = self._create_fake_instance_obj() # Set attach_status to 'fake' as nothing is reading the value. - volume = {'id': 1, 'attach_status': 'fake', - 'instance_uuid': instance['uuid']} + volume = {'id': 1, 'attach_status': 'fake'} def fake_check_detach(*args, **kwargs): called['fake_check_detach'] = True @@ -9701,7 +9719,7 @@ class ComputeAPITestCase(BaseTestCase): 'launched_at': timeutils.utcnow(), 'vm_state': vm_states.ACTIVE, 'task_state': None}) - volume = {'id': 1, 'attach_status': 'detached'} + volume = {'id': 1, 'attach_status': 'detached', 'status': 'available'} self.assertRaises(exception.InvalidVolume, self.compute_api.detach_volume, self.context, @@ -9716,8 +9734,8 @@ class ComputeAPITestCase(BaseTestCase): 'launched_at': timeutils.utcnow(), 'vm_state': vm_states.ACTIVE, 'task_state': None}) - volume = {'id': 1, 'attach_status': 'attached', - 'instance_uuid': 'f7000000-0000-0000-0000-000000000002'} + volume = {'id': 1, 'attach_status': 'attached', 'status': 'in-use', + 'attachments': {'fake_uuid': {'attachment_id': 'fakeid'}}} self.assertRaises(exception.VolumeUnattached, self.compute_api.detach_volume, self.context, @@ -9789,6 +9807,12 @@ class ComputeAPITestCase(BaseTestCase): 'connection_info': '{"test": "test"}'}) bdm = objects.BlockDeviceMapping(context=self.context, **fake_bdm) + # 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): + return {'id': volume_id} + self.stub_out('nova.volume.cinder.API.get', fake_volume_get) + with test.nested( mock.patch.object(self.compute.driver, 'detach_volume', side_effect=exception.DiskNotFound('sdb')), @@ -9808,7 +9832,8 @@ class ComputeAPITestCase(BaseTestCase): 'fake-id', 'fake-connector') mock_destroy.assert_called_once_with() - mock_detach.assert_called_once_with(mock.ANY, 'fake-id') + mock_detach.assert_called_once_with(mock.ANY, 'fake-id', + instance.uuid, None) def test_terminate_with_volumes(self): # Make sure that volumes get detached during instance termination. @@ -9831,7 +9856,7 @@ class ComputeAPITestCase(BaseTestCase): # Stub out and record whether it gets detached result = {"detached": False} - def fake_detach(self, context, volume_id_param): + def fake_detach(self, context, volume_id_param, instance_uuid): result["detached"] = volume_id_param == volume_id self.stubs.Set(cinder.API, "detach", fake_detach) @@ -9872,7 +9897,14 @@ class ComputeAPITestCase(BaseTestCase): bdm_obj.create() bdms.append(bdm_obj) - self.stubs.Set(self.compute, 'volume_api', mox.MockAnything()) + self.stub_out('nova.volume.cinder.API.terminate_connection', + mox.MockAnything()) + self.stub_out('nova.volume.cinder.API.detach', mox.MockAnything()) + + def fake_volume_get(self, context, volume_id): + return {'id': volume_id} + self.stub_out('nova.volume.cinder.API.get', fake_volume_get) + self.stubs.Set(self.compute, '_prep_block_device', mox.MockAnything()) self.compute.build_and_run_instance(self.context, instance, {}, {}, {}, block_device_mapping=[]) @@ -11409,7 +11441,7 @@ class EvacuateHostTestCase(BaseTestCase): # Stub out and record whether it gets detached result = {"detached": False} - def fake_detach(self, context, volume): + def fake_detach(self, context, volume, instance_uuid, attachment_id): result["detached"] = volume["id"] == 'fake_volume_id' self.stubs.Set(cinder.API, "detach", fake_detach) @@ -11425,7 +11457,8 @@ class EvacuateHostTestCase(BaseTestCase): # make sure volumes attach, detach are called self.mox.StubOutWithMock(self.compute.volume_api, 'detach') - self.compute.volume_api.detach(mox.IsA(self.context), mox.IgnoreArg()) + self.compute.volume_api.detach(mox.IsA(self.context), mox.IgnoreArg(), + mox.IgnoreArg(), None) self.mox.StubOutWithMock(self.compute, '_prep_block_device') self.compute._prep_block_device(mox.IsA(self.context), diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index b20d7ae82415..8a97da89bff8 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1796,16 +1796,21 @@ class _ComputeAPIUnitTestMixIn(object): volumes[old_volume_id] = {'id': old_volume_id, 'display_name': 'old_volume', 'attach_status': 'attached', - 'instance_uuid': uuids.vol_instance, 'size': 5, - 'status': 'in-use'} + 'status': 'in-use', + 'multiattach': False, + 'attachments': {uuids.vol_instance: { + 'attachment_id': 'fakeid' + } + } + } new_volume_id = uuidutils.generate_uuid() volumes[new_volume_id] = {'id': new_volume_id, 'display_name': 'new_volume', 'attach_status': 'detached', - 'instance_uuid': None, 'size': 5, - 'status': 'available'} + 'status': 'available', + 'multiattach': False} self.assertRaises(exception.InstanceInvalidState, self.compute_api.swap_volume, self.context, instance, volumes[old_volume_id], volumes[new_volume_id]) @@ -1822,13 +1827,15 @@ class _ComputeAPIUnitTestMixIn(object): volumes[old_volume_id]['attach_status'] = 'attached' # Should fail if old volume's instance_uuid is not that of the instance - volumes[old_volume_id]['instance_uuid'] = uuids.vol_instance_2 + volumes[old_volume_id]['attachments'] = {uuids.vol_instance_2: + {'attachment_id': 'fakeid'}} self.assertRaises(exception.InvalidVolume, self.compute_api.swap_volume, self.context, instance, volumes[old_volume_id], volumes[new_volume_id]) self.assertEqual(volumes[old_volume_id]['status'], 'in-use') self.assertEqual(volumes[new_volume_id]['status'], 'available') - volumes[old_volume_id]['instance_uuid'] = uuids.vol_instance + volumes[old_volume_id]['attachments'] = {uuids.vol_instance: + {'attachment_id': 'fakeid'}} # Should fail if new volume is attached volumes[new_volume_id]['attach_status'] = 'attached' diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 18b25d0f118c..6625488b1fd6 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -2201,9 +2201,10 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): '_notify_about_instance_usage') def _test_detach_volume(self, notify_inst_usage, detach, bdm_get, destroy_bdm=True): - volume_id = '123' + volume_id = uuids.volume inst_obj = mock.Mock() - inst_obj.uuid = 'uuid' + inst_obj.uuid = uuids.instance + attachment_id = uuids.attachment bdm = mock.MagicMock(spec=objects.BlockDeviceMapping) bdm.device_name = 'vdb' @@ -2216,13 +2217,16 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.compute._detach_volume(self.context, volume_id, inst_obj, - destroy_bdm=destroy_bdm) + destroy_bdm=destroy_bdm, + attachment_id=attachment_id) detach.assert_called_once_with(self.context, inst_obj, bdm) driver.get_volume_connector.assert_called_once_with(inst_obj) volume_api.terminate_connection.assert_called_once_with( self.context, volume_id, connector_sentinel) - volume_api.detach.assert_called_once_with(mock.ANY, volume_id) + volume_api.detach.assert_called_once_with(mock.ANY, volume_id, + inst_obj.uuid, + attachment_id) notify_inst_usage.assert_called_once_with( self.context, inst_obj, "volume.detach", extra_usage_info={'volume_id': volume_id} diff --git a/nova/tests/unit/compute/test_rpcapi.py b/nova/tests/unit/compute/test_rpcapi.py index 3b9bf9732058..c87494be4b0b 100644 --- a/nova/tests/unit/compute/test_rpcapi.py +++ b/nova/tests/unit/compute/test_rpcapi.py @@ -198,7 +198,31 @@ class ComputeRpcAPITestCase(test.NoDBTestCase): def test_detach_volume(self): self._test_compute_api('detach_volume', 'cast', instance=self.fake_instance_obj, volume_id='id', - version='4.0') + attachment_id='fake_id', version='4.7') + + def test_detach_volume_no_attachment_id(self): + ctxt = context.RequestContext('fake_user', 'fake_project') + instance = self.fake_instance_obj + rpcapi = compute_rpcapi.ComputeAPI() + cast_mock = mock.Mock() + cctxt_mock = mock.Mock(cast=cast_mock) + with test.nested( + mock.patch.object(rpcapi.client, 'can_send_version', + return_value=False), + mock.patch.object(rpcapi.client, 'prepare', + return_value=cctxt_mock) + ) as ( + can_send_mock, prepare_mock + ): + rpcapi.detach_volume(ctxt, instance=instance, + volume_id='id', attachment_id='fake_id') + # assert our mocks were called as expected + can_send_mock.assert_called_once_with('4.7') + prepare_mock.assert_called_once_with(server=instance['host'], + version='4.0') + cast_mock.assert_called_once_with(ctxt, 'detach_volume', + instance=instance, + volume_id='id') def test_finish_resize(self): self._test_compute_api('finish_resize', 'cast', diff --git a/nova/tests/unit/fake_volume.py b/nova/tests/unit/fake_volume.py index f910b7b83b3f..d2e2d090b155 100644 --- a/nova/tests/unit/fake_volume.py +++ b/nova/tests/unit/fake_volume.py @@ -66,7 +66,8 @@ class fake_volume(object): 'display_description': description, 'provider_location': 'fake-location', 'provider_auth': 'fake-auth', - 'volume_type_id': 99 + 'volume_type_id': 99, + 'multiattach': False } def get(self, key, default=None): @@ -193,31 +194,38 @@ class API(object): msg = "Instance and volume not in same availability_zone" raise exception.InvalidVolume(reason=msg) - def check_detach(self, context, volume): + def check_detach(self, context, volume, instance=None): if volume['status'] == "available": msg = "already detached" raise exception.InvalidVolume(reason=msg) + if volume['attach_status'] == 'detached': + msg = "Volume must be attached in order to detach." + raise exception.InvalidVolume(reason=msg) + + if instance and not volume.get('attachments', {}).get(instance.uuid): + raise exception.VolumeUnattached(volume_id=volume['id']) + def attach(self, context, volume_id, instance_uuid, mountpoint, mode='rw'): LOG.info('attaching volume %s', volume_id) volume = self.get(context, volume_id) volume['status'] = 'in-use' - volume['mountpoint'] = mountpoint volume['attach_status'] = 'attached' - volume['instance_uuid'] = instance_uuid volume['attach_time'] = timeutils.utcnow() + volume['multiattach'] = True + volume['attachments'] = {instance_uuid: + {'attachment_id': str(uuid.uuid4()), + 'mountpoint': mountpoint}} def reset_fake_api(self, context): del self.volume_list[:] del self.snapshot_list[:] - def detach(self, context, volume_id): + def detach(self, context, volume_id, instance_uuid, attachment_id=None): LOG.info('detaching volume %s', volume_id) volume = self.get(context, volume_id) volume['status'] = 'available' - volume['mountpoint'] = None volume['attach_status'] = 'detached' - volume['instance_uuid'] = None def initialize_connection(self, context, volume_id, connector): return {'driver_volume_type': 'iscsi', 'data': {}} diff --git a/nova/tests/unit/test_cinder.py b/nova/tests/unit/test_cinder.py index 1a32cb7e5361..482d0dee4752 100644 --- a/nova/tests/unit/test_cinder.py +++ b/nova/tests/unit/test_cinder.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + from cinderclient.v1 import client as cinder_client_v1 from cinderclient.v2 import client as cinder_client_v2 from requests_mock.contrib import fixture @@ -29,6 +31,42 @@ _image_metadata = { } +_volume_id = "6edbc2f4-1507-44f8-ac0d-eed1d2608d38" +_instance_uuid = "f4fda93b-06e0-4743-8117-bc8bcecd651b" +_instance_uuid_2 = "f4fda93b-06e0-4743-8117-bc8bcecd651c" +_attachment_id = "3b4db356-253d-4fab-bfa0-e3626c0b8405" +_attachment_id_2 = "3b4db356-253d-4fab-bfa0-e3626c0b8406" +_device = "/dev/vdb" +_device_2 = "/dev/vdc" + + +_volume_attachment = \ + [{"server_id": _instance_uuid, + "attachment_id": _attachment_id, + "host_name": "", + "volume_id": _volume_id, + "device": _device, + "id": _volume_id + }] + + +_volume_attachment_2 = _volume_attachment +_volume_attachment_2.append({"server_id": _instance_uuid_2, + "attachment_id": _attachment_id_2, + "host_name": "", + "volume_id": _volume_id, + "device": _device_2, + "id": _volume_id}) + + +exp_volume_attachment = collections.OrderedDict() +exp_volume_attachment[_instance_uuid] = {'attachment_id': _attachment_id, + 'mountpoint': _device} +exp_volume_attachment_2 = exp_volume_attachment +exp_volume_attachment_2[_instance_uuid_2] = {'attachment_id': _attachment_id_2, + 'mountpoint': _device_2} + + class BaseCinderTestCase(object): def setUp(self): @@ -97,13 +135,14 @@ class CinderTestCase(BaseCinderTestCase, test.NoDBTestCase): "attachments": [], "availability_zone": "cinder", "created_at": "2012-09-10T00:00:00.000000", - "id": '00000000-0000-0000-0000-000000000000', + "id": _volume_id, "metadata": {}, "size": 1, "snapshot_id": None, "status": "available", "volume_type": "None", - "bootable": "true" + "bootable": "true", + "multiattach": "true" } volume.update(kwargs) return volume @@ -161,13 +200,14 @@ class CinderV2TestCase(BaseCinderTestCase, test.NoDBTestCase): "attachments": [], "availability_zone": "cinderv2", "created_at": "2013-08-10T00:00:00.000000", - "id": '00000000-0000-0000-0000-000000000000', + "id": _volume_id, "metadata": {}, "size": 1, "snapshot_id": None, "status": "available", "volume_type": "None", - "bootable": "true" + "bootable": "true", + "multiattach": "true" } volume.update(kwargs) return volume @@ -191,3 +231,23 @@ class CinderV2TestCase(BaseCinderTestCase, test.NoDBTestCase): volume = self.api.get(self.context, '5678') self.assertIn('volume_image_metadata', volume) self.assertEqual(_image_metadata, volume['volume_image_metadata']) + + def test_volume_without_attachment(self): + v = self.stub_volume(id='1234') + self.requests.get(self.URL + '/volumes/5678', json={'volume': v}) + volume = self.api.get(self.context, '5678') + self.assertIsNone(volume.get('attachments')) + + def test_volume_with_one_attachment(self): + v = self.stub_volume(id='1234', attachments=_volume_attachment) + self.requests.get(self.URL + '/volumes/5678', json={'volume': v}) + volume = self.api.get(self.context, '5678') + self.assertIn('attachments', volume) + self.assertEqual(exp_volume_attachment, volume['attachments']) + + def test_volume_with_two_attachments(self): + v = self.stub_volume(id='1234', attachments=_volume_attachment_2) + self.requests.get(self.URL + '/volumes/5678', json={'volume': v}) + volume = self.api.get(self.context, '5678') + self.assertIn('attachments', volume) + self.assertEqual(exp_volume_attachment_2, volume['attachments']) diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 79a7fc3395de..6c86cdcbceb7 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -19,6 +19,8 @@ import mock from nova import context from nova import exception from nova import test +from nova.tests.unit.fake_instance import fake_instance_obj +from nova.tests import uuidsentinel as uuids from nova.volume import cinder @@ -171,6 +173,7 @@ class CinderApiTestCase(test.NoDBTestCase): volume = {'status': 'available'} volume['attach_status'] = "detached" volume['availability_zone'] = 'zone1' + volume['multiattach'] = False instance = {'availability_zone': 'zone1', 'host': 'fakehost'} cinder.CONF.set_override('cross_az_attach', False, group='cinder') @@ -182,11 +185,27 @@ class CinderApiTestCase(test.NoDBTestCase): cinder.CONF.reset() def test_check_detach(self): - volume = {'id': 'fake', 'status': 'available'} + volume = {'id': 'fake', 'status': 'in-use', + 'attach_status': 'attached', + 'attachments': {uuids.instance: { + 'attachment_id': uuids.attachment}} + } + self.assertIsNone(self.api.check_detach(self.ctx, volume)) + instance = fake_instance_obj(self.ctx) + instance.uuid = uuids.instance + self.assertIsNone(self.api.check_detach(self.ctx, volume, instance)) + instance.uuid = uuids.instance2 + self.assertRaises(exception.VolumeUnattached, + self.api.check_detach, self.ctx, volume, instance) + volume['attachments'] = {} + self.assertRaises(exception.VolumeUnattached, + self.api.check_detach, self.ctx, volume, instance) + volume['status'] = 'available' + self.assertRaises(exception.InvalidVolume, + self.api.check_detach, self.ctx, volume) + volume['attach_status'] = 'detached' self.assertRaises(exception.InvalidVolume, self.api.check_detach, self.ctx, volume) - volume['status'] = 'non-available' - self.assertIsNone(self.api.check_detach(self.ctx, volume)) def test_reserve_volume(self): cinder.cinderclient(self.ctx).AndReturn(self.cinderclient) @@ -251,14 +270,24 @@ class CinderApiTestCase(test.NoDBTestCase): mode='ro') def test_detach(self): + self.mox.StubOutWithMock(self.api, + 'get', + use_mock_anything=True) + self.api.get(self.ctx, 'id1').\ + AndReturn({'id': 'id1', 'status': 'in-use', + 'multiattach': True, + 'attach_status': 'attached', + 'attachments': {'fake_uuid': + {'attachment_id': 'fakeid'}} + }) cinder.cinderclient(self.ctx).AndReturn(self.cinderclient) self.mox.StubOutWithMock(self.cinderclient.volumes, 'detach', use_mock_anything=True) - self.cinderclient.volumes.detach('id1') + self.cinderclient.volumes.detach('id1', 'fakeid') self.mox.ReplayAll() - self.api.detach(self.ctx, 'id1') + self.api.detach(self.ctx, 'id1', instance_uuid='fake_uuid') def test_initialize_connection(self): cinder.cinderclient(self.ctx).AndReturn(self.cinderclient) diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 66d5aa33135f..264935da0ed4 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -18,6 +18,7 @@ Handles all requests relating to volumes + cinder. """ +import collections import copy import sys @@ -160,12 +161,18 @@ def _untranslate_volume_summary_view(context, vol): # removed. d['attach_time'] = "" d['mountpoint'] = "" + d['multiattach'] = getattr(vol, 'multiattach', False) if vol.attachments: - att = vol.attachments[0] + d['attachments'] = collections.OrderedDict() + for attachment in vol.attachments: + a = {attachment['server_id']: + {'attachment_id': attachment.get('attachment_id'), + 'mountpoint': attachment.get('device')} + } + d['attachments'].update(a.items()) + d['attach_status'] = 'attached' - d['instance_uuid'] = att['server_id'] - d['mountpoint'] = att['device'] else: d['attach_status'] = 'detached' # NOTE(dzyu) volume(cinder) v2 API uses 'name' instead of 'display_name', @@ -316,12 +323,24 @@ class API(object): 'vol_zone': volume['availability_zone']} raise exception.InvalidVolume(reason=msg) - def check_detach(self, context, volume): + def check_detach(self, context, volume, instance=None): # TODO(vish): abstract status checking? if volume['status'] == "available": msg = _("volume %s already detached") % volume['id'] raise exception.InvalidVolume(reason=msg) + if volume['attach_status'] == 'detached': + msg = _("Volume must be attached in order to detach.") + raise exception.InvalidVolume(reason=msg) + + # NOTE(ildikov):Preparation for multiattach support, when a volume + # can be attached to multiple hosts and/or instances, + # so just check the attachment specific to this instance + if instance is not None and instance.uuid not in volume['attachments']: + # TODO(ildikov): change it to a better exception, when enable + # multi-attach. + raise exception.VolumeUnattached(volume_id=volume['id']) + @translate_volume_exception def reserve_volume(self, context, volume_id): cinderclient(context).volumes.reserve(volume_id) @@ -344,8 +363,34 @@ class API(object): mountpoint, mode=mode) @translate_volume_exception - def detach(self, context, volume_id): - cinderclient(context).volumes.detach(volume_id) + def detach(self, context, volume_id, instance_uuid=None, + attachment_id=None): + if attachment_id is None: + volume = self.get(context, volume_id) + if volume['multiattach']: + attachments = volume.get('attachments', {}) + if instance_uuid: + attachment_id = attachments.get(instance_uuid, {}).\ + get('attachment_id') + if not attachment_id: + LOG.warning(_LW("attachment_id couldn't be retrieved " + "for volume %(volume_id)s with " + "instance_uuid %(instance_id)s. The " + "volume has the 'multiattach' flag " + "enabled, without the attachment_id " + "Cinder most probably cannot perform " + "the detach."), + {'volume_id': volume_id, + 'instance_id': instance_uuid}) + else: + LOG.warning(_LW("attachment_id couldn't be retrieved for " + "volume %(volume_id)s. The volume has the " + "'multiattach' flag enabled, without the " + "attachment_id Cinder most probably " + "cannot perform the detach."), + {'volume_id': volume_id}) + + cinderclient(context).volumes.detach(volume_id, attachment_id) @translate_volume_exception def initialize_connection(self, context, volume_id, connector):