diff --git a/nova/exception.py b/nova/exception.py index d7855e43db92..0d694aaab02f 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -571,6 +571,10 @@ class AgentBuildExists(NovaException): "architecture %(architecture)s exists.") +class VolumeAttachmentNotFound(NotFound): + msg_fmt = _("Volume attachment %(attachment_id)s could not be found.") + + class VolumeNotFound(NotFound): msg_fmt = _("Volume %(volume_id)s could not be found.") diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index 870ec2994f27..d3c0f0a40576 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -101,6 +101,7 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': 0}) volume_driver_bdm = { + 'attachment_id': None, 'mount_device': '/dev/sda1', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': False, @@ -129,6 +130,7 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) snapshot_driver_bdm = { + 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -157,6 +159,7 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) image_driver_bdm = { + 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -185,6 +188,7 @@ class TestDriverBlockDevice(test.NoDBTestCase): 'boot_index': -1}) blank_driver_bdm = { + 'attachment_id': None, 'mount_device': '/dev/sda2', 'connection_info': {"fake": "connection_info"}, 'delete_on_termination': True, @@ -383,6 +387,31 @@ class TestDriverBlockDevice(test.NoDBTestCase): def test_call_wait_no_delete_volume(self): self._test_call_wait_func(False) + def test_volume_delete_attachment(self): + attachment_id = uuids.attachment + driver_bdm = self.driver_classes['volume'](self.volume_bdm) + driver_bdm['attachment_id'] = attachment_id + + elevated_context = self.context.elevated() + instance_detail = {'id': '123', 'uuid': uuids.uuid, + 'availability_zone': None} + instance = fake_instance.fake_instance_obj(self.context, + **instance_detail) + connector = {'ip': 'fake_ip', 'host': 'fake_host'} + + with test.nested( + mock.patch.object(self.virt_driver, 'get_volume_connector', + return_value=connector), + mock.patch.object(self.volume_api, 'attachment_delete'), + ) as (_, vapi_attach_del): + + driver_bdm.detach(elevated_context, instance, + self.volume_api, self.virt_driver, + attachment_id=attachment_id) + + vapi_attach_del.assert_called_once_with(elevated_context, + attachment_id) + def _test_volume_attach(self, driver_bdm, bdm_dict, fake_volume, fail_check_av_zone=False, driver_attach=False, fail_driver_attach=False, diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index f99337457ce1..1f4c9a8bf700 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -280,6 +280,33 @@ class CinderApiTestCase(test.NoDBTestCase): mock_volumes.attach.assert_called_once_with('id1', 'uuid', 'point', mode='ro') + @mock.patch('nova.volume.cinder.cinderclient') + def test_attachment_delete(self, mock_cinderclient): + mock_attachments = mock.MagicMock() + mock_cinderclient.return_value = \ + mock.MagicMock(attachments=mock_attachments) + + attachment_id = uuids.attachment + self.api.attachment_delete(self.ctx, attachment_id) + + mock_cinderclient.assert_called_once_with(self.ctx) + mock_attachments.delete.assert_called_once_with(attachment_id) + + @mock.patch('nova.volume.cinder.LOG') + @mock.patch('nova.volume.cinder.cinderclient') + def test_attachment_delete_failed(self, mock_cinderclient, mock_log): + mock_cinderclient.return_value.attachments.delete.side_effect = ( + cinder_exception.NotFound(404, '404')) + + attachment_id = uuids.attachment + ex = self.assertRaises(exception.VolumeAttachmentNotFound, + self.api.attachment_delete, + self.ctx, + attachment_id) + + self.assertEqual(404, ex.code) + self.assertIn(attachment_id, ex.message) + @mock.patch('nova.volume.cinder.cinderclient') def test_detach(self, mock_cinderclient): mock_volumes = mock.MagicMock() diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index 4219fc10d21b..521c3e463cae 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -224,7 +224,8 @@ class DriverVolumeBlockDevice(DriverBlockDevice): _legacy_fields = set(['connection_info', 'mount_device', 'delete_on_termination']) _new_fields = set(['guest_format', 'device_type', - 'disk_bus', 'boot_index']) + 'disk_bus', 'boot_index', + 'attachment_id']) _fields = _legacy_fields | _new_fields _valid_source = 'volume' @@ -349,9 +350,16 @@ class DriverVolumeBlockDevice(DriverBlockDevice): 'schost': stashed_connector.get('host')}) connector = stashed_connector - volume_api.terminate_connection(context, volume_id, connector) - volume_api.detach(context.elevated(), volume_id, instance.uuid, - attachment_id) + # NOTE(jdg): For now we need to actually inspect the bdm for an + # attachment_id as opposed to relying on what may have been passed + # in, we want to force usage of the old detach flow for now and only + # use the new flow when we explicitly used it for the attach. + if not self['attachment_id']: + volume_api.terminate_connection(context, volume_id, connector) + volume_api.detach(context.elevated(), volume_id, instance.uuid, + attachment_id) + else: + volume_api.attachment_delete(context, self['attachment_id']) @update_db def attach(self, context, instance, volume_api, virt_driver, diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 0ebb56939bbb..4b3cde1af1f9 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -205,6 +205,19 @@ def translate_volume_exception(method): return translate_cinder_exception(wrapper) +def translate_attachment_exception(method): + """Transforms the exception for the attachment but keeps its traceback intact. + """ + def wrapper(self, ctx, attachment_id, *args, **kwargs): + try: + res = method(self, ctx, attachment_id, *args, **kwargs) + except (keystone_exception.NotFound, cinder_exception.NotFound): + _reraise(exception.VolumeAttachmentNotFound( + attachment_id=attachment_id)) + return res + return translate_cinder_exception(wrapper) + + def translate_snapshot_exception(method): """Transforms the exception for the snapshot but keeps its traceback intact. @@ -480,3 +493,16 @@ class API(object): {'status': status, 'progress': '90%'} ) + + @translate_attachment_exception + def attachment_delete(self, context, attachment_id): + try: + cinderclient( + context).attachments.delete(attachment_id) + except cinder_exception.ClientException as ex: + with excutils.save_and_reraise_exception(): + LOG.error(('Delete attachment failed for attachment ' + '%(id)s. Error: %(msg)s Code: %(code)s'), + {'id': attachment_id, + 'msg': six.text_type(ex), + 'code': getattr(ex, 'code', None)})