From 345189153191abe55a922ba49593ca09b9b65540 Mon Sep 17 00:00:00 2001 From: Steve Noyes Date: Fri, 9 Jun 2017 11:14:31 -0400 Subject: [PATCH] Update attachment_delete for multiple attachments - change attachment_delete to not remove the export if multiple attachments exist Also remove the unused return of has_shared_connections in attachment_delete method. It's not used or needed in this method. Closes-Bug: 1697008 Change-Id: I62b90c4cd4ad2bfb8a68718b43dc606e02fe776a --- .../attachments/test_attachments_manager.py | 54 +++++++++++++++++++ cinder/volume/drivers/lvm.py | 16 +++++- cinder/volume/manager.py | 12 ++--- 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index 104c0d83501..4f79b0bf033 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -18,6 +18,7 @@ from cinder import context from cinder import db from cinder import exception from cinder.objects import fields +from cinder.objects import volume_attachment from cinder import test from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import utils as tests_utils @@ -143,3 +144,56 @@ class AttachmentManagerTestCase(test.TestCase): db.volume_attachment_get, self.context, attachment_ref.id) + + def test_attachment_delete_multiple_attachments(self): + volume_params = {'status': 'available'} + vref = tests_utils.create_volume(self.context, **volume_params) + attachment1 = volume_attachment.VolumeAttachment() + attachment2 = volume_attachment.VolumeAttachment() + + attachment1.id = fake.UUID1 + attachment2.id = fake.UUID2 + + @mock.patch.object(self.manager.db, 'volume_admin_metadata_delete') + @mock.patch.object(self.manager.db, 'volume_detached') + @mock.patch.object(self.context, 'elevated') + @mock.patch.object(self.manager, '_connection_terminate') + @mock.patch.object(self.manager.driver, 'remove_export') + @mock.patch.object(self.manager.driver, 'detach_volume') + def _test(mock_detach, mock_rm_export, mock_con_term, + mock_elevated, mock_db_detached, mock_db_meta_delete): + mock_elevated.return_value = self.context + mock_con_term.return_value = False + + # test single attachment. This should call + # detach and remove_export + vref.volume_attachment.objects.append(attachment1) + + self.manager._do_attachment_delete(self.context, vref, attachment1) + + mock_detach.assert_called_once_with(self.context, vref, + attachment1) + mock_db_detached.called_once_with(self.context, vref, + attachment1.id) + mock_db_meta_delete.called_once_with(self.context, vref.id, + 'attached_mode') + mock_rm_export.assert_called_once_with(self.context, vref) + + # test more than 1 attachment. This should skip + # detach and remove_export + mock_con_term.return_value = True + vref.volume_attachment.objects.append(attachment2) + + mock_detach.reset_mock() + mock_rm_export.reset_mock() + mock_db_detached.reset_mock() + mock_db_meta_delete.reset_mock() + + self.manager._do_attachment_delete(self.context, vref, attachment2) + + mock_rm_export.assert_not_called() + mock_db_detached.called_once_with(self.context, vref, + attachment2.id) + mock_db_meta_delete.called_once_with(self.context, vref.id, + 'attached_mode') + _test() diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 4f88ec09afd..11a2f23d7a6 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -801,5 +801,17 @@ class LVMVolumeDriver(driver.VolumeDriver): return self.target_driver.validate_connector(connector) def terminate_connection(self, volume, connector, **kwargs): - return self.target_driver.terminate_connection(volume, connector, - **kwargs) + # NOTE(jdg): LVM has a single export for each volume, so what + # we need to do here is check if there is more than one attachment for + # the volume, if there is; let the caller know that they should NOT + # remove the export. + has_shared_connections = False + if len(volume.volume_attachment) > 1: + has_shared_connections = True + + # NOTE(jdg): For the TGT driver this is a noop, for LIO this removes + # the initiator IQN from the targets access list, so we're good + + self.target_driver.terminate_connection(volume, connector, + **kwargs) + return has_shared_connections diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index d650dc918dc..3926e8b8d16 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -4184,17 +4184,13 @@ class VolumeManager(manager.CleanableManager, NOTE if the attachment reference is None, we remove all existing attachments for the specified volume object. """ - has_shared_connection = False attachment_ref = objects.VolumeAttachment.get_by_id(context, attachment_id) if not attachment_ref: for attachment in VA_LIST.get_all_by_volume_id(context, vref.id): - if self._do_attachment_delete(context, vref, attachment): - has_shared_connection = True + self._do_attachment_delete(context, vref, attachment) else: - has_shared_connection = ( - self._do_attachment_delete(context, vref, attachment_ref)) - return has_shared_connection + self._do_attachment_delete(context, vref, attachment_ref) def _do_attachment_delete(self, context, vref, attachment): utils.require_driver_initialized(self.driver) @@ -4207,7 +4203,8 @@ class VolumeManager(manager.CleanableManager, {'attachment_id': attachment.id}, resource=vref) self.driver.detach_volume(context, vref, attachment) - self.driver.remove_export(context.elevated(), vref) + if not has_shared_connection: + self.driver.remove_export(context.elevated(), vref) except Exception: # FIXME(jdg): Obviously our volume object is going to need some # changes to deal with multi-attach and figuring out how to @@ -4224,7 +4221,6 @@ class VolumeManager(manager.CleanableManager, vref.id, 'attached_mode') self._notify_about_volume_usage(context, vref, "detach.end") - return has_shared_connection # Replication group API (Tiramisu) def enable_replication(self, ctxt, group):