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