From d0b59152ed413f39599ffd1554c0e4e0038431aa Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Wed, 7 Nov 2018 14:26:26 +0000 Subject: [PATCH] lvm: Avoid premature calls to terminate_connection for muiltiattach vols Previously the LVM volume driver would always call termiante_connection on the configured target driver regardless of of the associated volume being attached to multiple instances on the same host. This isn't an issue for the tgt target driver used by upstream CI as this call is a noop but does cause the lioadm driver to prematurely remove specific host ACLs for volumes that are still being used. This change introduces a simple check to ensure that the target driver is only called when a single attachment remains that is in turn using the connector provided by the caller. Finally as a result of this bugfix the changes introduced by I84f607de13bc17b00609ad37121d8678f7f4a920 to disable the multiattach feature when using the lioadm target driver are removed. Closes-bug: #1786327 Change-Id: Ib5aa1b7578f7d3200185566ff5f8634dd519d020 --- .../unit/volume/drivers/test_lvm_driver.py | 57 +++++++++++++++++++ cinder/volume/drivers/lvm.py | 24 ++++---- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/test_lvm_driver.py b/cinder/tests/unit/volume/drivers/test_lvm_driver.py index 08cd428b79e..7da792d2686 100644 --- a/cinder/tests/unit/volume/drivers/test_lvm_driver.py +++ b/cinder/tests/unit/volume/drivers/test_lvm_driver.py @@ -25,6 +25,7 @@ from cinder.objects import fields from cinder.tests import fake_driver from cinder.tests.unit.brick import fake_lvm from cinder.tests.unit import fake_constants as fake +from cinder.tests.unit import fake_volume from cinder.tests.unit import utils as tests_utils from cinder.tests.unit.volume import test_driver from cinder.volume import configuration as conf @@ -986,3 +987,59 @@ class LVMISCSITestCase(test_driver.BaseDriverTestCase): connector = {'ip': '10.0.0.2', 'host': 'fakehost'} self.assertRaises(exception.InvalidConnectorException, iscsi_driver.validate_connector, connector) + + def test_multiattach_terminate_connection(self): + # Ensure that target_driver.terminate_connection is only called when a + # single active volume attachment remains per host for each volume. + + host1_connector = {'ip': '10.0.0.2', + 'host': 'fakehost1', + 'initiator': 'iqn.2012-07.org.fake:01'} + + host2_connector = {'ip': '10.0.0.3', + 'host': 'fakehost2', + 'initiator': 'iqn.2012-07.org.fake:02'} + + host1_attachment1 = fake_volume.fake_volume_attachment_obj( + self.context) + host1_attachment1.connector = host1_connector + + host1_attachment2 = fake_volume.fake_volume_attachment_obj( + self.context) + host1_attachment2.connector = host1_connector + + host2_attachment = fake_volume.fake_volume_attachment_obj(self.context) + host2_attachment.connector = host2_connector + + # Create a multiattach volume object with two active attachments on + # host1 and another single attachment on host2. + vol = fake_volume.fake_volume_obj(self.context) + vol.multiattach = True + vol.volume_attachment.objects.append(host1_attachment1) + vol.volume_attachment.objects.append(host1_attachment2) + vol.volume_attachment.objects.append(host2_attachment) + + self.configuration = conf.Configuration(None) + vg_obj = fake_lvm.FakeBrickLVM('cinder-volumes', False, None, + 'default') + lvm_driver = lvm.LVMVolumeDriver(configuration=self.configuration, + db=db, vg_obj=vg_obj) + + with mock.patch.object(lvm_driver.target_driver, + 'terminate_connection') as mock_term_conn: + + # Verify that terminate_connection is not called against host1 when + # there are multiple active attachments against that host. + self.assertTrue(lvm_driver.terminate_connection(vol, + host1_connector)) + mock_term_conn.assert_not_called() + + # Verify that terminate_connection is called against either host + # when only one active attachment per host is present. + vol.volume_attachment.objects.remove(host1_attachment1) + self.assertTrue(lvm_driver.terminate_connection(vol, + host1_connector)) + self.assertTrue(lvm_driver.terminate_connection(vol, + host2_connector)) + mock_term_conn.assert_has_calls([mock.call(vol, host1_connector), + mock.call(vol, host2_connector)]) diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 9e2988a402d..c40e7ba2f46 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -244,10 +244,6 @@ class LVMVolumeDriver(driver.VolumeDriver): # This includes volumes and snapshots. total_volumes = len(self.vg.get_volumes()) - supports_multiattach = True - if self.configuration.target_helper == 'lioadm': - supports_multiattach = False - # Skip enabled_pools setting, treat the whole backend as one pool # XXX FIXME if multipool support is added to LVM driver. single_pool = {} @@ -266,7 +262,7 @@ class LVMVolumeDriver(driver.VolumeDriver): total_volumes=total_volumes, filter_function=self.get_filter_function(), goodness_function=self.get_goodness_function(), - multiattach=supports_multiattach, + multiattach=True, backend_state='up' )) data["pools"].append(single_pool) @@ -845,13 +841,17 @@ class LVMVolumeDriver(driver.VolumeDriver): # 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 + # NOTE(lyarwood): Given the above note we should only call + # terminate_connection for the target lioadm driver when there is only + # one attachment left for the host specified by the connector to + # remove, otherwise the ACL will be removed prematurely while other + # attachments on the same host are still accessing the volume. + attachments = volume.volume_attachment + if volume.multiattach: + if sum(1 for a in attachments if a.connector == connector) > 1: + return True - self.target_driver.terminate_connection(volume, connector, - **kwargs) - return has_shared_connections + self.target_driver.terminate_connection(volume, connector, **kwargs) + return len(attachments) > 1