* [RFC PATCH 0/2] dts: add basic scope to improve shell handling
@ 2024-12-20 17:23 Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 1/2] dts: add scoping and shell registration to Node Luca Vizzarro
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Luca Vizzarro @ 2024-12-20 17:23 UTC (permalink / raw)
To: dev; +Cc: Paul Szczepanek, Patrick Robb, Luca Vizzarro
Hi there,
To try to improve the ease of use of the framework for the test
developer, I have been trying to come up with a decent solution to
improve shell handling and consistency. At the moment we have 2 patterns
to do this, which could be confusing to the user.
It probably is that a good approach, which is what I am proposing, is to
introduce a scoping mechanism in DTS. What this would mean is to
associate any shells or modification to different scopes: global test
suite and individual test cases.
Here's an RFC for this, please have a look. Looking forward to your
feedback!
This could be massively improved, but it'd require a lot more changes.
One idea that I think is worth pursuing is turning the execution into a
FSM.
Best,
Luca
Luca Vizzarro (2):
dts: add scoping and shell registration to Node
dts: revert back shell split
dts/framework/remote_session/dpdk_shell.py | 8 +-
.../remote_session/interactive_shell.py | 262 +++++++++++++++--
.../single_active_interactive_shell.py | 266 ------------------
dts/framework/remote_session/testpmd_shell.py | 4 +-
dts/framework/runner.py | 15 +-
dts/framework/testbed_model/capability.py | 35 ++-
dts/framework/testbed_model/node.py | 65 ++++-
dts/framework/testbed_model/sut_node.py | 14 +-
dts/tests/TestSuite_blocklist.py | 16 +-
dts/tests/TestSuite_checksum_offload.py | 168 +++++------
dts/tests/TestSuite_dynamic_queue_conf.py | 52 ++--
dts/tests/TestSuite_l2fwd.py | 20 +-
dts/tests/TestSuite_mac_filter.py | 124 ++++----
dts/tests/TestSuite_pmd_buffer_scatter.py | 26 +-
dts/tests/TestSuite_smoke_tests.py | 4 +-
dts/tests/TestSuite_vlan.py | 42 +--
16 files changed, 575 insertions(+), 546 deletions(-)
delete mode 100644 dts/framework/remote_session/single_active_interactive_shell.py
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/2] dts: add scoping and shell registration to Node
2024-12-20 17:23 [RFC PATCH 0/2] dts: add basic scope to improve shell handling Luca Vizzarro
@ 2024-12-20 17:24 ` Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 2/2] dts: revert back shell split Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
2 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2024-12-20 17:24 UTC (permalink / raw)
To: dev; +Cc: Paul Szczepanek, Patrick Robb, Luca Vizzarro
Add a basic scoping mechanism to Nodes, to improve the control over
test suite led environmental changes. Moreover, keep a pool of active
shells based on scope, therefore allowing shells to register
themselves.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
.../single_active_interactive_shell.py | 2 +
dts/framework/runner.py | 15 +-
dts/framework/testbed_model/capability.py | 35 ++--
dts/framework/testbed_model/node.py | 65 ++++++-
dts/framework/testbed_model/sut_node.py | 14 +-
dts/tests/TestSuite_blocklist.py | 16 +-
dts/tests/TestSuite_checksum_offload.py | 168 +++++++++---------
dts/tests/TestSuite_dynamic_queue_conf.py | 52 +++---
dts/tests/TestSuite_l2fwd.py | 20 +--
dts/tests/TestSuite_mac_filter.py | 124 +++++++------
dts/tests/TestSuite_pmd_buffer_scatter.py | 26 ++-
dts/tests/TestSuite_smoke_tests.py | 4 +-
dts/tests/TestSuite_vlan.py | 42 ++---
13 files changed, 333 insertions(+), 250 deletions(-)
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
index c43c54e457..910af8f655 100644
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -112,7 +112,9 @@ def __init__(
the name of the underlying node which it is running on.
**kwargs: Any additional arguments if any.
"""
+ node.register_shell(self)
self._node = node
+
if name is None:
name = type(self).__name__
self._logger = get_dts_logger(f"{node.name}.{name}")
diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 510be1a870..fd3a934a9f 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -460,6 +460,10 @@ def _run_test_suite(
DtsStage.test_suite_setup, Path(SETTINGS.output_dir, test_suite_name)
)
test_suite = test_suite_with_cases.test_suite_class(sut_node, tg_node, topology)
+
+ sut_node.enter_scope("suite")
+ tg_node.enter_scope("suite")
+
try:
self._logger.info(f"Starting test suite setup: {test_suite_name}")
test_suite.set_up_suite()
@@ -479,7 +483,6 @@ def _run_test_suite(
try:
self._logger.set_stage(DtsStage.test_suite_teardown)
test_suite.tear_down_suite()
- sut_node.kill_cleanup_dpdk_apps()
test_suite_result.update_teardown(Result.PASS)
except Exception as e:
self._logger.exception(f"Test suite teardown ERROR: {test_suite_name}")
@@ -488,6 +491,10 @@ def _run_test_suite(
"the next test suite may be affected."
)
test_suite_result.update_setup(Result.ERROR, e)
+
+ sut_node.exit_scope()
+ tg_node.exit_scope()
+
if len(test_suite_result.get_errors()) > 0 and test_suite.is_blocking:
raise BlockingTestSuiteError(test_suite_name)
@@ -511,6 +518,9 @@ def _execute_test_suite(
"""
self._logger.set_stage(DtsStage.test_suite)
for test_case in test_cases:
+ test_suite.sut_node.enter_scope("case")
+ test_suite.tg_node.enter_scope("case")
+
test_case_name = test_case.__name__
test_case_result = test_suite_result.add_test_case(test_case_name)
all_attempts = SETTINGS.re_run + 1
@@ -531,6 +541,9 @@ def _execute_test_suite(
)
test_case_result.update_setup(Result.SKIP)
+ test_suite.sut_node.exit_scope()
+ test_suite.tg_node.exit_scope()
+
def _run_test_case(
self,
test_suite: TestSuite,
diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py
index 6a7a1f5b6c..e883f59d11 100644
--- a/dts/framework/testbed_model/capability.py
+++ b/dts/framework/testbed_model/capability.py
@@ -221,24 +221,23 @@ def get_supported_capabilities(
)
if cls.capabilities_to_check:
capabilities_to_check_map = cls._get_decorated_capabilities_map()
- with TestPmdShell(
- sut_node, privileged=True, disable_device_start=True
- ) as testpmd_shell:
- for (
- conditional_capability_fn,
- capabilities,
- ) in capabilities_to_check_map.items():
- supported_capabilities: set[NicCapability] = set()
- unsupported_capabilities: set[NicCapability] = set()
- capability_fn = cls._reduce_capabilities(
- capabilities, supported_capabilities, unsupported_capabilities
- )
- if conditional_capability_fn:
- capability_fn = conditional_capability_fn(capability_fn)
- capability_fn(testpmd_shell)
- for capability in capabilities:
- if capability.nic_capability in supported_capabilities:
- supported_conditional_capabilities.add(capability)
+ testpmd_shell = TestPmdShell(sut_node, privileged=True, disable_device_start=True)
+ for (
+ conditional_capability_fn,
+ capabilities,
+ ) in capabilities_to_check_map.items():
+ supported_capabilities: set[NicCapability] = set()
+ unsupported_capabilities: set[NicCapability] = set()
+ capability_fn = cls._reduce_capabilities(
+ capabilities, supported_capabilities, unsupported_capabilities
+ )
+ if conditional_capability_fn:
+ capability_fn = conditional_capability_fn(capability_fn)
+ capability_fn(testpmd_shell)
+ for capability in capabilities:
+ if capability.nic_capability in supported_capabilities:
+ supported_conditional_capabilities.add(capability)
+ testpmd_shell._close()
logger.debug(f"Found supported capabilities {supported_conditional_capabilities}.")
return supported_conditional_capabilities
diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index c6f12319ca..4f06968adc 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -14,6 +14,7 @@
"""
from abc import ABC
+from typing import TYPE_CHECKING, Literal, TypeVar
from framework.config import (
OS,
@@ -21,7 +22,7 @@
NodeConfiguration,
TestRunConfiguration,
)
-from framework.exception import ConfigurationError
+from framework.exception import ConfigurationError, InternalError
from framework.logger import DTSLogger, get_dts_logger
from .cpu import (
@@ -35,6 +36,15 @@
from .os_session import OSSession
from .port import Port
+if TYPE_CHECKING:
+ from framework.remote_session.single_active_interactive_shell import (
+ SingleActiveInteractiveShell,
+ )
+
+T = TypeVar("T")
+Scope = Literal["unknown", "suite", "case"]
+ScopedShell = tuple[Scope, SingleActiveInteractiveShell]
+
class Node(ABC):
"""The base class for node management.
@@ -62,6 +72,8 @@ class Node(ABC):
_logger: DTSLogger
_other_sessions: list[OSSession]
_test_run_config: TestRunConfiguration
+ _active_shells: list[ScopedShell]
+ _scope_stack: list[Scope]
def __init__(self, node_config: NodeConfiguration):
"""Connect to the node and gather info during initialization.
@@ -90,6 +102,8 @@ def __init__(self, node_config: NodeConfiguration):
self._other_sessions = []
self._init_ports()
+ self._active_shells = []
+ self._scope_stack = []
def _init_ports(self) -> None:
self.ports = [Port(self.name, port_config) for port_config in self.config.ports]
@@ -119,6 +133,55 @@ def tear_down_test_run(self) -> None:
Additional steps can be added by extending the method in subclasses with the use of super().
"""
+ @property
+ def current_scope(self) -> Scope:
+ """The current scope of the test run."""
+ try:
+ return self._scope_stack[-1]
+ except IndexError:
+ return "unknown"
+
+ def enter_scope(self, next_scope: Scope) -> None:
+ """Prepare the node for a new testing scope."""
+ self._scope_stack.append(next_scope)
+
+ def exit_scope(self) -> Scope:
+ """Clean up the node after the current testing scope.
+
+ This method must guarantee to never fail from a Node failure during runtime.
+
+ Returns:
+ The scope before exiting.
+
+ Raises:
+ InternalError: If there was no scope to exit from.
+ """
+ try:
+ current_scope = self._scope_stack.pop()
+ except IndexError:
+ raise InternalError("Attempted to exit a scope when the node wasn't in any.")
+ else:
+ self.clean_up_shells(current_scope)
+ return current_scope
+
+ def register_shell(self, shell: SingleActiveInteractiveShell) -> None:
+ """Register a new shell to the pool of active shells."""
+ self._active_shells.append((self.current_scope, shell))
+
+ def find_active_shell(self, shell_class: type[T]) -> T | None:
+ """Retrieve an active shell of a specific class."""
+ return next(sh for _, sh in self._active_shells if type(sh) is shell_class)
+
+ def clean_up_shells(self, scope: Scope) -> None:
+ """Clean up shells from the given `scope`."""
+ zombie_shells_indices = [
+ i for i, (shell_scope, _) in enumerate(self._active_shells) if scope == shell_scope
+ ]
+
+ for i in reversed(zombie_shells_indices):
+ self._active_shells[i][1]._close()
+ del self._active_shells[i]
+
def create_session(self, name: str) -> OSSession:
"""Create and return a new OS-aware remote session.
diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
index a9dc0a474a..3427596bd0 100644
--- a/dts/framework/testbed_model/sut_node.py
+++ b/dts/framework/testbed_model/sut_node.py
@@ -33,7 +33,7 @@
from framework.remote_session.remote_session import CommandResult
from framework.utils import MesonArgs, TarCompressionFormat
-from .node import Node
+from .node import Node, Scope
from .os_session import OSSession, OSSessionInfo
from .virtual_device import VirtualDevice
@@ -458,7 +458,7 @@ def build_dpdk_app(self, app_name: str, **meson_dpdk_args: str | bool) -> PurePa
self.remote_dpdk_build_dir, "examples", f"dpdk-{app_name}"
)
- def kill_cleanup_dpdk_apps(self) -> None:
+ def _kill_cleanup_dpdk_apps(self) -> None:
"""Kill all dpdk applications on the SUT, then clean up hugepages."""
if self._dpdk_kill_session and self._dpdk_kill_session.is_alive():
# we can use the session if it exists and responds
@@ -468,6 +468,16 @@ def kill_cleanup_dpdk_apps(self) -> None:
self._dpdk_kill_session = self.create_session("dpdk_kill")
self.dpdk_prefix_list = []
+ def exit_scope(self) -> Scope:
+ """Extend :meth:`~.node.Node.exit_test_suite_scope`.
+
+ Add the DPDK apps clean up.
+ """
+ previous_scope = super().exit_scope()
+ if previous_scope == "suite":
+ self._kill_cleanup_dpdk_apps()
+ return previous_scope
+
def run_dpdk_app(
self, app_path: PurePath, eal_params: EalParams, timeout: float = 30
) -> CommandResult:
diff --git a/dts/tests/TestSuite_blocklist.py b/dts/tests/TestSuite_blocklist.py
index b9e9cd1d1a..edce042f38 100644
--- a/dts/tests/TestSuite_blocklist.py
+++ b/dts/tests/TestSuite_blocklist.py
@@ -18,16 +18,16 @@ class TestBlocklist(TestSuite):
def verify_blocklisted_ports(self, ports_to_block: list[Port]):
"""Runs testpmd with the given ports blocklisted and verifies the ports."""
- with TestPmdShell(self.sut_node, allowed_ports=[], blocked_ports=ports_to_block) as testpmd:
- allowlisted_ports = {port.device_name for port in testpmd.show_port_info_all()}
- blocklisted_ports = {port.pci for port in ports_to_block}
+ testpmd = TestPmdShell(self.sut_node, allowed_ports=[], blocked_ports=ports_to_block)
+ allowlisted_ports = {port.device_name for port in testpmd.show_port_info_all()}
+ blocklisted_ports = {port.pci for port in ports_to_block}
- # sanity check
- allowed_len = len(allowlisted_ports - blocklisted_ports)
- self.verify(allowed_len > 0, "At least one port should have been allowed")
+ # sanity check
+ allowed_len = len(allowlisted_ports - blocklisted_ports)
+ self.verify(allowed_len > 0, "At least one port should have been allowed")
- blocked = not allowlisted_ports & blocklisted_ports
- self.verify(blocked, "At least one port was not blocklisted")
+ blocked = not allowlisted_ports & blocklisted_ports
+ self.verify(blocked, "At least one port was not blocklisted")
@func_test
def no_blocklisted(self):
diff --git a/dts/tests/TestSuite_checksum_offload.py b/dts/tests/TestSuite_checksum_offload.py
index c1680bd388..58b9609849 100644
--- a/dts/tests/TestSuite_checksum_offload.py
+++ b/dts/tests/TestSuite_checksum_offload.py
@@ -117,16 +117,16 @@ def test_insert_checksums(self) -> None:
Ether(dst=mac_id) / IPv6(src="::1") / UDP() / Raw(payload),
Ether(dst=mac_id) / IPv6(src="::1") / TCP() / Raw(payload),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- self.setup_hw_offload(testpmd=testpmd)
- testpmd.start()
- self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
- for i in range(0, len(packet_list)):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ self.setup_hw_offload(testpmd=testpmd)
+ testpmd.start()
+ self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
+ for i in range(0, len(packet_list)):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
@func_test
def test_no_insert_checksums(self) -> None:
@@ -139,15 +139,15 @@ def test_no_insert_checksums(self) -> None:
Ether(dst=mac_id) / IPv6(src="::1") / UDP() / Raw(payload),
Ether(dst=mac_id) / IPv6(src="::1") / TCP() / Raw(payload),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- testpmd.start()
- self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
- for i in range(0, len(packet_list)):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ testpmd.start()
+ self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
+ for i in range(0, len(packet_list)):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
@func_test
def test_l4_rx_checksum(self) -> None:
@@ -159,18 +159,18 @@ def test_l4_rx_checksum(self) -> None:
Ether(dst=mac_id) / IP() / UDP(chksum=0xF),
Ether(dst=mac_id) / IP() / TCP(chksum=0xF),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- self.setup_hw_offload(testpmd=testpmd)
- for i in range(0, 2):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
- for i in range(2, 4):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ self.setup_hw_offload(testpmd=testpmd)
+ for i in range(0, 2):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
+ for i in range(2, 4):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
+ )
@func_test
def test_l3_rx_checksum(self) -> None:
@@ -182,18 +182,18 @@ def test_l3_rx_checksum(self) -> None:
Ether(dst=mac_id) / IP(chksum=0xF) / UDP(),
Ether(dst=mac_id) / IP(chksum=0xF) / TCP(),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- self.setup_hw_offload(testpmd=testpmd)
- for i in range(0, 2):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
- for i in range(2, 4):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=False, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ self.setup_hw_offload(testpmd=testpmd)
+ for i in range(0, 2):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
+ for i in range(2, 4):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=False, testpmd=testpmd, id=mac_id
+ )
@func_test
def test_validate_rx_checksum(self) -> None:
@@ -209,22 +209,22 @@ def test_validate_rx_checksum(self) -> None:
Ether(dst=mac_id) / IPv6(src="::1") / UDP(chksum=0xF),
Ether(dst=mac_id) / IPv6(src="::1") / TCP(chksum=0xF),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- self.setup_hw_offload(testpmd=testpmd)
- for i in range(0, 4):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
- for i in range(4, 6):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=False, goodIP=False, testpmd=testpmd, id=mac_id
- )
- for i in range(6, 8):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ self.setup_hw_offload(testpmd=testpmd)
+ for i in range(0, 4):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
+ for i in range(4, 6):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=False, goodIP=False, testpmd=testpmd, id=mac_id
+ )
+ for i in range(6, 8):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
+ )
@requires(NicCapability.RX_OFFLOAD_VLAN)
@func_test
@@ -238,20 +238,20 @@ def test_vlan_checksum(self) -> None:
Ether(dst=mac_id) / Dot1Q(vlan=1) / IPv6(src="::1") / UDP(chksum=0xF) / Raw(payload),
Ether(dst=mac_id) / Dot1Q(vlan=1) / IPv6(src="::1") / TCP(chksum=0xF) / Raw(payload),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- self.setup_hw_offload(testpmd=testpmd)
- testpmd.start()
- self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
- for i in range(0, 2):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=False, goodIP=False, testpmd=testpmd, id=mac_id
- )
- for i in range(2, 4):
- self.send_packet_and_verify_checksum(
- packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ self.setup_hw_offload(testpmd=testpmd)
+ testpmd.start()
+ self.send_packets_and_verify(packet_list=packet_list, load=payload, should_receive=True)
+ for i in range(0, 2):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=False, goodIP=False, testpmd=testpmd, id=mac_id
+ )
+ for i in range(2, 4):
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[i], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
+ )
@requires(NicCapability.RX_OFFLOAD_SCTP_CKSUM)
@func_test
@@ -262,14 +262,14 @@ def test_validate_sctp_checksum(self) -> None:
Ether(dst=mac_id) / IP() / SCTP(),
Ether(dst=mac_id) / IP() / SCTP(chksum=0xF),
]
- with TestPmdShell(node=self.sut_node, enable_rx_cksum=True) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.csum)
- testpmd.set_verbose(level=1)
- testpmd.csum_set_hw(layers=ChecksumOffloadOptions.sctp)
- testpmd.start()
- self.send_packet_and_verify_checksum(
- packet=packet_list[0], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
- )
- self.send_packet_and_verify_checksum(
- packet=packet_list[1], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
- )
+ testpmd = TestPmdShell(node=self.sut_node, enable_rx_cksum=True)
+ testpmd.set_forward_mode(SimpleForwardingModes.csum)
+ testpmd.set_verbose(level=1)
+ testpmd.csum_set_hw(layers=ChecksumOffloadOptions.sctp)
+ testpmd.start()
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[0], goodL4=True, goodIP=True, testpmd=testpmd, id=mac_id
+ )
+ self.send_packet_and_verify_checksum(
+ packet=packet_list[1], goodL4=False, goodIP=True, testpmd=testpmd, id=mac_id
+ )
diff --git a/dts/tests/TestSuite_dynamic_queue_conf.py b/dts/tests/TestSuite_dynamic_queue_conf.py
index e55716f545..caf820151e 100644
--- a/dts/tests/TestSuite_dynamic_queue_conf.py
+++ b/dts/tests/TestSuite_dynamic_queue_conf.py
@@ -83,37 +83,37 @@ def wrap(self: "TestDynamicQueueConf", is_rx_testing: bool) -> None:
while len(queues_to_config) < self.num_ports_to_modify:
queues_to_config.add(random.randint(1, self.number_of_queues - 1))
unchanged_queues = set(range(self.number_of_queues)) - queues_to_config
- with TestPmdShell(
+ testpmd = TestPmdShell(
self.sut_node,
port_topology=PortTopology.chained,
rx_queues=self.number_of_queues,
tx_queues=self.number_of_queues,
- ) as testpmd:
- for q in queues_to_config:
- testpmd.stop_port_queue(port_id, q, is_rx_testing)
- testpmd.set_forward_mode(SimpleForwardingModes.mac)
-
- test_meth(
- self,
- port_id,
- queues_to_config,
- unchanged_queues,
- testpmd,
- is_rx_testing,
- )
-
- for queue_id in queues_to_config:
- testpmd.start_port_queue(port_id, queue_id, is_rx_testing)
+ )
+ for q in queues_to_config:
+ testpmd.stop_port_queue(port_id, q, is_rx_testing)
+ testpmd.set_forward_mode(SimpleForwardingModes.mac)
+
+ test_meth(
+ self,
+ port_id,
+ queues_to_config,
+ unchanged_queues,
+ testpmd,
+ is_rx_testing,
+ )
+
+ for queue_id in queues_to_config:
+ testpmd.start_port_queue(port_id, queue_id, is_rx_testing)
- testpmd.start()
- self.send_packets_with_different_addresses(self.number_of_packets_to_send)
- forwarding_stats = testpmd.stop()
- for queue_id in queues_to_config:
- self.verify(
- self.port_queue_in_stats(port_id, is_rx_testing, queue_id, forwarding_stats),
- f"Modified queue {queue_id} on port {port_id} failed to receive traffic after"
- "being started again.",
- )
+ testpmd.start()
+ self.send_packets_with_different_addresses(self.number_of_packets_to_send)
+ forwarding_stats = testpmd.stop()
+ for queue_id in queues_to_config:
+ self.verify(
+ self.port_queue_in_stats(port_id, is_rx_testing, queue_id, forwarding_stats),
+ f"Modified queue {queue_id} on port {port_id} failed to receive traffic after"
+ "being started again.",
+ )
return wrap
diff --git a/dts/tests/TestSuite_l2fwd.py b/dts/tests/TestSuite_l2fwd.py
index 0f6ff18907..9acc4365ea 100644
--- a/dts/tests/TestSuite_l2fwd.py
+++ b/dts/tests/TestSuite_l2fwd.py
@@ -44,20 +44,20 @@ def l2fwd_integrity(self) -> None:
"""
queues = [1, 2, 4, 8]
- with TestPmdShell(
+ shell = TestPmdShell(
self.sut_node,
lcore_filter_specifier=LogicalCoreCount(cores_per_socket=4),
forward_mode=SimpleForwardingModes.mac,
eth_peer=[EthPeer(1, self.tg_node.ports[1].mac_address)],
disable_device_start=True,
- ) as shell:
- for queues_num in queues:
- self._logger.info(f"Testing L2 forwarding with {queues_num} queue(s)")
- shell.set_ports_queues(queues_num)
- shell.start()
+ )
+ for queues_num in queues:
+ self._logger.info(f"Testing L2 forwarding with {queues_num} queue(s)")
+ shell.set_ports_queues(queues_num)
+ shell.start()
- received_packets = self.send_packets_and_capture(self.packets)
- expected_packets = self.get_expected_packets(self.packets)
- self.match_all_packets(expected_packets, received_packets)
+ received_packets = self.send_packets_and_capture(self.packets)
+ expected_packets = self.get_expected_packets(self.packets)
+ self.match_all_packets(expected_packets, received_packets)
- shell.stop()
+ shell.stop()
diff --git a/dts/tests/TestSuite_mac_filter.py b/dts/tests/TestSuite_mac_filter.py
index 11e4b595c7..ac9ceefd85 100644
--- a/dts/tests/TestSuite_mac_filter.py
+++ b/dts/tests/TestSuite_mac_filter.py
@@ -101,22 +101,22 @@ def test_add_remove_mac_addresses(self) -> None:
Remove the fake mac address from the PMD's address pool.
Send a packet with the fake mac address to the PMD. (Should not receive)
"""
- with TestPmdShell(self.sut_node) as testpmd:
- testpmd.set_promisc(0, enable=False)
- testpmd.start()
- mac_address = self._sut_port_ingress.mac_address
-
- # Send a packet with NIC default mac address
- self.send_packet_and_verify(mac_address=mac_address, should_receive=True)
- # Send a packet with different mac address
- fake_address = "00:00:00:00:00:01"
- self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
-
- # Add mac address to pool and rerun tests
- testpmd.set_mac_addr(0, mac_address=fake_address, add=True)
- self.send_packet_and_verify(mac_address=fake_address, should_receive=True)
- testpmd.set_mac_addr(0, mac_address=fake_address, add=False)
- self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
+ testpmd = TestPmdShell(self.sut_node)
+ testpmd.set_promisc(0, enable=False)
+ testpmd.start()
+ mac_address = self._sut_port_ingress.mac_address
+
+ # Send a packet with NIC default mac address
+ self.send_packet_and_verify(mac_address=mac_address, should_receive=True)
+ # Send a packet with different mac address
+ fake_address = "00:00:00:00:00:01"
+ self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
+
+ # Add mac address to pool and rerun tests
+ testpmd.set_mac_addr(0, mac_address=fake_address, add=True)
+ self.send_packet_and_verify(mac_address=fake_address, should_receive=True)
+ testpmd.set_mac_addr(0, mac_address=fake_address, add=False)
+ self.send_packet_and_verify(mac_address=fake_address, should_receive=False)
@func_test
def test_invalid_address(self) -> None:
@@ -137,44 +137,42 @@ def test_invalid_address(self) -> None:
Determine the device's mac address pool size, and fill the pool with fake addresses.
Attempt to add another fake mac address, overloading the address pool. (Should fail)
"""
- with TestPmdShell(self.sut_node) as testpmd:
- testpmd.start()
- mac_address = self._sut_port_ingress.mac_address
- try:
- testpmd.set_mac_addr(0, "00:00:00:00:00:00", add=True)
- self.verify(False, "Invalid mac address added.")
- except InteractiveCommandExecutionError:
- pass
- try:
- testpmd.set_mac_addr(0, mac_address, add=False)
- self.verify(False, "Default mac address removed.")
- except InteractiveCommandExecutionError:
- pass
- # Should be no errors adding this twice
- testpmd.set_mac_addr(0, "1" + mac_address[1:], add=True)
- testpmd.set_mac_addr(0, "1" + mac_address[1:], add=True)
- # Double check to see if default mac address can be removed
- try:
- testpmd.set_mac_addr(0, mac_address, add=False)
- self.verify(False, "Default mac address removed.")
- except InteractiveCommandExecutionError:
- pass
-
- for i in range(testpmd.show_port_info(0).max_mac_addresses_num - 1):
- # A0 fake address based on the index 'i'.
- fake_address = str(hex(i)[2:].zfill(12))
- # Insert ':' characters every two indexes to create a fake mac address.
- fake_address = ":".join(
- fake_address[x : x + 2] for x in range(0, len(fake_address), 2)
- )
- testpmd.set_mac_addr(0, fake_address, add=True, verify=False)
- try:
- testpmd.set_mac_addr(0, "E" + mac_address[1:], add=True)
- # We add an extra address to compensate for mac address pool inconsistencies.
- testpmd.set_mac_addr(0, "F" + mac_address[1:], add=True)
- self.verify(False, "Mac address limit exceeded.")
- except InteractiveCommandExecutionError:
- pass
+ testpmd = TestPmdShell(self.sut_node)
+ testpmd.start()
+ mac_address = self._sut_port_ingress.mac_address
+ try:
+ testpmd.set_mac_addr(0, "00:00:00:00:00:00", add=True)
+ self.verify(False, "Invalid mac address added.")
+ except InteractiveCommandExecutionError:
+ pass
+ try:
+ testpmd.set_mac_addr(0, mac_address, add=False)
+ self.verify(False, "Default mac address removed.")
+ except InteractiveCommandExecutionError:
+ pass
+ # Should be no errors adding this twice
+ testpmd.set_mac_addr(0, "1" + mac_address[1:], add=True)
+ testpmd.set_mac_addr(0, "1" + mac_address[1:], add=True)
+ # Double check to see if default mac address can be removed
+ try:
+ testpmd.set_mac_addr(0, mac_address, add=False)
+ self.verify(False, "Default mac address removed.")
+ except InteractiveCommandExecutionError:
+ pass
+
+ for i in range(testpmd.show_port_info(0).max_mac_addresses_num - 1):
+ # A0 fake address based on the index 'i'.
+ fake_address = str(hex(i)[2:].zfill(12))
+ # Insert ':' characters every two indexes to create a fake mac address.
+ fake_address = ":".join(fake_address[x : x + 2] for x in range(0, len(fake_address), 2))
+ testpmd.set_mac_addr(0, fake_address, add=True, verify=False)
+ try:
+ testpmd.set_mac_addr(0, "E" + mac_address[1:], add=True)
+ # We add an extra address to compensate for mac address pool inconsistencies.
+ testpmd.set_mac_addr(0, "F" + mac_address[1:], add=True)
+ self.verify(False, "Mac address limit exceeded.")
+ except InteractiveCommandExecutionError:
+ pass
@requires(NicCapability.MCAST_FILTERING)
@func_test
@@ -191,14 +189,14 @@ def test_multicast_filter(self) -> None:
Remove the fake multicast address from the PMDs multicast address filter.
Send a packet with the fake multicast address to the PMD. (Should not receive)
"""
- with TestPmdShell(self.sut_node) as testpmd:
- testpmd.start()
- testpmd.set_promisc(0, enable=False)
- multicast_address = "01:00:5E:00:00:00"
+ testpmd = TestPmdShell(self.sut_node)
+ testpmd.set_promisc(0, enable=False)
+ multicast_address = "01:00:5E:00:00:00"
- testpmd.set_multicast_mac_addr(0, multi_addr=multicast_address, add=True)
- self.send_packet_and_verify(multicast_address, should_receive=True)
+ testpmd.set_multicast_mac_addr(0, multi_addr=multicast_address, add=True)
+ testpmd.start()
+ self.send_packet_and_verify(multicast_address, should_receive=True)
- # Remove multicast filter and verify the packet was not received.
- testpmd.set_multicast_mac_addr(0, multicast_address, add=False)
- self.send_packet_and_verify(multicast_address, should_receive=False)
+ # Remove multicast filter and verify the packet was not received.
+ testpmd.set_multicast_mac_addr(0, multicast_address, add=False)
+ self.send_packet_and_verify(multicast_address, should_receive=False)
diff --git a/dts/tests/TestSuite_pmd_buffer_scatter.py b/dts/tests/TestSuite_pmd_buffer_scatter.py
index b2f42425d4..ffb345ae4e 100644
--- a/dts/tests/TestSuite_pmd_buffer_scatter.py
+++ b/dts/tests/TestSuite_pmd_buffer_scatter.py
@@ -103,26 +103,24 @@ def pmd_scatter(self, mbsize: int) -> None:
Test:
Start testpmd and run functional test with preset mbsize.
"""
- with TestPmdShell(
+ testpmd = TestPmdShell(
self.sut_node,
forward_mode=SimpleForwardingModes.mac,
mbcache=200,
mbuf_size=[mbsize],
max_pkt_len=9000,
tx_offloads=0x00008000,
- ) as testpmd:
- testpmd.start()
-
- for offset in [-1, 0, 1, 4, 5]:
- recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
- self._logger.debug(
- f"Payload of scattered packet after forwarding: \n{recv_payload}"
- )
- self.verify(
- ("58 " * 8).strip() in recv_payload,
- "Payload of scattered packet did not match expected payload with offset "
- f"{offset}.",
- )
+ )
+ testpmd.start()
+
+ for offset in [-1, 0, 1, 4, 5]:
+ recv_payload = self.scatter_pktgen_send_packet(mbsize + offset)
+ self._logger.debug(f"Payload of scattered packet after forwarding: \n{recv_payload}")
+ self.verify(
+ ("58 " * 8).strip() in recv_payload,
+ "Payload of scattered packet did not match expected payload with offset "
+ f"{offset}.",
+ )
@requires(NicCapability.SCATTERED_RX_ENABLED)
@func_test
diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
index bc3a2a6bf9..0681ad4ea7 100644
--- a/dts/tests/TestSuite_smoke_tests.py
+++ b/dts/tests/TestSuite_smoke_tests.py
@@ -104,8 +104,8 @@ def test_devices_listed_in_testpmd(self) -> None:
Test:
List all devices found in testpmd and verify the configured devices are among them.
"""
- with TestPmdShell(self.sut_node) as testpmd:
- dev_list = [str(x) for x in testpmd.get_devices()]
+ testpmd = TestPmdShell(self.sut_node)
+ dev_list = [str(x) for x in testpmd.get_devices()]
for nic in self.nics_in_node:
self.verify(
nic.pci in dev_list,
diff --git a/dts/tests/TestSuite_vlan.py b/dts/tests/TestSuite_vlan.py
index c67520baef..ede1f69495 100644
--- a/dts/tests/TestSuite_vlan.py
+++ b/dts/tests/TestSuite_vlan.py
@@ -124,10 +124,10 @@ def test_vlan_receipt_no_stripping(self) -> None:
Test:
Create an interactive testpmd shell and verify a VLAN packet.
"""
- with TestPmdShell(node=self.sut_node) as testpmd:
- self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
- testpmd.start()
- self.send_vlan_packet_and_verify(True, strip=False, vlan_id=1)
+ testpmd = TestPmdShell(node=self.sut_node)
+ self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
+ testpmd.start()
+ self.send_vlan_packet_and_verify(True, strip=False, vlan_id=1)
@requires(NicCapability.RX_OFFLOAD_VLAN_STRIP)
@func_test
@@ -137,11 +137,11 @@ def test_vlan_receipt_stripping(self) -> None:
Test:
Create an interactive testpmd shell and verify a VLAN packet.
"""
- with TestPmdShell(node=self.sut_node) as testpmd:
- self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
- testpmd.set_vlan_strip(port=0, enable=True)
- testpmd.start()
- self.send_vlan_packet_and_verify(should_receive=True, strip=True, vlan_id=1)
+ testpmd = TestPmdShell(node=self.sut_node)
+ self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
+ testpmd.set_vlan_strip(port=0, enable=True)
+ testpmd.start()
+ self.send_vlan_packet_and_verify(should_receive=True, strip=True, vlan_id=1)
@func_test
def test_vlan_no_receipt(self) -> None:
@@ -150,10 +150,10 @@ def test_vlan_no_receipt(self) -> None:
Test:
Create an interactive testpmd shell and verify a VLAN packet.
"""
- with TestPmdShell(node=self.sut_node) as testpmd:
- self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
- testpmd.start()
- self.send_vlan_packet_and_verify(should_receive=False, strip=False, vlan_id=2)
+ testpmd = TestPmdShell(node=self.sut_node)
+ self.vlan_setup(testpmd=testpmd, port_id=0, filtered_id=1)
+ testpmd.start()
+ self.send_vlan_packet_and_verify(should_receive=False, strip=False, vlan_id=2)
@func_test
def test_vlan_header_insertion(self) -> None:
@@ -162,11 +162,11 @@ def test_vlan_header_insertion(self) -> None:
Test:
Create an interactive testpmd shell and verify a non-VLAN packet.
"""
- with TestPmdShell(node=self.sut_node) as testpmd:
- testpmd.set_forward_mode(SimpleForwardingModes.mac)
- testpmd.set_promisc(port=0, enable=False)
- testpmd.stop_all_ports()
- testpmd.tx_vlan_set(port=1, enable=True, vlan=51)
- testpmd.start_all_ports()
- testpmd.start()
- self.send_packet_and_verify_insertion(expected_id=51)
+ testpmd = TestPmdShell(node=self.sut_node)
+ testpmd.set_forward_mode(SimpleForwardingModes.mac)
+ testpmd.set_promisc(port=0, enable=False)
+ testpmd.stop_all_ports()
+ testpmd.tx_vlan_set(port=1, enable=True, vlan=51)
+ testpmd.start_all_ports()
+ testpmd.start()
+ self.send_packet_and_verify_insertion(expected_id=51)
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 2/2] dts: revert back shell split
2024-12-20 17:23 [RFC PATCH 0/2] dts: add basic scope to improve shell handling Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 1/2] dts: add scoping and shell registration to Node Luca Vizzarro
@ 2024-12-20 17:24 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
2 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2024-12-20 17:24 UTC (permalink / raw)
To: dev; +Cc: Paul Szczepanek, Patrick Robb, Luca Vizzarro
The InteractiveShell was previously renamed to
SingleActiveInteractiveShell to represent a shell that can only be run
once. The mechanism used to enforce this was a context manager, which
turned out to be more constrictive on test suite development.
Shell closure is now handled by the scoping mechanism, and an attribute
is used to enforce the single active shell. Also the split has been
reverted.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
---
dts/framework/remote_session/dpdk_shell.py | 8 +-
.../remote_session/interactive_shell.py | 262 +++++++++++++++--
.../single_active_interactive_shell.py | 268 ------------------
dts/framework/remote_session/testpmd_shell.py | 4 +-
dts/framework/testbed_model/capability.py | 2 +-
dts/framework/testbed_model/node.py | 10 +-
6 files changed, 250 insertions(+), 304 deletions(-)
delete mode 100644 dts/framework/remote_session/single_active_interactive_shell.py
diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index c11d9ab81c..c37dcb2b62 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -8,10 +8,11 @@
from abc import ABC
from pathlib import PurePath
+from typing import ClassVar
from framework.params.eal import EalParams
-from framework.remote_session.single_active_interactive_shell import (
- SingleActiveInteractiveShell,
+from framework.remote_session.interactive_shell import (
+ InteractiveShell,
)
from framework.settings import SETTINGS
from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
@@ -61,7 +62,7 @@ def compute_eal_params(
return params
-class DPDKShell(SingleActiveInteractiveShell, ABC):
+class DPDKShell(InteractiveShell, ABC):
"""The base class for managing DPDK-based interactive shells.
This class shouldn't be instantiated directly, but instead be extended.
@@ -71,6 +72,7 @@ class DPDKShell(SingleActiveInteractiveShell, ABC):
_node: SutNode
_app_params: EalParams
+ _single_active_per_node: ClassVar[bool] = True
def __init__(
self,
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 9ca285b604..a136419181 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -1,44 +1,256 @@
# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2023 University of New Hampshire
+# Copyright(c) 2024 University of New Hampshire
# Copyright(c) 2024 Arm Limited
-"""Interactive shell with manual stop/start functionality.
+"""Common functionality for interactive shell handling.
-Provides a class that doesn't require being started/stopped using a context manager and can instead
-be started and stopped manually, or have the stopping process be handled at the time of garbage
-collection.
+The base class, :class:`InteractiveShell`, is meant to be extended by subclasses that
+contain functionality specific to that shell type. These subclasses will often modify things like
+the prompt to expect or the arguments to pass into the application, but still utilize
+the same method for sending a command and collecting output. How this output is handled however
+is often application specific. If an application needs elevated privileges to start it is expected
+that the method for gaining those privileges is provided when initializing the class.
+
+The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
+environment variable configure the timeout of getting the output from command execution.
"""
-import weakref
+from abc import ABC
+from pathlib import PurePath
from typing import ClassVar
-from .single_active_interactive_shell import SingleActiveInteractiveShell
+from paramiko import Channel, channel
+
+from framework.exception import (
+ InteractiveCommandExecutionError,
+ InteractiveSSHSessionDeadError,
+ InteractiveSSHTimeoutError,
+ InternalError,
+)
+from framework.logger import DTSLogger, get_dts_logger
+from framework.params import Params
+from framework.settings import SETTINGS
+from framework.testbed_model.node import Node
+from framework.utils import MultiInheritanceBaseClass
+
+
+class InteractiveShell(MultiInheritanceBaseClass, ABC):
+ """The base class for managing interactive shells.
+ This class shouldn't be instantiated directly, but instead be extended. It contains
+ methods for starting interactive shells as well as sending commands to these shells
+ and collecting input until reaching a certain prompt. All interactive applications
+ will use the same SSH connection, but each will create their own channel on that
+ session.
-class InteractiveShell(SingleActiveInteractiveShell):
- """Adds manual start and stop functionality to interactive shells.
+ Interactive shells are started and stopped using a context manager. This allows for the start
+ and cleanup of the application to happen at predictable times regardless of exceptions or
+ interrupts.
- Like its super-class, this class should not be instantiated directly and should instead be
- extended. This class also provides an option for automated cleanup of the application using a
- weakref and a finalize class. This finalize class allows for cleanup of the class at the time
- of garbage collection and also ensures that cleanup only happens once. This way if a user
- initiates the closing of the shell manually it is not repeated at the time of garbage
- collection.
+ Attributes:
+ is_alive: :data:`True` if the application has started successfully, :data:`False`
+ otherwise.
"""
- _finalizer: weakref.finalize
- #: One attempt should be enough for shells which don't have to worry about other instances
- #: closing before starting a new one.
- _init_attempts: ClassVar[int] = 1
+ _node: Node
+ _stdin: channel.ChannelStdinFile
+ _stdout: channel.ChannelFile
+ _ssh_channel: Channel
+ _logger: DTSLogger
+ _timeout: float
+ _app_params: Params
+ _privileged: bool
+ _real_path: PurePath
+
+ #: The number of times to try starting the application before considering it a failure.
+ _init_attempts: ClassVar[int] = 5
+
+ #: Prompt to expect at the end of output when sending a command.
+ #: This is often overridden by subclasses.
+ _default_prompt: ClassVar[str] = ""
+
+ #: Extra characters to add to the end of every command
+ #: before sending them. This is often overridden by subclasses and is
+ #: most commonly an additional newline character. This additional newline
+ #: character is used to force the line that is currently awaiting input
+ #: into the stdout buffer so that it can be consumed and checked against
+ #: the expected prompt.
+ _command_extra_chars: ClassVar[str] = ""
+
+ #: Condition which constraints the user of the class from attempting to run more than one
+ #: shell on the same node at the same time.
+ _single_active_per_node: ClassVar[bool] = False
+
+ #: Path to the executable to start the interactive application.
+ path: ClassVar[PurePath]
+
+ is_alive: bool = False
+
+ def __init__(
+ self,
+ node: Node,
+ privileged: bool = False,
+ timeout: float = SETTINGS.timeout,
+ app_params: Params = Params(),
+ name: str | None = None,
+ **kwargs,
+ ) -> None:
+ """Create an SSH channel during initialization.
+
+ Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other
+ constructors in the case of multiple inheritance.
+
+ Args:
+ node: The node on which to run start the interactive shell.
+ privileged: Enables the shell to run as superuser.
+ timeout: The timeout used for the SSH channel that is dedicated to this interactive
+ shell. This timeout is for collecting output, so if reading from the buffer
+ and no output is gathered within the timeout, an exception is thrown.
+ app_params: The command line parameters to be passed to the application on startup.
+ name: Name for the interactive shell to use for logging. This name will be appended to
+ the name of the underlying node which it is running on.
+ **kwargs: Any additional arguments if any.
+
+ Raises:
+ InternalError: If :attr:`_single_active_per_node` is :data:`True` and another shell of
+ the same class is already running.
+ """
+ if self._single_active_per_node and node.find_active_shell(type(self)) is not None:
+ raise InternalError(
+ "Attempted to run a single-active shell while another one was already open."
+ )
+
+ node.register_shell(self)
+ self._node = node
+
+ if name is None:
+ name = type(self).__name__
+ self._logger = get_dts_logger(f"{node.name}.{name}")
+ self._app_params = app_params
+ self._privileged = privileged
+ self._timeout = timeout
+ # Ensure path is properly formatted for the host
+ self._update_real_path(self.path)
+ super().__init__()
+
+ self.start_application()
+
+ def _setup_ssh_channel(self):
+ self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell()
+ self._stdin = self._ssh_channel.makefile_stdin("w")
+ self._stdout = self._ssh_channel.makefile("r")
+ self._ssh_channel.settimeout(self._timeout)
+ self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams
+
+ def _make_start_command(self) -> str:
+ """Makes the command that starts the interactive shell."""
+ start_command = f"{self._real_path} {self._app_params or ''}"
+ if self._privileged:
+ start_command = self._node.main_session._get_privileged_command(start_command)
+ return start_command
def start_application(self) -> None:
- """Start the application.
+ """Starts a new interactive application based on the path to the app.
+
+ This method is often overridden by subclasses as their process for starting may look
+ different. Initialization of the shell on the host can be retried up to
+ `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly
+ more time after exiting their script to clean up EAL before others can start.
+
+ Raises:
+ InteractiveCommandExecutionError: If the application fails to start within the allotted
+ number of retries.
+ """
+ self._setup_ssh_channel()
+ self._ssh_channel.settimeout(5)
+ start_command = self._make_start_command()
+ self.is_alive = True
+ for attempt in range(self._init_attempts):
+ try:
+ self.send_command(start_command)
+ break
+ except InteractiveSSHTimeoutError:
+ self._logger.info(
+ f"Interactive shell failed to start (attempt {attempt+1} out of "
+ f"{self._init_attempts})"
+ )
+ else:
+ self._ssh_channel.settimeout(self._timeout)
+ self.is_alive = False # update state on failure to start
+ raise InteractiveCommandExecutionError("Failed to start application.")
+ self._ssh_channel.settimeout(self._timeout)
+
+ def send_command(
+ self, command: str, prompt: str | None = None, skip_first_line: bool = False
+ ) -> str:
+ """Send `command` and get all output before the expected ending string.
+
+ Lines that expect input are not included in the stdout buffer, so they cannot
+ be used for expect.
+
+ Example:
+ If you were prompted to log into something with a username and password,
+ you cannot expect ``username:`` because it won't yet be in the stdout buffer.
+ A workaround for this could be consuming an extra newline character to force
+ the current `prompt` into the stdout buffer.
- After the application has started, use :class:`weakref.finalize` to manage cleanup.
+ Args:
+ command: The command to send.
+ prompt: After sending the command, `send_command` will be expecting this string.
+ If :data:`None`, will use the class's default prompt.
+ skip_first_line: Skip the first line when capturing the output.
+
+ Returns:
+ All output in the buffer before expected string.
+
+ Raises:
+ InteractiveCommandExecutionError: If attempting to send a command to a shell that is
+ not currently running.
+ InteractiveSSHSessionDeadError: The session died while executing the command.
+ InteractiveSSHTimeoutError: If command was sent but prompt could not be found in
+ the output before the timeout.
"""
- self._start_application()
- self._finalizer = weakref.finalize(self, self._close)
+ if not self.is_alive:
+ raise InteractiveCommandExecutionError(
+ f"Cannot send command {command} to application because the shell is not running."
+ )
+ self._logger.info(f"Sending: '{command}'")
+ if prompt is None:
+ prompt = self._default_prompt
+ out: str = ""
+ try:
+ self._stdin.write(f"{command}{self._command_extra_chars}\n")
+ self._stdin.flush()
+ for line in self._stdout:
+ if skip_first_line:
+ skip_first_line = False
+ continue
+ if line.rstrip().endswith(prompt):
+ break
+ out += line
+ except TimeoutError as e:
+ self._logger.exception(e)
+ self._logger.debug(
+ f"Prompt ({prompt}) was not found in output from command before timeout."
+ )
+ raise InteractiveSSHTimeoutError(command) from e
+ except OSError as e:
+ self._logger.exception(e)
+ raise InteractiveSSHSessionDeadError(
+ self._node.main_session.interactive_session.hostname
+ ) from e
+ finally:
+ self._logger.debug(f"Got output: {out}")
+ return out
def close(self) -> None:
- """Free all resources using :class:`weakref.finalize`."""
- self._finalizer()
+ """Close the shell."""
+ if not self._stdin.closed:
+ self._stdin.close()
+ if not self._ssh_channel.closed:
+ self._ssh_channel.close()
+ self.is_alive = False
+
+ def _update_real_path(self, path: PurePath) -> None:
+ """Updates the interactive shell's real path used at command line."""
+ self._real_path = self._node.main_session.join_remote_path(path)
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
deleted file mode 100644
index 910af8f655..0000000000
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ /dev/null
@@ -1,268 +0,0 @@
-# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2024 University of New Hampshire
-
-"""Common functionality for interactive shell handling.
-
-The base class, :class:`SingleActiveInteractiveShell`, is meant to be extended by subclasses that
-contain functionality specific to that shell type. These subclasses will often modify things like
-the prompt to expect or the arguments to pass into the application, but still utilize
-the same method for sending a command and collecting output. How this output is handled however
-is often application specific. If an application needs elevated privileges to start it is expected
-that the method for gaining those privileges is provided when initializing the class.
-
-This class is designed for applications like primary applications in DPDK where only one instance
-of the application can be running at a given time and, for this reason, is managed using a context
-manager. This context manager starts the application when you enter the context and cleans up the
-application when you exit. Using a context manager for this is useful since it allows us to ensure
-the application is cleaned up as soon as you leave the block regardless of the reason.
-
-The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
-environment variable configure the timeout of getting the output from command execution.
-"""
-
-from abc import ABC
-from pathlib import PurePath
-from typing import ClassVar
-
-from paramiko import Channel, channel
-from typing_extensions import Self
-
-from framework.exception import (
- InteractiveCommandExecutionError,
- InteractiveSSHSessionDeadError,
- InteractiveSSHTimeoutError,
-)
-from framework.logger import DTSLogger, get_dts_logger
-from framework.params import Params
-from framework.settings import SETTINGS
-from framework.testbed_model.node import Node
-from framework.utils import MultiInheritanceBaseClass
-
-
-class SingleActiveInteractiveShell(MultiInheritanceBaseClass, ABC):
- """The base class for managing interactive shells.
-
- This class shouldn't be instantiated directly, but instead be extended. It contains
- methods for starting interactive shells as well as sending commands to these shells
- and collecting input until reaching a certain prompt. All interactive applications
- will use the same SSH connection, but each will create their own channel on that
- session.
-
- Interactive shells are started and stopped using a context manager. This allows for the start
- and cleanup of the application to happen at predictable times regardless of exceptions or
- interrupts.
-
- Attributes:
- is_alive: :data:`True` if the application has started successfully, :data:`False`
- otherwise.
- """
-
- _node: Node
- _stdin: channel.ChannelStdinFile
- _stdout: channel.ChannelFile
- _ssh_channel: Channel
- _logger: DTSLogger
- _timeout: float
- _app_params: Params
- _privileged: bool
- _real_path: PurePath
-
- #: The number of times to try starting the application before considering it a failure.
- _init_attempts: ClassVar[int] = 5
-
- #: Prompt to expect at the end of output when sending a command.
- #: This is often overridden by subclasses.
- _default_prompt: ClassVar[str] = ""
-
- #: Extra characters to add to the end of every command
- #: before sending them. This is often overridden by subclasses and is
- #: most commonly an additional newline character. This additional newline
- #: character is used to force the line that is currently awaiting input
- #: into the stdout buffer so that it can be consumed and checked against
- #: the expected prompt.
- _command_extra_chars: ClassVar[str] = ""
-
- #: Path to the executable to start the interactive application.
- path: ClassVar[PurePath]
-
- is_alive: bool = False
-
- def __init__(
- self,
- node: Node,
- privileged: bool = False,
- timeout: float = SETTINGS.timeout,
- app_params: Params = Params(),
- name: str | None = None,
- **kwargs,
- ) -> None:
- """Create an SSH channel during initialization.
-
- Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other
- constructors in the case of multiple inheritance.
-
- Args:
- node: The node on which to run start the interactive shell.
- privileged: Enables the shell to run as superuser.
- timeout: The timeout used for the SSH channel that is dedicated to this interactive
- shell. This timeout is for collecting output, so if reading from the buffer
- and no output is gathered within the timeout, an exception is thrown.
- app_params: The command line parameters to be passed to the application on startup.
- name: Name for the interactive shell to use for logging. This name will be appended to
- the name of the underlying node which it is running on.
- **kwargs: Any additional arguments if any.
- """
- node.register_shell(self)
- self._node = node
-
- if name is None:
- name = type(self).__name__
- self._logger = get_dts_logger(f"{node.name}.{name}")
- self._app_params = app_params
- self._privileged = privileged
- self._timeout = timeout
- # Ensure path is properly formatted for the host
- self._update_real_path(self.path)
- super().__init__()
-
- def _setup_ssh_channel(self):
- self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell()
- self._stdin = self._ssh_channel.makefile_stdin("w")
- self._stdout = self._ssh_channel.makefile("r")
- self._ssh_channel.settimeout(self._timeout)
- self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams
-
- def _make_start_command(self) -> str:
- """Makes the command that starts the interactive shell."""
- start_command = f"{self._real_path} {self._app_params or ''}"
- if self._privileged:
- start_command = self._node.main_session._get_privileged_command(start_command)
- return start_command
-
- def _start_application(self) -> None:
- """Starts a new interactive application based on the path to the app.
-
- This method is often overridden by subclasses as their process for starting may look
- different. Initialization of the shell on the host can be retried up to
- `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly
- more time after exiting their script to clean up EAL before others can start.
-
- Raises:
- InteractiveCommandExecutionError: If the application fails to start within the allotted
- number of retries.
- """
- self._setup_ssh_channel()
- self._ssh_channel.settimeout(5)
- start_command = self._make_start_command()
- self.is_alive = True
- for attempt in range(self._init_attempts):
- try:
- self.send_command(start_command)
- break
- except InteractiveSSHTimeoutError:
- self._logger.info(
- f"Interactive shell failed to start (attempt {attempt+1} out of "
- f"{self._init_attempts})"
- )
- else:
- self._ssh_channel.settimeout(self._timeout)
- self.is_alive = False # update state on failure to start
- raise InteractiveCommandExecutionError("Failed to start application.")
- self._ssh_channel.settimeout(self._timeout)
-
- def send_command(
- self, command: str, prompt: str | None = None, skip_first_line: bool = False
- ) -> str:
- """Send `command` and get all output before the expected ending string.
-
- Lines that expect input are not included in the stdout buffer, so they cannot
- be used for expect.
-
- Example:
- If you were prompted to log into something with a username and password,
- you cannot expect ``username:`` because it won't yet be in the stdout buffer.
- A workaround for this could be consuming an extra newline character to force
- the current `prompt` into the stdout buffer.
-
- Args:
- command: The command to send.
- prompt: After sending the command, `send_command` will be expecting this string.
- If :data:`None`, will use the class's default prompt.
- skip_first_line: Skip the first line when capturing the output.
-
- Returns:
- All output in the buffer before expected string.
-
- Raises:
- InteractiveCommandExecutionError: If attempting to send a command to a shell that is
- not currently running.
- InteractiveSSHSessionDeadError: The session died while executing the command.
- InteractiveSSHTimeoutError: If command was sent but prompt could not be found in
- the output before the timeout.
- """
- if not self.is_alive:
- raise InteractiveCommandExecutionError(
- f"Cannot send command {command} to application because the shell is not running."
- )
- self._logger.info(f"Sending: '{command}'")
- if prompt is None:
- prompt = self._default_prompt
- out: str = ""
- try:
- self._stdin.write(f"{command}{self._command_extra_chars}\n")
- self._stdin.flush()
- for line in self._stdout:
- if skip_first_line:
- skip_first_line = False
- continue
- if line.rstrip().endswith(prompt):
- break
- out += line
- except TimeoutError as e:
- self._logger.exception(e)
- self._logger.debug(
- f"Prompt ({prompt}) was not found in output from command before timeout."
- )
- raise InteractiveSSHTimeoutError(command) from e
- except OSError as e:
- self._logger.exception(e)
- raise InteractiveSSHSessionDeadError(
- self._node.main_session.interactive_session.hostname
- ) from e
- finally:
- self._logger.debug(f"Got output: {out}")
- return out
-
- def _close(self) -> None:
- self._stdin.close()
- self._ssh_channel.close()
-
- def _update_real_path(self, path: PurePath) -> None:
- """Updates the interactive shell's real path used at command line."""
- self._real_path = self._node.main_session.join_remote_path(path)
-
- def __enter__(self) -> Self:
- """Enter the context block.
-
- Upon entering a context block with this class, the desired behavior is to create the
- channel for the application to use, and then start the application.
-
- Returns:
- Reference to the object for the application after it has been started.
- """
- self._start_application()
- return self
-
- def __exit__(self, *_) -> None:
- """Exit the context block.
-
- Upon exiting a context block with this class, we want to ensure that the instance of the
- application is explicitly closed and properly cleaned up using its close method. Note that
- because this method returns :data:`None` if an exception was raised within the block, it is
- not handled and will be re-raised after the application is closed.
-
- The desired behavior is to close the application regardless of the reason for exiting the
- context and then recreate that reason afterwards. All method arguments are ignored for
- this reason.
- """
- self._close()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index d187eaea94..cb8cd6f0ca 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -2077,11 +2077,11 @@ def set_verbose(self, level: int, verify: bool = True) -> None:
f"Testpmd failed to set verbose level to {level}."
)
- def _close(self) -> None:
+ def close(self) -> None:
"""Overrides :meth:`~.interactive_shell.close`."""
self.stop()
self.send_command("quit", "Bye...")
- return super()._close()
+ return super().close()
"""
====== Capability retrieval methods ======
diff --git a/dts/framework/testbed_model/capability.py b/dts/framework/testbed_model/capability.py
index e883f59d11..ad497e8ad9 100644
--- a/dts/framework/testbed_model/capability.py
+++ b/dts/framework/testbed_model/capability.py
@@ -237,7 +237,7 @@ def get_supported_capabilities(
for capability in capabilities:
if capability.nic_capability in supported_capabilities:
supported_conditional_capabilities.add(capability)
- testpmd_shell._close()
+ testpmd_shell.close()
logger.debug(f"Found supported capabilities {supported_conditional_capabilities}.")
return supported_conditional_capabilities
diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
index 4f06968adc..62d0c09e78 100644
--- a/dts/framework/testbed_model/node.py
+++ b/dts/framework/testbed_model/node.py
@@ -37,13 +37,13 @@
from .port import Port
if TYPE_CHECKING:
- from framework.remote_session.single_active_interactive_shell import (
- SingleActiveInteractiveShell,
+ from framework.remote_session.interactive_shell import (
+ InteractiveShell,
)
T = TypeVar("T")
Scope = Literal["unknown", "suite", "case"]
-ScopedShell = tuple[Scope, SingleActiveInteractiveShell]
+ScopedShell = tuple[Scope, InteractiveShell]
class Node(ABC):
@@ -164,7 +164,7 @@ def exit_scope(self) -> Scope:
self.clean_up_shells(current_scope)
return current_scope
- def register_shell(self, shell: SingleActiveInteractiveShell) -> None:
+ def register_shell(self, shell: InteractiveShell) -> None:
"""Register a new shell to the pool of active shells."""
self._active_shells.append((self.current_scope, shell))
@@ -179,7 +179,7 @@ def clean_up_shells(self, scope: Scope) -> None:
]
for i in reversed(zombie_shells_indices):
- self._active_shells[i][1]._close()
+ self._active_shells[i][1].close()
del self._active_shells[i]
def create_session(self, name: str) -> OSSession:
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 0/7] dts: shell improvements
2024-12-20 17:23 [RFC PATCH 0/2] dts: add basic scope to improve shell handling Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 1/2] dts: add scoping and shell registration to Node Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 2/2] dts: revert back shell split Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 1/7] dts: escape single quotes Luca Vizzarro
` (6 more replies)
2 siblings, 7 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Patrick Robb, Paul Szczepanek
Hi there,
sending in a v2, which should also remove the RFC status from v1, as I
believe these changes are now ready as they are.
v2:
- rebased
- added further improvements
- reworked shell registration based on the recent framework
improvements, e.g. context
- removed multi inheritance to simplify
- added a new pseudo-shell to handle blocking applications
Best,
Luca
Luca Vizzarro (7):
dts: escape single quotes
dts: add blocking dpdk app class
dts: add shells pool
dts: revert back to a single InteractiveShell
dts: make shells path dynamic
dts: remove multi-inheritance classes
dts: enable shell pooling
doc/api/dts/framework.remote_session.rst | 1 +
.../framework.remote_session.shell_pool.rst | 8 +
dts/framework/context.py | 2 +
dts/framework/remote_session/dpdk_app.py | 80 +++++
dts/framework/remote_session/dpdk_shell.py | 21 +-
.../remote_session/interactive_shell.py | 302 ++++++++++++++++--
dts/framework/remote_session/python_shell.py | 13 +-
dts/framework/remote_session/shell_pool.py | 106 ++++++
.../single_active_interactive_shell.py | 269 ----------------
dts/framework/remote_session/testpmd_shell.py | 16 +-
dts/framework/test_run.py | 5 +
dts/framework/testbed_model/linux_session.py | 1 +
.../testbed_model/traffic_generator/scapy.py | 38 ++-
.../traffic_generator/traffic_generator.py | 6 +-
dts/framework/utils.py | 14 -
15 files changed, 531 insertions(+), 351 deletions(-)
create mode 100644 doc/api/dts/framework.remote_session.shell_pool.rst
create mode 100644 dts/framework/remote_session/dpdk_app.py
create mode 100644 dts/framework/remote_session/shell_pool.py
delete mode 100644 dts/framework/remote_session/single_active_interactive_shell.py
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/7] dts: escape single quotes
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 2/7] dts: add blocking dpdk app class Luca Vizzarro
` (5 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
When making any command a privileged one in a LinuxSession, there
currently is no consideration whether this command already includes single
quotes. Therefore escape the existing single quotes before making the
command.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/testbed_model/linux_session.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
index 7c2b110c99..6c6a4b608d 100644
--- a/dts/framework/testbed_model/linux_session.py
+++ b/dts/framework/testbed_model/linux_session.py
@@ -67,6 +67,7 @@ class LinuxSession(PosixSession):
@staticmethod
def _get_privileged_command(command: str) -> str:
+ command = command.replace(r"'", r"\'")
return f"sudo -- sh -c '{command}'"
def get_remote_cpus(self) -> list[LogicalCore]:
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/7] dts: add blocking dpdk app class
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 1/7] dts: escape single quotes Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 3/7] dts: add shells pool Luca Vizzarro
` (4 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Add BlockingDPDKApp class. Some non-interactive applications are
blocking and run until the user interrupts them. As their main intended
usage is to be kept running in the background, this class exploits
InteractiveShell to spawn a dedicated shell to keep the blocking
application running, while detaching from it.
This class works by providing the `wait_until_ready` and `close`
methods. The former starts up the application and returns only when the
application readiness output ends in the string provided as an argument
to the same method. Whereas the latter works by simulating a Ctrl+C
keystroke, therefore sending a SIGINT to the app.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/remote_session/dpdk_app.py | 73 +++++++++++++++++++
dts/framework/remote_session/dpdk_shell.py | 3 +-
.../single_active_interactive_shell.py | 12 ++-
dts/framework/remote_session/testpmd_shell.py | 2 +-
4 files changed, 85 insertions(+), 5 deletions(-)
create mode 100644 dts/framework/remote_session/dpdk_app.py
diff --git a/dts/framework/remote_session/dpdk_app.py b/dts/framework/remote_session/dpdk_app.py
new file mode 100644
index 0000000000..c9945f302d
--- /dev/null
+++ b/dts/framework/remote_session/dpdk_app.py
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2025 Arm Limited
+
+"""Class to run blocking DPDK apps in the background.
+
+The class won't automatically start the app. The start-up is done as part of the
+:meth:`BlockingDPDKApp.wait_until_ready` method, which will return execution to the caller only
+when the desired stdout has been returned by the app. Usually this is used to detect when the app
+has been loaded and ready to be used.
+
+Example:
+ ..code:: python
+
+ pdump = BlockingDPDKApp(
+ PurePath("app/dpdk-pdump"),
+ app_params="--pdump 'port=0,queue=*,rx-dev=/tmp/rx-dev.pcap'"
+ )
+ pdump.wait_until_ready("65535") # start app
+
+ # pdump is now ready to capture
+
+ pdump.close() # stop/close app
+"""
+
+from pathlib import PurePath
+
+from framework.params.eal import EalParams
+from framework.remote_session.dpdk_shell import DPDKShell
+
+
+class BlockingDPDKApp(DPDKShell):
+ """Class to manage blocking DPDK apps."""
+
+ def __init__(
+ self,
+ path: PurePath,
+ name: str | None = None,
+ privileged: bool = True,
+ app_params: EalParams | str = "",
+ ) -> None:
+ """Constructor.
+
+ Overrides :meth:`~.dpdk_shell.DPDKShell.__init__`.
+
+ Args:
+ path: Path relative to the DPDK build to the executable.
+ name: Name to identify this application.
+ privileged: Run as privileged user.
+ app_params: The application parameters. If a string or an incomplete :class:`EalParams`
+ object are passed, the EAL params are computed based on the current context.
+ """
+ if isinstance(app_params, str):
+ eal_params = EalParams()
+ eal_params.append_str(app_params)
+ app_params = eal_params
+
+ super().__init__(name, privileged, path, app_params)
+
+ def wait_until_ready(self, end_token: str) -> None:
+ """Start app and wait until ready.
+
+ Args:
+ end_token: The string at the end of a line that indicates the app is ready.
+ """
+ self._start_application(end_token)
+
+ def close(self) -> None:
+ """Close the application.
+
+ Sends a SIGINT to close the application.
+ """
+ self.send_command("\x03")
+ self._close()
diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index 0962414876..f7ea2588ca 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -65,13 +65,14 @@ def __init__(
self,
name: str | None = None,
privileged: bool = True,
+ path: PurePath | None = None,
app_params: EalParams = EalParams(),
) -> None:
"""Extends :meth:`~.interactive_shell.InteractiveShell.__init__`."""
app_params = compute_eal_params(app_params)
node = get_ctx().sut_node
- super().__init__(node, name, privileged, app_params)
+ super().__init__(node, name, privileged, path, app_params)
def _update_real_path(self, path: PurePath) -> None:
"""Extends :meth:`~.interactive_shell.InteractiveShell._update_real_path`.
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
index c1369ef77e..2257b6156b 100644
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ b/dts/framework/remote_session/single_active_interactive_shell.py
@@ -92,6 +92,7 @@ def __init__(
node: Node,
name: str | None = None,
privileged: bool = False,
+ path: PurePath | None = None,
app_params: Params = Params(),
**kwargs,
) -> None:
@@ -105,6 +106,7 @@ def __init__(
name: Name for the interactive shell to use for logging. This name will be appended to
the name of the underlying node which it is running on.
privileged: Enables the shell to run as superuser.
+ path: Path to the executable. If :data:`None`, then the class' path attribute is used.
app_params: The command line parameters to be passed to the application on startup.
**kwargs: Any additional arguments if any.
"""
@@ -116,7 +118,7 @@ def __init__(
self._privileged = privileged
self._timeout = SETTINGS.timeout
# Ensure path is properly formatted for the host
- self._update_real_path(self.path)
+ self._update_real_path(path or self.path)
super().__init__(**kwargs)
def _setup_ssh_channel(self):
@@ -133,7 +135,7 @@ def _make_start_command(self) -> str:
start_command = self._node.main_session._get_privileged_command(start_command)
return start_command
- def _start_application(self) -> None:
+ def _start_application(self, prompt: str | None = None) -> None:
"""Starts a new interactive application based on the path to the app.
This method is often overridden by subclasses as their process for starting may look
@@ -141,6 +143,10 @@ def _start_application(self) -> None:
`self._init_attempts` - 1 times. This is done because some DPDK applications need slightly
more time after exiting their script to clean up EAL before others can start.
+ Args:
+ prompt: When starting up the application, expect this string at the end of stdout when
+ the application is ready. If :data:`None`, the class' default prompt will be used.
+
Raises:
InteractiveCommandExecutionError: If the application fails to start within the allotted
number of retries.
@@ -151,7 +157,7 @@ def _start_application(self) -> None:
self.is_alive = True
for attempt in range(self._init_attempts):
try:
- self.send_command(start_command)
+ self.send_command(start_command, prompt)
break
except InteractiveSSHTimeoutError:
self._logger.info(
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 1f291fcb68..db1bfaa9d1 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1540,7 +1540,7 @@ def __init__(
"""Overrides :meth:`~.dpdk_shell.DPDKShell.__init__`. Changes app_params to kwargs."""
if "port_topology" not in app_params and get_ctx().topology.type is TopologyType.one_link:
app_params["port_topology"] = PortTopology.loop
- super().__init__(name, privileged, TestPmdParams(**app_params))
+ super().__init__(name, privileged, app_params=TestPmdParams(**app_params))
self.ports_started = not self._app_params.disable_device_start
self._ports = None
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/7] dts: add shells pool
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 1/7] dts: escape single quotes Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 2/7] dts: add blocking dpdk app class Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 4/7] dts: revert back to a single InteractiveShell Luca Vizzarro
` (3 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Add a new class ShellPool which acts as a management component for
InteractiveShells. It pools together all the currently active shells,
which are meant to be registered to the pool upon start up. This way,
DTS can control the shells and make sure that these are stopped and
closed correctly when they go out of scope.
The implementation of ShellPool consists of a stack of pools, as each
pool in the stack is meant to own and control shells in the different
layers of execution. For example, if a shell is created in a test case,
which is considered a layer of execution on its own, this can be cleaned
up properly without affecting shells created on a lower level, like the
test suite.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
doc/api/dts/framework.remote_session.rst | 1 +
.../framework.remote_session.shell_pool.rst | 8 ++
dts/framework/context.py | 2 +
dts/framework/remote_session/shell_pool.py | 106 ++++++++++++++++++
4 files changed, 117 insertions(+)
create mode 100644 doc/api/dts/framework.remote_session.shell_pool.rst
create mode 100644 dts/framework/remote_session/shell_pool.py
diff --git a/doc/api/dts/framework.remote_session.rst b/doc/api/dts/framework.remote_session.rst
index 79d65e3444..27c9153e64 100644
--- a/doc/api/dts/framework.remote_session.rst
+++ b/doc/api/dts/framework.remote_session.rst
@@ -15,6 +15,7 @@ remote\_session - Node Connections Package
framework.remote_session.ssh_session
framework.remote_session.interactive_remote_session
framework.remote_session.interactive_shell
+ framework.remote_session.shell_pool
framework.remote_session.dpdk
framework.remote_session.dpdk_shell
framework.remote_session.testpmd_shell
diff --git a/doc/api/dts/framework.remote_session.shell_pool.rst b/doc/api/dts/framework.remote_session.shell_pool.rst
new file mode 100644
index 0000000000..ef506cdd80
--- /dev/null
+++ b/doc/api/dts/framework.remote_session.shell_pool.rst
@@ -0,0 +1,8 @@
+.. SPDX-License-Identifier: BSD-3-Clause
+
+shell\_pool- Shell Pooling Manager
+===========================================
+
+.. automodule:: framework.remote_session.shell_pool
+ :members:
+ :show-inheritance:
diff --git a/dts/framework/context.py b/dts/framework/context.py
index ddd7ed4d36..4360bc8699 100644
--- a/dts/framework/context.py
+++ b/dts/framework/context.py
@@ -8,6 +8,7 @@
from typing import TYPE_CHECKING, ParamSpec
from framework.exception import InternalError
+from framework.remote_session.shell_pool import ShellPool
from framework.settings import SETTINGS
from framework.testbed_model.cpu import LogicalCoreCount, LogicalCoreList
from framework.testbed_model.node import Node
@@ -70,6 +71,7 @@ class Context:
dpdk: "DPDKRuntimeEnvironment"
tg: "TrafficGenerator"
local: LocalContext = field(default_factory=LocalContext)
+ shell_pool: ShellPool = field(default_factory=ShellPool)
__current_ctx: Context | None = None
diff --git a/dts/framework/remote_session/shell_pool.py b/dts/framework/remote_session/shell_pool.py
new file mode 100644
index 0000000000..173aa8fd36
--- /dev/null
+++ b/dts/framework/remote_session/shell_pool.py
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2025 Arm Limited
+
+"""Module defining the shell pool class.
+
+The shell pool is used by the test run state machine to control
+the shells spawned by the test suites. Each layer of execution can
+stash the current pool and create a new layer of shells by calling `start_new_pool`.
+You can go back to the previous pool by calling `terminate_current_pool`. These layers are
+identified in the pool as levels where the higher the number, the deeper the layer of execution.
+As an example layers of execution could be: test run, test suite and test case.
+Which could appropriately identified with level numbers 0, 1 and 2 respectively.
+
+The shell pool layers are implemented as a stack. Therefore, when creating a new pool, this is
+pushed on top of the stack. Similarly, terminating the current pool also means removing the one
+at the top of the stack.
+"""
+
+from typing import TYPE_CHECKING
+
+from framework.logger import DTSLogger, get_dts_logger
+
+if TYPE_CHECKING:
+ from framework.remote_session.interactive_shell import (
+ InteractiveShell,
+ )
+
+
+class ShellPool:
+ """A pool managing active shells."""
+
+ _logger: DTSLogger
+ _pools: list[set["InteractiveShell"]]
+
+ def __init__(self):
+ """Shell pool constructor."""
+ self._logger = get_dts_logger("shell_pool")
+ self._pools = [set()]
+
+ @property
+ def pool_level(self) -> int:
+ """The current level of shell pool.
+
+ The higher level, the deeper we are in the execution state.
+ """
+ return len(self._pools) - 1
+
+ @property
+ def _current_pool(self) -> set["InteractiveShell"]:
+ """The pool in use for the current scope."""
+ return self._pools[-1]
+
+ def register_shell(self, shell: "InteractiveShell"):
+ """Register a new shell to the current pool."""
+ self._logger.debug(f"Registering shell {shell} to pool level {self.pool_level}.")
+ self._current_pool.add(shell)
+
+ def unregister_shell(self, shell: "InteractiveShell"):
+ """Unregister a shell from any pool."""
+ for level, pool in enumerate(self._pools):
+ try:
+ pool.remove(shell)
+ if pool == self._current_pool:
+ self._logger.debug(
+ f"Unregistering shell {shell} from pool level {self.pool_level}."
+ )
+ else:
+ self._logger.debug(
+ f"Unregistering shell {shell} from pool level {level}, "
+ f"but we currently are in level {self.pool_level}. Is this expected?"
+ )
+ except KeyError:
+ pass
+
+ def start_new_pool(self):
+ """Start a new shell pool."""
+ self._logger.debug(f"Starting new shell pool and advancing to level {self.pool_level+1}.")
+ self._pools.append(set())
+
+ def terminate_current_pool(self):
+ """Terminate all the shells in the current pool, and restore the previous pool if any.
+
+ If any failure occurs while closing any shell, this is tolerated allowing the termination
+ to continue until the current pool is empty and removed. But this function will re-raise the
+ last occurred exception back to the caller.
+ """
+ occurred_exception = None
+ current_pool_level = self.pool_level
+ self._logger.debug(f"Terminating shell pool level {current_pool_level}.")
+ for shell in self._pools.pop():
+ self._logger.debug(f"Closing shell {shell} in shell pool level {current_pool_level}.")
+ try:
+ shell._close()
+ except Exception as e:
+ self._logger.error(f"An exception has occurred while closing shell {shell}:")
+ self._logger.exception(e)
+ occurred_exception = e
+
+ if current_pool_level == 0:
+ self.start_new_pool()
+ else:
+ self._logger.debug(f"Restoring shell pool from level {self.pool_level}.")
+
+ # Raise the last occurred exception again to let the system register a failure.
+ if occurred_exception is not None:
+ raise occurred_exception
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/7] dts: revert back to a single InteractiveShell
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
` (2 preceding siblings ...)
2025-03-14 13:18 ` [PATCH v2 3/7] dts: add shells pool Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 5/7] dts: make shells path dynamic Luca Vizzarro
` (2 subsequent siblings)
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Previously InteractiveShell was split into two classes to differentiate
a shell which execution must be controlled in a tight scope through a
context manager, from a more looser approach. With the addition of the
shell pool this is no longer needed as the management of shells is now
delegated to the test run instead of the test suites.
Revert back to a single InteractiveShell to simplify the code. Keep the
context manager implementation but also render start_application and
close public methods again.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/remote_session/dpdk_app.py | 4 +-
dts/framework/remote_session/dpdk_shell.py | 6 +-
.../remote_session/interactive_shell.py | 289 ++++++++++++++++--
dts/framework/remote_session/python_shell.py | 4 +
dts/framework/remote_session/shell_pool.py | 2 +-
.../single_active_interactive_shell.py | 275 -----------------
dts/framework/remote_session/testpmd_shell.py | 4 +-
7 files changed, 275 insertions(+), 309 deletions(-)
delete mode 100644 dts/framework/remote_session/single_active_interactive_shell.py
diff --git a/dts/framework/remote_session/dpdk_app.py b/dts/framework/remote_session/dpdk_app.py
index c9945f302d..c5aae05e02 100644
--- a/dts/framework/remote_session/dpdk_app.py
+++ b/dts/framework/remote_session/dpdk_app.py
@@ -62,7 +62,7 @@ def wait_until_ready(self, end_token: str) -> None:
Args:
end_token: The string at the end of a line that indicates the app is ready.
"""
- self._start_application(end_token)
+ self.start_application(end_token)
def close(self) -> None:
"""Close the application.
@@ -70,4 +70,4 @@ def close(self) -> None:
Sends a SIGINT to close the application.
"""
self.send_command("\x03")
- self._close()
+ super().close()
diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index f7ea2588ca..24a39482ce 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -11,8 +11,8 @@
from framework.context import get_ctx
from framework.params.eal import EalParams
-from framework.remote_session.single_active_interactive_shell import (
- SingleActiveInteractiveShell,
+from framework.remote_session.interactive_shell import (
+ InteractiveShell,
)
from framework.testbed_model.cpu import LogicalCoreList
@@ -51,7 +51,7 @@ def compute_eal_params(
return params
-class DPDKShell(SingleActiveInteractiveShell, ABC):
+class DPDKShell(InteractiveShell, ABC):
"""The base class for managing DPDK-based interactive shells.
This class shouldn't be instantiated directly, but instead be extended.
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 9ca285b604..62f9984d3b 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -1,44 +1,281 @@
# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2023 University of New Hampshire
+# Copyright(c) 2024 University of New Hampshire
# Copyright(c) 2024 Arm Limited
-"""Interactive shell with manual stop/start functionality.
+"""Common functionality for interactive shell handling.
-Provides a class that doesn't require being started/stopped using a context manager and can instead
-be started and stopped manually, or have the stopping process be handled at the time of garbage
-collection.
+The base class, :class:`InteractiveShell`, is meant to be extended by subclasses that
+contain functionality specific to that shell type. These subclasses will often modify things like
+the prompt to expect or the arguments to pass into the application, but still utilize
+the same method for sending a command and collecting output. How this output is handled however
+is often application specific. If an application needs elevated privileges to start it is expected
+that the method for gaining those privileges is provided when initializing the class.
+
+This class is designed for applications like primary applications in DPDK where only one instance
+of the application can be running at a given time and, for this reason, is managed using a context
+manager. This context manager starts the application when you enter the context and cleans up the
+application when you exit. Using a context manager for this is useful since it allows us to ensure
+the application is cleaned up as soon as you leave the block regardless of the reason.
+
+The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
+environment variable configure the timeout of getting the output from command execution.
"""
-import weakref
+from abc import ABC
+from pathlib import PurePath
from typing import ClassVar
-from .single_active_interactive_shell import SingleActiveInteractiveShell
+from paramiko import Channel, channel
+from typing_extensions import Self
+
+from framework.exception import (
+ InteractiveCommandExecutionError,
+ InteractiveSSHSessionDeadError,
+ InteractiveSSHTimeoutError,
+)
+from framework.logger import DTSLogger, get_dts_logger
+from framework.params import Params
+from framework.settings import SETTINGS
+from framework.testbed_model.node import Node
+from framework.utils import MultiInheritanceBaseClass
+
+
+class InteractiveShell(MultiInheritanceBaseClass, ABC):
+ """The base class for managing interactive shells.
+ This class shouldn't be instantiated directly, but instead be extended. It contains
+ methods for starting interactive shells as well as sending commands to these shells
+ and collecting input until reaching a certain prompt. All interactive applications
+ will use the same SSH connection, but each will create their own channel on that
+ session.
-class InteractiveShell(SingleActiveInteractiveShell):
- """Adds manual start and stop functionality to interactive shells.
+ Interactive shells are started and stopped using a context manager. This allows for the start
+ and cleanup of the application to happen at predictable times regardless of exceptions or
+ interrupts.
- Like its super-class, this class should not be instantiated directly and should instead be
- extended. This class also provides an option for automated cleanup of the application using a
- weakref and a finalize class. This finalize class allows for cleanup of the class at the time
- of garbage collection and also ensures that cleanup only happens once. This way if a user
- initiates the closing of the shell manually it is not repeated at the time of garbage
- collection.
+ Attributes:
+ is_alive: :data:`True` if the application has started successfully, :data:`False`
+ otherwise.
"""
- _finalizer: weakref.finalize
- #: One attempt should be enough for shells which don't have to worry about other instances
- #: closing before starting a new one.
- _init_attempts: ClassVar[int] = 1
+ _node: Node
+ _stdin: channel.ChannelStdinFile
+ _stdout: channel.ChannelFile
+ _ssh_channel: Channel
+ _logger: DTSLogger
+ _timeout: float
+ _app_params: Params
+ _privileged: bool
+ _real_path: PurePath
+
+ #: The number of times to try starting the application before considering it a failure.
+ _init_attempts: ClassVar[int] = 5
+
+ #: Prompt to expect at the end of output when sending a command.
+ #: This is often overridden by subclasses.
+ _default_prompt: ClassVar[str] = ""
+
+ #: Extra characters to add to the end of every command
+ #: before sending them. This is often overridden by subclasses and is
+ #: most commonly an additional newline character. This additional newline
+ #: character is used to force the line that is currently awaiting input
+ #: into the stdout buffer so that it can be consumed and checked against
+ #: the expected prompt.
+ _command_extra_chars: ClassVar[str] = ""
+
+ #: Path to the executable to start the interactive application.
+ path: ClassVar[PurePath]
+
+ is_alive: bool = False
+
+ def __init__(
+ self,
+ node: Node,
+ name: str | None = None,
+ privileged: bool = False,
+ path: PurePath | None = None,
+ app_params: Params = Params(),
+ **kwargs,
+ ) -> None:
+ """Create an SSH channel during initialization.
+
+ Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other
+ constructors in the case of multiple inheritance.
+
+ Args:
+ node: The node on which to run start the interactive shell.
+ name: Name for the interactive shell to use for logging. This name will be appended to
+ the name of the underlying node which it is running on.
+ privileged: Enables the shell to run as superuser.
+ path: Path to the executable. If :data:`None`, then the class' path attribute is used.
+ app_params: The command line parameters to be passed to the application on startup.
+ **kwargs: Any additional arguments if any.
+ """
+ self._node = node
+ if name is None:
+ name = type(self).__name__
+ self._logger = get_dts_logger(f"{node.name}.{name}")
+ self._app_params = app_params
+ self._privileged = privileged
+ self._timeout = SETTINGS.timeout
+ # Ensure path is properly formatted for the host
+ self._update_real_path(path or self.path)
+ super().__init__(**kwargs)
+
+ def _setup_ssh_channel(self):
+ self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell()
+ self._stdin = self._ssh_channel.makefile_stdin("w")
+ self._stdout = self._ssh_channel.makefile("r")
+ self._ssh_channel.settimeout(self._timeout)
+ self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams
+
+ def _make_start_command(self) -> str:
+ """Makes the command that starts the interactive shell."""
+ start_command = f"{self._real_path} {self._app_params or ''}"
+ if self._privileged:
+ start_command = self._node.main_session._get_privileged_command(start_command)
+ return start_command
+
+ def start_application(self, prompt: str | None = None) -> None:
+ """Starts a new interactive application based on the path to the app.
+
+ This method is often overridden by subclasses as their process for starting may look
+ different. Initialization of the shell on the host can be retried up to
+ `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly
+ more time after exiting their script to clean up EAL before others can start.
- def start_application(self) -> None:
- """Start the application.
+ Args:
+ prompt: When starting up the application, expect this string at the end of stdout when
+ the application is ready. If :data:`None`, the class' default prompt will be used.
- After the application has started, use :class:`weakref.finalize` to manage cleanup.
+ Raises:
+ InteractiveCommandExecutionError: If the application fails to start within the allotted
+ number of retries.
"""
- self._start_application()
- self._finalizer = weakref.finalize(self, self._close)
+ self._setup_ssh_channel()
+ self._ssh_channel.settimeout(5)
+ start_command = self._make_start_command()
+ self.is_alive = True
+ for attempt in range(self._init_attempts):
+ try:
+ self.send_command(start_command, prompt)
+ break
+ except InteractiveSSHTimeoutError:
+ self._logger.info(
+ f"Interactive shell failed to start (attempt {attempt+1} out of "
+ f"{self._init_attempts})"
+ )
+ else:
+ self._ssh_channel.settimeout(self._timeout)
+ self.is_alive = False # update state on failure to start
+ raise InteractiveCommandExecutionError("Failed to start application.")
+ self._ssh_channel.settimeout(self._timeout)
+
+ def send_command(
+ self, command: str, prompt: str | None = None, skip_first_line: bool = False
+ ) -> str:
+ """Send `command` and get all output before the expected ending string.
+
+ Lines that expect input are not included in the stdout buffer, so they cannot
+ be used for expect.
+
+ Example:
+ If you were prompted to log into something with a username and password,
+ you cannot expect ``username:`` because it won't yet be in the stdout buffer.
+ A workaround for this could be consuming an extra newline character to force
+ the current `prompt` into the stdout buffer.
+
+ Args:
+ command: The command to send.
+ prompt: After sending the command, `send_command` will be expecting this string.
+ If :data:`None`, will use the class's default prompt.
+ skip_first_line: Skip the first line when capturing the output.
+
+ Returns:
+ All output in the buffer before expected string.
+
+ Raises:
+ InteractiveCommandExecutionError: If attempting to send a command to a shell that is
+ not currently running.
+ InteractiveSSHSessionDeadError: The session died while executing the command.
+ InteractiveSSHTimeoutError: If command was sent but prompt could not be found in
+ the output before the timeout.
+ """
+ if not self.is_alive:
+ raise InteractiveCommandExecutionError(
+ f"Cannot send command {command} to application because the shell is not running."
+ )
+ self._logger.info(f"Sending: '{command}'")
+ if prompt is None:
+ prompt = self._default_prompt
+ out: str = ""
+ try:
+ self._stdin.write(f"{command}{self._command_extra_chars}\n")
+ self._stdin.flush()
+ for line in self._stdout:
+ if skip_first_line:
+ skip_first_line = False
+ continue
+ if line.rstrip().endswith(prompt):
+ break
+ out += line
+ except TimeoutError as e:
+ self._logger.exception(e)
+ self._logger.debug(
+ f"Prompt ({prompt}) was not found in output from command before timeout."
+ )
+ raise InteractiveSSHTimeoutError(command) from e
+ except OSError as e:
+ self._logger.exception(e)
+ raise InteractiveSSHSessionDeadError(
+ self._node.main_session.interactive_session.hostname
+ ) from e
+ finally:
+ self._logger.debug(f"Got output: {out}")
+ return out
def close(self) -> None:
- """Free all resources using :class:`weakref.finalize`."""
- self._finalizer()
+ """Close the shell.
+
+ Raises:
+ InteractiveSSHTimeoutError: If the shell failed to exit within the set timeout.
+ """
+ try:
+ # Ensure the primary application has terminated via readiness of 'stdout'.
+ if self._ssh_channel.recv_ready():
+ self._ssh_channel.recv(1) # 'Waits' for a single byte to enter 'stdout' buffer.
+ except TimeoutError as e:
+ self._logger.exception(e)
+ self._logger.debug("Application failed to exit before set timeout.")
+ raise InteractiveSSHTimeoutError("Application 'exit' command") from e
+ self._ssh_channel.close()
+
+ def _update_real_path(self, path: PurePath) -> None:
+ """Updates the interactive shell's real path used at command line."""
+ self._real_path = self._node.main_session.join_remote_path(path)
+
+ def __enter__(self) -> Self:
+ """Enter the context block.
+
+ Upon entering a context block with this class, the desired behavior is to create the
+ channel for the application to use, and then start the application.
+
+ Returns:
+ Reference to the object for the application after it has been started.
+ """
+ self.start_application()
+ return self
+
+ def __exit__(self, *_) -> None:
+ """Exit the context block.
+
+ Upon exiting a context block with this class, we want to ensure that the instance of the
+ application is explicitly closed and properly cleaned up using its close method. Note that
+ because this method returns :data:`None` if an exception was raised within the block, it is
+ not handled and will be re-raised after the application is closed.
+
+ The desired behavior is to close the application regardless of the reason for exiting the
+ context and then recreate that reason afterwards. All method arguments are ignored for
+ this reason.
+ """
+ self.close()
diff --git a/dts/framework/remote_session/python_shell.py b/dts/framework/remote_session/python_shell.py
index 9d4abab12c..17e6c2559d 100644
--- a/dts/framework/remote_session/python_shell.py
+++ b/dts/framework/remote_session/python_shell.py
@@ -29,3 +29,7 @@ class PythonShell(InteractiveShell):
#: The Python executable.
path: ClassVar[PurePath] = PurePath("python3")
+
+ def close(self):
+ """Close Python shell."""
+ return super().close()
diff --git a/dts/framework/remote_session/shell_pool.py b/dts/framework/remote_session/shell_pool.py
index 173aa8fd36..da956950d5 100644
--- a/dts/framework/remote_session/shell_pool.py
+++ b/dts/framework/remote_session/shell_pool.py
@@ -90,7 +90,7 @@ def terminate_current_pool(self):
for shell in self._pools.pop():
self._logger.debug(f"Closing shell {shell} in shell pool level {current_pool_level}.")
try:
- shell._close()
+ shell.close()
except Exception as e:
self._logger.error(f"An exception has occurred while closing shell {shell}:")
self._logger.exception(e)
diff --git a/dts/framework/remote_session/single_active_interactive_shell.py b/dts/framework/remote_session/single_active_interactive_shell.py
deleted file mode 100644
index 2257b6156b..0000000000
--- a/dts/framework/remote_session/single_active_interactive_shell.py
+++ /dev/null
@@ -1,275 +0,0 @@
-# SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2024 University of New Hampshire
-
-"""Common functionality for interactive shell handling.
-
-The base class, :class:`SingleActiveInteractiveShell`, is meant to be extended by subclasses that
-contain functionality specific to that shell type. These subclasses will often modify things like
-the prompt to expect or the arguments to pass into the application, but still utilize
-the same method for sending a command and collecting output. How this output is handled however
-is often application specific. If an application needs elevated privileges to start it is expected
-that the method for gaining those privileges is provided when initializing the class.
-
-This class is designed for applications like primary applications in DPDK where only one instance
-of the application can be running at a given time and, for this reason, is managed using a context
-manager. This context manager starts the application when you enter the context and cleans up the
-application when you exit. Using a context manager for this is useful since it allows us to ensure
-the application is cleaned up as soon as you leave the block regardless of the reason.
-
-The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
-environment variable configure the timeout of getting the output from command execution.
-"""
-
-from abc import ABC
-from pathlib import PurePath
-from typing import ClassVar
-
-from paramiko import Channel, channel
-from typing_extensions import Self
-
-from framework.exception import (
- InteractiveCommandExecutionError,
- InteractiveSSHSessionDeadError,
- InteractiveSSHTimeoutError,
-)
-from framework.logger import DTSLogger, get_dts_logger
-from framework.params import Params
-from framework.settings import SETTINGS
-from framework.testbed_model.node import Node
-from framework.utils import MultiInheritanceBaseClass
-
-
-class SingleActiveInteractiveShell(MultiInheritanceBaseClass, ABC):
- """The base class for managing interactive shells.
-
- This class shouldn't be instantiated directly, but instead be extended. It contains
- methods for starting interactive shells as well as sending commands to these shells
- and collecting input until reaching a certain prompt. All interactive applications
- will use the same SSH connection, but each will create their own channel on that
- session.
-
- Interactive shells are started and stopped using a context manager. This allows for the start
- and cleanup of the application to happen at predictable times regardless of exceptions or
- interrupts.
-
- Attributes:
- is_alive: :data:`True` if the application has started successfully, :data:`False`
- otherwise.
- """
-
- _node: Node
- _stdin: channel.ChannelStdinFile
- _stdout: channel.ChannelFile
- _ssh_channel: Channel
- _logger: DTSLogger
- _timeout: float
- _app_params: Params
- _privileged: bool
- _real_path: PurePath
-
- #: The number of times to try starting the application before considering it a failure.
- _init_attempts: ClassVar[int] = 5
-
- #: Prompt to expect at the end of output when sending a command.
- #: This is often overridden by subclasses.
- _default_prompt: ClassVar[str] = ""
-
- #: Extra characters to add to the end of every command
- #: before sending them. This is often overridden by subclasses and is
- #: most commonly an additional newline character. This additional newline
- #: character is used to force the line that is currently awaiting input
- #: into the stdout buffer so that it can be consumed and checked against
- #: the expected prompt.
- _command_extra_chars: ClassVar[str] = ""
-
- #: Path to the executable to start the interactive application.
- path: ClassVar[PurePath]
-
- is_alive: bool = False
-
- def __init__(
- self,
- node: Node,
- name: str | None = None,
- privileged: bool = False,
- path: PurePath | None = None,
- app_params: Params = Params(),
- **kwargs,
- ) -> None:
- """Create an SSH channel during initialization.
-
- Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other
- constructors in the case of multiple inheritance.
-
- Args:
- node: The node on which to run start the interactive shell.
- name: Name for the interactive shell to use for logging. This name will be appended to
- the name of the underlying node which it is running on.
- privileged: Enables the shell to run as superuser.
- path: Path to the executable. If :data:`None`, then the class' path attribute is used.
- app_params: The command line parameters to be passed to the application on startup.
- **kwargs: Any additional arguments if any.
- """
- self._node = node
- if name is None:
- name = type(self).__name__
- self._logger = get_dts_logger(f"{node.name}.{name}")
- self._app_params = app_params
- self._privileged = privileged
- self._timeout = SETTINGS.timeout
- # Ensure path is properly formatted for the host
- self._update_real_path(path or self.path)
- super().__init__(**kwargs)
-
- def _setup_ssh_channel(self):
- self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell()
- self._stdin = self._ssh_channel.makefile_stdin("w")
- self._stdout = self._ssh_channel.makefile("r")
- self._ssh_channel.settimeout(self._timeout)
- self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams
-
- def _make_start_command(self) -> str:
- """Makes the command that starts the interactive shell."""
- start_command = f"{self._real_path} {self._app_params or ''}"
- if self._privileged:
- start_command = self._node.main_session._get_privileged_command(start_command)
- return start_command
-
- def _start_application(self, prompt: str | None = None) -> None:
- """Starts a new interactive application based on the path to the app.
-
- This method is often overridden by subclasses as their process for starting may look
- different. Initialization of the shell on the host can be retried up to
- `self._init_attempts` - 1 times. This is done because some DPDK applications need slightly
- more time after exiting their script to clean up EAL before others can start.
-
- Args:
- prompt: When starting up the application, expect this string at the end of stdout when
- the application is ready. If :data:`None`, the class' default prompt will be used.
-
- Raises:
- InteractiveCommandExecutionError: If the application fails to start within the allotted
- number of retries.
- """
- self._setup_ssh_channel()
- self._ssh_channel.settimeout(5)
- start_command = self._make_start_command()
- self.is_alive = True
- for attempt in range(self._init_attempts):
- try:
- self.send_command(start_command, prompt)
- break
- except InteractiveSSHTimeoutError:
- self._logger.info(
- f"Interactive shell failed to start (attempt {attempt+1} out of "
- f"{self._init_attempts})"
- )
- else:
- self._ssh_channel.settimeout(self._timeout)
- self.is_alive = False # update state on failure to start
- raise InteractiveCommandExecutionError("Failed to start application.")
- self._ssh_channel.settimeout(self._timeout)
-
- def send_command(
- self, command: str, prompt: str | None = None, skip_first_line: bool = False
- ) -> str:
- """Send `command` and get all output before the expected ending string.
-
- Lines that expect input are not included in the stdout buffer, so they cannot
- be used for expect.
-
- Example:
- If you were prompted to log into something with a username and password,
- you cannot expect ``username:`` because it won't yet be in the stdout buffer.
- A workaround for this could be consuming an extra newline character to force
- the current `prompt` into the stdout buffer.
-
- Args:
- command: The command to send.
- prompt: After sending the command, `send_command` will be expecting this string.
- If :data:`None`, will use the class's default prompt.
- skip_first_line: Skip the first line when capturing the output.
-
- Returns:
- All output in the buffer before expected string.
-
- Raises:
- InteractiveCommandExecutionError: If attempting to send a command to a shell that is
- not currently running.
- InteractiveSSHSessionDeadError: The session died while executing the command.
- InteractiveSSHTimeoutError: If command was sent but prompt could not be found in
- the output before the timeout.
- """
- if not self.is_alive:
- raise InteractiveCommandExecutionError(
- f"Cannot send command {command} to application because the shell is not running."
- )
- self._logger.info(f"Sending: '{command}'")
- if prompt is None:
- prompt = self._default_prompt
- out: str = ""
- try:
- self._stdin.write(f"{command}{self._command_extra_chars}\n")
- self._stdin.flush()
- for line in self._stdout:
- if skip_first_line:
- skip_first_line = False
- continue
- if line.rstrip().endswith(prompt):
- break
- out += line
- except TimeoutError as e:
- self._logger.exception(e)
- self._logger.debug(
- f"Prompt ({prompt}) was not found in output from command before timeout."
- )
- raise InteractiveSSHTimeoutError(command) from e
- except OSError as e:
- self._logger.exception(e)
- raise InteractiveSSHSessionDeadError(
- self._node.main_session.interactive_session.hostname
- ) from e
- finally:
- self._logger.debug(f"Got output: {out}")
- return out
-
- def _close(self) -> None:
- try:
- # Ensure the primary application has terminated via readiness of 'stdout'.
- if self._ssh_channel.recv_ready():
- self._ssh_channel.recv(1) # 'Waits' for a single byte to enter 'stdout' buffer.
- except TimeoutError as e:
- self._logger.exception(e)
- self._logger.debug("Application failed to exit before set timeout.")
- raise InteractiveSSHTimeoutError("Application 'exit' command") from e
- self._ssh_channel.close()
-
- def _update_real_path(self, path: PurePath) -> None:
- """Updates the interactive shell's real path used at command line."""
- self._real_path = self._node.main_session.join_remote_path(path)
-
- def __enter__(self) -> Self:
- """Enter the context block.
-
- Upon entering a context block with this class, the desired behavior is to create the
- channel for the application to use, and then start the application.
-
- Returns:
- Reference to the object for the application after it has been started.
- """
- self._start_application()
- return self
-
- def __exit__(self, *_) -> None:
- """Exit the context block.
-
- Upon exiting a context block with this class, we want to ensure that the instance of the
- application is explicitly closed and properly cleaned up using its close method. Note that
- because this method returns :data:`None` if an exception was raised within the block, it is
- not handled and will be re-raised after the application is closed.
-
- The desired behavior is to close the application regardless of the reason for exiting the
- context and then recreate that reason afterwards. All method arguments are ignored for
- this reason.
- """
- self._close()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index db1bfaa9d1..b83b0c82a0 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -2312,11 +2312,11 @@ def rx_vxlan(self, vxlan_id: int, port_id: int, enable: bool, verify: bool = Tru
self._logger.debug(f"Failed to set VXLAN:\n{vxlan_output}")
raise InteractiveCommandExecutionError(f"Failed to set VXLAN:\n{vxlan_output}")
- def _close(self) -> None:
+ def close(self) -> None:
"""Overrides :meth:`~.interactive_shell.close`."""
self.stop()
self.send_command("quit", "Bye...")
- return super()._close()
+ return super().close()
"""
====== Capability retrieval methods ======
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 5/7] dts: make shells path dynamic
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
` (3 preceding siblings ...)
2025-03-14 13:18 ` [PATCH v2 4/7] dts: revert back to a single InteractiveShell Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 6/7] dts: remove multi-inheritance classes Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 7/7] dts: enable shell pooling Luca Vizzarro
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Turn the `path` attribute of InteractiveShell into a method property.
This allows path to both be defined statically by the class
implementation and also to be defined dynamically as part of the class
construction.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
dts/framework/remote_session/dpdk_app.py | 9 +++++++-
dts/framework/remote_session/dpdk_shell.py | 18 ++++++++-------
.../remote_session/interactive_shell.py | 22 ++++++++-----------
dts/framework/remote_session/python_shell.py | 6 +++--
dts/framework/remote_session/testpmd_shell.py | 8 ++++---
5 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/dts/framework/remote_session/dpdk_app.py b/dts/framework/remote_session/dpdk_app.py
index c5aae05e02..dc4b817bdd 100644
--- a/dts/framework/remote_session/dpdk_app.py
+++ b/dts/framework/remote_session/dpdk_app.py
@@ -54,7 +54,14 @@ def __init__(
eal_params.append_str(app_params)
app_params = eal_params
- super().__init__(name, privileged, path, app_params)
+ self._path = path
+
+ super().__init__(name, privileged, app_params)
+
+ @property
+ def path(self) -> PurePath:
+ """The path of the DPDK app relative to the DPDK build folder."""
+ return self._path
def wait_until_ready(self, end_token: str) -> None:
"""Start app and wait until ready.
diff --git a/dts/framework/remote_session/dpdk_shell.py b/dts/framework/remote_session/dpdk_shell.py
index 24a39482ce..2d4f91052d 100644
--- a/dts/framework/remote_session/dpdk_shell.py
+++ b/dts/framework/remote_session/dpdk_shell.py
@@ -6,7 +6,7 @@
Provides a base class to create interactive shells based on DPDK.
"""
-from abc import ABC
+from abc import ABC, abstractmethod
from pathlib import PurePath
from framework.context import get_ctx
@@ -65,20 +65,22 @@ def __init__(
self,
name: str | None = None,
privileged: bool = True,
- path: PurePath | None = None,
app_params: EalParams = EalParams(),
) -> None:
"""Extends :meth:`~.interactive_shell.InteractiveShell.__init__`."""
app_params = compute_eal_params(app_params)
node = get_ctx().sut_node
- super().__init__(node, name, privileged, path, app_params)
+ super().__init__(node, name, privileged, app_params)
- def _update_real_path(self, path: PurePath) -> None:
- """Extends :meth:`~.interactive_shell.InteractiveShell._update_real_path`.
+ @property
+ @abstractmethod
+ def path(self) -> PurePath:
+ """Relative path to the shell executable from the build folder."""
+
+ def _make_real_path(self):
+ """Overrides :meth:`~.interactive_shell.InteractiveShell._make_real_path`.
Adds the remote DPDK build directory to the path.
"""
- super()._update_real_path(
- PurePath(get_ctx().dpdk_build.remote_dpdk_build_dir).joinpath(path)
- )
+ return get_ctx().dpdk_build.remote_dpdk_build_dir.joinpath(self.path)
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 62f9984d3b..6b7ca0b6af 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -21,7 +21,7 @@
environment variable configure the timeout of getting the output from command execution.
"""
-from abc import ABC
+from abc import ABC, abstractmethod
from pathlib import PurePath
from typing import ClassVar
@@ -66,7 +66,6 @@ class InteractiveShell(MultiInheritanceBaseClass, ABC):
_timeout: float
_app_params: Params
_privileged: bool
- _real_path: PurePath
#: The number of times to try starting the application before considering it a failure.
_init_attempts: ClassVar[int] = 5
@@ -83,9 +82,6 @@ class InteractiveShell(MultiInheritanceBaseClass, ABC):
#: the expected prompt.
_command_extra_chars: ClassVar[str] = ""
- #: Path to the executable to start the interactive application.
- path: ClassVar[PurePath]
-
is_alive: bool = False
def __init__(
@@ -93,7 +89,6 @@ def __init__(
node: Node,
name: str | None = None,
privileged: bool = False,
- path: PurePath | None = None,
app_params: Params = Params(),
**kwargs,
) -> None:
@@ -107,7 +102,6 @@ def __init__(
name: Name for the interactive shell to use for logging. This name will be appended to
the name of the underlying node which it is running on.
privileged: Enables the shell to run as superuser.
- path: Path to the executable. If :data:`None`, then the class' path attribute is used.
app_params: The command line parameters to be passed to the application on startup.
**kwargs: Any additional arguments if any.
"""
@@ -118,8 +112,6 @@ def __init__(
self._app_params = app_params
self._privileged = privileged
self._timeout = SETTINGS.timeout
- # Ensure path is properly formatted for the host
- self._update_real_path(path or self.path)
super().__init__(**kwargs)
def _setup_ssh_channel(self):
@@ -131,7 +123,7 @@ def _setup_ssh_channel(self):
def _make_start_command(self) -> str:
"""Makes the command that starts the interactive shell."""
- start_command = f"{self._real_path} {self._app_params or ''}"
+ start_command = f"{self._make_real_path()} {self._app_params or ''}"
if self._privileged:
start_command = self._node.main_session._get_privileged_command(start_command)
return start_command
@@ -250,9 +242,13 @@ def close(self) -> None:
raise InteractiveSSHTimeoutError("Application 'exit' command") from e
self._ssh_channel.close()
- def _update_real_path(self, path: PurePath) -> None:
- """Updates the interactive shell's real path used at command line."""
- self._real_path = self._node.main_session.join_remote_path(path)
+ @property
+ @abstractmethod
+ def path(self) -> PurePath:
+ """Path to the shell executable."""
+
+ def _make_real_path(self) -> PurePath:
+ return self._node.main_session.join_remote_path(self.path)
def __enter__(self) -> Self:
"""Enter the context block.
diff --git a/dts/framework/remote_session/python_shell.py b/dts/framework/remote_session/python_shell.py
index 17e6c2559d..6331d047c4 100644
--- a/dts/framework/remote_session/python_shell.py
+++ b/dts/framework/remote_session/python_shell.py
@@ -27,8 +27,10 @@ class PythonShell(InteractiveShell):
#: This forces the prompt to appear after sending a command.
_command_extra_chars: ClassVar[str] = "\n"
- #: The Python executable.
- path: ClassVar[PurePath] = PurePath("python3")
+ @property
+ def path(self) -> PurePath:
+ """Path to the Python3 executable."""
+ return PurePath("python3")
def close(self):
"""Close Python shell."""
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index b83b0c82a0..19437b6233 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1520,9 +1520,6 @@ class TestPmdShell(DPDKShell):
_app_params: TestPmdParams
_ports: list[TestPmdPort] | None
- #: The path to the testpmd executable.
- path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")
-
#: The testpmd's prompt.
_default_prompt: ClassVar[str] = "testpmd>"
@@ -1544,6 +1541,11 @@ def __init__(
self.ports_started = not self._app_params.disable_device_start
self._ports = None
+ @property
+ def path(self) -> PurePath:
+ """The path to the testpmd executable."""
+ return PurePath("app/dpdk-testpmd")
+
@property
def ports(self) -> list[TestPmdPort]:
"""The ports of the instance.
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 6/7] dts: remove multi-inheritance classes
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
` (4 preceding siblings ...)
2025-03-14 13:18 ` [PATCH v2 5/7] dts: make shells path dynamic Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 7/7] dts: enable shell pooling Luca Vizzarro
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Multi inheritance has proven to be flaky in Python, therefore revert
back to a simpler approach where classes will only inherit one base
class. As part of this change, instead of making the
ScapyTrafficGenerator class inehrit a PythonShell, use simple class
composition, by making the shell a class attribute.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
.../remote_session/interactive_shell.py | 9 +----
.../testbed_model/traffic_generator/scapy.py | 38 +++++++++----------
.../traffic_generator/traffic_generator.py | 6 +--
dts/framework/utils.py | 14 -------
4 files changed, 22 insertions(+), 45 deletions(-)
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 6b7ca0b6af..d7e566e5c4 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -37,10 +37,9 @@
from framework.params import Params
from framework.settings import SETTINGS
from framework.testbed_model.node import Node
-from framework.utils import MultiInheritanceBaseClass
-class InteractiveShell(MultiInheritanceBaseClass, ABC):
+class InteractiveShell(ABC):
"""The base class for managing interactive shells.
This class shouldn't be instantiated directly, but instead be extended. It contains
@@ -90,20 +89,15 @@ def __init__(
name: str | None = None,
privileged: bool = False,
app_params: Params = Params(),
- **kwargs,
) -> None:
"""Create an SSH channel during initialization.
- Additional keyword arguments can be passed through `kwargs` if needed for fulfilling other
- constructors in the case of multiple inheritance.
-
Args:
node: The node on which to run start the interactive shell.
name: Name for the interactive shell to use for logging. This name will be appended to
the name of the underlying node which it is running on.
privileged: Enables the shell to run as superuser.
app_params: The command line parameters to be passed to the application on startup.
- **kwargs: Any additional arguments if any.
"""
self._node = node
if name is None:
@@ -112,7 +106,6 @@ def __init__(
self._app_params = app_params
self._privileged = privileged
self._timeout = SETTINGS.timeout
- super().__init__(**kwargs)
def _setup_ssh_channel(self):
self._ssh_channel = self._node.main_session.interactive_session.session.invoke_shell()
diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py
index 520561b2eb..78a6ded74c 100644
--- a/dts/framework/testbed_model/traffic_generator/scapy.py
+++ b/dts/framework/testbed_model/traffic_generator/scapy.py
@@ -34,7 +34,7 @@
from .capturing_traffic_generator import CapturingTrafficGenerator
-class ScapyTrafficGenerator(PythonShell, CapturingTrafficGenerator):
+class ScapyTrafficGenerator(CapturingTrafficGenerator):
"""Provides access to scapy functions on a traffic generator node.
This class extends the base with remote execution of scapy functions. All methods for
@@ -56,6 +56,8 @@ class also extends :class:`.capturing_traffic_generator.CapturingTrafficGenerato
first.
"""
+ _shell: PythonShell
+
_config: ScapyTrafficGeneratorConfig
#: Name of sniffer to ensure the same is used in all places
@@ -82,25 +84,21 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig, **kwargs)
tg_node.config.os == OS.linux
), "Linux is the only supported OS for scapy traffic generation"
- super().__init__(node=tg_node, config=config, tg_node=tg_node, **kwargs)
- self.start_application()
+ super().__init__(tg_node=tg_node, config=config, **kwargs)
+ self._shell = PythonShell(tg_node, "scapy", privileged=True)
def setup(self, ports: Iterable[Port]):
"""Extends :meth:`.traffic_generator.TrafficGenerator.setup`.
- Brings up the port links.
+ Starts up the traffic generator and brings up the port links.
"""
- super().setup(ports)
self._tg_node.main_session.bring_up_link(ports)
+ self._shell.start_application()
+ self._shell.send_command("from scapy.all import *")
- def start_application(self) -> None:
- """Extends :meth:`framework.remote_session.interactive_shell.start_application`.
-
- Adds a command that imports everything from the scapy library immediately after starting
- the shell for usage in later calls to the methods of this class.
- """
- super().start_application()
- self.send_command("from scapy.all import *")
+ def close(self):
+ """Close traffic generator."""
+ self._shell.close()
def _send_packets(self, packets: list[Packet], port: Port) -> None:
"""Implementation for sending packets without capturing any received traffic.
@@ -116,7 +114,7 @@ def _send_packets(self, packets: list[Packet], port: Port) -> None:
"verbose=True",
")",
]
- self.send_command(f"\n{self._python_indentation}".join(send_command))
+ self._shell.send_command(f"\n{self._python_indentation}".join(send_command))
def _send_packets_and_capture(
self,
@@ -155,7 +153,7 @@ def _shell_set_packet_list(self, packets: list[Packet]) -> None:
packets: The list of packets to recreate in the shell.
"""
self._logger.info("Building a list of packets to send.")
- self.send_command(
+ self._shell.send_command(
f"{self._send_packet_list_name} = [{', '.join(map(Packet.command, packets))}]"
)
@@ -202,7 +200,7 @@ def _shell_create_sniffer(
"""
self._shell_set_packet_list(packets_to_send)
- self.send_command("import time")
+ self._shell.send_command("import time")
sniffer_commands = [
f"{self._sniffer_name} = AsyncSniffer(",
f"iface='{recv_port.logical_name}',",
@@ -220,7 +218,7 @@ def _shell_create_sniffer(
if filter_config:
sniffer_commands.insert(-1, f"filter='{filter_config}'")
- self.send_command(f"\n{self._python_indentation}".join(sniffer_commands))
+ self._shell.send_command(f"\n{self._python_indentation}".join(sniffer_commands))
def _shell_start_and_stop_sniffing(self, duration: float) -> list[Packet]:
"""Start asynchronous sniffer, run for a set `duration`, then collect received packets.
@@ -237,12 +235,12 @@ def _shell_start_and_stop_sniffing(self, duration: float) -> list[Packet]:
A list of all packets that were received while the sniffer was running.
"""
sniffed_packets_name = "gathered_packets"
- self.send_command(f"{self._sniffer_name}.start()")
+ self._shell.send_command(f"{self._sniffer_name}.start()")
# Insert a one second delay to prevent timeout errors from occurring
time.sleep(duration + 1)
- self.send_command(f"{sniffed_packets_name} = {self._sniffer_name}.stop(join=True)")
+ self._shell.send_command(f"{sniffed_packets_name} = {self._sniffer_name}.stop(join=True)")
# An extra newline is required here due to the nature of interactive Python shells
- packet_strs = self.send_command(
+ packet_strs = self._shell.send_command(
f"for pakt in {sniffed_packets_name}: print(bytes_base64(pakt.build()))\n"
)
# In the string of bytes "b'XXXX'", we only want the contents ("XXXX")
diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
index 804662e114..fbd9771eba 100644
--- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
+++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
@@ -17,10 +17,10 @@
from framework.logger import DTSLogger, get_dts_logger
from framework.testbed_model.node import Node
from framework.testbed_model.port import Port
-from framework.utils import MultiInheritanceBaseClass, get_packet_summaries
+from framework.utils import get_packet_summaries
-class TrafficGenerator(MultiInheritanceBaseClass, ABC):
+class TrafficGenerator(ABC):
"""The base traffic generator.
Exposes the common public methods of all traffic generators and defines private methods
@@ -48,13 +48,13 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig, **kwargs):
self._config = config
self._tg_node = tg_node
self._logger = get_dts_logger(f"{self._tg_node.name} {self._config.type}")
- super().__init__(**kwargs)
def setup(self, ports: Iterable[Port]):
"""Setup the traffic generator."""
def teardown(self, ports: Iterable[Port]):
"""Teardown the traffic generator."""
+ self.close()
def send_packet(self, packet: Packet, port: Port) -> None:
"""Send `packet` and block until it is fully sent.
diff --git a/dts/framework/utils.py b/dts/framework/utils.py
index d6f4c11d58..24277633c0 100644
--- a/dts/framework/utils.py
+++ b/dts/framework/utils.py
@@ -299,20 +299,6 @@ def _make_packet() -> Packet:
return [_make_packet() for _ in range(number_of)]
-class MultiInheritanceBaseClass:
- """A base class for classes utilizing multiple inheritance.
-
- This class enables it's subclasses to support both single and multiple inheritance by acting as
- a stopping point in the tree of calls to the constructors of superclasses. This class is able
- to exist at the end of the Method Resolution Order (MRO) so that subclasses can call
- :meth:`super.__init__` without repercussion.
- """
-
- def __init__(self) -> None:
- """Call the init method of :class:`object`."""
- super().__init__()
-
-
def to_pascal_case(text: str) -> str:
"""Convert `text` from snake_case to PascalCase."""
return "".join([seg.capitalize() for seg in text.split("_")])
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 7/7] dts: enable shell pooling
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
` (5 preceding siblings ...)
2025-03-14 13:18 ` [PATCH v2 6/7] dts: remove multi-inheritance classes Luca Vizzarro
@ 2025-03-14 13:18 ` Luca Vizzarro
6 siblings, 0 replies; 11+ messages in thread
From: Luca Vizzarro @ 2025-03-14 13:18 UTC (permalink / raw)
To: dev; +Cc: Luca Vizzarro, Paul Szczepanek, Patrick Robb
Enable the ShellPool class to be used in the test run, and let
InteractiveShells register themselves to the pool upon shell startup.
Moreover, to avoid the ShellPool to call InteractiveShell.close more
than once if the shell was not unregistered correctly, add a way to
prevent the method to be called if the shell has already been closed.
Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
.../remote_session/interactive_shell.py | 24 ++++++++++++++++++-
dts/framework/remote_session/python_shell.py | 3 ++-
dts/framework/remote_session/testpmd_shell.py | 2 ++
dts/framework/test_run.py | 5 ++++
4 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index d7e566e5c4..ba8489eafa 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -22,12 +22,14 @@
"""
from abc import ABC, abstractmethod
+from collections.abc import Callable
from pathlib import PurePath
-from typing import ClassVar
+from typing import ClassVar, Concatenate, ParamSpec, TypeAlias, TypeVar
from paramiko import Channel, channel
from typing_extensions import Self
+from framework.context import get_ctx
from framework.exception import (
InteractiveCommandExecutionError,
InteractiveSSHSessionDeadError,
@@ -38,6 +40,23 @@
from framework.settings import SETTINGS
from framework.testbed_model.node import Node
+P = ParamSpec("P")
+T = TypeVar("T", bound="InteractiveShell")
+R = TypeVar("R")
+InteractiveShellMethod = Callable[Concatenate[T, P], R]
+InteractiveShellDecorator: TypeAlias = Callable[[InteractiveShellMethod], InteractiveShellMethod]
+
+
+def only_active(func: InteractiveShellMethod) -> InteractiveShellMethod:
+ """This decorator will skip running the method if the SSH channel is not active."""
+
+ def _wrapper(self: "InteractiveShell", *args: P.args, **kwargs: P.kwargs) -> R | None:
+ if self._ssh_channel.active:
+ return func(self, *args, **kwargs)
+ return None
+
+ return _wrapper
+
class InteractiveShell(ABC):
"""The base class for managing interactive shells.
@@ -155,6 +174,7 @@ def start_application(self, prompt: str | None = None) -> None:
self.is_alive = False # update state on failure to start
raise InteractiveCommandExecutionError("Failed to start application.")
self._ssh_channel.settimeout(self._timeout)
+ get_ctx().shell_pool.register_shell(self)
def send_command(
self, command: str, prompt: str | None = None, skip_first_line: bool = False
@@ -219,6 +239,7 @@ def send_command(
self._logger.debug(f"Got output: {out}")
return out
+ @only_active
def close(self) -> None:
"""Close the shell.
@@ -234,6 +255,7 @@ def close(self) -> None:
self._logger.debug("Application failed to exit before set timeout.")
raise InteractiveSSHTimeoutError("Application 'exit' command") from e
self._ssh_channel.close()
+ get_ctx().shell_pool.unregister_shell(self)
@property
@abstractmethod
diff --git a/dts/framework/remote_session/python_shell.py b/dts/framework/remote_session/python_shell.py
index 6331d047c4..5b380a5c7a 100644
--- a/dts/framework/remote_session/python_shell.py
+++ b/dts/framework/remote_session/python_shell.py
@@ -15,7 +15,7 @@
from pathlib import PurePath
from typing import ClassVar
-from .interactive_shell import InteractiveShell
+from .interactive_shell import InteractiveShell, only_active
class PythonShell(InteractiveShell):
@@ -32,6 +32,7 @@ def path(self) -> PurePath:
"""Path to the Python3 executable."""
return PurePath("python3")
+ @only_active
def close(self):
"""Close Python shell."""
return super().close()
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 19437b6233..b1939e4a51 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -33,6 +33,7 @@
)
from framework.context import get_ctx
+from framework.remote_session.interactive_shell import only_active
from framework.testbed_model.topology import TopologyType
if TYPE_CHECKING or environ.get("DTS_DOC_BUILD"):
@@ -2314,6 +2315,7 @@ def rx_vxlan(self, vxlan_id: int, port_id: int, enable: bool, verify: bool = Tru
self._logger.debug(f"Failed to set VXLAN:\n{vxlan_output}")
raise InteractiveCommandExecutionError(f"Failed to set VXLAN:\n{vxlan_output}")
+ @only_active
def close(self) -> None:
"""Overrides :meth:`~.interactive_shell.close`."""
self.stop()
diff --git a/dts/framework/test_run.py b/dts/framework/test_run.py
index f9cfe5e908..7708fe31bd 100644
--- a/dts/framework/test_run.py
+++ b/dts/framework/test_run.py
@@ -430,6 +430,7 @@ def description(self) -> str:
def next(self) -> State | None:
"""Next state."""
+ self.test_run.ctx.shell_pool.terminate_current_pool()
self.test_run.ctx.tg.teardown(self.test_run.ctx.topology.tg_ports)
self.test_run.ctx.dpdk.teardown(self.test_run.ctx.topology.sut_ports)
self.test_run.ctx.tg_node.teardown()
@@ -473,6 +474,7 @@ def description(self) -> str:
def next(self) -> State | None:
"""Next state."""
+ self.test_run.ctx.shell_pool.start_new_pool()
self.test_suite.set_up_suite()
self.result.update_setup(Result.PASS)
return TestSuiteExecution(self.test_run, self.test_suite, self.result)
@@ -544,6 +546,7 @@ def next(self) -> State | None:
"""Next state."""
self.test_suite.tear_down_suite()
self.test_run.ctx.dpdk.kill_cleanup_dpdk_apps()
+ self.test_run.ctx.shell_pool.terminate_current_pool()
self.result.update_teardown(Result.PASS)
return TestRunExecution(self.test_run, self.test_run.result)
@@ -594,6 +597,7 @@ def description(self) -> str:
def next(self) -> State | None:
"""Next state."""
+ self.test_run.ctx.shell_pool.start_new_pool()
self.test_suite.set_up_test_case()
self.result.update_setup(Result.PASS)
return TestCaseExecution(
@@ -670,6 +674,7 @@ def description(self) -> str:
def next(self) -> State | None:
"""Next state."""
self.test_suite.tear_down_test_case()
+ self.test_run.ctx.shell_pool.terminate_current_pool()
self.result.update_teardown(Result.PASS)
return TestSuiteExecution(self.test_run, self.test_suite, self.test_suite_result)
--
2.43.0
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-03-14 13:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-20 17:23 [RFC PATCH 0/2] dts: add basic scope to improve shell handling Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 1/2] dts: add scoping and shell registration to Node Luca Vizzarro
2024-12-20 17:24 ` [RFC PATCH 2/2] dts: revert back shell split Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 0/7] dts: shell improvements Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 1/7] dts: escape single quotes Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 2/7] dts: add blocking dpdk app class Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 3/7] dts: add shells pool Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 4/7] dts: revert back to a single InteractiveShell Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 5/7] dts: make shells path dynamic Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 6/7] dts: remove multi-inheritance classes Luca Vizzarro
2025-03-14 13:18 ` [PATCH v2 7/7] dts: enable shell pooling Luca Vizzarro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).