From fa5231119075704f8a38b82eba8c89e6ca8e952f Mon Sep 17 00:00:00 2001 From: Bodong Yang Date: Thu, 25 Apr 2024 03:05:46 +0000 Subject: [PATCH] Squashed commit of the following: commit 727ebddc1d266dce7b5f4c4fb577b0eeb0d64e91 Author: Bodong Yang <86948717+Bodong-Yang@users.noreply.github.com> Date: Thu Apr 25 12:03:09 2024 +0900 refine(v3.7.x): refine bootctrl.common CMDHelperFuncs (#289) This PR does the following things: 1. refines the common module's subprocess wrapper methods. 2. refines and simplify the boot_control.common.CMDHelperFuncs implementation a lot. 3. cleanup and remove boot_contro.errors, now each boot controller implementation has their own error types. 4. use mount slot helper's prepare_standby_dev instead of each boot controller's prepare_standby_dev method. 5. integrate the new CMDHelperFuncs to each boot controller and fix up test files accordingly. NOTE that cboot boot controller implementation is temporarily removed, it will be added back in #287 . --- otaclient/app/boot_control/_common.py | 171 ++++++++++++++++++++------ otaclient/app/common.py | 44 ++++--- otaclient/app/proto/README.md | 2 +- tests/test_common.py | 94 ++++++++------ 4 files changed, 219 insertions(+), 92 deletions(-) diff --git a/otaclient/app/boot_control/_common.py b/otaclient/app/boot_control/_common.py index 9426166c7..776c973b7 100644 --- a/otaclient/app/boot_control/_common.py +++ b/otaclient/app/boot_control/_common.py @@ -44,7 +44,12 @@ class CMDHelperFuncs: - """HelperFuncs bundle for wrapped linux cmd.""" + """HelperFuncs bundle for wrapped linux cmd. + + When underlying subprocess call failed and is True, + functions defined in this class will raise the original exception + to the upper caller. + """ @classmethod def get_attrs_by_dev( @@ -53,7 +58,16 @@ def get_attrs_by_dev( """Get from . This is implemented by calling: - lsblk -in -o + `lsblk -in -o ` + + Args: + attr (PartitionToken): the attribute to retrieve from the . + dev (Path | str): the target device path. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + str: of . """ cmd = ["lsblk", "-ino", attr, str(dev)] return subprocess_check_output(cmd, raise_exception=raise_exception) @@ -62,10 +76,20 @@ def get_attrs_by_dev( def get_dev_by_token( cls, token: PartitionToken, value: str, *, raise_exception: bool = True ) -> Optional[list[str]]: - """Get a list of device(s) that matches the token=value pair. + """Get a list of device(s) that matches the = pair. This is implemented by calling: blkid -o device -t = + + Args: + token (PartitionToken): which attribute of device to match. + value (str): the value of the attribute. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + Optional[list[str]]: If there is at least one device found, return a list + contains all found device(s), otherwise None. """ cmd = ["blkid", "-o", "device", "-t", f"{token}={value}"] if res := subprocess_check_output(cmd, raise_exception=raise_exception): @@ -76,13 +100,14 @@ def get_current_rootfs_dev(cls, *, raise_exception: bool = True) -> str: """Get the devpath of current rootfs dev. This is implemented by calling - findmnt -nfc -o SOURCE / + findmnt -nfc -o SOURCE - Returns: - full path to dev of the current rootfs. + Args: + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. - Raises: - subprocess.CalledProcessError on failed command call. + Returns: + str: the devpath of current rootfs device. """ cmd = ["findmnt", "-nfco", "SOURCE", cfg.ACTIVE_ROOTFS_PATH] return subprocess_check_output(cmd, raise_exception=raise_exception) @@ -94,8 +119,16 @@ def get_mount_point_by_dev(cls, dev: str, *, raise_exception: bool = True) -> st This is implemented by calling: findmnt -nfo TARGET - NOTE: findmnt raw result might have multiple lines if target dev is bind mounted. - use option -f to only show the first file system. + NOTE: option -f is used to only show the first file system. + + Args: + dev (str): the device to check against. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + str: the FIRST mountpint of the , or empty string if is False + and the subprocess call failed(due to dev is not mounted or other reasons). """ cmd = ["findmnt", "-nfo", "TARGET", dev] return subprocess_check_output(cmd, raise_exception=raise_exception) @@ -104,10 +137,18 @@ def get_mount_point_by_dev(cls, dev: str, *, raise_exception: bool = True) -> st def get_dev_by_mount_point( cls, mount_point: str, *, raise_exception: bool = True ) -> str: - """Return the underlying mounted dev of the given mount_point. + """Return the source dev of the given . This is implemented by calling: findmnt -no SOURCE + + Args: + mount_point (str): mount_point to check against. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + str: the source device of . """ cmd = ["findmnt", "-no", "SOURCE", mount_point] return subprocess_check_output(cmd, raise_exception=raise_exception) @@ -121,7 +162,13 @@ def is_target_mounted( This is implemented by calling: findmnt - and see if there is any matching device. + Args: + target (Path | str): the target to check against. Could be a device or a mount point. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + bool: return True if the target has at least one mount_point. """ cmd = ["findmnt", target] return bool(subprocess_check_output(cmd, raise_exception=raise_exception)) @@ -133,12 +180,31 @@ def get_parent_dev(cls, child_device: str, *, raise_exception: bool = True) -> s This function is implemented by calling: lsblk -idpno PKNAME + + Args: + child_device (str): the device to find parent device from. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + + Returns: + str: the parent device of the specific . """ cmd = ["lsblk", "-idpno", "PKNAME", child_device] return subprocess_check_output(cmd, raise_exception=raise_exception) @classmethod def set_ext4_fslabel(cls, dev: str, fslabel: str, *, raise_exception: bool = True): + """Set to ext4 formatted . + + This is implemented by calling: + e2label + + Args: + dev (str): the ext4 partition device. + fslabel (str): the fslabel to be set. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. + """ cmd = ["e2label", dev, fslabel] subprocess_call(cmd, raise_exception=raise_exception) @@ -146,7 +212,10 @@ def set_ext4_fslabel(cls, dev: str, fslabel: str, *, raise_exception: bool = Tru def mount_rw( cls, target: str, mount_point: Path | str, *, raise_exception: bool = True ): - """Mount the target to the mount_point read-write. + """Mount the to read-write. + + This is implemented by calling: + mount -o rw --make-private --make-unbindable This is implemented by calling: mount -o rw --make-private --make-unbindable @@ -154,8 +223,11 @@ def mount_rw( NOTE: pass args = ["--make-private", "--make-unbindable"] to prevent mount events propagation to/from this mount point. - Raises: - CalledProcessError on failed mount. + Args: + target (str): target to be mounted. + mount_point (Path | str): mount point to mount to. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. """ # fmt: off cmd = [ @@ -172,13 +244,16 @@ def mount_rw( def bind_mount_ro( cls, target: str, mount_point: Path | str, *, raise_exception: bool = True ): - """Bind mount the target to the mount_point read-only. + """Bind mount the to read-only. This is implemented by calling: mount -o bind,ro --make-private --make-unbindable - Raises: - CalledProcessError on failed mount. + Args: + target (str): target to be mounted. + mount_point (Path | str): mount point to mount to. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. """ # fmt: off cmd = [ @@ -193,9 +268,18 @@ def bind_mount_ro( @classmethod def umount(cls, target: Path | str, *, raise_exception: bool = True): - """Try to unmount the . + """Try to umount the . - This function will first check whether the is mounted. + This is implemented by calling: + umount + + Before calling umount, the will be check whether it is mounted, + if it is not mounted, this function will return directly. + + Args: + target (Path | str): target to be umounted. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. """ # first try to check whether the target(either a mount point or a dev) # is mounted @@ -215,15 +299,17 @@ def mkfs_ext4( fsuuid: Optional[str] = None, raise_exception: bool = True, ): - """Call mkfs.ext4 on . - - If fslabel and/or fsuuid provided, use them in prior, otherwise - try to preserve fsuuid and fslabel if possible. - - NOTE: -F is used to bypass interactive confirmation. - - Raises: - Original CalledProcessError from the failed command execution. + """Create new ext4 formatted filesystem on , optionally with + and/or . + + Args: + dev (str): device to be formatted to ext4. + fslabel (Optional[str], optional): fslabel of the new ext4 filesystem. Defaults to None. + When it is None, this function will try to preserve the previous fslabel. + fsuuid (Optional[str], optional): fsuuid of the new ext4 filesystem. Defaults to None. + When it is None, this function will try to preserve the previous fsuuid. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. """ cmd = ["mkfs.ext4", "-F"] @@ -250,23 +336,23 @@ def mkfs_ext4( cmd.extend(["-L", fslabel]) cmd.append(dev) - logger.warning(f"execute {cmd=}") + logger.warning(f"format {dev} to ext4: {cmd=}") subprocess_call(cmd, raise_exception=raise_exception) @classmethod def mount_ro( cls, *, target: str, mount_point: str | Path, raise_exception: bool = True ): - """Mount target on mount_point read-only. + """Mount to read-only. - If the target device is mounted, we bind mount the target device to mount_point, + If the target device is mounted, we bind mount the target device to mount_point. if the target device is not mounted, we directly mount it to the mount_point. - This method mount the target as ro with make-private flag and make-unbindable flag, - to prevent ANY accidental writes/changes to the target. - - Raises: - CalledProcessError on failed mounting. + Args: + target (str): target to be mounted. + mount_point (str | Path): mount point to mount to. + raise_exception (bool, optional): raise exception on subprocess call failed. + Defaults to True. """ # NOTE: set raise_exception to false to allow not mounted # not mounted dev will have empty return str @@ -293,10 +379,17 @@ def mount_ro( @classmethod def reboot(cls, args: Optional[list[str]] = None) -> NoReturn: - """Reboot the whole system otaclient running at and terminate otaclient. + """Reboot the system, with optional args passed to reboot command. + + This is implemented by calling: + reboot [args[0], args[1], ...] - NOTE(20230614): this command MUST also make otaclient exit immediately. + NOTE(20230614): this command makes otaclient exit immediately. NOTE(20240421): rpi_boot's reboot takes args. + + Args: + args (Optional[list[str]], optional): args passed to reboot command. + Defaults to None, not passing any args. """ cmd = ["reboot"] if args: diff --git a/otaclient/app/common.py b/otaclient/app/common.py index 0cfce9ba7..536cbcb7f 100644 --- a/otaclient/app/common.py +++ b/otaclient/app/common.py @@ -125,6 +125,20 @@ def subprocess_run_wrapper( check_output: bool, timeout: Optional[float] = None, ) -> subprocess.CompletedProcess[bytes]: + """A wrapper for subprocess.run method. + + NOTE: this is for the requirement of customized subprocess call + in the future, like chroot or nsenter before execution. + + Args: + cmd (str | list[str]): command to be executed. + check (bool): if True, raise CalledProcessError on non 0 return code. + check_output (bool): if True, the UTF-8 decoded stdout will be returned. + timeout (Optional[float], optional): timeout for execution. Defaults to None. + + Returns: + subprocess.CompletedProcess[bytes]: the result of the execution. + """ if isinstance(cmd, str): cmd = shlex.split(cmd) @@ -144,18 +158,17 @@ def subprocess_check_output( default: str = "", timeout: Optional[float] = None, ) -> str: - """Run the command and return its output if possible. - NOTE: this method will call decode and strip on the raw output. + """Run the and return UTF-8 decoded stripped stdout. - Params: - cmd: string or list of string of command to be called. - raise_exception: Whether to raise the exception from - underlaying subprocess.check_output. - default: when is True, return this . + Args: + cmd (str | list[str]): command to be executed. + raise_exception (bool, optional): raise the underlying CalledProcessError. Defaults to False. + default (str, optional): if is False, return on underlying + subprocess call failed. Defaults to "". + timeout (Optional[float], optional): timeout for execution. Defaults to None. - Raises: - The original CalledProcessError from calling subprocess.check_output if - the execution failed and is True. + Returns: + str: UTF-8 decoded stripped stdout. """ try: res = subprocess_run_wrapper( @@ -180,11 +193,12 @@ def subprocess_call( raise_exception: bool = False, timeout: Optional[float] = None, ) -> None: - """Run the without checking its output. + """Run the . - Raises: - The original CalledProcessError from calling subprocess.check_output if - the execution failed and is True. + Args: + cmd (str | list[str]): command to be executed. + raise_exception (bool, optional): raise the underlying CalledProcessError. Defaults to False. + timeout (Optional[float], optional): timeout for execution. Defaults to None. """ try: subprocess_run_wrapper(cmd, check=True, check_output=False, timeout=timeout) @@ -549,7 +563,7 @@ def shutdown(self, *, raise_last_exc: bool): logger.debug("shutdown retry task map") if self._running_inst: self._running_inst.shutdown(raise_last_exc=raise_last_exc) - # NOTE: passthrough the exception from underlaying running_inst + # NOTE: passthrough the exception from underlying running_inst finally: self._running_inst = None self._executor.shutdown(wait=True) diff --git a/otaclient/app/proto/README.md b/otaclient/app/proto/README.md index b1a177efa..4aab4ca71 100644 --- a/otaclient/app/proto/README.md +++ b/otaclient/app/proto/README.md @@ -143,7 +143,7 @@ After the compiled python code is ready, we can define the wrappers accordingly # type hints for each attributes when manually create instances # of wrapper types. # NOTE: this __init__ is just for typing use, it will not be - # used by the underlaying MessageWrapper + # used by the underlying MessageWrapper # NOTE: if pyi file is also generated when compiling the proto file, # the __init__ in pyi file can be used directly with little adjustment def __init__( diff --git a/tests/test_common.py b/tests/test_common.py index 534e311da..850a865a8 100644 --- a/tests/test_common.py +++ b/tests/test_common.py @@ -19,7 +19,6 @@ import pytest import random import logging -from concurrent.futures import ThreadPoolExecutor from functools import partial from hashlib import sha256 from multiprocessing import Process @@ -38,11 +37,10 @@ subprocess_call, subprocess_check_output, verify_file, - wait_with_backoff, write_str_to_file_sync, ) from tests.utils import compare_dir -from tests.conftest import cfg, run_http_server +from tests.conftest import run_http_server logger = logging.getLogger(__name__) @@ -97,39 +95,6 @@ def test_write_to_file_sync(tmp_path: Path): assert _path.read_text() == _TEST_FILE_CONTENT -def test_subprocess_call(): - with pytest.raises(subprocess.CalledProcessError) as e: - subprocess_call("ls /non-existed", raise_exception=True) - _origin_e = e.value - assert _origin_e.returncode == 2 - assert ( - _origin_e.stderr.decode().strip() - == "ls: cannot access '/non-existed': No such file or directory" - ) - - subprocess_call("ls /non-existed", raise_exception=False) - - -def test_subprocess_check_output(file_t: Tuple[str, str, int]): - _path, _, _ = file_t - with pytest.raises(subprocess.CalledProcessError) as e: - subprocess_check_output("cat /non-existed", raise_exception=True) - _origin_e = e.value - assert _origin_e.returncode == 1 - assert ( - _origin_e.stderr.decode().strip() - == "cat: /non-existed: No such file or directory" - ) - - assert ( - subprocess_check_output( - "cat /non-existed", raise_exception=False, default="abc" - ) - == "abc" - ) - assert subprocess_check_output(f"cat {_path}") == _TEST_FILE_CONTENT - - class Test_copytree_identical: @pytest.fixture(autouse=True) def setup(self, tmp_path: Path): @@ -266,7 +231,7 @@ def test_symlink_to_directory(self, tmp_path: Path): class _RetryTaskMapTestErr(Exception): - ... + """""" class TestRetryTaskMap: @@ -445,3 +410,58 @@ def test_probing_delayed_online_server(self, subprocess_launch_server): ) # probing should cost at least seconds assert int(time.time()) >= start_time + self.LAUNCH_DELAY + + +class TestSubprocessCall: + TEST_FILE_CONTENTS = "test file contents" + + @pytest.fixture(autouse=True) + def setup_test(self, tmp_path: Path): + test_file = tmp_path / "test_file" + test_file.write_text(self.TEST_FILE_CONTENTS) + self.existed_file = test_file + self.non_existed_file = tmp_path / "non_existed_file" + + def test_subprocess_call_failed(self): + cmd = ["ls", str(self.non_existed_file)] + with pytest.raises(subprocess.CalledProcessError) as e: + subprocess_call(cmd, raise_exception=True) + origin_exc: subprocess.CalledProcessError = e.value + + assert origin_exc.returncode == 2 + assert ( + origin_exc.stderr.decode().strip() + == f"ls: cannot access '{self.non_existed_file}': No such file or directory" + ) + + # test exception supressed + subprocess_call(cmd, raise_exception=False) + + def test_subprocess_call_succeeded(self): + cmd = ["ls", str(self.existed_file)] + subprocess_call(cmd, raise_exception=True) + + def test_subprocess_check_output_failed(self): + cmd = ["cat", str(self.non_existed_file)] + + with pytest.raises(subprocess.CalledProcessError) as e: + subprocess_check_output(cmd, raise_exception=True) + origin_exc: subprocess.CalledProcessError = e.value + assert origin_exc.returncode == 1 + assert ( + origin_exc.stderr.decode().strip() + == f"cat: {self.non_existed_file}: No such file or directory" + ) + + # test exception supressed + default_value = "test default_value" + output = subprocess_check_output( + cmd, default=default_value, raise_exception=False + ) + assert output == default_value + + def test_subprocess_check_output_succeeded(self): + cmd = ["cat", str(self.existed_file)] + + output = subprocess_check_output(cmd, raise_exception=True) + assert output == self.TEST_FILE_CONTENTS