openstack
/
cinder
Code Issues Proposed changes

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
This commit is contained in:
Lee Yarwood 2018-11-07 14:26:26 +00:00
parent 476ed701e2
commit d0b59152ed
2 changed files with 69 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
cinder/tests/unit/volume/drivers/test_lvm_driver.py
View File

@ -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)])

24
cinder/volume/drivers/lvm.py
View File

@ -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