On Fri, May 26, 2023 at 3:36 AM Juraj Linkeš wrote: > On Thu, May 25, 2023 at 8:03 PM Jeremy Spewock > wrote: > > > > Hey Juraj, > > > > On Thu, May 25, 2023 at 4:33 AM Juraj Linkeš > wrote: > >> > >> One more point that doesn't fit elsewhere: > >> > >> On Wed, May 24, 2023 at 10:45 PM Jeremy Spewock > wrote: > >> > > >> > > >> > > >> > On Tue, May 23, 2023 at 4:05 AM Juraj Linkeš > 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 wrote: > >> >> > > >> >> > From: Jeremy Spewock > >> >> > > >> >> > 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 > >> >> > --- > >> >> > 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: "" > >> >> > >> >> > >> >> > >> >> > 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. >