From 79c2f72387864f30708f4683e8974bafe2e94df4 Mon Sep 17 00:00:00 2001 From: Sandy Kaur Date: Wed, 26 Apr 2023 09:10:54 -0500 Subject: [PATCH 1/4] refactor(tests): subdirectory for functional tests This commit establishes a test subdirectory for functional tests, which rely on libRADOS and SPDK. Signed-off-by: Sandy Kaur --- tests/{ => functional}/__init__.py | 0 tests/{ => functional}/conftest.py | 0 tests/{ => functional}/test_cli.py | 0 tests/{ => functional}/test_multi_gateway.py | 0 tests/{ => functional}/test_server.py | 0 tests/{ => functional}/test_state.py | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename tests/{ => functional}/__init__.py (100%) rename tests/{ => functional}/conftest.py (100%) rename tests/{ => functional}/test_cli.py (100%) rename tests/{ => functional}/test_multi_gateway.py (100%) rename tests/{ => functional}/test_server.py (100%) rename tests/{ => functional}/test_state.py (100%) diff --git a/tests/__init__.py b/tests/functional/__init__.py similarity index 100% rename from tests/__init__.py rename to tests/functional/__init__.py diff --git a/tests/conftest.py b/tests/functional/conftest.py similarity index 100% rename from tests/conftest.py rename to tests/functional/conftest.py diff --git a/tests/test_cli.py b/tests/functional/test_cli.py similarity index 100% rename from tests/test_cli.py rename to tests/functional/test_cli.py diff --git a/tests/test_multi_gateway.py b/tests/functional/test_multi_gateway.py similarity index 100% rename from tests/test_multi_gateway.py rename to tests/functional/test_multi_gateway.py diff --git a/tests/test_server.py b/tests/functional/test_server.py similarity index 100% rename from tests/test_server.py rename to tests/functional/test_server.py diff --git a/tests/test_state.py b/tests/functional/test_state.py similarity index 100% rename from tests/test_state.py rename to tests/functional/test_state.py From e38c6095798f0b9d03e8fc47362f62a1729c2fde Mon Sep 17 00:00:00 2001 From: Sandy Kaur Date: Thu, 11 May 2023 10:36:57 -0500 Subject: [PATCH 2/4] fix: try to stop updates gracefully before exit Signed-off-by: Sandy Kaur --- control/server.py | 2 ++ control/state.py | 18 +++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/control/server.py b/control/server.py index bc740fc7..b0acf445 100644 --- a/control/server.py +++ b/control/server.py @@ -79,6 +79,8 @@ def __exit__(self, exc_type, exc_value, traceback): if exc_type is not None: self.logger.exception("GatewayServer exception occurred:") + self.gateway_state.stop_event.set() # Stop updates + if self.spdk_process is not None: self._stop_spdk() diff --git a/control/state.py b/control/state.py index b99664c3..daf8297a 100644 --- a/control/state.py +++ b/control/state.py @@ -319,6 +319,8 @@ class GatewayStateHandler: update_interval: Interval to periodically poll for updates update_timer: Timer to check for gateway state updates use_notify: Flag to indicate use of OMAP watch/notify + notify_event: Event to trigger immediate update + stop_event: Event to stop updates """ def __init__(self, config, local, omap, gateway_rpc_caller): @@ -335,6 +337,8 @@ def __init__(self, config, local, omap, gateway_rpc_caller): self.update_interval = 1 self.use_notify = self.config.getboolean("gateway", "state_update_notify") + self.notify_event = threading.Event() + self.stop_event = threading.Event() def add_bdev(self, bdev_name: str, val: str): """Adds a bdev to the state data stores.""" @@ -399,27 +403,27 @@ def delete_state(self): def start_update(self): """Initiates periodic polling and watch/notify for updates.""" - notify_event = threading.Event() if self.use_notify: # Register a watch on omap state - self.omap.register_watch(notify_event) + self.omap.register_watch(self.notify_event) # Start polling for state updates if self.update_timer is None: self.update_timer = threading.Thread(target=self._update_caller, - daemon=True, - args=(notify_event,)) + daemon=True) self.update_timer.start() else: self.logger.info("Update timer already set.") - def _update_caller(self, notify_event): + def _update_caller(self): """Periodically calls for update.""" while True: update_time = time.time() + self.update_interval self.update() - notify_event.wait(max(update_time - time.time(), 0)) - notify_event.clear() + self.notify_event.wait(max(update_time - time.time(), 0)) + self.notify_event.clear() + if self.stop_event.is_set(): + break def update(self): """Checks for updated omap state and initiates local update.""" From c50d878f1cdb48c034828e9d7473bbc1ec61a7a6 Mon Sep 17 00:00:00 2001 From: Sandy Kaur Date: Thu, 11 May 2023 10:39:12 -0500 Subject: [PATCH 3/4] test: state update component tests Signed-off-by: Sandy Kaur --- tests/component/__init__.py | 0 tests/component/test_state.py | 414 ++++++++++++++++++++++++++++++++++ 2 files changed, 414 insertions(+) create mode 100644 tests/component/__init__.py create mode 100644 tests/component/test_state.py diff --git a/tests/component/__init__.py b/tests/component/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/component/test_state.py b/tests/component/test_state.py new file mode 100644 index 00000000..e3b314d8 --- /dev/null +++ b/tests/component/test_state.py @@ -0,0 +1,414 @@ +import pytest +import time +from typing import Dict, List, NamedTuple +from unittest import mock +with mock.patch.dict("sys.modules", rados=mock.Mock()): + from control.state import ( + GatewayState, + LocalGatewayState, + OmapGatewayState, + GatewayStateHandler, + ) + + +class UpdateCall(NamedTuple): + """Holds a single component update.""" + + component_update: Dict[str, str] + is_add_req: bool + + +class StateUpdateTest(NamedTuple): + """Holds inputs and expected results of a state update.""" + + local: Dict[str, str] + omap: Dict[str, str] + calls: List[UpdateCall] + + +class TestGatewayStateHandler: + """Tests for GatewayStateHandler.""" + + @pytest.fixture(autouse=True) + def state_init_with_mocks(self) -> None: + """Initializes GatewayStateHandler with mocked arguments.""" + mock_config = mock.Mock() + mock_config.getint.return_value = 1 + self.state = GatewayStateHandler( + mock_config, + mock.Mock(spec=LocalGatewayState), + mock.Mock(spec=OmapGatewayState), + mock.Mock(), + ) + + @pytest.mark.parametrize( + "input", + [ + # Tests no change. + # + # Expects 0 calls to gateway_rpc_caller. + StateUpdateTest(local={}, + omap={OmapGatewayState.OMAP_VERSION_KEY: "2"}, + calls=[]), + # Tests key addition of an invalid prefix. + # + # Expects 0 calls to gateway_rpc_caller. + StateUpdateTest( + local={}, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + "foo": "bar", + }, + calls=[], + ), + # Tests key additions of a single valid prefix. + # + # Expects 1 call to gateway_rpc_caller containing a dictionary + # with the added keys and a flag (True) to indicate addition. + StateUpdateTest( + local={}, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_baz": "qux", + }, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_baz": "qux", + }, + is_add_req=True, + ) + ], + ), + # Tests key additions of every valid prefix. + # + # Expects 5 ordered calls to gateway_rpc_caller, each containing + # a dictionary with an added key and a flag (True) to indicate + # addition. + StateUpdateTest( + local={}, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + f"{GatewayState.HOST_PREFIX}_foo": "bar", + f"{GatewayState.NAMESPACE_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "bar", + f"{GatewayState.LISTENER_PREFIX}_foo": "bar", + }, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "bar" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.NAMESPACE_PREFIX}_foo": "bar" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.HOST_PREFIX}_foo": "bar" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.LISTENER_PREFIX}_foo": "bar" + }, + is_add_req=True, + ), + ], + ), + # Tests key removal of an invalid prefix. + # + # Expects 0 calls to gateway_rpc_caller. + StateUpdateTest( + local={"foo": "bar"}, + omap={OmapGatewayState.OMAP_VERSION_KEY: "2"}, + calls=[], + ), + # Tests key removals of a single valid prefix. + # + # Expects 1 call to gateway_rpc_caller containing a dictionary + # with the removed keys and a flag (False) to indicate removal. + StateUpdateTest( + local={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_baz": "qux", + }, + omap={OmapGatewayState.OMAP_VERSION_KEY: "2"}, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_baz": "qux", + }, + is_add_req=False, + ) + ], + ), + # Tests key removals of every valid prefix. + # + # Expects 5 ordered calls to gateway_rpc_caller, each containing + # a dictionary with a removed key and a flag (False) to indicate + # removal. + StateUpdateTest( + local={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.NAMESPACE_PREFIX}_foo": "bar", + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "bar", + f"{GatewayState.HOST_PREFIX}_foo": "bar", + f"{GatewayState.LISTENER_PREFIX}_foo": "bar", + }, + omap={OmapGatewayState.OMAP_VERSION_KEY: "2"}, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.LISTENER_PREFIX}_foo": "bar" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.HOST_PREFIX}_foo": "bar" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.NAMESPACE_PREFIX}_foo": "bar" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "bar" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar" + }, + is_add_req=False, + ), + ], + ), + # Tests value change on key of an invalid prefix. + # + # Expects 0 calls to gateway_rpc_caller. + StateUpdateTest( + local={"foo": "bar"}, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + "foo": "quux", + }, + calls=[], + ), + # Tests value changes on keys of a single valid prefix. + # + # Expects 2 ordered calls to gateway_rpc_caller. The first call + # contains a dictionary with the changed keys and a flag (False) + # to indicate removal. The second call contains a flag (True) to + # indicate addition with new values. + StateUpdateTest( + local={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.BDEV_PREFIX}_baz": "qux", + }, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + f"{GatewayState.BDEV_PREFIX}_foo": "quux", + f"{GatewayState.BDEV_PREFIX}_baz": "quuz", + }, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "quux", + f"{GatewayState.BDEV_PREFIX}_baz": "quuz", + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "quux", + f"{GatewayState.BDEV_PREFIX}_baz": "quuz", + }, + is_add_req=True, + ), + ], + ), + # Tests value changes on keys of every valid prefix. + # + # Expects 10 ordered calls to gateway_rpc_caller, 2 per prefix. + # The first call for each prefix contains a dictionary with the + # changed keys and a flag (False) to indicate removal. The + # second call contains a flag (True) to indicate addition with + # new values. Calls for removal are ordered by prefix and occur + # in reverse order of calls for addition. + StateUpdateTest( + local={ + f"{GatewayState.BDEV_PREFIX}_foo": "bar", + f"{GatewayState.NAMESPACE_PREFIX}_foo": "bar", + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "bar", + f"{GatewayState.HOST_PREFIX}_foo": "bar", + f"{GatewayState.LISTENER_PREFIX}_foo": "bar", + }, + omap={ + OmapGatewayState.OMAP_VERSION_KEY: "2", + f"{GatewayState.BDEV_PREFIX}_foo": "quux", + f"{GatewayState.NAMESPACE_PREFIX}_foo": "quux", + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "quux", + f"{GatewayState.HOST_PREFIX}_foo": "quux", + f"{GatewayState.LISTENER_PREFIX}_foo": "quux", + }, + calls=[ + UpdateCall( + component_update={ + f"{GatewayState.LISTENER_PREFIX}_foo": "quux" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.HOST_PREFIX}_foo": "quux" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.NAMESPACE_PREFIX}_foo": "quux" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "quux" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "quux" + }, + is_add_req=False, + ), + UpdateCall( + component_update={ + f"{GatewayState.BDEV_PREFIX}_foo": "quux" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.SUBSYSTEM_PREFIX}_foo": "quux" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.NAMESPACE_PREFIX}_foo": "quux" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.HOST_PREFIX}_foo": "quux" + }, + is_add_req=True, + ), + UpdateCall( + component_update={ + f"{GatewayState.LISTENER_PREFIX}_foo": "quux" + }, + is_add_req=True, + ), + ], + ), + ], + ) + def test_update_from_omap(self, input: StateUpdateTest): + """Confirms call order for local update to reflect changes in OMAP.""" + self.state.local.get_state.return_value = input.local + self.state.omap.get_local_version.return_value = 1 + self.state.omap.OMAP_VERSION_KEY = OmapGatewayState.OMAP_VERSION_KEY + self.state.omap.get_state.return_value = input.omap + self.state.update() + assert self.state.gateway_rpc_caller.call_args_list == [ + mock.call(i.component_update, i.is_add_req) for i in input.calls + ] + + def test_update_reset_local_state(self): + """Confirms reset of local state after update.""" + self.state.local.get_state.return_value = {} + self.state.omap.get_local_version.return_value = 1 + self.state.omap.OMAP_VERSION_KEY = OmapGatewayState.OMAP_VERSION_KEY + omap_dict = { + OmapGatewayState.OMAP_VERSION_KEY: "2", + f"{GatewayState.BDEV_PREFIX}_foo": "bar" + } + self.state.omap.get_state.return_value = omap_dict + self.state.update() + self.state.local.reset.assert_called_once_with(omap_dict) + + def test_update_reset_local_version(self): + """Confirms reset of local version after update.""" + self.state.local.get_state.return_value = {} + self.state.omap.get_local_version.return_value = 1 + self.state.omap.OMAP_VERSION_KEY = OmapGatewayState.OMAP_VERSION_KEY + omap_version = 2 + self.state.omap.get_state.return_value = { + OmapGatewayState.OMAP_VERSION_KEY: str(omap_version), + f"{GatewayState.BDEV_PREFIX}_foo": "bar" + } + self.state.update() + self.state.omap.set_local_version.assert_called_once_with(omap_version) + + @pytest.mark.parametrize("omap", [{ + OmapGatewayState.OMAP_VERSION_KEY: "0" + }, { + OmapGatewayState.OMAP_VERSION_KEY: "1" + }]) + def test_update_not_needed(self, omap: Dict[str, str]): + """Confirms lack of update when the local version >= OMAP version.""" + self.state.omap.get_local_version.return_value = 1 + self.state.omap.OMAP_VERSION_KEY = OmapGatewayState.OMAP_VERSION_KEY + self.state.omap.get_state.return_value = omap + self.state.update() + self.state.omap.set_local_version.assert_not_called() + + def test_update_caller_periodic(self): + """Confirms periodic call for update.""" + with mock.patch.object(self.state, "update") as mock_update: + self.state.update_interval = 1 + start = time.time() + self.state.start_update() + time.sleep(3) + self.state.stop_event.set() + self.state.update_timer.join() + expected_count = int(time.time() - start) + assert len(mock_update.call_args_list) == expected_count + + def test_update_caller_notify(self): + """Confirms notify signal interrupts periodic call for update.""" + with mock.patch.object(self.state, "update") as mock_update: + self.state.update_interval = 1 + start = time.time() + self.state.start_update() + self.state.notify_event.set() + time.sleep(3) + self.state.stop_event.set() + self.state.update_timer.join() + expected_count = int(time.time() - start) + 1 + assert len(mock_update.call_args_list) == expected_count From 931a7a53cbc4af150cc28eb1af271a0830846f76 Mon Sep 17 00:00:00 2001 From: Sandy Kaur Date: Thu, 11 May 2023 11:04:41 -0500 Subject: [PATCH 4/4] feat: github action for running component tests Signed-off-by: Sandy Kaur --- .github/workflows/test-components.yml | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 .github/workflows/test-components.yml diff --git a/.github/workflows/test-components.yml b/.github/workflows/test-components.yml new file mode 100644 index 00000000..7c5cb810 --- /dev/null +++ b/.github/workflows/test-components.yml @@ -0,0 +1,26 @@ +# Runs unit-style tests when a PR is opened/reopened/updated on the devel branch + +name: Test components +on: + pull_request: + branches: + - devel + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Set up Python 3.9 + uses: actions/setup-python@v1 + with: + python-version: 3.9 + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install .[test] + + - name: Run component tests with pytest + run: pytest tests/component