patches/openstack/os-brick/attach-globalid.patch

From eeea4310317d10a6f730777e1e0b1cb73cb004d4 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     |  81 ++++++++--
 .../initiator/connectors/test_storpool.py     | 151 +++++++++++-------
 2 files changed, 165 insertions(+), 67 deletions(-)

diff --git a/os_brick/initiator/connectors/storpool.py b/os_brick/initiator/connectors/storpool.py
index e1bd55c..72d2acf 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,35 @@ class StorPoolConnector(base.BaseLinuxConnector):
         else:
             self._attach = None
 
+    def _detach_retry(self, sp_ourid, volume):
+        """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 +110,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 +171,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..9cc1076 100644
--- a/os_brick/tests/initiator/connectors/test_storpool.py
+++ b/os_brick/tests/initiator/connectors/test_storpool.py
@@ -27,44 +27,13 @@ def volumeNameExt(vid):
 
 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)
@@ -101,6 +70,7 @@ class StorPoolConnectorTestCase(test_connector.ConnectorTestCase):
             'client_id': 1,
             'access_mode': 'rw',
         }
+        self.fakeGlobalId = 'OneNiceGlobalId'
         self.fakeConnection = None
         self.fakeSize = 1024 * 1024 * 1024
 
@@ -109,30 +79,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__'])
+        api.volumeInfo = mock.Mock(
+            return_value={'data': {'globalId': self.fakeGlobalId}})
+
+        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]}]}
+
+            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 +142,53 @@ 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__'])
+        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)
+
     def test_extend_volume(self):
         if self.fakeConnection is None:
             self.test_connect_volume()
-- 
2.25.1