From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D2B3D4339B; Wed, 22 Nov 2023 13:18:51 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7A6B7402E8; Wed, 22 Nov 2023 13:18:51 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 9A59D4028C for ; Wed, 22 Nov 2023 13:18:50 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D07C81595; Wed, 22 Nov 2023 04:19:36 -0800 (PST) Received: from [10.57.72.130] (unknown [10.57.72.130]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AEFDE3F7A6; Wed, 22 Nov 2023 04:18:48 -0800 (PST) Message-ID: Date: Wed, 22 Nov 2023 12:18:47 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 17/21] dts: node docstring update Content-Language: en-US To: =?UTF-8?Q?Juraj_Linke=C5=A1?= , thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com Cc: dev@dpdk.org References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-18-juraj.linkes@pantheon.tech> From: Yoan Picchi In-Reply-To: <20231115130959.39420-18-juraj.linkes@pantheon.tech> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 11/15/23 13:09, Juraj Linkeš wrote: > Format according to the Google format and PEP257, with slight > deviations. > > Signed-off-by: Juraj Linkeš > --- > dts/framework/testbed_model/node.py | 191 +++++++++++++++++++--------- > 1 file changed, 131 insertions(+), 60 deletions(-) > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py > index fa5b143cdd..f93b4acecd 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -3,8 +3,13 @@ > # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. > # Copyright(c) 2022-2023 University of New Hampshire > > -""" > -A node is a generic host that DTS connects to and manages. > +"""Common functionality for node management. > + > +A node is any host/server DTS connects to. > + > +The base class, :class:`Node`, provides functionality common to all nodes and is supposed > +to be extended by subclasses with functionality specific to each node type. functionality -> functionalities > +The decorator :func:`Node.skip_setup` can be used without subclassing. > """ > > from abc import ABC > @@ -35,10 +40,22 @@ > > > class Node(ABC): > - """ > - Basic class for node management. This class implements methods that > - manage a node, such as information gathering (of CPU/PCI/NIC) and > - environment setup. > + """The base class for node management. > + > + It shouldn't be instantiated, but rather subclassed. > + It implements common methods to manage any node: > + > + * Connection to the node, > + * Hugepages setup. > + > + Attributes: > + main_session: The primary OS-aware remote session used to communicate with the node. > + config: The node configuration. > + name: The name of the node. > + lcores: The list of logical cores that DTS can use on the node. > + It's derived from logical cores present on the node and the test run configuration. > + ports: The ports of this node specified in the test run configuration. > + virtual_devices: The virtual devices used on the node. > """ > > main_session: OSSession > @@ -52,6 +69,17 @@ class Node(ABC): > virtual_devices: list[VirtualDevice] > > def __init__(self, node_config: NodeConfiguration): > + """Connect to the node and gather info during initialization. > + > + Extra gathered information: > + > + * The list of available logical CPUs. This is then filtered by > + the ``lcores`` configuration in the YAML test run configuration file, > + * Information about ports from the YAML test run configuration file. > + > + Args: > + node_config: The node's test run configuration. > + """ > self.config = node_config > self.name = node_config.name > self._logger = getLogger(self.name) > @@ -60,7 +88,7 @@ def __init__(self, node_config: NodeConfiguration): > self._logger.info(f"Connected to node: {self.name}") > > self._get_remote_cpus() > - # filter the node lcores according to user config > + # filter the node lcores according to the test run configuration > self.lcores = LogicalCoreListFilter( > self.lcores, LogicalCoreList(self.config.lcores) > ).filter() > @@ -76,9 +104,14 @@ def _init_ports(self) -> None: > self.configure_port_state(port) > > def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > - """ > - Perform the execution setup that will be done for each execution > - this node is part of. > + """Execution setup steps. > + > + Configure hugepages and call :meth:`_set_up_execution` where > + the rest of the configuration steps (if any) are implemented. > + > + Args: > + execution_config: The execution test run configuration according to which > + the setup steps will be taken. > """ > self._setup_hugepages() > self._set_up_execution(execution_config) > @@ -87,58 +120,74 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > self.virtual_devices.append(VirtualDevice(vdev)) > > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional execution setup steps for subclasses. > + > + Subclasses should override this if they need to add additional execution setup steps. > """ > > def tear_down_execution(self) -> None: > - """ > - Perform the execution teardown that will be done after each execution > - this node is part of concludes. > + """Execution teardown steps. > + > + There are currently no common execution teardown steps common to all DTS node types. > """ > self.virtual_devices = [] > self._tear_down_execution() > > def _tear_down_execution(self) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional execution teardown steps for subclasses. > + > + Subclasses should override this if they need to add additional execution teardown steps. > """ > > def set_up_build_target( > self, build_target_config: BuildTargetConfiguration > ) -> None: > - """ > - Perform the build target setup that will be done for each build target > - tested on this node. > + """Build target setup steps. > + > + There are currently no common build target setup steps common to all DTS node types. > + > + Args: > + build_target_config: The build target test run configuration according to which > + the setup steps will be taken. > """ > self._set_up_build_target(build_target_config) > > def _set_up_build_target( > self, build_target_config: BuildTargetConfiguration > ) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional build target setup steps for subclasses. > + > + Subclasses should override this if they need to add additional build target setup steps. > """ > > def tear_down_build_target(self) -> None: > - """ > - Perform the build target teardown that will be done after each build target > - tested on this node. > + """Build target teardown steps. > + > + There are currently no common build target teardown steps common to all DTS node types. > """ > self._tear_down_build_target() > > def _tear_down_build_target(self) -> None: > - """ > - This method exists to be optionally overwritten by derived classes and > - is not decorated so that the derived class doesn't have to use the decorator. > + """Optional additional build target teardown steps for subclasses. > + > + Subclasses should override this if they need to add additional build target teardown steps. > """ > > def create_session(self, name: str) -> OSSession: > - """ > - Create and return a new OSSession tailored to the remote OS. > + """Create and return a new OS-aware remote session. > + > + The returned session won't be used by the node creating it. The session must be used by > + the caller. The session will be maintained for the entire lifecycle of the node object, > + at the end of which the session will be cleaned up automatically. > + > + Note: > + Any number of these supplementary sessions may be created. > + > + Args: > + name: The name of the session. > + > + Returns: > + A new OS-aware remote session. > """ > session_name = f"{self.name} {name}" > connection = create_session( > @@ -156,19 +205,19 @@ def create_interactive_shell( > privileged: bool = False, > app_args: str = "", > ) -> InteractiveShellType: > - """Create a handler for an interactive session. > + """Factory for interactive session handlers. > > - Instantiate shell_cls according to the remote OS specifics. > + Instantiate `shell_cls` according to the remote OS specifics. > > Args: > shell_cls: The class of the shell. > - timeout: Timeout for reading output from the SSH channel. If you are > - reading from the buffer and don't receive any data within the timeout > - it will throw an error. > + timeout: Timeout for reading output from the SSH channel. If you are reading from > + the buffer and don't receive any data within the timeout it will throw an error. > privileged: Whether to run the shell with administrative privileges. > app_args: The arguments to be passed to the application. > + > Returns: > - Instance of the desired interactive application. > + An instance of the desired interactive application shell. > """ > if not shell_cls.dpdk_app: > shell_cls.path = self.main_session.join_remote_path(shell_cls.path) > @@ -185,14 +234,22 @@ def filter_lcores( > filter_specifier: LogicalCoreCount | LogicalCoreList, > ascending: bool = True, > ) -> list[LogicalCore]: > - """ > - Filter the LogicalCores found on the Node according to > - a LogicalCoreCount or a LogicalCoreList. > + """Filter the node's logical cores that DTS can use. > + > + Logical cores that DTS can use are the ones that are present on the node, but filtered > + according to the test run configuration. The `filter_specifier` will filter cores from > + those logical cores. > + > + Args: > + filter_specifier: Two different filters can be used, one that specifies the number > + of logical cores per core, cores per socket and the number of sockets, > + and another one that specifies a logical core list. > + ascending: If :data:`True`, use cores with the lowest numerical id first and continue > + in ascending order. If :data:`False`, start with the highest id and continue > + in descending order. This ordering affects which sockets to consider first as well. > > - If ascending is True, use cores with the lowest numerical id first > - and continue in ascending order. If False, start with the highest > - id and continue in descending order. This ordering affects which > - sockets to consider first as well. > + Returns: > + The filtered logical cores. > """ > self._logger.debug(f"Filtering {filter_specifier} from {self.lcores}.") > return lcore_filter( > @@ -202,17 +259,14 @@ def filter_lcores( > ).filter() > > def _get_remote_cpus(self) -> None: > - """ > - Scan CPUs in the remote OS and store a list of LogicalCores. > - """ > + """Scan CPUs in the remote OS and store a list of LogicalCores.""" > self._logger.info("Getting CPU information.") > self.lcores = self.main_session.get_remote_cpus(self.config.use_first_core) > > def _setup_hugepages(self) -> None: > - """ > - Setup hugepages on the Node. Different architectures can supply different > - amounts of memory for hugepages and numa-based hugepage allocation may need > - to be considered. > + """Setup hugepages on the node. > + > + Configure the hugepages only if they're specified in the node's test run configuration. > """ > if self.config.hugepages: > self.main_session.setup_hugepages( > @@ -220,8 +274,11 @@ def _setup_hugepages(self) -> None: > ) > > def configure_port_state(self, port: Port, enable: bool = True) -> None: > - """ > - Enable/disable port. > + """Enable/disable `port`. > + > + Args: > + port: The port to enable/disable. > + enable: :data:`True` to enable, :data:`False` to disable. > """ > self.main_session.configure_port_state(port, enable) > > @@ -231,15 +288,17 @@ def configure_port_ip_address( > port: Port, > delete: bool = False, > ) -> None: > - """ > - Configure the IP address of a port on this node. > + """Add an IP address to `port` on this node. > + > + Args: > + address: The IP address with mask in CIDR format. Can be either IPv4 or IPv6. > + port: The port to which to add the address. > + delete: If :data:`True`, will delete the address from the port instead of adding it. > """ > self.main_session.configure_port_ip_address(address, port, delete) > > def close(self) -> None: > - """ > - Close all connections and free other resources. > - """ > + """Close all connections and free other resources.""" > if self.main_session: > self.main_session.close() > for session in self._other_sessions: > @@ -248,6 +307,11 @@ def close(self) -> None: > > @staticmethod > def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]: > + """Skip the decorated function. > + > + The :option:`--skip-setup` command line argument and the :envvar:`DTS_SKIP_SETUP` > + environment variable enable the decorator. > + """ > if SETTINGS.skip_setup: > return lambda *args: None > else: > @@ -257,6 +321,13 @@ def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]: > def create_session( > node_config: NodeConfiguration, name: str, logger: DTSLOG > ) -> OSSession: > + """Factory for OS-aware sessions. > + > + Args: > + node_config: The test run configuration of the node to connect to. > + name: The name of the session. > + logger: The logger instance this session will use. > + """ > match node_config.os: > case OS.linux: > return LinuxSession(node_config, name, logger)