* [PATCH v1 0/2] Improve interactive shell output gathering @ 2024-03-12 17:25 jspewock 2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: jspewock @ 2024-03-12 17:25 UTC (permalink / raw) To: Honnappa.Nagarahalli, juraj.linkes, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> When the interactive shell was originally added, the gathering of output was implented in a way that was trying to be flexible enough to gather output until a given string was found anywhere in the output. It has been found more recently that this greater flexibility provides less guarantees when writing tools that implement the shell and ultimately can lead to unexpected behaviour. This is especially true when sending multi-line commands into the shell which is desirable in some cases. To account for this, the prompt will strictly be expected at the end of a line from the output. There were also some quality of life changes made to this system such as better wrapping of errors to privde some more useful output. Jeremy Spewock (2): dts: Improve output gathering in interactive shells dts: Add missing docstring from XML-RPC server dts/framework/exception.py | 7 +++ .../remote_session/interactive_shell.py | 26 +++++++---- .../testbed_model/traffic_generator/scapy.py | 46 ++++++++++++++++++- 3 files changed, 70 insertions(+), 9 deletions(-) -- 2.43.2 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v1 1/2] dts: Improve output gathering in interactive shells 2024-03-12 17:25 [PATCH v1 0/2] Improve interactive shell output gathering jspewock @ 2024-03-12 17:25 ` jspewock 2024-04-03 9:00 ` Juraj Linkeš 2024-03-12 17:25 ` [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server jspewock 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock 2 siblings, 1 reply; 28+ messages in thread From: jspewock @ 2024-03-12 17:25 UTC (permalink / raw) To: Honnappa.Nagarahalli, juraj.linkes, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> The current implementation of consuming output from interactive shells relies on being able to find an expected prompt somewhere within the output buffer after sending the command. This is useful in situations where the prompt does not appear in the output itself, but in some practical cases (such as the starting of an XML-RPC server for scapy) the prompt exists in one of the commands sent to the shell and this can cause the command to exit early and creates a race condition between the server starting and the first command being sent to the server. This patch addresses this problem by searching for a line that strictly ends with the provided prompt, rather than one that simply contains it, so that the detection that a command is finished is more consistent. It also adds a catch to detect when a command times out before finding the prompt so that the exception can be wrapped into a more explicit one and display the output that it did manage to gather before timing out. Bugzilla ID: 1359 Fixes: 88489c0501af ("dts: add smoke tests") Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/framework/exception.py | 7 +++++ .../remote_session/interactive_shell.py | 26 +++++++++++++------ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/dts/framework/exception.py b/dts/framework/exception.py index 658eee2c38..cce1e0231a 100644 --- a/dts/framework/exception.py +++ b/dts/framework/exception.py @@ -146,6 +146,13 @@ def __str__(self) -> str: return f"Command {self.command} returned a non-zero exit code: {self._command_return_code}" +class InteractiveCommandExecutionError(DTSError): + """An unsuccessful execution of a remote command in an interactive environment.""" + + #: + severity: ClassVar[ErrorSeverity] = ErrorSeverity.REMOTE_CMD_EXEC_ERR + + class RemoteDirectoryExistsError(DTSError): """A directory that exists on a remote node.""" diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 5cfe202e15..2bcfdcb3c7 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -20,6 +20,7 @@ from paramiko import Channel, SSHClient, channel # type: ignore[import] +from framework.exception import InteractiveCommandExecutionError from framework.logger import DTSLogger from framework.settings import SETTINGS @@ -124,6 +125,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str: Returns: All output in the buffer before expected string. + + Raises: + InteractiveCommandExecutionError: If command was sent but prompt could not be found in + the output. """ self._logger.info(f"Sending: '{command}'") if prompt is None: @@ -131,14 +136,19 @@ def send_command(self, command: str, prompt: str | None = None) -> str: self._stdin.write(f"{command}{self._command_extra_chars}\n") self._stdin.flush() out: str = "" - for line in self._stdout: - out += line - if prompt in line and not line.rstrip().endswith( - command.rstrip() - ): # ignore line that sent command - break - self._logger.debug(f"Got output: {out}") - return out + try: + for line in self._stdout: + out += line + if line.rstrip().endswith(prompt): + break + except TimeoutError: + raise InteractiveCommandExecutionError( + f"Failed to find the prompt ({prompt}) at the end of a line in the output from the" + f" command ({command}). Got:\n{out}" + ) + else: + self._logger.debug(f"Got output: {out}") + return out def close(self) -> None: """Properly free all resources.""" -- 2.43.2 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells 2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock @ 2024-04-03 9:00 ` Juraj Linkeš 2024-04-08 16:20 ` Jeremy Spewock 0 siblings, 1 reply; 28+ messages in thread From: Juraj Linkeš @ 2024-04-03 9:00 UTC (permalink / raw) To: jspewock Cc: Honnappa.Nagarahalli, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro, dev On Tue, Mar 12, 2024 at 6:26 PM <jspewock@iol.unh.edu> wrote: > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > The current implementation of consuming output from interactive shells > relies on being able to find an expected prompt somewhere within the > output buffer after sending the command. This is useful in situations > where the prompt does not appear in the output itself, but in some > practical cases (such as the starting of an XML-RPC server for scapy) > the prompt exists in one of the commands sent to the shell and this can > cause the command to exit early and creates a race condition between the > server starting and the first command being sent to the server. > > This patch addresses this problem by searching for a line that strictly > ends with the provided prompt, rather than one that simply contains it, > so that the detection that a command is finished is more consistent. It > also adds a catch to detect when a command times out before finding the > prompt so that the exception can be wrapped into a more explicit one and > display the output that it did manage to gather before timing out. > This could still cause problems if the prompt appears at the end of a line in the output. Maybe we don't need to worry about that until we actually hit that problem. In any case, I'd like to test this, so please rebase the patch. :-) > Bugzilla ID: 1359 > Fixes: 88489c0501af ("dts: add smoke tests") > > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> > --- > dts/framework/exception.py | 7 +++++ > .../remote_session/interactive_shell.py | 26 +++++++++++++------ > 2 files changed, 25 insertions(+), 8 deletions(-) > > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > index 658eee2c38..cce1e0231a 100644 > --- a/dts/framework/exception.py > +++ b/dts/framework/exception.py > @@ -146,6 +146,13 @@ def __str__(self) -> str: > return f"Command {self.command} returned a non-zero exit code: {self._command_return_code}" > > > +class InteractiveCommandExecutionError(DTSError): > + """An unsuccessful execution of a remote command in an interactive environment.""" > + > + #: > + severity: ClassVar[ErrorSeverity] = ErrorSeverity.REMOTE_CMD_EXEC_ERR > + > + > class RemoteDirectoryExistsError(DTSError): > """A directory that exists on a remote node.""" > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py > index 5cfe202e15..2bcfdcb3c7 100644 > --- a/dts/framework/remote_session/interactive_shell.py > +++ b/dts/framework/remote_session/interactive_shell.py > @@ -20,6 +20,7 @@ > > from paramiko import Channel, SSHClient, channel # type: ignore[import] > > +from framework.exception import InteractiveCommandExecutionError > from framework.logger import DTSLogger > from framework.settings import SETTINGS > > @@ -124,6 +125,10 @@ def send_command(self, command: str, prompt: str | None = None) -> str: > > Returns: > All output in the buffer before expected string. > + > + Raises: > + InteractiveCommandExecutionError: If command was sent but prompt could not be found in > + the output. > """ > self._logger.info(f"Sending: '{command}'") > if prompt is None: > @@ -131,14 +136,19 @@ def send_command(self, command: str, prompt: str | None = None) -> str: > self._stdin.write(f"{command}{self._command_extra_chars}\n") > self._stdin.flush() > out: str = "" > - for line in self._stdout: > - out += line > - if prompt in line and not line.rstrip().endswith( > - command.rstrip() > - ): # ignore line that sent command > - break > - self._logger.debug(f"Got output: {out}") > - return out > + try: > + for line in self._stdout: > + out += line > + if line.rstrip().endswith(prompt): > + break > + except TimeoutError: I like this addition, but maybe we could do better. In the regular SSH session, we're expected to raise: * SSHSessionDeadError if the session is not alive, * SSHTimeoutError if the command execution times out. Can we do that here as well? > + raise InteractiveCommandExecutionError( We could just reuse the SSHTimeoutError exception. Is there a reason to distinguish between interactive and non-interactive timeouts? > + f"Failed to find the prompt ({prompt}) at the end of a line in the output from the" > + f" command ({command}). Got:\n{out}" > + ) > + else: > + self._logger.debug(f"Got output: {out}") > + return out > > def close(self) -> None: > """Properly free all resources.""" > -- > 2.43.2 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells 2024-04-03 9:00 ` Juraj Linkeš @ 2024-04-08 16:20 ` Jeremy Spewock 2024-04-10 10:20 ` Juraj Linkeš 0 siblings, 1 reply; 28+ messages in thread From: Jeremy Spewock @ 2024-04-08 16:20 UTC (permalink / raw) To: Juraj Linkeš Cc: Honnappa.Nagarahalli, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro, dev On Wed, Apr 3, 2024 at 5:00 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > > On Tue, Mar 12, 2024 at 6:26 PM <jspewock@iol.unh.edu> wrote: > > > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > > > The current implementation of consuming output from interactive shells > > relies on being able to find an expected prompt somewhere within the > > output buffer after sending the command. This is useful in situations > > where the prompt does not appear in the output itself, but in some > > practical cases (such as the starting of an XML-RPC server for scapy) > > the prompt exists in one of the commands sent to the shell and this can > > cause the command to exit early and creates a race condition between the > > server starting and the first command being sent to the server. > > > > This patch addresses this problem by searching for a line that strictly > > ends with the provided prompt, rather than one that simply contains it, > > so that the detection that a command is finished is more consistent. It > > also adds a catch to detect when a command times out before finding the > > prompt so that the exception can be wrapped into a more explicit one and > > display the output that it did manage to gather before timing out. > > > > This could still cause problems if the prompt appears at the end of a > line in the output. Maybe we don't need to worry about that until we > actually hit that problem. In any case, I'd like to test this, so > please rebase the patch. :-) I will rebase and send out a v2, but that is a good point. it would be just as easy to instead check to see that the prompt is the only thing on the line, so we could do that instead, which do you think is better? I'm sort of indifferent, I can see how verifying that the line only contains the prompt would be useful in cases like it appears in a docstring or something similar (and it's nice to be more explicit in general), and I think that leaving it as the end of the line could potentially save some verbosity if the line you are looking for is long so you can just detect the end of it. Another problem that I could think of with long lines potentially is if, somehow, you were looking for a prompt that the pseudo-terminal split across a few lines and it split your prompt, but I'm not sure this is really relevant to us at all since we only really expect prompts that are usually short. > <snip> > > + try: > > + for line in self._stdout: > > + out += line > > + if line.rstrip().endswith(prompt): > > + break > > + except TimeoutError: > > I like this addition, but maybe we could do better. In the regular SSH > session, we're expected to raise: > * SSHSessionDeadError if the session is not alive, > * SSHTimeoutError if the command execution times out. > > Can we do that here as well? This is a good point, I don't see why we couldn't and thinking about it I like the idea of making this more consistent, good catch. > > > + raise InteractiveCommandExecutionError( > > We could just reuse the SSHTimeoutError exception. Is there a reason > to distinguish between interactive and non-interactive timeouts? I wanted to add a distinction between interactive and non-interactive just because in general the way we log messages when sending commands to interactive shells looks pretty close to what it looks like when you do the same for non-interactive, so if there was an error I thought it might be more helpful in the logs to know which session you were sending the command to when an error was encountered. Maybe, however, a better approach to this would just be keep the exception types the same but change the messages. > <snip> > > 2.43.2 > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells 2024-04-08 16:20 ` Jeremy Spewock @ 2024-04-10 10:20 ` Juraj Linkeš 0 siblings, 0 replies; 28+ messages in thread From: Juraj Linkeš @ 2024-04-10 10:20 UTC (permalink / raw) To: Jeremy Spewock Cc: Honnappa.Nagarahalli, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro, dev On Mon, Apr 8, 2024 at 6:20 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote: > > On Wed, Apr 3, 2024 at 5:00 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > > > > On Tue, Mar 12, 2024 at 6:26 PM <jspewock@iol.unh.edu> wrote: > > > > > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > > > > > The current implementation of consuming output from interactive shells > > > relies on being able to find an expected prompt somewhere within the > > > output buffer after sending the command. This is useful in situations > > > where the prompt does not appear in the output itself, but in some > > > practical cases (such as the starting of an XML-RPC server for scapy) > > > the prompt exists in one of the commands sent to the shell and this can > > > cause the command to exit early and creates a race condition between the > > > server starting and the first command being sent to the server. > > > > > > This patch addresses this problem by searching for a line that strictly > > > ends with the provided prompt, rather than one that simply contains it, > > > so that the detection that a command is finished is more consistent. It > > > also adds a catch to detect when a command times out before finding the > > > prompt so that the exception can be wrapped into a more explicit one and > > > display the output that it did manage to gather before timing out. > > > > > > > This could still cause problems if the prompt appears at the end of a > > line in the output. Maybe we don't need to worry about that until we > > actually hit that problem. In any case, I'd like to test this, so > > please rebase the patch. :-) > > I will rebase and send out a v2, but that is a good point. it would be > just as easy to instead check to see that the prompt is the only thing > on the line, so we could do that instead, which do you think is > better? Would this work? I'm thinking we may need to send another extra newline character to put the solitary prompt into the buffer if the command output doesn't contain a newline. > I'm sort of indifferent, I can see how verifying that the line > only contains the prompt would be useful in cases like it appears in a > docstring or something similar (and it's nice to be more explicit in > general), and I think that leaving it as the end of the line could > potentially save some verbosity if the line you are looking for is > long so you can just detect the end of it. Another problem that I > could think of with long lines potentially is if, somehow, you were > looking for a prompt that the pseudo-terminal split across a few lines > and it split your prompt, but I'm not sure this is really relevant to > us at all since we only really expect prompts that are usually short. > If it works with checking just the end of the line let's leave it like this (because of the possibility above). I think there shouldn't be any prompts without something after them in docstrings. > > > <snip> > > > + try: > > > + for line in self._stdout: > > > + out += line > > > + if line.rstrip().endswith(prompt): > > > + break > > > + except TimeoutError: > > > > I like this addition, but maybe we could do better. In the regular SSH > > session, we're expected to raise: > > * SSHSessionDeadError if the session is not alive, > > * SSHTimeoutError if the command execution times out. > > > > Can we do that here as well? > > This is a good point, I don't see why we couldn't and thinking about > it I like the idea of making this more consistent, good catch. > > > > > > + raise InteractiveCommandExecutionError( > > > > We could just reuse the SSHTimeoutError exception. Is there a reason > > to distinguish between interactive and non-interactive timeouts? > > I wanted to add a distinction between interactive and non-interactive > just because in general the way we log messages when sending commands > to interactive shells looks pretty close to what it looks like when > you do the same for non-interactive, so if there was an error I > thought it might be more helpful in the logs to know which session you > were sending the command to when an error was encountered. Maybe, > however, a better approach to this would just be keep the exception > types the same but change the messages. > There is probably some value to distinguish between them. We could just mirror the non-interactive exceptions and keep the messages the same. > > > <snip> > > > 2.43.2 > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server 2024-03-12 17:25 [PATCH v1 0/2] Improve interactive shell output gathering jspewock 2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock @ 2024-03-12 17:25 ` jspewock 2024-04-24 13:42 ` Patrick Robb 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock 2 siblings, 1 reply; 28+ messages in thread From: jspewock @ 2024-03-12 17:25 UTC (permalink / raw) To: Honnappa.Nagarahalli, juraj.linkes, thomas, wathsala.vithanage, probb, paul.szczepanek, yoan.picchi, Luca.Vizzarro Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> When this XML-RPC server implementation was added, the docstring had to be shortened in order to reduce the chances of this race condition being encountered. Now that this race condition issue is resolved, the full docstring can be restored. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- .../testbed_model/traffic_generator/scapy.py | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py index 5b60f66237..1b46e613a4 100644 --- a/dts/framework/testbed_model/traffic_generator/scapy.py +++ b/dts/framework/testbed_model/traffic_generator/scapy.py @@ -125,9 +125,53 @@ def scapy_send_packets(xmlrpc_packets: list[xmlrpc.client.Binary], send_iface: s class QuittableXMLRPCServer(SimpleXMLRPCServer): - """Basic XML-RPC server. + r"""Basic XML-RPC server. The server may be augmented by functions serializable by the :mod:`marshal` module. + + Example: + :: + + def hello_world(): + # to be sent to the XML-RPC server + print("Hello World!") + + # start the XML-RPC server on the remote node + # the example assumes you're already connect to a tg_node + # this is done by starting a Python shell on the remote node + from framework.remote_session import PythonShell + session = tg_node.create_interactive_shell(PythonShell, timeout=5, privileged=True) + + # then importing the modules needed to run the server + # and the modules for any functions later added to the server + session.send_command("import xmlrpc") + session.send_command("from xmlrpc.server import SimpleXMLRPCServer") + + # sending the source code of this class to the Python shell + from xmlrpc.server import SimpleXMLRPCServer + src = inspect.getsource(QuittableXMLRPCServer) + src = "\n".join([l for l in src.splitlines() if not l.isspace() and l != ""]) + spacing = "\n" * 4 + session.send_command(spacing + src + spacing) + + # then starting the server with: + command = "s = QuittableXMLRPCServer(('0.0.0.0', {listen_port}));s.serve_forever()" + session.send_command(command, "XMLRPC OK") + + # now the server is running on the remote node and we can add functions to it + # first connect to the server from the execution node + import xmlrpc.client + server_url = f"http://{tg_node.config.hostname}:8000" + rpc_server_proxy = xmlrpc.client.ServerProxy(server_url) + + # get the function bytes to send + import marshal + function_bytes = marshal.dumps(hello_world.__code__) + rpc_server_proxy.add_rpc_function(hello_world.__name__, function_bytes) + + # now we can execute the function on the server + xmlrpc_binary_recv: xmlrpc.client.Binary = rpc_server_proxy.hello_world() + print(str(xmlrpc_binary_recv)) """ def __init__(self, *args, **kwargs): -- 2.43.2 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server 2024-03-12 17:25 ` [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server jspewock @ 2024-04-24 13:42 ` Patrick Robb 0 siblings, 0 replies; 28+ messages in thread From: Patrick Robb @ 2024-04-24 13:42 UTC (permalink / raw) To: jspewock Cc: Honnappa.Nagarahalli, juraj.linkes, thomas, wathsala.vithanage, paul.szczepanek, yoan.picchi, Luca.Vizzarro, dev [-- Attachment #1: Type: text/plain, Size: 30 bytes --] Recheck-request: github-robot [-- Attachment #2: Type: text/html, Size: 113 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 0/3] Improve interactive shell output gathering and logging 2024-03-12 17:25 [PATCH v1 0/2] Improve interactive shell output gathering jspewock 2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock 2024-03-12 17:25 ` [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server jspewock @ 2024-05-01 16:16 ` jspewock 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock ` (4 more replies) 2 siblings, 5 replies; 28+ messages in thread From: jspewock @ 2024-05-01 16:16 UTC (permalink / raw) To: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> This version addresses comments from the last and adds an additional improvement to the logging output that comes from interactive shells. The new way of handling logging allows for not only specification of the host that the command is being sent to like before, but also the type of shell that you are addressing. This is beneficial for 2 reasons: 1. Less ambiguity on if a command is being sent to the host directly or into an interactive shell 2. Ability to name shells so that there is distinction in the logs between two different interactive shells of the same type that serve a different purpose (e.g. a python shell and a python shell that is serving and XML-RPC server for scapy interactions) Jeremy Spewock (3): dts: Improve output gathering in interactive shells dts: Add missing docstring from XML-RPC server dts: Improve logging for interactive shells dts/framework/exception.py | 66 ++++++++++++------- .../remote_session/interactive_shell.py | 54 ++++++++++----- dts/framework/testbed_model/node.py | 7 ++ dts/framework/testbed_model/os_session.py | 6 +- dts/framework/testbed_model/sut_node.py | 7 +- .../testbed_model/traffic_generator/scapy.py | 48 +++++++++++++- 6 files changed, 145 insertions(+), 43 deletions(-) -- 2.44.0 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/3] dts: Improve output gathering in interactive shells 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock @ 2024-05-01 16:16 ` jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock ` (3 subsequent siblings) 4 siblings, 2 replies; 28+ messages in thread From: jspewock @ 2024-05-01 16:16 UTC (permalink / raw) To: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> The current implementation of consuming output from interactive shells relies on being able to find an expected prompt somewhere within the output buffer after sending the command. This is useful in situations where the prompt does not appear in the output itself, but in some practical cases (such as the starting of an XML-RPC server for scapy) the prompt exists in one of the commands sent to the shell and this can cause the command to exit early and creates a race condition between the server starting and the first command being sent to the server. This patch addresses this problem by searching for a line that strictly ends with the provided prompt, rather than one that simply contains it, so that the detection that a command is finished is more consistent. It also adds a catch to detect when a command times out before finding the prompt or the underlying SSH session dies so that the exception can be wrapped into a more explicit one and be more consistent with the non-interactive shells. Bugzilla ID: 1359 Fixes: 88489c0501af ("dts: add smoke tests") Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/framework/exception.py | 66 ++++++++++++------- .../remote_session/interactive_shell.py | 46 +++++++++---- 2 files changed, 77 insertions(+), 35 deletions(-) diff --git a/dts/framework/exception.py b/dts/framework/exception.py index cce1e0231a..627190c781 100644 --- a/dts/framework/exception.py +++ b/dts/framework/exception.py @@ -48,26 +48,6 @@ class DTSError(Exception): severity: ClassVar[ErrorSeverity] = ErrorSeverity.GENERIC_ERR -class SSHTimeoutError(DTSError): - """The SSH execution of a command timed out.""" - - #: - severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR - _command: str - - def __init__(self, command: str): - """Define the meaning of the first argument. - - Args: - command: The executed command. - """ - self._command = command - - def __str__(self) -> str: - """Add some context to the string representation.""" - return f"{self._command} execution timed out." - - class SSHConnectionError(DTSError): """An unsuccessful SSH connection.""" @@ -95,8 +75,42 @@ def __str__(self) -> str: return message -class SSHSessionDeadError(DTSError): - """The SSH session is no longer alive.""" +class _SSHTimeoutError(DTSError): + """The execution of a command via SSH timed out. + + This class is private and meant to be raised as its interactive and non-interactive variants. + """ + + #: + severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR + _command: str + + def __init__(self, command: str): + """Define the meaning of the first argument. + + Args: + command: The executed command. + """ + self._command = command + + def __str__(self) -> str: + """Add some context to the string representation.""" + return f"{self._command} execution timed out." + + +class SSHTimeoutError(_SSHTimeoutError): + """The execution of a command on a non-interactive SSH session timed out.""" + + +class InteractiveSSHTimeoutError(_SSHTimeoutError): + """The execution of a command on an interactive SSH session timed out.""" + + +class _SSHSessionDeadError(DTSError): + """The SSH session is no longer alive. + + This class is private and meant to be raised as its interactive and non-interactive variants. + """ #: severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR @@ -115,6 +129,14 @@ def __str__(self) -> str: return f"SSH session with {self._host} has died." +class SSHSessionDeadError(_SSHSessionDeadError): + """Non-interactive SSH session has died.""" + + +class InteractiveSSHSessionDeadError(_SSHSessionDeadError): + """Interactive SSH session as died.""" + + class ConfigurationError(DTSError): """An invalid configuration.""" diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 5cfe202e15..0b0ccdb545 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -18,11 +18,17 @@ from pathlib import PurePath from typing import Callable, ClassVar -from paramiko import Channel, SSHClient, channel # type: ignore[import] +from paramiko import Channel, channel # type: ignore[import] +from framework.exception import ( + InteractiveSSHSessionDeadError, + InteractiveSSHTimeoutError, +) from framework.logger import DTSLogger from framework.settings import SETTINGS +from .interactive_remote_session import InteractiveRemoteSession + class InteractiveShell(ABC): """The base class for managing interactive shells. @@ -34,7 +40,7 @@ class InteractiveShell(ABC): session. """ - _interactive_session: SSHClient + _interactive_session: InteractiveRemoteSession _stdin: channel.ChannelStdinFile _stdout: channel.ChannelFile _ssh_channel: Channel @@ -60,7 +66,7 @@ class InteractiveShell(ABC): def __init__( self, - interactive_session: SSHClient, + interactive_session: InteractiveRemoteSession, logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: str = "", @@ -80,7 +86,7 @@ def __init__( and no output is gathered within the timeout, an exception is thrown. """ self._interactive_session = interactive_session - self._ssh_channel = self._interactive_session.invoke_shell() + self._ssh_channel = self._interactive_session.session.invoke_shell() self._stdin = self._ssh_channel.makefile_stdin("w") self._stdout = self._ssh_channel.makefile("r") self._ssh_channel.settimeout(timeout) @@ -124,20 +130,34 @@ def send_command(self, command: str, prompt: str | None = None) -> str: Returns: All output in the buffer before expected string. + + Raises: + InteractiveSSHSessionDeadError: The session died while executing the command. + InteractiveSSHTimeoutError: If command was sent but prompt could not be found in + the output before the timeout. """ self._logger.info(f"Sending: '{command}'") if prompt is None: prompt = self._default_prompt - self._stdin.write(f"{command}{self._command_extra_chars}\n") - self._stdin.flush() out: str = "" - for line in self._stdout: - out += line - if prompt in line and not line.rstrip().endswith( - command.rstrip() - ): # ignore line that sent command - break - self._logger.debug(f"Got output: {out}") + try: + self._stdin.write(f"{command}{self._command_extra_chars}\n") + self._stdin.flush() + for line in self._stdout: + out += line + if line.rstrip().endswith(prompt): + break + except TimeoutError as e: + self._logger.exception(e) + self._logger.debug( + f"Prompt ({prompt}) was not found in output from command before timeout." + ) + raise InteractiveSSHTimeoutError(command) from e + except OSError as e: + self._logger.exception(e) + raise InteractiveSSHSessionDeadError(self._interactive_session.hostname) from e + finally: + self._logger.debug(f"Got output: {out}") return out def close(self) -> None: -- 2.44.0 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/3] dts: Improve output gathering in interactive shells 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock @ 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 1 sibling, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-09 9:57 UTC (permalink / raw) To: jspewock, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/3] dts: Improve output gathering in interactive shells 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock 2024-05-09 9:57 ` Luca Vizzarro @ 2024-05-13 14:58 ` Juraj Linkeš 2024-05-15 19:13 ` Jeremy Spewock 1 sibling, 1 reply; 28+ messages in thread From: Juraj Linkeš @ 2024-05-13 14:58 UTC (permalink / raw) To: jspewock Cc: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev Other than the one minor documentation nitpick, Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech> <snip> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py > @@ -124,20 +130,34 @@ def send_command(self, command: str, prompt: str | None = None) -> str: > > Returns: > All output in the buffer before expected string. > + > + Raises: > + InteractiveSSHSessionDeadError: The session died while executing the command. > + InteractiveSSHTimeoutError: If command was sent but prompt could not be found in > + the output before the timeout. > """ > self._logger.info(f"Sending: '{command}'") > if prompt is None: > prompt = self._default_prompt > - self._stdin.write(f"{command}{self._command_extra_chars}\n") > - self._stdin.flush() > out: str = "" > - for line in self._stdout: > - out += line > - if prompt in line and not line.rstrip().endswith( > - command.rstrip() > - ): # ignore line that sent command > - break > - self._logger.debug(f"Got output: {out}") > + try: > + self._stdin.write(f"{command}{self._command_extra_chars}\n") > + self._stdin.flush() > + for line in self._stdout: > + out += line > + if line.rstrip().endswith(prompt): > + break We should document the (currently) hidden assumption of us needing to use the extra command chars to force another prompt in the docstring. > + except TimeoutError as e: > + self._logger.exception(e) > + self._logger.debug( > + f"Prompt ({prompt}) was not found in output from command before timeout." > + ) > + raise InteractiveSSHTimeoutError(command) from e > + except OSError as e: > + self._logger.exception(e) > + raise InteractiveSSHSessionDeadError(self._interactive_session.hostname) from e > + finally: > + self._logger.debug(f"Got output: {out}") > return out > > def close(self) -> None: > -- > 2.44.0 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/3] dts: Improve output gathering in interactive shells 2024-05-13 14:58 ` Juraj Linkeš @ 2024-05-15 19:13 ` Jeremy Spewock 0 siblings, 0 replies; 28+ messages in thread From: Jeremy Spewock @ 2024-05-15 19:13 UTC (permalink / raw) To: Juraj Linkeš Cc: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev On Mon, May 13, 2024 at 10:58 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > > Other than the one minor documentation nitpick, > Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech> > > <snip> > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py > > > @@ -124,20 +130,34 @@ def send_command(self, command: str, prompt: str | None = None) -> str: <snip> > > - self._stdin.write(f"{command}{self._command_extra_chars}\n") > > - self._stdin.flush() > > out: str = "" > > - for line in self._stdout: > > - out += line > > - if prompt in line and not line.rstrip().endswith( > > - command.rstrip() > > - ): # ignore line that sent command > > - break > > - self._logger.debug(f"Got output: {out}") > > + try: > > + self._stdin.write(f"{command}{self._command_extra_chars}\n") > > + self._stdin.flush() > > + for line in self._stdout: > > + out += line > > + if line.rstrip().endswith(prompt): > > + break > > We should document the (currently) hidden assumption of us needing to > use the extra command chars to force another prompt in the docstring. > This is a good point, it is mostly internal knowledge currently so it would be good to explain this more. I'll add it to the comment documenting what the class var is for. <snip> > > 2.44.0 > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock @ 2024-05-01 16:16 ` jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock ` (2 subsequent siblings) 4 siblings, 2 replies; 28+ messages in thread From: jspewock @ 2024-05-01 16:16 UTC (permalink / raw) To: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> When this XML-RPC server implementation was added, the docstring had to be shortened in order to reduce the chances of this race condition being encountered. Now that this race condition issue is resolved, the full docstring can be restored. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- .../testbed_model/traffic_generator/scapy.py | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py index df3069d516..d0e0a7c64e 100644 --- a/dts/framework/testbed_model/traffic_generator/scapy.py +++ b/dts/framework/testbed_model/traffic_generator/scapy.py @@ -128,9 +128,53 @@ def scapy_send_packets(xmlrpc_packets: list[xmlrpc.client.Binary], send_iface: s class QuittableXMLRPCServer(SimpleXMLRPCServer): - """Basic XML-RPC server. + r"""Basic XML-RPC server. The server may be augmented by functions serializable by the :mod:`marshal` module. + + Example: + :: + + def hello_world(): + # to be sent to the XML-RPC server + print("Hello World!") + + # start the XML-RPC server on the remote node + # the example assumes you're already connect to a tg_node + # this is done by starting a Python shell on the remote node + from framework.remote_session import PythonShell + session = tg_node.create_interactive_shell(PythonShell, timeout=5, privileged=True) + + # then importing the modules needed to run the server + # and the modules for any functions later added to the server + session.send_command("import xmlrpc") + session.send_command("from xmlrpc.server import SimpleXMLRPCServer") + + # sending the source code of this class to the Python shell + from xmlrpc.server import SimpleXMLRPCServer + src = inspect.getsource(QuittableXMLRPCServer) + src = "\n".join([l for l in src.splitlines() if not l.isspace() and l != ""]) + spacing = "\n" * 4 + session.send_command(spacing + src + spacing) + + # then starting the server with: + command = "s = QuittableXMLRPCServer(('0.0.0.0', {listen_port}));s.serve_forever()" + session.send_command(command, "XMLRPC OK") + + # now the server is running on the remote node and we can add functions to it + # first connect to the server from the execution node + import xmlrpc.client + server_url = f"http://{tg_node.config.hostname}:8000" + rpc_server_proxy = xmlrpc.client.ServerProxy(server_url) + + # get the function bytes to send + import marshal + function_bytes = marshal.dumps(hello_world.__code__) + rpc_server_proxy.add_rpc_function(hello_world.__name__, function_bytes) + + # now we can execute the function on the server + xmlrpc_binary_recv: xmlrpc.client.Binary = rpc_server_proxy.hello_world() + print(str(xmlrpc_binary_recv)) """ def __init__(self, *args, **kwargs): -- 2.44.0 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server 2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock @ 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 1 sibling, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-09 9:57 UTC (permalink / raw) To: jspewock, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server 2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock 2024-05-09 9:57 ` Luca Vizzarro @ 2024-05-13 14:58 ` Juraj Linkeš 1 sibling, 0 replies; 28+ messages in thread From: Juraj Linkeš @ 2024-05-13 14:58 UTC (permalink / raw) To: jspewock Cc: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech> On Wed, May 1, 2024 at 6:17 PM <jspewock@iol.unh.edu> wrote: > > From: Jeremy Spewock <jspewock@iol.unh.edu> > > When this XML-RPC server implementation was added, the docstring had to > be shortened in order to reduce the chances of this race condition being > encountered. Now that this race condition issue is resolved, the full > docstring can be restored. > > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 3/3] dts: Improve logging for interactive shells 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock 2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock @ 2024-05-01 16:16 ` jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 15:02 ` Juraj Linkeš 2024-05-09 9:59 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging Luca Vizzarro 2024-05-29 19:49 ` [PATCH v3 " jspewock 4 siblings, 2 replies; 28+ messages in thread From: jspewock @ 2024-05-01 16:16 UTC (permalink / raw) To: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> The messages being logged by interactive shells currently are using the same logger as the node they were created from. Because of this, when sending interactive commands, the logs make no distinction between when you are sending a command directly to the host and when you are using an interactive shell on the host. This change adds names to interactive shells so that they are able to use their own loggers with distinct names. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/framework/remote_session/interactive_shell.py | 9 +++++---- dts/framework/testbed_model/node.py | 7 +++++++ dts/framework/testbed_model/os_session.py | 6 ++++-- dts/framework/testbed_model/sut_node.py | 7 ++++++- dts/framework/testbed_model/traffic_generator/scapy.py | 2 +- 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 0b0ccdb545..eb9c9b6843 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -24,7 +24,7 @@ InteractiveSSHSessionDeadError, InteractiveSSHTimeoutError, ) -from framework.logger import DTSLogger +from framework.logger import DTSLogger, get_dts_logger from framework.settings import SETTINGS from .interactive_remote_session import InteractiveRemoteSession @@ -66,8 +66,8 @@ class InteractiveShell(ABC): def __init__( self, + name: str, interactive_session: InteractiveRemoteSession, - logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: str = "", timeout: float = SETTINGS.timeout, @@ -75,8 +75,9 @@ def __init__( """Create an SSH channel during initialization. Args: + name: Name for the interactive shell to use for logging. This name will be appended to + the name of the underlying node which it is running on. interactive_session: The SSH session dedicated to interactive shells. - logger: The logger instance this session will use. get_privileged_command: A method for modifying a command to allow it to use elevated privileges. If :data:`None`, the application will not be started with elevated privileges. @@ -91,7 +92,7 @@ def __init__( self._stdout = self._ssh_channel.makefile("r") self._ssh_channel.settimeout(timeout) self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams - self._logger = logger + self._logger = get_dts_logger(f"{interactive_session._node_config.name}.{name}") self._timeout = timeout self._app_args = app_args self._start_application(get_privileged_command) diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py index 74061f6262..a5beeae7e7 100644 --- a/dts/framework/testbed_model/node.py +++ b/dts/framework/testbed_model/node.py @@ -199,6 +199,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float = SETTINGS.timeout, privileged: bool = False, + name: str = "", app_args: str = "", ) -> InteractiveShellType: """Factory for interactive session handlers. @@ -210,6 +211,8 @@ def create_interactive_shell( timeout: Timeout for reading output from the SSH channel. If you are reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: The name to use for the interactive application in the logs. If not specified, + this will default to the name of `shell_cls`. app_args: The arguments to be passed to the application. Returns: @@ -218,10 +221,14 @@ def create_interactive_shell( if not shell_cls.dpdk_app: shell_cls.path = self.main_session.join_remote_path(shell_cls.path) + if name == "": + name = shell_cls.__name__ + return self.main_session.create_interactive_shell( shell_cls, timeout, privileged, + name, app_args, ) diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py index d5bf7e0401..1f6d8257ed 100644 --- a/dts/framework/testbed_model/os_session.py +++ b/dts/framework/testbed_model/os_session.py @@ -134,6 +134,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float, privileged: bool, + name: str, app_args: str, ) -> InteractiveShellType: """Factory for interactive session handlers. @@ -146,14 +147,15 @@ def create_interactive_shell( reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: Name for the shell to use in the logs. app_args: The arguments to be passed to the application. Returns: An instance of the desired interactive application shell. """ return shell_cls( - self.interactive_session.session, - self._logger, + name, + self.interactive_session, self._get_privileged_command if privileged else None, app_args, timeout, diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py index 97aa26d419..0bddef6f44 100644 --- a/dts/framework/testbed_model/sut_node.py +++ b/dts/framework/testbed_model/sut_node.py @@ -442,6 +442,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float = SETTINGS.timeout, privileged: bool = False, + name: str = "", app_parameters: str = "", eal_parameters: EalParameters | None = None, ) -> InteractiveShellType: @@ -459,6 +460,8 @@ def create_interactive_shell( reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: The name to use for the interactive application in the logs. If not specified, + this will default to the name `shell_cls`. eal_parameters: List of EAL parameters to use to launch the app. If this isn't provided or an empty string is passed, it will default to calling :meth:`create_eal_parameters`. @@ -478,7 +481,9 @@ def create_interactive_shell( self.remote_dpdk_build_dir, shell_cls.path ) - return super().create_interactive_shell(shell_cls, timeout, privileged, app_parameters) + return super().create_interactive_shell( + shell_cls, timeout, privileged, name, app_parameters + ) def bind_ports_to_driver(self, for_dpdk: bool = True) -> None: """Bind all ports on the SUT to a driver. diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py index d0e0a7c64e..5676235119 100644 --- a/dts/framework/testbed_model/traffic_generator/scapy.py +++ b/dts/framework/testbed_model/traffic_generator/scapy.py @@ -262,7 +262,7 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig): ), "Linux is the only supported OS for scapy traffic generation" self.session = self._tg_node.create_interactive_shell( - PythonShell, timeout=5, privileged=True + PythonShell, timeout=5, privileged=True, name="ScapyXMLRPCServer" ) # import libs in remote python console -- 2.44.0 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/3] dts: Improve logging for interactive shells 2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock @ 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 15:02 ` Juraj Linkeš 1 sibling, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-09 9:57 UTC (permalink / raw) To: jspewock, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/3] dts: Improve logging for interactive shells 2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock 2024-05-09 9:57 ` Luca Vizzarro @ 2024-05-13 15:02 ` Juraj Linkeš 2024-05-15 19:23 ` Jeremy Spewock 1 sibling, 1 reply; 28+ messages in thread From: Juraj Linkeš @ 2024-05-13 15:02 UTC (permalink / raw) To: jspewock Cc: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev Good idea. And again, with the one minor point below, Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech> > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py > index 0b0ccdb545..eb9c9b6843 100644 > --- a/dts/framework/remote_session/interactive_shell.py > +++ b/dts/framework/remote_session/interactive_shell.py > @@ -91,7 +92,7 @@ def __init__( > self._stdout = self._ssh_channel.makefile("r") > self._ssh_channel.settimeout(timeout) > self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams > - self._logger = logger > + self._logger = get_dts_logger(f"{interactive_session._node_config.name}.{name}") We should make interactive_session._node_config public since we're referencing it outside the class. > self._timeout = timeout > self._app_args = app_args > self._start_application(get_privileged_command) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 3/3] dts: Improve logging for interactive shells 2024-05-13 15:02 ` Juraj Linkeš @ 2024-05-15 19:23 ` Jeremy Spewock 0 siblings, 0 replies; 28+ messages in thread From: Jeremy Spewock @ 2024-05-15 19:23 UTC (permalink / raw) To: Juraj Linkeš Cc: Luca.Vizzarro, wathsala.vithanage, yoan.picchi, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev On Mon, May 13, 2024 at 11:03 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: > > Good idea. And again, with the one minor point below, > Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech> > > > diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py > > index 0b0ccdb545..eb9c9b6843 100644 > > --- a/dts/framework/remote_session/interactive_shell.py > > +++ b/dts/framework/remote_session/interactive_shell.py > > > @@ -91,7 +92,7 @@ def __init__( > > self._stdout = self._ssh_channel.makefile("r") > > self._ssh_channel.settimeout(timeout) > > self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams > > - self._logger = logger > > + self._logger = get_dts_logger(f"{interactive_session._node_config.name}.{name}") > > We should make interactive_session._node_config public since we're > referencing it outside the class. good point, I'll make this change as well. > > > self._timeout = timeout > > self._app_args = app_args > > self._start_application(get_privileged_command) ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] Improve interactive shell output gathering and logging 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock ` (2 preceding siblings ...) 2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock @ 2024-05-09 9:59 ` Luca Vizzarro 2024-05-20 15:08 ` Nicholas Pratte 2024-05-29 19:49 ` [PATCH v3 " jspewock 4 siblings, 1 reply; 28+ messages in thread From: Luca Vizzarro @ 2024-05-09 9:59 UTC (permalink / raw) To: jspewock, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli Cc: dev On 01/05/2024 17:16, jspewock@iol.unh.edu wrote: > This version addresses comments from the last and adds an additional > improvement to the logging output that comes from interactive shells. > The new way of handling logging allows for not only specification of the > host that the command is being sent to like before, but also the type of > shell that you are addressing. This is beneficial for 2 reasons: > > 1. Less ambiguity on if a command is being sent to the host directly or > into an interactive shell > 2. Ability to name shells so that there is distinction in the logs > between two different interactive shells of the same type that serve a > different purpose (e.g. a python shell and a python shell that is > serving and XML-RPC server for scapy interactions) Hi Jeremy, Thank you for your patches! Everything looks good to me. Best, Luca ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 0/3] Improve interactive shell output gathering and logging 2024-05-09 9:59 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging Luca Vizzarro @ 2024-05-20 15:08 ` Nicholas Pratte 0 siblings, 0 replies; 28+ messages in thread From: Nicholas Pratte @ 2024-05-20 15:08 UTC (permalink / raw) To: Luca Vizzarro Cc: jspewock, wathsala.vithanage, yoan.picchi, juraj.linkes, paul.szczepanek, probb, thomas, Honnappa.Nagarahalli, dev Acked-by: Nicholas Pratte <npratte@iol.unh.edu> On Thu, May 9, 2024 at 5:59 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote: > > On 01/05/2024 17:16, jspewock@iol.unh.edu wrote: > > This version addresses comments from the last and adds an additional > > improvement to the logging output that comes from interactive shells. > > The new way of handling logging allows for not only specification of the > > host that the command is being sent to like before, but also the type of > > shell that you are addressing. This is beneficial for 2 reasons: > > > > 1. Less ambiguity on if a command is being sent to the host directly or > > into an interactive shell > > 2. Ability to name shells so that there is distinction in the logs > > between two different interactive shells of the same type that serve a > > different purpose (e.g. a python shell and a python shell that is > > serving and XML-RPC server for scapy interactions) > > Hi Jeremy, > > Thank you for your patches! Everything looks good to me. > > Best, > Luca ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 0/3] Improve interactive shell output gathering and logging 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock ` (3 preceding siblings ...) 2024-05-09 9:59 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging Luca Vizzarro @ 2024-05-29 19:49 ` jspewock 2024-05-29 19:49 ` [PATCH v3 1/3] dts: Improve output gathering in interactive shells jspewock ` (2 more replies) 4 siblings, 3 replies; 28+ messages in thread From: jspewock @ 2024-05-29 19:49 UTC (permalink / raw) To: paul.szczepanek, wathsala.vithanage, probb, npratte, Luca.Vizzarro, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> This version addresses the small comments that were on the previous version such as documentation of variable name changes. Jeremy Spewock (3): dts: Improve output gathering in interactive shells dts: Add missing docstring from XML-RPC server dts: Improve logging for interactive shells dts/framework/exception.py | 66 ++++++++++++------- .../interactive_remote_session.py | 5 +- .../remote_session/interactive_shell.py | 60 ++++++++++++----- dts/framework/testbed_model/node.py | 7 ++ dts/framework/testbed_model/os_session.py | 6 +- dts/framework/testbed_model/sut_node.py | 7 +- .../testbed_model/traffic_generator/scapy.py | 48 +++++++++++++- 7 files changed, 152 insertions(+), 47 deletions(-) -- 2.45.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 1/3] dts: Improve output gathering in interactive shells 2024-05-29 19:49 ` [PATCH v3 " jspewock @ 2024-05-29 19:49 ` jspewock 2024-05-31 16:49 ` Luca Vizzarro 2024-05-29 19:49 ` [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server jspewock 2024-05-29 19:49 ` [PATCH v3 3/3] dts: Improve logging for interactive shells jspewock 2 siblings, 1 reply; 28+ messages in thread From: jspewock @ 2024-05-29 19:49 UTC (permalink / raw) To: paul.szczepanek, wathsala.vithanage, probb, npratte, Luca.Vizzarro, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> The current implementation of consuming output from interactive shells relies on being able to find an expected prompt somewhere within the output buffer after sending the command. This is useful in situations where the prompt does not appear in the output itself, but in some practical cases (such as the starting of an XML-RPC server for scapy) the prompt exists in one of the commands sent to the shell and this can cause the command to exit early and creates a race condition between the server starting and the first command being sent to the server. This patch addresses this problem by searching for a line that strictly ends with the provided prompt, rather than one that simply contains it, so that the detection that a command is finished is more consistent. It also adds a catch to detect when a command times out before finding the prompt or the underlying SSH session dies so that the exception can be wrapped into a more explicit one and be more consistent with the non-interactive shells. Bugzilla ID: 1359 Fixes: 88489c0501af ("dts: add smoke tests") Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- dts/framework/exception.py | 66 ++++++++++++------- .../remote_session/interactive_shell.py | 51 ++++++++++---- 2 files changed, 81 insertions(+), 36 deletions(-) diff --git a/dts/framework/exception.py b/dts/framework/exception.py index cce1e0231a..627190c781 100644 --- a/dts/framework/exception.py +++ b/dts/framework/exception.py @@ -48,26 +48,6 @@ class DTSError(Exception): severity: ClassVar[ErrorSeverity] = ErrorSeverity.GENERIC_ERR -class SSHTimeoutError(DTSError): - """The SSH execution of a command timed out.""" - - #: - severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR - _command: str - - def __init__(self, command: str): - """Define the meaning of the first argument. - - Args: - command: The executed command. - """ - self._command = command - - def __str__(self) -> str: - """Add some context to the string representation.""" - return f"{self._command} execution timed out." - - class SSHConnectionError(DTSError): """An unsuccessful SSH connection.""" @@ -95,8 +75,42 @@ def __str__(self) -> str: return message -class SSHSessionDeadError(DTSError): - """The SSH session is no longer alive.""" +class _SSHTimeoutError(DTSError): + """The execution of a command via SSH timed out. + + This class is private and meant to be raised as its interactive and non-interactive variants. + """ + + #: + severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR + _command: str + + def __init__(self, command: str): + """Define the meaning of the first argument. + + Args: + command: The executed command. + """ + self._command = command + + def __str__(self) -> str: + """Add some context to the string representation.""" + return f"{self._command} execution timed out." + + +class SSHTimeoutError(_SSHTimeoutError): + """The execution of a command on a non-interactive SSH session timed out.""" + + +class InteractiveSSHTimeoutError(_SSHTimeoutError): + """The execution of a command on an interactive SSH session timed out.""" + + +class _SSHSessionDeadError(DTSError): + """The SSH session is no longer alive. + + This class is private and meant to be raised as its interactive and non-interactive variants. + """ #: severity: ClassVar[ErrorSeverity] = ErrorSeverity.SSH_ERR @@ -115,6 +129,14 @@ def __str__(self) -> str: return f"SSH session with {self._host} has died." +class SSHSessionDeadError(_SSHSessionDeadError): + """Non-interactive SSH session has died.""" + + +class InteractiveSSHSessionDeadError(_SSHSessionDeadError): + """Interactive SSH session as died.""" + + class ConfigurationError(DTSError): """An invalid configuration.""" diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 5cfe202e15..148907f645 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -18,11 +18,17 @@ from pathlib import PurePath from typing import Callable, ClassVar -from paramiko import Channel, SSHClient, channel # type: ignore[import] +from paramiko import Channel, channel # type: ignore[import] +from framework.exception import ( + InteractiveSSHSessionDeadError, + InteractiveSSHTimeoutError, +) from framework.logger import DTSLogger from framework.settings import SETTINGS +from .interactive_remote_session import InteractiveRemoteSession + class InteractiveShell(ABC): """The base class for managing interactive shells. @@ -34,7 +40,7 @@ class InteractiveShell(ABC): session. """ - _interactive_session: SSHClient + _interactive_session: InteractiveRemoteSession _stdin: channel.ChannelStdinFile _stdout: channel.ChannelFile _ssh_channel: Channel @@ -48,7 +54,10 @@ class InteractiveShell(ABC): #: Extra characters to add to the end of every command #: before sending them. This is often overridden by subclasses and is - #: most commonly an additional newline character. + #: most commonly an additional newline character. This additional newline + #: character is used to force the line that is currently awaiting input + #: into the stdout buffer so that it can be consumed and checked against + #: the expected prompt. _command_extra_chars: ClassVar[str] = "" #: Path to the executable to start the interactive application. @@ -60,7 +69,7 @@ class InteractiveShell(ABC): def __init__( self, - interactive_session: SSHClient, + interactive_session: InteractiveRemoteSession, logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: str = "", @@ -80,7 +89,7 @@ def __init__( and no output is gathered within the timeout, an exception is thrown. """ self._interactive_session = interactive_session - self._ssh_channel = self._interactive_session.invoke_shell() + self._ssh_channel = self._interactive_session.session.invoke_shell() self._stdin = self._ssh_channel.makefile_stdin("w") self._stdout = self._ssh_channel.makefile("r") self._ssh_channel.settimeout(timeout) @@ -124,20 +133,34 @@ def send_command(self, command: str, prompt: str | None = None) -> str: Returns: All output in the buffer before expected string. + + Raises: + InteractiveSSHSessionDeadError: The session died while executing the command. + InteractiveSSHTimeoutError: If command was sent but prompt could not be found in + the output before the timeout. """ self._logger.info(f"Sending: '{command}'") if prompt is None: prompt = self._default_prompt - self._stdin.write(f"{command}{self._command_extra_chars}\n") - self._stdin.flush() out: str = "" - for line in self._stdout: - out += line - if prompt in line and not line.rstrip().endswith( - command.rstrip() - ): # ignore line that sent command - break - self._logger.debug(f"Got output: {out}") + try: + self._stdin.write(f"{command}{self._command_extra_chars}\n") + self._stdin.flush() + for line in self._stdout: + out += line + if line.rstrip().endswith(prompt): + break + except TimeoutError as e: + self._logger.exception(e) + self._logger.debug( + f"Prompt ({prompt}) was not found in output from command before timeout." + ) + raise InteractiveSSHTimeoutError(command) from e + except OSError as e: + self._logger.exception(e) + raise InteractiveSSHSessionDeadError(self._interactive_session.hostname) from e + finally: + self._logger.debug(f"Got output: {out}") return out def close(self) -> None: -- 2.45.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/3] dts: Improve output gathering in interactive shells 2024-05-29 19:49 ` [PATCH v3 1/3] dts: Improve output gathering in interactive shells jspewock @ 2024-05-31 16:49 ` Luca Vizzarro 0 siblings, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-31 16:49 UTC (permalink / raw) To: jspewock, paul.szczepanek, wathsala.vithanage, probb, npratte, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server 2024-05-29 19:49 ` [PATCH v3 " jspewock 2024-05-29 19:49 ` [PATCH v3 1/3] dts: Improve output gathering in interactive shells jspewock @ 2024-05-29 19:49 ` jspewock 2024-05-31 16:50 ` Luca Vizzarro 2024-05-29 19:49 ` [PATCH v3 3/3] dts: Improve logging for interactive shells jspewock 2 siblings, 1 reply; 28+ messages in thread From: jspewock @ 2024-05-29 19:49 UTC (permalink / raw) To: paul.szczepanek, wathsala.vithanage, probb, npratte, Luca.Vizzarro, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> When this XML-RPC server implementation was added, the docstring had to be shortened in order to reduce the chances of this race condition being encountered. Now that this race condition issue is resolved, the full docstring can be restored. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- .../testbed_model/traffic_generator/scapy.py | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py index df3069d516..d0e0a7c64e 100644 --- a/dts/framework/testbed_model/traffic_generator/scapy.py +++ b/dts/framework/testbed_model/traffic_generator/scapy.py @@ -128,9 +128,53 @@ def scapy_send_packets(xmlrpc_packets: list[xmlrpc.client.Binary], send_iface: s class QuittableXMLRPCServer(SimpleXMLRPCServer): - """Basic XML-RPC server. + r"""Basic XML-RPC server. The server may be augmented by functions serializable by the :mod:`marshal` module. + + Example: + :: + + def hello_world(): + # to be sent to the XML-RPC server + print("Hello World!") + + # start the XML-RPC server on the remote node + # the example assumes you're already connect to a tg_node + # this is done by starting a Python shell on the remote node + from framework.remote_session import PythonShell + session = tg_node.create_interactive_shell(PythonShell, timeout=5, privileged=True) + + # then importing the modules needed to run the server + # and the modules for any functions later added to the server + session.send_command("import xmlrpc") + session.send_command("from xmlrpc.server import SimpleXMLRPCServer") + + # sending the source code of this class to the Python shell + from xmlrpc.server import SimpleXMLRPCServer + src = inspect.getsource(QuittableXMLRPCServer) + src = "\n".join([l for l in src.splitlines() if not l.isspace() and l != ""]) + spacing = "\n" * 4 + session.send_command(spacing + src + spacing) + + # then starting the server with: + command = "s = QuittableXMLRPCServer(('0.0.0.0', {listen_port}));s.serve_forever()" + session.send_command(command, "XMLRPC OK") + + # now the server is running on the remote node and we can add functions to it + # first connect to the server from the execution node + import xmlrpc.client + server_url = f"http://{tg_node.config.hostname}:8000" + rpc_server_proxy = xmlrpc.client.ServerProxy(server_url) + + # get the function bytes to send + import marshal + function_bytes = marshal.dumps(hello_world.__code__) + rpc_server_proxy.add_rpc_function(hello_world.__name__, function_bytes) + + # now we can execute the function on the server + xmlrpc_binary_recv: xmlrpc.client.Binary = rpc_server_proxy.hello_world() + print(str(xmlrpc_binary_recv)) """ def __init__(self, *args, **kwargs): -- 2.45.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server 2024-05-29 19:49 ` [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server jspewock @ 2024-05-31 16:50 ` Luca Vizzarro 0 siblings, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-31 16:50 UTC (permalink / raw) To: jspewock, paul.szczepanek, wathsala.vithanage, probb, npratte, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/3] dts: Improve logging for interactive shells 2024-05-29 19:49 ` [PATCH v3 " jspewock 2024-05-29 19:49 ` [PATCH v3 1/3] dts: Improve output gathering in interactive shells jspewock 2024-05-29 19:49 ` [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server jspewock @ 2024-05-29 19:49 ` jspewock 2024-05-31 16:50 ` Luca Vizzarro 2 siblings, 1 reply; 28+ messages in thread From: jspewock @ 2024-05-29 19:49 UTC (permalink / raw) To: paul.szczepanek, wathsala.vithanage, probb, npratte, Luca.Vizzarro, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev, Jeremy Spewock From: Jeremy Spewock <jspewock@iol.unh.edu> The messages being logged by interactive shells currently are using the same logger as the node they were created from. Because of this, when sending interactive commands, the logs make no distinction between when you are sending a command directly to the host and when you are using an interactive shell on the host. This change adds names to interactive shells so that they are able to use their own loggers with distinct names. Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu> --- .../remote_session/interactive_remote_session.py | 5 +++-- dts/framework/remote_session/interactive_shell.py | 9 +++++---- dts/framework/testbed_model/node.py | 7 +++++++ dts/framework/testbed_model/os_session.py | 6 ++++-- dts/framework/testbed_model/sut_node.py | 7 ++++++- dts/framework/testbed_model/traffic_generator/scapy.py | 2 +- 6 files changed, 26 insertions(+), 10 deletions(-) diff --git a/dts/framework/remote_session/interactive_remote_session.py b/dts/framework/remote_session/interactive_remote_session.py index c50790db79..4ddad4f428 100644 --- a/dts/framework/remote_session/interactive_remote_session.py +++ b/dts/framework/remote_session/interactive_remote_session.py @@ -39,6 +39,7 @@ class InteractiveRemoteSession: password: Password of the user connecting to the host. This will default to an empty string if a password is not provided. session: The underlying paramiko connection. + node_config: The configuration of the node that this session is connected to. Raises: SSHConnectionError: There is an error creating the SSH connection. @@ -50,8 +51,8 @@ class InteractiveRemoteSession: username: str password: str session: SSHClient + node_config: NodeConfiguration _logger: DTSLogger - _node_config: NodeConfiguration _transport: Transport | None def __init__(self, node_config: NodeConfiguration, logger: DTSLogger) -> None: @@ -61,7 +62,7 @@ def __init__(self, node_config: NodeConfiguration, logger: DTSLogger) -> None: node_config: The test run configuration of the node to connect to. logger: The logger instance this session will use. """ - self._node_config = node_config + self.node_config = node_config self._logger = logger self.hostname = node_config.hostname self.username = node_config.user diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py index 148907f645..edc8f11443 100644 --- a/dts/framework/remote_session/interactive_shell.py +++ b/dts/framework/remote_session/interactive_shell.py @@ -24,7 +24,7 @@ InteractiveSSHSessionDeadError, InteractiveSSHTimeoutError, ) -from framework.logger import DTSLogger +from framework.logger import DTSLogger, get_dts_logger from framework.settings import SETTINGS from .interactive_remote_session import InteractiveRemoteSession @@ -69,8 +69,8 @@ class InteractiveShell(ABC): def __init__( self, + name: str, interactive_session: InteractiveRemoteSession, - logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: str = "", timeout: float = SETTINGS.timeout, @@ -78,8 +78,9 @@ def __init__( """Create an SSH channel during initialization. Args: + name: Name for the interactive shell to use for logging. This name will be appended to + the name of the underlying node which it is running on. interactive_session: The SSH session dedicated to interactive shells. - logger: The logger instance this session will use. get_privileged_command: A method for modifying a command to allow it to use elevated privileges. If :data:`None`, the application will not be started with elevated privileges. @@ -94,7 +95,7 @@ def __init__( self._stdout = self._ssh_channel.makefile("r") self._ssh_channel.settimeout(timeout) self._ssh_channel.set_combine_stderr(True) # combines stdout and stderr streams - self._logger = logger + self._logger = get_dts_logger(f"{interactive_session.node_config.name}.{name}") self._timeout = timeout self._app_args = app_args self._start_application(get_privileged_command) diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py index 74061f6262..a5beeae7e7 100644 --- a/dts/framework/testbed_model/node.py +++ b/dts/framework/testbed_model/node.py @@ -199,6 +199,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float = SETTINGS.timeout, privileged: bool = False, + name: str = "", app_args: str = "", ) -> InteractiveShellType: """Factory for interactive session handlers. @@ -210,6 +211,8 @@ def create_interactive_shell( timeout: Timeout for reading output from the SSH channel. If you are reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: The name to use for the interactive application in the logs. If not specified, + this will default to the name of `shell_cls`. app_args: The arguments to be passed to the application. Returns: @@ -218,10 +221,14 @@ def create_interactive_shell( if not shell_cls.dpdk_app: shell_cls.path = self.main_session.join_remote_path(shell_cls.path) + if name == "": + name = shell_cls.__name__ + return self.main_session.create_interactive_shell( shell_cls, timeout, privileged, + name, app_args, ) diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py index d5bf7e0401..1f6d8257ed 100644 --- a/dts/framework/testbed_model/os_session.py +++ b/dts/framework/testbed_model/os_session.py @@ -134,6 +134,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float, privileged: bool, + name: str, app_args: str, ) -> InteractiveShellType: """Factory for interactive session handlers. @@ -146,14 +147,15 @@ def create_interactive_shell( reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: Name for the shell to use in the logs. app_args: The arguments to be passed to the application. Returns: An instance of the desired interactive application shell. """ return shell_cls( - self.interactive_session.session, - self._logger, + name, + self.interactive_session, self._get_privileged_command if privileged else None, app_args, timeout, diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py index 97aa26d419..0bddef6f44 100644 --- a/dts/framework/testbed_model/sut_node.py +++ b/dts/framework/testbed_model/sut_node.py @@ -442,6 +442,7 @@ def create_interactive_shell( shell_cls: Type[InteractiveShellType], timeout: float = SETTINGS.timeout, privileged: bool = False, + name: str = "", app_parameters: str = "", eal_parameters: EalParameters | None = None, ) -> InteractiveShellType: @@ -459,6 +460,8 @@ def create_interactive_shell( reading from the buffer and don't receive any data within the timeout it will throw an error. privileged: Whether to run the shell with administrative privileges. + name: The name to use for the interactive application in the logs. If not specified, + this will default to the name `shell_cls`. eal_parameters: List of EAL parameters to use to launch the app. If this isn't provided or an empty string is passed, it will default to calling :meth:`create_eal_parameters`. @@ -478,7 +481,9 @@ def create_interactive_shell( self.remote_dpdk_build_dir, shell_cls.path ) - return super().create_interactive_shell(shell_cls, timeout, privileged, app_parameters) + return super().create_interactive_shell( + shell_cls, timeout, privileged, name, app_parameters + ) def bind_ports_to_driver(self, for_dpdk: bool = True) -> None: """Bind all ports on the SUT to a driver. diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py b/dts/framework/testbed_model/traffic_generator/scapy.py index d0e0a7c64e..5676235119 100644 --- a/dts/framework/testbed_model/traffic_generator/scapy.py +++ b/dts/framework/testbed_model/traffic_generator/scapy.py @@ -262,7 +262,7 @@ def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig): ), "Linux is the only supported OS for scapy traffic generation" self.session = self._tg_node.create_interactive_shell( - PythonShell, timeout=5, privileged=True + PythonShell, timeout=5, privileged=True, name="ScapyXMLRPCServer" ) # import libs in remote python console -- 2.45.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 3/3] dts: Improve logging for interactive shells 2024-05-29 19:49 ` [PATCH v3 3/3] dts: Improve logging for interactive shells jspewock @ 2024-05-31 16:50 ` Luca Vizzarro 0 siblings, 0 replies; 28+ messages in thread From: Luca Vizzarro @ 2024-05-31 16:50 UTC (permalink / raw) To: jspewock, paul.szczepanek, wathsala.vithanage, probb, npratte, thomas, juraj.linkes, Honnappa.Nagarahalli, yoan.picchi Cc: dev Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com> ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2024-05-31 16:50 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-12 17:25 [PATCH v1 0/2] Improve interactive shell output gathering jspewock 2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock 2024-04-03 9:00 ` Juraj Linkeš 2024-04-08 16:20 ` Jeremy Spewock 2024-04-10 10:20 ` Juraj Linkeš 2024-03-12 17:25 ` [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server jspewock 2024-04-24 13:42 ` Patrick Robb 2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock 2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 2024-05-15 19:13 ` Jeremy Spewock 2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 14:58 ` Juraj Linkeš 2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock 2024-05-09 9:57 ` Luca Vizzarro 2024-05-13 15:02 ` Juraj Linkeš 2024-05-15 19:23 ` Jeremy Spewock 2024-05-09 9:59 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging Luca Vizzarro 2024-05-20 15:08 ` Nicholas Pratte 2024-05-29 19:49 ` [PATCH v3 " jspewock 2024-05-29 19:49 ` [PATCH v3 1/3] dts: Improve output gathering in interactive shells jspewock 2024-05-31 16:49 ` Luca Vizzarro 2024-05-29 19:49 ` [PATCH v3 2/3] dts: Add missing docstring from XML-RPC server jspewock 2024-05-31 16:50 ` Luca Vizzarro 2024-05-29 19:49 ` [PATCH v3 3/3] dts: Improve logging for interactive shells jspewock 2024-05-31 16:50 ` Luca Vizzarro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).