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