From 3e0f383327b02d0e0cfa49b7693be94c0be3a09a Mon Sep 17 00:00:00 2001 From: mulhern Date: Tue, 30 Jan 2024 13:02:28 -0500 Subject: [PATCH 1/5] Remove redundant remove_stratis_dm_devices() The _clean_up method always throws one of these in, so there is no point in doing it at the end of the test. Signed-off-by: mulhern --- tests/client-dbus/tests/udev/test_udev.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/client-dbus/tests/udev/test_udev.py b/tests/client-dbus/tests/udev/test_udev.py index 0bb3685041..91ce680fd6 100644 --- a/tests/client-dbus/tests/udev/test_udev.py +++ b/tests/client-dbus/tests/udev/test_udev.py @@ -124,8 +124,6 @@ def _test_driver(self, number_of_pools, dev_count_pool): for name in pool_data: self.wait_for_pools(1, name=name) - remove_stratis_dm_devices() - def test_generic(self): """ See _test_driver for description. @@ -199,8 +197,6 @@ def _single_pool(self, num_devices, *, num_hotplugs=0): self.wait_for_pools(1) - remove_stratis_dm_devices() - def test_simultaneous(self): """ See documentation for _single_pool. @@ -281,8 +277,6 @@ def _simple_initial_discovery_test( self.wait_for_pools(1) - remove_stratis_dm_devices() - def test_encryption_simple_initial_discovery(self): """ See documentation for _simple_initial_discovery_test. @@ -404,8 +398,6 @@ def _simple_event_test(self, *, key_spec=None): # pylint: disable=too-many-loca self.wait_for_pools(1) - remove_stratis_dm_devices() - def test_simple_event(self): """ See documentation for _simple_event_test. @@ -557,8 +549,6 @@ def test_duplicate_pool_name( self.wait_for_pools(num_pools) - remove_stratis_dm_devices() - class UdevTest6(UdevTest): """ From 0a8578d95bd3223e07e720433f79c01260531f46 Mon Sep 17 00:00:00 2001 From: mulhern Date: Tue, 30 Jan 2024 13:24:33 -0500 Subject: [PATCH 2/5] Remove remove_stratis_dm_devices entirely It can be made redundant by having remove_stratis_setup raise an exception if there are still devices remaining. Also, this supports slightly better encapsulation. Signed-off-by: mulhern --- tests/client-dbus/tests/udev/_dm.py | 3 +++ tests/client-dbus/tests/udev/_utils.py | 15 ++------------- tests/client-dbus/tests/udev/test_udev.py | 20 ++++++++++---------- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/tests/client-dbus/tests/udev/_dm.py b/tests/client-dbus/tests/udev/_dm.py index f99d83da77..a71168348c 100644 --- a/tests/client-dbus/tests/udev/_dm.py +++ b/tests/client-dbus/tests/udev/_dm.py @@ -63,6 +63,9 @@ def remove_stratis_setup(): if _remove_device(dev): devices.remove(dev) + if _get_stratis_devices() != []: + raise RuntimeError("Some devices were not removed") + if __name__ == "__main__": remove_stratis_setup() diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 88a5067f64..75bd611202 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -43,7 +43,7 @@ ) from stratisd_client_dbus._constants import TOP_OBJECT -from ._dm import _get_stratis_devices, remove_stratis_setup +from ._dm import remove_stratis_setup from ._loopback import LoopBackDevices _STRATISD = os.environ["STRATISD"] @@ -232,17 +232,6 @@ def processes(name): pass -def remove_stratis_dm_devices(): - """ - Remove Stratis device mapper devices, fail with a runtime error if - some have been missed. - :raises RuntimeError: if some devices are remaining - """ - remove_stratis_setup() - if _get_stratis_devices() != []: - raise RuntimeError("Some devices were not removed") - - class _Service: """ Start and stop stratisd. @@ -429,7 +418,7 @@ def _clean_up(self): process.terminate() psutil.wait_procs(stratisds) - remove_stratis_dm_devices() + remove_stratis_setup() self._lb_mgr.destroy_all() def wait_for_pools(self, expected_num, *, name=None): diff --git a/tests/client-dbus/tests/udev/test_udev.py b/tests/client-dbus/tests/udev/test_udev.py index 91ce680fd6..2884eb2a1f 100644 --- a/tests/client-dbus/tests/udev/test_udev.py +++ b/tests/client-dbus/tests/udev/test_udev.py @@ -23,6 +23,7 @@ from stratisd_client_dbus._constants import TOP_OBJECT from stratisd_client_dbus._stratisd_constants import EncryptionMethod +from ._dm import remove_stratis_setup from ._loopback import UDEV_ADD_EVENT, UDEV_REMOVE_EVENT from ._utils import ( CRYPTO_LUKS_FS_TYPE, @@ -33,7 +34,6 @@ create_pool, get_devnodes, random_string, - remove_stratis_dm_devices, settle, wait_for_udev, wait_for_udev_count, @@ -79,7 +79,7 @@ def _test_driver(self, number_of_pools, dev_count_pool): create_pool(pool_name, self._lb_mgr.device_files(device_tokens)) pool_data[pool_name] = device_tokens - remove_stratis_dm_devices() + remove_stratis_setup() all_tokens = [ dev for device_tokens in pool_data.values() for dev in device_tokens @@ -91,7 +91,7 @@ def _test_driver(self, number_of_pools, dev_count_pool): with ServiceContextManager(): self.wait_for_pools(number_of_pools) - remove_stratis_dm_devices() + remove_stratis_setup() self._lb_mgr.unplug(all_tokens) @@ -168,12 +168,12 @@ def _single_pool(self, num_devices, *, num_hotplugs=0): self.assertEqual(len(device_object_paths), len(devnodes)) wait_for_udev(STRATIS_FS_TYPE, get_devnodes(device_object_paths)) - remove_stratis_dm_devices() + remove_stratis_setup() with ServiceContextManager(): self.wait_for_pools(1) - remove_stratis_dm_devices() + remove_stratis_setup() self._lb_mgr.unplug(device_tokens) @@ -255,7 +255,7 @@ def _simple_initial_discovery_test( pool_uuid = Pool.Properties.Uuid.Get(get_object(pool_object_path)) if take_down_dm: - remove_stratis_dm_devices() + remove_stratis_setup() with OptionalKeyServiceContextManager(key_spec=key_spec): ((changed, _), exit_code, _) = Manager.Methods.StartPool( @@ -340,7 +340,7 @@ def _simple_event_test(self, *, key_spec=None): # pylint: disable=too-many-loca self.wait_for_pools(1) pool_uuid = Pool.Properties.Uuid.Get(get_object(pool_object_path)) - remove_stratis_dm_devices() + remove_stratis_setup() self._lb_mgr.unplug(device_tokens) wait_for_udev(udev_wait_type, []) @@ -466,7 +466,7 @@ def test_duplicate_pool_name( pool_tokens.append(this_pool) - remove_stratis_dm_devices() + remove_stratis_setup() self._lb_mgr.unplug(this_pool) @@ -592,7 +592,7 @@ def _simple_stop_test(self): self.wait_for_pools(2) - remove_stratis_dm_devices() + remove_stratis_setup() with OptionalKeyServiceContextManager(): self.wait_for_pools(2) @@ -616,7 +616,7 @@ def _simple_stop_test(self): 1, ) - remove_stratis_dm_devices() + remove_stratis_setup() with OptionalKeyServiceContextManager(): self.wait_for_pools(1) From afea88e4d70e66ad9304f18c1da65c955505ad42 Mon Sep 17 00:00:00 2001 From: mulhern Date: Mon, 5 Feb 2024 21:39:58 -0500 Subject: [PATCH 3/5] Set timeout of 30 sec after interrupt signal sent Without, the test will wait indefinitely for stratisd to handle the signal. Signed-off-by: mulhern --- tests/client-dbus/tests/udev/_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 75bd611202..78d64b2b71 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -286,7 +286,7 @@ def stop_service(self): :return: None """ self._service.send_signal(signal.SIGINT) - self._service.wait() + self._service.wait(timeout=30) if next(processes("stratisd"), None) is not None: raise RuntimeError("Failed to stop stratisd service") From 4c54c3b980039ebe3adde026cb07c4c08a64341e Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 31 Jan 2024 09:13:14 -0500 Subject: [PATCH 4/5] Kill stratisd process if a terminate has not worked Signed-off-by: mulhern --- tests/client-dbus/tests/udev/_utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 78d64b2b71..8a034afa3b 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -416,7 +416,9 @@ def _clean_up(self): stratisds = list(processes("stratisd")) for process in stratisds: process.terminate() - psutil.wait_procs(stratisds) + (_, alive) = psutil.wait_procs(stratisds, timeout=10) + for process in alive: + process.kill() remove_stratis_setup() self._lb_mgr.destroy_all() From 4db2c9994e69f2445d7a10358bd9dbfb2505359d Mon Sep 17 00:00:00 2001 From: mulhern Date: Wed, 31 Jan 2024 13:30:48 -0500 Subject: [PATCH 5/5] Add some Python logging in test infrastructure Signed-off-by: mulhern --- tests/client-dbus/tests/udev/_utils.py | 6 ++++++ tests/client-dbus/tests/udev/test_udev.py | 3 +++ 2 files changed, 9 insertions(+) diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 8a034afa3b..529690b9d1 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -16,6 +16,7 @@ """ # isort: STDLIB +import logging import os import random import signal @@ -415,9 +416,14 @@ def _clean_up(self): """ stratisds = list(processes("stratisd")) for process in stratisds: + logging.warning("stratisd process %s still running, terminating", process) process.terminate() (_, alive) = psutil.wait_procs(stratisds, timeout=10) for process in alive: + logging.warning( + "stratisd process %s did not respond to terminate signal, killing", + process, + ) process.kill() remove_stratis_setup() diff --git a/tests/client-dbus/tests/udev/test_udev.py b/tests/client-dbus/tests/udev/test_udev.py index 2884eb2a1f..caf8564508 100644 --- a/tests/client-dbus/tests/udev/test_udev.py +++ b/tests/client-dbus/tests/udev/test_udev.py @@ -16,6 +16,7 @@ """ # isort: STDLIB +import logging import random # isort: LOCAL @@ -39,6 +40,8 @@ wait_for_udev_count, ) +logging.basicConfig(level=logging.INFO) + class UdevTest1(UdevTest): """