diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 45f5a07ec6..972cf61439 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -12,7 +12,7 @@ make fmt-ci && make clippy && make yamllint && make tmtlint && - make pylint && + make lint && make check-typos || exit 1 export PYTHONPATH=$PWD/tests/client-dbus/src diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ca03810fa1..7520b17534 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -192,11 +192,17 @@ jobs: - name: Install dependencies run: > dnf install -y + bandit make + pip pylint python3-dbus - - name: Run pylint - run: make -f Makefile pylint + - name: Install pyright + run: pip install --user pyright + - name: Run lint + run: > + PATH=${PATH}:/github/home/.local/bin + make -f Makefile lint python-based-tests: runs-on: ubuntu-22.04 diff --git a/.github/workflows/support.yml b/.github/workflows/support.yml index 0f7adcf087..3534145df6 100644 --- a/.github/workflows/support.yml +++ b/.github/workflows/support.yml @@ -39,6 +39,7 @@ jobs: matrix: include: - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -47,7 +48,9 @@ jobs: python3-pyudev python3-semantic_version python3-tenacity - task: PYTHONPATH=./src make -f Makefile lint + task: > + PATH=${PATH}:/github/home/.local/bin + PYTHONPATH=./src make -f Makefile lint working-directory: ./tests/client-dbus - dependencies: black python3-isort task: make -f Makefile fmt-ci @@ -64,7 +67,10 @@ jobs: dnf install -y make ncurses + pip ${{ matrix.dependencies }} + - name: Install pyright + run: pip install --user pyright - name: Run test run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/.github/workflows/weekly.yml b/.github/workflows/weekly.yml index ebc6d0c872..84ba2d38be 100644 --- a/.github/workflows/weekly.yml +++ b/.github/workflows/weekly.yml @@ -17,6 +17,7 @@ jobs: include: # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -25,7 +26,9 @@ jobs: python3-pyudev python3-semantic_version python3-tenacity - task: PYTHONPATH=./src make -f Makefile lint + task: > + PATH=${PATH}:/github/home/.local/bin + PYTHONPATH=./src make -f Makefile lint working-directory: ./tests/client-dbus - dependencies: black python3-isort task: make -f Makefile fmt-ci @@ -45,10 +48,13 @@ jobs: dnf install -y make ncurses + pip python-unversioned-command ${{ matrix.dependencies }} - name: Display Python version run: python --version + - name: Install pyright + run: pip install --user pyright - name: ${{ matrix.task }} run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/Makefile b/Makefile index 2edbbf197f..425ec7a6cd 100644 --- a/Makefile +++ b/Makefile @@ -430,8 +430,10 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl cargo clippy ${CLIPPY_OPTS} ## Lint Python parts of the source code -pylint: +lint: pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm + bandit ./src/bin/utils/stratis-decode-dm --skip B101 + pyright ./src/bin/utils/stratis-decode-dm .PHONY: audit @@ -477,7 +479,7 @@ pylint: install-udev-binaries install-udev-cfg license - pylint + lint test test-valgrind test-loop diff --git a/tests/client-dbus/Makefile b/tests/client-dbus/Makefile index 8a9cddbf7b..b0528087c0 100644 --- a/tests/client-dbus/Makefile +++ b/tests/client-dbus/Makefile @@ -1,9 +1,17 @@ UNITTEST_OPTS = --verbose +# Ignore bandit B404 errors. Any import of the subprocess module causes this +# error. We know what we are doing when we import that module and do not +# need to be warned. +BANDIT_SKIP = --skip B404,B603,B311 + .PHONY: lint lint: pylint src/stratisd_client_dbus --ignore=_introspect.py pylint tests --disable=R0801 + bandit --recursive ./src ${BANDIT_SKIP} + bandit --recursive ./tests ${BANDIT_SKIP},B101 + pyright .PHONY: startup-tests startup-tests: diff --git a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py index 81dcb4e28b..ef680f0c64 100644 --- a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py +++ b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py @@ -16,7 +16,7 @@ """ # isort: STDLIB -import xml.etree.ElementTree as ET +import xml.etree.ElementTree as ET # nosec B405 # isort: FIRSTPARTY from dbus_client_gen import managed_object_class, mo_query_builder @@ -31,9 +31,9 @@ ) from ._introspect import SPECS -_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) -_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) -_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) +_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) # nosec B314 +_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) # nosec B314 +_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) # nosec B314 pools = mo_query_builder(_POOL_SPEC) filesystems = mo_query_builder(_FILESYSTEM_SPEC) @@ -46,11 +46,15 @@ ObjectManager = make_class( "ObjectManager", - ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), + ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), # nosec B314 TIME_OUT, ) -Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT) -Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT) +Report = make_class( + "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT # nosec B314 +) +Manager = make_class( + "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT # nosec B314 +) Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT) Pool = make_class("Pool", _POOL_SPEC, TIME_OUT) Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT) diff --git a/tests/client-dbus/tests/udev/_loopback.py b/tests/client-dbus/tests/udev/_loopback.py index 94e0a27cca..9b4aafe8d9 100644 --- a/tests/client-dbus/tests/udev/_loopback.py +++ b/tests/client-dbus/tests/udev/_loopback.py @@ -105,6 +105,8 @@ def create_devices(self, number): :return: list of keys for the devices :rtype: list of uuid.UUID """ + assert self.dir is not None + tokens = [] first_creation = self.count == 0 @@ -220,6 +222,8 @@ def destroy_all(self): Detach all the devices and delete the file(s) and directory! :return: None """ + assert self.dir is not None + self.destroy_devices() os.rmdir(self.dir) self.dir = None diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 40c4fcd935..35e142cadc 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -31,7 +31,14 @@ import dbus import psutil import pyudev -from tenacity import Retrying, retry_if_exception_type, stop_after_delay, wait_fixed +from tenacity import ( + RetryError, + Retrying, + retry_if_exception_type, + stop_after_attempt, + stop_after_delay, + wait_fixed, +) # isort: LOCAL from stratisd_client_dbus import ( @@ -235,7 +242,7 @@ def settle(): :return: None """ time.sleep(2) - subprocess.check_call(["udevadm", "settle"]) + subprocess.check_call(["/usr/bin/udevadm", "settle"]) def wait_for_udev_count(expected_num): @@ -288,25 +295,28 @@ def wait_for_udev(fs_type, expected_paths): :raises RuntimeError: if unexpected device nodes are found """ expected_devnodes = frozenset((os.path.realpath(x) for x in expected_paths)) - found_devnodes = None context = pyudev.Context() - end_time = time.time() + 10.0 - while time.time() < end_time and not expected_devnodes == found_devnodes: - found_devnodes = frozenset( + def find_devnodes(): + return frozenset( [ x.device_node for x in context.list_devices(subsystem="block", ID_FS_TYPE=fs_type) + if x.device_node is not None ] ) - time.sleep(1) - if expected_devnodes != found_devnodes: + try: + for attempt in Retrying(wait=wait_fixed(1), stop=stop_after_delay(10)): + with attempt: + if expected_devnodes == find_devnodes(): + break + except RetryError as err: raise RuntimeError( f'Found unexpected devnodes: expected devnodes: {", ".join(expected_devnodes)} ' - f'!= found_devnodes: {", ".join(found_devnodes)}' - ) + f'!= found_devnodes: {", ".join(find_devnodes())}' + ) from err def processes(name): @@ -538,32 +548,24 @@ def wait_for_pools(self, expected_num, *, name=None): :return: list of pool information found :rtype: list of (str * MOPool) """ - (count, limit, dbus_err, found_num, known_pools, start_time) = ( - 0, - expected_num + 1, - None, - None, - None, - time.time(), - ) - while count < limit and not expected_num == found_num: - try: - known_pools = get_pools(name=name) - except dbus.exceptions.DBusException as err: - dbus_err = err - - if known_pools is not None: - found_num = len(known_pools) - - time.sleep(3) - count += 1 - - if found_num is None and dbus_err is not None: + try: + for attempt in Retrying( + retry=(retry_if_exception_type(dbus.exceptions.DBusException)), + wait=wait_fixed(3), + stop=stop_after_attempt(expected_num + 1), + reraise=True, + ): + with attempt: + known_pools = get_pools(name=name) + if len(known_pools) == expected_num: + break + except dbus.exceptions.DBusException as err: raise RuntimeError( - f"After {time.time() - start_time:.2f} seconds, the only " - "response is a D-Bus exception" - ) from dbus_err + "Failed to obtain any information about pools from the D-Bus" + ) from err - self.assertEqual(found_num, expected_num) + time.sleep(3) # Wait for the D-Bus to add an additional pool + known_pools = get_pools(name=name) + self.assertEqual(len(known_pools), expected_num) return known_pools diff --git a/tests/client-dbus/tests/udev/test_predict.py b/tests/client-dbus/tests/udev/test_predict.py index c6d52e26db..214c64eeab 100644 --- a/tests/client-dbus/tests/udev/test_predict.py +++ b/tests/client-dbus/tests/udev/test_predict.py @@ -94,7 +94,7 @@ def _call_blockdev_size(dev): :rtype: str """ with subprocess.Popen( - ["blockdev", "--getsize64", dev], + ["/usr/sbin/blockdev", "--getsize64", dev], stdout=subprocess.PIPE, ) as command: outs, _ = command.communicate() diff --git a/tests/client-dbus/tests/udev/test_revert.py b/tests/client-dbus/tests/udev/test_revert.py index db010b5cd2..99bfe00106 100644 --- a/tests/client-dbus/tests/udev/test_revert.py +++ b/tests/client-dbus/tests/udev/test_revert.py @@ -42,6 +42,9 @@ settle, ) +_MOUNT = "/usr/bin/mount" +_UMOUNT = "/usr/bin/umount" + def write_file(mountdir, filename): """ @@ -99,7 +102,7 @@ def test_revert(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -119,7 +122,7 @@ def test_revert(self): # pylint: disable=too-many-locals write_file(mountdir, file2) Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) @@ -131,12 +134,12 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.read_file(mountdir, file2) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) # Now stop the pool, which should tear down the devices (_, return_code, message) = Manager.Methods.StopPool( @@ -167,14 +170,14 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.assertFalse(os.path.exists(os.path.join(mountdir, file2))) self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals """ @@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals Filesystem.Properties.MergeScheduled.Set( get_object(snap_object_path_1), True ) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) @@ -285,7 +288,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) @@ -293,4 +296,4 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir])