Context manager to handle shared_targets

We introduced "shared_targets" and "service_uuid" fields in volumes to
allow volume consumers to protect themselves from unintended leftover
devices when handling iSCSI connections with shared targets.

The way they protect themselves from the automatic scans that happen on
detach/map race conditions is by locking and only allowing one attach or
one detach operation for each server to happen at a given time.

When using an up to date Open iSCSI initiator we don't need to use
locks, as it has the possibility to disable automatic LUN scans (which
are the real cause of the leftover devices), and OS-Brick already
supports this feature.

This is currently not the case, since Nova is blindly locking whenever
"shared_targets" is set to True.

Thanks to the context manager introduced in this patch we can improve
our separation of concerns (Nova doesn't have to care when we have to
lock), and we can fine tune the locking to only lock when the iSCSI
initiator doesn't support disabling automatic scans.

The context manager "guard_connection" lives in
"os_brick.initiator.utils" and needs a cinder volume.  The volume can be
passed as a dictionary or OVO instance.

Change-Id: I4970363301d5d1f4e7d0f07e09b34d15ee6884c3
Related-Bug: #1800515

os_brick/initiator/connectors/iscsi.py

@@ -32,6 +32,7 @@ from os_brick.i18n import _
 from os_brick import initiator
 from os_brick.initiator.connectors import base
 from os_brick.initiator.connectors import base_iscsi
+from os_brick.initiator import utils as initiator_utils
 from os_brick import utils
 
 synchronized = lockutils.synchronized_with_prefix('os-brick-')
@@ -1030,6 +1031,11 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
                                     'node.session.scan', 'manual',
                                     check_exit_code=False)
         manual_scan = not res[1]
+        # Update global indicator of manual scan support used for
+        # shared_targets locking so we support upgrading open iscsi to a
+        # version supporting the manual scan feature without restarting Nova
+        # or Cinder.
+        initiator_utils.ISCSI_SUPPORTS_MANUAL_SCAN = manual_scan
 
         if connection_properties.get('auth_method'):
             self._iscsiadm_update(connection_properties,

os_brick/initiator/utils.py

@@ -0,0 +1,46 @@
+# Copyright 2018 Red Hat, Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import contextlib
+import os
+
+from oslo_concurrency import lockutils
+from oslo_concurrency import processutils as putils
+
+
+def check_manual_scan():
+    if os.name == 'nt':
+        return False
+
+    try:
+        putils.execute('grep', '-F', 'node.session.scan', '/sbin/iscsiadm')
+    except putils.ProcessExecutionError:
+        return False
+    return True
+
+
+ISCSI_SUPPORTS_MANUAL_SCAN = check_manual_scan()
+
+
+@contextlib.contextmanager
+def guard_connection(device):
+    """Context Manager handling locks for attach/detach operations."""
+    if ISCSI_SUPPORTS_MANUAL_SCAN or not device.get('shared_targets'):
+        yield
+    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-'):
+            yield

os_brick/tests/initiator/connectors/test_iscsi.py

@@ -21,6 +21,7 @@ from oslo_concurrency import processutils as putils
 from os_brick import exception
 from os_brick.initiator.connectors import iscsi
 from os_brick.initiator import linuxscsi
+from os_brick.initiator import utils
 from os_brick.privileged import rootwrap as priv_rootwrap
 from os_brick.tests.initiator import test_connector
 
@@ -987,6 +988,7 @@ Setting up iSCSI targets: unused
             [('tcp:', 'session1', 'ip1:port1', '1', 'tgt'),
              ('tcp:', session, 'ip1:port1', '-1', 'tgt1')]
         ]
+        utils.ISCSI_SUPPORTS_MANUAL_SCAN = None
         with mock.patch.object(self.connector, '_execute') as exec_mock:
             exec_mock.side_effect = [('', 'error'), ('', None),
                                      ('', None), ('', None),
@@ -996,6 +998,7 @@ Setting up iSCSI targets: unused
         # True refers to "manual scans", since the call to update
         # node.session.scan didn't fail they are set to manual
         self.assertEqual((session, True), res)
+        self.assertTrue(utils.ISCSI_SUPPORTS_MANUAL_SCAN)
         prefix = 'iscsiadm -m node -T tgt1 -p ip1:port1'
         expected_cmds = [
             prefix,
@@ -1018,6 +1021,7 @@ Setting up iSCSI targets: unused
             [('tcp:', 'session1', 'Ip1:port1', '1', 'tgt'),
              ('tcp:', session, 'IP1:port1', '-1', 'tgt1')]
         ]
+        utils.ISCSI_SUPPORTS_MANUAL_SCAN = None
         with mock.patch.object(self.connector, '_execute') as exec_mock:
             exec_mock.side_effect = [('', 'error'), ('', None),
                                      ('', None), ('', None),
@@ -1027,6 +1031,7 @@ Setting up iSCSI targets: unused
         # True refers to "manual scans", since the call to update
         # node.session.scan didn't fail they are set to manual
         self.assertEqual((session, True), res)
+        self.assertTrue(utils.ISCSI_SUPPORTS_MANUAL_SCAN)
         prefix = 'iscsiadm -m node -T tgt1 -p ip1:port1'
         expected_cmds = [
             prefix,
@@ -1048,9 +1053,11 @@ Setting up iSCSI targets: unused
         con_props = self.CON_PROPS.copy()
         con_props.update(auth_method='CHAP', auth_username='user',
                          auth_password='pwd')
+        utils.ISCSI_SUPPORTS_MANUAL_SCAN = None
         res = self.connector._connect_to_iscsi_portal(con_props)
         # False refers to "manual scans", so we have manual iscsi scans
         self.assertEqual((session, True), res)
+        self.assertTrue(utils.ISCSI_SUPPORTS_MANUAL_SCAN)
         prefix = 'iscsiadm -m node -T tgt1 -p ip1:port1'
         expected_cmds = [
             prefix,

os_brick/tests/initiator/test_utils.py

@@ -0,0 +1,62 @@
+# Copyright 2018 Red Hat, Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import mock
+
+from os_brick.initiator import utils
+from os_brick.tests import base
+
+
+class InitiatorUtilsTestCase(base.TestCase):
+
+    @mock.patch('os.name', 'nt')
+    def test_check_manual_scan_windows(self):
+        self.assertFalse(utils.check_manual_scan())
+
+    @mock.patch('os.name', 'posix')
+    @mock.patch('oslo_concurrency.processutils.execute')
+    def test_check_manual_scan_supported(self, mock_exec):
+        self.assertTrue(utils.check_manual_scan())
+        mock_exec.assert_called_once_with('grep', '-F', 'node.session.scan',
+                                          '/sbin/iscsiadm')
+
+    @mock.patch('os.name', 'posix')
+    @mock.patch('oslo_concurrency.processutils.execute',
+                side_effect=utils.putils.ProcessExecutionError)
+    def test_check_manual_scan_not_supported(self, mock_exec):
+        self.assertFalse(utils.check_manual_scan())
+        mock_exec.assert_called_once_with('grep', '-F', 'node.session.scan',
+                                          '/sbin/iscsiadm')
+
+    @mock.patch('oslo_concurrency.lockutils.lock')
+    def test_guard_connection_manual_scan_support(self, mock_lock):
+        utils.ISCSI_SUPPORTS_MANUAL_SCAN = True
+        # We confirm that shared_targets is ignored
+        with utils.guard_connection({'shared_targets': True}):
+            mock_lock.assert_not_called()
+
+    @mock.patch('oslo_concurrency.lockutils.lock')
+    def test_guard_connection_manual_scan_unsupported_not_shared(self,
+                                                                 mock_lock):
+        utils.ISCSI_SUPPORTS_MANUAL_SCAN = False
+        with utils.guard_connection({'shared_targets': False}):
+            mock_lock.assert_not_called()
+
+    @mock.patch('oslo_concurrency.lockutils.lock')
+    def test_guard_connection_manual_scan_unsupported_hared(self, mock_lock):
+        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-')