diff --git a/os_brick/initiator/connectors/fibre_channel.py b/os_brick/initiator/connectors/fibre_channel.py index 50f404b27..b50e67ddd 100644 --- a/os_brick/initiator/connectors/fibre_channel.py +++ b/os_brick/initiator/connectors/fibre_channel.py @@ -164,7 +164,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): return volume_paths @utils.trace - @synchronized('extend_volume') + @synchronized('extend_volume', external=True) def extend_volume(self, connection_properties): """Update the local kernel's size information. @@ -185,7 +185,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): raise exception.VolumePathsNotFound() @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Attach the volume to instance_name. @@ -310,7 +310,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): return raw_devices @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Detach the volume from instance_name. diff --git a/os_brick/initiator/connectors/huawei.py b/os_brick/initiator/connectors/huawei.py index b33131ea8..fd0c61706 100644 --- a/os_brick/initiator/connectors/huawei.py +++ b/os_brick/initiator/connectors/huawei.py @@ -85,7 +85,7 @@ class HuaweiStorHyperConnector(base.BaseLinuxConnector): return out['dev_addr'] @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Connect to a volume. @@ -116,7 +116,7 @@ class HuaweiStorHyperConnector(base.BaseLinuxConnector): return device_info @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect a volume from the local host. diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index 5011947ff..53cdf5f1a 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -468,7 +468,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): root_helper=self._root_helper) @utils.trace - @synchronized('extend_volume') + @synchronized('extend_volume', external=True) def extend_volume(self, connection_properties): """Update the local kernel's size information. @@ -491,18 +491,10 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): raise exception.VolumePathsNotFound() @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Attach the volume to instance_name. - NOTE: Will retry up to three times to handle the case where c-vol - and n-cpu are both using os-brick to manage iSCSI sessions but they - are on the same node and using different locking directories. In this - case, even though this call is synchronized, they will be separate - locks and can still overlap with connect and disconnect. Since a - disconnect during an initial attach can't cause IO failure (the device - has not been made available yet), we just try the connection again. - :param connection_properties: The valid dictionary that describes all of the target volume attributes. :type connection_properties: dict @@ -862,7 +854,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): return device_map @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Detach the volume from instance_name. @@ -1045,7 +1037,8 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): target_iqn = connection_properties['target_iqn'] lock_name = 'connect_to_iscsi_portal-{}-{}'.format(portal, target_iqn) - method = synchronized(lock_name)(self._connect_to_iscsi_portal_unsafe) + method = synchronized( + lock_name, external=True)(self._connect_to_iscsi_portal_unsafe) return method(connection_properties) @utils.retry((exception.BrickException)) diff --git a/os_brick/initiator/connectors/nvmeof.py b/os_brick/initiator/connectors/nvmeof.py index eeeaf0853..c6cc1853e 100644 --- a/os_brick/initiator/connectors/nvmeof.py +++ b/os_brick/initiator/connectors/nvmeof.py @@ -127,7 +127,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): run_as_root=True) @utils.trace - @synchronized('connect_volume') + @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Discover and attach the volume. @@ -167,7 +167,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): return device_info @utils.trace - @synchronized('disconnect_volume') + @synchronized('disconnect_volume', external=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Detach and flush the volume. @@ -218,7 +218,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): raise @utils.trace - @synchronized('extend_volume') + @synchronized('extend_volume', external=True) def extend_volume(self, connection_properties): """Update the local kernel's size information. diff --git a/os_brick/initiator/connectors/scaleio.py b/os_brick/initiator/connectors/scaleio.py index 6b82c0de6..b1feddd41 100644 --- a/os_brick/initiator/connectors/scaleio.py +++ b/os_brick/initiator/connectors/scaleio.py @@ -336,7 +336,7 @@ class ScaleIOConnector(base.BaseLinuxConnector): return device_info @utils.trace - @lockutils.synchronized('scaleio', 'scaleio-') + @lockutils.synchronized('scaleio', 'scaleio-', external=True) def connect_volume(self, connection_properties): """Connect the volume. @@ -450,7 +450,7 @@ class ScaleIOConnector(base.BaseLinuxConnector): return device_info @utils.trace - @lockutils.synchronized('scaleio', 'scaleio-') + @lockutils.synchronized('scaleio', 'scaleio-', external=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect the ScaleIO volume. diff --git a/os_brick/initiator/utils.py b/os_brick/initiator/utils.py index ae4b0af88..967c55761 100644 --- a/os_brick/initiator/utils.py +++ b/os_brick/initiator/utils.py @@ -42,5 +42,6 @@ def guard_connection(device): else: # Cinder passes an OVO, but Nova passes a dictionary, so we use dict # key access that works with both. - with lockutils.lock(device['service_uuid'], 'os-brick-'): + with lockutils.lock(device['service_uuid'], 'os-brick-', + external=True): yield diff --git a/os_brick/tests/base.py b/os_brick/tests/base.py index 2cbe6d185..7a3ee85af 100644 --- a/os_brick/tests/base.py +++ b/os_brick/tests/base.py @@ -19,6 +19,8 @@ import testtools import fixtures import mock +from oslo_concurrency import lockutils +from oslo_config import fixture as config_fixture from oslo_utils import strutils @@ -60,6 +62,13 @@ class TestCase(testtools.TestCase): format=log_format, level=level)) + # At runtime this would be set by the library user: Cinder, Nova, etc. + self.useFixture(fixtures.NestedTempfile()) + lock_path = self.useFixture(fixtures.TempDir()).path + self.fixture = self.useFixture(config_fixture.Config(lockutils.CONF)) + self.fixture.config(lock_path=lock_path, group='oslo_concurrency') + lockutils.set_defaults(lock_path) + def _common_cleanup(self): """Runs after each test method to tear down test environment.""" diff --git a/os_brick/tests/initiator/test_utils.py b/os_brick/tests/initiator/test_utils.py index 57f701439..6ab76156f 100644 --- a/os_brick/tests/initiator/test_utils.py +++ b/os_brick/tests/initiator/test_utils.py @@ -59,4 +59,5 @@ class InitiatorUtilsTestCase(base.TestCase): utils.ISCSI_SUPPORTS_MANUAL_SCAN = False with utils.guard_connection({'service_uuid': mock.sentinel.uuid, 'shared_targets': True}): - mock_lock.assert_called_once_with(mock.sentinel.uuid, 'os-brick-') + mock_lock.assert_called_once_with(mock.sentinel.uuid, 'os-brick-', + external=True) diff --git a/releasenotes/notes/external-locks-9f015988ebdc37d6.yaml b/releasenotes/notes/external-locks-9f015988ebdc37d6.yaml new file mode 100644 index 000000000..b31124a76 --- /dev/null +++ b/releasenotes/notes/external-locks-9f015988ebdc37d6.yaml @@ -0,0 +1,12 @@ +--- +upgrade: + - | + Services using os-brick need to set the ``lock_path`` configuration option + in their ``[oslo_concurrency]`` section since it doesn't have a valid + default (related `bug #1947370 + `_). +fixes: + - | + `Bug #1947370 `_: Fixed + race conditions on iSCSI with shared targets and NVMe ``connect_volume`` + and ``disconnect_volume`` calls.