* [RFC v2 0/2] add DTS smoke tests @ 2023-05-12 19:25 jspewock 2023-05-12 19:25 ` [RFC v2 1/2] dts: add " jspewock 2023-05-12 19:25 ` [RFC v2 2/2] dts: added paramiko to dependencies jspewock 0 siblings, 2 replies; 12+ messages in thread From: jspewock @ 2023-05-12 19:25 UTC (permalink / raw) To: dev; +Cc: Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> This update to the RFC adds a few more things such as physical devices and virtual devices to the config, driver checks in the smoke tests, and interactive shell handling. Interactive shells are handled by creating two SSH connections upon connecting to the SUT, one which is for normal non-interactive opertaions and another (using paramiko) that is reserved for interactive shells. This interactive session is stored in the main session of the SUT and can be piloted using the interactive handler. This handler creates a new channel off the session and provides methods for sending commands both blindly and by expecting output. Then, classes are made for individual DPDK applications that use one of these interactive handlers and know how to handle that applications specific behaviors and outputs. Old RFC: * http://patches.dpdk.org/project/dpdk/patch/20230413175415.7683-3-jspewock@iol.unh.edu/ Jeremy Spewock (1): dts: added paramiko to dependencies Jeremy Spewock (1): dts: add smoke tests dts/conf.yaml | 9 + dts/framework/config/__init__.py | 21 +++ dts/framework/config/conf_yaml_schema.json | 32 +++- dts/framework/dts.py | 19 ++- dts/framework/exception.py | 11 ++ dts/framework/remote_session/os_session.py | 6 +- .../remote_session/remote/__init__.py | 28 +++ dts/framework/test_result.py | 13 +- dts/framework/test_suite.py | 24 ++- dts/framework/testbed_model/__init__.py | 5 + .../interactive_apps/__init__.py | 6 + .../interactive_apps/interactive_command.py | 57 +++++++ .../interactive_apps/testpmd_driver.py | 24 +++ dts/framework/testbed_model/node.py | 2 + dts/framework/testbed_model/sut_node.py | 6 + dts/poetry.lock | 160 ++++++++++++++---- dts/pyproject.toml | 1 + dts/tests/TestSuite_smoke_tests.py | 94 ++++++++++ 18 files changed, 472 insertions(+), 46 deletions(-) create mode 100644 dts/framework/testbed_model/interactive_apps/__init__.py create mode 100644 dts/framework/testbed_model/interactive_apps/interactive_command.py create mode 100644 dts/framework/testbed_model/interactive_apps/testpmd_driver.py create mode 100644 dts/tests/TestSuite_smoke_tests.py -- 2.40.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC v2 1/2] dts: add smoke tests 2023-05-12 19:25 [RFC v2 0/2] add DTS smoke tests jspewock @ 2023-05-12 19:25 ` jspewock 2023-05-23 8:05 ` Juraj Linkeš 2023-05-24 10:03 ` Juraj Linkeš 2023-05-12 19:25 ` [RFC v2 2/2] dts: added paramiko to dependencies jspewock 1 sibling, 2 replies; 12+ messages in thread From: jspewock @ 2023-05-12 19:25 UTC (permalink / raw) To: dev; +Cc: Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> Adds a new test suite for running smoke tests that verify general configuration aspects of the system under test. If any of these tests fail, the DTS execution terminates as part of a "fail-fast" model. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/conf.yaml | 9 ++ dts/framework/config/__init__.py | 21 +++++ dts/framework/config/conf_yaml_schema.json | 32 ++++++- dts/framework/dts.py | 19 +++- dts/framework/exception.py | 11 +++ dts/framework/remote_session/os_session.py | 6 +- .../remote_session/remote/__init__.py | 28 ++++++ dts/framework/test_result.py | 13 ++- dts/framework/test_suite.py | 24 ++++- dts/framework/testbed_model/__init__.py | 5 + .../interactive_apps/__init__.py | 6 ++ .../interactive_apps/interactive_command.py | 57 +++++++++++ .../interactive_apps/testpmd_driver.py | 24 +++++ dts/framework/testbed_model/node.py | 2 + dts/framework/testbed_model/sut_node.py | 6 ++ dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++++++ 16 files changed, 348 insertions(+), 9 deletions(-) create mode 100644 dts/framework/testbed_model/interactive_apps/__init__.py create mode 100644 dts/framework/testbed_model/interactive_apps/interactive_command.py create mode 100644 dts/framework/testbed_model/interactive_apps/testpmd_driver.py create mode 100644 dts/tests/TestSuite_smoke_tests.py diff --git a/dts/conf.yaml b/dts/conf.yaml index a9bd8a3e..042ef954 100644 --- a/dts/conf.yaml +++ b/dts/conf.yaml @@ -10,13 +10,22 @@ executions: compiler_wrapper: ccache perf: false func: true + nics: #physical devices to be used for testing + - addresses: + - "0000:11:00.0" + - "0000:11:00.1" + driver: "i40e" + vdevs: #names of virtual devices to be used for testing + - "crypto_openssl" test_suites: + - smoke_tests - hello_world system_under_test: "SUT 1" nodes: - name: "SUT 1" hostname: sut1.change.me.localhost user: root + password: "" arch: x86_64 os: linux lcores: "" diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py index ebb0823f..f3b8b6e3 100644 --- a/dts/framework/config/__init__.py +++ b/dts/framework/config/__init__.py @@ -106,6 +106,21 @@ def from_dict(d: dict) -> "NodeConfiguration": hugepages=hugepage_config, ) +@dataclass(slots=True, frozen=True) +class NICConfiguration: + addresses: list[str] + driver: str + + @staticmethod + def from_dict(d:dict) -> "NICConfiguration": + return NICConfiguration( + addresses=[addr for addr in d.get("addresses", [])], + driver=d.get("driver") + ) + @staticmethod + def from_list(l:list[dict]) -> list["NICConfiguration"]: + return [] + [NICConfiguration.from_dict(x) for x in l] + @dataclass(slots=True, frozen=True) class BuildTargetConfiguration: @@ -157,6 +172,8 @@ class ExecutionConfiguration: func: bool test_suites: list[TestSuiteConfig] system_under_test: NodeConfiguration + nics: list[NICConfiguration] + vdevs: list[str] @staticmethod def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration": @@ -166,7 +183,9 @@ def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration": test_suites: list[TestSuiteConfig] = list( map(TestSuiteConfig.from_dict, d["test_suites"]) ) + nic_conf: NICConfiguration = NICConfiguration.from_list(d['nics']) sut_name = d["system_under_test"] + list_of_vdevs = d["vdevs"] assert sut_name in node_map, f"Unknown SUT {sut_name} in execution {d}" return ExecutionConfiguration( @@ -174,7 +193,9 @@ def from_dict(d: dict, node_map: dict) -> "ExecutionConfiguration": perf=d["perf"], func=d["func"], test_suites=test_suites, + nics=nic_conf, system_under_test=node_map[sut_name], + vdevs=list_of_vdevs ) diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json index ca2d4a1e..603859de 100644 --- a/dts/framework/config/conf_yaml_schema.json +++ b/dts/framework/config/conf_yaml_schema.json @@ -40,6 +40,18 @@ "mscv" ] }, + "single_nic" : { + "type":"object", + "description": "an object that holds nic information", + "properties": { + "addresses": { + "type":"array", + "items": { + "type":"string" + } + } + } + }, "build_target": { "type": "object", "description": "Targets supported by DTS", @@ -97,7 +109,8 @@ "test_suite": { "type": "string", "enum": [ - "hello_world" + "hello_world", + "smoke_tests" ] }, "test_target": { @@ -211,6 +224,23 @@ ] } }, + "nics": { + "type": "array", + "items": { + "oneOf": [ + { + "$ref": "#/definitions/single_nic" + } + ] + } + }, + "vdevs": { + "description": "Names of vdevs to be used in execution", + "type": "array", + "items": { + "type":"string" + } + }, "system_under_test": { "$ref": "#/definitions/node_name" } diff --git a/dts/framework/dts.py b/dts/framework/dts.py index 05022845..0d03e158 100644 --- a/dts/framework/dts.py +++ b/dts/framework/dts.py @@ -5,6 +5,8 @@ import sys +from .exception import BlockingTestSuiteError + from .config import CONFIGURATION, BuildTargetConfiguration, ExecutionConfiguration from .logger import DTSLOG, getLogger from .test_result import BuildTargetResult, DTSResult, ExecutionResult, Result @@ -49,6 +51,7 @@ def run_all() -> None: nodes[sut_node.name] = sut_node if sut_node: + #SMOKE TEST EXECUTION GOES HERE! _run_execution(sut_node, execution, result) except Exception as e: @@ -118,7 +121,7 @@ def _run_build_target( try: sut_node.set_up_build_target(build_target) - result.dpdk_version = sut_node.dpdk_version + # result.dpdk_version = sut_node.dpdk_version build_target_result.update_setup(Result.PASS) except Exception as e: dts_logger.exception("Build target setup failed.") @@ -146,6 +149,7 @@ def _run_suites( with possibly only a subset of test cases. If no subset is specified, run all test cases. """ + end_execution = False for test_suite_config in execution.test_suites: try: full_suite_path = f"tests.TestSuite_{test_suite_config.test_suite}" @@ -160,13 +164,24 @@ def _run_suites( else: for test_suite_class in test_suite_classes: + #HERE NEEDS CHANGING test_suite = test_suite_class( sut_node, test_suite_config.test_cases, execution.func, build_target_result, + sut_node._build_target_config, + result ) - test_suite.run() + try: + test_suite.run() + except BlockingTestSuiteError as e: + dts_logger.exception("An error occurred within a blocking TestSuite, execution will now end.") + result.add_error(e) + end_execution = True + #if a blocking test failed and we need to bail out of suite executions + if end_execution: + break def _exit_dts() -> None: diff --git a/dts/framework/exception.py b/dts/framework/exception.py index ca353d98..4e3f63d1 100644 --- a/dts/framework/exception.py +++ b/dts/framework/exception.py @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): SSH_ERR = 4 DPDK_BUILD_ERR = 10 TESTCASE_VERIFY_ERR = 20 + BLOCKING_TESTSUITE_ERR = 25 class DTSError(Exception): @@ -144,3 +145,13 @@ def __init__(self, value: str): def __str__(self) -> str: return repr(self.value) + +class BlockingTestSuiteError(DTSError): + suite_name: str + severity: ClassVar[ErrorSeverity] = ErrorSeverity.BLOCKING_TESTSUITE_ERR + + def __init__(self, suite_name:str) -> None: + self.suite_name = suite_name + + def __str__(self) -> str: + return f"Blocking suite {self.suite_name} failed." diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py index 4c48ae25..22776bc1 100644 --- a/dts/framework/remote_session/os_session.py +++ b/dts/framework/remote_session/os_session.py @@ -12,7 +12,9 @@ from framework.testbed_model import LogicalCore from framework.utils import EnvVarsDict, MesonArgs -from .remote import CommandResult, RemoteSession, create_remote_session +from .remote import CommandResult, RemoteSession, create_remote_session, create_interactive_session + +from paramiko import SSHClient class OSSession(ABC): @@ -26,6 +28,7 @@ class OSSession(ABC): name: str _logger: DTSLOG remote_session: RemoteSession + _interactive_session: SSHClient def __init__( self, @@ -37,6 +40,7 @@ def __init__( self.name = name self._logger = logger self.remote_session = create_remote_session(node_config, name, logger) + self._interactive_session = create_interactive_session(node_config, name, logger) def close(self, force: bool = False) -> None: """ diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py index 8a151221..abca8edc 100644 --- a/dts/framework/remote_session/remote/__init__.py +++ b/dts/framework/remote_session/remote/__init__.py @@ -9,8 +9,36 @@ from .remote_session import CommandResult, RemoteSession from .ssh_session import SSHSession +from paramiko import SSHClient, AutoAddPolicy +from framework.utils import GREEN def create_remote_session( node_config: NodeConfiguration, name: str, logger: DTSLOG ) -> RemoteSession: return SSHSession(node_config, name, logger) + +def create_interactive_session( + node_config: NodeConfiguration, name: str, logger: DTSLOG +) -> SSHClient: + """ + Creates a paramiko SSH session that is designed to be used for interactive shells + + This session is meant to be used on an "as needed" basis and may never be utilized + """ + client: SSHClient = SSHClient() + client.set_missing_host_key_policy(AutoAddPolicy) + ip: str = node_config.hostname + logger.info(GREEN(f"Connecting to host {ip}")) + #Preset to 22 because paramiko doesn't accept None + port: int = 22 + if ":" in node_config.hostname: + ip, port = node_config.hostname.split(":") + port = int(port) + client.connect( + ip, + username=node_config.user, + port=port, + password=node_config.password or "", + timeout=20 if port else 10 + ) + return client diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py index 74391982..77202ae2 100644 --- a/dts/framework/test_result.py +++ b/dts/framework/test_result.py @@ -8,6 +8,7 @@ import os.path from collections.abc import MutableSequence from enum import Enum, auto +from typing import Dict from .config import ( OS, @@ -67,12 +68,13 @@ class Statistics(dict): Using a dict provides a convenient way to format the data. """ - def __init__(self, dpdk_version): + def __init__(self, output_info: Dict[str, str] | None): super(Statistics, self).__init__() for result in Result: self[result.name] = 0 self["PASS RATE"] = 0.0 - self["DPDK VERSION"] = dpdk_version + if output_info: + for info_key, info_val in output_info.items(): self[info_key] = info_val def __iadd__(self, other: Result) -> "Statistics": """ @@ -258,6 +260,7 @@ class DTSResult(BaseResult): """ dpdk_version: str | None + output: dict | None _logger: DTSLOG _errors: list[Exception] _return_code: ErrorSeverity @@ -267,6 +270,7 @@ class DTSResult(BaseResult): def __init__(self, logger: DTSLOG): super(DTSResult, self).__init__() self.dpdk_version = None + self.output = None self._logger = logger self._errors = [] self._return_code = ErrorSeverity.NO_ERR @@ -296,7 +300,10 @@ def process(self) -> None: for error in self._errors: self._logger.debug(repr(error)) - self._stats_result = Statistics(self.dpdk_version) + self._stats_result = Statistics(self.output) + #add information gathered from the smoke tests to the statistics + # for info_key, info_val in smoke_test_info.items(): self._stats_result[info_key] = info_val + # print(self._stats_result) self.add_stats(self._stats_result) with open(self._stats_filename, "w+") as stats_file: stats_file.write(str(self._stats_result)) diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py index 0705f38f..1518fb8a 100644 --- a/dts/framework/test_suite.py +++ b/dts/framework/test_suite.py @@ -10,11 +10,14 @@ import inspect import re from types import MethodType +from typing import Dict -from .exception import ConfigurationError, SSHTimeoutError, TestCaseVerifyError +from .config import BuildTargetConfiguration + +from .exception import BlockingTestSuiteError, ConfigurationError, SSHTimeoutError, TestCaseVerifyError from .logger import DTSLOG, getLogger from .settings import SETTINGS -from .test_result import BuildTargetResult, Result, TestCaseResult, TestSuiteResult +from .test_result import BuildTargetResult, DTSResult, Result, TestCaseResult, TestSuiteResult from .testbed_model import SutNode @@ -37,10 +40,12 @@ class TestSuite(object): """ sut_node: SutNode + is_blocking = False _logger: DTSLOG _test_cases_to_run: list[str] _func: bool _result: TestSuiteResult + _dts_result: DTSResult def __init__( self, @@ -48,6 +53,8 @@ def __init__( test_cases: list[str], func: bool, build_target_result: BuildTargetResult, + build_target_conf: BuildTargetConfiguration, + dts_result: DTSResult ): self.sut_node = sut_node self._logger = getLogger(self.__class__.__name__) @@ -55,6 +62,8 @@ def __init__( self._test_cases_to_run.extend(SETTINGS.test_cases) self._func = func self._result = build_target_result.add_test_suite(self.__class__.__name__) + self.build_target_info = build_target_conf + self._dts_result = dts_result def set_up_suite(self) -> None: """ @@ -118,6 +127,9 @@ def run(self) -> None: f"the next test suite may be affected." ) self._result.update_setup(Result.ERROR, e) + if len(self._result.get_errors()) > 0 and self.is_blocking: + raise BlockingTestSuiteError(test_suite_name) + def _execute_test_suite(self) -> None: """ @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: f"Attempt number {attempt_nr} out of {all_attempts}." ) self._run_test_case(test_case_method, test_case_result) + def _get_functional_test_cases(self) -> list[MethodType]: """ @@ -232,6 +245,11 @@ def _execute_test_case( test_case_result.update(Result.SKIP) raise KeyboardInterrupt("Stop DTS") + def write_to_statistics_file(self, output: Dict[str, str]): + if self._dts_result.output != None: + self._dts_result.output.update(output) + else: + self._dts_result.output = output def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]]: def is_test_suite(object) -> bool: @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: test_suite_class for _, test_suite_class in inspect.getmembers(testcase_module, is_test_suite) ] + + diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py index f54a9470..63f17cc3 100644 --- a/dts/framework/testbed_model/__init__.py +++ b/dts/framework/testbed_model/__init__.py @@ -20,3 +20,8 @@ ) from .node import Node from .sut_node import SutNode + +from .interactive_apps import ( + InteractiveScriptHandler, + TestpmdDriver +) diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py new file mode 100644 index 00000000..0382d7e0 --- /dev/null +++ b/dts/framework/testbed_model/interactive_apps/__init__.py @@ -0,0 +1,6 @@ +from .interactive_command import ( + InteractiveScriptHandler +) +from .testpmd_driver import ( + TestpmdDriver +) \ No newline at end of file diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py new file mode 100644 index 00000000..7467911b --- /dev/null +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py @@ -0,0 +1,57 @@ +# import paramiko +from paramiko import SSHClient, Channel, channel +from framework.settings import SETTINGS + +class InteractiveScriptHandler: + + _ssh_client: SSHClient + _stdin: channel.ChannelStdinFile + _ssh_channel: Channel + + def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None: + self._ssh_client = ssh_client + self._ssh_channel = self._ssh_client.invoke_shell() + self._stdin = self._ssh_channel.makefile_stdin("wb") + self._ssh_channel.settimeout(timeout) + + def send_command(self, command:str) -> None: + """ + Send command to channel without recording output. + + This method will not verify any input or output, it will + simply assume the command succeeded + """ + self._stdin.write(command + '\n') + self._stdin.flush() + + def send_command_get_output(self, command:str, expect:str) -> str: + """ + Send a command and get all output before the expected ending string. + + **NOTE** + Lines that expect input are not included in the stdout buffer so they cannot be + used for expect. For example, if you were prompted to log into something + with a username and password, you cannot expect "username:" because it wont + yet be in the stdout buffer. A work around for this could be consuming an + extra newline character to force the current prompt into the stdout buffer. + + *Return* + All output before expected string + """ + stdout = self._ssh_channel.makefile("r") + self._stdin.write(command + '\n') + self._stdin.flush() + out:str = "" + for line in stdout: + out += str(line) + if expect in str(line): + break + stdout.close() #close the buffer to flush the output + return out + + def close(self): + self._stdin.close() + self._ssh_channel.close() + + def __del__(self): + self.close() diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py new file mode 100644 index 00000000..1993eae6 --- /dev/null +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py @@ -0,0 +1,24 @@ +from framework.testbed_model.interactive_apps import InteractiveScriptHandler + +from pathlib import PurePath + +class TestpmdDriver: + prompt:str = "testpmd>" + interactive_handler: InteractiveScriptHandler + + def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None: + """ + Sets the handler to drive the SSH session and starts testpmd + """ + self.interactive_handler = handler + # self.interactive_handler.send_command("sudo su") + # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build") + self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt) + + def send_command(self, command:str, expect:str = prompt) -> str: + """ + Specific way of handling the command for testpmd + + An extra newline character is consumed in order to force the current line into the stdout buffer + """ + return self.interactive_handler.send_command_get_output(command + "\n", expect) \ No newline at end of file diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py index d48fafe6..c5147e0e 100644 --- a/dts/framework/testbed_model/node.py +++ b/dts/framework/testbed_model/node.py @@ -40,6 +40,7 @@ class Node(object): lcores: list[LogicalCore] _logger: DTSLOG _other_sessions: list[OSSession] + _execution_config: ExecutionConfiguration def __init__(self, node_config: NodeConfiguration): self.config = node_config @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: """ self._setup_hugepages() self._set_up_execution(execution_config) + self._execution_config = execution_config def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: """ diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py index 2b2b50d9..8c39a66d 100644 --- a/dts/framework/testbed_model/sut_node.py +++ b/dts/framework/testbed_model/sut_node.py @@ -14,6 +14,7 @@ from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice from .node import Node +from .interactive_apps import InteractiveScriptHandler class SutNode(Node): @@ -261,6 +262,11 @@ def run_dpdk_app( return self.main_session.send_command( f"{app_path} {eal_args}", timeout, verify=True ) + def create_interactive_session_handler(self) -> InteractiveScriptHandler: + """ + Create a handler for interactive sessions + """ + return InteractiveScriptHandler(self.main_session._interactive_session) class EalParameters(object): diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py new file mode 100644 index 00000000..bacf289d --- /dev/null +++ b/dts/tests/TestSuite_smoke_tests.py @@ -0,0 +1,94 @@ +from framework.test_suite import TestSuite +from framework.testbed_model.sut_node import SutNode + +from framework.testbed_model.interactive_apps import TestpmdDriver + +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> str: + match compiler_name: + case "gcc": + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] + case "clang": + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] + case "msvc": + return sut_node.main_session.send_command(f"cl", 60).stdout + case "icc": + return sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout + +class SmokeTests(TestSuite): + is_blocking = True + + def set_up_suite(self) -> None: + """ + Setup: + build all DPDK + """ + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir + + + def test_unit_tests(self) -> None: + """ + Test: + run the fast-test unit-test suite through meson + """ + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests", 300) + + def test_driver_tests(self) -> None: + """ + Test: + run the driver-test unit-test suite through meson + """ + list_of_vdevs = "" + for dev in self.sut_node._execution_config.vdevs: + list_of_vdevs += f"{dev}," + print(list_of_vdevs) + if len(list_of_vdevs) > 0: + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev {list_of_vdevs}\"", 300) + else: + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests", 300) + + def test_gather_info(self) -> None: + """ + Test: + gather information about the system and send output to statistics.txt + """ + out = {} + + out['OS'] = self.sut_node.main_session.send_command("awk -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout + out["OS VERSION"] = self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print $2}' /etc/os-release", 60, True).stdout + out["COMPILER VERSION"] = get_compiler_version(self.build_target_info.compiler.name, self.sut_node) + out["DPDK VERSION"] = self.sut_node.dpdk_version + if self.build_target_info.os.name == "linux": + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -r", 60).stdout + elif self.build_target_info.os.name == "windows": + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -a", 60).stdout + self.write_to_statistics_file(out) + + + + def test_start_testpmd(self) -> None: + """ + Creates and instance of the testpmd driver to run the testpmd app + """ + driver: TestpmdDriver = TestpmdDriver(self.sut_node.create_interactive_session_handler(), self.dpdk_build_dir_path) + + print(driver.send_command("show port summary all")) + + def test_device_bound_to_driver(self) -> None: + """ + Test: + ensure that all drivers listed in the config are bound to the correct drivers + """ + for nic in self.sut_node._execution_config.nics: + for address in nic.addresses: + out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60) + self.verify( + len(out.stdout) != 0, + f"Failed to find configured device ({address}) using dpdk-devbind.py", + ) + for string in out.stdout.split(" "): + if 'drv=' in string: + self.verify( + string.split("=")[1] == nic.driver.strip(), + f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})', + ) + \ No newline at end of file -- 2.40.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-12 19:25 ` [RFC v2 1/2] dts: add " jspewock @ 2023-05-23 8:05 ` Juraj Linkeš 2023-05-23 8:33 ` Juraj Linkeš 2023-05-24 20:44 ` Jeremy Spewock 2023-05-24 10:03 ` Juraj Linkeš 1 sibling, 2 replies; 12+ messages in thread From: Juraj Linkeš @ 2023-05-23 8:05 UTC (permalink / raw) To: jspewock; +Cc: dev Hi Jeremy, first, a few general points: 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and anyone else involved with DTS or who might be interested) and add the devlist to cc. 2. Run the linter script before submitting. 3. The use of the various nested objects breaks the current abstractions. The basic idea is that the test suite developers should ideally only use the sut/tg node objects and those objects should delegate logic further to their nested objects. More below. I have many comments about the implementation, but I haven't run it yet. I'm going to do that after this round of comments and I may have more ideas. On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > Adds a new test suite for running smoke tests that verify general > configuration aspects of the system under test. If any of these tests > fail, the DTS execution terminates as part of a "fail-fast" model. > > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> > --- > dts/conf.yaml | 9 ++ > dts/framework/config/__init__.py | 21 +++++ > dts/framework/config/conf_yaml_schema.json | 32 ++++++- > dts/framework/dts.py | 19 +++- > dts/framework/exception.py | 11 +++ > dts/framework/remote_session/os_session.py | 6 +- > .../remote_session/remote/__init__.py | 28 ++++++ > dts/framework/test_result.py | 13 ++- > dts/framework/test_suite.py | 24 ++++- > dts/framework/testbed_model/__init__.py | 5 + > .../interactive_apps/__init__.py | 6 ++ > .../interactive_apps/interactive_command.py | 57 +++++++++++ > .../interactive_apps/testpmd_driver.py | 24 +++++ > dts/framework/testbed_model/node.py | 2 + > dts/framework/testbed_model/sut_node.py | 6 ++ > dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++++++ > 16 files changed, 348 insertions(+), 9 deletions(-) > create mode 100644 dts/framework/testbed_model/interactive_apps/__init__.py > create mode 100644 dts/framework/testbed_model/interactive_apps/interactive_command.py > create mode 100644 dts/framework/testbed_model/interactive_apps/testpmd_driver.py Let's not add any more levels. I don't like even the current hw subdirectory (which I want to remove in the docs patch) and we don't need it. I'd also like to move this functionality into remote_session, as it's handling a type of remote session. > create mode 100644 dts/tests/TestSuite_smoke_tests.py > > diff --git a/dts/conf.yaml b/dts/conf.yaml > index a9bd8a3e..042ef954 100644 > --- a/dts/conf.yaml > +++ b/dts/conf.yaml > @@ -10,13 +10,22 @@ executions: > compiler_wrapper: ccache > perf: false > func: true > + nics: #physical devices to be used for testing > + - addresses: > + - "0000:11:00.0" > + - "0000:11:00.1" > + driver: "i40e" > + vdevs: #names of virtual devices to be used for testing > + - "crypto_openssl" I believe we specified the NICs under SUTs in the original DTS, just as Owen did in his internal GitLab patch. If you can access it, have a look at how he did it. This brings an interesting question of where we want to specify which NICs/vdevs to test. It could be on the SUT level, but also on the execution or even the build target level. This should be informed by testing needs. What makes the most sense? We could specify NIC details per SUT/TG and then just reference the NICs on the execution/build target level. > test_suites: > + - smoke_tests > - hello_world > system_under_test: "SUT 1" > nodes: > - name: "SUT 1" > hostname: sut1.change.me.localhost > user: root > + password: "" This was deliberately left out to discourage the use of passwords. > arch: x86_64 > os: linux > lcores: "" <snip> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py > index 05022845..0d03e158 100644 > --- a/dts/framework/dts.py > +++ b/dts/framework/dts.py > @@ -5,6 +5,8 @@ > > import sys > > +from .exception import BlockingTestSuiteError > + > from .config import CONFIGURATION, BuildTargetConfiguration, ExecutionConfiguration > from .logger import DTSLOG, getLogger > from .test_result import BuildTargetResult, DTSResult, ExecutionResult, Result > @@ -49,6 +51,7 @@ def run_all() -> None: > nodes[sut_node.name] = sut_node > > if sut_node: > + #SMOKE TEST EXECUTION GOES HERE! > _run_execution(sut_node, execution, result) > > except Exception as e: > @@ -118,7 +121,7 @@ def _run_build_target( > > try: > sut_node.set_up_build_target(build_target) > - result.dpdk_version = sut_node.dpdk_version > + # result.dpdk_version = sut_node.dpdk_version > build_target_result.update_setup(Result.PASS) > except Exception as e: > dts_logger.exception("Build target setup failed.") > @@ -146,6 +149,7 @@ def _run_suites( > with possibly only a subset of test cases. > If no subset is specified, run all test cases. > """ > + end_execution = False This only ends the build target stage, not the execution stage. We should either find a better name for the variable or make sure that the whole execution is blocked. I think we're aiming for the latter - maybe we could just check whether the last exception in result is a BlockingTestSuiteError (or better, just call a new method of result that will check that), which we could do at multiple stages. > for test_suite_config in execution.test_suites: > try: > full_suite_path = f"tests.TestSuite_{test_suite_config.test_suite}" > @@ -160,13 +164,24 @@ def _run_suites( > > else: > for test_suite_class in test_suite_classes: > + #HERE NEEDS CHANGING > test_suite = test_suite_class( > sut_node, > test_suite_config.test_cases, > execution.func, > build_target_result, > + sut_node._build_target_config, > + result > ) > - test_suite.run() > + try: > + test_suite.run() > + except BlockingTestSuiteError as e: > + dts_logger.exception("An error occurred within a blocking TestSuite, execution will now end.") > + result.add_error(e) > + end_execution = True > + #if a blocking test failed and we need to bail out of suite executions > + if end_execution: > + break > > > def _exit_dts() -> None: > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > index ca353d98..4e3f63d1 100644 > --- a/dts/framework/exception.py > +++ b/dts/framework/exception.py > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): > SSH_ERR = 4 > DPDK_BUILD_ERR = 10 > TESTCASE_VERIFY_ERR = 20 > + BLOCKING_TESTSUITE_ERR = 25 > > > class DTSError(Exception): > @@ -144,3 +145,13 @@ def __init__(self, value: str): > > def __str__(self) -> str: > return repr(self.value) > + > +class BlockingTestSuiteError(DTSError): > + suite_name: str > + severity: ClassVar[ErrorSeverity] = ErrorSeverity.BLOCKING_TESTSUITE_ERR > + > + def __init__(self, suite_name:str) -> None: > + self.suite_name = suite_name > + > + def __str__(self) -> str: > + return f"Blocking suite {self.suite_name} failed." > diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py > index 4c48ae25..22776bc1 100644 > --- a/dts/framework/remote_session/os_session.py > +++ b/dts/framework/remote_session/os_session.py > @@ -12,7 +12,9 @@ > from framework.testbed_model import LogicalCore > from framework.utils import EnvVarsDict, MesonArgs > > -from .remote import CommandResult, RemoteSession, create_remote_session > +from .remote import CommandResult, RemoteSession, create_remote_session, create_interactive_session > + > +from paramiko import SSHClient > > > class OSSession(ABC): > @@ -26,6 +28,7 @@ class OSSession(ABC): > name: str > _logger: DTSLOG > remote_session: RemoteSession > + _interactive_session: SSHClient > > def __init__( > self, > @@ -37,6 +40,7 @@ def __init__( > self.name = name > self._logger = logger > self.remote_session = create_remote_session(node_config, name, logger) > + self._interactive_session = create_interactive_session(node_config, name, logger) This session should be stored in SutNode (it should be os-agnostic (the apps should behave the same on all os's, right?) and SutNode could use it without accessing another object), but created in OSSession (this way we make sure any os-specific inputs (such as paths) are passed properly). > > def close(self, force: bool = False) -> None: > """ > diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py > index 8a151221..abca8edc 100644 > --- a/dts/framework/remote_session/remote/__init__.py > +++ b/dts/framework/remote_session/remote/__init__.py > @@ -9,8 +9,36 @@ > from .remote_session import CommandResult, RemoteSession > from .ssh_session import SSHSession > > +from paramiko import SSHClient, AutoAddPolicy > +from framework.utils import GREEN > > def create_remote_session( > node_config: NodeConfiguration, name: str, logger: DTSLOG > ) -> RemoteSession: > return SSHSession(node_config, name, logger) > + > +def create_interactive_session( > + node_config: NodeConfiguration, name: str, logger: DTSLOG > +) -> SSHClient: > + """ > + Creates a paramiko SSH session that is designed to be used for interactive shells > + > + This session is meant to be used on an "as needed" basis and may never be utilized > + """ > + client: SSHClient = SSHClient() > + client.set_missing_host_key_policy(AutoAddPolicy) > + ip: str = node_config.hostname > + logger.info(GREEN(f"Connecting to host {ip}")) Using colors is a remnant from the original DTS. If we want to (re-)introduce colors I'd do that in a separate patch in a uniform manner. > + #Preset to 22 because paramiko doesn't accept None > + port: int = 22 This configuration handling should be moved to NodeConfiguration. The logic should also be moved to InteractiveScriptHandler. We also probably don't need a factory for this. > + if ":" in node_config.hostname: > + ip, port = node_config.hostname.split(":") > + port = int(port) > + client.connect( > + ip, > + username=node_config.user, > + port=port, > + password=node_config.password or "", > + timeout=20 if port else 10 > + ) > + return client > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py > index 74391982..77202ae2 100644 > --- a/dts/framework/test_result.py > +++ b/dts/framework/test_result.py > @@ -8,6 +8,7 @@ > import os.path > from collections.abc import MutableSequence > from enum import Enum, auto > +from typing import Dict > > from .config import ( > OS, > @@ -67,12 +68,13 @@ class Statistics(dict): > Using a dict provides a convenient way to format the data. > """ > > - def __init__(self, dpdk_version): > + def __init__(self, output_info: Dict[str, str] | None): > super(Statistics, self).__init__() > for result in Result: > self[result.name] = 0 > self["PASS RATE"] = 0.0 > - self["DPDK VERSION"] = dpdk_version > + if output_info: > + for info_key, info_val in output_info.items(): self[info_key] = info_val > > def __iadd__(self, other: Result) -> "Statistics": > """ > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): > """ > > dpdk_version: str | None > + output: dict | None > _logger: DTSLOG > _errors: list[Exception] > _return_code: ErrorSeverity > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): > def __init__(self, logger: DTSLOG): > super(DTSResult, self).__init__() > self.dpdk_version = None > + self.output = None > self._logger = logger > self._errors = [] > self._return_code = ErrorSeverity.NO_ERR > @@ -296,7 +300,10 @@ def process(self) -> None: > for error in self._errors: > self._logger.debug(repr(error)) > > - self._stats_result = Statistics(self.dpdk_version) > + self._stats_result = Statistics(self.output) > + #add information gathered from the smoke tests to the statistics > + # for info_key, info_val in smoke_test_info.items(): self._stats_result[info_key] = info_val > + # print(self._stats_result) > self.add_stats(self._stats_result) > with open(self._stats_filename, "w+") as stats_file: > stats_file.write(str(self._stats_result)) > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > index 0705f38f..1518fb8a 100644 > --- a/dts/framework/test_suite.py > +++ b/dts/framework/test_suite.py > @@ -10,11 +10,14 @@ > import inspect > import re > from types import MethodType > +from typing import Dict > > -from .exception import ConfigurationError, SSHTimeoutError, TestCaseVerifyError > +from .config import BuildTargetConfiguration > + > +from .exception import BlockingTestSuiteError, ConfigurationError, SSHTimeoutError, TestCaseVerifyError > from .logger import DTSLOG, getLogger > from .settings import SETTINGS > -from .test_result import BuildTargetResult, Result, TestCaseResult, TestSuiteResult > +from .test_result import BuildTargetResult, DTSResult, Result, TestCaseResult, TestSuiteResult > from .testbed_model import SutNode > > > @@ -37,10 +40,12 @@ class TestSuite(object): > """ > > sut_node: SutNode > + is_blocking = False > _logger: DTSLOG > _test_cases_to_run: list[str] > _func: bool > _result: TestSuiteResult > + _dts_result: DTSResult > > def __init__( > self, > @@ -48,6 +53,8 @@ def __init__( > test_cases: list[str], > func: bool, > build_target_result: BuildTargetResult, > + build_target_conf: BuildTargetConfiguration, > + dts_result: DTSResult > ): > self.sut_node = sut_node > self._logger = getLogger(self.__class__.__name__) > @@ -55,6 +62,8 @@ def __init__( > self._test_cases_to_run.extend(SETTINGS.test_cases) > self._func = func > self._result = build_target_result.add_test_suite(self.__class__.__name__) > + self.build_target_info = build_target_conf > + self._dts_result = dts_result > > def set_up_suite(self) -> None: > """ > @@ -118,6 +127,9 @@ def run(self) -> None: > f"the next test suite may be affected." > ) > self._result.update_setup(Result.ERROR, e) > + if len(self._result.get_errors()) > 0 and self.is_blocking: > + raise BlockingTestSuiteError(test_suite_name) > + > > def _execute_test_suite(self) -> None: > """ > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: > f"Attempt number {attempt_nr} out of {all_attempts}." > ) > self._run_test_case(test_case_method, test_case_result) > + > > def _get_functional_test_cases(self) -> list[MethodType]: > """ > @@ -232,6 +245,11 @@ def _execute_test_case( > test_case_result.update(Result.SKIP) > raise KeyboardInterrupt("Stop DTS") > > + def write_to_statistics_file(self, output: Dict[str, str]): > + if self._dts_result.output != None: > + self._dts_result.output.update(output) > + else: > + self._dts_result.output = output > > def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]]: > def is_test_suite(object) -> bool: > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: > test_suite_class > for _, test_suite_class in inspect.getmembers(testcase_module, is_test_suite) > ] > + > + > diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py > index f54a9470..63f17cc3 100644 > --- a/dts/framework/testbed_model/__init__.py > +++ b/dts/framework/testbed_model/__init__.py > @@ -20,3 +20,8 @@ > ) > from .node import Node > from .sut_node import SutNode > + > +from .interactive_apps import ( > + InteractiveScriptHandler, > + TestpmdDriver > +) > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py > new file mode 100644 > index 00000000..0382d7e0 > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > @@ -0,0 +1,6 @@ > +from .interactive_command import ( > + InteractiveScriptHandler > +) > +from .testpmd_driver import ( > + TestpmdDriver > +) > \ No newline at end of file > diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py > new file mode 100644 > index 00000000..7467911b > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py In general, the file name should match the class name. > @@ -0,0 +1,57 @@ > +# import paramiko > +from paramiko import SSHClient, Channel, channel > +from framework.settings import SETTINGS > + > +class InteractiveScriptHandler: Once merged with the init functionality and moved to remote_session, I'd rename it to InteractiveAppSession or something similar. > + > + _ssh_client: SSHClient > + _stdin: channel.ChannelStdinFile > + _ssh_channel: Channel > + > + def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None: > + self._ssh_client = ssh_client > + self._ssh_channel = self._ssh_client.invoke_shell() > + self._stdin = self._ssh_channel.makefile_stdin("wb") > + self._ssh_channel.settimeout(timeout) > + > + def send_command(self, command:str) -> None: > + """ > + Send command to channel without recording output. > + > + This method will not verify any input or output, it will > + simply assume the command succeeded > + """ > + self._stdin.write(command + '\n') > + self._stdin.flush() > + > + def send_command_get_output(self, command:str, expect:str) -> str: Let's rename expect to prompt. At least for me it was just confusing in the original DTS. > + """ > + Send a command and get all output before the expected ending string. > + > + **NOTE** > + Lines that expect input are not included in the stdout buffer so they cannot be > + used for expect. For example, if you were prompted to log into something > + with a username and password, you cannot expect "username:" because it wont > + yet be in the stdout buffer. A work around for this could be consuming an > + extra newline character to force the current prompt into the stdout buffer. > + > + *Return* > + All output before expected string > + """ > + stdout = self._ssh_channel.makefile("r") > + self._stdin.write(command + '\n') > + self._stdin.flush() > + out:str = "" > + for line in stdout: > + out += str(line) > + if expect in str(line): > + break > + stdout.close() #close the buffer to flush the output > + return out > + > + def close(self): > + self._stdin.close() > + self._ssh_channel.close() > + > + def __del__(self): > + self.close() > diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > new file mode 100644 > index 00000000..1993eae6 > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > @@ -0,0 +1,24 @@ > +from framework.testbed_model.interactive_apps import InteractiveScriptHandler > + > +from pathlib import PurePath > + > +class TestpmdDriver: This could also be called TestPmdSession. Not sure which is better. This should extend InteractiveScriptHandler, which should contain basic functionality common to all apps and then the drivers would contain app-specific functionality. > + prompt:str = "testpmd>" > + interactive_handler: InteractiveScriptHandler > + > + def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None: > + """ > + Sets the handler to drive the SSH session and starts testpmd > + """ > + self.interactive_handler = handler > + # self.interactive_handler.send_command("sudo su") > + # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build") > + self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt) The paths need to be handled in os-agnostic manner in os_session and then passed here. > + > + def send_command(self, command:str, expect:str = prompt) -> str: > + """ > + Specific way of handling the command for testpmd > + > + An extra newline character is consumed in order to force the current line into the stdout buffer > + """ > + return self.interactive_handler.send_command_get_output(command + "\n", expect) > \ No newline at end of file > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py > index d48fafe6..c5147e0e 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -40,6 +40,7 @@ class Node(object): > lcores: list[LogicalCore] > _logger: DTSLOG > _other_sessions: list[OSSession] > + _execution_config: ExecutionConfiguration > > def __init__(self, node_config: NodeConfiguration): > self.config = node_config > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > """ > self._setup_hugepages() > self._set_up_execution(execution_config) > + self._execution_config = execution_config > > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: > """ > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py > index 2b2b50d9..8c39a66d 100644 > --- a/dts/framework/testbed_model/sut_node.py > +++ b/dts/framework/testbed_model/sut_node.py > @@ -14,6 +14,7 @@ > > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice > from .node import Node > +from .interactive_apps import InteractiveScriptHandler > > > class SutNode(Node): > @@ -261,6 +262,11 @@ def run_dpdk_app( > return self.main_session.send_command( > f"{app_path} {eal_args}", timeout, verify=True > ) > + def create_interactive_session_handler(self) -> InteractiveScriptHandler: > + """ > + Create a handler for interactive sessions > + """ > + return InteractiveScriptHandler(self.main_session._interactive_session) > > > class EalParameters(object): > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py > new file mode 100644 > index 00000000..bacf289d > --- /dev/null > +++ b/dts/tests/TestSuite_smoke_tests.py > @@ -0,0 +1,94 @@ > +from framework.test_suite import TestSuite > +from framework.testbed_model.sut_node import SutNode > + > +from framework.testbed_model.interactive_apps import TestpmdDriver > + > +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> str: I don't see a reason why this is outside SmokeTest. > + match compiler_name: > + case "gcc": > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] As I alluded to earlier, the call here should be sut_node.get_compiler_version(). This is to hide implementation details from test suite developers. Then, SutNode.get_compiler_version should then call self.main_session.get_compiler_version(). The reason here is this is an os-agnostic call. get_compiler_version() should then be defined in os_session and implemented in os specific classes. > + case "clang": > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] > + case "msvc": > + return sut_node.main_session.send_command(f"cl", 60).stdout > + case "icc": > + return sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout > + > +class SmokeTests(TestSuite): > + is_blocking = True > + > + def set_up_suite(self) -> None: > + """ > + Setup: > + build all DPDK > + """ > + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir > + > + > + def test_unit_tests(self) -> None: > + """ > + Test: > + run the fast-test unit-test suite through meson > + """ > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests", 300) Same here, there already are methods that build dpdk. If anything needs to be added, we should expand those methods. > + > + def test_driver_tests(self) -> None: > + """ > + Test: > + run the driver-test unit-test suite through meson > + """ > + list_of_vdevs = "" > + for dev in self.sut_node._execution_config.vdevs: > + list_of_vdevs += f"{dev}," > + print(list_of_vdevs) > + if len(list_of_vdevs) > 0: > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev {list_of_vdevs}\"", 300) > + else: > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests", 300) > + > + def test_gather_info(self) -> None: > + """ > + Test: > + gather information about the system and send output to statistics.txt > + """ > + out = {} > + > + out['OS'] = self.sut_node.main_session.send_command("awk -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout > + out["OS VERSION"] = self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print $2}' /etc/os-release", 60, True).stdout > + out["COMPILER VERSION"] = get_compiler_version(self.build_target_info.compiler.name, self.sut_node) > + out["DPDK VERSION"] = self.sut_node.dpdk_version > + if self.build_target_info.os.name == "linux": > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -r", 60).stdout > + elif self.build_target_info.os.name == "windows": > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -a", 60).stdout > + self.write_to_statistics_file(out) > + This is not really a test. If we want to add this, it should be stored elsewhere (in their respective objects, probably in result objects). Three of these (os, os and kernel versions) are node data, the remaining two are build target data. I'm not sure it's a good idea to put these into statistics, as the statistics are aggregated over all executions and build targets. In case of multiple SUTs (in different executions) and multiple build targets we'd record misleading data. We could include data from all build targets and SUTs though. The reason we (and original DTS) are storing DPDK version in the current manner is that that doesn't change across anything, we're always testing the same tarball. > + > + > + def test_start_testpmd(self) -> None: > + """ > + Creates and instance of the testpmd driver to run the testpmd app > + """ > + driver: TestpmdDriver = TestpmdDriver(self.sut_node.create_interactive_session_handler(), self.dpdk_build_dir_path) The driver should be returned by a method of self.sut_node. > + > + print(driver.send_command("show port summary all")) > + > + def test_device_bound_to_driver(self) -> None: > + """ > + Test: > + ensure that all drivers listed in the config are bound to the correct drivers > + """ > + for nic in self.sut_node._execution_config.nics: > + for address in nic.addresses: > + out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60) This should follow the same abstractions as I outlined above. However, we don't need to use dpdk-devbind here. It's probably safe to do so (the script is likely not going to be changed), but we could > + self.verify( > + len(out.stdout) != 0, > + f"Failed to find configured device ({address}) using dpdk-devbind.py", > + ) > + for string in out.stdout.split(" "): > + if 'drv=' in string: > + self.verify( > + string.split("=")[1] == nic.driver.strip(), > + f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})', > + ) > + > \ No newline at end of file > -- > 2.40.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-23 8:05 ` Juraj Linkeš @ 2023-05-23 8:33 ` Juraj Linkeš 2023-05-24 20:44 ` Jeremy Spewock 1 sibling, 0 replies; 12+ messages in thread From: Juraj Linkeš @ 2023-05-23 8:33 UTC (permalink / raw) To: jspewock; +Cc: dev On Tue, May 23, 2023 at 10:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > > Hi Jeremy, first, a few general points: > One more general point - don't forget to add licenses to all new files and possibly update licenses in other affected files. Juraj ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-23 8:05 ` Juraj Linkeš 2023-05-23 8:33 ` Juraj Linkeš @ 2023-05-24 20:44 ` Jeremy Spewock 2023-05-25 8:33 ` Juraj Linkeš 1 sibling, 1 reply; 12+ messages in thread From: Jeremy Spewock @ 2023-05-24 20:44 UTC (permalink / raw) To: Juraj Linkeš; +Cc: dev [-- Attachment #1: Type: text/plain, Size: 37752 bytes --] On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > Hi Jeremy, first, a few general points: > > 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and > anyone else involved with DTS or who might be interested) and add the > devlist to cc. > Thank you for the tip! I'm still new to sending patches and didn't think to do something like this but I will in the future. > 2. Run the linter script before submitting. > I did forget to run this, I will in the future. > 3. The use of the various nested objects breaks the current > abstractions. The basic idea is that the test suite developers should > ideally only use the sut/tg node objects and those objects should > delegate logic further to their nested objects. More below. > > I have many comments about the implementation, but I haven't run it > yet. I'm going to do that after this round of comments and I may have > more ideas. > > On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: > > > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > > > Adds a new test suite for running smoke tests that verify general > > configuration aspects of the system under test. If any of these tests > > fail, the DTS execution terminates as part of a "fail-fast" model. > > > > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> > > --- > > dts/conf.yaml | 9 ++ > > dts/framework/config/__init__.py | 21 +++++ > > dts/framework/config/conf_yaml_schema.json | 32 ++++++- > > dts/framework/dts.py | 19 +++- > > dts/framework/exception.py | 11 +++ > > dts/framework/remote_session/os_session.py | 6 +- > > .../remote_session/remote/__init__.py | 28 ++++++ > > dts/framework/test_result.py | 13 ++- > > dts/framework/test_suite.py | 24 ++++- > > dts/framework/testbed_model/__init__.py | 5 + > > .../interactive_apps/__init__.py | 6 ++ > > .../interactive_apps/interactive_command.py | 57 +++++++++++ > > .../interactive_apps/testpmd_driver.py | 24 +++++ > > dts/framework/testbed_model/node.py | 2 + > > dts/framework/testbed_model/sut_node.py | 6 ++ > > dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++++++ > > 16 files changed, 348 insertions(+), 9 deletions(-) > > create mode 100644 > dts/framework/testbed_model/interactive_apps/__init__.py > > create mode 100644 > dts/framework/testbed_model/interactive_apps/interactive_command.py > > create mode 100644 > dts/framework/testbed_model/interactive_apps/testpmd_driver.py > > Let's not add any more levels. I don't like even the current hw > subdirectory (which I want to remove in the docs patch) and we don't > need it. I'd also like to move this functionality into remote_session, > as it's handling a type of remote session. > I think it makes sense to add a proper wrapper for it, I just didn't create a subclass for it off of remote_session because the idea behind it was only allowing users to interact with the session through the InteractiveScriptHandler/DPDK app handlers. If it were part of the remote_session class it would have to include operations for sending commands the way it is written now. I could do this but it seems like a bigger discussion about whether interactive sessions should create a new SSH session every time or instead just use one existing session and create channels off of it. > > > create mode 100644 dts/tests/TestSuite_smoke_tests.py > > > > diff --git a/dts/conf.yaml b/dts/conf.yaml > > index a9bd8a3e..042ef954 100644 > > --- a/dts/conf.yaml > > +++ b/dts/conf.yaml > > @@ -10,13 +10,22 @@ executions: > > compiler_wrapper: ccache > > perf: false > > func: true > > + nics: #physical devices to be used for testing > > + - addresses: > > + - "0000:11:00.0" > > + - "0000:11:00.1" > > + driver: "i40e" > > + vdevs: #names of virtual devices to be used for testing > > + - "crypto_openssl" > > I believe we specified the NICs under SUTs in the original DTS, just > as Owen did in his internal GitLab patch. If you can access it, have a > look at how he did it. > This brings an interesting question of where we want to specify which > NICs/vdevs to test. It could be on the SUT level, but also on the > execution or even the build target level. This should be informed by > testing needs. What makes the most sense? We could specify NIC details > per SUT/TG and then just reference the NICs on the execution/build > target level. > I put it on the execution level with the thought that you might have multiple NICs in your SUT but want to separate them into different executions. I guess in the case of smoke tests, you'd only need to know them per target because it was talked about previously that then is when we should run the smoke tests. I think however it would make sense to specify the NIC you are using for that execution rather than having to assume or specify elsewhere. > > > test_suites: > > + - smoke_tests > > - hello_world > > system_under_test: "SUT 1" > > nodes: > > - name: "SUT 1" > > hostname: sut1.change.me.localhost > > user: root > > + password: "" > > This was deliberately left out to discourage the use of passwords. > > > arch: x86_64 > > os: linux > > lcores: "" > > <snip> > > > diff --git a/dts/framework/dts.py b/dts/framework/dts.py > > index 05022845..0d03e158 100644 > > --- a/dts/framework/dts.py > > +++ b/dts/framework/dts.py > > @@ -5,6 +5,8 @@ > > > > import sys > > > > +from .exception import BlockingTestSuiteError > > + > > from .config import CONFIGURATION, BuildTargetConfiguration, > ExecutionConfiguration > > from .logger import DTSLOG, getLogger > > from .test_result import BuildTargetResult, DTSResult, ExecutionResult, > Result > > @@ -49,6 +51,7 @@ def run_all() -> None: > > nodes[sut_node.name] = sut_node > > > > if sut_node: > > + #SMOKE TEST EXECUTION GOES HERE! > > _run_execution(sut_node, execution, result) > > > > except Exception as e: > > @@ -118,7 +121,7 @@ def _run_build_target( > > > > try: > > sut_node.set_up_build_target(build_target) > > - result.dpdk_version = sut_node.dpdk_version > > + # result.dpdk_version = sut_node.dpdk_version > > build_target_result.update_setup(Result.PASS) > > except Exception as e: > > dts_logger.exception("Build target setup failed.") > > @@ -146,6 +149,7 @@ def _run_suites( > > with possibly only a subset of test cases. > > If no subset is specified, run all test cases. > > """ > > + end_execution = False > > This only ends the build target stage, not the execution stage. We > should either find a better name for the variable or make sure that > the whole execution is blocked. I think we're aiming for the latter - > maybe we could just check whether the last exception in result is a > BlockingTestSuiteError (or better, just call a new method of result > that will check that), which we could do at multiple stages. > Good catch. When writing this and figuring out how to get it to work, I was thinking about smoke tests on a per build target basis because if it failed on one build target it could theoretically pass on another, but you'd likely want your entire execution to fail than have partial results. > > > for test_suite_config in execution.test_suites: > > try: > > full_suite_path = > f"tests.TestSuite_{test_suite_config.test_suite}" > > @@ -160,13 +164,24 @@ def _run_suites( > > > > else: > > for test_suite_class in test_suite_classes: > > + #HERE NEEDS CHANGING > > test_suite = test_suite_class( > > sut_node, > > test_suite_config.test_cases, > > execution.func, > > build_target_result, > > + sut_node._build_target_config, > > + result > > ) > > - test_suite.run() > > + try: > > + test_suite.run() > > + except BlockingTestSuiteError as e: > > + dts_logger.exception("An error occurred within a > blocking TestSuite, execution will now end.") > > + result.add_error(e) > > + end_execution = True > > + #if a blocking test failed and we need to bail out of suite > executions > > + if end_execution: > > + break > > > > > > def _exit_dts() -> None: > > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > > index ca353d98..4e3f63d1 100644 > > --- a/dts/framework/exception.py > > +++ b/dts/framework/exception.py > > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): > > SSH_ERR = 4 > > DPDK_BUILD_ERR = 10 > > TESTCASE_VERIFY_ERR = 20 > > + BLOCKING_TESTSUITE_ERR = 25 > > > > > > class DTSError(Exception): > > @@ -144,3 +145,13 @@ def __init__(self, value: str): > > > > def __str__(self) -> str: > > return repr(self.value) > > + > > +class BlockingTestSuiteError(DTSError): > > + suite_name: str > > + severity: ClassVar[ErrorSeverity] = > ErrorSeverity.BLOCKING_TESTSUITE_ERR > > + > > + def __init__(self, suite_name:str) -> None: > > + self.suite_name = suite_name > > + > > + def __str__(self) -> str: > > + return f"Blocking suite {self.suite_name} failed." > > diff --git a/dts/framework/remote_session/os_session.py > b/dts/framework/remote_session/os_session.py > > index 4c48ae25..22776bc1 100644 > > --- a/dts/framework/remote_session/os_session.py > > +++ b/dts/framework/remote_session/os_session.py > > @@ -12,7 +12,9 @@ > > from framework.testbed_model import LogicalCore > > from framework.utils import EnvVarsDict, MesonArgs > > > > -from .remote import CommandResult, RemoteSession, create_remote_session > > +from .remote import CommandResult, RemoteSession, > create_remote_session, create_interactive_session > > + > > +from paramiko import SSHClient > > > > > > class OSSession(ABC): > > @@ -26,6 +28,7 @@ class OSSession(ABC): > > name: str > > _logger: DTSLOG > > remote_session: RemoteSession > > + _interactive_session: SSHClient > > > > def __init__( > > self, > > @@ -37,6 +40,7 @@ def __init__( > > self.name = name > > self._logger = logger > > self.remote_session = create_remote_session(node_config, name, > logger) > > + self._interactive_session = > create_interactive_session(node_config, name, logger) > > This session should be stored in SutNode (it should be os-agnostic > (the apps should behave the same on all os's, right?) and SutNode > could use it without accessing another object), but created in > OSSession (this way we make sure any os-specific inputs (such as > paths) are passed properly). > I can move it into SutNode, the main reason I left it there is because I was essentially thinking of it the same way as the remote session that gets created once per node. However, I could just as easily have SutNode store it and call upon OSSession to create one inside its constructor. This would make creating classes for DPDK apps easier as well once those are subclasses of the InteractiveScriptHandler. > > > > def close(self, force: bool = False) -> None: > > """ > > diff --git a/dts/framework/remote_session/remote/__init__.py > b/dts/framework/remote_session/remote/__init__.py > > index 8a151221..abca8edc 100644 > > --- a/dts/framework/remote_session/remote/__init__.py > > +++ b/dts/framework/remote_session/remote/__init__.py > > @@ -9,8 +9,36 @@ > > from .remote_session import CommandResult, RemoteSession > > from .ssh_session import SSHSession > > > > +from paramiko import SSHClient, AutoAddPolicy > > +from framework.utils import GREEN > > > > def create_remote_session( > > node_config: NodeConfiguration, name: str, logger: DTSLOG > > ) -> RemoteSession: > > return SSHSession(node_config, name, logger) > > + > > +def create_interactive_session( > > + node_config: NodeConfiguration, name: str, logger: DTSLOG > > +) -> SSHClient: > > + """ > > + Creates a paramiko SSH session that is designed to be used for > interactive shells > > + > > + This session is meant to be used on an "as needed" basis and may > never be utilized > > + """ > > + client: SSHClient = SSHClient() > > + client.set_missing_host_key_policy(AutoAddPolicy) > > + ip: str = node_config.hostname > > + logger.info(GREEN(f"Connecting to host {ip}")) > > Using colors is a remnant from the original DTS. If we want to > (re-)introduce colors I'd do that in a separate patch in a uniform > manner. > I agree this is likely outside the scope of this patch and the color here really isn't necessary either so I'll remove it. > > > + #Preset to 22 because paramiko doesn't accept None > > + port: int = 22 > > This configuration handling should be moved to NodeConfiguration. > The logic should also be moved to InteractiveScriptHandler. We also > probably don't need a factory for this. > I agree that the configuration could be moved into NodeConfiguration, however the reason I use a factory and don't have this connect logic inside InteractiveScriptHandler is because that would mean there would have to be an SSH session per application. This is one potential solution, but what I am doing here instead is creating one SSH session for interactive scripts at the start alongside the usual remote session. This session essentially exists in the background for the duration of the execution and then each application for DPDK creates a channel off that session when they are created. Then, when you are done using the application, it closes the channel but the session itself stays open. > > > + if ":" in node_config.hostname: > > + ip, port = node_config.hostname.split(":") > > + port = int(port) > > + client.connect( > > + ip, > > + username=node_config.user, > > + port=port, > > + password=node_config.password or "", > > + timeout=20 if port else 10 > > + ) > > + return client > > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py > > index 74391982..77202ae2 100644 > > --- a/dts/framework/test_result.py > > +++ b/dts/framework/test_result.py > > @@ -8,6 +8,7 @@ > > import os.path > > from collections.abc import MutableSequence > > from enum import Enum, auto > > +from typing import Dict > > > > from .config import ( > > OS, > > @@ -67,12 +68,13 @@ class Statistics(dict): > > Using a dict provides a convenient way to format the data. > > """ > > > > - def __init__(self, dpdk_version): > > + def __init__(self, output_info: Dict[str, str] | None): > > super(Statistics, self).__init__() > > for result in Result: > > self[result.name] = 0 > > self["PASS RATE"] = 0.0 > > - self["DPDK VERSION"] = dpdk_version > > + if output_info: > > + for info_key, info_val in output_info.items(): > self[info_key] = info_val > > > > def __iadd__(self, other: Result) -> "Statistics": > > """ > > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): > > """ > > > > dpdk_version: str | None > > + output: dict | None > > _logger: DTSLOG > > _errors: list[Exception] > > _return_code: ErrorSeverity > > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): > > def __init__(self, logger: DTSLOG): > > super(DTSResult, self).__init__() > > self.dpdk_version = None > > + self.output = None > > self._logger = logger > > self._errors = [] > > self._return_code = ErrorSeverity.NO_ERR > > @@ -296,7 +300,10 @@ def process(self) -> None: > > for error in self._errors: > > self._logger.debug(repr(error)) > > > > - self._stats_result = Statistics(self.dpdk_version) > > + self._stats_result = Statistics(self.output) > > + #add information gathered from the smoke tests to the statistics > > + # for info_key, info_val in smoke_test_info.items(): > self._stats_result[info_key] = info_val > > + # print(self._stats_result) > > self.add_stats(self._stats_result) > > with open(self._stats_filename, "w+") as stats_file: > > stats_file.write(str(self._stats_result)) > > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > > index 0705f38f..1518fb8a 100644 > > --- a/dts/framework/test_suite.py > > +++ b/dts/framework/test_suite.py > > @@ -10,11 +10,14 @@ > > import inspect > > import re > > from types import MethodType > > +from typing import Dict > > > > -from .exception import ConfigurationError, SSHTimeoutError, > TestCaseVerifyError > > +from .config import BuildTargetConfiguration > > + > > +from .exception import BlockingTestSuiteError, ConfigurationError, > SSHTimeoutError, TestCaseVerifyError > > from .logger import DTSLOG, getLogger > > from .settings import SETTINGS > > -from .test_result import BuildTargetResult, Result, TestCaseResult, > TestSuiteResult > > +from .test_result import BuildTargetResult, DTSResult, Result, > TestCaseResult, TestSuiteResult > > from .testbed_model import SutNode > > > > > > @@ -37,10 +40,12 @@ class TestSuite(object): > > """ > > > > sut_node: SutNode > > + is_blocking = False > > _logger: DTSLOG > > _test_cases_to_run: list[str] > > _func: bool > > _result: TestSuiteResult > > + _dts_result: DTSResult > > > > def __init__( > > self, > > @@ -48,6 +53,8 @@ def __init__( > > test_cases: list[str], > > func: bool, > > build_target_result: BuildTargetResult, > > + build_target_conf: BuildTargetConfiguration, > > + dts_result: DTSResult > > ): > > self.sut_node = sut_node > > self._logger = getLogger(self.__class__.__name__) > > @@ -55,6 +62,8 @@ def __init__( > > self._test_cases_to_run.extend(SETTINGS.test_cases) > > self._func = func > > self._result = > build_target_result.add_test_suite(self.__class__.__name__) > > + self.build_target_info = build_target_conf > > + self._dts_result = dts_result > > > > def set_up_suite(self) -> None: > > """ > > @@ -118,6 +127,9 @@ def run(self) -> None: > > f"the next test suite may be affected." > > ) > > self._result.update_setup(Result.ERROR, e) > > + if len(self._result.get_errors()) > 0 and self.is_blocking: > > + raise BlockingTestSuiteError(test_suite_name) > > + > > > > def _execute_test_suite(self) -> None: > > """ > > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: > > f"Attempt number {attempt_nr} out of > {all_attempts}." > > ) > > self._run_test_case(test_case_method, > test_case_result) > > + > > > > def _get_functional_test_cases(self) -> list[MethodType]: > > """ > > @@ -232,6 +245,11 @@ def _execute_test_case( > > test_case_result.update(Result.SKIP) > > raise KeyboardInterrupt("Stop DTS") > > > > + def write_to_statistics_file(self, output: Dict[str, str]): > > + if self._dts_result.output != None: > > + self._dts_result.output.update(output) > > + else: > > + self._dts_result.output = output > > > > def get_test_suites(testsuite_module_path: str) -> > list[type[TestSuite]]: > > def is_test_suite(object) -> bool: > > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: > > test_suite_class > > for _, test_suite_class in inspect.getmembers(testcase_module, > is_test_suite) > > ] > > + > > + > > diff --git a/dts/framework/testbed_model/__init__.py > b/dts/framework/testbed_model/__init__.py > > index f54a9470..63f17cc3 100644 > > --- a/dts/framework/testbed_model/__init__.py > > +++ b/dts/framework/testbed_model/__init__.py > > @@ -20,3 +20,8 @@ > > ) > > from .node import Node > > from .sut_node import SutNode > > + > > +from .interactive_apps import ( > > + InteractiveScriptHandler, > > + TestpmdDriver > > +) > > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py > b/dts/framework/testbed_model/interactive_apps/__init__.py > > new file mode 100644 > > index 00000000..0382d7e0 > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > > @@ -0,0 +1,6 @@ > > +from .interactive_command import ( > > + InteractiveScriptHandler > > +) > > +from .testpmd_driver import ( > > + TestpmdDriver > > +) > > \ No newline at end of file > > diff --git > a/dts/framework/testbed_model/interactive_apps/interactive_command.py > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > > new file mode 100644 > > index 00000000..7467911b > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py > > In general, the file name should match the class name. > > > @@ -0,0 +1,57 @@ > > +# import paramiko > > +from paramiko import SSHClient, Channel, channel > > +from framework.settings import SETTINGS > > + > > +class InteractiveScriptHandler: > > Once merged with the init functionality and moved to remote_session, > I'd rename it to InteractiveAppSession or something similar. > Good point, I'll rename the class and the file. > > > + > > + _ssh_client: SSHClient > > + _stdin: channel.ChannelStdinFile > > + _ssh_channel: Channel > > + > > + def __init__(self, ssh_client: SSHClient, timeout:float = > SETTINGS.timeout) -> None: > > + self._ssh_client = ssh_client > > + self._ssh_channel = self._ssh_client.invoke_shell() > > + self._stdin = self._ssh_channel.makefile_stdin("wb") > > + self._ssh_channel.settimeout(timeout) > > + > > + def send_command(self, command:str) -> None: > > + """ > > + Send command to channel without recording output. > > + > > + This method will not verify any input or output, it will > > + simply assume the command succeeded > > + """ > > + self._stdin.write(command + '\n') > > + self._stdin.flush() > > + > > + def send_command_get_output(self, command:str, expect:str) -> str: > > Let's rename expect to prompt. At least for me it was just confusing > in the original DTS. > It definitely can be confusing, I'll change it accordingly as you're right, prompt is more clear. > > > + """ > > + Send a command and get all output before the expected ending > string. > > + > > + **NOTE** > > + Lines that expect input are not included in the stdout buffer > so they cannot be > > + used for expect. For example, if you were prompted to log into > something > > + with a username and password, you cannot expect "username:" > because it wont > > + yet be in the stdout buffer. A work around for this could be > consuming an > > + extra newline character to force the current prompt into the > stdout buffer. > > + > > + *Return* > > + All output before expected string > > + """ > > + stdout = self._ssh_channel.makefile("r") > > + self._stdin.write(command + '\n') > > + self._stdin.flush() > > + out:str = "" > > + for line in stdout: > > + out += str(line) > > + if expect in str(line): > > + break > > + stdout.close() #close the buffer to flush the output > > + return out > > + > > + def close(self): > > + self._stdin.close() > > + self._ssh_channel.close() > > + > > + def __del__(self): > > + self.close() > > diff --git > a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > > new file mode 100644 > > index 00000000..1993eae6 > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > > @@ -0,0 +1,24 @@ > > +from framework.testbed_model.interactive_apps import > InteractiveScriptHandler > > + > > +from pathlib import PurePath > > + > > +class TestpmdDriver: > > This could also be called TestPmdSession. Not sure which is better. > This should extend InteractiveScriptHandler, which should contain > basic functionality common to all apps and then the drivers would > contain app-specific functionality. > Originally the reason I avoided doing this was because I viewed them as separate layers of the process essentially. I wrote it in a way where the InteractiveScriptHandler was almost like an interface for any CLI and then DPDK apps could interact with this interface, but after thinking about it more, this isn't necessary. There is no reason really that the applications themselves can't just use these methods directly and it also avoids the need to have a reference to the handler within the object. > > > + prompt:str = "testpmd>" > > + interactive_handler: InteractiveScriptHandler > > + > > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> > None: > > + """ > > + Sets the handler to drive the SSH session and starts testpmd > > + """ > > + self.interactive_handler = handler > > + # self.interactive_handler.send_command("sudo su") > > + # self.interactive_handler.send_command("cd > /root/testpmd-testing/dpdk/build") > > + > self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > > The paths need to be handled in os-agnostic manner in os_session and > then passed here. > > > + > > + def send_command(self, command:str, expect:str = prompt) -> str: > > + """ > > + Specific way of handling the command for testpmd > > + > > + An extra newline character is consumed in order to force the > current line into the stdout buffer > > + """ > > + return self.interactive_handler.send_command_get_output(command > + "\n", expect) > > \ No newline at end of file > > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_model/node.py > > index d48fafe6..c5147e0e 100644 > > --- a/dts/framework/testbed_model/node.py > > +++ b/dts/framework/testbed_model/node.py > > @@ -40,6 +40,7 @@ class Node(object): > > lcores: list[LogicalCore] > > _logger: DTSLOG > > _other_sessions: list[OSSession] > > + _execution_config: ExecutionConfiguration > > > > def __init__(self, node_config: NodeConfiguration): > > self.config = node_config > > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > > """ > > self._setup_hugepages() > > self._set_up_execution(execution_config) > > + self._execution_config = execution_config > > > > def _set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > > """ > > diff --git a/dts/framework/testbed_model/sut_node.py > b/dts/framework/testbed_model/sut_node.py > > index 2b2b50d9..8c39a66d 100644 > > --- a/dts/framework/testbed_model/sut_node.py > > +++ b/dts/framework/testbed_model/sut_node.py > > @@ -14,6 +14,7 @@ > > > > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice > > from .node import Node > > +from .interactive_apps import InteractiveScriptHandler > > > > > > class SutNode(Node): > > @@ -261,6 +262,11 @@ def run_dpdk_app( > > return self.main_session.send_command( > > f"{app_path} {eal_args}", timeout, verify=True > > ) > > + def create_interactive_session_handler(self) -> > InteractiveScriptHandler: > > + """ > > + Create a handler for interactive sessions > > + """ > > + return > InteractiveScriptHandler(self.main_session._interactive_session) > > > > > > class EalParameters(object): > > diff --git a/dts/tests/TestSuite_smoke_tests.py > b/dts/tests/TestSuite_smoke_tests.py > > new file mode 100644 > > index 00000000..bacf289d > > --- /dev/null > > +++ b/dts/tests/TestSuite_smoke_tests.py > > @@ -0,0 +1,94 @@ > > +from framework.test_suite import TestSuite > > +from framework.testbed_model.sut_node import SutNode > > + > > +from framework.testbed_model.interactive_apps import TestpmdDriver > > + > > +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> str: > > I don't see a reason why this is outside SmokeTest. > > + match compiler_name: > > + case "gcc": > > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > > As I alluded to earlier, the call here should be > sut_node.get_compiler_version(). This is to hide implementation > details from test suite developers. > Then, SutNode.get_compiler_version should then call > self.main_session.get_compiler_version(). The reason here is this is > an os-agnostic call. > get_compiler_version() should then be defined in os_session and > implemented in os specific classes. > I originally placed the method outside of smoke tests just so it wouldn't be run with the rest of the suite but it does make sense to make this and the other comments os-agnostic. > > > + case "clang": > > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > > + case "msvc": > > + return sut_node.main_session.send_command(f"cl", > 60).stdout > > + case "icc": > > + return > sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout > > + > > +class SmokeTests(TestSuite): > > + is_blocking = True > > + > > + def set_up_suite(self) -> None: > > + """ > > + Setup: > > + build all DPDK > > + """ > > + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir > > + > > + > > + def test_unit_tests(self) -> None: > > + """ > > + Test: > > + run the fast-test unit-test suite through meson > > + """ > > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite fast-tests", 300) > > Same here, there already are methods that build dpdk. If anything > needs to be added, we should expand those methods. > I don't think this is building DPDK, this is just running fast tests and ensuring that you are in the correct directory. > > > + > > + def test_driver_tests(self) -> None: > > + """ > > + Test: > > + run the driver-test unit-test suite through meson > > + """ > > + list_of_vdevs = "" > > + for dev in self.sut_node._execution_config.vdevs: > > + list_of_vdevs += f"{dev}," > > + print(list_of_vdevs) > > + if len(list_of_vdevs) > 0: > > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev > {list_of_vdevs}\"", 300) > > + else: > > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite driver-tests", 300) > > + > > + def test_gather_info(self) -> None: > > + """ > > + Test: > > + gather information about the system and send output to > statistics.txt > > + """ > > + out = {} > > + > > + out['OS'] = self.sut_node.main_session.send_command("awk -F= > '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout > > + out["OS VERSION"] = > self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print > $2}' /etc/os-release", 60, True).stdout > > + out["COMPILER VERSION"] = get_compiler_version( > self.build_target_info.compiler.name, self.sut_node) > > + out["DPDK VERSION"] = self.sut_node.dpdk_version > > + if self.build_target_info.os.name == "linux": > > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -r", 60).stdout > > + elif self.build_target_info.os.name == "windows": > > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -a", 60).stdout > > + self.write_to_statistics_file(out) > > + > > This is not really a test. If we want to add this, it should be stored > elsewhere (in their respective objects, probably in result objects). > Three of these (os, os and kernel versions) are node data, the > remaining two are build target data. > I'm not sure it's a good idea to put these into statistics, as the > statistics are aggregated over all executions and build targets. In > case of multiple SUTs (in different executions) and multiple build > targets we'd record misleading data. We could include data from all > build targets and SUTs though. > The reason we (and original DTS) are storing DPDK version in the > current manner is that that doesn't change across anything, we're > always testing the same tarball. > You're right that this isn't really testing anything, I had originally included it here because it was just part of the smoke test outline. It's definitely out of place though and I can move it out to their respective classes. I think it might make sense to organize/label data based on the SUT it comes from, I just thought statistics made the most sense because you could see the test statistics as well as what it was testing in one place. > > > + > > + > > + def test_start_testpmd(self) -> None: > > + """ > > + Creates and instance of the testpmd driver to run the testpmd > app > > + """ > > + driver: TestpmdDriver = > TestpmdDriver(self.sut_node.create_interactive_session_handler(), > self.dpdk_build_dir_path) > > The driver should be returned by a method of self.sut_node. > Does it make more sense to have methods for creating handlers for various DPDK apps in SutNode? I had assumed it would be cleaner to just make the classes as needed so that you don't have multiple methods doing something very similar in SutNode (basically just making a class with a different name that takes in similar information). I suppose that if you were trying to have developers only interact with the SutNode class this makes sense. This is also something that could make good use of having the SSHClient in the SutNode and I could see how it would be useful. > > > + > > + print(driver.send_command("show port summary all")) > > + > > + def test_device_bound_to_driver(self) -> None: > > + """ > > + Test: > > + ensure that all drivers listed in the config are bound to > the correct drivers > > + """ > > + for nic in self.sut_node._execution_config.nics: > > + for address in nic.addresses: > > + out = > self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py > --status | grep {address}", 60) > > This should follow the same abstractions as I outlined above. > However, we don't need to use dpdk-devbind here. It's probably safe to > do so (the script is likely not going to be changed), but we could > That's a good point about dev-bind, it might be better here to just avoid the reliance on another script. > > > + self.verify( > > + len(out.stdout) != 0, > > + f"Failed to find configured device ({address}) > using dpdk-devbind.py", > > + ) > > + for string in out.stdout.split(" "): > > + if 'drv=' in string: > > + self.verify( > > + string.split("=")[1] == nic.driver.strip(), > > + f'Driver for device {address} does not > match driver listed in configuration (bound to {string.split("=")[1]})', > > + ) > > + > > \ No newline at end of file > > -- > > 2.40.1 > > > I'll work on these changes, but I'd like to hear what you think about what I had mentioned about moving the connect logic to the InteractiveScriptHandler in the comments above. I had originally written it to use one session throughout rather than opening and closing SSH connections for every application but I'd like to hear which you think would be easier/better if there is a difference. [-- Attachment #2: Type: text/html, Size: 48012 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-24 20:44 ` Jeremy Spewock @ 2023-05-25 8:33 ` Juraj Linkeš 2023-05-25 18:02 ` Jeremy Spewock 0 siblings, 1 reply; 12+ messages in thread From: Juraj Linkeš @ 2023-05-25 8:33 UTC (permalink / raw) To: Jeremy Spewock; +Cc: dev One more point that doesn't fit elsewhere: On Wed, May 24, 2023 at 10:45 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote: > > > > On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: >> >> Hi Jeremy, first, a few general points: >> >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and >> anyone else involved with DTS or who might be interested) and add the >> devlist to cc. > > > Thank you for the tip! I'm still new to sending patches and didn't think to do something like this but I will in the future. > >> >> 2. Run the linter script before submitting. > > > I did forget to run this, I will in the future. > >> >> 3. The use of the various nested objects breaks the current >> abstractions. The basic idea is that the test suite developers should >> ideally only use the sut/tg node objects and those objects should >> delegate logic further to their nested objects. More below. >> >> I have many comments about the implementation, but I haven't run it >> yet. I'm going to do that after this round of comments and I may have >> more ideas. >> >> On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: >> > >> > From: Jeremy Spewock <jspewock@iol.unh.edu> >> > >> > Adds a new test suite for running smoke tests that verify general >> > configuration aspects of the system under test. If any of these tests >> > fail, the DTS execution terminates as part of a "fail-fast" model. >> > >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> >> > --- >> > dts/conf.yaml | 9 ++ >> > dts/framework/config/__init__.py | 21 +++++ >> > dts/framework/config/conf_yaml_schema.json | 32 ++++++- >> > dts/framework/dts.py | 19 +++- >> > dts/framework/exception.py | 11 +++ >> > dts/framework/remote_session/os_session.py | 6 +- >> > .../remote_session/remote/__init__.py | 28 ++++++ >> > dts/framework/test_result.py | 13 ++- >> > dts/framework/test_suite.py | 24 ++++- >> > dts/framework/testbed_model/__init__.py | 5 + >> > .../interactive_apps/__init__.py | 6 ++ >> > .../interactive_apps/interactive_command.py | 57 +++++++++++ >> > .../interactive_apps/testpmd_driver.py | 24 +++++ >> > dts/framework/testbed_model/node.py | 2 + >> > dts/framework/testbed_model/sut_node.py | 6 ++ >> > dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++++++ >> > 16 files changed, 348 insertions(+), 9 deletions(-) >> > create mode 100644 dts/framework/testbed_model/interactive_apps/__init__.py >> > create mode 100644 dts/framework/testbed_model/interactive_apps/interactive_command.py >> > create mode 100644 dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> >> Let's not add any more levels. I don't like even the current hw >> subdirectory (which I want to remove in the docs patch) and we don't >> need it. I'd also like to move this functionality into remote_session, >> as it's handling a type of remote session. > > > I think it makes sense to add a proper wrapper for it, I just didn't create a subclass for it off of remote_session because the idea behind it was only allowing users to interact with the session through the InteractiveScriptHandler/DPDK app handlers. If it were part of the remote_session class it would have to include operations for sending commands the way it is written now. I could do this but it seems like a bigger discussion about whether interactive sessions should create a new SSH session every time or instead just use one existing session and create channels off of it. > I wasn't clear, I meant to move the python modules into the remote_session folder. The subclassing would be there only to have a common API across these remote sessions (as the InteractiveScriptHandler is a kind of a remote session). If at all possible, this should be our aim. >> >> >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py >> > >> > diff --git a/dts/conf.yaml b/dts/conf.yaml >> > index a9bd8a3e..042ef954 100644 >> > --- a/dts/conf.yaml >> > +++ b/dts/conf.yaml >> > @@ -10,13 +10,22 @@ executions: >> > compiler_wrapper: ccache >> > perf: false >> > func: true >> > + nics: #physical devices to be used for testing >> > + - addresses: >> > + - "0000:11:00.0" >> > + - "0000:11:00.1" >> > + driver: "i40e" >> > + vdevs: #names of virtual devices to be used for testing >> > + - "crypto_openssl" >> >> I believe we specified the NICs under SUTs in the original DTS, just >> as Owen did in his internal GitLab patch. If you can access it, have a >> look at how he did it. >> This brings an interesting question of where we want to specify which >> NICs/vdevs to test. It could be on the SUT level, but also on the >> execution or even the build target level. This should be informed by >> testing needs. What makes the most sense? We could specify NIC details >> per SUT/TG and then just reference the NICs on the execution/build >> target level. > > > I put it on the execution level with the thought that you might have multiple NICs in your SUT but want to separate them into different executions. This is a good point. Is this something you're interested in in the lab? We should ask Lijuan what she thinks of this. In general, I like this, so we should at least think of how to add NIC config implementation so that we could add this later if we're not adding it now. > I guess in the case of smoke tests, you'd only need to know them per target because it was talked about previously that then is when we should run the smoke tests. I think however it would make sense to specify the NIC you are using for that execution rather than having to assume or specify elsewhere. > Nothing is set in stone, we don't have to run them per build target. We could have two smoke test suites, one per execution and one per build target. I actually like that a lot and we should explore that. If it's not much extra work, we could just do the split like that. >> >> >> > test_suites: >> > + - smoke_tests >> > - hello_world >> > system_under_test: "SUT 1" >> > nodes: >> > - name: "SUT 1" >> > hostname: sut1.change.me.localhost >> > user: root >> > + password: "" >> >> This was deliberately left out to discourage the use of passwords. >> >> > arch: x86_64 >> > os: linux >> > lcores: "" >> >> <snip> >> >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py >> > index 05022845..0d03e158 100644 >> > --- a/dts/framework/dts.py >> > +++ b/dts/framework/dts.py >> > @@ -5,6 +5,8 @@ >> > >> > import sys >> > >> > +from .exception import BlockingTestSuiteError >> > + >> > from .config import CONFIGURATION, BuildTargetConfiguration, ExecutionConfiguration >> > from .logger import DTSLOG, getLogger >> > from .test_result import BuildTargetResult, DTSResult, ExecutionResult, Result >> > @@ -49,6 +51,7 @@ def run_all() -> None: >> > nodes[sut_node.name] = sut_node >> > >> > if sut_node: >> > + #SMOKE TEST EXECUTION GOES HERE! >> > _run_execution(sut_node, execution, result) >> > >> > except Exception as e: >> > @@ -118,7 +121,7 @@ def _run_build_target( >> > >> > try: >> > sut_node.set_up_build_target(build_target) >> > - result.dpdk_version = sut_node.dpdk_version >> > + # result.dpdk_version = sut_node.dpdk_version >> > build_target_result.update_setup(Result.PASS) >> > except Exception as e: >> > dts_logger.exception("Build target setup failed.") >> > @@ -146,6 +149,7 @@ def _run_suites( >> > with possibly only a subset of test cases. >> > If no subset is specified, run all test cases. >> > """ >> > + end_execution = False >> >> This only ends the build target stage, not the execution stage. We >> should either find a better name for the variable or make sure that >> the whole execution is blocked. I think we're aiming for the latter - >> maybe we could just check whether the last exception in result is a >> BlockingTestSuiteError (or better, just call a new method of result >> that will check that), which we could do at multiple stages. > > > Good catch. When writing this and figuring out how to get it to work, I was thinking about smoke tests on a per build target basis because if it failed on one build target it could theoretically pass on another, but you'd likely want your entire execution to fail than have partial results. > Well, these will be well defined partial results (and definitely valuable, imagine one build target passing and one failing, we could easily compare the two to sometimes quickly identify the failure), so if there's a possibility that the test would pass on another build target, we should try to run the build target. In this case, let's just rename the variable to end_build_target or something similar. >> >> >> > for test_suite_config in execution.test_suites: >> > try: >> > full_suite_path = f"tests.TestSuite_{test_suite_config.test_suite}" >> > @@ -160,13 +164,24 @@ def _run_suites( >> > >> > else: >> > for test_suite_class in test_suite_classes: >> > + #HERE NEEDS CHANGING >> > test_suite = test_suite_class( >> > sut_node, >> > test_suite_config.test_cases, >> > execution.func, >> > build_target_result, >> > + sut_node._build_target_config, >> > + result >> > ) >> > - test_suite.run() >> > + try: >> > + test_suite.run() >> > + except BlockingTestSuiteError as e: >> > + dts_logger.exception("An error occurred within a blocking TestSuite, execution will now end.") >> > + result.add_error(e) >> > + end_execution = True >> > + #if a blocking test failed and we need to bail out of suite executions >> > + if end_execution: >> > + break >> > >> > >> > def _exit_dts() -> None: >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py >> > index ca353d98..4e3f63d1 100644 >> > --- a/dts/framework/exception.py >> > +++ b/dts/framework/exception.py >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): >> > SSH_ERR = 4 >> > DPDK_BUILD_ERR = 10 >> > TESTCASE_VERIFY_ERR = 20 >> > + BLOCKING_TESTSUITE_ERR = 25 >> > >> > >> > class DTSError(Exception): >> > @@ -144,3 +145,13 @@ def __init__(self, value: str): >> > >> > def __str__(self) -> str: >> > return repr(self.value) >> > + >> > +class BlockingTestSuiteError(DTSError): >> > + suite_name: str >> > + severity: ClassVar[ErrorSeverity] = ErrorSeverity.BLOCKING_TESTSUITE_ERR >> > + >> > + def __init__(self, suite_name:str) -> None: >> > + self.suite_name = suite_name >> > + >> > + def __str__(self) -> str: >> > + return f"Blocking suite {self.suite_name} failed." >> > diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py >> > index 4c48ae25..22776bc1 100644 >> > --- a/dts/framework/remote_session/os_session.py >> > +++ b/dts/framework/remote_session/os_session.py >> > @@ -12,7 +12,9 @@ >> > from framework.testbed_model import LogicalCore >> > from framework.utils import EnvVarsDict, MesonArgs >> > >> > -from .remote import CommandResult, RemoteSession, create_remote_session >> > +from .remote import CommandResult, RemoteSession, create_remote_session, create_interactive_session >> > + >> > +from paramiko import SSHClient >> > >> > >> > class OSSession(ABC): >> > @@ -26,6 +28,7 @@ class OSSession(ABC): >> > name: str >> > _logger: DTSLOG >> > remote_session: RemoteSession >> > + _interactive_session: SSHClient >> > >> > def __init__( >> > self, >> > @@ -37,6 +40,7 @@ def __init__( >> > self.name = name >> > self._logger = logger >> > self.remote_session = create_remote_session(node_config, name, logger) >> > + self._interactive_session = create_interactive_session(node_config, name, logger) >> >> This session should be stored in SutNode (it should be os-agnostic >> (the apps should behave the same on all os's, right?) and SutNode >> could use it without accessing another object), but created in >> OSSession (this way we make sure any os-specific inputs (such as >> paths) are passed properly). > > > I can move it into SutNode, the main reason I left it there is because I was essentially thinking of it the same way as the remote session that gets created once per node. However, I could just as easily have SutNode store it and call upon OSSession to create one inside its constructor. This would make creating classes for DPDK apps easier as well once those are subclasses of the InteractiveScriptHandler. > I was thinking of this interactive session more like Node._other_sessions, created only when needed. Now that I think about it, it's always going to be needed if we use it in smoke tests, so we might as well create it in OSSession (or its subclasses if needed). Thinking further, we want to use only the app driver object in test suites. So the SutNode object should have a method that returns the object and the rest of the implementation needs to be delegated to the rest of the objects in the hierarchy to ensure proper remote OS handling. >> >> > >> > def close(self, force: bool = False) -> None: >> > """ >> > diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py >> > index 8a151221..abca8edc 100644 >> > --- a/dts/framework/remote_session/remote/__init__.py >> > +++ b/dts/framework/remote_session/remote/__init__.py >> > @@ -9,8 +9,36 @@ >> > from .remote_session import CommandResult, RemoteSession >> > from .ssh_session import SSHSession >> > >> > +from paramiko import SSHClient, AutoAddPolicy >> > +from framework.utils import GREEN >> > >> > def create_remote_session( >> > node_config: NodeConfiguration, name: str, logger: DTSLOG >> > ) -> RemoteSession: >> > return SSHSession(node_config, name, logger) >> > + >> > +def create_interactive_session( >> > + node_config: NodeConfiguration, name: str, logger: DTSLOG >> > +) -> SSHClient: >> > + """ >> > + Creates a paramiko SSH session that is designed to be used for interactive shells >> > + >> > + This session is meant to be used on an "as needed" basis and may never be utilized >> > + """ >> > + client: SSHClient = SSHClient() >> > + client.set_missing_host_key_policy(AutoAddPolicy) >> > + ip: str = node_config.hostname >> > + logger.info(GREEN(f"Connecting to host {ip}")) >> >> Using colors is a remnant from the original DTS. If we want to >> (re-)introduce colors I'd do that in a separate patch in a uniform >> manner. > > > I agree this is likely outside the scope of this patch and the color here really isn't necessary either so I'll remove it. > >> >> >> > + #Preset to 22 because paramiko doesn't accept None >> > + port: int = 22 >> >> This configuration handling should be moved to NodeConfiguration. >> The logic should also be moved to InteractiveScriptHandler. We also >> probably don't need a factory for this. > > > I agree that the configuration could be moved into NodeConfiguration, however the reason I use a factory and don't have this connect logic inside InteractiveScriptHandler is because that would mean there would have to be an SSH session per application. This is one potential solution, but what I am doing here instead is creating one SSH session for interactive scripts at the start alongside the usual remote session. This session essentially exists in the background for the duration of the execution and then each application for DPDK creates a channel off that session when they are created. Then, when you are done using the application, it closes the channel but the session itself stays open. > Ok, I see. What do you think about this: Let's then move the session creation into a subclass of RemoteSession. The session would be stored as a class variable and with that we could subclass further: RemoteSession -> new subclass -> InteractiveScriptHandler -> DpdkApp Everything could be handled from within the DdpdkApp objects, but we'd need to be careful about cleanup (maybe we'd only need to check that there are no open channels when closing the session). Or alternatively, the new subclass could just have a method that returns DpdkApps. We'd have two different class relations: RemoteSession -> new subclass and InteractiveScriptHandler -> DpdkApp. I'd put all of the classes into the remote_session directory. >> >> >> > + if ":" in node_config.hostname: >> > + ip, port = node_config.hostname.split(":") >> > + port = int(port) >> > + client.connect( >> > + ip, >> > + username=node_config.user, >> > + port=port, >> > + password=node_config.password or "", >> > + timeout=20 if port else 10 >> > + ) >> > + return client >> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py >> > index 74391982..77202ae2 100644 >> > --- a/dts/framework/test_result.py >> > +++ b/dts/framework/test_result.py >> > @@ -8,6 +8,7 @@ >> > import os.path >> > from collections.abc import MutableSequence >> > from enum import Enum, auto >> > +from typing import Dict >> > >> > from .config import ( >> > OS, >> > @@ -67,12 +68,13 @@ class Statistics(dict): >> > Using a dict provides a convenient way to format the data. >> > """ >> > >> > - def __init__(self, dpdk_version): >> > + def __init__(self, output_info: Dict[str, str] | None): >> > super(Statistics, self).__init__() >> > for result in Result: >> > self[result.name] = 0 >> > self["PASS RATE"] = 0.0 >> > - self["DPDK VERSION"] = dpdk_version >> > + if output_info: >> > + for info_key, info_val in output_info.items(): self[info_key] = info_val >> > >> > def __iadd__(self, other: Result) -> "Statistics": >> > """ >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): >> > """ >> > >> > dpdk_version: str | None >> > + output: dict | None >> > _logger: DTSLOG >> > _errors: list[Exception] >> > _return_code: ErrorSeverity >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): >> > def __init__(self, logger: DTSLOG): >> > super(DTSResult, self).__init__() >> > self.dpdk_version = None >> > + self.output = None >> > self._logger = logger >> > self._errors = [] >> > self._return_code = ErrorSeverity.NO_ERR >> > @@ -296,7 +300,10 @@ def process(self) -> None: >> > for error in self._errors: >> > self._logger.debug(repr(error)) >> > >> > - self._stats_result = Statistics(self.dpdk_version) >> > + self._stats_result = Statistics(self.output) >> > + #add information gathered from the smoke tests to the statistics >> > + # for info_key, info_val in smoke_test_info.items(): self._stats_result[info_key] = info_val >> > + # print(self._stats_result) >> > self.add_stats(self._stats_result) >> > with open(self._stats_filename, "w+") as stats_file: >> > stats_file.write(str(self._stats_result)) >> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py >> > index 0705f38f..1518fb8a 100644 >> > --- a/dts/framework/test_suite.py >> > +++ b/dts/framework/test_suite.py >> > @@ -10,11 +10,14 @@ >> > import inspect >> > import re >> > from types import MethodType >> > +from typing import Dict >> > >> > -from .exception import ConfigurationError, SSHTimeoutError, TestCaseVerifyError >> > +from .config import BuildTargetConfiguration >> > + >> > +from .exception import BlockingTestSuiteError, ConfigurationError, SSHTimeoutError, TestCaseVerifyError >> > from .logger import DTSLOG, getLogger >> > from .settings import SETTINGS >> > -from .test_result import BuildTargetResult, Result, TestCaseResult, TestSuiteResult >> > +from .test_result import BuildTargetResult, DTSResult, Result, TestCaseResult, TestSuiteResult >> > from .testbed_model import SutNode >> > >> > >> > @@ -37,10 +40,12 @@ class TestSuite(object): >> > """ >> > >> > sut_node: SutNode >> > + is_blocking = False >> > _logger: DTSLOG >> > _test_cases_to_run: list[str] >> > _func: bool >> > _result: TestSuiteResult >> > + _dts_result: DTSResult >> > >> > def __init__( >> > self, >> > @@ -48,6 +53,8 @@ def __init__( >> > test_cases: list[str], >> > func: bool, >> > build_target_result: BuildTargetResult, >> > + build_target_conf: BuildTargetConfiguration, >> > + dts_result: DTSResult >> > ): >> > self.sut_node = sut_node >> > self._logger = getLogger(self.__class__.__name__) >> > @@ -55,6 +62,8 @@ def __init__( >> > self._test_cases_to_run.extend(SETTINGS.test_cases) >> > self._func = func >> > self._result = build_target_result.add_test_suite(self.__class__.__name__) >> > + self.build_target_info = build_target_conf >> > + self._dts_result = dts_result >> > >> > def set_up_suite(self) -> None: >> > """ >> > @@ -118,6 +127,9 @@ def run(self) -> None: >> > f"the next test suite may be affected." >> > ) >> > self._result.update_setup(Result.ERROR, e) >> > + if len(self._result.get_errors()) > 0 and self.is_blocking: >> > + raise BlockingTestSuiteError(test_suite_name) >> > + >> > >> > def _execute_test_suite(self) -> None: >> > """ >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: >> > f"Attempt number {attempt_nr} out of {all_attempts}." >> > ) >> > self._run_test_case(test_case_method, test_case_result) >> > + >> > >> > def _get_functional_test_cases(self) -> list[MethodType]: >> > """ >> > @@ -232,6 +245,11 @@ def _execute_test_case( >> > test_case_result.update(Result.SKIP) >> > raise KeyboardInterrupt("Stop DTS") >> > >> > + def write_to_statistics_file(self, output: Dict[str, str]): >> > + if self._dts_result.output != None: >> > + self._dts_result.output.update(output) >> > + else: >> > + self._dts_result.output = output >> > >> > def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]]: >> > def is_test_suite(object) -> bool: >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: >> > test_suite_class >> > for _, test_suite_class in inspect.getmembers(testcase_module, is_test_suite) >> > ] >> > + >> > + >> > diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py >> > index f54a9470..63f17cc3 100644 >> > --- a/dts/framework/testbed_model/__init__.py >> > +++ b/dts/framework/testbed_model/__init__.py >> > @@ -20,3 +20,8 @@ >> > ) >> > from .node import Node >> > from .sut_node import SutNode >> > + >> > +from .interactive_apps import ( >> > + InteractiveScriptHandler, >> > + TestpmdDriver >> > +) >> > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py >> > new file mode 100644 >> > index 00000000..0382d7e0 >> > --- /dev/null >> > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py >> > @@ -0,0 +1,6 @@ >> > +from .interactive_command import ( >> > + InteractiveScriptHandler >> > +) >> > +from .testpmd_driver import ( >> > + TestpmdDriver >> > +) >> > \ No newline at end of file >> > diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py >> > new file mode 100644 >> > index 00000000..7467911b >> > --- /dev/null >> > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py >> >> In general, the file name should match the class name. >> >> > @@ -0,0 +1,57 @@ >> > +# import paramiko >> > +from paramiko import SSHClient, Channel, channel >> > +from framework.settings import SETTINGS >> > + >> > +class InteractiveScriptHandler: >> >> Once merged with the init functionality and moved to remote_session, >> I'd rename it to InteractiveAppSession or something similar. > > > Good point, I'll rename the class and the file. > >> >> >> > + >> > + _ssh_client: SSHClient >> > + _stdin: channel.ChannelStdinFile >> > + _ssh_channel: Channel >> > + >> > + def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None: >> > + self._ssh_client = ssh_client >> > + self._ssh_channel = self._ssh_client.invoke_shell() >> > + self._stdin = self._ssh_channel.makefile_stdin("wb") >> > + self._ssh_channel.settimeout(timeout) >> > + >> > + def send_command(self, command:str) -> None: >> > + """ >> > + Send command to channel without recording output. >> > + >> > + This method will not verify any input or output, it will >> > + simply assume the command succeeded >> > + """ >> > + self._stdin.write(command + '\n') >> > + self._stdin.flush() >> > + >> > + def send_command_get_output(self, command:str, expect:str) -> str: >> >> Let's rename expect to prompt. At least for me it was just confusing >> in the original DTS. > > > It definitely can be confusing, I'll change it accordingly as you're right, prompt is more clear. > >> >> >> > + """ >> > + Send a command and get all output before the expected ending string. >> > + >> > + **NOTE** >> > + Lines that expect input are not included in the stdout buffer so they cannot be >> > + used for expect. For example, if you were prompted to log into something >> > + with a username and password, you cannot expect "username:" because it wont >> > + yet be in the stdout buffer. A work around for this could be consuming an >> > + extra newline character to force the current prompt into the stdout buffer. >> > + >> > + *Return* >> > + All output before expected string >> > + """ >> > + stdout = self._ssh_channel.makefile("r") >> > + self._stdin.write(command + '\n') >> > + self._stdin.flush() >> > + out:str = "" >> > + for line in stdout: >> > + out += str(line) >> > + if expect in str(line): >> > + break >> > + stdout.close() #close the buffer to flush the output >> > + return out >> > + >> > + def close(self): >> > + self._stdin.close() >> > + self._ssh_channel.close() >> > + >> > + def __del__(self): >> > + self.close() >> > diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> > new file mode 100644 >> > index 00000000..1993eae6 >> > --- /dev/null >> > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> > @@ -0,0 +1,24 @@ >> > +from framework.testbed_model.interactive_apps import InteractiveScriptHandler >> > + >> > +from pathlib import PurePath >> > + >> > +class TestpmdDriver: >> >> This could also be called TestPmdSession. Not sure which is better. >> This should extend InteractiveScriptHandler, which should contain >> basic functionality common to all apps and then the drivers would >> contain app-specific functionality. > > > Originally the reason I avoided doing this was because I viewed them as separate layers of the process essentially. I wrote it in a way where the InteractiveScriptHandler was almost like an interface for any CLI and then DPDK apps could interact with this interface, but after thinking about it more, this isn't necessary. There is no reason really that the applications themselves can't just use these methods directly and it also avoids the need to have a reference to the handler within the object. > >> >> >> > + prompt:str = "testpmd>" >> > + interactive_handler: InteractiveScriptHandler >> > + >> > + def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None: >> > + """ >> > + Sets the handler to drive the SSH session and starts testpmd >> > + """ >> > + self.interactive_handler = handler >> > + # self.interactive_handler.send_command("sudo su") >> > + # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build") >> > + self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt) >> >> The paths need to be handled in os-agnostic manner in os_session and >> then passed here. >> >> > + >> > + def send_command(self, command:str, expect:str = prompt) -> str: >> > + """ >> > + Specific way of handling the command for testpmd >> > + >> > + An extra newline character is consumed in order to force the current line into the stdout buffer >> > + """ >> > + return self.interactive_handler.send_command_get_output(command + "\n", expect) >> > \ No newline at end of file >> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py >> > index d48fafe6..c5147e0e 100644 >> > --- a/dts/framework/testbed_model/node.py >> > +++ b/dts/framework/testbed_model/node.py >> > @@ -40,6 +40,7 @@ class Node(object): >> > lcores: list[LogicalCore] >> > _logger: DTSLOG >> > _other_sessions: list[OSSession] >> > + _execution_config: ExecutionConfiguration >> > >> > def __init__(self, node_config: NodeConfiguration): >> > self.config = node_config >> > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: >> > """ >> > self._setup_hugepages() >> > self._set_up_execution(execution_config) >> > + self._execution_config = execution_config >> > >> > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: >> > """ >> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py >> > index 2b2b50d9..8c39a66d 100644 >> > --- a/dts/framework/testbed_model/sut_node.py >> > +++ b/dts/framework/testbed_model/sut_node.py >> > @@ -14,6 +14,7 @@ >> > >> > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice >> > from .node import Node >> > +from .interactive_apps import InteractiveScriptHandler >> > >> > >> > class SutNode(Node): >> > @@ -261,6 +262,11 @@ def run_dpdk_app( >> > return self.main_session.send_command( >> > f"{app_path} {eal_args}", timeout, verify=True >> > ) >> > + def create_interactive_session_handler(self) -> InteractiveScriptHandler: >> > + """ >> > + Create a handler for interactive sessions >> > + """ >> > + return InteractiveScriptHandler(self.main_session._interactive_session) >> > >> > >> > class EalParameters(object): >> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py >> > new file mode 100644 >> > index 00000000..bacf289d >> > --- /dev/null >> > +++ b/dts/tests/TestSuite_smoke_tests.py >> > @@ -0,0 +1,94 @@ >> > +from framework.test_suite import TestSuite >> > +from framework.testbed_model.sut_node import SutNode >> > + >> > +from framework.testbed_model.interactive_apps import TestpmdDriver >> > + >> > +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> str: >> >> I don't see a reason why this is outside SmokeTest. >> >> >> > + match compiler_name: >> > + case "gcc": >> > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] >> >> As I alluded to earlier, the call here should be >> sut_node.get_compiler_version(). This is to hide implementation >> details from test suite developers. >> Then, SutNode.get_compiler_version should then call >> self.main_session.get_compiler_version(). The reason here is this is >> an os-agnostic call. >> get_compiler_version() should then be defined in os_session and >> implemented in os specific classes. > > > I originally placed the method outside of smoke tests just so it wouldn't be run with the rest of the suite but it does make sense to make this and the other comments os-agnostic. > >> >> >> > + case "clang": >> > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] >> > + case "msvc": >> > + return sut_node.main_session.send_command(f"cl", 60).stdout >> > + case "icc": >> > + return sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout >> > + >> > +class SmokeTests(TestSuite): >> > + is_blocking = True >> > + >> > + def set_up_suite(self) -> None: >> > + """ >> > + Setup: >> > + build all DPDK >> > + """ >> > + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir >> > + >> > + >> > + def test_unit_tests(self) -> None: >> > + """ >> > + Test: >> > + run the fast-test unit-test suite through meson >> > + """ >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests", 300) >> >> Same here, there already are methods that build dpdk. If anything >> needs to be added, we should expand those methods. > > > I don't think this is building DPDK, this is just running fast tests and ensuring that you are in the correct directory. > Ah, right. These would be probably used only in smoke tests, so it's probably fine to leave them here. Maybe we could add a method to the suite? I'm thinking it would make the code a bit more readable. >> >> >> > + >> > + def test_driver_tests(self) -> None: >> > + """ >> > + Test: >> > + run the driver-test unit-test suite through meson >> > + """ >> > + list_of_vdevs = "" >> > + for dev in self.sut_node._execution_config.vdevs: >> > + list_of_vdevs += f"{dev}," >> > + print(list_of_vdevs) >> > + if len(list_of_vdevs) > 0: >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev {list_of_vdevs}\"", 300) >> > + else: >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests", 300) >> > + >> > + def test_gather_info(self) -> None: >> > + """ >> > + Test: >> > + gather information about the system and send output to statistics.txt >> > + """ >> > + out = {} >> > + >> > + out['OS'] = self.sut_node.main_session.send_command("awk -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout >> > + out["OS VERSION"] = self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print $2}' /etc/os-release", 60, True).stdout >> > + out["COMPILER VERSION"] = get_compiler_version(self.build_target_info.compiler.name, self.sut_node) >> > + out["DPDK VERSION"] = self.sut_node.dpdk_version >> > + if self.build_target_info.os.name == "linux": >> > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -r", 60).stdout >> > + elif self.build_target_info.os.name == "windows": >> > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -a", 60).stdout >> > + self.write_to_statistics_file(out) >> > + >> >> This is not really a test. If we want to add this, it should be stored >> elsewhere (in their respective objects, probably in result objects). >> Three of these (os, os and kernel versions) are node data, the >> remaining two are build target data. >> I'm not sure it's a good idea to put these into statistics, as the >> statistics are aggregated over all executions and build targets. In >> case of multiple SUTs (in different executions) and multiple build >> targets we'd record misleading data. We could include data from all >> build targets and SUTs though. >> The reason we (and original DTS) are storing DPDK version in the >> current manner is that that doesn't change across anything, we're >> always testing the same tarball. > > > You're right that this isn't really testing anything, I had originally included it here because it was just part of the smoke test outline. It's definitely out of place though and I can move it out to their respective classes. I think it might make sense to organize/label data based on the SUT it comes from, I just thought statistics made the most sense because you could see the test statistics as well as what it was testing in one place. > We could put anything we want into statistics (it's not a good idea now, but if we change the format of statistics it'd be fine, but I think changing the format is out of the scope of this patch) and this would make sense there, but the first step is properly storing the data. Moving them to statistics would be trivial then. >> >> >> > + >> > + >> > + def test_start_testpmd(self) -> None: >> > + """ >> > + Creates and instance of the testpmd driver to run the testpmd app >> > + """ >> > + driver: TestpmdDriver = TestpmdDriver(self.sut_node.create_interactive_session_handler(), self.dpdk_build_dir_path) >> >> The driver should be returned by a method of self.sut_node. > > > Does it make more sense to have methods for creating handlers for various DPDK apps in SutNode? I had assumed it would be cleaner to just make the classes as needed so that you don't have multiple methods doing something very similar in SutNode (basically just making a class with a different name that takes in similar information). I suppose that if you were trying to have developers only interact with the SutNode class this makes sense. > That's what I meant. The method would return the appropriate app based on input (which could be an enum) - I think that's preferable to having a method for each app. We should think about non-interactive apps here as well (such as the helloworld app that's already in use). > This is also something that could make good use of having the SSHClient in the SutNode and I could see how it would be useful. > >> >> >> > + >> > + print(driver.send_command("show port summary all")) >> > + >> > + def test_device_bound_to_driver(self) -> None: >> > + """ >> > + Test: >> > + ensure that all drivers listed in the config are bound to the correct drivers >> > + """ >> > + for nic in self.sut_node._execution_config.nics: >> > + for address in nic.addresses: >> > + out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60) >> >> This should follow the same abstractions as I outlined above. >> However, we don't need to use dpdk-devbind here. It's probably safe to >> do so (the script is likely not going to be changed), but we could > > > That's a good point about dev-bind, it might be better here to just avoid the reliance on another script. > >> >> >> > + self.verify( >> > + len(out.stdout) != 0, >> > + f"Failed to find configured device ({address}) using dpdk-devbind.py", >> > + ) >> > + for string in out.stdout.split(" "): >> > + if 'drv=' in string: >> > + self.verify( >> > + string.split("=")[1] == nic.driver.strip(), >> > + f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})', >> > + ) >> > + >> > \ No newline at end of file >> > -- >> > 2.40.1 >> > > > > I'll work on these changes, but I'd like to hear what you think about what I had mentioned about moving the connect logic to the InteractiveScriptHandler in the comments above. I had originally written it to use one session throughout rather than opening and closing SSH connections for every application but I'd like to hear which you think would be easier/better if there is a difference. Yes, let's do one session with each app having its own channel. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-25 8:33 ` Juraj Linkeš @ 2023-05-25 18:02 ` Jeremy Spewock 2023-05-26 7:36 ` Juraj Linkeš 0 siblings, 1 reply; 12+ messages in thread From: Jeremy Spewock @ 2023-05-25 18:02 UTC (permalink / raw) To: Juraj Linkeš; +Cc: dev [-- Attachment #1: Type: text/plain, Size: 46547 bytes --] Hey Juraj, On Thu, May 25, 2023 at 4:33 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > One more point that doesn't fit elsewhere: > > On Wed, May 24, 2023 at 10:45 PM Jeremy Spewock <jspewock@iol.unh.edu> > wrote: > > > > > > > > On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> > wrote: > >> > >> Hi Jeremy, first, a few general points: > >> > >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and > >> anyone else involved with DTS or who might be interested) and add the > >> devlist to cc. > > > > > > Thank you for the tip! I'm still new to sending patches and didn't think > to do something like this but I will in the future. > > > >> > >> 2. Run the linter script before submitting. > > > > > > I did forget to run this, I will in the future. > > > >> > >> 3. The use of the various nested objects breaks the current > >> abstractions. The basic idea is that the test suite developers should > >> ideally only use the sut/tg node objects and those objects should > >> delegate logic further to their nested objects. More below. > >> > >> I have many comments about the implementation, but I haven't run it > >> yet. I'm going to do that after this round of comments and I may have > >> more ideas. > >> > >> On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: > >> > > >> > From: Jeremy Spewock <jspewock@iol.unh.edu> > >> > > >> > Adds a new test suite for running smoke tests that verify general > >> > configuration aspects of the system under test. If any of these tests > >> > fail, the DTS execution terminates as part of a "fail-fast" model. > >> > > >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> > >> > --- > >> > dts/conf.yaml | 9 ++ > >> > dts/framework/config/__init__.py | 21 +++++ > >> > dts/framework/config/conf_yaml_schema.json | 32 ++++++- > >> > dts/framework/dts.py | 19 +++- > >> > dts/framework/exception.py | 11 +++ > >> > dts/framework/remote_session/os_session.py | 6 +- > >> > .../remote_session/remote/__init__.py | 28 ++++++ > >> > dts/framework/test_result.py | 13 ++- > >> > dts/framework/test_suite.py | 24 ++++- > >> > dts/framework/testbed_model/__init__.py | 5 + > >> > .../interactive_apps/__init__.py | 6 ++ > >> > .../interactive_apps/interactive_command.py | 57 +++++++++++ > >> > .../interactive_apps/testpmd_driver.py | 24 +++++ > >> > dts/framework/testbed_model/node.py | 2 + > >> > dts/framework/testbed_model/sut_node.py | 6 ++ > >> > dts/tests/TestSuite_smoke_tests.py | 94 > +++++++++++++++++++ > >> > 16 files changed, 348 insertions(+), 9 deletions(-) > >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/__init__.py > >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/interactive_command.py > >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> > >> Let's not add any more levels. I don't like even the current hw > >> subdirectory (which I want to remove in the docs patch) and we don't > >> need it. I'd also like to move this functionality into remote_session, > >> as it's handling a type of remote session. > > > > > > I think it makes sense to add a proper wrapper for it, I just didn't > create a subclass for it off of remote_session because the idea behind it > was only allowing users to interact with the session through the > InteractiveScriptHandler/DPDK app handlers. If it were part of the > remote_session class it would have to include operations for sending > commands the way it is written now. I could do this but it seems like a > bigger discussion about whether interactive sessions should create a new > SSH session every time or instead just use one existing session and create > channels off of it. > > > > I wasn't clear, I meant to move the python modules into the > remote_session folder. The subclassing would be there only to have a > common API across these remote sessions (as the > InteractiveScriptHandler is a kind of a remote session). If at all > possible, this should be our aim. > I see what you mean now, moving them there shouldn't be a problem. > > >> > >> > >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py > >> > > >> > diff --git a/dts/conf.yaml b/dts/conf.yaml > >> > index a9bd8a3e..042ef954 100644 > >> > --- a/dts/conf.yaml > >> > +++ b/dts/conf.yaml > >> > @@ -10,13 +10,22 @@ executions: > >> > compiler_wrapper: ccache > >> > perf: false > >> > func: true > >> > + nics: #physical devices to be used for testing > >> > + - addresses: > >> > + - "0000:11:00.0" > >> > + - "0000:11:00.1" > >> > + driver: "i40e" > >> > + vdevs: #names of virtual devices to be used for testing > >> > + - "crypto_openssl" > >> > >> I believe we specified the NICs under SUTs in the original DTS, just > >> as Owen did in his internal GitLab patch. If you can access it, have a > >> look at how he did it. > >> This brings an interesting question of where we want to specify which > >> NICs/vdevs to test. It could be on the SUT level, but also on the > >> execution or even the build target level. This should be informed by > >> testing needs. What makes the most sense? We could specify NIC details > >> per SUT/TG and then just reference the NICs on the execution/build > >> target level. > > > > > > I put it on the execution level with the thought that you might have > multiple NICs in your SUT but want to separate them into different > executions. > > This is a good point. Is this something you're interested in in the > lab? We should ask Lijuan what she thinks of this. In general, I like > this, so we should at least think of how to add NIC config > implementation so that we could add this later if we're not adding it > now. > > > I guess in the case of smoke tests, you'd only need to know them per > target because it was talked about previously that then is when we should > run the smoke tests. I think however it would make sense to specify the NIC > you are using for that execution rather than having to assume or specify > elsewhere. > > > > Nothing is set in stone, we don't have to run them per build target. > We could have two smoke test suites, one per execution and one per > build target. I actually like that a lot and we should explore that. > If it's not much extra work, we could just do the split like that. > > I also like the sound of that. In theory it doesn't seem that hard, if we made two different suites that tested different things all that needs to change is when they are run. This would probably take some more planning as to what we want to run at which points during the run and it might be easier to get these tests in a better spot first. I do like the idea though, and agree that it's worth exploring. >> > >> > >> > test_suites: > >> > + - smoke_tests > >> > - hello_world > >> > system_under_test: "SUT 1" > >> > nodes: > >> > - name: "SUT 1" > >> > hostname: sut1.change.me.localhost > >> > user: root > >> > + password: "" > >> > >> This was deliberately left out to discourage the use of passwords. > >> > >> > arch: x86_64 > >> > os: linux > >> > lcores: "" > >> > >> <snip> > >> > >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py > >> > index 05022845..0d03e158 100644 > >> > --- a/dts/framework/dts.py > >> > +++ b/dts/framework/dts.py > >> > @@ -5,6 +5,8 @@ > >> > > >> > import sys > >> > > >> > +from .exception import BlockingTestSuiteError > >> > + > >> > from .config import CONFIGURATION, BuildTargetConfiguration, > ExecutionConfiguration > >> > from .logger import DTSLOG, getLogger > >> > from .test_result import BuildTargetResult, DTSResult, > ExecutionResult, Result > >> > @@ -49,6 +51,7 @@ def run_all() -> None: > >> > nodes[sut_node.name] = sut_node > >> > > >> > if sut_node: > >> > + #SMOKE TEST EXECUTION GOES HERE! > >> > _run_execution(sut_node, execution, result) > >> > > >> > except Exception as e: > >> > @@ -118,7 +121,7 @@ def _run_build_target( > >> > > >> > try: > >> > sut_node.set_up_build_target(build_target) > >> > - result.dpdk_version = sut_node.dpdk_version > >> > + # result.dpdk_version = sut_node.dpdk_version > >> > build_target_result.update_setup(Result.PASS) > >> > except Exception as e: > >> > dts_logger.exception("Build target setup failed.") > >> > @@ -146,6 +149,7 @@ def _run_suites( > >> > with possibly only a subset of test cases. > >> > If no subset is specified, run all test cases. > >> > """ > >> > + end_execution = False > >> > >> This only ends the build target stage, not the execution stage. We > >> should either find a better name for the variable or make sure that > >> the whole execution is blocked. I think we're aiming for the latter - > >> maybe we could just check whether the last exception in result is a > >> BlockingTestSuiteError (or better, just call a new method of result > >> that will check that), which we could do at multiple stages. > > > > > > Good catch. When writing this and figuring out how to get it to work, I > was thinking about smoke tests on a per build target basis because if it > failed on one build target it could theoretically pass on another, but > you'd likely want your entire execution to fail than have partial results. > > > > Well, these will be well defined partial results (and definitely > valuable, imagine one build target passing and one failing, we could > easily compare the two to sometimes quickly identify the failure), so > if there's a possibility that the test would pass on another build > target, we should try to run the build target. > > In this case, let's just rename the variable to end_build_target or > something similar. > > Alright, will do. > >> > >> > >> > for test_suite_config in execution.test_suites: > >> > try: > >> > full_suite_path = > f"tests.TestSuite_{test_suite_config.test_suite}" > >> > @@ -160,13 +164,24 @@ def _run_suites( > >> > > >> > else: > >> > for test_suite_class in test_suite_classes: > >> > + #HERE NEEDS CHANGING > >> > test_suite = test_suite_class( > >> > sut_node, > >> > test_suite_config.test_cases, > >> > execution.func, > >> > build_target_result, > >> > + sut_node._build_target_config, > >> > + result > >> > ) > >> > - test_suite.run() > >> > + try: > >> > + test_suite.run() > >> > + except BlockingTestSuiteError as e: > >> > + dts_logger.exception("An error occurred within a > blocking TestSuite, execution will now end.") > >> > + result.add_error(e) > >> > + end_execution = True > >> > + #if a blocking test failed and we need to bail out of suite > executions > >> > + if end_execution: > >> > + break > >> > > >> > > >> > def _exit_dts() -> None: > >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > >> > index ca353d98..4e3f63d1 100644 > >> > --- a/dts/framework/exception.py > >> > +++ b/dts/framework/exception.py > >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): > >> > SSH_ERR = 4 > >> > DPDK_BUILD_ERR = 10 > >> > TESTCASE_VERIFY_ERR = 20 > >> > + BLOCKING_TESTSUITE_ERR = 25 > >> > > >> > > >> > class DTSError(Exception): > >> > @@ -144,3 +145,13 @@ def __init__(self, value: str): > >> > > >> > def __str__(self) -> str: > >> > return repr(self.value) > >> > + > >> > +class BlockingTestSuiteError(DTSError): > >> > + suite_name: str > >> > + severity: ClassVar[ErrorSeverity] = > ErrorSeverity.BLOCKING_TESTSUITE_ERR > >> > + > >> > + def __init__(self, suite_name:str) -> None: > >> > + self.suite_name = suite_name > >> > + > >> > + def __str__(self) -> str: > >> > + return f"Blocking suite {self.suite_name} failed." > >> > diff --git a/dts/framework/remote_session/os_session.py > b/dts/framework/remote_session/os_session.py > >> > index 4c48ae25..22776bc1 100644 > >> > --- a/dts/framework/remote_session/os_session.py > >> > +++ b/dts/framework/remote_session/os_session.py > >> > @@ -12,7 +12,9 @@ > >> > from framework.testbed_model import LogicalCore > >> > from framework.utils import EnvVarsDict, MesonArgs > >> > > >> > -from .remote import CommandResult, RemoteSession, > create_remote_session > >> > +from .remote import CommandResult, RemoteSession, > create_remote_session, create_interactive_session > >> > + > >> > +from paramiko import SSHClient > >> > > >> > > >> > class OSSession(ABC): > >> > @@ -26,6 +28,7 @@ class OSSession(ABC): > >> > name: str > >> > _logger: DTSLOG > >> > remote_session: RemoteSession > >> > + _interactive_session: SSHClient > >> > > >> > def __init__( > >> > self, > >> > @@ -37,6 +40,7 @@ def __init__( > >> > self.name = name > >> > self._logger = logger > >> > self.remote_session = create_remote_session(node_config, > name, logger) > >> > + self._interactive_session = > create_interactive_session(node_config, name, logger) > >> > >> This session should be stored in SutNode (it should be os-agnostic > >> (the apps should behave the same on all os's, right?) and SutNode > >> could use it without accessing another object), but created in > >> OSSession (this way we make sure any os-specific inputs (such as > >> paths) are passed properly). > > > > > > I can move it into SutNode, the main reason I left it there is because I > was essentially thinking of it the same way as the remote session that gets > created once per node. However, I could just as easily have SutNode store > it and call upon OSSession to create one inside its constructor. This would > make creating classes for DPDK apps easier as well once those are > subclasses of the InteractiveScriptHandler. > > > > I was thinking of this interactive session more like > Node._other_sessions, created only when needed. Now that I think about > it, it's always going to be needed if we use it in smoke tests, so we > might as well create it in OSSession (or its subclasses if needed). > > Thinking further, we want to use only the app driver object in test > suites. So the SutNode object should have a method that returns the > object and the rest of the implementation needs to be delegated to the > rest of the objects in the hierarchy to ensure proper remote OS > handling. > I like the sound of this. If we use a method and something like an enum inside of SutNode as well to create these objects then we don't even really need a method to return this object because it would never be interacted with directly. > > >> > >> > > >> > def close(self, force: bool = False) -> None: > >> > """ > >> > diff --git a/dts/framework/remote_session/remote/__init__.py > b/dts/framework/remote_session/remote/__init__.py > >> > index 8a151221..abca8edc 100644 > >> > --- a/dts/framework/remote_session/remote/__init__.py > >> > +++ b/dts/framework/remote_session/remote/__init__.py > >> > @@ -9,8 +9,36 @@ > >> > from .remote_session import CommandResult, RemoteSession > >> > from .ssh_session import SSHSession > >> > > >> > +from paramiko import SSHClient, AutoAddPolicy > >> > +from framework.utils import GREEN > >> > > >> > def create_remote_session( > >> > node_config: NodeConfiguration, name: str, logger: DTSLOG > >> > ) -> RemoteSession: > >> > return SSHSession(node_config, name, logger) > >> > + > >> > +def create_interactive_session( > >> > + node_config: NodeConfiguration, name: str, logger: DTSLOG > >> > +) -> SSHClient: > >> > + """ > >> > + Creates a paramiko SSH session that is designed to be used for > interactive shells > >> > + > >> > + This session is meant to be used on an "as needed" basis and may > never be utilized > >> > + """ > >> > + client: SSHClient = SSHClient() > >> > + client.set_missing_host_key_policy(AutoAddPolicy) > >> > + ip: str = node_config.hostname > >> > + logger.info(GREEN(f"Connecting to host {ip}")) > >> > >> Using colors is a remnant from the original DTS. If we want to > >> (re-)introduce colors I'd do that in a separate patch in a uniform > >> manner. > > > > > > I agree this is likely outside the scope of this patch and the color > here really isn't necessary either so I'll remove it. > > > >> > >> > >> > + #Preset to 22 because paramiko doesn't accept None > >> > + port: int = 22 > >> > >> This configuration handling should be moved to NodeConfiguration. > >> The logic should also be moved to InteractiveScriptHandler. We also > >> probably don't need a factory for this. > > > > > > I agree that the configuration could be moved into NodeConfiguration, > however the reason I use a factory and don't have this connect logic inside > InteractiveScriptHandler is because that would mean there would have to be > an SSH session per application. This is one potential solution, but what I > am doing here instead is creating one SSH session for interactive scripts > at the start alongside the usual remote session. This session essentially > exists in the background for the duration of the execution and then each > application for DPDK creates a channel off that session when they are > created. Then, when you are done using the application, it closes the > channel but the session itself stays open. > > > > Ok, I see. What do you think about this: > Let's then move the session creation into a subclass of RemoteSession. > The session would be stored as a class variable and with that we could > subclass further: > RemoteSession -> new subclass -> InteractiveScriptHandler -> DpdkApp > > Everything could be handled from within the DdpdkApp objects, but we'd > need to be careful about cleanup (maybe we'd only need to check that > there are no open channels when closing the session). > > Or alternatively, the new subclass could just have a method that > returns DpdkApps. We'd have two different class relations: > RemoteSession -> new subclass and InteractiveScriptHandler -> DpdkApp. > > I'd put all of the classes into the remote_session directory. > > I like the idea of making a subclass of the remote session and then a method in the subclass that generates DpdkApps. This way you have a proper class that maintains the interactive session and InteractiveScriptHandlers as well as any other DpdkApp can be created and destroyed as needed. The only reason I wouldn't make InteractiveScriptHandler a subclass of RemoteSession is because then it would make the InteractiveScriptHandler something that stored the main SSH session rather than something that just uses it to make a channel. Originally, I avoided this because I didn't want to implement the send_command methods from inside RemoteSession, but a solution to this could just be creating an InteractiveScriptHandler, sending the command blindly, and not returning the output. Or maybe there would be some kind of default prompt I could expect to still collect the output, but I'm not sure there would be something that is os-agnostic for this. I'm not sure the best way to show that I don't want people to use the method on the new subclass that I'm going to create though. I guess one way to handle this would be just not creating a method that returns this subclass in SutNode. SutNode could just have a method that returns DPDK apps which should discourage touching this session directly. > >> > >> > >> > + if ":" in node_config.hostname: > >> > + ip, port = node_config.hostname.split(":") > >> > + port = int(port) > >> > + client.connect( > >> > + ip, > >> > + username=node_config.user, > >> > + port=port, > >> > + password=node_config.password or "", > >> > + timeout=20 if port else 10 > >> > + ) > >> > + return client > >> > diff --git a/dts/framework/test_result.py > b/dts/framework/test_result.py > >> > index 74391982..77202ae2 100644 > >> > --- a/dts/framework/test_result.py > >> > +++ b/dts/framework/test_result.py > >> > @@ -8,6 +8,7 @@ > >> > import os.path > >> > from collections.abc import MutableSequence > >> > from enum import Enum, auto > >> > +from typing import Dict > >> > > >> > from .config import ( > >> > OS, > >> > @@ -67,12 +68,13 @@ class Statistics(dict): > >> > Using a dict provides a convenient way to format the data. > >> > """ > >> > > >> > - def __init__(self, dpdk_version): > >> > + def __init__(self, output_info: Dict[str, str] | None): > >> > super(Statistics, self).__init__() > >> > for result in Result: > >> > self[result.name] = 0 > >> > self["PASS RATE"] = 0.0 > >> > - self["DPDK VERSION"] = dpdk_version > >> > + if output_info: > >> > + for info_key, info_val in output_info.items(): > self[info_key] = info_val > >> > > >> > def __iadd__(self, other: Result) -> "Statistics": > >> > """ > >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): > >> > """ > >> > > >> > dpdk_version: str | None > >> > + output: dict | None > >> > _logger: DTSLOG > >> > _errors: list[Exception] > >> > _return_code: ErrorSeverity > >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): > >> > def __init__(self, logger: DTSLOG): > >> > super(DTSResult, self).__init__() > >> > self.dpdk_version = None > >> > + self.output = None > >> > self._logger = logger > >> > self._errors = [] > >> > self._return_code = ErrorSeverity.NO_ERR > >> > @@ -296,7 +300,10 @@ def process(self) -> None: > >> > for error in self._errors: > >> > self._logger.debug(repr(error)) > >> > > >> > - self._stats_result = Statistics(self.dpdk_version) > >> > + self._stats_result = Statistics(self.output) > >> > + #add information gathered from the smoke tests to the > statistics > >> > + # for info_key, info_val in smoke_test_info.items(): > self._stats_result[info_key] = info_val > >> > + # print(self._stats_result) > >> > self.add_stats(self._stats_result) > >> > with open(self._stats_filename, "w+") as stats_file: > >> > stats_file.write(str(self._stats_result)) > >> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > >> > index 0705f38f..1518fb8a 100644 > >> > --- a/dts/framework/test_suite.py > >> > +++ b/dts/framework/test_suite.py > >> > @@ -10,11 +10,14 @@ > >> > import inspect > >> > import re > >> > from types import MethodType > >> > +from typing import Dict > >> > > >> > -from .exception import ConfigurationError, SSHTimeoutError, > TestCaseVerifyError > >> > +from .config import BuildTargetConfiguration > >> > + > >> > +from .exception import BlockingTestSuiteError, ConfigurationError, > SSHTimeoutError, TestCaseVerifyError > >> > from .logger import DTSLOG, getLogger > >> > from .settings import SETTINGS > >> > -from .test_result import BuildTargetResult, Result, TestCaseResult, > TestSuiteResult > >> > +from .test_result import BuildTargetResult, DTSResult, Result, > TestCaseResult, TestSuiteResult > >> > from .testbed_model import SutNode > >> > > >> > > >> > @@ -37,10 +40,12 @@ class TestSuite(object): > >> > """ > >> > > >> > sut_node: SutNode > >> > + is_blocking = False > >> > _logger: DTSLOG > >> > _test_cases_to_run: list[str] > >> > _func: bool > >> > _result: TestSuiteResult > >> > + _dts_result: DTSResult > >> > > >> > def __init__( > >> > self, > >> > @@ -48,6 +53,8 @@ def __init__( > >> > test_cases: list[str], > >> > func: bool, > >> > build_target_result: BuildTargetResult, > >> > + build_target_conf: BuildTargetConfiguration, > >> > + dts_result: DTSResult > >> > ): > >> > self.sut_node = sut_node > >> > self._logger = getLogger(self.__class__.__name__) > >> > @@ -55,6 +62,8 @@ def __init__( > >> > self._test_cases_to_run.extend(SETTINGS.test_cases) > >> > self._func = func > >> > self._result = > build_target_result.add_test_suite(self.__class__.__name__) > >> > + self.build_target_info = build_target_conf > >> > + self._dts_result = dts_result > >> > > >> > def set_up_suite(self) -> None: > >> > """ > >> > @@ -118,6 +127,9 @@ def run(self) -> None: > >> > f"the next test suite may be affected." > >> > ) > >> > self._result.update_setup(Result.ERROR, e) > >> > + if len(self._result.get_errors()) > 0 and > self.is_blocking: > >> > + raise BlockingTestSuiteError(test_suite_name) > >> > + > >> > > >> > def _execute_test_suite(self) -> None: > >> > """ > >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: > >> > f"Attempt number {attempt_nr} out of > {all_attempts}." > >> > ) > >> > self._run_test_case(test_case_method, > test_case_result) > >> > + > >> > > >> > def _get_functional_test_cases(self) -> list[MethodType]: > >> > """ > >> > @@ -232,6 +245,11 @@ def _execute_test_case( > >> > test_case_result.update(Result.SKIP) > >> > raise KeyboardInterrupt("Stop DTS") > >> > > >> > + def write_to_statistics_file(self, output: Dict[str, str]): > >> > + if self._dts_result.output != None: > >> > + self._dts_result.output.update(output) > >> > + else: > >> > + self._dts_result.output = output > >> > > >> > def get_test_suites(testsuite_module_path: str) -> > list[type[TestSuite]]: > >> > def is_test_suite(object) -> bool: > >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: > >> > test_suite_class > >> > for _, test_suite_class in > inspect.getmembers(testcase_module, is_test_suite) > >> > ] > >> > + > >> > + > >> > diff --git a/dts/framework/testbed_model/__init__.py > b/dts/framework/testbed_model/__init__.py > >> > index f54a9470..63f17cc3 100644 > >> > --- a/dts/framework/testbed_model/__init__.py > >> > +++ b/dts/framework/testbed_model/__init__.py > >> > @@ -20,3 +20,8 @@ > >> > ) > >> > from .node import Node > >> > from .sut_node import SutNode > >> > + > >> > +from .interactive_apps import ( > >> > + InteractiveScriptHandler, > >> > + TestpmdDriver > >> > +) > >> > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py > b/dts/framework/testbed_model/interactive_apps/__init__.py > >> > new file mode 100644 > >> > index 00000000..0382d7e0 > >> > --- /dev/null > >> > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > >> > @@ -0,0 +1,6 @@ > >> > +from .interactive_command import ( > >> > + InteractiveScriptHandler > >> > +) > >> > +from .testpmd_driver import ( > >> > + TestpmdDriver > >> > +) > >> > \ No newline at end of file > >> > diff --git > a/dts/framework/testbed_model/interactive_apps/interactive_command.py > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > >> > new file mode 100644 > >> > index 00000000..7467911b > >> > --- /dev/null > >> > +++ > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > >> > >> In general, the file name should match the class name. > >> > >> > @@ -0,0 +1,57 @@ > >> > +# import paramiko > >> > +from paramiko import SSHClient, Channel, channel > >> > +from framework.settings import SETTINGS > >> > + > >> > +class InteractiveScriptHandler: > >> > >> Once merged with the init functionality and moved to remote_session, > >> I'd rename it to InteractiveAppSession or something similar. > > > > > > Good point, I'll rename the class and the file. > > > >> > >> > >> > + > >> > + _ssh_client: SSHClient > >> > + _stdin: channel.ChannelStdinFile > >> > + _ssh_channel: Channel > >> > + > >> > + def __init__(self, ssh_client: SSHClient, timeout:float = > SETTINGS.timeout) -> None: > >> > + self._ssh_client = ssh_client > >> > + self._ssh_channel = self._ssh_client.invoke_shell() > >> > + self._stdin = self._ssh_channel.makefile_stdin("wb") > >> > + self._ssh_channel.settimeout(timeout) > >> > + > >> > + def send_command(self, command:str) -> None: > >> > + """ > >> > + Send command to channel without recording output. > >> > + > >> > + This method will not verify any input or output, it will > >> > + simply assume the command succeeded > >> > + """ > >> > + self._stdin.write(command + '\n') > >> > + self._stdin.flush() > >> > + > >> > + def send_command_get_output(self, command:str, expect:str) -> > str: > >> > >> Let's rename expect to prompt. At least for me it was just confusing > >> in the original DTS. > > > > > > It definitely can be confusing, I'll change it accordingly as you're > right, prompt is more clear. > > > >> > >> > >> > + """ > >> > + Send a command and get all output before the expected ending > string. > >> > + > >> > + **NOTE** > >> > + Lines that expect input are not included in the stdout > buffer so they cannot be > >> > + used for expect. For example, if you were prompted to log > into something > >> > + with a username and password, you cannot expect "username:" > because it wont > >> > + yet be in the stdout buffer. A work around for this could be > consuming an > >> > + extra newline character to force the current prompt into the > stdout buffer. > >> > + > >> > + *Return* > >> > + All output before expected string > >> > + """ > >> > + stdout = self._ssh_channel.makefile("r") > >> > + self._stdin.write(command + '\n') > >> > + self._stdin.flush() > >> > + out:str = "" > >> > + for line in stdout: > >> > + out += str(line) > >> > + if expect in str(line): > >> > + break > >> > + stdout.close() #close the buffer to flush the output > >> > + return out > >> > + > >> > + def close(self): > >> > + self._stdin.close() > >> > + self._ssh_channel.close() > >> > + > >> > + def __del__(self): > >> > + self.close() > >> > diff --git > a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> > new file mode 100644 > >> > index 00000000..1993eae6 > >> > --- /dev/null > >> > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> > @@ -0,0 +1,24 @@ > >> > +from framework.testbed_model.interactive_apps import > InteractiveScriptHandler > >> > + > >> > +from pathlib import PurePath > >> > + > >> > +class TestpmdDriver: > >> > >> This could also be called TestPmdSession. Not sure which is better. > >> This should extend InteractiveScriptHandler, which should contain > >> basic functionality common to all apps and then the drivers would > >> contain app-specific functionality. > > > > > > Originally the reason I avoided doing this was because I viewed them as > separate layers of the process essentially. I wrote it in a way where the > InteractiveScriptHandler was almost like an interface for any CLI and then > DPDK apps could interact with this interface, but after thinking about it > more, this isn't necessary. There is no reason really that the applications > themselves can't just use these methods directly and it also avoids the > need to have a reference to the handler within the object. > > > >> > >> > >> > + prompt:str = "testpmd>" > >> > + interactive_handler: InteractiveScriptHandler > >> > + > >> > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> > None: > >> > + """ > >> > + Sets the handler to drive the SSH session and starts testpmd > >> > + """ > >> > + self.interactive_handler = handler > >> > + # self.interactive_handler.send_command("sudo su") > >> > + # self.interactive_handler.send_command("cd > /root/testpmd-testing/dpdk/build") > >> > + > self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > >> > >> The paths need to be handled in os-agnostic manner in os_session and > >> then passed here. > >> > >> > + > >> > + def send_command(self, command:str, expect:str = prompt) -> str: > >> > + """ > >> > + Specific way of handling the command for testpmd > >> > + > >> > + An extra newline character is consumed in order to force the > current line into the stdout buffer > >> > + """ > >> > + return > self.interactive_handler.send_command_get_output(command + "\n", expect) > >> > \ No newline at end of file > >> > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_model/node.py > >> > index d48fafe6..c5147e0e 100644 > >> > --- a/dts/framework/testbed_model/node.py > >> > +++ b/dts/framework/testbed_model/node.py > >> > @@ -40,6 +40,7 @@ class Node(object): > >> > lcores: list[LogicalCore] > >> > _logger: DTSLOG > >> > _other_sessions: list[OSSession] > >> > + _execution_config: ExecutionConfiguration > >> > > >> > def __init__(self, node_config: NodeConfiguration): > >> > self.config = node_config > >> > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > >> > """ > >> > self._setup_hugepages() > >> > self._set_up_execution(execution_config) > >> > + self._execution_config = execution_config > >> > > >> > def _set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > >> > """ > >> > diff --git a/dts/framework/testbed_model/sut_node.py > b/dts/framework/testbed_model/sut_node.py > >> > index 2b2b50d9..8c39a66d 100644 > >> > --- a/dts/framework/testbed_model/sut_node.py > >> > +++ b/dts/framework/testbed_model/sut_node.py > >> > @@ -14,6 +14,7 @@ > >> > > >> > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice > >> > from .node import Node > >> > +from .interactive_apps import InteractiveScriptHandler > >> > > >> > > >> > class SutNode(Node): > >> > @@ -261,6 +262,11 @@ def run_dpdk_app( > >> > return self.main_session.send_command( > >> > f"{app_path} {eal_args}", timeout, verify=True > >> > ) > >> > + def create_interactive_session_handler(self) -> > InteractiveScriptHandler: > >> > + """ > >> > + Create a handler for interactive sessions > >> > + """ > >> > + return > InteractiveScriptHandler(self.main_session._interactive_session) > >> > > >> > > >> > class EalParameters(object): > >> > diff --git a/dts/tests/TestSuite_smoke_tests.py > b/dts/tests/TestSuite_smoke_tests.py > >> > new file mode 100644 > >> > index 00000000..bacf289d > >> > --- /dev/null > >> > +++ b/dts/tests/TestSuite_smoke_tests.py > >> > @@ -0,0 +1,94 @@ > >> > +from framework.test_suite import TestSuite > >> > +from framework.testbed_model.sut_node import SutNode > >> > + > >> > +from framework.testbed_model.interactive_apps import TestpmdDriver > >> > + > >> > +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> > str: > >> > >> I don't see a reason why this is outside SmokeTest. > >> > >> > >> > + match compiler_name: > >> > + case "gcc": > >> > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > >> > >> As I alluded to earlier, the call here should be > >> sut_node.get_compiler_version(). This is to hide implementation > >> details from test suite developers. > >> Then, SutNode.get_compiler_version should then call > >> self.main_session.get_compiler_version(). The reason here is this is > >> an os-agnostic call. > >> get_compiler_version() should then be defined in os_session and > >> implemented in os specific classes. > > > > > > I originally placed the method outside of smoke tests just so it > wouldn't be run with the rest of the suite but it does make sense to make > this and the other comments os-agnostic. > > > >> > >> > >> > + case "clang": > >> > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > >> > + case "msvc": > >> > + return sut_node.main_session.send_command(f"cl", > 60).stdout > >> > + case "icc": > >> > + return > sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout > >> > + > >> > +class SmokeTests(TestSuite): > >> > + is_blocking = True > >> > + > >> > + def set_up_suite(self) -> None: > >> > + """ > >> > + Setup: > >> > + build all DPDK > >> > + """ > >> > + self.dpdk_build_dir_path = > self.sut_node.remote_dpdk_build_dir > >> > + > >> > + > >> > + def test_unit_tests(self) -> None: > >> > + """ > >> > + Test: > >> > + run the fast-test unit-test suite through meson > >> > + """ > >> > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite fast-tests", 300) > >> > >> Same here, there already are methods that build dpdk. If anything > >> needs to be added, we should expand those methods. > > > > > > I don't think this is building DPDK, this is just running fast tests and > ensuring that you are in the correct directory. > > > > Ah, right. These would be probably used only in smoke tests, so it's > probably fine to leave them here. Maybe we could add a method to the > suite? I'm thinking it would make the code a bit more readable. > > I could look into this. > >> > >> > >> > + > >> > + def test_driver_tests(self) -> None: > >> > + """ > >> > + Test: > >> > + run the driver-test unit-test suite through meson > >> > + """ > >> > + list_of_vdevs = "" > >> > + for dev in self.sut_node._execution_config.vdevs: > >> > + list_of_vdevs += f"{dev}," > >> > + print(list_of_vdevs) > >> > + if len(list_of_vdevs) > 0: > >> > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev > {list_of_vdevs}\"", 300) > >> > + else: > >> > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite driver-tests", 300) > >> > + > >> > + def test_gather_info(self) -> None: > >> > + """ > >> > + Test: > >> > + gather information about the system and send output to > statistics.txt > >> > + """ > >> > + out = {} > >> > + > >> > + out['OS'] = self.sut_node.main_session.send_command("awk -F= > '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout > >> > + out["OS VERSION"] = > self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print > $2}' /etc/os-release", 60, True).stdout > >> > + out["COMPILER VERSION"] = get_compiler_version( > self.build_target_info.compiler.name, self.sut_node) > >> > + out["DPDK VERSION"] = self.sut_node.dpdk_version > >> > + if self.build_target_info.os.name == "linux": > >> > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -r", 60).stdout > >> > + elif self.build_target_info.os.name == "windows": > >> > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -a", 60).stdout > >> > + self.write_to_statistics_file(out) > >> > + > >> > >> This is not really a test. If we want to add this, it should be stored > >> elsewhere (in their respective objects, probably in result objects). > >> Three of these (os, os and kernel versions) are node data, the > >> remaining two are build target data. > >> I'm not sure it's a good idea to put these into statistics, as the > >> statistics are aggregated over all executions and build targets. In > >> case of multiple SUTs (in different executions) and multiple build > >> targets we'd record misleading data. We could include data from all > >> build targets and SUTs though. > >> The reason we (and original DTS) are storing DPDK version in the > >> current manner is that that doesn't change across anything, we're > >> always testing the same tarball. > > > > > > You're right that this isn't really testing anything, I had originally > included it here because it was just part of the smoke test outline. It's > definitely out of place though and I can move it out to their respective > classes. I think it might make sense to organize/label data based on the > SUT it comes from, I just thought statistics made the most sense because > you could see the test statistics as well as what it was testing in one > place. > > > > We could put anything we want into statistics (it's not a good idea > now, but if we change the format of statistics it'd be fine, but I > think changing the format is out of the scope of this patch) and this > would make sense there, but the first step is properly storing the > data. Moving them to statistics would be trivial then. > > Right, that makes sense. > >> > >> > >> > + > >> > + > >> > + def test_start_testpmd(self) -> None: > >> > + """ > >> > + Creates and instance of the testpmd driver to run the > testpmd app > >> > + """ > >> > + driver: TestpmdDriver = > TestpmdDriver(self.sut_node.create_interactive_session_handler(), > self.dpdk_build_dir_path) > >> > >> The driver should be returned by a method of self.sut_node. > > > > > > Does it make more sense to have methods for creating handlers for > various DPDK apps in SutNode? I had assumed it would be cleaner to just > make the classes as needed so that you don't have multiple methods doing > something very similar in SutNode (basically just making a class with a > different name that takes in similar information). I suppose that if you > were trying to have developers only interact with the SutNode class this > makes sense. > > > > That's what I meant. The method would return the appropriate app based > on input (which could be an enum) - I think that's preferable to > having a method for each app. We should think about non-interactive > apps here as well (such as the helloworld app that's already in use). > > I like this idea for how to handle the creation of the DpdkApps. > > This is also something that could make good use of having the SSHClient > in the SutNode and I could see how it would be useful. > > > >> > >> > >> > + > >> > + print(driver.send_command("show port summary all")) > >> > + > >> > + def test_device_bound_to_driver(self) -> None: > >> > + """ > >> > + Test: > >> > + ensure that all drivers listed in the config are bound > to the correct drivers > >> > + """ > >> > + for nic in self.sut_node._execution_config.nics: > >> > + for address in nic.addresses: > >> > + out = > self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py > --status | grep {address}", 60) > >> > >> This should follow the same abstractions as I outlined above. > >> However, we don't need to use dpdk-devbind here. It's probably safe to > >> do so (the script is likely not going to be changed), but we could > > > > > > That's a good point about dev-bind, it might be better here to just > avoid the reliance on another script. > > > >> > >> > >> > + self.verify( > >> > + len(out.stdout) != 0, > >> > + f"Failed to find configured device ({address}) > using dpdk-devbind.py", > >> > + ) > >> > + for string in out.stdout.split(" "): > >> > + if 'drv=' in string: > >> > + self.verify( > >> > + string.split("=")[1] == > nic.driver.strip(), > >> > + f'Driver for device {address} does not > match driver listed in configuration (bound to {string.split("=")[1]})', > >> > + ) > >> > + > >> > \ No newline at end of file > >> > -- > >> > 2.40.1 > >> > > > > > > > I'll work on these changes, but I'd like to hear what you think about > what I had mentioned about moving the connect logic to the > InteractiveScriptHandler in the comments above. I had originally written it > to use one session throughout rather than opening and closing SSH > connections for every application but I'd like to hear which you think > would be easier/better if there is a difference. > > Yes, let's do one session with each app having its own channel. > [-- Attachment #2: Type: text/html, Size: 60569 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-25 18:02 ` Jeremy Spewock @ 2023-05-26 7:36 ` Juraj Linkeš 2023-05-26 13:24 ` Jeremy Spewock 0 siblings, 1 reply; 12+ messages in thread From: Juraj Linkeš @ 2023-05-26 7:36 UTC (permalink / raw) To: Jeremy Spewock; +Cc: dev On Thu, May 25, 2023 at 8:03 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote: > > Hey Juraj, > > On Thu, May 25, 2023 at 4:33 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: >> >> One more point that doesn't fit elsewhere: >> >> On Wed, May 24, 2023 at 10:45 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote: >> > >> > >> > >> > On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: >> >> >> >> Hi Jeremy, first, a few general points: >> >> >> >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and >> >> anyone else involved with DTS or who might be interested) and add the >> >> devlist to cc. >> > >> > >> > Thank you for the tip! I'm still new to sending patches and didn't think to do something like this but I will in the future. >> > >> >> >> >> 2. Run the linter script before submitting. >> > >> > >> > I did forget to run this, I will in the future. >> > >> >> >> >> 3. The use of the various nested objects breaks the current >> >> abstractions. The basic idea is that the test suite developers should >> >> ideally only use the sut/tg node objects and those objects should >> >> delegate logic further to their nested objects. More below. >> >> >> >> I have many comments about the implementation, but I haven't run it >> >> yet. I'm going to do that after this round of comments and I may have >> >> more ideas. >> >> >> >> On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: >> >> > >> >> > From: Jeremy Spewock <jspewock@iol.unh.edu> >> >> > >> >> > Adds a new test suite for running smoke tests that verify general >> >> > configuration aspects of the system under test. If any of these tests >> >> > fail, the DTS execution terminates as part of a "fail-fast" model. >> >> > >> >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> >> >> > --- >> >> > dts/conf.yaml | 9 ++ >> >> > dts/framework/config/__init__.py | 21 +++++ >> >> > dts/framework/config/conf_yaml_schema.json | 32 ++++++- >> >> > dts/framework/dts.py | 19 +++- >> >> > dts/framework/exception.py | 11 +++ >> >> > dts/framework/remote_session/os_session.py | 6 +- >> >> > .../remote_session/remote/__init__.py | 28 ++++++ >> >> > dts/framework/test_result.py | 13 ++- >> >> > dts/framework/test_suite.py | 24 ++++- >> >> > dts/framework/testbed_model/__init__.py | 5 + >> >> > .../interactive_apps/__init__.py | 6 ++ >> >> > .../interactive_apps/interactive_command.py | 57 +++++++++++ >> >> > .../interactive_apps/testpmd_driver.py | 24 +++++ >> >> > dts/framework/testbed_model/node.py | 2 + >> >> > dts/framework/testbed_model/sut_node.py | 6 ++ >> >> > dts/tests/TestSuite_smoke_tests.py | 94 +++++++++++++++++++ >> >> > 16 files changed, 348 insertions(+), 9 deletions(-) >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/__init__.py >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/interactive_command.py >> >> > create mode 100644 dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> >> >> >> Let's not add any more levels. I don't like even the current hw >> >> subdirectory (which I want to remove in the docs patch) and we don't >> >> need it. I'd also like to move this functionality into remote_session, >> >> as it's handling a type of remote session. >> > >> > >> > I think it makes sense to add a proper wrapper for it, I just didn't create a subclass for it off of remote_session because the idea behind it was only allowing users to interact with the session through the InteractiveScriptHandler/DPDK app handlers. If it were part of the remote_session class it would have to include operations for sending commands the way it is written now. I could do this but it seems like a bigger discussion about whether interactive sessions should create a new SSH session every time or instead just use one existing session and create channels off of it. >> > >> >> I wasn't clear, I meant to move the python modules into the >> remote_session folder. The subclassing would be there only to have a >> common API across these remote sessions (as the >> InteractiveScriptHandler is a kind of a remote session). If at all >> possible, this should be our aim. > > > I see what you mean now, moving them there shouldn't be a problem. > >> >> >> >> >> >> >> >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py >> >> > >> >> > diff --git a/dts/conf.yaml b/dts/conf.yaml >> >> > index a9bd8a3e..042ef954 100644 >> >> > --- a/dts/conf.yaml >> >> > +++ b/dts/conf.yaml >> >> > @@ -10,13 +10,22 @@ executions: >> >> > compiler_wrapper: ccache >> >> > perf: false >> >> > func: true >> >> > + nics: #physical devices to be used for testing >> >> > + - addresses: >> >> > + - "0000:11:00.0" >> >> > + - "0000:11:00.1" >> >> > + driver: "i40e" >> >> > + vdevs: #names of virtual devices to be used for testing >> >> > + - "crypto_openssl" >> >> >> >> I believe we specified the NICs under SUTs in the original DTS, just >> >> as Owen did in his internal GitLab patch. If you can access it, have a >> >> look at how he did it. >> >> This brings an interesting question of where we want to specify which >> >> NICs/vdevs to test. It could be on the SUT level, but also on the >> >> execution or even the build target level. This should be informed by >> >> testing needs. What makes the most sense? We could specify NIC details >> >> per SUT/TG and then just reference the NICs on the execution/build >> >> target level. >> > >> > >> > I put it on the execution level with the thought that you might have multiple NICs in your SUT but want to separate them into different executions. >> >> This is a good point. Is this something you're interested in in the >> lab? We should ask Lijuan what she thinks of this. In general, I like >> this, so we should at least think of how to add NIC config >> implementation so that we could add this later if we're not adding it >> now. >> >> > I guess in the case of smoke tests, you'd only need to know them per target because it was talked about previously that then is when we should run the smoke tests. I think however it would make sense to specify the NIC you are using for that execution rather than having to assume or specify elsewhere. >> > >> >> Nothing is set in stone, we don't have to run them per build target. >> We could have two smoke test suites, one per execution and one per >> build target. I actually like that a lot and we should explore that. >> If it's not much extra work, we could just do the split like that. >> > > I also like the sound of that. In theory it doesn't seem that hard, if we made two different suites that tested different things all that needs to change is when they are run. This would probably take some more planning as to what we want to run at which points during the run and it might be easier to get these tests in a better spot first. I do like the idea though, and agree that it's worth exploring. > >> >> >> >> >> >> > test_suites: >> >> > + - smoke_tests >> >> > - hello_world >> >> > system_under_test: "SUT 1" >> >> > nodes: >> >> > - name: "SUT 1" >> >> > hostname: sut1.change.me.localhost >> >> > user: root >> >> > + password: "" >> >> >> >> This was deliberately left out to discourage the use of passwords. >> >> >> >> > arch: x86_64 >> >> > os: linux >> >> > lcores: "" >> >> >> >> <snip> >> >> >> >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py >> >> > index 05022845..0d03e158 100644 >> >> > --- a/dts/framework/dts.py >> >> > +++ b/dts/framework/dts.py >> >> > @@ -5,6 +5,8 @@ >> >> > >> >> > import sys >> >> > >> >> > +from .exception import BlockingTestSuiteError >> >> > + >> >> > from .config import CONFIGURATION, BuildTargetConfiguration, ExecutionConfiguration >> >> > from .logger import DTSLOG, getLogger >> >> > from .test_result import BuildTargetResult, DTSResult, ExecutionResult, Result >> >> > @@ -49,6 +51,7 @@ def run_all() -> None: >> >> > nodes[sut_node.name] = sut_node >> >> > >> >> > if sut_node: >> >> > + #SMOKE TEST EXECUTION GOES HERE! >> >> > _run_execution(sut_node, execution, result) >> >> > >> >> > except Exception as e: >> >> > @@ -118,7 +121,7 @@ def _run_build_target( >> >> > >> >> > try: >> >> > sut_node.set_up_build_target(build_target) >> >> > - result.dpdk_version = sut_node.dpdk_version >> >> > + # result.dpdk_version = sut_node.dpdk_version >> >> > build_target_result.update_setup(Result.PASS) >> >> > except Exception as e: >> >> > dts_logger.exception("Build target setup failed.") >> >> > @@ -146,6 +149,7 @@ def _run_suites( >> >> > with possibly only a subset of test cases. >> >> > If no subset is specified, run all test cases. >> >> > """ >> >> > + end_execution = False >> >> >> >> This only ends the build target stage, not the execution stage. We >> >> should either find a better name for the variable or make sure that >> >> the whole execution is blocked. I think we're aiming for the latter - >> >> maybe we could just check whether the last exception in result is a >> >> BlockingTestSuiteError (or better, just call a new method of result >> >> that will check that), which we could do at multiple stages. >> > >> > >> > Good catch. When writing this and figuring out how to get it to work, I was thinking about smoke tests on a per build target basis because if it failed on one build target it could theoretically pass on another, but you'd likely want your entire execution to fail than have partial results. >> > >> >> Well, these will be well defined partial results (and definitely >> valuable, imagine one build target passing and one failing, we could >> easily compare the two to sometimes quickly identify the failure), so >> if there's a possibility that the test would pass on another build >> target, we should try to run the build target. >> >> In this case, let's just rename the variable to end_build_target or >> something similar. >> > > Alright, will do. > >> >> >> >> >> >> >> > for test_suite_config in execution.test_suites: >> >> > try: >> >> > full_suite_path = f"tests.TestSuite_{test_suite_config.test_suite}" >> >> > @@ -160,13 +164,24 @@ def _run_suites( >> >> > >> >> > else: >> >> > for test_suite_class in test_suite_classes: >> >> > + #HERE NEEDS CHANGING >> >> > test_suite = test_suite_class( >> >> > sut_node, >> >> > test_suite_config.test_cases, >> >> > execution.func, >> >> > build_target_result, >> >> > + sut_node._build_target_config, >> >> > + result >> >> > ) >> >> > - test_suite.run() >> >> > + try: >> >> > + test_suite.run() >> >> > + except BlockingTestSuiteError as e: >> >> > + dts_logger.exception("An error occurred within a blocking TestSuite, execution will now end.") >> >> > + result.add_error(e) >> >> > + end_execution = True >> >> > + #if a blocking test failed and we need to bail out of suite executions >> >> > + if end_execution: >> >> > + break >> >> > >> >> > >> >> > def _exit_dts() -> None: >> >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py >> >> > index ca353d98..4e3f63d1 100644 >> >> > --- a/dts/framework/exception.py >> >> > +++ b/dts/framework/exception.py >> >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): >> >> > SSH_ERR = 4 >> >> > DPDK_BUILD_ERR = 10 >> >> > TESTCASE_VERIFY_ERR = 20 >> >> > + BLOCKING_TESTSUITE_ERR = 25 >> >> > >> >> > >> >> > class DTSError(Exception): >> >> > @@ -144,3 +145,13 @@ def __init__(self, value: str): >> >> > >> >> > def __str__(self) -> str: >> >> > return repr(self.value) >> >> > + >> >> > +class BlockingTestSuiteError(DTSError): >> >> > + suite_name: str >> >> > + severity: ClassVar[ErrorSeverity] = ErrorSeverity.BLOCKING_TESTSUITE_ERR >> >> > + >> >> > + def __init__(self, suite_name:str) -> None: >> >> > + self.suite_name = suite_name >> >> > + >> >> > + def __str__(self) -> str: >> >> > + return f"Blocking suite {self.suite_name} failed." >> >> > diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/remote_session/os_session.py >> >> > index 4c48ae25..22776bc1 100644 >> >> > --- a/dts/framework/remote_session/os_session.py >> >> > +++ b/dts/framework/remote_session/os_session.py >> >> > @@ -12,7 +12,9 @@ >> >> > from framework.testbed_model import LogicalCore >> >> > from framework.utils import EnvVarsDict, MesonArgs >> >> > >> >> > -from .remote import CommandResult, RemoteSession, create_remote_session >> >> > +from .remote import CommandResult, RemoteSession, create_remote_session, create_interactive_session >> >> > + >> >> > +from paramiko import SSHClient >> >> > >> >> > >> >> > class OSSession(ABC): >> >> > @@ -26,6 +28,7 @@ class OSSession(ABC): >> >> > name: str >> >> > _logger: DTSLOG >> >> > remote_session: RemoteSession >> >> > + _interactive_session: SSHClient >> >> > >> >> > def __init__( >> >> > self, >> >> > @@ -37,6 +40,7 @@ def __init__( >> >> > self.name = name >> >> > self._logger = logger >> >> > self.remote_session = create_remote_session(node_config, name, logger) >> >> > + self._interactive_session = create_interactive_session(node_config, name, logger) >> >> >> >> This session should be stored in SutNode (it should be os-agnostic >> >> (the apps should behave the same on all os's, right?) and SutNode >> >> could use it without accessing another object), but created in >> >> OSSession (this way we make sure any os-specific inputs (such as >> >> paths) are passed properly). >> > >> > >> > I can move it into SutNode, the main reason I left it there is because I was essentially thinking of it the same way as the remote session that gets created once per node. However, I could just as easily have SutNode store it and call upon OSSession to create one inside its constructor. This would make creating classes for DPDK apps easier as well once those are subclasses of the InteractiveScriptHandler. >> > >> >> I was thinking of this interactive session more like >> Node._other_sessions, created only when needed. Now that I think about >> it, it's always going to be needed if we use it in smoke tests, so we >> might as well create it in OSSession (or its subclasses if needed). >> >> Thinking further, we want to use only the app driver object in test >> suites. So the SutNode object should have a method that returns the >> object and the rest of the implementation needs to be delegated to the >> rest of the objects in the hierarchy to ensure proper remote OS >> handling. > > > I like the sound of this. If we use a method and something like an enum inside of SutNode as well to create these objects then we don't even really need a method to return this object because it would never be interacted with directly. > >> >> >> >> >> >> > >> >> > def close(self, force: bool = False) -> None: >> >> > """ >> >> > diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py >> >> > index 8a151221..abca8edc 100644 >> >> > --- a/dts/framework/remote_session/remote/__init__.py >> >> > +++ b/dts/framework/remote_session/remote/__init__.py >> >> > @@ -9,8 +9,36 @@ >> >> > from .remote_session import CommandResult, RemoteSession >> >> > from .ssh_session import SSHSession >> >> > >> >> > +from paramiko import SSHClient, AutoAddPolicy >> >> > +from framework.utils import GREEN >> >> > >> >> > def create_remote_session( >> >> > node_config: NodeConfiguration, name: str, logger: DTSLOG >> >> > ) -> RemoteSession: >> >> > return SSHSession(node_config, name, logger) >> >> > + >> >> > +def create_interactive_session( >> >> > + node_config: NodeConfiguration, name: str, logger: DTSLOG >> >> > +) -> SSHClient: >> >> > + """ >> >> > + Creates a paramiko SSH session that is designed to be used for interactive shells >> >> > + >> >> > + This session is meant to be used on an "as needed" basis and may never be utilized >> >> > + """ >> >> > + client: SSHClient = SSHClient() >> >> > + client.set_missing_host_key_policy(AutoAddPolicy) >> >> > + ip: str = node_config.hostname >> >> > + logger.info(GREEN(f"Connecting to host {ip}")) >> >> >> >> Using colors is a remnant from the original DTS. If we want to >> >> (re-)introduce colors I'd do that in a separate patch in a uniform >> >> manner. >> > >> > >> > I agree this is likely outside the scope of this patch and the color here really isn't necessary either so I'll remove it. >> > >> >> >> >> >> >> > + #Preset to 22 because paramiko doesn't accept None >> >> > + port: int = 22 >> >> >> >> This configuration handling should be moved to NodeConfiguration. >> >> The logic should also be moved to InteractiveScriptHandler. We also >> >> probably don't need a factory for this. >> > >> > >> > I agree that the configuration could be moved into NodeConfiguration, however the reason I use a factory and don't have this connect logic inside InteractiveScriptHandler is because that would mean there would have to be an SSH session per application. This is one potential solution, but what I am doing here instead is creating one SSH session for interactive scripts at the start alongside the usual remote session. This session essentially exists in the background for the duration of the execution and then each application for DPDK creates a channel off that session when they are created. Then, when you are done using the application, it closes the channel but the session itself stays open. >> > >> >> Ok, I see. What do you think about this: >> Let's then move the session creation into a subclass of RemoteSession. >> The session would be stored as a class variable and with that we could >> subclass further: >> RemoteSession -> new subclass -> InteractiveScriptHandler -> DpdkApp >> >> Everything could be handled from within the DdpdkApp objects, but we'd >> need to be careful about cleanup (maybe we'd only need to check that >> there are no open channels when closing the session). >> >> Or alternatively, the new subclass could just have a method that >> returns DpdkApps. We'd have two different class relations: >> RemoteSession -> new subclass and InteractiveScriptHandler -> DpdkApp. >> >> I'd put all of the classes into the remote_session directory. >> > > I like the idea of making a subclass of the remote session and then a method in the subclass that generates DpdkApps. This way you have a proper class that maintains the interactive session and InteractiveScriptHandlers as well as any other DpdkApp can be created and destroyed as needed. The only reason I wouldn't make InteractiveScriptHandler a subclass of RemoteSession is because then it would make the InteractiveScriptHandler something that stored the main SSH session rather than something that just uses it to make a channel. > > Originally, I avoided this because I didn't want to implement the send_command methods from inside RemoteSession, but a solution to this could just be creating an InteractiveScriptHandler, sending the command blindly, and not returning the output. Or maybe there would be some kind of default prompt I could expect to still collect the output, but I'm not sure there would be something that is os-agnostic for this. I'm not sure the best way to show that I don't want people to use the method on the new subclass that I'm going to create though. I guess one way to handle this would be just not creating a method that returns this subclass in SutNode. SutNode could just have a method that returns DPDK apps which should discourage touching this session directly. > Ah right, the fact that the workflow with interactive sessions is a bit different doesn't make subclassing RemoteSession ideal - the abstract methods _send_command and copy_file don't make much sense in the new class. With that in mind, I think we have two options: 1. Don't use one session with multiple channels. This would make it more in line with RemoteSession, but the copy_file method still doesn't make sense, so I prefer 2. 2. Don't subclass RemoteSession as it's a bit too different from what we need. Just create a new class (named InteractiveRemoteSession perpahs) that would basically be a factory for drivers (with two parts - ssh session init and then the creation of driver/app objects). The InteractiveScriptHandler (I'm thinking of renaming it to InteractiveShell now) -> DpdkApp would be the other part. I'd put InteractiveRemoteSession and InteractiveScriptHandler/InteractiveShell into the same file (those two basically define the API on the user and developer side). >> >> >> >> >> >> >> > + if ":" in node_config.hostname: >> >> > + ip, port = node_config.hostname.split(":") >> >> > + port = int(port) >> >> > + client.connect( >> >> > + ip, >> >> > + username=node_config.user, >> >> > + port=port, >> >> > + password=node_config.password or "", >> >> > + timeout=20 if port else 10 >> >> > + ) >> >> > + return client >> >> > diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py >> >> > index 74391982..77202ae2 100644 >> >> > --- a/dts/framework/test_result.py >> >> > +++ b/dts/framework/test_result.py >> >> > @@ -8,6 +8,7 @@ >> >> > import os.path >> >> > from collections.abc import MutableSequence >> >> > from enum import Enum, auto >> >> > +from typing import Dict >> >> > >> >> > from .config import ( >> >> > OS, >> >> > @@ -67,12 +68,13 @@ class Statistics(dict): >> >> > Using a dict provides a convenient way to format the data. >> >> > """ >> >> > >> >> > - def __init__(self, dpdk_version): >> >> > + def __init__(self, output_info: Dict[str, str] | None): >> >> > super(Statistics, self).__init__() >> >> > for result in Result: >> >> > self[result.name] = 0 >> >> > self["PASS RATE"] = 0.0 >> >> > - self["DPDK VERSION"] = dpdk_version >> >> > + if output_info: >> >> > + for info_key, info_val in output_info.items(): self[info_key] = info_val >> >> > >> >> > def __iadd__(self, other: Result) -> "Statistics": >> >> > """ >> >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): >> >> > """ >> >> > >> >> > dpdk_version: str | None >> >> > + output: dict | None >> >> > _logger: DTSLOG >> >> > _errors: list[Exception] >> >> > _return_code: ErrorSeverity >> >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): >> >> > def __init__(self, logger: DTSLOG): >> >> > super(DTSResult, self).__init__() >> >> > self.dpdk_version = None >> >> > + self.output = None >> >> > self._logger = logger >> >> > self._errors = [] >> >> > self._return_code = ErrorSeverity.NO_ERR >> >> > @@ -296,7 +300,10 @@ def process(self) -> None: >> >> > for error in self._errors: >> >> > self._logger.debug(repr(error)) >> >> > >> >> > - self._stats_result = Statistics(self.dpdk_version) >> >> > + self._stats_result = Statistics(self.output) >> >> > + #add information gathered from the smoke tests to the statistics >> >> > + # for info_key, info_val in smoke_test_info.items(): self._stats_result[info_key] = info_val >> >> > + # print(self._stats_result) >> >> > self.add_stats(self._stats_result) >> >> > with open(self._stats_filename, "w+") as stats_file: >> >> > stats_file.write(str(self._stats_result)) >> >> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py >> >> > index 0705f38f..1518fb8a 100644 >> >> > --- a/dts/framework/test_suite.py >> >> > +++ b/dts/framework/test_suite.py >> >> > @@ -10,11 +10,14 @@ >> >> > import inspect >> >> > import re >> >> > from types import MethodType >> >> > +from typing import Dict >> >> > >> >> > -from .exception import ConfigurationError, SSHTimeoutError, TestCaseVerifyError >> >> > +from .config import BuildTargetConfiguration >> >> > + >> >> > +from .exception import BlockingTestSuiteError, ConfigurationError, SSHTimeoutError, TestCaseVerifyError >> >> > from .logger import DTSLOG, getLogger >> >> > from .settings import SETTINGS >> >> > -from .test_result import BuildTargetResult, Result, TestCaseResult, TestSuiteResult >> >> > +from .test_result import BuildTargetResult, DTSResult, Result, TestCaseResult, TestSuiteResult >> >> > from .testbed_model import SutNode >> >> > >> >> > >> >> > @@ -37,10 +40,12 @@ class TestSuite(object): >> >> > """ >> >> > >> >> > sut_node: SutNode >> >> > + is_blocking = False >> >> > _logger: DTSLOG >> >> > _test_cases_to_run: list[str] >> >> > _func: bool >> >> > _result: TestSuiteResult >> >> > + _dts_result: DTSResult >> >> > >> >> > def __init__( >> >> > self, >> >> > @@ -48,6 +53,8 @@ def __init__( >> >> > test_cases: list[str], >> >> > func: bool, >> >> > build_target_result: BuildTargetResult, >> >> > + build_target_conf: BuildTargetConfiguration, >> >> > + dts_result: DTSResult >> >> > ): >> >> > self.sut_node = sut_node >> >> > self._logger = getLogger(self.__class__.__name__) >> >> > @@ -55,6 +62,8 @@ def __init__( >> >> > self._test_cases_to_run.extend(SETTINGS.test_cases) >> >> > self._func = func >> >> > self._result = build_target_result.add_test_suite(self.__class__.__name__) >> >> > + self.build_target_info = build_target_conf >> >> > + self._dts_result = dts_result >> >> > >> >> > def set_up_suite(self) -> None: >> >> > """ >> >> > @@ -118,6 +127,9 @@ def run(self) -> None: >> >> > f"the next test suite may be affected." >> >> > ) >> >> > self._result.update_setup(Result.ERROR, e) >> >> > + if len(self._result.get_errors()) > 0 and self.is_blocking: >> >> > + raise BlockingTestSuiteError(test_suite_name) >> >> > + >> >> > >> >> > def _execute_test_suite(self) -> None: >> >> > """ >> >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: >> >> > f"Attempt number {attempt_nr} out of {all_attempts}." >> >> > ) >> >> > self._run_test_case(test_case_method, test_case_result) >> >> > + >> >> > >> >> > def _get_functional_test_cases(self) -> list[MethodType]: >> >> > """ >> >> > @@ -232,6 +245,11 @@ def _execute_test_case( >> >> > test_case_result.update(Result.SKIP) >> >> > raise KeyboardInterrupt("Stop DTS") >> >> > >> >> > + def write_to_statistics_file(self, output: Dict[str, str]): >> >> > + if self._dts_result.output != None: >> >> > + self._dts_result.output.update(output) >> >> > + else: >> >> > + self._dts_result.output = output >> >> > >> >> > def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]]: >> >> > def is_test_suite(object) -> bool: >> >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: >> >> > test_suite_class >> >> > for _, test_suite_class in inspect.getmembers(testcase_module, is_test_suite) >> >> > ] >> >> > + >> >> > + >> >> > diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py >> >> > index f54a9470..63f17cc3 100644 >> >> > --- a/dts/framework/testbed_model/__init__.py >> >> > +++ b/dts/framework/testbed_model/__init__.py >> >> > @@ -20,3 +20,8 @@ >> >> > ) >> >> > from .node import Node >> >> > from .sut_node import SutNode >> >> > + >> >> > +from .interactive_apps import ( >> >> > + InteractiveScriptHandler, >> >> > + TestpmdDriver >> >> > +) >> >> > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py >> >> > new file mode 100644 >> >> > index 00000000..0382d7e0 >> >> > --- /dev/null >> >> > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py >> >> > @@ -0,0 +1,6 @@ >> >> > +from .interactive_command import ( >> >> > + InteractiveScriptHandler >> >> > +) >> >> > +from .testpmd_driver import ( >> >> > + TestpmdDriver >> >> > +) >> >> > \ No newline at end of file >> >> > diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py >> >> > new file mode 100644 >> >> > index 00000000..7467911b >> >> > --- /dev/null >> >> > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py >> >> >> >> In general, the file name should match the class name. >> >> >> >> > @@ -0,0 +1,57 @@ >> >> > +# import paramiko >> >> > +from paramiko import SSHClient, Channel, channel >> >> > +from framework.settings import SETTINGS >> >> > + >> >> > +class InteractiveScriptHandler: >> >> >> >> Once merged with the init functionality and moved to remote_session, >> >> I'd rename it to InteractiveAppSession or something similar. >> > >> > >> > Good point, I'll rename the class and the file. >> > >> >> >> >> >> >> > + >> >> > + _ssh_client: SSHClient >> >> > + _stdin: channel.ChannelStdinFile >> >> > + _ssh_channel: Channel >> >> > + >> >> > + def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None: >> >> > + self._ssh_client = ssh_client >> >> > + self._ssh_channel = self._ssh_client.invoke_shell() >> >> > + self._stdin = self._ssh_channel.makefile_stdin("wb") >> >> > + self._ssh_channel.settimeout(timeout) >> >> > + >> >> > + def send_command(self, command:str) -> None: >> >> > + """ >> >> > + Send command to channel without recording output. >> >> > + >> >> > + This method will not verify any input or output, it will >> >> > + simply assume the command succeeded >> >> > + """ >> >> > + self._stdin.write(command + '\n') >> >> > + self._stdin.flush() >> >> > + >> >> > + def send_command_get_output(self, command:str, expect:str) -> str: >> >> >> >> Let's rename expect to prompt. At least for me it was just confusing >> >> in the original DTS. >> > >> > >> > It definitely can be confusing, I'll change it accordingly as you're right, prompt is more clear. >> > >> >> >> >> >> >> > + """ >> >> > + Send a command and get all output before the expected ending string. >> >> > + >> >> > + **NOTE** >> >> > + Lines that expect input are not included in the stdout buffer so they cannot be >> >> > + used for expect. For example, if you were prompted to log into something >> >> > + with a username and password, you cannot expect "username:" because it wont >> >> > + yet be in the stdout buffer. A work around for this could be consuming an >> >> > + extra newline character to force the current prompt into the stdout buffer. >> >> > + >> >> > + *Return* >> >> > + All output before expected string >> >> > + """ >> >> > + stdout = self._ssh_channel.makefile("r") >> >> > + self._stdin.write(command + '\n') >> >> > + self._stdin.flush() >> >> > + out:str = "" >> >> > + for line in stdout: >> >> > + out += str(line) >> >> > + if expect in str(line): >> >> > + break >> >> > + stdout.close() #close the buffer to flush the output >> >> > + return out >> >> > + >> >> > + def close(self): >> >> > + self._stdin.close() >> >> > + self._ssh_channel.close() >> >> > + >> >> > + def __del__(self): >> >> > + self.close() >> >> > diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> >> > new file mode 100644 >> >> > index 00000000..1993eae6 >> >> > --- /dev/null >> >> > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py >> >> > @@ -0,0 +1,24 @@ >> >> > +from framework.testbed_model.interactive_apps import InteractiveScriptHandler >> >> > + >> >> > +from pathlib import PurePath >> >> > + >> >> > +class TestpmdDriver: >> >> >> >> This could also be called TestPmdSession. Not sure which is better. >> >> This should extend InteractiveScriptHandler, which should contain >> >> basic functionality common to all apps and then the drivers would >> >> contain app-specific functionality. >> > >> > >> > Originally the reason I avoided doing this was because I viewed them as separate layers of the process essentially. I wrote it in a way where the InteractiveScriptHandler was almost like an interface for any CLI and then DPDK apps could interact with this interface, but after thinking about it more, this isn't necessary. There is no reason really that the applications themselves can't just use these methods directly and it also avoids the need to have a reference to the handler within the object. >> > >> >> >> >> >> >> > + prompt:str = "testpmd>" >> >> > + interactive_handler: InteractiveScriptHandler >> >> > + >> >> > + def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None: >> >> > + """ >> >> > + Sets the handler to drive the SSH session and starts testpmd >> >> > + """ >> >> > + self.interactive_handler = handler >> >> > + # self.interactive_handler.send_command("sudo su") >> >> > + # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build") >> >> > + self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt) >> >> >> >> The paths need to be handled in os-agnostic manner in os_session and >> >> then passed here. >> >> >> >> > + >> >> > + def send_command(self, command:str, expect:str = prompt) -> str: >> >> > + """ >> >> > + Specific way of handling the command for testpmd >> >> > + >> >> > + An extra newline character is consumed in order to force the current line into the stdout buffer >> >> > + """ >> >> > + return self.interactive_handler.send_command_get_output(command + "\n", expect) >> >> > \ No newline at end of file >> >> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py >> >> > index d48fafe6..c5147e0e 100644 >> >> > --- a/dts/framework/testbed_model/node.py >> >> > +++ b/dts/framework/testbed_model/node.py >> >> > @@ -40,6 +40,7 @@ class Node(object): >> >> > lcores: list[LogicalCore] >> >> > _logger: DTSLOG >> >> > _other_sessions: list[OSSession] >> >> > + _execution_config: ExecutionConfiguration >> >> > >> >> > def __init__(self, node_config: NodeConfiguration): >> >> > self.config = node_config >> >> > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None: >> >> > """ >> >> > self._setup_hugepages() >> >> > self._set_up_execution(execution_config) >> >> > + self._execution_config = execution_config >> >> > >> >> > def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None: >> >> > """ >> >> > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py >> >> > index 2b2b50d9..8c39a66d 100644 >> >> > --- a/dts/framework/testbed_model/sut_node.py >> >> > +++ b/dts/framework/testbed_model/sut_node.py >> >> > @@ -14,6 +14,7 @@ >> >> > >> >> > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice >> >> > from .node import Node >> >> > +from .interactive_apps import InteractiveScriptHandler >> >> > >> >> > >> >> > class SutNode(Node): >> >> > @@ -261,6 +262,11 @@ def run_dpdk_app( >> >> > return self.main_session.send_command( >> >> > f"{app_path} {eal_args}", timeout, verify=True >> >> > ) >> >> > + def create_interactive_session_handler(self) -> InteractiveScriptHandler: >> >> > + """ >> >> > + Create a handler for interactive sessions >> >> > + """ >> >> > + return InteractiveScriptHandler(self.main_session._interactive_session) >> >> > >> >> > >> >> > class EalParameters(object): >> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py >> >> > new file mode 100644 >> >> > index 00000000..bacf289d >> >> > --- /dev/null >> >> > +++ b/dts/tests/TestSuite_smoke_tests.py >> >> > @@ -0,0 +1,94 @@ >> >> > +from framework.test_suite import TestSuite >> >> > +from framework.testbed_model.sut_node import SutNode >> >> > + >> >> > +from framework.testbed_model.interactive_apps import TestpmdDriver >> >> > + >> >> > +def get_compiler_version(compiler_name: str, sut_node: SutNode) -> str: >> >> >> >> I don't see a reason why this is outside SmokeTest. >> >> >> >> >> >> > + match compiler_name: >> >> > + case "gcc": >> >> > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] >> >> >> >> As I alluded to earlier, the call here should be >> >> sut_node.get_compiler_version(). This is to hide implementation >> >> details from test suite developers. >> >> Then, SutNode.get_compiler_version should then call >> >> self.main_session.get_compiler_version(). The reason here is this is >> >> an os-agnostic call. >> >> get_compiler_version() should then be defined in os_session and >> >> implemented in os specific classes. >> > >> > >> > I originally placed the method outside of smoke tests just so it wouldn't be run with the rest of the suite but it does make sense to make this and the other comments os-agnostic. >> > >> >> >> >> >> >> > + case "clang": >> >> > + return sut_node.main_session.send_command(f"{compiler_name} --version", 60).stdout.split("\n")[0] >> >> > + case "msvc": >> >> > + return sut_node.main_session.send_command(f"cl", 60).stdout >> >> > + case "icc": >> >> > + return sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout >> >> > + >> >> > +class SmokeTests(TestSuite): >> >> > + is_blocking = True >> >> > + >> >> > + def set_up_suite(self) -> None: >> >> > + """ >> >> > + Setup: >> >> > + build all DPDK >> >> > + """ >> >> > + self.dpdk_build_dir_path = self.sut_node.remote_dpdk_build_dir >> >> > + >> >> > + >> >> > + def test_unit_tests(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + run the fast-test unit-test suite through meson >> >> > + """ >> >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite fast-tests", 300) >> >> >> >> Same here, there already are methods that build dpdk. If anything >> >> needs to be added, we should expand those methods. >> > >> > >> > I don't think this is building DPDK, this is just running fast tests and ensuring that you are in the correct directory. >> > >> >> Ah, right. These would be probably used only in smoke tests, so it's >> probably fine to leave them here. Maybe we could add a method to the >> suite? I'm thinking it would make the code a bit more readable. >> > > I could look into this. > >> >> >> >> >> >> >> > + >> >> > + def test_driver_tests(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + run the driver-test unit-test suite through meson >> >> > + """ >> >> > + list_of_vdevs = "" >> >> > + for dev in self.sut_node._execution_config.vdevs: >> >> > + list_of_vdevs += f"{dev}," >> >> > + print(list_of_vdevs) >> >> > + if len(list_of_vdevs) > 0: >> >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev {list_of_vdevs}\"", 300) >> >> > + else: >> >> > + self.sut_node.main_session.send_command(f"meson test -C {self.dpdk_build_dir_path} --suite driver-tests", 300) >> >> > + >> >> > + def test_gather_info(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + gather information about the system and send output to statistics.txt >> >> > + """ >> >> > + out = {} >> >> > + >> >> > + out['OS'] = self.sut_node.main_session.send_command("awk -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout >> >> > + out["OS VERSION"] = self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print $2}' /etc/os-release", 60, True).stdout >> >> > + out["COMPILER VERSION"] = get_compiler_version(self.build_target_info.compiler.name, self.sut_node) >> >> > + out["DPDK VERSION"] = self.sut_node.dpdk_version >> >> > + if self.build_target_info.os.name == "linux": >> >> > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -r", 60).stdout >> >> > + elif self.build_target_info.os.name == "windows": >> >> > + out['KERNEL VERSION'] = self.sut_node.main_session.send_command("uname -a", 60).stdout >> >> > + self.write_to_statistics_file(out) >> >> > + >> >> >> >> This is not really a test. If we want to add this, it should be stored >> >> elsewhere (in their respective objects, probably in result objects). >> >> Three of these (os, os and kernel versions) are node data, the >> >> remaining two are build target data. >> >> I'm not sure it's a good idea to put these into statistics, as the >> >> statistics are aggregated over all executions and build targets. In >> >> case of multiple SUTs (in different executions) and multiple build >> >> targets we'd record misleading data. We could include data from all >> >> build targets and SUTs though. >> >> The reason we (and original DTS) are storing DPDK version in the >> >> current manner is that that doesn't change across anything, we're >> >> always testing the same tarball. >> > >> > >> > You're right that this isn't really testing anything, I had originally included it here because it was just part of the smoke test outline. It's definitely out of place though and I can move it out to their respective classes. I think it might make sense to organize/label data based on the SUT it comes from, I just thought statistics made the most sense because you could see the test statistics as well as what it was testing in one place. >> > >> >> We could put anything we want into statistics (it's not a good idea >> now, but if we change the format of statistics it'd be fine, but I >> think changing the format is out of the scope of this patch) and this >> would make sense there, but the first step is properly storing the >> data. Moving them to statistics would be trivial then. >> > > Right, that makes sense. > >> >> >> >> >> >> >> > + >> >> > + >> >> > + def test_start_testpmd(self) -> None: >> >> > + """ >> >> > + Creates and instance of the testpmd driver to run the testpmd app >> >> > + """ >> >> > + driver: TestpmdDriver = TestpmdDriver(self.sut_node.create_interactive_session_handler(), self.dpdk_build_dir_path) >> >> >> >> The driver should be returned by a method of self.sut_node. >> > >> > >> > Does it make more sense to have methods for creating handlers for various DPDK apps in SutNode? I had assumed it would be cleaner to just make the classes as needed so that you don't have multiple methods doing something very similar in SutNode (basically just making a class with a different name that takes in similar information). I suppose that if you were trying to have developers only interact with the SutNode class this makes sense. >> > >> >> That's what I meant. The method would return the appropriate app based >> on input (which could be an enum) - I think that's preferable to >> having a method for each app. We should think about non-interactive >> apps here as well (such as the helloworld app that's already in use). >> > > I like this idea for how to handle the creation of the DpdkApps. > >> >> > This is also something that could make good use of having the SSHClient in the SutNode and I could see how it would be useful. >> > >> >> >> >> >> >> > + >> >> > + print(driver.send_command("show port summary all")) >> >> > + >> >> > + def test_device_bound_to_driver(self) -> None: >> >> > + """ >> >> > + Test: >> >> > + ensure that all drivers listed in the config are bound to the correct drivers >> >> > + """ >> >> > + for nic in self.sut_node._execution_config.nics: >> >> > + for address in nic.addresses: >> >> > + out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60) >> >> >> >> This should follow the same abstractions as I outlined above. >> >> However, we don't need to use dpdk-devbind here. It's probably safe to >> >> do so (the script is likely not going to be changed), but we could >> > >> > >> > That's a good point about dev-bind, it might be better here to just avoid the reliance on another script. >> > >> >> >> >> >> >> > + self.verify( >> >> > + len(out.stdout) != 0, >> >> > + f"Failed to find configured device ({address}) using dpdk-devbind.py", >> >> > + ) >> >> > + for string in out.stdout.split(" "): >> >> > + if 'drv=' in string: >> >> > + self.verify( >> >> > + string.split("=")[1] == nic.driver.strip(), >> >> > + f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})', >> >> > + ) >> >> > + >> >> > \ No newline at end of file >> >> > -- >> >> > 2.40.1 >> >> > >> > >> > >> > I'll work on these changes, but I'd like to hear what you think about what I had mentioned about moving the connect logic to the InteractiveScriptHandler in the comments above. I had originally written it to use one session throughout rather than opening and closing SSH connections for every application but I'd like to hear which you think would be easier/better if there is a difference. >> >> Yes, let's do one session with each app having its own channel. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-26 7:36 ` Juraj Linkeš @ 2023-05-26 13:24 ` Jeremy Spewock 0 siblings, 0 replies; 12+ messages in thread From: Jeremy Spewock @ 2023-05-26 13:24 UTC (permalink / raw) To: Juraj Linkeš; +Cc: dev [-- Attachment #1: Type: text/plain, Size: 50956 bytes --] On Fri, May 26, 2023 at 3:36 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > On Thu, May 25, 2023 at 8:03 PM Jeremy Spewock <jspewock@iol.unh.edu> > wrote: > > > > Hey Juraj, > > > > On Thu, May 25, 2023 at 4:33 AM Juraj Linkeš <juraj.linkes@pantheon.tech> > wrote: > >> > >> One more point that doesn't fit elsewhere: > >> > >> On Wed, May 24, 2023 at 10:45 PM Jeremy Spewock <jspewock@iol.unh.edu> > wrote: > >> > > >> > > >> > > >> > On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš > <juraj.linkes@pantheon.tech> wrote: > >> >> > >> >> Hi Jeremy, first, a few general points: > >> >> > >> >> 1. Send patches to maintainers (Thomas, me, Honnappa, Lijuan and > >> >> anyone else involved with DTS or who might be interested) and add the > >> >> devlist to cc. > >> > > >> > > >> > Thank you for the tip! I'm still new to sending patches and didn't > think to do something like this but I will in the future. > >> > > >> >> > >> >> 2. Run the linter script before submitting. > >> > > >> > > >> > I did forget to run this, I will in the future. > >> > > >> >> > >> >> 3. The use of the various nested objects breaks the current > >> >> abstractions. The basic idea is that the test suite developers should > >> >> ideally only use the sut/tg node objects and those objects should > >> >> delegate logic further to their nested objects. More below. > >> >> > >> >> I have many comments about the implementation, but I haven't run it > >> >> yet. I'm going to do that after this round of comments and I may have > >> >> more ideas. > >> >> > >> >> On Fri, May 12, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote: > >> >> > > >> >> > From: Jeremy Spewock <jspewock@iol.unh.edu> > >> >> > > >> >> > Adds a new test suite for running smoke tests that verify general > >> >> > configuration aspects of the system under test. If any of these > tests > >> >> > fail, the DTS execution terminates as part of a "fail-fast" model. > >> >> > > >> >> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> > >> >> > --- > >> >> > dts/conf.yaml | 9 ++ > >> >> > dts/framework/config/__init__.py | 21 +++++ > >> >> > dts/framework/config/conf_yaml_schema.json | 32 ++++++- > >> >> > dts/framework/dts.py | 19 +++- > >> >> > dts/framework/exception.py | 11 +++ > >> >> > dts/framework/remote_session/os_session.py | 6 +- > >> >> > .../remote_session/remote/__init__.py | 28 ++++++ > >> >> > dts/framework/test_result.py | 13 ++- > >> >> > dts/framework/test_suite.py | 24 ++++- > >> >> > dts/framework/testbed_model/__init__.py | 5 + > >> >> > .../interactive_apps/__init__.py | 6 ++ > >> >> > .../interactive_apps/interactive_command.py | 57 +++++++++++ > >> >> > .../interactive_apps/testpmd_driver.py | 24 +++++ > >> >> > dts/framework/testbed_model/node.py | 2 + > >> >> > dts/framework/testbed_model/sut_node.py | 6 ++ > >> >> > dts/tests/TestSuite_smoke_tests.py | 94 > +++++++++++++++++++ > >> >> > 16 files changed, 348 insertions(+), 9 deletions(-) > >> >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/__init__.py > >> >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/interactive_command.py > >> >> > create mode 100644 > dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> >> > >> >> Let's not add any more levels. I don't like even the current hw > >> >> subdirectory (which I want to remove in the docs patch) and we don't > >> >> need it. I'd also like to move this functionality into > remote_session, > >> >> as it's handling a type of remote session. > >> > > >> > > >> > I think it makes sense to add a proper wrapper for it, I just didn't > create a subclass for it off of remote_session because the idea behind it > was only allowing users to interact with the session through the > InteractiveScriptHandler/DPDK app handlers. If it were part of the > remote_session class it would have to include operations for sending > commands the way it is written now. I could do this but it seems like a > bigger discussion about whether interactive sessions should create a new > SSH session every time or instead just use one existing session and create > channels off of it. > >> > > >> > >> I wasn't clear, I meant to move the python modules into the > >> remote_session folder. The subclassing would be there only to have a > >> common API across these remote sessions (as the > >> InteractiveScriptHandler is a kind of a remote session). If at all > >> possible, this should be our aim. > > > > > > I see what you mean now, moving them there shouldn't be a problem. > > > >> > >> > >> >> > >> >> > >> >> > create mode 100644 dts/tests/TestSuite_smoke_tests.py > >> >> > > >> >> > diff --git a/dts/conf.yaml b/dts/conf.yaml > >> >> > index a9bd8a3e..042ef954 100644 > >> >> > --- a/dts/conf.yaml > >> >> > +++ b/dts/conf.yaml > >> >> > @@ -10,13 +10,22 @@ executions: > >> >> > compiler_wrapper: ccache > >> >> > perf: false > >> >> > func: true > >> >> > + nics: #physical devices to be used for testing > >> >> > + - addresses: > >> >> > + - "0000:11:00.0" > >> >> > + - "0000:11:00.1" > >> >> > + driver: "i40e" > >> >> > + vdevs: #names of virtual devices to be used for testing > >> >> > + - "crypto_openssl" > >> >> > >> >> I believe we specified the NICs under SUTs in the original DTS, just > >> >> as Owen did in his internal GitLab patch. If you can access it, have > a > >> >> look at how he did it. > >> >> This brings an interesting question of where we want to specify which > >> >> NICs/vdevs to test. It could be on the SUT level, but also on the > >> >> execution or even the build target level. This should be informed by > >> >> testing needs. What makes the most sense? We could specify NIC > details > >> >> per SUT/TG and then just reference the NICs on the execution/build > >> >> target level. > >> > > >> > > >> > I put it on the execution level with the thought that you might have > multiple NICs in your SUT but want to separate them into different > executions. > >> > >> This is a good point. Is this something you're interested in in the > >> lab? We should ask Lijuan what she thinks of this. In general, I like > >> this, so we should at least think of how to add NIC config > >> implementation so that we could add this later if we're not adding it > >> now. > >> > >> > I guess in the case of smoke tests, you'd only need to know them per > target because it was talked about previously that then is when we should > run the smoke tests. I think however it would make sense to specify the NIC > you are using for that execution rather than having to assume or specify > elsewhere. > >> > > >> > >> Nothing is set in stone, we don't have to run them per build target. > >> We could have two smoke test suites, one per execution and one per > >> build target. I actually like that a lot and we should explore that. > >> If it's not much extra work, we could just do the split like that. > >> > > > > I also like the sound of that. In theory it doesn't seem that hard, if > we made two different suites that tested different things all that needs to > change is when they are run. This would probably take some more planning as > to what we want to run at which points during the run and it might be > easier to get these tests in a better spot first. I do like the idea > though, and agree that it's worth exploring. > > > >> >> > >> >> > >> >> > test_suites: > >> >> > + - smoke_tests > >> >> > - hello_world > >> >> > system_under_test: "SUT 1" > >> >> > nodes: > >> >> > - name: "SUT 1" > >> >> > hostname: sut1.change.me.localhost > >> >> > user: root > >> >> > + password: "" > >> >> > >> >> This was deliberately left out to discourage the use of passwords. > >> >> > >> >> > arch: x86_64 > >> >> > os: linux > >> >> > lcores: "" > >> >> > >> >> <snip> > >> >> > >> >> > diff --git a/dts/framework/dts.py b/dts/framework/dts.py > >> >> > index 05022845..0d03e158 100644 > >> >> > --- a/dts/framework/dts.py > >> >> > +++ b/dts/framework/dts.py > >> >> > @@ -5,6 +5,8 @@ > >> >> > > >> >> > import sys > >> >> > > >> >> > +from .exception import BlockingTestSuiteError > >> >> > + > >> >> > from .config import CONFIGURATION, BuildTargetConfiguration, > ExecutionConfiguration > >> >> > from .logger import DTSLOG, getLogger > >> >> > from .test_result import BuildTargetResult, DTSResult, > ExecutionResult, Result > >> >> > @@ -49,6 +51,7 @@ def run_all() -> None: > >> >> > nodes[sut_node.name] = sut_node > >> >> > > >> >> > if sut_node: > >> >> > + #SMOKE TEST EXECUTION GOES HERE! > >> >> > _run_execution(sut_node, execution, result) > >> >> > > >> >> > except Exception as e: > >> >> > @@ -118,7 +121,7 @@ def _run_build_target( > >> >> > > >> >> > try: > >> >> > sut_node.set_up_build_target(build_target) > >> >> > - result.dpdk_version = sut_node.dpdk_version > >> >> > + # result.dpdk_version = sut_node.dpdk_version > >> >> > build_target_result.update_setup(Result.PASS) > >> >> > except Exception as e: > >> >> > dts_logger.exception("Build target setup failed.") > >> >> > @@ -146,6 +149,7 @@ def _run_suites( > >> >> > with possibly only a subset of test cases. > >> >> > If no subset is specified, run all test cases. > >> >> > """ > >> >> > + end_execution = False > >> >> > >> >> This only ends the build target stage, not the execution stage. We > >> >> should either find a better name for the variable or make sure that > >> >> the whole execution is blocked. I think we're aiming for the latter - > >> >> maybe we could just check whether the last exception in result is a > >> >> BlockingTestSuiteError (or better, just call a new method of result > >> >> that will check that), which we could do at multiple stages. > >> > > >> > > >> > Good catch. When writing this and figuring out how to get it to work, > I was thinking about smoke tests on a per build target basis because if it > failed on one build target it could theoretically pass on another, but > you'd likely want your entire execution to fail than have partial results. > >> > > >> > >> Well, these will be well defined partial results (and definitely > >> valuable, imagine one build target passing and one failing, we could > >> easily compare the two to sometimes quickly identify the failure), so > >> if there's a possibility that the test would pass on another build > >> target, we should try to run the build target. > >> > >> In this case, let's just rename the variable to end_build_target or > >> something similar. > >> > > > > Alright, will do. > > > >> > >> >> > >> >> > >> >> > for test_suite_config in execution.test_suites: > >> >> > try: > >> >> > full_suite_path = > f"tests.TestSuite_{test_suite_config.test_suite}" > >> >> > @@ -160,13 +164,24 @@ def _run_suites( > >> >> > > >> >> > else: > >> >> > for test_suite_class in test_suite_classes: > >> >> > + #HERE NEEDS CHANGING > >> >> > test_suite = test_suite_class( > >> >> > sut_node, > >> >> > test_suite_config.test_cases, > >> >> > execution.func, > >> >> > build_target_result, > >> >> > + sut_node._build_target_config, > >> >> > + result > >> >> > ) > >> >> > - test_suite.run() > >> >> > + try: > >> >> > + test_suite.run() > >> >> > + except BlockingTestSuiteError as e: > >> >> > + dts_logger.exception("An error occurred > within a blocking TestSuite, execution will now end.") > >> >> > + result.add_error(e) > >> >> > + end_execution = True > >> >> > + #if a blocking test failed and we need to bail out of > suite executions > >> >> > + if end_execution: > >> >> > + break > >> >> > > >> >> > > >> >> > def _exit_dts() -> None: > >> >> > diff --git a/dts/framework/exception.py > b/dts/framework/exception.py > >> >> > index ca353d98..4e3f63d1 100644 > >> >> > --- a/dts/framework/exception.py > >> >> > +++ b/dts/framework/exception.py > >> >> > @@ -25,6 +25,7 @@ class ErrorSeverity(IntEnum): > >> >> > SSH_ERR = 4 > >> >> > DPDK_BUILD_ERR = 10 > >> >> > TESTCASE_VERIFY_ERR = 20 > >> >> > + BLOCKING_TESTSUITE_ERR = 25 > >> >> > > >> >> > > >> >> > class DTSError(Exception): > >> >> > @@ -144,3 +145,13 @@ def __init__(self, value: str): > >> >> > > >> >> > def __str__(self) -> str: > >> >> > return repr(self.value) > >> >> > + > >> >> > +class BlockingTestSuiteError(DTSError): > >> >> > + suite_name: str > >> >> > + severity: ClassVar[ErrorSeverity] = > ErrorSeverity.BLOCKING_TESTSUITE_ERR > >> >> > + > >> >> > + def __init__(self, suite_name:str) -> None: > >> >> > + self.suite_name = suite_name > >> >> > + > >> >> > + def __str__(self) -> str: > >> >> > + return f"Blocking suite {self.suite_name} failed." > >> >> > diff --git a/dts/framework/remote_session/os_session.py > b/dts/framework/remote_session/os_session.py > >> >> > index 4c48ae25..22776bc1 100644 > >> >> > --- a/dts/framework/remote_session/os_session.py > >> >> > +++ b/dts/framework/remote_session/os_session.py > >> >> > @@ -12,7 +12,9 @@ > >> >> > from framework.testbed_model import LogicalCore > >> >> > from framework.utils import EnvVarsDict, MesonArgs > >> >> > > >> >> > -from .remote import CommandResult, RemoteSession, > create_remote_session > >> >> > +from .remote import CommandResult, RemoteSession, > create_remote_session, create_interactive_session > >> >> > + > >> >> > +from paramiko import SSHClient > >> >> > > >> >> > > >> >> > class OSSession(ABC): > >> >> > @@ -26,6 +28,7 @@ class OSSession(ABC): > >> >> > name: str > >> >> > _logger: DTSLOG > >> >> > remote_session: RemoteSession > >> >> > + _interactive_session: SSHClient > >> >> > > >> >> > def __init__( > >> >> > self, > >> >> > @@ -37,6 +40,7 @@ def __init__( > >> >> > self.name = name > >> >> > self._logger = logger > >> >> > self.remote_session = create_remote_session(node_config, > name, logger) > >> >> > + self._interactive_session = > create_interactive_session(node_config, name, logger) > >> >> > >> >> This session should be stored in SutNode (it should be os-agnostic > >> >> (the apps should behave the same on all os's, right?) and SutNode > >> >> could use it without accessing another object), but created in > >> >> OSSession (this way we make sure any os-specific inputs (such as > >> >> paths) are passed properly). > >> > > >> > > >> > I can move it into SutNode, the main reason I left it there is > because I was essentially thinking of it the same way as the remote session > that gets created once per node. However, I could just as easily have > SutNode store it and call upon OSSession to create one inside its > constructor. This would make creating classes for DPDK apps easier as well > once those are subclasses of the InteractiveScriptHandler. > >> > > >> > >> I was thinking of this interactive session more like > >> Node._other_sessions, created only when needed. Now that I think about > >> it, it's always going to be needed if we use it in smoke tests, so we > >> might as well create it in OSSession (or its subclasses if needed). > >> > >> Thinking further, we want to use only the app driver object in test > >> suites. So the SutNode object should have a method that returns the > >> object and the rest of the implementation needs to be delegated to the > >> rest of the objects in the hierarchy to ensure proper remote OS > >> handling. > > > > > > I like the sound of this. If we use a method and something like an enum > inside of SutNode as well to create these objects then we don't even really > need a method to return this object because it would never be interacted > with directly. > > > >> > >> > >> >> > >> >> > > >> >> > def close(self, force: bool = False) -> None: > >> >> > """ > >> >> > diff --git a/dts/framework/remote_session/remote/__init__.py > b/dts/framework/remote_session/remote/__init__.py > >> >> > index 8a151221..abca8edc 100644 > >> >> > --- a/dts/framework/remote_session/remote/__init__.py > >> >> > +++ b/dts/framework/remote_session/remote/__init__.py > >> >> > @@ -9,8 +9,36 @@ > >> >> > from .remote_session import CommandResult, RemoteSession > >> >> > from .ssh_session import SSHSession > >> >> > > >> >> > +from paramiko import SSHClient, AutoAddPolicy > >> >> > +from framework.utils import GREEN > >> >> > > >> >> > def create_remote_session( > >> >> > node_config: NodeConfiguration, name: str, logger: DTSLOG > >> >> > ) -> RemoteSession: > >> >> > return SSHSession(node_config, name, logger) > >> >> > + > >> >> > +def create_interactive_session( > >> >> > + node_config: NodeConfiguration, name: str, logger: DTSLOG > >> >> > +) -> SSHClient: > >> >> > + """ > >> >> > + Creates a paramiko SSH session that is designed to be used > for interactive shells > >> >> > + > >> >> > + This session is meant to be used on an "as needed" basis and > may never be utilized > >> >> > + """ > >> >> > + client: SSHClient = SSHClient() > >> >> > + client.set_missing_host_key_policy(AutoAddPolicy) > >> >> > + ip: str = node_config.hostname > >> >> > + logger.info(GREEN(f"Connecting to host {ip}")) > >> >> > >> >> Using colors is a remnant from the original DTS. If we want to > >> >> (re-)introduce colors I'd do that in a separate patch in a uniform > >> >> manner. > >> > > >> > > >> > I agree this is likely outside the scope of this patch and the color > here really isn't necessary either so I'll remove it. > >> > > >> >> > >> >> > >> >> > + #Preset to 22 because paramiko doesn't accept None > >> >> > + port: int = 22 > >> >> > >> >> This configuration handling should be moved to NodeConfiguration. > >> >> The logic should also be moved to InteractiveScriptHandler. We also > >> >> probably don't need a factory for this. > >> > > >> > > >> > I agree that the configuration could be moved into NodeConfiguration, > however the reason I use a factory and don't have this connect logic inside > InteractiveScriptHandler is because that would mean there would have to be > an SSH session per application. This is one potential solution, but what I > am doing here instead is creating one SSH session for interactive scripts > at the start alongside the usual remote session. This session essentially > exists in the background for the duration of the execution and then each > application for DPDK creates a channel off that session when they are > created. Then, when you are done using the application, it closes the > channel but the session itself stays open. > >> > > >> > >> Ok, I see. What do you think about this: > >> Let's then move the session creation into a subclass of RemoteSession. > >> The session would be stored as a class variable and with that we could > >> subclass further: > >> RemoteSession -> new subclass -> InteractiveScriptHandler -> DpdkApp > >> > >> Everything could be handled from within the DdpdkApp objects, but we'd > >> need to be careful about cleanup (maybe we'd only need to check that > >> there are no open channels when closing the session). > >> > >> Or alternatively, the new subclass could just have a method that > >> returns DpdkApps. We'd have two different class relations: > >> RemoteSession -> new subclass and InteractiveScriptHandler -> DpdkApp. > >> > >> I'd put all of the classes into the remote_session directory. > >> > > > > I like the idea of making a subclass of the remote session and then a > method in the subclass that generates DpdkApps. This way you have a proper > class that maintains the interactive session and InteractiveScriptHandlers > as well as any other DpdkApp can be created and destroyed as needed. The > only reason I wouldn't make InteractiveScriptHandler a subclass of > RemoteSession is because then it would make the InteractiveScriptHandler > something that stored the main SSH session rather than something that just > uses it to make a channel. > > > > Originally, I avoided this because I didn't want to implement the > send_command methods from inside RemoteSession, but a solution to this > could just be creating an InteractiveScriptHandler, sending the command > blindly, and not returning the output. Or maybe there would be some kind of > default prompt I could expect to still collect the output, but I'm not sure > there would be something that is os-agnostic for this. I'm not sure the > best way to show that I don't want people to use the method on the new > subclass that I'm going to create though. I guess one way to handle this > would be just not creating a method that returns this subclass in SutNode. > SutNode could just have a method that returns DPDK apps which should > discourage touching this session directly. > > > > Ah right, the fact that the workflow with interactive sessions is a > bit different doesn't make subclassing RemoteSession ideal - the > abstract methods _send_command and copy_file don't make much sense in > the new class. > > With that in mind, I think we have two options: > 1. Don't use one session with multiple channels. This would make it > more in line with RemoteSession, but the copy_file method still > doesn't make sense, so I prefer 2. > 2. Don't subclass RemoteSession as it's a bit too different from what > we need. Just create a new class (named InteractiveRemoteSession > perpahs) that would basically be a factory for drivers (with two parts > - ssh session init and then the creation of driver/app objects). The > InteractiveScriptHandler (I'm thinking of renaming it to > InteractiveShell now) -> DpdkApp would be the other part. I'd put > InteractiveRemoteSession and InteractiveScriptHandler/InteractiveShell > into the same file (those two basically define the API on the user and > developer side). > I think I also prefer 2, I can work on putting that together. These comments definitely gave me a lot of good insight into where to go from here though, thank you! > > >> > >> >> > >> >> > >> >> > + if ":" in node_config.hostname: > >> >> > + ip, port = node_config.hostname.split(":") > >> >> > + port = int(port) > >> >> > + client.connect( > >> >> > + ip, > >> >> > + username=node_config.user, > >> >> > + port=port, > >> >> > + password=node_config.password or "", > >> >> > + timeout=20 if port else 10 > >> >> > + ) > >> >> > + return client > >> >> > diff --git a/dts/framework/test_result.py > b/dts/framework/test_result.py > >> >> > index 74391982..77202ae2 100644 > >> >> > --- a/dts/framework/test_result.py > >> >> > +++ b/dts/framework/test_result.py > >> >> > @@ -8,6 +8,7 @@ > >> >> > import os.path > >> >> > from collections.abc import MutableSequence > >> >> > from enum import Enum, auto > >> >> > +from typing import Dict > >> >> > > >> >> > from .config import ( > >> >> > OS, > >> >> > @@ -67,12 +68,13 @@ class Statistics(dict): > >> >> > Using a dict provides a convenient way to format the data. > >> >> > """ > >> >> > > >> >> > - def __init__(self, dpdk_version): > >> >> > + def __init__(self, output_info: Dict[str, str] | None): > >> >> > super(Statistics, self).__init__() > >> >> > for result in Result: > >> >> > self[result.name] = 0 > >> >> > self["PASS RATE"] = 0.0 > >> >> > - self["DPDK VERSION"] = dpdk_version > >> >> > + if output_info: > >> >> > + for info_key, info_val in output_info.items(): > self[info_key] = info_val > >> >> > > >> >> > def __iadd__(self, other: Result) -> "Statistics": > >> >> > """ > >> >> > @@ -258,6 +260,7 @@ class DTSResult(BaseResult): > >> >> > """ > >> >> > > >> >> > dpdk_version: str | None > >> >> > + output: dict | None > >> >> > _logger: DTSLOG > >> >> > _errors: list[Exception] > >> >> > _return_code: ErrorSeverity > >> >> > @@ -267,6 +270,7 @@ class DTSResult(BaseResult): > >> >> > def __init__(self, logger: DTSLOG): > >> >> > super(DTSResult, self).__init__() > >> >> > self.dpdk_version = None > >> >> > + self.output = None > >> >> > self._logger = logger > >> >> > self._errors = [] > >> >> > self._return_code = ErrorSeverity.NO_ERR > >> >> > @@ -296,7 +300,10 @@ def process(self) -> None: > >> >> > for error in self._errors: > >> >> > self._logger.debug(repr(error)) > >> >> > > >> >> > - self._stats_result = Statistics(self.dpdk_version) > >> >> > + self._stats_result = Statistics(self.output) > >> >> > + #add information gathered from the smoke tests to the > statistics > >> >> > + # for info_key, info_val in smoke_test_info.items(): > self._stats_result[info_key] = info_val > >> >> > + # print(self._stats_result) > >> >> > self.add_stats(self._stats_result) > >> >> > with open(self._stats_filename, "w+") as stats_file: > >> >> > stats_file.write(str(self._stats_result)) > >> >> > diff --git a/dts/framework/test_suite.py > b/dts/framework/test_suite.py > >> >> > index 0705f38f..1518fb8a 100644 > >> >> > --- a/dts/framework/test_suite.py > >> >> > +++ b/dts/framework/test_suite.py > >> >> > @@ -10,11 +10,14 @@ > >> >> > import inspect > >> >> > import re > >> >> > from types import MethodType > >> >> > +from typing import Dict > >> >> > > >> >> > -from .exception import ConfigurationError, SSHTimeoutError, > TestCaseVerifyError > >> >> > +from .config import BuildTargetConfiguration > >> >> > + > >> >> > +from .exception import BlockingTestSuiteError, > ConfigurationError, SSHTimeoutError, TestCaseVerifyError > >> >> > from .logger import DTSLOG, getLogger > >> >> > from .settings import SETTINGS > >> >> > -from .test_result import BuildTargetResult, Result, > TestCaseResult, TestSuiteResult > >> >> > +from .test_result import BuildTargetResult, DTSResult, Result, > TestCaseResult, TestSuiteResult > >> >> > from .testbed_model import SutNode > >> >> > > >> >> > > >> >> > @@ -37,10 +40,12 @@ class TestSuite(object): > >> >> > """ > >> >> > > >> >> > sut_node: SutNode > >> >> > + is_blocking = False > >> >> > _logger: DTSLOG > >> >> > _test_cases_to_run: list[str] > >> >> > _func: bool > >> >> > _result: TestSuiteResult > >> >> > + _dts_result: DTSResult > >> >> > > >> >> > def __init__( > >> >> > self, > >> >> > @@ -48,6 +53,8 @@ def __init__( > >> >> > test_cases: list[str], > >> >> > func: bool, > >> >> > build_target_result: BuildTargetResult, > >> >> > + build_target_conf: BuildTargetConfiguration, > >> >> > + dts_result: DTSResult > >> >> > ): > >> >> > self.sut_node = sut_node > >> >> > self._logger = getLogger(self.__class__.__name__) > >> >> > @@ -55,6 +62,8 @@ def __init__( > >> >> > self._test_cases_to_run.extend(SETTINGS.test_cases) > >> >> > self._func = func > >> >> > self._result = > build_target_result.add_test_suite(self.__class__.__name__) > >> >> > + self.build_target_info = build_target_conf > >> >> > + self._dts_result = dts_result > >> >> > > >> >> > def set_up_suite(self) -> None: > >> >> > """ > >> >> > @@ -118,6 +127,9 @@ def run(self) -> None: > >> >> > f"the next test suite may be affected." > >> >> > ) > >> >> > self._result.update_setup(Result.ERROR, e) > >> >> > + if len(self._result.get_errors()) > 0 and > self.is_blocking: > >> >> > + raise BlockingTestSuiteError(test_suite_name) > >> >> > + > >> >> > > >> >> > def _execute_test_suite(self) -> None: > >> >> > """ > >> >> > @@ -137,6 +149,7 @@ def _execute_test_suite(self) -> None: > >> >> > f"Attempt number {attempt_nr} out of > {all_attempts}." > >> >> > ) > >> >> > self._run_test_case(test_case_method, > test_case_result) > >> >> > + > >> >> > > >> >> > def _get_functional_test_cases(self) -> list[MethodType]: > >> >> > """ > >> >> > @@ -232,6 +245,11 @@ def _execute_test_case( > >> >> > test_case_result.update(Result.SKIP) > >> >> > raise KeyboardInterrupt("Stop DTS") > >> >> > > >> >> > + def write_to_statistics_file(self, output: Dict[str, str]): > >> >> > + if self._dts_result.output != None: > >> >> > + self._dts_result.output.update(output) > >> >> > + else: > >> >> > + self._dts_result.output = output > >> >> > > >> >> > def get_test_suites(testsuite_module_path: str) -> > list[type[TestSuite]]: > >> >> > def is_test_suite(object) -> bool: > >> >> > @@ -252,3 +270,5 @@ def is_test_suite(object) -> bool: > >> >> > test_suite_class > >> >> > for _, test_suite_class in > inspect.getmembers(testcase_module, is_test_suite) > >> >> > ] > >> >> > + > >> >> > + > >> >> > diff --git a/dts/framework/testbed_model/__init__.py > b/dts/framework/testbed_model/__init__.py > >> >> > index f54a9470..63f17cc3 100644 > >> >> > --- a/dts/framework/testbed_model/__init__.py > >> >> > +++ b/dts/framework/testbed_model/__init__.py > >> >> > @@ -20,3 +20,8 @@ > >> >> > ) > >> >> > from .node import Node > >> >> > from .sut_node import SutNode > >> >> > + > >> >> > +from .interactive_apps import ( > >> >> > + InteractiveScriptHandler, > >> >> > + TestpmdDriver > >> >> > +) > >> >> > diff --git > a/dts/framework/testbed_model/interactive_apps/__init__.py > b/dts/framework/testbed_model/interactive_apps/__init__.py > >> >> > new file mode 100644 > >> >> > index 00000000..0382d7e0 > >> >> > --- /dev/null > >> >> > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > >> >> > @@ -0,0 +1,6 @@ > >> >> > +from .interactive_command import ( > >> >> > + InteractiveScriptHandler > >> >> > +) > >> >> > +from .testpmd_driver import ( > >> >> > + TestpmdDriver > >> >> > +) > >> >> > \ No newline at end of file > >> >> > diff --git > a/dts/framework/testbed_model/interactive_apps/interactive_command.py > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > >> >> > new file mode 100644 > >> >> > index 00000000..7467911b > >> >> > --- /dev/null > >> >> > +++ > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > >> >> > >> >> In general, the file name should match the class name. > >> >> > >> >> > @@ -0,0 +1,57 @@ > >> >> > +# import paramiko > >> >> > +from paramiko import SSHClient, Channel, channel > >> >> > +from framework.settings import SETTINGS > >> >> > + > >> >> > +class InteractiveScriptHandler: > >> >> > >> >> Once merged with the init functionality and moved to remote_session, > >> >> I'd rename it to InteractiveAppSession or something similar. > >> > > >> > > >> > Good point, I'll rename the class and the file. > >> > > >> >> > >> >> > >> >> > + > >> >> > + _ssh_client: SSHClient > >> >> > + _stdin: channel.ChannelStdinFile > >> >> > + _ssh_channel: Channel > >> >> > + > >> >> > + def __init__(self, ssh_client: SSHClient, timeout:float = > SETTINGS.timeout) -> None: > >> >> > + self._ssh_client = ssh_client > >> >> > + self._ssh_channel = self._ssh_client.invoke_shell() > >> >> > + self._stdin = self._ssh_channel.makefile_stdin("wb") > >> >> > + self._ssh_channel.settimeout(timeout) > >> >> > + > >> >> > + def send_command(self, command:str) -> None: > >> >> > + """ > >> >> > + Send command to channel without recording output. > >> >> > + > >> >> > + This method will not verify any input or output, it will > >> >> > + simply assume the command succeeded > >> >> > + """ > >> >> > + self._stdin.write(command + '\n') > >> >> > + self._stdin.flush() > >> >> > + > >> >> > + def send_command_get_output(self, command:str, expect:str) -> > str: > >> >> > >> >> Let's rename expect to prompt. At least for me it was just confusing > >> >> in the original DTS. > >> > > >> > > >> > It definitely can be confusing, I'll change it accordingly as you're > right, prompt is more clear. > >> > > >> >> > >> >> > >> >> > + """ > >> >> > + Send a command and get all output before the expected > ending string. > >> >> > + > >> >> > + **NOTE** > >> >> > + Lines that expect input are not included in the stdout > buffer so they cannot be > >> >> > + used for expect. For example, if you were prompted to log > into something > >> >> > + with a username and password, you cannot expect > "username:" because it wont > >> >> > + yet be in the stdout buffer. A work around for this could > be consuming an > >> >> > + extra newline character to force the current prompt into > the stdout buffer. > >> >> > + > >> >> > + *Return* > >> >> > + All output before expected string > >> >> > + """ > >> >> > + stdout = self._ssh_channel.makefile("r") > >> >> > + self._stdin.write(command + '\n') > >> >> > + self._stdin.flush() > >> >> > + out:str = "" > >> >> > + for line in stdout: > >> >> > + out += str(line) > >> >> > + if expect in str(line): > >> >> > + break > >> >> > + stdout.close() #close the buffer to flush the output > >> >> > + return out > >> >> > + > >> >> > + def close(self): > >> >> > + self._stdin.close() > >> >> > + self._ssh_channel.close() > >> >> > + > >> >> > + def __del__(self): > >> >> > + self.close() > >> >> > diff --git > a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> >> > new file mode 100644 > >> >> > index 00000000..1993eae6 > >> >> > --- /dev/null > >> >> > +++ > b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > >> >> > @@ -0,0 +1,24 @@ > >> >> > +from framework.testbed_model.interactive_apps import > InteractiveScriptHandler > >> >> > + > >> >> > +from pathlib import PurePath > >> >> > + > >> >> > +class TestpmdDriver: > >> >> > >> >> This could also be called TestPmdSession. Not sure which is better. > >> >> This should extend InteractiveScriptHandler, which should contain > >> >> basic functionality common to all apps and then the drivers would > >> >> contain app-specific functionality. > >> > > >> > > >> > Originally the reason I avoided doing this was because I viewed them > as separate layers of the process essentially. I wrote it in a way where > the InteractiveScriptHandler was almost like an interface for any CLI and > then DPDK apps could interact with this interface, but after thinking about > it more, this isn't necessary. There is no reason really that the > applications themselves can't just use these methods directly and it also > avoids the need to have a reference to the handler within the object. > >> > > >> >> > >> >> > >> >> > + prompt:str = "testpmd>" > >> >> > + interactive_handler: InteractiveScriptHandler > >> >> > + > >> >> > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> > None: > >> >> > + """ > >> >> > + Sets the handler to drive the SSH session and starts > testpmd > >> >> > + """ > >> >> > + self.interactive_handler = handler > >> >> > + # self.interactive_handler.send_command("sudo su") > >> >> > + # self.interactive_handler.send_command("cd > /root/testpmd-testing/dpdk/build") > >> >> > + > self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > >> >> > >> >> The paths need to be handled in os-agnostic manner in os_session and > >> >> then passed here. > >> >> > >> >> > + > >> >> > + def send_command(self, command:str, expect:str = prompt) -> > str: > >> >> > + """ > >> >> > + Specific way of handling the command for testpmd > >> >> > + > >> >> > + An extra newline character is consumed in order to force > the current line into the stdout buffer > >> >> > + """ > >> >> > + return > self.interactive_handler.send_command_get_output(command + "\n", expect) > >> >> > \ No newline at end of file > >> >> > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_model/node.py > >> >> > index d48fafe6..c5147e0e 100644 > >> >> > --- a/dts/framework/testbed_model/node.py > >> >> > +++ b/dts/framework/testbed_model/node.py > >> >> > @@ -40,6 +40,7 @@ class Node(object): > >> >> > lcores: list[LogicalCore] > >> >> > _logger: DTSLOG > >> >> > _other_sessions: list[OSSession] > >> >> > + _execution_config: ExecutionConfiguration > >> >> > > >> >> > def __init__(self, node_config: NodeConfiguration): > >> >> > self.config = node_config > >> >> > @@ -64,6 +65,7 @@ def set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > >> >> > """ > >> >> > self._setup_hugepages() > >> >> > self._set_up_execution(execution_config) > >> >> > + self._execution_config = execution_config > >> >> > > >> >> > def _set_up_execution(self, execution_config: > ExecutionConfiguration) -> None: > >> >> > """ > >> >> > diff --git a/dts/framework/testbed_model/sut_node.py > b/dts/framework/testbed_model/sut_node.py > >> >> > index 2b2b50d9..8c39a66d 100644 > >> >> > --- a/dts/framework/testbed_model/sut_node.py > >> >> > +++ b/dts/framework/testbed_model/sut_node.py > >> >> > @@ -14,6 +14,7 @@ > >> >> > > >> >> > from .hw import LogicalCoreCount, LogicalCoreList, VirtualDevice > >> >> > from .node import Node > >> >> > +from .interactive_apps import InteractiveScriptHandler > >> >> > > >> >> > > >> >> > class SutNode(Node): > >> >> > @@ -261,6 +262,11 @@ def run_dpdk_app( > >> >> > return self.main_session.send_command( > >> >> > f"{app_path} {eal_args}", timeout, verify=True > >> >> > ) > >> >> > + def create_interactive_session_handler(self) -> > InteractiveScriptHandler: > >> >> > + """ > >> >> > + Create a handler for interactive sessions > >> >> > + """ > >> >> > + return > InteractiveScriptHandler(self.main_session._interactive_session) > >> >> > > >> >> > > >> >> > class EalParameters(object): > >> >> > diff --git a/dts/tests/TestSuite_smoke_tests.py > b/dts/tests/TestSuite_smoke_tests.py > >> >> > new file mode 100644 > >> >> > index 00000000..bacf289d > >> >> > --- /dev/null > >> >> > +++ b/dts/tests/TestSuite_smoke_tests.py > >> >> > @@ -0,0 +1,94 @@ > >> >> > +from framework.test_suite import TestSuite > >> >> > +from framework.testbed_model.sut_node import SutNode > >> >> > + > >> >> > +from framework.testbed_model.interactive_apps import TestpmdDriver > >> >> > + > >> >> > +def get_compiler_version(compiler_name: str, sut_node: SutNode) > -> str: > >> >> > >> >> I don't see a reason why this is outside SmokeTest. > >> >> > >> >> > >> >> > + match compiler_name: > >> >> > + case "gcc": > >> >> > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > >> >> > >> >> As I alluded to earlier, the call here should be > >> >> sut_node.get_compiler_version(). This is to hide implementation > >> >> details from test suite developers. > >> >> Then, SutNode.get_compiler_version should then call > >> >> self.main_session.get_compiler_version(). The reason here is this is > >> >> an os-agnostic call. > >> >> get_compiler_version() should then be defined in os_session and > >> >> implemented in os specific classes. > >> > > >> > > >> > I originally placed the method outside of smoke tests just so it > wouldn't be run with the rest of the suite but it does make sense to make > this and the other comments os-agnostic. > >> > > >> >> > >> >> > >> >> > + case "clang": > >> >> > + return > sut_node.main_session.send_command(f"{compiler_name} --version", > 60).stdout.split("\n")[0] > >> >> > + case "msvc": > >> >> > + return sut_node.main_session.send_command(f"cl", > 60).stdout > >> >> > + case "icc": > >> >> > + return > sut_node.main_session.send_command(f"{compiler_name} -V", 60).stdout > >> >> > + > >> >> > +class SmokeTests(TestSuite): > >> >> > + is_blocking = True > >> >> > + > >> >> > + def set_up_suite(self) -> None: > >> >> > + """ > >> >> > + Setup: > >> >> > + build all DPDK > >> >> > + """ > >> >> > + self.dpdk_build_dir_path = > self.sut_node.remote_dpdk_build_dir > >> >> > + > >> >> > + > >> >> > + def test_unit_tests(self) -> None: > >> >> > + """ > >> >> > + Test: > >> >> > + run the fast-test unit-test suite through meson > >> >> > + """ > >> >> > + self.sut_node.main_session.send_command(f"meson test -C > {self.dpdk_build_dir_path} --suite fast-tests", 300) > >> >> > >> >> Same here, there already are methods that build dpdk. If anything > >> >> needs to be added, we should expand those methods. > >> > > >> > > >> > I don't think this is building DPDK, this is just running fast tests > and ensuring that you are in the correct directory. > >> > > >> > >> Ah, right. These would be probably used only in smoke tests, so it's > >> probably fine to leave them here. Maybe we could add a method to the > >> suite? I'm thinking it would make the code a bit more readable. > >> > > > > I could look into this. > > > >> > >> >> > >> >> > >> >> > + > >> >> > + def test_driver_tests(self) -> None: > >> >> > + """ > >> >> > + Test: > >> >> > + run the driver-test unit-test suite through meson > >> >> > + """ > >> >> > + list_of_vdevs = "" > >> >> > + for dev in self.sut_node._execution_config.vdevs: > >> >> > + list_of_vdevs += f"{dev}," > >> >> > + print(list_of_vdevs) > >> >> > + if len(list_of_vdevs) > 0: > >> >> > + self.sut_node.main_session.send_command(f"meson test > -C {self.dpdk_build_dir_path} --suite driver-tests --test-args \"--vdev > {list_of_vdevs}\"", 300) > >> >> > + else: > >> >> > + self.sut_node.main_session.send_command(f"meson test > -C {self.dpdk_build_dir_path} --suite driver-tests", 300) > >> >> > + > >> >> > + def test_gather_info(self) -> None: > >> >> > + """ > >> >> > + Test: > >> >> > + gather information about the system and send output > to statistics.txt > >> >> > + """ > >> >> > + out = {} > >> >> > + > >> >> > + out['OS'] = self.sut_node.main_session.send_command("awk > -F= '$1==\"NAME\" {print $2}' /etc/os-release", 60).stdout > >> >> > + out["OS VERSION"] = > self.sut_node.main_session.send_command("awk -F= '$1==\"VERSION\" {print > $2}' /etc/os-release", 60, True).stdout > >> >> > + out["COMPILER VERSION"] = get_compiler_version( > self.build_target_info.compiler.name, self.sut_node) > >> >> > + out["DPDK VERSION"] = self.sut_node.dpdk_version > >> >> > + if self.build_target_info.os.name == "linux": > >> >> > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -r", 60).stdout > >> >> > + elif self.build_target_info.os.name == "windows": > >> >> > + out['KERNEL VERSION'] = > self.sut_node.main_session.send_command("uname -a", 60).stdout > >> >> > + self.write_to_statistics_file(out) > >> >> > + > >> >> > >> >> This is not really a test. If we want to add this, it should be > stored > >> >> elsewhere (in their respective objects, probably in result objects). > >> >> Three of these (os, os and kernel versions) are node data, the > >> >> remaining two are build target data. > >> >> I'm not sure it's a good idea to put these into statistics, as the > >> >> statistics are aggregated over all executions and build targets. In > >> >> case of multiple SUTs (in different executions) and multiple build > >> >> targets we'd record misleading data. We could include data from all > >> >> build targets and SUTs though. > >> >> The reason we (and original DTS) are storing DPDK version in the > >> >> current manner is that that doesn't change across anything, we're > >> >> always testing the same tarball. > >> > > >> > > >> > You're right that this isn't really testing anything, I had > originally included it here because it was just part of the smoke test > outline. It's definitely out of place though and I can move it out to their > respective classes. I think it might make sense to organize/label data > based on the SUT it comes from, I just thought statistics made the most > sense because you could see the test statistics as well as what it was > testing in one place. > >> > > >> > >> We could put anything we want into statistics (it's not a good idea > >> now, but if we change the format of statistics it'd be fine, but I > >> think changing the format is out of the scope of this patch) and this > >> would make sense there, but the first step is properly storing the > >> data. Moving them to statistics would be trivial then. > >> > > > > Right, that makes sense. > > > >> > >> >> > >> >> > >> >> > + > >> >> > + > >> >> > + def test_start_testpmd(self) -> None: > >> >> > + """ > >> >> > + Creates and instance of the testpmd driver to run the > testpmd app > >> >> > + """ > >> >> > + driver: TestpmdDriver = > TestpmdDriver(self.sut_node.create_interactive_session_handler(), > self.dpdk_build_dir_path) > >> >> > >> >> The driver should be returned by a method of self.sut_node. > >> > > >> > > >> > Does it make more sense to have methods for creating handlers for > various DPDK apps in SutNode? I had assumed it would be cleaner to just > make the classes as needed so that you don't have multiple methods doing > something very similar in SutNode (basically just making a class with a > different name that takes in similar information). I suppose that if you > were trying to have developers only interact with the SutNode class this > makes sense. > >> > > >> > >> That's what I meant. The method would return the appropriate app based > >> on input (which could be an enum) - I think that's preferable to > >> having a method for each app. We should think about non-interactive > >> apps here as well (such as the helloworld app that's already in use). > >> > > > > I like this idea for how to handle the creation of the DpdkApps. > > > >> > >> > This is also something that could make good use of having the > SSHClient in the SutNode and I could see how it would be useful. > >> > > >> >> > >> >> > >> >> > + > >> >> > + print(driver.send_command("show port summary all")) > >> >> > + > >> >> > + def test_device_bound_to_driver(self) -> None: > >> >> > + """ > >> >> > + Test: > >> >> > + ensure that all drivers listed in the config are > bound to the correct drivers > >> >> > + """ > >> >> > + for nic in self.sut_node._execution_config.nics: > >> >> > + for address in nic.addresses: > >> >> > + out = > self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py > --status | grep {address}", 60) > >> >> > >> >> This should follow the same abstractions as I outlined above. > >> >> However, we don't need to use dpdk-devbind here. It's probably safe > to > >> >> do so (the script is likely not going to be changed), but we could > >> > > >> > > >> > That's a good point about dev-bind, it might be better here to just > avoid the reliance on another script. > >> > > >> >> > >> >> > >> >> > + self.verify( > >> >> > + len(out.stdout) != 0, > >> >> > + f"Failed to find configured device > ({address}) using dpdk-devbind.py", > >> >> > + ) > >> >> > + for string in out.stdout.split(" "): > >> >> > + if 'drv=' in string: > >> >> > + self.verify( > >> >> > + string.split("=")[1] == > nic.driver.strip(), > >> >> > + f'Driver for device {address} does > not match driver listed in configuration (bound to {string.split("=")[1]})', > >> >> > + ) > >> >> > + > >> >> > \ No newline at end of file > >> >> > -- > >> >> > 2.40.1 > >> >> > > >> > > >> > > >> > I'll work on these changes, but I'd like to hear what you think about > what I had mentioned about moving the connect logic to the > InteractiveScriptHandler in the comments above. I had originally written it > to use one session throughout rather than opening and closing SSH > connections for every application but I'd like to hear which you think > would be easier/better if there is a difference. > >> > >> Yes, let's do one session with each app having its own channel. > [-- Attachment #2: Type: text/html, Size: 69652 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-12 19:25 ` [RFC v2 1/2] dts: add " jspewock 2023-05-23 8:05 ` Juraj Linkeš @ 2023-05-24 10:03 ` Juraj Linkeš 2023-05-25 15:40 ` Jeremy Spewock 1 sibling, 1 reply; 12+ messages in thread From: Juraj Linkeš @ 2023-05-24 10:03 UTC (permalink / raw) To: jspewock; +Cc: dev I'm sending the second part of the review separately. This is only about implementation-specific details, the other comments still apply (such as how I'd like to move/structure the code). In general, it really looks like we have to go this route. It's kind of a pexpect-lite approach, which seems fine for this simple use case (by which I mean the prompt is very well defined and we don't need to cover exotic corner cases). > diff --git a/dts/framework/remote_session/remote/__init__.py b/dts/framework/remote_session/remote/__init__.py > index 8a151221..abca8edc 100644 > --- a/dts/framework/remote_session/remote/__init__.py > +++ b/dts/framework/remote_session/remote/__init__.py > @@ -9,8 +9,36 @@ > from .remote_session import CommandResult, RemoteSession > from .ssh_session import SSHSession > > +from paramiko import SSHClient, AutoAddPolicy > +from framework.utils import GREEN > > def create_remote_session( > node_config: NodeConfiguration, name: str, logger: DTSLOG > ) -> RemoteSession: > return SSHSession(node_config, name, logger) > + > +def create_interactive_session( > + node_config: NodeConfiguration, name: str, logger: DTSLOG > +) -> SSHClient: > + """ > + Creates a paramiko SSH session that is designed to be used for interactive shells > + > + This session is meant to be used on an "as needed" basis and may never be utilized > + """ > + client: SSHClient = SSHClient() > + client.set_missing_host_key_policy(AutoAddPolicy) > + ip: str = node_config.hostname > + logger.info(GREEN(f"Connecting to host {ip}")) > + #Preset to 22 because paramiko doesn't accept None > + port: int = 22 > + if ":" in node_config.hostname: > + ip, port = node_config.hostname.split(":") > + port = int(port) > + client.connect( > + ip, > + username=node_config.user, > + port=port, > + password=node_config.password or "", > + timeout=20 if port else 10 > + ) The connect call needs to be in a try/except block where we should catch the exceptions listed in paramiko docs and convert them into either SSHConnectionError or SSHTimeoutError. I'd like to make it as close to what we're doing in http://patches.dpdk.org/project/dpdk/patch/20230424133537.58698-1-juraj.linkes@pantheon.tech/ as possible. > + return client > diff --git a/dts/framework/testbed_model/__init__.py b/dts/framework/testbed_model/__init__.py > index f54a9470..63f17cc3 100644 > --- a/dts/framework/testbed_model/__init__.py > +++ b/dts/framework/testbed_model/__init__.py > @@ -20,3 +20,8 @@ > ) > from .node import Node > from .sut_node import SutNode > + > +from .interactive_apps import ( > + InteractiveScriptHandler, > + TestpmdDriver > +) > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py b/dts/framework/testbed_model/interactive_apps/__init__.py > new file mode 100644 > index 00000000..0382d7e0 > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > @@ -0,0 +1,6 @@ > +from .interactive_command import ( > + InteractiveScriptHandler > +) > +from .testpmd_driver import ( > + TestpmdDriver > +) > \ No newline at end of file > diff --git a/dts/framework/testbed_model/interactive_apps/interactive_command.py b/dts/framework/testbed_model/interactive_apps/interactive_command.py > new file mode 100644 > index 00000000..7467911b > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py > @@ -0,0 +1,57 @@ > +# import paramiko > +from paramiko import SSHClient, Channel, channel > +from framework.settings import SETTINGS > + > +class InteractiveScriptHandler: > + > + _ssh_client: SSHClient > + _stdin: channel.ChannelStdinFile > + _ssh_channel: Channel > + > + def __init__(self, ssh_client: SSHClient, timeout:float = SETTINGS.timeout) -> None: > + self._ssh_client = ssh_client > + self._ssh_channel = self._ssh_client.invoke_shell() > + self._stdin = self._ssh_channel.makefile_stdin("wb") Do we need to open this channel in binary mode? Are there any dpdk apps which send binary data instead of text? > + self._ssh_channel.settimeout(timeout) Have you tried using the recv and send methods? Reading the docs, it looks like using the file-like object approach may be more straightforward for our case (as the prompt we're expecting is at the end of a line). What are the differences in practice? > + > + def send_command(self, command:str) -> None: > + """ > + Send command to channel without recording output. > + > + This method will not verify any input or output, it will > + simply assume the command succeeded > + """ > + self._stdin.write(command + '\n') > + self._stdin.flush() I don't see this used anywhere. You mentioned it would be needed for username/password prompts, but paramiko handles that. Do we need this for any DPDK app? And even if we do, we should only add it when the need arises. > + > + def send_command_get_output(self, command:str, expect:str) -> str: > + """ > + Send a command and get all output before the expected ending string. > + > + **NOTE** The note preface is not needed. We haven't yet 100% agreed on the docstring format, but in the meantime, let's use the best candidate, google docs format: https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings > + Lines that expect input are not included in the stdout buffer so they cannot be > + used for expect. I'm curious, in the case of testpmd, aren't the lines that contain the prompt also the ones that expect input? In which case we'd never match the prompt without the workaround below. Or am I misunderstanding something? For example, if you were prompted to log into something > + with a username and password, you cannot expect "username:" because it wont > + yet be in the stdout buffer. A work around for this could be consuming an > + extra newline character to force the current prompt into the stdout buffer. > + > + *Return* > + All output before expected string > + """ > + stdout = self._ssh_channel.makefile("r") Why the different workflow for stdin and stdout? And where's stderr? I imagine we'd want to capture that as well (to check errors), or do dpdk apps not emit anything to stderr? > + self._stdin.write(command + '\n') Let's use an f-string here like everywhere else. There are other places where this applies as well. We should also add a logger so that we capture what we send and receive. > + self._stdin.flush() > + out:str = "" > + for line in stdout: Do we need to worry about different line breaks on different OS's? Or do dpdk apps always terminate lines with \n? > + out += str(line) Do we need the string conversion even if the channel has not been opened in binary mode? Or is it opened as such by default? > + if expect in str(line): The prompt should always be at the end of the line, right? Would checking the line.endswith(expect) make more sense (or line.rstrip().endswith(expect))? > + break > + stdout.close() #close the buffer to flush the output The docs say closing only does flushing, so we might as well call flush so the code is more consistent (we're calling flush on stdin, not close). > + return out > + > + def close(self): > + self._stdin.close() > + self._ssh_channel.close() > + > + def __del__(self): > + self.close() > diff --git a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > new file mode 100644 > index 00000000..1993eae6 > --- /dev/null > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > @@ -0,0 +1,24 @@ > +from framework.testbed_model.interactive_apps import InteractiveScriptHandler > + > +from pathlib import PurePath > + > +class TestpmdDriver: > + prompt:str = "testpmd>" > + interactive_handler: InteractiveScriptHandler > + > + def __init__(self, handler: InteractiveScriptHandler, dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> None: > + """ > + Sets the handler to drive the SSH session and starts testpmd > + """ > + self.interactive_handler = handler > + # self.interactive_handler.send_command("sudo su") > + # self.interactive_handler.send_command("cd /root/testpmd-testing/dpdk/build") > + self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > + > + def send_command(self, command:str, expect:str = prompt) -> str: > + """ > + Specific way of handling the command for testpmd > + > + An extra newline character is consumed in order to force the current line into the stdout buffer > + """ > + return self.interactive_handler.send_command_get_output(command + "\n", expect) The newline is already being added. Or is this the workaround (i.e. we need two newlines?). If so, move the second newline to send_command_get_output. > \ No newline at end of file > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py <snip> > + def test_device_bound_to_driver(self) -> None: > + """ > + Test: > + ensure that all drivers listed in the config are bound to the correct drivers > + """ > + for nic in self.sut_node._execution_config.nics: > + for address in nic.addresses: > + out = self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py --status | grep {address}", 60) I didn't finish my previous comment, so here's the finish: :-) This should follow the same abstractions as I outlined above. However, we don't need to use dpdk-devbind here. It's probably safe to do so (the script is likely not going to be changed), but we could interrogate the device the same way the script does: ls /sys/bus/pci/devices/ And then check that the PCI is listed. Inside the PCI dir, we can check what driver it's bound to (if any) and basically anything else we need about the device (but not everything - firmware version comes to mind). > + self.verify( > + len(out.stdout) != 0, > + f"Failed to find configured device ({address}) using dpdk-devbind.py", > + ) > + for string in out.stdout.split(" "): > + if 'drv=' in string: > + self.verify( > + string.split("=")[1] == nic.driver.strip(), > + f'Driver for device {address} does not match driver listed in configuration (bound to {string.split("=")[1]})', > + ) > + ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC v2 1/2] dts: add smoke tests 2023-05-24 10:03 ` Juraj Linkeš @ 2023-05-25 15:40 ` Jeremy Spewock 0 siblings, 0 replies; 12+ messages in thread From: Jeremy Spewock @ 2023-05-25 15:40 UTC (permalink / raw) To: Juraj Linkeš; +Cc: dev [-- Attachment #1: Type: text/plain, Size: 17093 bytes --] On Wed, May 24, 2023 at 6:03 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > I'm sending the second part of the review separately. This is only > about implementation-specific details, the other comments still apply > (such as how I'd like to move/structure the code). > > In general, it really looks like we have to go this route. It's kind > of a pexpect-lite approach, which seems fine for this simple use case > (by which I mean the prompt is very well defined and we don't need to > cover exotic corner cases). > > > diff --git a/dts/framework/remote_session/remote/__init__.py > b/dts/framework/remote_session/remote/__init__.py > > index 8a151221..abca8edc 100644 > > --- a/dts/framework/remote_session/remote/__init__.py > > +++ b/dts/framework/remote_session/remote/__init__.py > > @@ -9,8 +9,36 @@ > > from .remote_session import CommandResult, RemoteSession > > from .ssh_session import SSHSession > > > > +from paramiko import SSHClient, AutoAddPolicy > > +from framework.utils import GREEN > > > > def create_remote_session( > > node_config: NodeConfiguration, name: str, logger: DTSLOG > > ) -> RemoteSession: > > return SSHSession(node_config, name, logger) > > + > > +def create_interactive_session( > > + node_config: NodeConfiguration, name: str, logger: DTSLOG > > +) -> SSHClient: > > + """ > > + Creates a paramiko SSH session that is designed to be used for > interactive shells > > + > > + This session is meant to be used on an "as needed" basis and may > never be utilized > > + """ > > + client: SSHClient = SSHClient() > > + client.set_missing_host_key_policy(AutoAddPolicy) > > + ip: str = node_config.hostname > > + logger.info(GREEN(f"Connecting to host {ip}")) > > + #Preset to 22 because paramiko doesn't accept None > > + port: int = 22 > > + if ":" in node_config.hostname: > > + ip, port = node_config.hostname.split(":") > > + port = int(port) > > + client.connect( > > + ip, > > + username=node_config.user, > > + port=port, > > + password=node_config.password or "", > > + timeout=20 if port else 10 > > + ) > > The connect call needs to be in a try/except block where we should > catch the exceptions listed in paramiko docs and convert them into > either SSHConnectionError or SSHTimeoutError. I'd like to make it as > close to what we're doing in > > http://patches.dpdk.org/project/dpdk/patch/20230424133537.58698-1-juraj.linkes@pantheon.tech/ > as possible. > You're right, in my attempts to just figure it out and get everything working I neglected to think about catching the error here, good catch. > > > + return client > > > diff --git a/dts/framework/testbed_model/__init__.py > b/dts/framework/testbed_model/__init__.py > > index f54a9470..63f17cc3 100644 > > --- a/dts/framework/testbed_model/__init__.py > > +++ b/dts/framework/testbed_model/__init__.py > > @@ -20,3 +20,8 @@ > > ) > > from .node import Node > > from .sut_node import SutNode > > + > > +from .interactive_apps import ( > > + InteractiveScriptHandler, > > + TestpmdDriver > > +) > > diff --git a/dts/framework/testbed_model/interactive_apps/__init__.py > b/dts/framework/testbed_model/interactive_apps/__init__.py > > new file mode 100644 > > index 00000000..0382d7e0 > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/__init__.py > > @@ -0,0 +1,6 @@ > > +from .interactive_command import ( > > + InteractiveScriptHandler > > +) > > +from .testpmd_driver import ( > > + TestpmdDriver > > +) > > \ No newline at end of file > > diff --git > a/dts/framework/testbed_model/interactive_apps/interactive_command.py > b/dts/framework/testbed_model/interactive_apps/interactive_command.py > > new file mode 100644 > > index 00000000..7467911b > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/interactive_command.py > > @@ -0,0 +1,57 @@ > > +# import paramiko > > +from paramiko import SSHClient, Channel, channel > > +from framework.settings import SETTINGS > > + > > +class InteractiveScriptHandler: > > + > > + _ssh_client: SSHClient > > + _stdin: channel.ChannelStdinFile > > + _ssh_channel: Channel > > + > > + def __init__(self, ssh_client: SSHClient, timeout:float = > SETTINGS.timeout) -> None: > > + self._ssh_client = ssh_client > > + self._ssh_channel = self._ssh_client.invoke_shell() > > + self._stdin = self._ssh_channel.makefile_stdin("wb") > > Do we need to open this channel in binary mode? Are there any dpdk > apps which send binary data instead of text? > It does work without the binary mode as well. I don't think there are any that necessarily need the binary data so you're right, this could likely be removed. > > > + self._ssh_channel.settimeout(timeout) > > Have you tried using the recv and send methods? Reading the docs, it > looks like using the file-like object approach may be more > straightforward for our case (as the prompt we're expecting is at the > end of a line). What are the differences in practice? > > I did try using recv instead and I believe both would work in theory. I ended up choosing to go with the file-like approach because it seemed simpler while still reaching the same goal. The only difference I could think of between the two is that with file-like buffers we can stop receiving output as soon as you hit the expected prompt so you could expect something in the center of the output rather than the end if you felt the need to. I believe with recv we would just have to repeatedly accept all the data in the buffer and then stop if that data had the expected output. In our case however, this likely wouldn't cause a problem. > > + > > + def send_command(self, command:str) -> None: > > + """ > > + Send command to channel without recording output. > > + > > + This method will not verify any input or output, it will > > + simply assume the command succeeded > > + """ > > + self._stdin.write(command + '\n') > > + self._stdin.flush() > > I don't see this used anywhere. You mentioned it would be needed for > username/password prompts, but paramiko handles that. Do we need this > for any DPDK app? And even if we do, we should only add it when the > need arises. > The newline in this buffer is treated as just sending the command. The username/password prompts were just an example I used of something that would expect input from the user, you're right that this is handled internally (and maybe I should change my example!). Adding a newline here is just simulating you pressing enter after writing the command and is used by the shell to know that you're finished adding input. > > > + > > + def send_command_get_output(self, command:str, expect:str) -> str: > > + """ > > + Send a command and get all output before the expected ending > string. > > + > > + **NOTE** > > The note preface is not needed. We haven't yet 100% agreed on the > docstring format, but in the meantime, let's use the best candidate, > google docs format: > https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings I'll adapt to this format, I was just trying to write something that would stand out here as that's an important detail. > > > > + Lines that expect input are not included in the stdout buffer > so they cannot be > > + used for expect. > > I'm curious, in the case of testpmd, aren't the lines that contain the > prompt also the ones that expect input? In which case we'd never match > the prompt without the workaround below. Or am I misunderstanding > something? > That is correct, without the workaround that adds an extra newline, the prompt "testpmd>" wouldn't make it into the buffer at the end of the output. > > For example, if you were prompted to log into something > > + with a username and password, you cannot expect "username:" > because it wont > > + yet be in the stdout buffer. A work around for this could be > consuming an > > + extra newline character to force the current prompt into the > stdout buffer. > > + > > + *Return* > > + All output before expected string > > + """ > > + stdout = self._ssh_channel.makefile("r") > > Why the different workflow for stdin and stdout? > And where's stderr? I imagine we'd want to capture that as well (to > check errors), or do dpdk apps not emit anything to stderr? > The different workflows for stdin and stdout exist because you can't actually flush stdout using the flush method. Originally, I wanted to have them both as buffers that are members of the class but this would mean that stdout would include all of the output that has been collected since it has been opened. So, we have to only open stdout when we need to capture output and close it when we no longer want any output (this also disposes of old output) whereas stdin is flushed every time you send the input and so it can be reused. As for the stderr, I believe most DPDK apps only emit data to stdout, but this is a good idea to capture. I had thought that it was still picked up by the output file here but after some more testing this isn't the case so I'll look into capturing stderr too for detecting errors. > > > + self._stdin.write(command + '\n') > > Let's use an f-string here like everywhere else. There are other > places where this applies as well. > We should also add a logger so that we capture what we send and receive. > > > + self._stdin.flush() > > + out:str = "" > > + for line in stdout: > > Do we need to worry about different line breaks on different OS's? Or > do dpdk apps always terminate lines with \n? > I don't believe we do, I think this is something that is handled by the next() method of the buffer. It's listed in the paramiko documentation that this should be handled on newline which I would think should handle different OS's but this is a good thing to lookout for and ensure does work. > > > + out += str(line) > > Do we need the string conversion even if the channel has not been > opened in binary mode? Or is it opened as such by default? > Looking back at the documentation, you're right that this isn't necessary if you aren't reading in byte mode, I'll remove it. > > > + if expect in str(line): > > The prompt should always be at the end of the line, right? Would > checking the line.endswith(expect) make more sense (or > line.rstrip().endswith(expect))? > It would be in terms of DPDK apps, but on this level I was thinking it might make sense to allow users to expect any part of the output, not just the end of the line. This way it could potentially also be used by a developer for their own interactive script if needed. This was something I was writing to be generic enough to be used with anything, not just DPDK apps, so I thought it would make sense to not enforce the restriction and allow the expected string to be anywhere in the output. However, I see how enforcing it at the end of the line could make it more clear and consistent. > > > + break > > + stdout.close() #close the buffer to flush the output > > The docs say closing only does flushing, so we might as well call > flush so the code is more consistent (we're calling flush on stdin, > not close). > Originally, I tried this and made the stdout buffer a member variable, similar to the stdin buffer, but the flush method only flushes write buffers. Because stdout is a read buffer, when you call flush() on it the method doesn't do anything and the old data in the stdout buffer remains. I tried to find another way to empty the buffer but I didn't find anything other than closing it and reopening it as needed. > > > + return out > > + > > + def close(self): > > + self._stdin.close() > > + self._ssh_channel.close() > > + > > + def __del__(self): > > + self.close() > > diff --git > a/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > > new file mode 100644 > > index 00000000..1993eae6 > > --- /dev/null > > +++ b/dts/framework/testbed_model/interactive_apps/testpmd_driver.py > > @@ -0,0 +1,24 @@ > > +from framework.testbed_model.interactive_apps import > InteractiveScriptHandler > > + > > +from pathlib import PurePath > > + > > +class TestpmdDriver: > > + prompt:str = "testpmd>" > > + interactive_handler: InteractiveScriptHandler > > + > > + def __init__(self, handler: InteractiveScriptHandler, > dpdk_build_dir:PurePath, eal_flags:str = "", cmd_line_options:str = "") -> > None: > > + """ > > + Sets the handler to drive the SSH session and starts testpmd > > + """ > > + self.interactive_handler = handler > > + # self.interactive_handler.send_command("sudo su") > > + # self.interactive_handler.send_command("cd > /root/testpmd-testing/dpdk/build") > > + > self.interactive_handler.send_command_get_output(f"{dpdk_build_dir}/app/dpdk-testpmd > {eal_flags} -- -i {cmd_line_options}\n", self.prompt) > > + > > + def send_command(self, command:str, expect:str = prompt) -> str: > > + """ > > + Specific way of handling the command for testpmd > > + > > + An extra newline character is consumed in order to force the > current line into the stdout buffer > > + """ > > + return self.interactive_handler.send_command_get_output(command > + "\n", expect) > > The newline is already being added. Or is this the workaround (i.e. we > need two newlines?). If so, move the second newline to > send_command_get_output. > The newline we are adding here is the workaround which does end up adding two new lines. The first newline (added in send_command_get_output) is to actually send the command, the second one (added here) is a specific workaround that forces an additional newline to be entered. What this does is it causes the current line after the command finishes (in this case, a "testpmd>" line that is expecting input) to accept an empty string which does nothing but add the current line into the output buffer. The reason I left it in the testpmd specific driver is because this work-around only works for "bash-like" tools that prompt you with the same line again after accepting an empty string. I was trying to keep send_command_get_output generic enough to be used on any interactive environment, not just DPDK apps or "bash-like" environments and left this work around in the TestpmdDriver because it seemed more specific to that tool. > > > \ No newline at end of file > > diff --git a/dts/framework/testbed_model/node.py > b/dts/framework/testbed_model/node.py > > <snip> > > > + def test_device_bound_to_driver(self) -> None: > > + """ > > + Test: > > + ensure that all drivers listed in the config are bound to > the correct drivers > > + """ > > + for nic in self.sut_node._execution_config.nics: > > + for address in nic.addresses: > > + out = > self.sut_node.main_session.send_command(f"{self.dpdk_build_dir_path}/../usertools/dpdk-devbind.py > --status | grep {address}", 60) > > I didn't finish my previous comment, so here's the finish: :-) > This should follow the same abstractions as I outlined above. > However, we don't need to use dpdk-devbind here. It's probably safe to > do so (the script is likely not going to be changed), but we could > interrogate the device the same way the script does: > ls /sys/bus/pci/devices/ > > And then check that the PCI is listed. Inside the PCI dir, we can > check what driver it's bound to (if any) and basically anything else > we need about the device (but not everything - firmware version comes > to mind). > This definitely does need to be os-agnostic, but that's a good point about removing the reliance on the devbind script and might be a good thing to implement. > > > + self.verify( > > + len(out.stdout) != 0, > > + f"Failed to find configured device ({address}) > using dpdk-devbind.py", > > + ) > > + for string in out.stdout.split(" "): > > + if 'drv=' in string: > > + self.verify( > > + string.split("=")[1] == nic.driver.strip(), > > + f'Driver for device {address} does not > match driver listed in configuration (bound to {string.split("=")[1]})', > > + ) > > + > [-- Attachment #2: Type: text/html, Size: 21965 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC v2 2/2] dts: added paramiko to dependencies 2023-05-12 19:25 [RFC v2 0/2] add DTS smoke tests jspewock 2023-05-12 19:25 ` [RFC v2 1/2] dts: add " jspewock @ 2023-05-12 19:25 ` jspewock 1 sibling, 0 replies; 12+ messages in thread From: jspewock @ 2023-05-12 19:25 UTC (permalink / raw) To: dev; +Cc: Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> added paramiko to the dependency files Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/poetry.lock | 160 ++++++++++++++++++++++++++++++++++----------- dts/pyproject.toml | 1 + 2 files changed, 124 insertions(+), 37 deletions(-) diff --git a/dts/poetry.lock b/dts/poetry.lock index 0b2a007d..d5b41550 100644 --- a/dts/poetry.lock +++ b/dts/poetry.lock @@ -1,20 +1,33 @@ [[package]] name = "attrs" -version = "22.1.0" +version = "23.1.0" description = "Classes Without Boilerplate" category = "main" optional = false -python-versions = ">=3.5" +python-versions = ">=3.7" [package.extras] -dev = ["coverage[toml] (>=5.0.2)", "hypothesis", "pympler", "pytest (>=4.3.0)", "mypy (>=0.900,!=0.940)", "pytest-mypy-plugins", "zope.interface", "furo", "sphinx", "sphinx-notfound-page", "pre-commit", "cloudpickle"] -docs = ["furo", "sphinx", "zope.interface", "sphinx-notfound-page"] -tests = ["coverage[toml] (>=5.0.2)", "hypothesis", "pympler", "pytest (>=4.3.0)", "mypy (>=0.900,!=0.940)", "pytest-mypy-plugins", "zope.interface", "cloudpickle"] -tests_no_zope = ["coverage[toml] (>=5.0.2)", "hypothesis", "pympler", "pytest (>=4.3.0)", "mypy (>=0.900,!=0.940)", "pytest-mypy-plugins", "cloudpickle"] +cov = ["attrs", "coverage[toml] (>=5.3)"] +dev = ["attrs", "pre-commit"] +docs = ["furo", "myst-parser", "sphinx", "sphinx-notfound-page", "sphinxcontrib-towncrier", "towncrier", "zope-interface"] +tests = ["attrs", "zope-interface"] +tests-no-zope = ["cloudpickle", "hypothesis", "mypy (>=1.1.1)", "pympler", "pytest-mypy-plugins", "pytest-xdist", "pytest (>=4.3.0)"] + +[[package]] +name = "bcrypt" +version = "4.0.1" +description = "Modern password hashing for your software and your servers" +category = "main" +optional = false +python-versions = ">=3.6" + +[package.extras] +tests = ["pytest (>=3.2.1,!=3.3.0)"] +typecheck = ["mypy"] [[package]] name = "black" -version = "22.10.0" +version = "22.12.0" description = "The uncompromising code formatter." category = "dev" optional = false @@ -33,6 +46,17 @@ d = ["aiohttp (>=3.7.4)"] jupyter = ["ipython (>=7.8.0)", "tokenize-rt (>=3.2.0)"] uvloop = ["uvloop (>=0.15.2)"] +[[package]] +name = "cffi" +version = "1.15.1" +description = "Foreign Function Interface for Python calling C code." +category = "main" +optional = false +python-versions = "*" + +[package.dependencies] +pycparser = "*" + [[package]] name = "click" version = "8.1.3" @@ -52,18 +76,39 @@ category = "dev" optional = false python-versions = "!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*,!=3.5.*,!=3.6.*,>=2.7" +[[package]] +name = "cryptography" +version = "40.0.2" +description = "cryptography is a package which provides cryptographic recipes and primitives to Python developers." +category = "main" +optional = false +python-versions = ">=3.6" + +[package.dependencies] +cffi = ">=1.12" + +[package.extras] +docs = ["sphinx (>=5.3.0)", "sphinx-rtd-theme (>=1.1.1)"] +docstest = ["pyenchant (>=1.6.11)", "twine (>=1.12.0)", "sphinxcontrib-spelling (>=4.0.1)"] +pep8test = ["black", "ruff", "mypy", "check-manifest"] +sdist = ["setuptools-rust (>=0.11.4)"] +ssh = ["bcrypt (>=3.1.5)"] +test = ["pytest (>=6.2.0)", "pytest-shard (>=0.1.2)", "pytest-benchmark", "pytest-cov", "pytest-subtests", "pytest-xdist", "pretend", "iso8601"] +test-randomorder = ["pytest-randomly"] +tox = ["tox"] + [[package]] name = "isort" -version = "5.10.1" +version = "5.12.0" description = "A Python utility / library to sort Python imports." category = "dev" optional = false -python-versions = ">=3.6.1,<4.0" +python-versions = ">=3.8.0" [package.extras] -pipfile_deprecated_finder = ["pipreqs", "requirementslib"] -requirements_deprecated_finder = ["pipreqs", "pip-api"] -colors = ["colorama (>=0.4.3,<0.5.0)"] +colors = ["colorama (>=0.4.3)"] +requirements-deprecated-finder = ["pip-api", "pipreqs"] +pipfile-deprecated-finder = ["pip-shims (>=0.5.2)", "pipreqs", "requirementslib"] plugins = ["setuptools"] [[package]] @@ -87,7 +132,7 @@ python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*" [[package]] name = "jsonschema" -version = "4.17.0" +version = "4.17.3" description = "An implementation of JSON Schema validation for Python" category = "main" optional = false @@ -129,15 +174,33 @@ reports = ["lxml"] [[package]] name = "mypy-extensions" -version = "0.4.3" -description = "Experimental type system extensions for programs checked with the mypy typechecker." +version = "1.0.0" +description = "Type system extensions for programs checked with the mypy type checker." category = "dev" optional = false -python-versions = "*" +python-versions = ">=3.5" + +[[package]] +name = "paramiko" +version = "3.1.0" +description = "SSH2 protocol library" +category = "main" +optional = false +python-versions = ">=3.6" + +[package.dependencies] +bcrypt = ">=3.2" +cryptography = ">=3.3" +pynacl = ">=1.5" + +[package.extras] +all = ["pyasn1 (>=0.1.7)", "invoke (>=2.0)", "gssapi (>=1.4.1)", "pywin32 (>=2.1.8)"] +gssapi = ["pyasn1 (>=0.1.7)", "gssapi (>=1.4.1)", "pywin32 (>=2.1.8)"] +invoke = ["invoke (>=2.0)"] [[package]] name = "pathspec" -version = "0.10.1" +version = "0.11.1" description = "Utility library for gitignore style pattern matching of file paths." category = "dev" optional = false @@ -156,15 +219,15 @@ ptyprocess = ">=0.5" [[package]] name = "platformdirs" -version = "2.5.2" -description = "A small Python module for determining appropriate platform-specific dirs, e.g. a \"user data dir\"." +version = "3.5.1" +description = "A small Python package for determining appropriate platform-specific dirs, e.g. a \"user data dir\"." category = "dev" optional = false python-versions = ">=3.7" [package.extras] -docs = ["furo (>=2021.7.5b38)", "proselint (>=0.10.2)", "sphinx-autodoc-typehints (>=1.12)", "sphinx (>=4)"] -test = ["appdirs (==1.4.4)", "pytest-cov (>=2.7)", "pytest-mock (>=3.6)", "pytest (>=6)"] +docs = ["furo (>=2023.3.27)", "proselint (>=0.13)", "sphinx-autodoc-typehints (>=1.23,!=1.23.4)", "sphinx (>=6.2.1)"] +test = ["appdirs (==1.4.4)", "covdefaults (>=2.3)", "pytest-cov (>=4)", "pytest-mock (>=3.10)", "pytest (>=7.3.1)"] [[package]] name = "ptyprocess" @@ -176,25 +239,33 @@ python-versions = "*" [[package]] name = "pycodestyle" -version = "2.9.1" +version = "2.10.0" description = "Python style guide checker" category = "dev" optional = false python-versions = ">=3.6" +[[package]] +name = "pycparser" +version = "2.21" +description = "C parser in Python" +category = "main" +optional = false +python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*" + [[package]] name = "pydocstyle" -version = "6.1.1" +version = "6.3.0" description = "Python docstring style checker" category = "dev" optional = false python-versions = ">=3.6" [package.dependencies] -snowballstemmer = "*" +snowballstemmer = ">=2.2.0" [package.extras] -toml = ["toml"] +toml = ["tomli (>=1.2.3)"] [[package]] name = "pyflakes" @@ -228,9 +299,24 @@ tests = ["pytest (>=7.1.2)", "pytest-mypy", "eradicate (>=2.0.0)", "radon (>=5.1 toml = ["toml (>=0.10.2)"] vulture = ["vulture"] +[[package]] +name = "pynacl" +version = "1.5.0" +description = "Python binding to the Networking and Cryptography (NaCl) library" +category = "main" +optional = false +python-versions = ">=3.6" + +[package.dependencies] +cffi = ">=1.4.1" + +[package.extras] +docs = ["sphinx (>=1.6.5)", "sphinx-rtd-theme"] +tests = ["pytest (>=3.2.1,!=3.3.0)", "hypothesis (>=3.27.0)"] + [[package]] name = "pyrsistent" -version = "0.19.1" +version = "0.19.3" description = "Persistent/Functional/Immutable data structures" category = "main" optional = false @@ -270,7 +356,7 @@ python-versions = ">=3.7" [[package]] name = "types-pyyaml" -version = "6.0.12.1" +version = "6.0.12.9" description = "Typing stubs for PyYAML" category = "main" optional = false @@ -278,7 +364,7 @@ python-versions = "*" [[package]] name = "typing-extensions" -version = "4.4.0" +version = "4.5.0" description = "Backported and Experimental Type Hints for Python 3.7+" category = "dev" optional = false @@ -299,13 +385,16 @@ jsonschema = ">=4,<5" [metadata] lock-version = "1.1" python-versions = "^3.10" -content-hash = "a0f040b07fc6ce4deb0be078b9a88c2a465cb6bccb9e260a67e92c2403e2319f" +content-hash = "c119901b1c13b14adf9b53624430aff8720d9a3a180b028579b82c65d49474df" [metadata.files] attrs = [] +bcrypt = [] black = [] +cffi = [] click = [] colorama = [] +cryptography = [] isort = [] jsonpatch = [] jsonpointer = [] @@ -313,20 +402,17 @@ jsonschema = [] mccabe = [] mypy = [] mypy-extensions = [] +paramiko = [] pathspec = [] -pexpect = [ - {file = "pexpect-4.8.0-py2.py3-none-any.whl", hash = "sha256:0b48a55dcb3c05f3329815901ea4fc1537514d6ba867a152b581d69ae3710937"}, - {file = "pexpect-4.8.0.tar.gz", hash = "sha256:fc65a43959d153d0114afe13997d439c22823a27cefceb5ff35c2178c6784c0c"}, -] -platformdirs = [ - {file = "platformdirs-2.5.2-py3-none-any.whl", hash = "sha256:027d8e83a2d7de06bbac4e5ef7e023c02b863d7ea5d079477e722bb41ab25788"}, - {file = "platformdirs-2.5.2.tar.gz", hash = "sha256:58c8abb07dcb441e6ee4b11d8df0ac856038f944ab98b7be6b27b2a3c7feef19"}, -] +pexpect = [] +platformdirs = [] ptyprocess = [] pycodestyle = [] +pycparser = [] pydocstyle = [] pyflakes = [] pylama = [] +pynacl = [] pyrsistent = [] pyyaml = [] snowballstemmer = [] diff --git a/dts/pyproject.toml b/dts/pyproject.toml index a136c91e..928837cb 100644 --- a/dts/pyproject.toml +++ b/dts/pyproject.toml @@ -13,6 +13,7 @@ pexpect = "^4.8.0" warlock = "^2.0.1" PyYAML = "^6.0" types-PyYAML = "^6.0.8" +paramiko = "^3.1.0" [tool.poetry.dev-dependencies] mypy = "^0.961" -- 2.40.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-05-26 13:24 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-12 19:25 [RFC v2 0/2] add DTS smoke tests jspewock 2023-05-12 19:25 ` [RFC v2 1/2] dts: add " jspewock 2023-05-23 8:05 ` Juraj Linkeš 2023-05-23 8:33 ` Juraj Linkeš 2023-05-24 20:44 ` Jeremy Spewock 2023-05-25 8:33 ` Juraj Linkeš 2023-05-25 18:02 ` Jeremy Spewock 2023-05-26 7:36 ` Juraj Linkeš 2023-05-26 13:24 ` Jeremy Spewock 2023-05-24 10:03 ` Juraj Linkeš 2023-05-25 15:40 ` Jeremy Spewock 2023-05-12 19:25 ` [RFC v2 2/2] dts: added paramiko to dependencies jspewock
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).