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 72B4143382; Wed, 22 Nov 2023 14:28:29 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5C13740ED8; Wed, 22 Nov 2023 14:28:29 +0100 (CET) Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) by mails.dpdk.org (Postfix) with ESMTP id EDB864028C for ; Wed, 22 Nov 2023 14:28:27 +0100 (CET) Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-507962561adso9935962e87.0 for ; Wed, 22 Nov 2023 05:28:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700659707; x=1701264507; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=7xYyUzQqHHKLS9FpsYblKvUKz3iXyTaOoyksfFhD9MM=; b=R0e5fu+E83UaUJehB7XJVIkrnYvvTlsv1EL7+UJFGhOUXXCjjwBdTWTWfh2XJ5BaHe 85uBUVEnC8Cc7CpQrYQjYuy4zLIKkeqeHmXXl5H4jaf3xXUutR78D962qatBAU2lNJK3 lvpKxsAs2iNCzMmk8OT/78R8HxDEy/3QVYdhI6XzxizR1nXcroIv6gGHhvUEZM3vrQGy 3y/ZHefpvj5FkgRw1Q+3JpWVLHbWzx7VBP7aR25uVlOQs3GPmyn7+mrt6pEsghUzhSRa bwyCTuYduFP+jOeLqxLJYm68F3FdkrElGOjxru/sPQfTMtZrMBxf7rz0eeQPTXjgraHA 1YcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700659707; x=1701264507; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7xYyUzQqHHKLS9FpsYblKvUKz3iXyTaOoyksfFhD9MM=; b=Tb8x82ej4/8YCLGPDz8iMnM7MhqRN6s5xpisctIUIgYWZE53xYmLlhQUHGurtOR7Py FhBWe8dqcHzI5nQW0HAzniKXa6M4llVPnP0PgSmW+0WBatehlnlcQKmxset7JdU8h0zs WY9pHJ1GLQNH1tzKY66ZgLv9GjhRy8Pbpobe4kMcnTxz1TeaHgZyrJj1l4XONX6PT16p IYZY5H1UxZ7N2qmDwl69sD1UqugrCh0/pravQ7XNXzzKUtEIJFp1CJDyqVXxDvpQq/9M vyIlYQ6aP1mKi4AUXQhZpmmAyzh8kfhhNylxpuGM24aY7pANs25tVbbvgsY1f80By+P0 KHcQ== X-Gm-Message-State: AOJu0Yzglcy82/DRL6K0qfzmOVBHHIZ11RAM8vxl3yj6mdatAMfXGsL9 8o5v86+t6reH2fYfqajUup9zT0KSXPTAdnJtqZ7JdQ== X-Google-Smtp-Source: AGHT+IEwq+2CpF1wdfGOvuYzUwejLcZmQRAMjR28fj9xQNlc81gVCR0Z+BY87hSav1B9YHATVJ84yREfkXpq5X7Ev/E= X-Received: by 2002:ac2:5057:0:b0:500:cb2b:8678 with SMTP id a23-20020ac25057000000b00500cb2b8678mr281347lfm.40.1700659707144; Wed, 22 Nov 2023 05:28:27 -0800 (PST) MIME-Version: 1.0 References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-18-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 14:28:16 +0100 Message-ID: Subject: Re: [PATCH v7 17/21] dts: node docstring update To: Yoan Picchi Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Wed, Nov 22, 2023 at 1:18=E2=80=AFPM Yoan Picchi wrote: > > On 11/15/23 13:09, Juraj Linke=C5=A1 wrote: > > Format according to the Google format and PEP257, with slight > > deviations. > > > > Signed-off-by: Juraj Linke=C5=A1 > > --- > > 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/testbe= d_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 no= des and is supposed > > +to be extended by subclasses with functionality specific to each node = type. > > functionality -> functionalities > Ack. > > +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 tha= t > > - 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 comm= unicate 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 th= e test run configuration. > > + ports: The ports of this node specified in the test run config= uration. > > + 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 configurat= ion file, > > + * Information about ports from the YAML test run configuration= file. > > + > > + Args: > > + node_config: The node's test run configuration. > > + """ > > self.config =3D node_config > > self.name =3D node_config.name > > self._logger =3D 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 configurati= on > > self.lcores =3D 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: ExecutionConfigurati= on) -> None: > > - """ > > - Perform the execution setup that will be done for each executi= on > > - 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 acc= ording 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: Execu= tionConfiguration) -> None: > > self.virtual_devices.append(VirtualDevice(vdev)) > > > > def _set_up_execution(self, execution_config: ExecutionConfigurat= ion) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses 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 ex= ecution > > - 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 =3D [] > > self._tear_down_execution() > > > > def _tear_down_execution(self) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses 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 buil= d 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 configurati= on 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 cla= sses 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 comm= on 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 cla= sses and > > - is not decorated so that the derived class doesn't have to use= the decorator. > > + """Optional additional build target teardown steps for subclas= ses. > > + > > + 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. Th= e session must be used by > > + the caller. The session will be maintained for the entire life= cycle of the node object, > > + at the end of which the session will be cleaned up automatical= ly. > > + > > + 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 =3D f"{self.name} {name}" > > connection =3D create_session( > > @@ -156,19 +205,19 @@ def create_interactive_shell( > > privileged: bool =3D False, > > app_args: str =3D "", > > ) -> 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 wit= hin 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 timeo= ut 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 =3D self.main_session.join_remote_path(she= ll_cls.path) > > @@ -185,14 +234,22 @@ def filter_lcores( > > filter_specifier: LogicalCoreCount | LogicalCoreList, > > ascending: bool =3D 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 o= n 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 t= hat specifies the number > > + of logical cores per core, cores per socket and the nu= mber of sockets, > > + and another one that specifies a logical core list. > > + ascending: If :data:`True`, use cores with the lowest nume= rical id first and continue > > + in ascending order. If :data:`False`, start with the h= ighest id and continue > > + in descending order. This ordering affects which socke= ts to consider first as well. > > > > - If ascending is True, use cores with the lowest numerical id f= irst > > - and continue in ascending order. If False, start with the high= est > > - id and continue in descending order. This ordering affects whi= ch > > - 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 =3D self.main_session.get_remote_cpus(self.config= .use_first_core) > > > > def _setup_hugepages(self) -> None: > > - """ > > - Setup hugepages on the Node. Different architectures can suppl= y different > > - amounts of memory for hugepages and numa-based hugepage alloca= tion 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 =3D 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 =3D 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 e= ither 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, de= lete) > > > > 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 :envv= ar:`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]) -> Callab= le[..., 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) >