Re: [PATCH v3 3/3] dts: Improve logging for interactive shells

[Nicholas Pratte <npratte@iol.unh.edu>]

Ran a couple test suites to observe the output. As expected,
everything works fine.

Tested-by: Nicholas Pratte <npratte@iol.unh.edu>
Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

On Wed, May 29, 2024 at 3:49 PM <jspewock@iol.unh.edu> wrote:
>
> 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
>