From 315448f05b310d4e4b2bad084ddcdc888e0c2cd0 Mon Sep 17 00:00:00 2001
From: Biser Milanov <biser.milanov@storpool.com>
Date: Tue, 23 May 2023 17:55:09 +0300
Subject: [PATCH] storpool.py: Use StorPool's API for Attach/Detach

Drop the use of the node-scoped file that helped manage volumes on that
node.

Volumes are now attached and detached by calling the StorPool API
directly.

Additionally refactor tests to reflect this.

Change-Id: I0fabcde1dc249b9b09f99ee0b7d759432972314a
---
 os_brick/initiator/connectors/storpool.py     |  84 +++++--
 .../initiator/connectors/test_storpool.py     | 219 +++++++++++++-----
 2 files changed, 236 insertions(+), 67 deletions(-)

diff --git a/os_brick/initiator/connectors/storpool.py b/os_brick/initiator/connectors/storpool.py
index e1bd55c..9752ad9 100644
--- a/os_brick/initiator/connectors/storpool.py
+++ b/os_brick/initiator/connectors/storpool.py
@@ -26,6 +26,7 @@ from os_brick import utils
 LOG = logging.getLogger(__name__)
 
 spopenstack = importutils.try_import('storpool.spopenstack')
+spapi = importutils.try_import('storpool.spapi')
 
 
 class StorPoolConnector(base.BaseLinuxConnector):
@@ -37,6 +38,10 @@ class StorPoolConnector(base.BaseLinuxConnector):
         super(StorPoolConnector, self).__init__(root_helper, driver=driver,
                                                 *args, **kwargs)
 
+        if spapi is None:
+            raise exception.BrickException(
+                'Could not import the StorPool API bindings')
+
         if spopenstack is not None:
             try:
                 self._attach = spopenstack.AttachDB(log=LOG)
@@ -46,6 +51,38 @@ class StorPoolConnector(base.BaseLinuxConnector):
         else:
             self._attach = None
 
+    def _detach_retry(self, sp_ourid, volume):
+        """Retry detaching.
+
+        Retries attempt to handle LUKS tests-related failures:
+          busy: volume ... open at ...
+        """
+
+        count = 10
+        while True:
+            try:
+                force = count == 0
+                self._attach.api().volumesReassignWait(
+                    {
+                        "reassign": [{
+                            "volume": volume,
+                            "detach": [sp_ourid],
+                            "force": force,
+                        }]
+                    }
+                )
+                break
+            except spapi.ApiError as exc:
+                if (
+                    exc.name in ("busy", "invalidParam")
+                    and "is open at" in exc.desc
+                ):
+                    assert count > 0
+                    time.sleep(0.2)
+                    count -= 1
+                else:
+                    raise
+
     @staticmethod
     def get_connector_properties(root_helper, *args, **kwargs):
         """The StorPool connector properties."""
@@ -76,16 +113,29 @@ class StorPoolConnector(base.BaseLinuxConnector):
         if mode is None or mode not in ('rw', 'ro'):
             raise exception.BrickException(
                 'Invalid access_mode specified in the connection data.')
-        req_id = 'brick-%s-%s' % (client_id, volume_id)
-        self._attach.add(req_id, {
-            'volume': volume,
-            'type': 'brick',
-            'id': req_id,
-            'rights': 1 if mode == 'ro' else 2,
-            'volsnap': False
-        })
-        self._attach.sync(req_id, None)
-        return {'type': 'block', 'path': '/dev/storpool/' + volume}
+        try:
+            sp_ourid = self._attach.config()["SP_OURID"]
+        except KeyError:
+            raise exception.BrickException(
+                'SP_OURID missing, cannot connect volume %s' % volume_id)
+
+        try:
+            self._attach.api().volumesReassignWait(
+                {"reassign": [{"volume": volume, mode: [sp_ourid]}]})
+        except Exception as exc:
+            raise exception.BrickException(
+                'Communication with the StorPool API '
+                'failed: %s' % (exc)) from exc
+
+        try:
+            volume_info = self._attach.api().volumeInfo(volume)
+        except Exception as exc:
+            raise exception.BrickException(
+                'Communication with the StorPool API '
+                'failed: %s' % (exc)) from exc
+
+        sp_global_id = volume_info.globalId
+        return {'type': 'block', 'path': '/dev/storpool-byid/' + sp_global_id}
 
     @utils.connect_volume_undo_prepare_result(unlink_after=True)
     def disconnect_volume(self, connection_properties, device_info,
@@ -124,9 +174,17 @@ class StorPoolConnector(base.BaseLinuxConnector):
             raise exception.BrickException(
                 'Invalid StorPool connection data, no volume ID specified.')
         volume = self._attach.volumeName(volume_id)
-        req_id = 'brick-%s-%s' % (client_id, volume_id)
-        self._attach.sync(req_id, volume)
-        self._attach.remove(req_id)
+        try:
+            sp_ourid = self._attach.config()["SP_OURID"]
+        except KeyError:
+            raise exception.BrickException(
+                'SP_OURID missing, cannot disconnect volume %s' % volume)
+        try:
+            self._detach_retry(sp_ourid, volume)
+        except Exception as exc:
+            raise exception.BrickException(
+                'Communication with the StorPool API '
+                'failed: %s' % (exc)) from exc
 
     def get_search_path(self):
         return '/dev/storpool'
diff --git a/os_brick/tests/initiator/connectors/test_storpool.py b/os_brick/tests/initiator/connectors/test_storpool.py
index 614deba..b539e2b 100644
--- a/os_brick/tests/initiator/connectors/test_storpool.py
+++ b/os_brick/tests/initiator/connectors/test_storpool.py
@@ -25,46 +25,35 @@ def volumeNameExt(vid):
     return 'os--volume--{id}'.format(id=vid)
 
 
+def faulty_api(req):
+    faulty_api.real_fn(req)
+    if faulty_api.fail_count > 0:
+        faulty_api.fail_count -= 1
+        raise MockApiError("busy",
+                           "'os--volume--sp-vol-1' is open at client 19")
+
+
+class MockApiError(Exception):
+    def __init__(self, name, desc):
+        super(MockApiError, self).__init__()
+        self.name = name
+        self.desc = desc
+
+
+class MockVolumeInfo(object):
+    def __init__(self, global_id):
+        self.globalId = global_id
+
+
 class MockStorPoolADB(object):
     def __init__(self, log):
-        self.requests = {}
-        self.attached = {}
+        pass
 
     def api(self):
         pass
 
-    def add(self, req_id, req):
-        if req_id in self.requests:
-            raise Exception('Duplicate MockStorPool request added')
-        self.requests[req_id] = req
-
-    def remove(self, req_id):
-        req = self.requests.get(req_id, None)
-        if req is None:
-            raise Exception('Unknown MockStorPool request removed')
-        elif req['volume'] in self.attached:
-            raise Exception('Removing attached MockStorPool volume')
-        del self.requests[req_id]
-
-    def sync(self, req_id, detached):
-        req = self.requests.get(req_id, None)
-        if req is None:
-            raise Exception('Unknown MockStorPool request synced')
-        volume = req.get('volume', None)
-        if volume is None:
-            raise Exception('MockStorPool request without volume')
-
-        if detached is None:
-            if volume in self.attached:
-                raise Exception('Duplicate MockStorPool request synced')
-            self.attached[volume] = req
-        else:
-            if volume != detached:
-                raise Exception(
-                    'Mismatched volumes on a MockStorPool request removal')
-            elif detached not in self.attached:
-                raise Exception('MockStorPool request not attached yet')
-            del self.attached[detached]
+    def config(self):
+        return {"SP_OURID": 1}
 
     def volumeName(self, vid):
         return volumeNameExt(vid)
@@ -74,6 +63,10 @@ spopenstack = mock.Mock()
 spopenstack.AttachDB = MockStorPoolADB
 connector.spopenstack = spopenstack
 
+spapi = mock.Mock()
+spapi.ApiError = MockApiError
+connector.spapi = spapi
+
 
 class StorPoolConnectorTestCase(test_connector.ConnectorTestCase):
     def volumeName(self, vid):
@@ -101,6 +94,8 @@ class StorPoolConnectorTestCase(test_connector.ConnectorTestCase):
             'client_id': 1,
             'access_mode': 'rw',
         }
+        self.fakeGlobalId = 'OneNiceGlobalId'
+        self.api_calls_retry_max = 10
         self.fakeConnection = None
         self.fakeSize = 1024 * 1024 * 1024
 
@@ -109,30 +104,56 @@ class StorPoolConnectorTestCase(test_connector.ConnectorTestCase):
         self.adb = self.connector._attach
 
     def test_connect_volume(self):
-        self.assertNotIn(self.volumeName(self.fakeProp['volume']),
-                         self.adb.attached)
-        conn = self.connector.connect_volume(self.fakeProp)
-        self.assertIn('type', conn)
-        self.assertIn('path', conn)
-        self.assertIn(self.volumeName(self.fakeProp['volume']),
-                      self.adb.attached)
-
-        self.assertEqual(self.connector.get_search_path(), '/dev/storpool')
-        paths = self.connector.get_volume_paths(self.fakeProp)
-        self.assertEqual(len(paths), 1)
-        self.assertEqual(paths[0],
-                         "/dev/storpool/" +
-                         self.volumeName(self.fakeProp['volume']))
-        self.fakeConnection = conn
+        volume_name = volumeNameExt(self.fakeProp['volume'])
+        api = mock.MagicMock(spec=['volumesReassignWait', 'volumeInfo'])
+        api.volumesReassignWait = mock.MagicMock(spec=['__call__'])
+        volume_info = MockVolumeInfo(self.fakeGlobalId)
+        api.volumeInfo = mock.Mock(return_value=volume_info)
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+
+            conn = self.connector.connect_volume(self.fakeProp)
+            self.assertIn('type', conn)
+            self.assertIn('path', conn)
+            self.assertEqual(conn['path'],
+                             '/dev/storpool-byid/' + self.fakeGlobalId)
+            self.assertEqual(len(api.volumesReassignWait.mock_calls), 1)
+            self.assertEqual(api.volumesReassignWait.mock_calls[0], mock.call(
+                {'reassign': [{'volume': 'os--volume--sp-vol-1', 'rw': [1]}]}))
+            self.assertEqual(len(api.volumeInfo.mock_calls), 1)
+            self.assertEqual(api.volumeInfo.mock_calls[0],
+                             mock.call(volume_name))
+
+            self.assertEqual(self.connector.get_search_path(), '/dev/storpool')
+
+            paths = self.connector.get_volume_paths(self.fakeProp)
+            self.assertEqual(len(paths), 1)
+            self.assertEqual(paths[0],
+                             "/dev/storpool/" +
+                             self.volumeName(self.fakeProp['volume']))
+            self.fakeConnection = conn
 
     def test_disconnect_volume(self):
         if self.fakeConnection is None:
             self.test_connect_volume()
-        self.assertIn(self.volumeName(self.fakeProp['volume']),
-                      self.adb.attached)
-        self.connector.disconnect_volume(self.fakeProp, None)
-        self.assertNotIn(self.volumeName(self.fakeProp['volume']),
-                         self.adb.attached)
+
+        api = mock.MagicMock(spec=['volumesReassignWait'])
+        api.volumesReassignWait = mock.MagicMock(spec=['__call__'])
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+            reassign_wait_data = {'reassign': [
+                {'volume': volumeNameExt(self.fakeProp['volume']),
+                 'detach': [1], 'force': False}]}
+
+            self.connector.disconnect_volume(self.fakeProp, None)
+            self.assertEqual(api.volumesReassignWait.mock_calls[0],
+                             (mock.call(reassign_wait_data)))
 
     def test_connect_exceptions(self):
         """Raise exceptions on missing connection information"""
@@ -146,6 +167,96 @@ class StorPoolConnectorTestCase(test_connector.ConnectorTestCase):
                 self.assertRaises(exception.BrickException,
                                   self.connector.disconnect_volume, c, None)
 
+    def test_sp_ourid_exceptions(self):
+        """Raise exceptions on missing SP_OURID"""
+        with mock.patch.object(self.connector._attach, 'config')\
+                as fake_config:
+            fake_config.return_value = {}
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.connect_volume, self.fakeProp)
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.disconnect_volume, self.fakeProp,
+                              None)
+
+    def test_sp_api_exceptions(self):
+        """Handle SP API exceptions"""
+        api = mock.MagicMock(spec=['volumesReassignWait', 'volumeInfo'])
+        api.volumesReassignWait = mock.MagicMock(spec=['__call__'])
+        # The generic exception should bypass the SP API exception handling
+        api.volumesReassignWait.side_effect = Exception()
+        api.volumeInfo = mock.MagicMock(spec=['__call__'])
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.connect_volume, self.fakeProp)
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.disconnect_volume, self.fakeProp,
+                              None)
+
+        api.volumesReassignWait.side_effect = ""
+        api.volumeInfo = Exception()
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.connect_volume, self.fakeProp)
+
+            self.assertRaises(exception.BrickException,
+                              self.connector.disconnect_volume, self.fakeProp,
+                              None)
+
+        # Test the retry logic
+        faulty_api.fail_count = self.api_calls_retry_max - 1
+        faulty_api.real_fn = mock.MagicMock(spec=['__call__'])
+        api.volumesReassignWait = faulty_api
+        api.volumeInfo = mock.MagicMock(spec=['__call__'])
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+            reassign_wait_data = {'reassign': [
+                {'volume': volumeNameExt(self.fakeProp['volume']),
+                 'detach': [1], 'force': False}]}
+
+            self.connector.disconnect_volume(self.fakeProp, None)
+            self.assertEqual(self.api_calls_retry_max,
+                             len(faulty_api.real_fn.mock_calls))
+            for mock_call in faulty_api.real_fn.mock_calls:
+                self.assertEqual(mock_call, mock.call(reassign_wait_data))
+
+        faulty_api.fail_count = self.api_calls_retry_max
+        faulty_api.real_fn = mock.MagicMock(spec=['__call__'])
+        api.volumesReassignWait = faulty_api
+        api.volumeInfo = mock.MagicMock(spec=['__call__'])
+
+        with mock.patch.object(
+                self.adb, attribute='api', spec=['__call__']
+        ) as fake_api:
+            fake_api.return_value = api
+            reassign_wait_data = {'reassign': [
+                {'volume': volumeNameExt(self.fakeProp['volume']),
+                 'detach': [1], 'force': False}]}
+
+            self.connector.disconnect_volume(self.fakeProp, None)
+            self.assertEqual(self.api_calls_retry_max + 1,
+                             len(faulty_api.real_fn.mock_calls))
+            for mock_call in faulty_api.real_fn.mock_calls[:-1]:
+                self.assertEqual(mock_call, mock.call(reassign_wait_data))
+            reassign_wait_data['reassign'][0]['force'] = True
+            self.assertEqual(faulty_api.real_fn.mock_calls[-1],
+                             mock.call(reassign_wait_data))
+
     def test_extend_volume(self):
         if self.fakeConnection is None:
             self.test_connect_volume()
-- 
2.25.1