On 8/31/23 11:04, Juraj Linkeš wrote:
WIP: only one module is reformatted to serve as a demonstration.
The google format is documented here [0].
[0]: https://google.github.io/styleguide/pyguide.html
Signed-off-by: Juraj Linkeš <juraj.lin...@pantheon.tech>
Acked-by: Jeremy Spweock <jspwe...@iol.unh.edu>
---
dts/framework/testbed_model/node.py | 171 +++++++++++++++++++---------
1 file changed, 118 insertions(+), 53 deletions(-)
diff --git a/dts/framework/testbed_model/node.py
b/dts/framework/testbed_model/node.py
index 23efa79c50..619743ebe7 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.
+
+There's a base class, Node, that's supposed to be extended by other classes
This comment and all the following ones are all something of a nitpick.
This sounds too passive. Why not having something simpler like:
The virtual class Node is meant to be extended by other classes
with functionality specific to that node type.
+with functionality specific to that node type.
+The only part that can be used standalone is the Node.skip_setup static method,
+which is a decorator used to skip method execution if skip_setup is passed
+by the user on the cmdline or in an env variable.
I'd extend env to the full word as this is meant to go in the documentation.
"""
from abc import ABC
@@ -35,10 +40,26 @@
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 extended.
+ It implements common methods to manage any node:
+
+ * connection to the node
+ * information gathering of CPU
+ * hugepages setup
+
+ Arguments:
+ node_config: The config from the input configuration file.
+
+ Attributes:
+ main_session: The primary OS-agnostic remote session used
+ to communicate with the node.
+ config: The configuration used to create the node.
+ 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 user
configuration.
+ ports: The ports of this node specified in user configuration.
"""
main_session: OSSession
@@ -77,9 +98,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 self._set_up_execution where
+ the rest of the configuration steps (if any) are implemented.
+
+ Args:
+ execution_config: The execution configuration according to which
+ the setup steps will be taken.
"""
self._setup_hugepages()
self._set_up_execution(execution_config)
@@ -88,58 +114,78 @@ 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 derived classes.
+
+ Derived classes should overwrite this
+ if they want to add additional execution setup steps.
I'd probably use need or require instead of want (it's the dev that
wants, not the class)
"""
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 derived classes.
+
+ Derived classes should overwrite this
+ if they want 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 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 derived classes.
+
+ Derived classes should optionally overwrite this
Is there a reason for the "optionally" here when it's absent in all the
other functions?
+ if they want 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 derived classes.
+
+ Derived classes should overwrite this
+ if they want 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-agnostic remote session.
+
+ The returned session won't be used by the node creating it.
+ The session must be used by the caller.
+ Will be cleaned up automatically.
I had a double take reading this before I understood that the subject
was the previously mentioned session. I'd recommend to either add "it"
or "the session". Also, it will be cleaned automatically... when? When I
create a new session? when the node is deleted?
+
+ Args:
+ name: The name of the session.
+
+ Returns:
+ A new OS-agnostic remote session.
"""
session_name = f"{self.name} {name}"
connection = create_session(
@@ -186,14 +232,24 @@ 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.
- 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.
+ Logical cores that DTS can use are ones that are present on the node,
+ but filtered according to user config.
+ 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,
+ the other that specifies a logical core list.
+ ascending: If 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:
+ A list of logical cores.
"""
self._logger.debug(f"Filtering {filter_specifier} from
{self.lcores}.")
return lcore_filter(
@@ -203,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):
- """
- 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 user
configuration.
"""
if self.config.hugepages:
self.main_session.setup_hugepages(
@@ -221,8 +274,11 @@ def _setup_hugepages(self):
)
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: True to enable, false to disable.
"""
self.main_session.configure_port_state(port, enable)
@@ -232,15 +288,19 @@ 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 a 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 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:
@@ -249,6 +309,11 @@ def close(self) -> None:
@staticmethod
def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]:
+ """A decorator that skips the decorated function.
+
+ When used, the decorator executes an empty lambda function
+ instead of the decorated function.
+ """
if SETTINGS.skip_setup:
return lambda *args: None
else:
