[v4,4/9] dts: add ssh pexpect library
Checks
Commit Message
The library uses the pexpect python library and implements connection to
a node and two ways to interact with the node:
1. Send a string with specified prompt which will be matched after
the string has been sent to the node.
2. Send a command to be executed. No prompt is specified here.
Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
dts/framework/exception.py | 57 ++++++++++
dts/framework/ssh_pexpect.py | 205 +++++++++++++++++++++++++++++++++++
dts/framework/utils.py | 12 ++
3 files changed, 274 insertions(+)
create mode 100644 dts/framework/exception.py
create mode 100644 dts/framework/ssh_pexpect.py
create mode 100644 dts/framework/utils.py
Comments
> -----Original Message-----
> From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Sent: Friday, July 29, 2022 6:56 PM
> To: thomas@monjalon.net; david.marchand@redhat.com; Randles, Ronan
> <ronan.randles@intel.com>; Honnappa.Nagarahalli@arm.com;
> ohilyard@iol.unh.edu; Tu, Lijuan <lijuan.tu@intel.com>
> Cc: dev@dpdk.org; Juraj Linkeš <juraj.linkes@pantheon.tech>
> Subject: [PATCH v4 4/9] dts: add ssh pexpect library
>
> The library uses the pexpect python library and implements connection to a node
> and two ways to interact with the node:
> 1. Send a string with specified prompt which will be matched after
> the string has been sent to the node.
> 2. Send a command to be executed. No prompt is specified here.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> dts/framework/exception.py | 57 ++++++++++
> dts/framework/ssh_pexpect.py | 205
> +++++++++++++++++++++++++++++++++++
> dts/framework/utils.py | 12 ++
> 3 files changed, 274 insertions(+)
> create mode 100644 dts/framework/exception.py create mode 100644
> dts/framework/ssh_pexpect.py create mode 100644 dts/framework/utils.py
>
Reviewed-by: Lijuan Tu <lijuan.tu@intel.com>
Thanks,
Lijuan
On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> The library uses the pexpect python library and implements connection to
> a node and two ways to interact with the node:
> 1. Send a string with specified prompt which will be matched after
> the string has been sent to the node.
> 2. Send a command to be executed. No prompt is specified here.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Comments inline below.
Thanks,
/Bruce
> ---
> dts/framework/exception.py | 57 ++++++++++
> dts/framework/ssh_pexpect.py | 205 +++++++++++++++++++++++++++++++++++
> dts/framework/utils.py | 12 ++
> 3 files changed, 274 insertions(+)
> create mode 100644 dts/framework/exception.py
> create mode 100644 dts/framework/ssh_pexpect.py
> create mode 100644 dts/framework/utils.py
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> new file mode 100644
> index 0000000000..35e81a4d99
> --- /dev/null
> +++ b/dts/framework/exception.py
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +"""
> +User-defined exceptions used across the framework.
> +"""
> +
> +
> +class TimeoutException(Exception):
> + """
> + Command execution timeout.
> + """
> +
> + command: str
> + output: str
> +
> + def __init__(self, command: str, output: str):
> + self.command = command
> + self.output = output
> +
> + def __str__(self) -> str:
> + return f"TIMEOUT on {self.command}"
> +
> + def get_output(self) -> str:
> + return self.output
> +
> +
> +class SSHConnectionException(Exception):
> + """
> + SSH connection error.
> + """
> +
> + host: str
> +
> + def __init__(self, host: str):
> + self.host = host
> +
> + def __str__(self) -> str:
> + return f"Error trying to connect with {self.host}"
> +
> +
> +class SSHSessionDeadException(Exception):
> + """
> + SSH session is not alive.
> + It can no longer be used.
> + """
> +
> + host: str
> +
> + def __init__(self, host: str):
> + self.host = host
> +
> + def __str__(self) -> str:
> + return f"SSH session with {self.host} has died"
> diff --git a/dts/framework/ssh_pexpect.py b/dts/framework/ssh_pexpect.py
> new file mode 100644
> index 0000000000..e8f64515c0
> --- /dev/null
> +++ b/dts/framework/ssh_pexpect.py
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +import time
> +from typing import Optional
> +
> +from pexpect import pxssh
> +
> +from .exception import SSHConnectionException, SSHSessionDeadException, TimeoutException
> +from .logger import DTSLOG
> +from .utils import GREEN, RED
> +
> +"""
> +The module handles ssh sessions to TG and SUT.
> +It implements the send_expect function to send commands and get output data.
> +"""
> +
> +
> +class SSHPexpect:
> + username: str
> + password: str
> + node: str
> + logger: DTSLOG
> + magic_prompt: str
> +
> + def __init__(
> + self,
> + node: str,
> + username: str,
> + password: Optional[str],
> + logger: DTSLOG,
> + ):
> + self.magic_prompt = "MAGIC PROMPT"
> + self.logger = logger
> +
> + self.node = node
> + self.username = username
> + self.password = password or ""
> + self.logger.info(f"ssh {self.username}@{self.node}")
> +
> + self._connect_host()
> +
> + def _connect_host(self) -> None:
> + """
> + Create connection to assigned node.
> + """
> + retry_times = 10
> + try:
> + if ":" in self.node:
Should this check and the relevant splitting below to assign to self.ip and
self.port, not be don at init when the node is passed in? Certainly the
splitting should probably be done outside the loop, rather than re-doing
the split into ip and port 10 times?
> + while retry_times:
> + self.ip = self.node.split(":")[0]
> + self.port = int(self.node.split(":")[1])
> + self.session = pxssh.pxssh(encoding="utf-8")
> + try:
> + self.session.login(
> + self.ip,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + port=self.port,
> + login_timeout=20,
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + except Exception as e:
> + print(e)
> + time.sleep(2)
> + retry_times -= 1
> + print("retry %d times connecting..." % (10 - retry_times))
> + else:
> + break
> + else:
> + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> + else:
> + self.session = pxssh.pxssh(encoding="utf-8")
> + self.session.login(
> + self.node,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + self.logger.info(f"Connection to {self.node} succeeded")
> + self.send_expect("stty -echo", "#")
> + self.send_expect("stty columns 1000", "#")
> + except Exception as e:
> + print(RED(str(e)))
> + if getattr(self, "port", None):
> + suggestion = (
> + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> + + "is stopped\n"
> + )
I'd suggest using f-strings here to avoid splitting error messages across
lines. They can also be used for strings above too to increase readability.
We should probably look to standardize all strings used in DTS to a single
format - either f-strings or the style given here, rather than using a mix.
> + print(GREEN(suggestion))
> +
> + raise SSHConnectionException(self.node)
> +
> + def send_expect_base(self, command: str, expected: str, timeout: float) -> str:
> + self.clean_session()
> + self.session.PROMPT = expected
> + self.__sendline(command)
> + self.__prompt(command, timeout)
> +
> + before = self.get_output_before()
> + return before
> +
> + def send_expect(
> + self, command: str, expected: str, timeout: float = 15, verify: bool = False
> + ) -> str | int:
> +
> + try:
> + ret = self.send_expect_base(command, expected, timeout)
> + if verify:
> + ret_status = self.send_expect_base("echo $?", expected, timeout)
> + if not int(ret_status):
> + return ret
> + else:
> + self.logger.error("Command: %s failure!" % command)
> + self.logger.error(ret)
> + return int(ret_status)
> + else:
> + return ret
> + except Exception as e:
> + print(
> + RED(
> + "Exception happened in [%s] and output is [%s]"
> + % (command, self.get_output_before())
> + )
> + )
> + raise e
> +
> + def send_command(self, command: str, timeout: float = 1) -> str:
> + try:
> + self.clean_session()
> + self.__sendline(command)
> + except Exception as e:
> + raise e
> +
> + output = self.get_session_before(timeout=timeout)
> + self.session.PROMPT = self.session.UNIQUE_PROMPT
> + self.session.prompt(0.1)
> +
> + return output
> +
> + def clean_session(self) -> None:
> + self.get_session_before(timeout=0.01)
> +
> + def get_session_before(self, timeout: float = 15) -> str:
> + """
> + Get all output before timeout
> + """
> + self.session.PROMPT = self.magic_prompt
> + try:
> + self.session.prompt(timeout)
> + except Exception as e:
> + pass
> +
> + before = self.get_output_all()
> + self.__flush()
> +
> + return before
> +
> + def __flush(self) -> None:
> + """
> + Clear all session buffer
> + """
> + self.session.buffer = ""
> + self.session.before = ""
> +
> + def __prompt(self, command: str, timeout: float) -> None:
> + if not self.session.prompt(timeout):
> + raise TimeoutException(command, self.get_output_all()) from None
> +
> + def __sendline(self, command: str) -> None:
> + if not self.isalive():
> + raise SSHSessionDeadException(self.node)
> + if len(command) == 2 and command.startswith("^"):
> + self.session.sendcontrol(command[1])
> + else:
> + self.session.sendline(command)
> +
> + def get_output_before(self) -> str:
> + if not self.isalive():
> + raise SSHSessionDeadException(self.node)
> + before: list[str] = self.session.before.rsplit("\r\n", 1)
The cast in the middle of pyton code seems strange. Does rsplit not always
return a list value? In quick testing here even doing "".rsplit("x",1)
returns a list with the empty string.
> + if before[0] == "[PEXPECT]":
> + before[0] = ""
> +
> + return before[0]
Might be slightly more readable as:
retval = self.session.before.rsplit("\r\n", 1)[0]
if retval == "[PEXPECT]"
return ""
return retval
> +
> + def get_output_all(self) -> str:
> + output: str = self.session.before
> + output.replace("[PEXPECT]", "")
> + return output
> +
This function is missing the isalive() check in the previous function
above.
Also, could you rewrite "get_output_before" to use "get_output_all" to
avoid having checks for [PEXPECT] in multiple places - and also checks for
isalive().
> + def close(self, force: bool = False) -> None:
> + if force is True:
> + self.session.close()
> + else:
> + if self.isalive():
> + self.session.logout()
> +
> + def isalive(self) -> bool:
> + return self.session.isalive()
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> new file mode 100644
> index 0000000000..db87349827
> --- /dev/null
> +++ b/dts/framework/utils.py
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +def RED(text: str) -> str:
> + return f"\u001B[31;1m{str(text)}\u001B[0m"
> +
> +
> +def GREEN(text: str) -> str:
> + return f"\u001B[32;1m{str(text)}\u001B[0m"
> --
> 2.30.2
>
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, September 8, 2022 11:53 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
>
> On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > The library uses the pexpect python library and implements connection
> > to a node and two ways to interact with the node:
> > 1. Send a string with specified prompt which will be matched after
> > the string has been sent to the node.
> > 2. Send a command to be executed. No prompt is specified here.
> >
> > Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>
> Comments inline below.
> Thanks,
> /Bruce
>
> > ---
> > dts/framework/exception.py | 57 ++++++++++
> > dts/framework/ssh_pexpect.py | 205
> +++++++++++++++++++++++++++++++++++
> > dts/framework/utils.py | 12 ++
> > 3 files changed, 274 insertions(+)
> > create mode 100644 dts/framework/exception.py create mode 100644
> > dts/framework/ssh_pexpect.py create mode 100644
> > dts/framework/utils.py
> >
> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> > new file mode 100644 index 0000000000..35e81a4d99
> > --- /dev/null
> > +++ b/dts/framework/exception.py
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > +# Copyright(c) 2022 University of New Hampshire #
> > +
> > +"""
> > +User-defined exceptions used across the framework.
> > +"""
> > +
> > +
> > +class TimeoutException(Exception):
> > + """
> > + Command execution timeout.
> > + """
> > +
> > + command: str
> > + output: str
> > +
> > + def __init__(self, command: str, output: str):
> > + self.command = command
> > + self.output = output
> > +
> > + def __str__(self) -> str:
> > + return f"TIMEOUT on {self.command}"
> > +
> > + def get_output(self) -> str:
> > + return self.output
> > +
> > +
> > +class SSHConnectionException(Exception):
> > + """
> > + SSH connection error.
> > + """
> > +
> > + host: str
> > +
> > + def __init__(self, host: str):
> > + self.host = host
> > +
> > + def __str__(self) -> str:
> > + return f"Error trying to connect with {self.host}"
> > +
> > +
> > +class SSHSessionDeadException(Exception):
> > + """
> > + SSH session is not alive.
> > + It can no longer be used.
> > + """
> > +
> > + host: str
> > +
> > + def __init__(self, host: str):
> > + self.host = host
> > +
> > + def __str__(self) -> str:
> > + return f"SSH session with {self.host} has died"
> > diff --git a/dts/framework/ssh_pexpect.py
> > b/dts/framework/ssh_pexpect.py new file mode 100644 index
> > 0000000000..e8f64515c0
> > --- /dev/null
> > +++ b/dts/framework/ssh_pexpect.py
> > @@ -0,0 +1,205 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > +# Copyright(c) 2022 University of New Hampshire #
> > +
> > +import time
> > +from typing import Optional
> > +
> > +from pexpect import pxssh
> > +
> > +from .exception import SSHConnectionException,
> > +SSHSessionDeadException, TimeoutException from .logger import DTSLOG
> > +from .utils import GREEN, RED
> > +
> > +"""
> > +The module handles ssh sessions to TG and SUT.
> > +It implements the send_expect function to send commands and get output
> data.
> > +"""
> > +
> > +
> > +class SSHPexpect:
> > + username: str
> > + password: str
> > + node: str
> > + logger: DTSLOG
> > + magic_prompt: str
> > +
> > + def __init__(
> > + self,
> > + node: str,
> > + username: str,
> > + password: Optional[str],
> > + logger: DTSLOG,
> > + ):
> > + self.magic_prompt = "MAGIC PROMPT"
> > + self.logger = logger
> > +
> > + self.node = node
> > + self.username = username
> > + self.password = password or ""
> > + self.logger.info(f"ssh {self.username}@{self.node}")
> > +
> > + self._connect_host()
> > +
> > + def _connect_host(self) -> None:
> > + """
> > + Create connection to assigned node.
> > + """
> > + retry_times = 10
> > + try:
> > + if ":" in self.node:
>
> Should this check and the relevant splitting below to assign to self.ip and
> self.port, not be don at init when the node is passed in? Certainly the splitting
> should probably be done outside the loop, rather than re-doing the split into ip
> and port 10 times?
>
It definitely should :-).
I'll move it to init.
> > + while retry_times:
> > + self.ip = self.node.split(":")[0]
> > + self.port = int(self.node.split(":")[1])
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + try:
> > + self.session.login(
> > + self.ip,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > + port=self.port,
> > + login_timeout=20,
> > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> key)|(?i)(password for .+:)",
> > + )
> > + except Exception as e:
> > + print(e)
> > + time.sleep(2)
> > + retry_times -= 1
> > + print("retry %d times connecting..." % (10 - retry_times))
> > + else:
> > + break
> > + else:
> > + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> > + else:
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + self.session.login(
> > + self.node,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> key)|(?i)(password for .+:)",
> > + )
> > + self.logger.info(f"Connection to {self.node} succeeded")
> > + self.send_expect("stty -echo", "#")
> > + self.send_expect("stty columns 1000", "#")
> > + except Exception as e:
> > + print(RED(str(e)))
> > + if getattr(self, "port", None):
> > + suggestion = (
> > + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> > + + "is stopped\n"
> > + )
>
> I'd suggest using f-strings here to avoid splitting error messages across lines.
> They can also be used for strings above too to increase readability.
>
> We should probably look to standardize all strings used in DTS to a single format
> - either f-strings or the style given here, rather than using a mix.
>
This is one of the many things we left from the original code to facilitate discussion - would this be a requirement or can we skip it (possibly changing it later)? I prefer f-strings everywhere and I'll change it where I can, at least in this patch.
Maybe we could do this one patch the best we can to showcase what the code should ideally look like and possibly loosen requirements in subsequent patches? This will leave technical debt so it doesn't sound good.
> > + print(GREEN(suggestion))
> > +
> > + raise SSHConnectionException(self.node)
> > +
> > + def send_expect_base(self, command: str, expected: str, timeout: float) ->
> str:
> > + self.clean_session()
> > + self.session.PROMPT = expected
> > + self.__sendline(command)
> > + self.__prompt(command, timeout)
> > +
> > + before = self.get_output_before()
> > + return before
> > +
> > + def send_expect(
> > + self, command: str, expected: str, timeout: float = 15, verify: bool = False
> > + ) -> str | int:
> > +
> > + try:
> > + ret = self.send_expect_base(command, expected, timeout)
> > + if verify:
> > + ret_status = self.send_expect_base("echo $?", expected, timeout)
> > + if not int(ret_status):
> > + return ret
> > + else:
> > + self.logger.error("Command: %s failure!" % command)
> > + self.logger.error(ret)
> > + return int(ret_status)
> > + else:
> > + return ret
> > + except Exception as e:
> > + print(
> > + RED(
> > + "Exception happened in [%s] and output is [%s]"
> > + % (command, self.get_output_before())
> > + )
> > + )
> > + raise e
> > +
> > + def send_command(self, command: str, timeout: float = 1) -> str:
> > + try:
> > + self.clean_session()
> > + self.__sendline(command)
> > + except Exception as e:
> > + raise e
> > +
> > + output = self.get_session_before(timeout=timeout)
> > + self.session.PROMPT = self.session.UNIQUE_PROMPT
> > + self.session.prompt(0.1)
> > +
> > + return output
> > +
> > + def clean_session(self) -> None:
> > + self.get_session_before(timeout=0.01)
> > +
> > + def get_session_before(self, timeout: float = 15) -> str:
> > + """
> > + Get all output before timeout
> > + """
> > + self.session.PROMPT = self.magic_prompt
> > + try:
> > + self.session.prompt(timeout)
> > + except Exception as e:
> > + pass
> > +
> > + before = self.get_output_all()
> > + self.__flush()
> > +
> > + return before
> > +
> > + def __flush(self) -> None:
> > + """
> > + Clear all session buffer
> > + """
> > + self.session.buffer = ""
> > + self.session.before = ""
> > +
> > + def __prompt(self, command: str, timeout: float) -> None:
> > + if not self.session.prompt(timeout):
> > + raise TimeoutException(command, self.get_output_all())
> > + from None
> > +
> > + def __sendline(self, command: str) -> None:
> > + if not self.isalive():
> > + raise SSHSessionDeadException(self.node)
> > + if len(command) == 2 and command.startswith("^"):
> > + self.session.sendcontrol(command[1])
> > + else:
> > + self.session.sendline(command)
> > +
> > + def get_output_before(self) -> str:
> > + if not self.isalive():
> > + raise SSHSessionDeadException(self.node)
> > + before: list[str] = self.session.before.rsplit("\r\n", 1)
>
> The cast in the middle of pyton code seems strange. Does rsplit not always
> return a list value? In quick testing here even doing "".rsplit("x",1) returns a list
> with the empty string.
>
It doesn't need to be there, I'll remove it.
> > + if before[0] == "[PEXPECT]":
> > + before[0] = ""
> > +
> > + return before[0]
>
> Might be slightly more readable as:
> retval = self.session.before.rsplit("\r\n", 1)[0]
> if retval == "[PEXPECT]"
> return ""
> return retval
>
Ack.
> > +
> > + def get_output_all(self) -> str:
> > + output: str = self.session.before
> > + output.replace("[PEXPECT]", "")
> > + return output
> > +
>
> This function is missing the isalive() check in the previous function above.
> Also, could you rewrite "get_output_before" to use "get_output_all" to avoid
> having checks for [PEXPECT] in multiple places - and also checks for isalive().
>
Yea, the code has these sorts of antipatterns everywhere. I'll refactor it a bit.
> > + def close(self, force: bool = False) -> None:
> > + if force is True:
> > + self.session.close()
> > + else:
> > + if self.isalive():
> > + self.session.logout()
> > +
> > + def isalive(self) -> bool:
> > + return self.session.isalive()
> > diff --git a/dts/framework/utils.py b/dts/framework/utils.py new file
> > mode 100644 index 0000000000..db87349827
> > --- /dev/null
> > +++ b/dts/framework/utils.py
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > +# Copyright(c) 2022 University of New Hampshire #
> > +
> > +def RED(text: str) -> str:
> > + return f"\u001B[31;1m{str(text)}\u001B[0m"
> > +
> > +
> > +def GREEN(text: str) -> str:
> > + return f"\u001B[32;1m{str(text)}\u001B[0m"
> > --
> > 2.30.2
> >
On Tue, Sep 13, 2022 at 01:36:42PM +0000, Juraj Linkeš wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, September 8, 2022 11:53 AM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> > Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
> >
> > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
<snip>
> > > + self.send_expect("stty -echo", "#")
> > > + self.send_expect("stty columns 1000", "#")
> > > + except Exception as e:
> > > + print(RED(str(e)))
> > > + if getattr(self, "port", None):
> > > + suggestion = (
> > > + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> > > + + "is stopped\n"
> > > + )
> >
> > I'd suggest using f-strings here to avoid splitting error messages across lines.
> > They can also be used for strings above too to increase readability.
> >
> > We should probably look to standardize all strings used in DTS to a single format
> > - either f-strings or the style given here, rather than using a mix.
> >
>
> This is one of the many things we left from the original code to facilitate discussion - would this be a requirement or can we skip it (possibly changing it later)? I prefer f-strings everywhere and I'll change it where I can, at least in this patch.
> Maybe we could do this one patch the best we can to showcase what the code should ideally look like and possibly loosen requirements in subsequent patches? This will leave technical debt so it doesn't sound good.
>
Yes, I can understand that a huge amount of tech-debt has built up in the
code, and it's probably a fairly huge undertaking to remove it all.
On the other hand, this move to the main repo seems the best opportunity we
are likely to get to try and clean this up and standardise it. Therefore,
I'd really like to see us use f-strings everywhere. Is there a
style-checker that can be used automatically to flag older-style strings?
On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
<snip>
> + self.session = pxssh.pxssh(encoding="utf-8")
> + self.session.login(
> + self.node,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + self.logger.info(f"Connection to {self.node} succeeded")
> + self.send_expect("stty -echo", "#")
> + self.send_expect("stty columns 1000", "#")
First of all, thanks for those changes! Having DTS inside DPDK makes
test synchronization a lot easier. I'm happy to say (unsurprisingly)
that it works with my RISC-V HiFive Unmatched board like a charm.
Though there is a small issue with the lines above. They assume "#" as
the prompt sign, even though original_prompt was set to "[$#>]". This
touches on two problems:
1. # is usually a root prompt - is DTS assumed to be run with root
privileges? DPDK may (in theory) run without them with some permission
adjustment (hugetlb, VFIO container, etc.). If we assume DTS needs
root access, this has to be both documented and validated before
running the whole suite. Otherwise it'll be hard to debug.
2. Different shells use different prompts on different distros. Hence
perhaps there should be a regex here (same as with original_prompt)
and there could be a conf.yaml option to modify it on a per-host
basis?
>
> On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> <snip>
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + self.session.login(
> > + self.node,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > + password_regex=r"(?i)(?:password:)|(?:passphrase
> for key)|(?i)(password for .+:)",
> > + )
> > + self.logger.info(f"Connection to {self.node}
> succeeded")
> > + self.send_expect("stty -echo", "#")
> > + self.send_expect("stty columns 1000", "#")
> First of all, thanks for those changes! Having DTS inside DPDK makes
> test synchronization a lot easier. I'm happy to say (unsurprisingly)
> that it works with my RISC-V HiFive Unmatched board like a charm.
> Though there is a small issue with the lines above. They assume "#" as
> the prompt sign, even though original_prompt was set to "[$#>]". This
> touches on two problems:
> 1. # is usually a root prompt - is DTS assumed to be run with root
> privileges? DPDK may (in theory) run without them with some permission
> adjustment (hugetlb, VFIO container, etc.). If we assume DTS needs
> root access, this has to be both documented and validated before
> running the whole suite. Otherwise it'll be hard to debug.
>
Around a year ago there were some attempts to get DTS to not require root.
This ended up running into issues because DTS sets up drivers for you,
which requires root as far as I know, as well as setting up hugepages,
which I think also requires root. The current version of DTS can probably
run without root, but it will probably stop working as soon as DTS starts
interacting with PCI devices. Elevating privileges using pkexec or sudo is
less portable and would require supporting a lot more forms of
authentication (kerberos/ldap for enterprise deployments, passwords, 2fa,
etc). It is much easier to say that the default SSH agent must provide root
access to the SUT and Traffic Generator either with a password or
pre-configured passwordless authentication (ssh keys, kerberos, etc).
I agree it should be documented. I honestly didn't consider that anyone
would try running DTS as a non-root user.
> 2. Different shells use different prompts on different distros. Hence
> perhaps there should be a regex here (same as with original_prompt)
> and there could be a conf.yaml option to modify it on a per-host
> basis?
As far as customizing the prompts, I think that is doable via a
configuration option.
As far as different shells, I don't think we were planning to support
anything besides either bash or posix-compatible shells. At the moment all
of the community lab systems use bash, and for ease of test development it
will be easier to mandate that everyone uses one shell. Otherwise DTS CI
will need to run once for each shell to catch issues, which in my opinion
are resources better spent on more in-depth testing of DTS and DPDK.
<snip>
On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
<snip>
> + self.session = pxssh.pxssh(encoding="utf-8")
> + self.session.login(
> + self.node,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + self.logger.info<http://self.logger.info/>(f"Connection to {self.node} succeeded")
> + self.send_expect("stty -echo", "#")
> + self.send_expect("stty columns 1000", "#")
First of all, thanks for those changes! Having DTS inside DPDK makes
test synchronization a lot easier. I'm happy to say (unsurprisingly)
that it works with my RISC-V HiFive Unmatched board like a charm.
Though there is a small issue with the lines above. They assume "#" as
the prompt sign, even though original_prompt was set to "[$#>]". This
touches on two problems:
1. # is usually a root prompt - is DTS assumed to be run with root
privileges? DPDK may (in theory) run without them with some permission
adjustment (hugetlb, VFIO container, etc.). If we assume DTS needs
root access, this has to be both documented and validated before
running the whole suite. Otherwise it'll be hard to debug.
Around a year ago there were some attempts to get DTS to not require root. This ended up running into issues because DTS sets up drivers for you, which requires root as far as I know, as well as setting up hugepages, which I think also requires root. The current version of DTS can probably run without root, but it will probably stop working as soon as DTS starts interacting with PCI devices. Elevating privileges using pkexec or sudo is less portable and would require supporting a lot more forms of authentication (kerberos/ldap for enterprise deployments, passwords, 2fa, etc). It is much easier to say that the default SSH agent must provide root access to the SUT and Traffic Generator either with a password or pre-configured passwordless authentication (ssh keys, kerberos, etc).
[Honnappa] One of the feedback we collected asks to deprecate the use of clear text passwords in config files and root user. It suggests to use keys and sudo. It is a ‘Must Have’ item.
I agree it should be documented. I honestly didn't consider that anyone would try running DTS as a non-root user.
[Honnappa] +1 for supporting root users for now and documenting.
2. Different shells use different prompts on different distros. Hence
perhaps there should be a regex here (same as with original_prompt)
and there could be a conf.yaml option to modify it on a per-host
basis?
As far as customizing the prompts, I think that is doable via a configuration option.
As far as different shells, I don't think we were planning to support anything besides either bash or posix-compatible shells. At the moment all of the community lab systems use bash, and for ease of test development it will be easier to mandate that everyone uses one shell. Otherwise DTS CI will need to run once for each shell to catch issues, which in my opinion are resources better spent on more in-depth testing of DTS and DPDK.
[Honnappa] +1 for using just bash, we can document this as well.
On Wed, Sep 14, 2022 at 12:03:34AM +0000, Honnappa Nagarahalli wrote:
> <snip>
>
>
> On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> <snip>
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + self.session.login(
> > + self.node,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > +
> password_regex=r"(?i)(?:password:)|(?:passphrase for
> key)|(?i)(password for .+:)",
> > + )
> > + [1]self.logger.info(f"Connection to {self.node}
> succeeded")
> > + self.send_expect("stty -echo", "#")
> > + self.send_expect("stty columns 1000", "#")
> First of all, thanks for those changes! Having DTS inside DPDK makes
> test synchronization a lot easier. I'm happy to say (unsurprisingly)
> that it works with my RISC-V HiFive Unmatched board like a charm.
>
>
> Though there is a small issue with the lines above. They assume "#"
> as
> the prompt sign, even though original_prompt was set to "[$#>]".
> This
> touches on two problems:
> 1. # is usually a root prompt - is DTS assumed to be run with root
> privileges? DPDK may (in theory) run without them with some
> permission
> adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> needs
> root access, this has to be both documented and validated before
> running the whole suite. Otherwise it'll be hard to debug.
>
>
> Around a year ago there were some attempts to get DTS to not require
> root. This ended up running into issues because DTS sets up drivers for
> you, which requires root as far as I know, as well as setting up
> hugepages, which I think also requires root. The current version of DTS
> can probably run without root, but it will probably stop working as
> soon as DTS starts interacting with PCI devices. Elevating privileges
> using pkexec or sudo is less portable and would require supporting a
> lot more forms of authentication (kerberos/ldap for enterprise
> deployments, passwords, 2fa, etc). It is much easier to say that the
> default SSH agent must provide root access to the SUT and Traffic
> Generator either with a password or pre-configured passwordless
> authentication (ssh keys, kerberos, etc).
>
> [Honnappa] One of the feedback we collected asks to deprecate the use
> of clear text passwords in config files and root user. It suggests to
> use keys and sudo. It is a ‘Must Have’ item.
>
>
> I agree it should be documented. I honestly didn't consider that anyone
> would try running DTS as a non-root user.
>
> [Honnappa] +1 for supporting root users for now and documenting.
>
>
> 2. Different shells use different prompts on different distros.
> Hence
> perhaps there should be a regex here (same as with
> original_prompt)
> and there could be a conf.yaml option to modify it on a per-host
> basis?
>
>
> As far as customizing the prompts, I think that is doable via a
> configuration option.
> As far as different shells, I don't think we were planning to support
> anything besides either bash or posix-compatible shells. At the moment
> all of the community lab systems use bash, and for ease of test
> development it will be easier to mandate that everyone uses one shell.
> Otherwise DTS CI will need to run once for each shell to catch issues,
> which in my opinion are resources better spent on more in-depth testing
> of DTS and DPDK.
>
> [Honnappa] +1 for using just bash, we can document this as well.
>
I would agree overall. Just supporting one shell is fine - certainly for
now. Also completely agree that we need to remove hard-coded passwords and
ideally non-root. However, I think for the initial versions the main thing
should be removing the passwords so I would be ok for keeping the "root"
login requirement, so long as we support using ssh keys for login rather
than hard-coded passwords.
/Bruce
On Wed, Sep 14, 2022 at 08:42:07AM +0100, Bruce Richardson wrote:
<snip>
> >
> > As far as customizing the prompts, I think that is doable via a
> > configuration option.
> > As far as different shells, I don't think we were planning to support
> > anything besides either bash or posix-compatible shells. At the moment
> > all of the community lab systems use bash, and for ease of test
> > development it will be easier to mandate that everyone uses one shell.
> > Otherwise DTS CI will need to run once for each shell to catch issues,
> > which in my opinion are resources better spent on more in-depth testing
> > of DTS and DPDK.
> >
> > [Honnappa] +1 for using just bash, we can document this as well.
> >
>
> I would agree overall. Just supporting one shell is fine - certainly for
> now. Also completely agree that we need to remove hard-coded passwords and
> ideally non-root. However, I think for the initial versions the main thing
> should be removing the passwords so I would be ok for keeping the "root"
> login requirement, so long as we support using ssh keys for login rather
> than hard-coded passwords.
>
> /Bruce
The only reason I have mentioned different shells is to illustrate that
prompt might be changed not only due to user modifying PS1 env variable.
I agree with the rest (root-only, bash-only).
On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> The library uses the pexpect python library and implements connection to
> a node and two ways to interact with the node:
> 1. Send a string with specified prompt which will be matched after
> the string has been sent to the node.
> 2. Send a command to be executed. No prompt is specified here.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> dts/framework/exception.py | 57 ++++++++++
> dts/framework/ssh_pexpect.py | 205 +++++++++++++++++++++++++++++++++++
> dts/framework/utils.py | 12 ++
> 3 files changed, 274 insertions(+)
> create mode 100644 dts/framework/exception.py
> create mode 100644 dts/framework/ssh_pexpect.py
> create mode 100644 dts/framework/utils.py
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> new file mode 100644
> index 0000000000..35e81a4d99
> --- /dev/null
> +++ b/dts/framework/exception.py
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +"""
> +User-defined exceptions used across the framework.
> +"""
> +
> +
> +class TimeoutException(Exception):
> + """
> + Command execution timeout.
> + """
> +
> + command: str
> + output: str
> +
> + def __init__(self, command: str, output: str):
> + self.command = command
> + self.output = output
> +
> + def __str__(self) -> str:
> + return f"TIMEOUT on {self.command}"
> +
> + def get_output(self) -> str:
> + return self.output
> +
> +
> +class SSHConnectionException(Exception):
> + """
> + SSH connection error.
> + """
> +
> + host: str
> +
> + def __init__(self, host: str):
> + self.host = host
> +
> + def __str__(self) -> str:
> + return f"Error trying to connect with {self.host}"
> +
> +
> +class SSHSessionDeadException(Exception):
> + """
> + SSH session is not alive.
> + It can no longer be used.
> + """
> +
> + host: str
> +
> + def __init__(self, host: str):
> + self.host = host
> +
> + def __str__(self) -> str:
> + return f"SSH session with {self.host} has died"
> diff --git a/dts/framework/ssh_pexpect.py b/dts/framework/ssh_pexpect.py
> new file mode 100644
> index 0000000000..e8f64515c0
> --- /dev/null
> +++ b/dts/framework/ssh_pexpect.py
> @@ -0,0 +1,205 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +import time
> +from typing import Optional
> +
> +from pexpect import pxssh
> +
> +from .exception import SSHConnectionException, SSHSessionDeadException, TimeoutException
> +from .logger import DTSLOG
> +from .utils import GREEN, RED
> +
> +"""
> +The module handles ssh sessions to TG and SUT.
> +It implements the send_expect function to send commands and get output data.
> +"""
> +
> +
> +class SSHPexpect:
> + username: str
> + password: str
> + node: str
> + logger: DTSLOG
> + magic_prompt: str
> +
> + def __init__(
> + self,
> + node: str,
> + username: str,
> + password: Optional[str],
> + logger: DTSLOG,
> + ):
> + self.magic_prompt = "MAGIC PROMPT"
Why is this necessary? pxssh is already setting target prompt to
pxssh.UNIQUE_PROMPT in the session constructor, to be specific:
self.UNIQUE_PROMPT = r"\[PEXPECT\][\$\#] "
self.PROMPT = self.UNIQUE_PROMPT
Also session.login() will change target prompt to that, exactly for the
reason of achieving a unique prompt that can be easily matched by pxssh.
So if "MAGIC PROMPT is the prompt that you'd like to have on the remote
host, then the following should be run after opening the session:
self.session.PROMPT = self.magic_prompt
if not self.session.set_unique_prompt():
do_some_error_handling()
Otherwise it's unnecessary.
> + self.logger = logger
> +
> + self.node = node
> + self.username = username
> + self.password = password or ""
> + self.logger.info(f"ssh {self.username}@{self.node}")
> +
> + self._connect_host()
> +
> + def _connect_host(self) -> None:
> + """
> + Create connection to assigned node.
> + """
> + retry_times = 10
> + try:
> + if ":" in self.node:
> + while retry_times:
> + self.ip = self.node.split(":")[0]
> + self.port = int(self.node.split(":")[1])
> + self.session = pxssh.pxssh(encoding="utf-8")
> + try:
> + self.session.login(
> + self.ip,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + port=self.port,
> + login_timeout=20,
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + except Exception as e:
> + print(e)
> + time.sleep(2)
> + retry_times -= 1
> + print("retry %d times connecting..." % (10 - retry_times))
> + else:
> + break
> + else:
> + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> + else:
> + self.session = pxssh.pxssh(encoding="utf-8")
> + self.session.login(
> + self.node,
> + self.username,
> + self.password,
> + original_prompt="[$#>]",
> + password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
> + )
> + self.logger.info(f"Connection to {self.node} succeeded")
> + self.send_expect("stty -echo", "#")
> + self.send_expect("stty columns 1000", "#")
This works only by chance and makes hacks in get_output_before()
necessary. After some testing it seems that pxssh is matching AND
chomping the session.PROMPT when session.prompt() is called. Given the
UNIQUE_PROMPT, the root user prompt will be "[PEXPECT]#" so this
send_expect() will chomp # and leave "[PEXPECT]" as part of the output.
Given that the two above lines do not require any special output I think
self.send_command() should be used here.
> + except Exception as e:
> + print(RED(str(e)))
> + if getattr(self, "port", None):
> + suggestion = (
> + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> + + "is stopped\n"
> + )
> + print(GREEN(suggestion))
> +
> + raise SSHConnectionException(self.node)
> +
> + def send_expect_base(self, command: str, expected: str, timeout: float) -> str:
> + self.clean_session()
> + self.session.PROMPT = expected
> + self.__sendline(command)
> + self.__prompt(command, timeout)
> +
> + before = self.get_output_before()
Prompt should be reverted to whatever it was before leaving this
function.
> + return before
> +
> + def send_expect(
> + self, command: str, expected: str, timeout: float = 15, verify: bool = False
> + ) -> str | int:
> +
> + try:
> + ret = self.send_expect_base(command, expected, timeout)
> + if verify:
> + ret_status = self.send_expect_base("echo $?", expected, timeout)
"echo $?" will only print the return code. How is it supposed to match
"expected"? If "expected" is a return code then the first command's
output probably won't match.
I think send_command() should be used here.
> + if not int(ret_status):
> + return ret
The condition above seems like a C-ism used in python which again works
by mistake. Return code 0 will convert to integer 0 which will be
promoted to a boolean False. It would be more readable to change this
block to:
ri = int(ret_status)
if ri != 0:
# error prints
return ri
> + else:
> + self.logger.error("Command: %s failure!" % command)
> + self.logger.error(ret)
> + return int(ret_status)
> + else:
> + return ret
> + except Exception as e:
> + print(
> + RED(
> + "Exception happened in [%s] and output is [%s]"
> + % (command, self.get_output_before())
> + )
> + )
> + raise e
> +
> + def send_command(self, command: str, timeout: float = 1) -> str:
> + try:
> + self.clean_session()
> + self.__sendline(command)
> + except Exception as e:
> + raise e
> +
> + output = self.get_session_before(timeout=timeout)
> + self.session.PROMPT = self.session.UNIQUE_PROMPT
> + self.session.prompt(0.1)
This is wrong:
1. self.get_session_before() will return output of the command but since
it changed the expected (not real!) prompt to self.magic_prompt, that
won't be matched so the output will contain the prompt set by pxssh
(UNIQUE_PROMPT).
2. Then prompt is reset to UNIQUE_PROMPT but and prompt() is called but
that will only clean up the pxssh buffer. If get_session_before() was
not changing the session.PROMPT from UNIQUE_PROMPT to magic_prompt,
the second prompt() call would be unnecessary.
> +
> + return output
> +
> + def clean_session(self) -> None:
> + self.get_session_before(timeout=0.01)
What if remote host is slow for any reason? We'll timeout here. It seems
that such a small timeout value was used because clean_session() is
used in every send_command() call.
Come to think of it, why is this call necessary when we have
self.__flush()?
> +
> + def get_session_before(self, timeout: float = 15) -> str:
> + """
> + Get all output before timeout
> + """
> + self.session.PROMPT = self.magic_prompt
This line has no effect. Remote prompt was never set to
self.magic_prompt.
> + try:
> + self.session.prompt(timeout)
> + except Exception as e:
> + pass
> +
> + before = self.get_output_all()
> + self.__flush()
> +
> + return before
> +
> + def __flush(self) -> None:
> + """
> + Clear all session buffer
> + """
> + self.session.buffer = ""
> + self.session.before = ""
> +
> + def __prompt(self, command: str, timeout: float) -> None:
> + if not self.session.prompt(timeout):
> + raise TimeoutException(command, self.get_output_all()) from None
> +
> + def __sendline(self, command: str) -> None:
> + if not self.isalive():
> + raise SSHSessionDeadException(self.node)
> + if len(command) == 2 and command.startswith("^"):
> + self.session.sendcontrol(command[1])
> + else:
> + self.session.sendline(command)
> +
> + def get_output_before(self) -> str:
The name is missleading. In pxssh terms "before" means all the lines
before the matched expect()/prompt(). Here it returns the last line of
the output. Perhaps get_last_output_line() is better?
> + if not self.isalive():
> + raise SSHSessionDeadException(self.node)
> + before: list[str] = self.session.before.rsplit("\r\n", 1)
> + if before[0] == "[PEXPECT]":
> + before[0] = ""
Unnecessary if prompt was handled in proper way as mentioned above.
> +
> + return before[0]
> +
> + def get_output_all(self) -> str:
> + output: str = self.session.before
> + output.replace("[PEXPECT]", "")
Ditto. If session.PROMPT was restored properly, this function would not
be necessary at all.
> + return output
> +
> + def close(self, force: bool = False) -> None:
> + if force is True:
> + self.session.close()
> + else:
> + if self.isalive():
> + self.session.logout()
> +
> + def isalive(self) -> bool:
> + return self.session.isalive()
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> new file mode 100644
> index 0000000000..db87349827
> --- /dev/null
> +++ b/dts/framework/utils.py
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2010-2014 Intel Corporation
> +# Copyright(c) 2022 PANTHEON.tech s.r.o.
> +# Copyright(c) 2022 University of New Hampshire
> +#
> +
> +def RED(text: str) -> str:
> + return f"\u001B[31;1m{str(text)}\u001B[0m"
> +
> +
> +def GREEN(text: str) -> str:
> + return f"\u001B[32;1m{str(text)}\u001B[0m"
> --
> 2.30.2
>
<snip>
> >
> > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > <snip>
> > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > + self.session.login(
> > > + self.node,
> > > + self.username,
> > > + self.password,
> > > + original_prompt="[$#>]",
> > > +
> > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > key)|(?i)(password for .+:)",
> > > + )
> > > + [1]self.logger.info(f"Connection to {self.node}
> > succeeded")
> > > + self.send_expect("stty -echo", "#")
> > > + self.send_expect("stty columns 1000", "#")
> > First of all, thanks for those changes! Having DTS inside DPDK makes
> > test synchronization a lot easier. I'm happy to say (unsurprisingly)
> > that it works with my RISC-V HiFive Unmatched board like a charm.
> >
> >
> > Though there is a small issue with the lines above. They assume "#"
> > as
> > the prompt sign, even though original_prompt was set to "[$#>]".
> > This
> > touches on two problems:
> > 1. # is usually a root prompt - is DTS assumed to be run with root
> > privileges? DPDK may (in theory) run without them with some
> > permission
> > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > needs
> > root access, this has to be both documented and validated before
> > running the whole suite. Otherwise it'll be hard to debug.
> >
> >
> > Around a year ago there were some attempts to get DTS to not require
> > root. This ended up running into issues because DTS sets up drivers for
> > you, which requires root as far as I know, as well as setting up
> > hugepages, which I think also requires root. The current version of DTS
> > can probably run without root, but it will probably stop working as
> > soon as DTS starts interacting with PCI devices. Elevating privileges
> > using pkexec or sudo is less portable and would require supporting a
> > lot more forms of authentication (kerberos/ldap for enterprise
> > deployments, passwords, 2fa, etc). It is much easier to say that the
> > default SSH agent must provide root access to the SUT and Traffic
> > Generator either with a password or pre-configured passwordless
> > authentication (ssh keys, kerberos, etc).
> >
> > [Honnappa] One of the feedback we collected asks to deprecate the use
> > of clear text passwords in config files and root user. It suggests to
> > use keys and sudo. It is a ‘Must Have’ item.
> >
> >
> > I agree it should be documented. I honestly didn't consider that anyone
> > would try running DTS as a non-root user.
> >
> > [Honnappa] +1 for supporting root users for now and documenting.
> >
> >
> > 2. Different shells use different prompts on different distros.
> > Hence
> > perhaps there should be a regex here (same as with
> > original_prompt)
> > and there could be a conf.yaml option to modify it on a per-host
> > basis?
> >
> >
> > As far as customizing the prompts, I think that is doable via a
> > configuration option.
> > As far as different shells, I don't think we were planning to support
> > anything besides either bash or posix-compatible shells. At the moment
> > all of the community lab systems use bash, and for ease of test
> > development it will be easier to mandate that everyone uses one shell.
> > Otherwise DTS CI will need to run once for each shell to catch issues,
> > which in my opinion are resources better spent on more in-depth testing
> > of DTS and DPDK.
> >
> > [Honnappa] +1 for using just bash, we can document this as well.
> >
>
> I would agree overall. Just supporting one shell is fine - certainly for now. Also
> completely agree that we need to remove hard-coded passwords and ideally
> non-root. However, I think for the initial versions the main thing should be
> removing the passwords so I would be ok for keeping the "root"
> login requirement, so long as we support using ssh keys for login rather than
> hard-coded passwords.
I would be for dropping support for the hard-coded passwords completely. Setting up the password-less SSH is straightforward (not sure if you meant the same).
>
> /Bruce
On Wed, Sep 14, 2022 at 3:57 PM Honnappa Nagarahalli <
Honnappa.Nagarahalli@arm.com> wrote:
> <snip>
>
> > >
> > > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > > <snip>
> > > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > > + self.session.login(
> > > > + self.node,
> > > > + self.username,
> > > > + self.password,
> > > > + original_prompt="[$#>]",
> > > > +
> > > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > > key)|(?i)(password for .+:)",
> > > > + )
> > > > + [1]self.logger.info(f"Connection to
> {self.node}
> > > succeeded")
> > > > + self.send_expect("stty -echo", "#")
> > > > + self.send_expect("stty columns 1000", "#")
> > > First of all, thanks for those changes! Having DTS inside DPDK
> makes
> > > test synchronization a lot easier. I'm happy to say
> (unsurprisingly)
> > > that it works with my RISC-V HiFive Unmatched board like a charm.
> > >
> > >
> > > Though there is a small issue with the lines above. They assume
> "#"
> > > as
> > > the prompt sign, even though original_prompt was set to "[$#>]".
> > > This
> > > touches on two problems:
> > > 1. # is usually a root prompt - is DTS assumed to be run with root
> > > privileges? DPDK may (in theory) run without them with some
> > > permission
> > > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > > needs
> > > root access, this has to be both documented and validated
> before
> > > running the whole suite. Otherwise it'll be hard to debug.
> > >
> > >
> > > Around a year ago there were some attempts to get DTS to not require
> > > root. This ended up running into issues because DTS sets up drivers
> for
> > > you, which requires root as far as I know, as well as setting up
> > > hugepages, which I think also requires root. The current version of
> DTS
> > > can probably run without root, but it will probably stop working as
> > > soon as DTS starts interacting with PCI devices. Elevating
> privileges
> > > using pkexec or sudo is less portable and would require supporting a
> > > lot more forms of authentication (kerberos/ldap for enterprise
> > > deployments, passwords, 2fa, etc). It is much easier to say that the
> > > default SSH agent must provide root access to the SUT and Traffic
> > > Generator either with a password or pre-configured passwordless
> > > authentication (ssh keys, kerberos, etc).
> > >
> > > [Honnappa] One of the feedback we collected asks to deprecate the
> use
> > > of clear text passwords in config files and root user. It suggests
> to
> > > use keys and sudo. It is a ‘Must Have’ item.
> > >
> > >
> > > I agree it should be documented. I honestly didn't consider that
> anyone
> > > would try running DTS as a non-root user.
> > >
> > > [Honnappa] +1 for supporting root users for now and documenting.
> > >
> > >
> > > 2. Different shells use different prompts on different distros.
> > > Hence
> > > perhaps there should be a regex here (same as with
> > > original_prompt)
> > > and there could be a conf.yaml option to modify it on a
> per-host
> > > basis?
> > >
> > >
> > > As far as customizing the prompts, I think that is doable via a
> > > configuration option.
> > > As far as different shells, I don't think we were planning to
> support
> > > anything besides either bash or posix-compatible shells. At the
> moment
> > > all of the community lab systems use bash, and for ease of test
> > > development it will be easier to mandate that everyone uses one
> shell.
> > > Otherwise DTS CI will need to run once for each shell to catch
> issues,
> > > which in my opinion are resources better spent on more in-depth
> testing
> > > of DTS and DPDK.
> > >
> > > [Honnappa] +1 for using just bash, we can document this as well.
> > >
> >
> > I would agree overall. Just supporting one shell is fine - certainly for
> now. Also
> > completely agree that we need to remove hard-coded passwords and ideally
> > non-root. However, I think for the initial versions the main thing
> should be
> > removing the passwords so I would be ok for keeping the "root"
> > login requirement, so long as we support using ssh keys for login rather
> than
> > hard-coded passwords.
> I would be for dropping support for the hard-coded passwords completely.
> Setting up the password-less SSH is straightforward (not sure if you meant
> the same).
>
> >
> > /Bruce
>
I think the question is whether there are any platforms/devices that should
be tested by DTS that do not support passwordless SSH. Right now, the
community lab is using SSH keys for everything. If Intel also doesn't need
passwords, then it's up to the community whether to support them at all. It
does make it a lot easier on DTS if we can just require that the active
OpenSSH agent can log into all of the systems involved without a password.
This would also make it easier to enable AD authentication for Windows.
<snip>
> >
> > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > <snip>
> > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > + self.session.login(
> > > + self.node,
> > > + self.username,
> > > + self.password,
> > > + original_prompt="[$#>]",
> > > +
> > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > key)|(?i)(password for .+:)",
> > > + )
> > > + [1]self.logger.info<http://self.logger.info>(f"Connection to {self.node}
> > succeeded")
> > > + self.send_expect("stty -echo", "#")
> > > + self.send_expect("stty columns 1000", "#")
> > First of all, thanks for those changes! Having DTS inside DPDK makes
> > test synchronization a lot easier. I'm happy to say (unsurprisingly)
> > that it works with my RISC-V HiFive Unmatched board like a charm.
> >
> >
> > Though there is a small issue with the lines above. They assume "#"
> > as
> > the prompt sign, even though original_prompt was set to "[$#>]".
> > This
> > touches on two problems:
> > 1. # is usually a root prompt - is DTS assumed to be run with root
> > privileges? DPDK may (in theory) run without them with some
> > permission
> > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > needs
> > root access, this has to be both documented and validated before
> > running the whole suite. Otherwise it'll be hard to debug.
> >
> >
> > Around a year ago there were some attempts to get DTS to not require
> > root. This ended up running into issues because DTS sets up drivers for
> > you, which requires root as far as I know, as well as setting up
> > hugepages, which I think also requires root. The current version of DTS
> > can probably run without root, but it will probably stop working as
> > soon as DTS starts interacting with PCI devices. Elevating privileges
> > using pkexec or sudo is less portable and would require supporting a
> > lot more forms of authentication (kerberos/ldap for enterprise
> > deployments, passwords, 2fa, etc). It is much easier to say that the
> > default SSH agent must provide root access to the SUT and Traffic
> > Generator either with a password or pre-configured passwordless
> > authentication (ssh keys, kerberos, etc).
> >
> > [Honnappa] One of the feedback we collected asks to deprecate the use
> > of clear text passwords in config files and root user. It suggests to
> > use keys and sudo. It is a ‘Must Have’ item.
> >
> >
> > I agree it should be documented. I honestly didn't consider that anyone
> > would try running DTS as a non-root user.
> >
> > [Honnappa] +1 for supporting root users for now and documenting.
> >
> >
> > 2. Different shells use different prompts on different distros.
> > Hence
> > perhaps there should be a regex here (same as with
> > original_prompt)
> > and there could be a conf.yaml option to modify it on a per-host
> > basis?
> >
> >
> > As far as customizing the prompts, I think that is doable via a
> > configuration option.
> > As far as different shells, I don't think we were planning to support
> > anything besides either bash or posix-compatible shells. At the moment
> > all of the community lab systems use bash, and for ease of test
> > development it will be easier to mandate that everyone uses one shell.
> > Otherwise DTS CI will need to run once for each shell to catch issues,
> > which in my opinion are resources better spent on more in-depth testing
> > of DTS and DPDK.
> >
> > [Honnappa] +1 for using just bash, we can document this as well.
> >
>
> I would agree overall. Just supporting one shell is fine - certainly for now. Also
> completely agree that we need to remove hard-coded passwords and ideally
> non-root. However, I think for the initial versions the main thing should be
> removing the passwords so I would be ok for keeping the "root"
> login requirement, so long as we support using ssh keys for login rather than
> hard-coded passwords.
I would be for dropping support for the hard-coded passwords completely. Setting up the password-less SSH is straightforward (not sure if you meant the same).
>
> /Bruce
I think the question is whether there are any platforms/devices that should be tested by DTS that do not support passwordless SSH. Right now, the community lab is using SSH keys for everything. If Intel also doesn't need passwords, then it's up to the community whether to support them at all. It does make it a lot easier on DTS if we can just require that the active OpenSSH agent can log into all of the systems involved without a password. This would also make it easier to enable AD authentication for Windows.
[Honnappa] Jerin, Hemant do you have any concerns for your platforms regarding this?
<snip>
<snip>
> >
> > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > <snip>
> > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > + self.session.login(
> > > + self.node,
> > > + self.username,
> > > + self.password,
> > > + original_prompt="[$#>]",
> > > +
> > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > key)|(?i)(password for .+:)",
> > > + )
> > > + [1]self.logger.info<http://self.logger.info>(f"Connection to {self.node}
> > succeeded")
> > > + self.send_expect("stty -echo", "#")
> > > + self.send_expect("stty columns 1000", "#")
> > First of all, thanks for those changes! Having DTS inside DPDK makes
> > test synchronization a lot easier. I'm happy to say (unsurprisingly)
> > that it works with my RISC-V HiFive Unmatched board like a charm.
> >
> >
> > Though there is a small issue with the lines above. They assume "#"
> > as
> > the prompt sign, even though original_prompt was set to "[$#>]".
> > This
> > touches on two problems:
> > 1. # is usually a root prompt - is DTS assumed to be run with root
> > privileges? DPDK may (in theory) run without them with some
> > permission
> > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > needs
> > root access, this has to be both documented and validated before
> > running the whole suite. Otherwise it'll be hard to debug.
> >
> >
> > Around a year ago there were some attempts to get DTS to not require
> > root. This ended up running into issues because DTS sets up drivers for
> > you, which requires root as far as I know, as well as setting up
> > hugepages, which I think also requires root. The current version of DTS
> > can probably run without root, but it will probably stop working as
> > soon as DTS starts interacting with PCI devices. Elevating privileges
> > using pkexec or sudo is less portable and would require supporting a
> > lot more forms of authentication (kerberos/ldap for enterprise
> > deployments, passwords, 2fa, etc). It is much easier to say that the
> > default SSH agent must provide root access to the SUT and Traffic
> > Generator either with a password or pre-configured passwordless
> > authentication (ssh keys, kerberos, etc).
> >
> > [Honnappa] One of the feedback we collected asks to deprecate the use
> > of clear text passwords in config files and root user. It suggests to
> > use keys and sudo. It is a ‘Must Have’ item.
> >
> >
> > I agree it should be documented. I honestly didn't consider that anyone
> > would try running DTS as a non-root user.
> >
> > [Honnappa] +1 for supporting root users for now and documenting.
> >
> >
> > 2. Different shells use different prompts on different distros.
> > Hence
> > perhaps there should be a regex here (same as with
> > original_prompt)
> > and there could be a conf.yaml option to modify it on a per-host
> > basis?
> >
> >
> > As far as customizing the prompts, I think that is doable via a
> > configuration option.
> > As far as different shells, I don't think we were planning to support
> > anything besides either bash or posix-compatible shells. At the moment
> > all of the community lab systems use bash, and for ease of test
> > development it will be easier to mandate that everyone uses one shell.
> > Otherwise DTS CI will need to run once for each shell to catch issues,
> > which in my opinion are resources better spent on more in-depth testing
> > of DTS and DPDK.
> >
> > [Honnappa] +1 for using just bash, we can document this as well.
> >
>
> I would agree overall. Just supporting one shell is fine - certainly for now. Also
> completely agree that we need to remove hard-coded passwords and ideally
> non-root. However, I think for the initial versions the main thing should be
> removing the passwords so I would be ok for keeping the "root"
> login requirement, so long as we support using ssh keys for login rather than
> hard-coded passwords.
I would be for dropping support for the hard-coded passwords completely. Setting up the password-less SSH is straightforward (not sure if you meant the same).
>
> /Bruce
I think the question is whether there are any platforms/devices that should be tested by DTS that do not support passwordless SSH. Right now, the community lab is using SSH keys for everything. If Intel also doesn't need passwords, then it's up to the community whether to support them at all. It does make it a lot easier on DTS if we can just require that the active OpenSSH agent can log into all of the systems involved without a password. This would also make it easier to enable AD authentication for Windows.
[Honnappa] Jerin, Hemant do you have any concerns for your platforms regarding this?
Intel lab is using passwords, but using SSH keys is fine, it did this some years ago. As setting up SSH keys is a bit more complex than passwords, can we provide a user tool to do this quickly.
Furthermore, it is necessary to document that pre-configure SSH keys .
On Tue, Sep 20, 2022 at 11:25 PM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <snip>
> > >
> > > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > > <snip>
> > > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > > + self.session.login(
> > > > + self.node,
> > > > + self.username,
> > > > + self.password,
> > > > + original_prompt="[$#>]",
> > > > +
> > > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > > key)|(?i)(password for .+:)",
> > > > + )
> > > > + [1]self.logger.info(f"Connection to {self.node}
> > > succeeded")
> > > > + self.send_expect("stty -echo", "#")
> > > > + self.send_expect("stty columns 1000", "#")
> > > First of all, thanks for those changes! Having DTS inside DPDK makes
> > > test synchronization a lot easier. I'm happy to say (unsurprisingly)
> > > that it works with my RISC-V HiFive Unmatched board like a charm.
> > >
> > >
> > > Though there is a small issue with the lines above. They assume "#"
> > > as
> > > the prompt sign, even though original_prompt was set to "[$#>]".
> > > This
> > > touches on two problems:
> > > 1. # is usually a root prompt - is DTS assumed to be run with root
> > > privileges? DPDK may (in theory) run without them with some
> > > permission
> > > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > > needs
> > > root access, this has to be both documented and validated before
> > > running the whole suite. Otherwise it'll be hard to debug.
> > >
> > >
> > > Around a year ago there were some attempts to get DTS to not require
> > > root. This ended up running into issues because DTS sets up drivers for
> > > you, which requires root as far as I know, as well as setting up
> > > hugepages, which I think also requires root. The current version of DTS
> > > can probably run without root, but it will probably stop working as
> > > soon as DTS starts interacting with PCI devices. Elevating privileges
> > > using pkexec or sudo is less portable and would require supporting a
> > > lot more forms of authentication (kerberos/ldap for enterprise
> > > deployments, passwords, 2fa, etc). It is much easier to say that the
> > > default SSH agent must provide root access to the SUT and Traffic
> > > Generator either with a password or pre-configured passwordless
> > > authentication (ssh keys, kerberos, etc).
> > >
> > > [Honnappa] One of the feedback we collected asks to deprecate the use
> > > of clear text passwords in config files and root user. It suggests to
> > > use keys and sudo. It is a ‘Must Have’ item.
> > >
> > >
> > > I agree it should be documented. I honestly didn't consider that anyone
> > > would try running DTS as a non-root user.
> > >
> > > [Honnappa] +1 for supporting root users for now and documenting.
> > >
> > >
> > > 2. Different shells use different prompts on different distros.
> > > Hence
> > > perhaps there should be a regex here (same as with
> > > original_prompt)
> > > and there could be a conf.yaml option to modify it on a per-host
> > > basis?
> > >
> > >
> > > As far as customizing the prompts, I think that is doable via a
> > > configuration option.
> > > As far as different shells, I don't think we were planning to support
> > > anything besides either bash or posix-compatible shells. At the moment
> > > all of the community lab systems use bash, and for ease of test
> > > development it will be easier to mandate that everyone uses one shell.
> > > Otherwise DTS CI will need to run once for each shell to catch issues,
> > > which in my opinion are resources better spent on more in-depth testing
> > > of DTS and DPDK.
> > >
> > > [Honnappa] +1 for using just bash, we can document this as well.
> > >
> >
> > I would agree overall. Just supporting one shell is fine - certainly for now. Also
> > completely agree that we need to remove hard-coded passwords and ideally
> > non-root. However, I think for the initial versions the main thing should be
> > removing the passwords so I would be ok for keeping the "root"
> > login requirement, so long as we support using ssh keys for login rather than
> > hard-coded passwords.
> I would be for dropping support for the hard-coded passwords completely. Setting up the password-less SSH is straightforward (not sure if you meant the same).
>
> >
> > /Bruce
>
>
> I think the question is whether there are any platforms/devices that should be tested by DTS that do not support passwordless SSH. Right now, the community lab is using SSH keys for everything. If Intel also doesn't need passwords, then it's up to the community whether to support them at all. It does make it a lot easier on DTS if we can just require that the active OpenSSH agent can log into all of the systems involved without a password. This would also make it easier to enable AD authentication for Windows.
>
> [Honnappa] Jerin, Hemant do you have any concerns for your platforms regarding this?
No. We are using passwordless SSH in our CI testing framework.
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, September 21, 2022 7:37 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: Owen Hilyard <ohilyard@iol.unh.edu>; Bruce Richardson
> <bruce.richardson@intel.com>; Stanislaw Kardach <kda@semihalf.com>; Juraj
> Linkeš <juraj.linkes@pantheon.tech>; thomas@monjalon.net; David Marchand
> <david.marchand@redhat.com>; ronan.randles@intel.com; Tu, Lijuan
> <lijuan.tu@intel.com>; dev <dev@dpdk.org>; nd <nd@arm.com>;
> jerinj@marvell.com; hemant.agrawal@nxp.com
> Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
>
> On Tue, Sep 20, 2022 at 11:25 PM Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com> wrote:
> >
> > <snip>
> > > >
> > > > On Fri, Jul 29, 2022 at 10:55:45AM +0000, Juraj Linkeš wrote:
> > > > <snip>
> > > > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > > > + self.session.login(
> > > > > + self.node,
> > > > > + self.username,
> > > > > + self.password,
> > > > > + original_prompt="[$#>]",
> > > > > +
> > > > password_regex=r"(?i)(?:password:)|(?:passphrase for
> > > > key)|(?i)(password for .+:)",
> > > > > + )
> > > > > + [1]self.logger.info(f"Connection to {self.node}
> > > > succeeded")
> > > > > + self.send_expect("stty -echo", "#")
> > > > > + self.send_expect("stty columns 1000", "#")
> > > > First of all, thanks for those changes! Having DTS inside DPDK makes
> > > > test synchronization a lot easier. I'm happy to say (unsurprisingly)
> > > > that it works with my RISC-V HiFive Unmatched board like a charm.
> > > >
> > > >
> > > > Though there is a small issue with the lines above. They assume "#"
> > > > as
> > > > the prompt sign, even though original_prompt was set to "[$#>]".
> > > > This
> > > > touches on two problems:
> > > > 1. # is usually a root prompt - is DTS assumed to be run with root
> > > > privileges? DPDK may (in theory) run without them with some
> > > > permission
> > > > adjustment (hugetlb, VFIO container, etc.). If we assume DTS
> > > > needs
> > > > root access, this has to be both documented and validated before
> > > > running the whole suite. Otherwise it'll be hard to debug.
> > > >
> > > >
> > > > Around a year ago there were some attempts to get DTS to not require
> > > > root. This ended up running into issues because DTS sets up drivers for
> > > > you, which requires root as far as I know, as well as setting up
> > > > hugepages, which I think also requires root. The current version of DTS
> > > > can probably run without root, but it will probably stop working as
> > > > soon as DTS starts interacting with PCI devices. Elevating privileges
> > > > using pkexec or sudo is less portable and would require supporting a
> > > > lot more forms of authentication (kerberos/ldap for enterprise
> > > > deployments, passwords, 2fa, etc). It is much easier to say that the
> > > > default SSH agent must provide root access to the SUT and Traffic
> > > > Generator either with a password or pre-configured passwordless
> > > > authentication (ssh keys, kerberos, etc).
> > > >
> > > > [Honnappa] One of the feedback we collected asks to deprecate the use
> > > > of clear text passwords in config files and root user. It suggests to
> > > > use keys and sudo. It is a ‘Must Have’ item.
> > > >
> > > >
> > > > I agree it should be documented. I honestly didn't consider that anyone
> > > > would try running DTS as a non-root user.
> > > >
> > > > [Honnappa] +1 for supporting root users for now and documenting.
> > > >
> > > >
> > > > 2. Different shells use different prompts on different distros.
> > > > Hence
> > > > perhaps there should be a regex here (same as with
> > > > original_prompt)
> > > > and there could be a conf.yaml option to modify it on a per-host
> > > > basis?
> > > >
> > > >
> > > > As far as customizing the prompts, I think that is doable via a
> > > > configuration option.
> > > > As far as different shells, I don't think we were planning to support
> > > > anything besides either bash or posix-compatible shells. At the moment
> > > > all of the community lab systems use bash, and for ease of test
> > > > development it will be easier to mandate that everyone uses one shell.
> > > > Otherwise DTS CI will need to run once for each shell to catch issues,
> > > > which in my opinion are resources better spent on more in-depth testing
> > > > of DTS and DPDK.
> > > >
> > > > [Honnappa] +1 for using just bash, we can document this as well.
> > > >
> > >
> > > I would agree overall. Just supporting one shell is fine - certainly
> > > for now. Also completely agree that we need to remove hard-coded
> > > passwords and ideally non-root. However, I think for the initial
> > > versions the main thing should be removing the passwords so I would be ok
> for keeping the "root"
> > > login requirement, so long as we support using ssh keys for login
> > > rather than hard-coded passwords.
> > I would be for dropping support for the hard-coded passwords completely.
> Setting up the password-less SSH is straightforward (not sure if you meant the
> same).
> >
> > >
> > > /Bruce
> >
> >
> > I think the question is whether there are any platforms/devices that should be
> tested by DTS that do not support passwordless SSH. Right now, the community
> lab is using SSH keys for everything. If Intel also doesn't need passwords, then
> it's up to the community whether to support them at all. It does make it a lot
> easier on DTS if we can just require that the active OpenSSH agent can log into
> all of the systems involved without a password. This would also make it easier to
> enable AD authentication for Windows.
> >
> > [Honnappa] Jerin, Hemant do you have any concerns for your platforms
> regarding this?
>
> No. We are using passwordless SSH in our CI testing framework.
Based on the e-mail discussion and a brainstorming session (where we talked over the implications for the whole DTS codebase) the best approach seems to be:
1. Don't use root. Intead, use passwordless sudo with a regular user (where needed). This requires a bit of server setup, which I'll document.
2. Support password authentication, but don't document it. It would be basically a last resort if for any reason keys can't be used.
3. Strongly encourage the use of SSH keys by documenting it, but not documenting password logins.
Let me know what you all think, especially about 2 - does that make sense or do we want to remove password authentication support altogether?
Hi Stanislaw,
Neither of the current DTS maintainer nor me are the author of the code, so we can only speculate as to why certain parts were implemented the way they are.
We've thought a lot about replacing pexpect with something else, such as Fabric. Fabric is supposedly faster, which is the biggest draw and instead of fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For this PoC version though, we'd like to stay with this pexpect code and work on other appoaches in the next release cycle. The code is well tested so there's not much point in poking in it if it's to be replaced.
With that said, some more comments inline.
> > +class SSHPexpect:
> > + username: str
> > + password: str
> > + node: str
> > + logger: DTSLOG
> > + magic_prompt: str
> > +
> > + def __init__(
> > + self,
> > + node: str,
> > + username: str,
> > + password: Optional[str],
> > + logger: DTSLOG,
> > + ):
> > + self.magic_prompt = "MAGIC PROMPT"
> Why is this necessary? pxssh is already setting target prompt to
> pxssh.UNIQUE_PROMPT in the session constructor, to be specific:
>
> self.UNIQUE_PROMPT = r"\[PEXPECT\][\$\#] "
> self.PROMPT = self.UNIQUE_PROMPT
>
> Also session.login() will change target prompt to that, exactly for the reason of
> achieving a unique prompt that can be easily matched by pxssh.
>
> So if "MAGIC PROMPT is the prompt that you'd like to have on the remote host,
> then the following should be run after opening the session:
>
I believe this is here to have a prompt that won't be matched anywhere, to induce a timeout a collect all data up to that point.
> self.session.PROMPT = self.magic_prompt
> if not self.session.set_unique_prompt():
> do_some_error_handling()
>
> Otherwise it's unnecessary.
> > + self.logger = logger
> > +
> > + self.node = node
> > + self.username = username
> > + self.password = password or ""
> > + self.logger.info(f"ssh {self.username}@{self.node}")
> > +
> > + self._connect_host()
> > +
> > + def _connect_host(self) -> None:
> > + """
> > + Create connection to assigned node.
> > + """
> > + retry_times = 10
> > + try:
> > + if ":" in self.node:
> > + while retry_times:
> > + self.ip = self.node.split(":")[0]
> > + self.port = int(self.node.split(":")[1])
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + try:
> > + self.session.login(
> > + self.ip,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > + port=self.port,
> > + login_timeout=20,
> > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> key)|(?i)(password for .+:)",
> > + )
> > + except Exception as e:
> > + print(e)
> > + time.sleep(2)
> > + retry_times -= 1
> > + print("retry %d times connecting..." % (10 - retry_times))
> > + else:
> > + break
> > + else:
> > + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> > + else:
> > + self.session = pxssh.pxssh(encoding="utf-8")
> > + self.session.login(
> > + self.node,
> > + self.username,
> > + self.password,
> > + original_prompt="[$#>]",
> > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> key)|(?i)(password for .+:)",
> > + )
> > + self.logger.info(f"Connection to {self.node} succeeded")
> > + self.send_expect("stty -echo", "#")
> > + self.send_expect("stty columns 1000", "#")
> This works only by chance and makes hacks in get_output_before() necessary.
> After some testing it seems that pxssh is matching AND chomping the
> session.PROMPT when session.prompt() is called. Given the UNIQUE_PROMPT,
> the root user prompt will be "[PEXPECT]#" so this
> send_expect() will chomp # and leave "[PEXPECT]" as part of the output.
>
> Given that the two above lines do not require any special output I think
> self.send_command() should be used here.
Since we want to move away form using root, we'd need to address this, but this would be better left to the Fabric implementation and stay with the root requirement for now.
> > + except Exception as e:
> > + print(RED(str(e)))
> > + if getattr(self, "port", None):
> > + suggestion = (
> > + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> > + + "is stopped\n"
> > + )
> > + print(GREEN(suggestion))
> > +
> > + raise SSHConnectionException(self.node)
> > +
> > + def send_expect_base(self, command: str, expected: str, timeout: float) ->
> str:
> > + self.clean_session()
> > + self.session.PROMPT = expected
> > + self.__sendline(command)
> > + self.__prompt(command, timeout)
> > +
> > + before = self.get_output_before()
> Prompt should be reverted to whatever it was before leaving this function.
Seems reasonable. I guess it wasn't needed because of the requirement to use root. Adding this seems innocent enough.
> > + return before
> > +
> > + def send_expect(
> > + self, command: str, expected: str, timeout: float = 15, verify: bool = False
> > + ) -> str | int:
> > +
> > + try:
> > + ret = self.send_expect_base(command, expected, timeout)
> > + if verify:
> > + ret_status = self.send_expect_base("echo $?",
> > + expected, timeout)
> "echo $?" will only print the return code. How is it supposed to match
> "expected"? If "expected" is a return code then the first command's output
> probably won't match.
> I think send_command() should be used here.
Expected is the prompt to expect, "#" in most cases.
> > + if not int(ret_status):
> > + return ret
> The condition above seems like a C-ism used in python which again works by
> mistake. Return code 0 will convert to integer 0 which will be promoted to a
> boolean False. It would be more readable to change this block to:
> ri = int(ret_status)
> if ri != 0:
> # error prints
> return ri
This is common in Python, but really only usable if you know the object type. In this case it's always integer and it's perfectly fine to use it this way (0 = False, anything else = 1), but I agree that the double negative doesn't help with readibility. In any case, this is a minor thing a I there's not much of a reason to change it if we're to replace it with Fabric.
> > + else:
> > + self.logger.error("Command: %s failure!" % command)
> > + self.logger.error(ret)
> > + return int(ret_status)
> > + else:
> > + return ret
> > + except Exception as e:
> > + print(
> > + RED(
> > + "Exception happened in [%s] and output is [%s]"
> > + % (command, self.get_output_before())
> > + )
> > + )
> > + raise e
> > +
> > + def send_command(self, command: str, timeout: float = 1) -> str:
> > + try:
> > + self.clean_session()
> > + self.__sendline(command)
> > + except Exception as e:
> > + raise e
> > +
> > + output = self.get_session_before(timeout=timeout)
> > + self.session.PROMPT = self.session.UNIQUE_PROMPT
> > + self.session.prompt(0.1)
> This is wrong:
> 1. self.get_session_before() will return output of the command but since
> it changed the expected (not real!) prompt to self.magic_prompt, that
> won't be matched so the output will contain the prompt set by pxssh
> (UNIQUE_PROMPT).
> 2. Then prompt is reset to UNIQUE_PROMPT but and prompt() is called but
> that will only clean up the pxssh buffer. If get_session_before() was
> not changing the session.PROMPT from UNIQUE_PROMPT to magic_prompt,
> the second prompt() call would be unnecessary.
> > +
> > + return output
> > +
> > + def clean_session(self) -> None:
> > + self.get_session_before(timeout=0.01)
> What if remote host is slow for any reason? We'll timeout here. It seems that
> such a small timeout value was used because clean_session() is used in every
> send_command() call.
> Come to think of it, why is this call necessary when we have self.__flush()?
I think the timeout is deliberate. More below.
> > +
> > + def get_session_before(self, timeout: float = 15) -> str:
> > + """
> > + Get all output before timeout
> > + """
> > + self.session.PROMPT = self.magic_prompt
> This line has no effect. Remote prompt was never set to self.magic_prompt.
I think this is to ensure that we hit the timeout.
> > + try:
> > + self.session.prompt(timeout)
> > + except Exception as e:
> > + pass
> > +
> > + before = self.get_output_all()
> > + self.__flush()
> > +
> > + return before
> > +
> > + def __flush(self) -> None:
> > + """
> > + Clear all session buffer
> > + """
> > + self.session.buffer = ""
> > + self.session.before = ""
> > +
> > + def __prompt(self, command: str, timeout: float) -> None:
> > + if not self.session.prompt(timeout):
> > + raise TimeoutException(command, self.get_output_all())
> > + from None
> > +
> > + def __sendline(self, command: str) -> None:
> > + if not self.isalive():
> > + raise SSHSessionDeadException(self.node)
> > + if len(command) == 2 and command.startswith("^"):
> > + self.session.sendcontrol(command[1])
> > + else:
> > + self.session.sendline(command)
> > +
> > + def get_output_before(self) -> str:
> The name is missleading. In pxssh terms "before" means all the lines before the
> matched expect()/prompt(). Here it returns the last line of the output. Perhaps
> get_last_output_line() is better?
I thought so too, but this actually returns all lines except the last:
'a.b.c.d'.rsplit('.', 1)[0]
'a.b.c'
> > + if not self.isalive():
> > + raise SSHSessionDeadException(self.node)
> > + before: list[str] = self.session.before.rsplit("\r\n", 1)
> > + if before[0] == "[PEXPECT]":
> > + before[0] = ""
> Unnecessary if prompt was handled in proper way as mentioned above.
> > +
> > + return before[0]
> > +
> > + def get_output_all(self) -> str:
> > + output: str = self.session.before
> > + output.replace("[PEXPECT]", "")
> Ditto. If session.PROMPT was restored properly, this function would not be
> necessary at all.
> > + return output
> > +
> > + def close(self, force: bool = False) -> None:
> > + if force is True:
> > + self.session.close()
> > + else:
> > + if self.isalive():
> > + self.session.logout()
> > +
> > + def isalive(self) -> bool:
> > + return self.session.isalive()
> > diff --git a/dts/framework/utils.py b/dts/framework/utils.py new file
> > mode 100644 index 0000000000..db87349827
> > --- /dev/null
> > +++ b/dts/framework/utils.py
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > +# Copyright(c) 2022 University of New Hampshire #
> > +
> > +def RED(text: str) -> str:
> > + return f"\u001B[31;1m{str(text)}\u001B[0m"
> > +
> > +
> > +def GREEN(text: str) -> str:
> > + return f"\u001B[32;1m{str(text)}\u001B[0m"
> > --
> > 2.30.2
> >
>
> --
> Best Regards,
> Stanislaw Kardach
On Thu, Sep 22, 2022 at 09:41:40AM +0000, Juraj Linkeš wrote:
> Hi Stanislaw,
First a preface. I understand that the DTS rework is sponsored by
someone and there may be people waiting with their labs for this job to
be done.
Everything that I'll write below is from a point of view of a developer
who'd like to utilize DTS as a test suite for DPDK when adding support
for new PMDs or architectures/boards. This might be in conflict with
time-to-market metric at this point in time but I'm more focused on the
state of DPDK+DTS in the long run.
So feel free to disregard my comments if there are higher priorities.
>
> Neither of the current DTS maintainer nor me are the author of the code, so we can only speculate as to why certain parts were implemented the way they are.
>
> We've thought a lot about replacing pexpect with something else, such as Fabric. Fabric is supposedly faster, which is the biggest draw and instead of fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For this PoC version though, we'd like to stay with this pexpect code and work on other appoaches in the next release cycle. The code is well tested so there's not much point in poking in it if it's to be replaced.
I have a nasty experience of code staying without re-factoring for long
"because it works". When it comes to DTS my experience is that it works
only if used exactly on the setups it was meant for. Adapting it to a
new setup, new PMD or "even" running in the cloud shows that parts of it
are held together with a string.
I'm not blaming DTS devs here. Such approach is often needed for various
reasons (usually time-to-market) and it's hard to be forward-compatible.
That said I would suggest to use this opportunity to refactor DTS while
it's still not merged. Otherwise we'll be left with code that we're
uncertain why it works. That's not a quality-first approach and it'll
bite us in the backside in the future.
Let's do things right, not fast.
>
> With that said, some more comments inline.
>
> > > +class SSHPexpect:
> > > + username: str
> > > + password: str
> > > + node: str
> > > + logger: DTSLOG
> > > + magic_prompt: str
> > > +
> > > + def __init__(
> > > + self,
> > > + node: str,
> > > + username: str,
> > > + password: Optional[str],
> > > + logger: DTSLOG,
> > > + ):
> > > + self.magic_prompt = "MAGIC PROMPT"
> > Why is this necessary? pxssh is already setting target prompt to
> > pxssh.UNIQUE_PROMPT in the session constructor, to be specific:
> >
> > self.UNIQUE_PROMPT = r"\[PEXPECT\][\$\#] "
> > self.PROMPT = self.UNIQUE_PROMPT
> >
> > Also session.login() will change target prompt to that, exactly for the reason of
> > achieving a unique prompt that can be easily matched by pxssh.
> >
> > So if "MAGIC PROMPT is the prompt that you'd like to have on the remote host,
> > then the following should be run after opening the session:
> >
>
> I believe this is here to have a prompt that won't be matched anywhere, to induce a timeout a collect all data up to that point.
The only reason I can think of for doing this is to get the intermediate
output updates while the test is running. Are there any others?
If I'm right, how would that be different from matching an actual prompt
with a timeout? pexpect is already setting an unusual prompt that will
not be matched by anything else ("[PEXPECT] #").
Just use that with a small timeout and you get the same effect,
including timeout triggering an exception.
>
> > self.session.PROMPT = self.magic_prompt
> > if not self.session.set_unique_prompt():
> > do_some_error_handling()
> >
> > Otherwise it's unnecessary.
> > > + self.logger = logger
> > > +
> > > + self.node = node
> > > + self.username = username
> > > + self.password = password or ""
> > > + self.logger.info(f"ssh {self.username}@{self.node}")
> > > +
> > > + self._connect_host()
> > > +
> > > + def _connect_host(self) -> None:
> > > + """
> > > + Create connection to assigned node.
> > > + """
> > > + retry_times = 10
> > > + try:
> > > + if ":" in self.node:
> > > + while retry_times:
> > > + self.ip = self.node.split(":")[0]
> > > + self.port = int(self.node.split(":")[1])
> > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > + try:
> > > + self.session.login(
> > > + self.ip,
> > > + self.username,
> > > + self.password,
> > > + original_prompt="[$#>]",
> > > + port=self.port,
> > > + login_timeout=20,
> > > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> > key)|(?i)(password for .+:)",
> > > + )
> > > + except Exception as e:
> > > + print(e)
> > > + time.sleep(2)
> > > + retry_times -= 1
> > > + print("retry %d times connecting..." % (10 - retry_times))
> > > + else:
> > > + break
> > > + else:
> > > + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> > > + else:
> > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > + self.session.login(
> > > + self.node,
> > > + self.username,
> > > + self.password,
> > > + original_prompt="[$#>]",
> > > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> > key)|(?i)(password for .+:)",
> > > + )
> > > + self.logger.info(f"Connection to {self.node} succeeded")
> > > + self.send_expect("stty -echo", "#")
> > > + self.send_expect("stty columns 1000", "#")
> > This works only by chance and makes hacks in get_output_before() necessary.
> > After some testing it seems that pxssh is matching AND chomping the
> > session.PROMPT when session.prompt() is called. Given the UNIQUE_PROMPT,
> > the root user prompt will be "[PEXPECT]#" so this
> > send_expect() will chomp # and leave "[PEXPECT]" as part of the output.
> >
> > Given that the two above lines do not require any special output I think
> > self.send_command() should be used here.
>
> Since we want to move away form using root, we'd need to address this, but this would be better left to the Fabric implementation and stay with the root requirement for now.
It's more than that. This self.send_expect("...", "#") pattern will
cause the hacks on removing the pexpect's prompt that are done below.
Those hack are not necessary at all if pexpect is used properly. If this
is not understood, then when switching to Fabric, someone might repeat
them thinking they're on purpose.
>
> > > + except Exception as e:
> > > + print(RED(str(e)))
> > > + if getattr(self, "port", None):
> > > + suggestion = (
> > > + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> > > + + "is stopped\n"
> > > + )
> > > + print(GREEN(suggestion))
> > > +
> > > + raise SSHConnectionException(self.node)
> > > +
> > > + def send_expect_base(self, command: str, expected: str, timeout: float) ->
> > str:
> > > + self.clean_session()
> > > + self.session.PROMPT = expected
> > > + self.__sendline(command)
> > > + self.__prompt(command, timeout)
> > > +
> > > + before = self.get_output_before()
> > Prompt should be reverted to whatever it was before leaving this function.
>
> Seems reasonable. I guess it wasn't needed because of the requirement to use root. Adding this seems innocent enough.
>
> > > + return before
> > > +
> > > + def send_expect(
> > > + self, command: str, expected: str, timeout: float = 15, verify: bool = False
> > > + ) -> str | int:
> > > +
> > > + try:
> > > + ret = self.send_expect_base(command, expected, timeout)
> > > + if verify:
> > > + ret_status = self.send_expect_base("echo $?",
> > > + expected, timeout)
> > "echo $?" will only print the return code. How is it supposed to match
> > "expected"? If "expected" is a return code then the first command's output
> > probably won't match.
> > I think send_command() should be used here.
>
> Expected is the prompt to expect, "#" in most cases.
Then it should not be called "expected" but "prompt", otherwise someone
might use this API to actually match something in the ssh output (which
is a totally valid use-case). Look at the docstring of
dts.framework.node.Node.send_expect():
"""
Send commands to node and return string before expected string.
"""
Not to mention that the "verify" parameter is not documented and will
NOT work if "expected" is anything else than a prompt.
I wonder how many DTS tests have to work around or utilize those hacks?
There may be some test cases which work only because of them which is a
wrong way to solve a problem but again - maybe there were other
priorities.
>
> > > + if not int(ret_status):
> > > + return ret
> > The condition above seems like a C-ism used in python which again works by
> > mistake. Return code 0 will convert to integer 0 which will be promoted to a
> > boolean False. It would be more readable to change this block to:
> > ri = int(ret_status)
> > if ri != 0:
> > # error prints
> > return ri
>
> This is common in Python, but really only usable if you know the object type. In this case it's always integer and it's perfectly fine to use it this way (0 = False, anything else = 1), but I agree that the double negative doesn't help with readibility. In any case, this is a minor thing a I there's not much of a reason to change it if we're to replace it with Fabric.
Agreed, this can wait.
>
> > > + else:
> > > + self.logger.error("Command: %s failure!" % command)
> > > + self.logger.error(ret)
> > > + return int(ret_status)
> > > + else:
> > > + return ret
> > > + except Exception as e:
> > > + print(
> > > + RED(
> > > + "Exception happened in [%s] and output is [%s]"
> > > + % (command, self.get_output_before())
> > > + )
> > > + )
> > > + raise e
> > > +
> > > + def send_command(self, command: str, timeout: float = 1) -> str:
> > > + try:
> > > + self.clean_session()
> > > + self.__sendline(command)
> > > + except Exception as e:
> > > + raise e
> > > +
> > > + output = self.get_session_before(timeout=timeout)
> > > + self.session.PROMPT = self.session.UNIQUE_PROMPT
> > > + self.session.prompt(0.1)
> > This is wrong:
> > 1. self.get_session_before() will return output of the command but since
> > it changed the expected (not real!) prompt to self.magic_prompt, that
> > won't be matched so the output will contain the prompt set by pxssh
> > (UNIQUE_PROMPT).
> > 2. Then prompt is reset to UNIQUE_PROMPT but and prompt() is called but
> > that will only clean up the pxssh buffer. If get_session_before() was
> > not changing the session.PROMPT from UNIQUE_PROMPT to magic_prompt,
> > the second prompt() call would be unnecessary.
> > > +
> > > + return output
> > > +
> > > + def clean_session(self) -> None:
> > > + self.get_session_before(timeout=0.01)
> > What if remote host is slow for any reason? We'll timeout here. It seems that
> > such a small timeout value was used because clean_session() is used in every
> > send_command() call.
> > Come to think of it, why is this call necessary when we have self.__flush()?
>
> I think the timeout is deliberate. More below.
>
> > > +
> > > + def get_session_before(self, timeout: float = 15) -> str:
> > > + """
> > > + Get all output before timeout
> > > + """
> > > + self.session.PROMPT = self.magic_prompt
> > This line has no effect. Remote prompt was never set to self.magic_prompt.
>
> I think this is to ensure that we hit the timeout.
Why would we intentionally want to hit a timeout? Unless I'm missing
something, it'll only cause us to wait for "timeout" even if the command
ended already. Also note the full logic of the "get_session_before()":
1. Trigger self.session.prompt() to get the session buffer updated with
whatever output was between now and now+timeout seconds.
2. Get the output but since we were matching the self.magic_prompt,
self.get_output_all() has to remove the real prompt ([PEXPECT]) if
the command ended.
3. Flush the pexpect's ssh buffer so that next call won't read what has
already been returned (that's the job of self.__flush()).
So I don't think there is any reason to use this always-timeout logic.
>
> > > + try:
> > > + self.session.prompt(timeout)
> > > + except Exception as e:
> > > + pass
> > > +
> > > + before = self.get_output_all()
> > > + self.__flush()
> > > +
> > > + return before
> > > +
> > > + def __flush(self) -> None:
> > > + """
> > > + Clear all session buffer
> > > + """
> > > + self.session.buffer = ""
> > > + self.session.before = ""
> > > +
> > > + def __prompt(self, command: str, timeout: float) -> None:
> > > + if not self.session.prompt(timeout):
> > > + raise TimeoutException(command, self.get_output_all())
> > > + from None
> > > +
> > > + def __sendline(self, command: str) -> None:
> > > + if not self.isalive():
> > > + raise SSHSessionDeadException(self.node)
> > > + if len(command) == 2 and command.startswith("^"):
> > > + self.session.sendcontrol(command[1])
> > > + else:
> > > + self.session.sendline(command)
> > > +
> > > + def get_output_before(self) -> str:
> > The name is missleading. In pxssh terms "before" means all the lines before the
> > matched expect()/prompt(). Here it returns the last line of the output. Perhaps
> > get_last_output_line() is better?
>
> I thought so too, but this actually returns all lines except the last:
> 'a.b.c.d'.rsplit('.', 1)[0]
> 'a.b.c'
>
Oh, I've totally missed that!
If that's the case then this function is a result of the always-timeout
logic. Note that get_output_before() is only called from
send_expect_base() (and indirectly by send_expect()) to get the output
of the command without the prompt.
So the reason why self.send_expect("echo 'foo'", "#") returns a proper
output ('foo\n') is:
0. On login() time pexpect sets the remote prompt to "[PEXPECT]#".
1. send_expect() -> self.send_expect_base() sets the match prompt to #.
This causes self.session.prompt() to update the self.session.buffer,
match # and chomp it, leaving the ssh buffer as:
foo
[PEXPECT]
2. send_expect_base() calls self.get_output_before() which cuts the last
line of the output. Note that the "if" at the end is necessary for
commands that do not return any output. Otherwise the output would
be "[PEXPECT]". Also note that if there are any control characters
printed by the remote shell, then this will also not work because
a straight string comparison is done, not a regex match.
Contrary, if self.magic_prompt was not used and pexpect's original
prompt was left undisturbed, then all the hacks above would not be
needed and the code would be cleaner.
> > > + if not self.isalive():
> > > + raise SSHSessionDeadException(self.node)
> > > + before: list[str] = self.session.before.rsplit("\r\n", 1)
> > > + if before[0] == "[PEXPECT]":
> > > + before[0] = ""
> > Unnecessary if prompt was handled in proper way as mentioned above.
> > > +
> > > + return before[0]
> > > +
> > > + def get_output_all(self) -> str:
> > > + output: str = self.session.before
> > > + output.replace("[PEXPECT]", "")
> > Ditto. If session.PROMPT was restored properly, this function would not be
> > necessary at all.
> > > + return output
> > > +
> > > + def close(self, force: bool = False) -> None:
> > > + if force is True:
> > > + self.session.close()
> > > + else:
> > > + if self.isalive():
> > > + self.session.logout()
> > > +
> > > + def isalive(self) -> bool:
> > > + return self.session.isalive()
> > > diff --git a/dts/framework/utils.py b/dts/framework/utils.py new file
> > > mode 100644 index 0000000000..db87349827
> > > --- /dev/null
> > > +++ b/dts/framework/utils.py
> > > @@ -0,0 +1,12 @@
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > > +# Copyright(c) 2022 University of New Hampshire #
> > > +
> > > +def RED(text: str) -> str:
> > > + return f"\u001B[31;1m{str(text)}\u001B[0m"
> > > +
> > > +
> > > +def GREEN(text: str) -> str:
> > > + return f"\u001B[32;1m{str(text)}\u001B[0m"
> > > --
> > > 2.30.2
> > >
> >
> > --
> > Best Regards,
> > Stanislaw Kardach
>
> -----Original Message-----
> From: Stanislaw Kardach <kda@semihalf.com>
> Sent: Thursday, September 22, 2022 4:32 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
>
> On Thu, Sep 22, 2022 at 09:41:40AM +0000, Juraj Linkeš wrote:
> > Hi Stanislaw,
> First a preface. I understand that the DTS rework is sponsored by someone and
> there may be people waiting with their labs for this job to be done.
> Everything that I'll write below is from a point of view of a developer who'd like
> to utilize DTS as a test suite for DPDK when adding support for new PMDs or
> architectures/boards. This might be in conflict with time-to-market metric at this
> point in time but I'm more focused on the state of DPDK+DTS in the long run.
> So feel free to disregard my comments if there are higher priorities.
> >
> > Neither of the current DTS maintainer nor me are the author of the code, so
> we can only speculate as to why certain parts were implemented the way they
> are.
> >
> > We've thought a lot about replacing pexpect with something else, such as
> Fabric. Fabric is supposedly faster, which is the biggest draw and instead of
> fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For
> this PoC version though, we'd like to stay with this pexpect code and work on
> other appoaches in the next release cycle. The code is well tested so there's not
> much point in poking in it if it's to be replaced.
> I have a nasty experience of code staying without re-factoring for long "because
> it works". When it comes to DTS my experience is that it works only if used
> exactly on the setups it was meant for. Adapting it to a new setup, new PMD or
> "even" running in the cloud shows that parts of it are held together with a string.
> I'm not blaming DTS devs here. Such approach is often needed for various
> reasons (usually time-to-market) and it's hard to be forward-compatible.
>
> That said I would suggest to use this opportunity to refactor DTS while it's still
> not merged. Otherwise we'll be left with code that we're uncertain why it works.
> That's not a quality-first approach and it'll bite us in the backside in the future.
>
> Let's do things right, not fast.
Absolutely, but effective time use is also something to consider. Our current plan doesn't won't really have to contend with problems in the future, as we want to add the Farbic implementation in the next release cycle. I'm also working on refactoring the code a bit - I'm adding an abstraction that would allow us to easily replace the pexpect implementation with Fabric (with no impact on DTS behavior - the same APIs will need to be implemented). Also, we'll remove the pexpect implementation once Fabric is in place (unless we can think of a reason for pexpect to stay, in which case we'll need to refactor it). I think that instead of focusing on pexpect we could focus on making sure the replacement won't cause any issues. What do you think?
> >
> > With that said, some more comments inline.
> >
> > > > +class SSHPexpect:
> > > > + username: str
> > > > + password: str
> > > > + node: str
> > > > + logger: DTSLOG
> > > > + magic_prompt: str
> > > > +
> > > > + def __init__(
> > > > + self,
> > > > + node: str,
> > > > + username: str,
> > > > + password: Optional[str],
> > > > + logger: DTSLOG,
> > > > + ):
> > > > + self.magic_prompt = "MAGIC PROMPT"
> > > Why is this necessary? pxssh is already setting target prompt to
> > > pxssh.UNIQUE_PROMPT in the session constructor, to be specific:
> > >
> > > self.UNIQUE_PROMPT = r"\[PEXPECT\][\$\#] "
> > > self.PROMPT = self.UNIQUE_PROMPT
> > >
> > > Also session.login() will change target prompt to that, exactly for
> > > the reason of achieving a unique prompt that can be easily matched by
> pxssh.
> > >
> > > So if "MAGIC PROMPT is the prompt that you'd like to have on the
> > > remote host, then the following should be run after opening the session:
> > >
> >
> > I believe this is here to have a prompt that won't be matched anywhere, to
> induce a timeout a collect all data up to that point.
> The only reason I can think of for doing this is to get the intermediate output
> updates while the test is running. Are there any others?
>
> If I'm right, how would that be different from matching an actual prompt with a
> timeout? pexpect is already setting an unusual prompt that will not be matched
> by anything else ("[PEXPECT] #").
> Just use that with a small timeout and you get the same effect, including
> timeout triggering an exception.
Some of my comments were only speculation I could come up. That doesn't justify anything, but it could shed some light on the motivation behind the implementation.
You're probably right on this point. Parts of the DTS code seem somewhat pointless to me as well.
> >
> > > self.session.PROMPT = self.magic_prompt
> > > if not self.session.set_unique_prompt():
> > > do_some_error_handling()
> > >
> > > Otherwise it's unnecessary.
> > > > + self.logger = logger
> > > > +
> > > > + self.node = node
> > > > + self.username = username
> > > > + self.password = password or ""
> > > > + self.logger.info(f"ssh {self.username}@{self.node}")
> > > > +
> > > > + self._connect_host()
> > > > +
> > > > + def _connect_host(self) -> None:
> > > > + """
> > > > + Create connection to assigned node.
> > > > + """
> > > > + retry_times = 10
> > > > + try:
> > > > + if ":" in self.node:
> > > > + while retry_times:
> > > > + self.ip = self.node.split(":")[0]
> > > > + self.port = int(self.node.split(":")[1])
> > > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > > + try:
> > > > + self.session.login(
> > > > + self.ip,
> > > > + self.username,
> > > > + self.password,
> > > > + original_prompt="[$#>]",
> > > > + port=self.port,
> > > > + login_timeout=20,
> > > > +
> > > > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> > > key)|(?i)(password for .+:)",
> > > > + )
> > > > + except Exception as e:
> > > > + print(e)
> > > > + time.sleep(2)
> > > > + retry_times -= 1
> > > > + print("retry %d times connecting..." % (10 - retry_times))
> > > > + else:
> > > > + break
> > > > + else:
> > > > + raise Exception("connect to %s:%s failed" % (self.ip, self.port))
> > > > + else:
> > > > + self.session = pxssh.pxssh(encoding="utf-8")
> > > > + self.session.login(
> > > > + self.node,
> > > > + self.username,
> > > > + self.password,
> > > > + original_prompt="[$#>]",
> > > > +
> > > > + password_regex=r"(?i)(?:password:)|(?:passphrase for
> > > key)|(?i)(password for .+:)",
> > > > + )
> > > > + self.logger.info(f"Connection to {self.node} succeeded")
> > > > + self.send_expect("stty -echo", "#")
> > > > + self.send_expect("stty columns 1000", "#")
> > > This works only by chance and makes hacks in get_output_before()
> necessary.
> > > After some testing it seems that pxssh is matching AND chomping the
> > > session.PROMPT when session.prompt() is called. Given the
> > > UNIQUE_PROMPT, the root user prompt will be "[PEXPECT]#" so this
> > > send_expect() will chomp # and leave "[PEXPECT]" as part of the output.
> > >
> > > Given that the two above lines do not require any special output I
> > > think
> > > self.send_command() should be used here.
> >
> > Since we want to move away form using root, we'd need to address this, but
> this would be better left to the Fabric implementation and stay with the root
> requirement for now.
> It's more than that. This self.send_expect("...", "#") pattern will cause the hacks
> on removing the pexpect's prompt that are done below.
> Those hack are not necessary at all if pexpect is used properly. If this is not
> understood, then when switching to Fabric, someone might repeat them
> thinking they're on purpose.
True. But we know the code is not great so the new implementation won't really consider the pexpect implementation. I mentioned an abstraction above - that will define what needs to be implemented in Fabric.
> >
> > > > + except Exception as e:
> > > > + print(RED(str(e)))
> > > > + if getattr(self, "port", None):
> > > > + suggestion = (
> > > > + "\nSuggession: Check if the firewall on [ %s ] " % self.ip
> > > > + + "is stopped\n"
> > > > + )
> > > > + print(GREEN(suggestion))
> > > > +
> > > > + raise SSHConnectionException(self.node)
> > > > +
> > > > + def send_expect_base(self, command: str, expected: str,
> > > > + timeout: float) ->
> > > str:
> > > > + self.clean_session()
> > > > + self.session.PROMPT = expected
> > > > + self.__sendline(command)
> > > > + self.__prompt(command, timeout)
> > > > +
> > > > + before = self.get_output_before()
> > > Prompt should be reverted to whatever it was before leaving this function.
> >
> > Seems reasonable. I guess it wasn't needed because of the requirement to use
> root. Adding this seems innocent enough.
> >
> > > > + return before
> > > > +
> > > > + def send_expect(
> > > > + self, command: str, expected: str, timeout: float = 15, verify: bool =
> False
> > > > + ) -> str | int:
> > > > +
> > > > + try:
> > > > + ret = self.send_expect_base(command, expected, timeout)
> > > > + if verify:
> > > > + ret_status = self.send_expect_base("echo $?",
> > > > + expected, timeout)
> > > "echo $?" will only print the return code. How is it supposed to
> > > match "expected"? If "expected" is a return code then the first
> > > command's output probably won't match.
> > > I think send_command() should be used here.
> >
> > Expected is the prompt to expect, "#" in most cases.
> Then it should not be called "expected" but "prompt", otherwise someone might
> use this API to actually match something in the ssh output (which is a totally
> valid use-case). Look at the docstring of
> dts.framework.node.Node.send_expect():
>
> """
> Send commands to node and return string before expected string.
> """
>
> Not to mention that the "verify" parameter is not documented and will NOT
> work if "expected" is anything else than a prompt.
>
I'll rename it, that won't really impact anything. And I can also document the "verify" parameter.
> I wonder how many DTS tests have to work around or utilize those hacks?
> There may be some test cases which work only because of them which is a
> wrong way to solve a problem but again - maybe there were other priorities.
I'd image this being the case.
> >
> > > > + if not int(ret_status):
> > > > + return ret
> > > The condition above seems like a C-ism used in python which again
> > > works by mistake. Return code 0 will convert to integer 0 which will
> > > be promoted to a boolean False. It would be more readable to change this
> block to:
> > > ri = int(ret_status)
> > > if ri != 0:
> > > # error prints
> > > return ri
> >
> > This is common in Python, but really only usable if you know the object type. In
> this case it's always integer and it's perfectly fine to use it this way (0 = False,
> anything else = 1), but I agree that the double negative doesn't help with
> readibility. In any case, this is a minor thing a I there's not much of a reason to
> change it if we're to replace it with Fabric.
> Agreed, this can wait.
> >
> > > > + else:
> > > > + self.logger.error("Command: %s failure!" % command)
> > > > + self.logger.error(ret)
> > > > + return int(ret_status)
> > > > + else:
> > > > + return ret
> > > > + except Exception as e:
> > > > + print(
> > > > + RED(
> > > > + "Exception happened in [%s] and output is [%s]"
> > > > + % (command, self.get_output_before())
> > > > + )
> > > > + )
> > > > + raise e
> > > > +
> > > > + def send_command(self, command: str, timeout: float = 1) -> str:
> > > > + try:
> > > > + self.clean_session()
> > > > + self.__sendline(command)
> > > > + except Exception as e:
> > > > + raise e
> > > > +
> > > > + output = self.get_session_before(timeout=timeout)
> > > > + self.session.PROMPT = self.session.UNIQUE_PROMPT
> > > > + self.session.prompt(0.1)
> > > This is wrong:
> > > 1. self.get_session_before() will return output of the command but since
> > > it changed the expected (not real!) prompt to self.magic_prompt, that
> > > won't be matched so the output will contain the prompt set by pxssh
> > > (UNIQUE_PROMPT).
> > > 2. Then prompt is reset to UNIQUE_PROMPT but and prompt() is called but
> > > that will only clean up the pxssh buffer. If get_session_before() was
> > > not changing the session.PROMPT from UNIQUE_PROMPT to
> magic_prompt,
> > > the second prompt() call would be unnecessary.
> > > > +
> > > > + return output
> > > > +
> > > > + def clean_session(self) -> None:
> > > > + self.get_session_before(timeout=0.01)
> > > What if remote host is slow for any reason? We'll timeout here. It
> > > seems that such a small timeout value was used because
> > > clean_session() is used in every
> > > send_command() call.
> > > Come to think of it, why is this call necessary when we have self.__flush()?
> >
> > I think the timeout is deliberate. More below.
> >
> > > > +
> > > > + def get_session_before(self, timeout: float = 15) -> str:
> > > > + """
> > > > + Get all output before timeout
> > > > + """
> > > > + self.session.PROMPT = self.magic_prompt
> > > This line has no effect. Remote prompt was never set to self.magic_prompt.
> >
> > I think this is to ensure that we hit the timeout.
> Why would we intentionally want to hit a timeout? Unless I'm missing
> something, it'll only cause us to wait for "timeout" even if the command ended
> already. Also note the full logic of the "get_session_before()":
> 1. Trigger self.session.prompt() to get the session buffer updated with
> whatever output was between now and now+timeout seconds.
> 2. Get the output but since we were matching the self.magic_prompt,
> self.get_output_all() has to remove the real prompt ([PEXPECT]) if
> the command ended.
> 3. Flush the pexpect's ssh buffer so that next call won't read what has
> already been returned (that's the job of self.__flush()).
>
> So I don't think there is any reason to use this always-timeout logic.
Yes. It seems to me there's a lot of superfluous stuff in the pexpect implementation. We basically only need send_command and get_output apart from the init/cleanup methods.
> >
> > > > + try:
> > > > + self.session.prompt(timeout)
> > > > + except Exception as e:
> > > > + pass
> > > > +
> > > > + before = self.get_output_all()
> > > > + self.__flush()
> > > > +
> > > > + return before
> > > > +
> > > > + def __flush(self) -> None:
> > > > + """
> > > > + Clear all session buffer
> > > > + """
> > > > + self.session.buffer = ""
> > > > + self.session.before = ""
> > > > +
> > > > + def __prompt(self, command: str, timeout: float) -> None:
> > > > + if not self.session.prompt(timeout):
> > > > + raise TimeoutException(command,
> > > > + self.get_output_all()) from None
> > > > +
> > > > + def __sendline(self, command: str) -> None:
> > > > + if not self.isalive():
> > > > + raise SSHSessionDeadException(self.node)
> > > > + if len(command) == 2 and command.startswith("^"):
> > > > + self.session.sendcontrol(command[1])
> > > > + else:
> > > > + self.session.sendline(command)
> > > > +
> > > > + def get_output_before(self) -> str:
> > > The name is missleading. In pxssh terms "before" means all the lines
> > > before the matched expect()/prompt(). Here it returns the last line
> > > of the output. Perhaps
> > > get_last_output_line() is better?
> >
> > I thought so too, but this actually returns all lines except the last:
> > 'a.b.c.d'.rsplit('.', 1)[0]
> > 'a.b.c'
> >
> Oh, I've totally missed that!
> If that's the case then this function is a result of the always-timeout logic. Note
> that get_output_before() is only called from
> send_expect_base() (and indirectly by send_expect()) to get the output of the
> command without the prompt.
> So the reason why self.send_expect("echo 'foo'", "#") returns a proper output
> ('foo\n') is:
> 0. On login() time pexpect sets the remote prompt to "[PEXPECT]#".
> 1. send_expect() -> self.send_expect_base() sets the match prompt to #.
> This causes self.session.prompt() to update the self.session.buffer,
> match # and chomp it, leaving the ssh buffer as:
> foo
> [PEXPECT]
> 2. send_expect_base() calls self.get_output_before() which cuts the last
> line of the output. Note that the "if" at the end is necessary for
> commands that do not return any output. Otherwise the output would
> be "[PEXPECT]". Also note that if there are any control characters
> printed by the remote shell, then this will also not work because
> a straight string comparison is done, not a regex match.
>
> Contrary, if self.magic_prompt was not used and pexpect's original prompt was
> left undisturbed, then all the hacks above would not be needed and the code
> would be cleaner.
Yes. On top of that, the two output functions actually do the same thing. Or maybe not strictly, but I think the intention in both was to remove the last line containing "[PEXPECT]".
> > > > + if not self.isalive():
> > > > + raise SSHSessionDeadException(self.node)
> > > > + before: list[str] = self.session.before.rsplit("\r\n", 1)
> > > > + if before[0] == "[PEXPECT]":
> > > > + before[0] = ""
> > > Unnecessary if prompt was handled in proper way as mentioned above.
> > > > +
> > > > + return before[0]
> > > > +
> > > > + def get_output_all(self) -> str:
> > > > + output: str = self.session.before
> > > > + output.replace("[PEXPECT]", "")
> > > Ditto. If session.PROMPT was restored properly, this function would
> > > not be necessary at all.
> > > > + return output
> > > > +
> > > > + def close(self, force: bool = False) -> None:
> > > > + if force is True:
> > > > + self.session.close()
> > > > + else:
> > > > + if self.isalive():
> > > > + self.session.logout()
> > > > +
> > > > + def isalive(self) -> bool:
> > > > + return self.session.isalive()
> > > > diff --git a/dts/framework/utils.py b/dts/framework/utils.py new
> > > > file mode 100644 index 0000000000..db87349827
> > > > --- /dev/null
> > > > +++ b/dts/framework/utils.py
> > > > @@ -0,0 +1,12 @@
> > > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2010-2014
> > > > +Intel Corporation # Copyright(c) 2022 PANTHEON.tech s.r.o.
> > > > +# Copyright(c) 2022 University of New Hampshire #
> > > > +
> > > > +def RED(text: str) -> str:
> > > > + return f"\u001B[31;1m{str(text)}\u001B[0m"
> > > > +
> > > > +
> > > > +def GREEN(text: str) -> str:
> > > > + return f"\u001B[32;1m{str(text)}\u001B[0m"
> > > > --
> > > > 2.30.2
> > > >
> > >
> > > --
> > > Best Regards,
> > > Stanislaw Kardach
> >
>
> --
> Best Regards,
> Stanislaw Kardach
On Fri, Sep 23, 2022 at 07:22:26AM +0000, Juraj Linkeš wrote:
>
>
> > -----Original Message-----
> > From: Stanislaw Kardach <kda@semihalf.com>
> > Sent: Thursday, September 22, 2022 4:32 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > ronan.randles@intel.com; Honnappa.Nagarahalli@arm.com;
> > ohilyard@iol.unh.edu; lijuan.tu@intel.com; dev@dpdk.org
> > Subject: Re: [PATCH v4 4/9] dts: add ssh pexpect library
> >
> > On Thu, Sep 22, 2022 at 09:41:40AM +0000, Juraj Linkeš wrote:
> > > Hi Stanislaw,
> > First a preface. I understand that the DTS rework is sponsored by someone and
> > there may be people waiting with their labs for this job to be done.
> > Everything that I'll write below is from a point of view of a developer who'd like
> > to utilize DTS as a test suite for DPDK when adding support for new PMDs or
> > architectures/boards. This might be in conflict with time-to-market metric at this
> > point in time but I'm more focused on the state of DPDK+DTS in the long run.
> > So feel free to disregard my comments if there are higher priorities.
> > >
> > > Neither of the current DTS maintainer nor me are the author of the code, so
> > we can only speculate as to why certain parts were implemented the way they
> > are.
> > >
> > > We've thought a lot about replacing pexpect with something else, such as
> > Fabric. Fabric is supposedly faster, which is the biggest draw and instead of
> > fixing/reworking pexpect code, it makes sense to switch our focus on Fabric. For
> > this PoC version though, we'd like to stay with this pexpect code and work on
> > other appoaches in the next release cycle. The code is well tested so there's not
> > much point in poking in it if it's to be replaced.
> > I have a nasty experience of code staying without re-factoring for long "because
> > it works". When it comes to DTS my experience is that it works only if used
> > exactly on the setups it was meant for. Adapting it to a new setup, new PMD or
> > "even" running in the cloud shows that parts of it are held together with a string.
> > I'm not blaming DTS devs here. Such approach is often needed for various
> > reasons (usually time-to-market) and it's hard to be forward-compatible.
> >
> > That said I would suggest to use this opportunity to refactor DTS while it's still
> > not merged. Otherwise we'll be left with code that we're uncertain why it works.
> > That's not a quality-first approach and it'll bite us in the backside in the future.
> >
> > Let's do things right, not fast.
>
> Absolutely, but effective time use is also something to consider. Our current plan doesn't won't really have to contend with problems in the future, as we want to add the Farbic implementation in the next release cycle. I'm also working on refactoring the code a bit - I'm adding an abstraction that would allow us to easily replace the pexpect implementation with Fabric (with no impact on DTS behavior - the same APIs will need to be implemented). Also, we'll remove the pexpect implementation once Fabric is in place (unless we can think of a reason for pexpect to stay, in which case we'll need to refactor it). I think that instead of focusing on pexpect we could focus on making sure the replacement won't cause any issues. What do you think?
>
Personally, I would be very keen to get the move of DTS to the main repo
underway, and so I wouldn't look to have too many massive changes required
before we start seeing patches merged in. Basic code cleanup and
refactoring is fine, but I would think that requiring massive changes like
replacing expect with fabric may be too big an ask. After all, the rest of
DPDK is moving on, meaning more and more DTS content is being added to the
separate DTS repo every release, making the job bigger each time. :-(
Tl;dr - I'm ok to leave fabric replacement for a release next year.
/Bruce
On Fri, Sep 23, 2022 at 09:15:07AM +0100, Bruce Richardson wrote:
> On Fri, Sep 23, 2022 at 07:22:26AM +0000, Juraj Linkeš wrote:
<snip>
> >
> > Absolutely, but effective time use is also something to consider. Our current plan doesn't won't really have to contend with problems in the future, as we want to add the Farbic implementation in the next release cycle. I'm also working on refactoring the code a bit - I'm adding an abstraction that would allow us to easily replace the pexpect implementation with Fabric (with no impact on DTS behavior - the same APIs will need to be implemented). Also, we'll remove the pexpect implementation once Fabric is in place (unless we can think of a reason for pexpect to stay, in which case we'll need to refactor it). I think that instead of focusing on pexpect we could focus on making sure the replacement won't cause any issues. What do you think?
> >
>
> Personally, I would be very keen to get the move of DTS to the main repo
> underway, and so I wouldn't look to have too many massive changes required
> before we start seeing patches merged in. Basic code cleanup and
> refactoring is fine, but I would think that requiring massive changes like
> replacing expect with fabric may be too big an ask. After all, the rest of
> DPDK is moving on, meaning more and more DTS content is being added to the
> separate DTS repo every release, making the job bigger each time. :-(
>
> Tl;dr - I'm ok to leave fabric replacement for a release next year.
>
> /Bruce
That makes sense. I would suggest however to put comments around
implementation hacks and the public APIs simply to not forget.
Though I'd do the refactoring sooner than later because once tests start
being merged there will be a higher possibility of them relying on
hacks.
new file mode 100644
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2010-2014 Intel Corporation
+# Copyright(c) 2022 PANTHEON.tech s.r.o.
+# Copyright(c) 2022 University of New Hampshire
+#
+
+"""
+User-defined exceptions used across the framework.
+"""
+
+
+class TimeoutException(Exception):
+ """
+ Command execution timeout.
+ """
+
+ command: str
+ output: str
+
+ def __init__(self, command: str, output: str):
+ self.command = command
+ self.output = output
+
+ def __str__(self) -> str:
+ return f"TIMEOUT on {self.command}"
+
+ def get_output(self) -> str:
+ return self.output
+
+
+class SSHConnectionException(Exception):
+ """
+ SSH connection error.
+ """
+
+ host: str
+
+ def __init__(self, host: str):
+ self.host = host
+
+ def __str__(self) -> str:
+ return f"Error trying to connect with {self.host}"
+
+
+class SSHSessionDeadException(Exception):
+ """
+ SSH session is not alive.
+ It can no longer be used.
+ """
+
+ host: str
+
+ def __init__(self, host: str):
+ self.host = host
+
+ def __str__(self) -> str:
+ return f"SSH session with {self.host} has died"
new file mode 100644
@@ -0,0 +1,205 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2010-2014 Intel Corporation
+# Copyright(c) 2022 PANTHEON.tech s.r.o.
+# Copyright(c) 2022 University of New Hampshire
+#
+
+import time
+from typing import Optional
+
+from pexpect import pxssh
+
+from .exception import SSHConnectionException, SSHSessionDeadException, TimeoutException
+from .logger import DTSLOG
+from .utils import GREEN, RED
+
+"""
+The module handles ssh sessions to TG and SUT.
+It implements the send_expect function to send commands and get output data.
+"""
+
+
+class SSHPexpect:
+ username: str
+ password: str
+ node: str
+ logger: DTSLOG
+ magic_prompt: str
+
+ def __init__(
+ self,
+ node: str,
+ username: str,
+ password: Optional[str],
+ logger: DTSLOG,
+ ):
+ self.magic_prompt = "MAGIC PROMPT"
+ self.logger = logger
+
+ self.node = node
+ self.username = username
+ self.password = password or ""
+ self.logger.info(f"ssh {self.username}@{self.node}")
+
+ self._connect_host()
+
+ def _connect_host(self) -> None:
+ """
+ Create connection to assigned node.
+ """
+ retry_times = 10
+ try:
+ if ":" in self.node:
+ while retry_times:
+ self.ip = self.node.split(":")[0]
+ self.port = int(self.node.split(":")[1])
+ self.session = pxssh.pxssh(encoding="utf-8")
+ try:
+ self.session.login(
+ self.ip,
+ self.username,
+ self.password,
+ original_prompt="[$#>]",
+ port=self.port,
+ login_timeout=20,
+ password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
+ )
+ except Exception as e:
+ print(e)
+ time.sleep(2)
+ retry_times -= 1
+ print("retry %d times connecting..." % (10 - retry_times))
+ else:
+ break
+ else:
+ raise Exception("connect to %s:%s failed" % (self.ip, self.port))
+ else:
+ self.session = pxssh.pxssh(encoding="utf-8")
+ self.session.login(
+ self.node,
+ self.username,
+ self.password,
+ original_prompt="[$#>]",
+ password_regex=r"(?i)(?:password:)|(?:passphrase for key)|(?i)(password for .+:)",
+ )
+ self.logger.info(f"Connection to {self.node} succeeded")
+ self.send_expect("stty -echo", "#")
+ self.send_expect("stty columns 1000", "#")
+ except Exception as e:
+ print(RED(str(e)))
+ if getattr(self, "port", None):
+ suggestion = (
+ "\nSuggession: Check if the firewall on [ %s ] " % self.ip
+ + "is stopped\n"
+ )
+ print(GREEN(suggestion))
+
+ raise SSHConnectionException(self.node)
+
+ def send_expect_base(self, command: str, expected: str, timeout: float) -> str:
+ self.clean_session()
+ self.session.PROMPT = expected
+ self.__sendline(command)
+ self.__prompt(command, timeout)
+
+ before = self.get_output_before()
+ return before
+
+ def send_expect(
+ self, command: str, expected: str, timeout: float = 15, verify: bool = False
+ ) -> str | int:
+
+ try:
+ ret = self.send_expect_base(command, expected, timeout)
+ if verify:
+ ret_status = self.send_expect_base("echo $?", expected, timeout)
+ if not int(ret_status):
+ return ret
+ else:
+ self.logger.error("Command: %s failure!" % command)
+ self.logger.error(ret)
+ return int(ret_status)
+ else:
+ return ret
+ except Exception as e:
+ print(
+ RED(
+ "Exception happened in [%s] and output is [%s]"
+ % (command, self.get_output_before())
+ )
+ )
+ raise e
+
+ def send_command(self, command: str, timeout: float = 1) -> str:
+ try:
+ self.clean_session()
+ self.__sendline(command)
+ except Exception as e:
+ raise e
+
+ output = self.get_session_before(timeout=timeout)
+ self.session.PROMPT = self.session.UNIQUE_PROMPT
+ self.session.prompt(0.1)
+
+ return output
+
+ def clean_session(self) -> None:
+ self.get_session_before(timeout=0.01)
+
+ def get_session_before(self, timeout: float = 15) -> str:
+ """
+ Get all output before timeout
+ """
+ self.session.PROMPT = self.magic_prompt
+ try:
+ self.session.prompt(timeout)
+ except Exception as e:
+ pass
+
+ before = self.get_output_all()
+ self.__flush()
+
+ return before
+
+ def __flush(self) -> None:
+ """
+ Clear all session buffer
+ """
+ self.session.buffer = ""
+ self.session.before = ""
+
+ def __prompt(self, command: str, timeout: float) -> None:
+ if not self.session.prompt(timeout):
+ raise TimeoutException(command, self.get_output_all()) from None
+
+ def __sendline(self, command: str) -> None:
+ if not self.isalive():
+ raise SSHSessionDeadException(self.node)
+ if len(command) == 2 and command.startswith("^"):
+ self.session.sendcontrol(command[1])
+ else:
+ self.session.sendline(command)
+
+ def get_output_before(self) -> str:
+ if not self.isalive():
+ raise SSHSessionDeadException(self.node)
+ before: list[str] = self.session.before.rsplit("\r\n", 1)
+ if before[0] == "[PEXPECT]":
+ before[0] = ""
+
+ return before[0]
+
+ def get_output_all(self) -> str:
+ output: str = self.session.before
+ output.replace("[PEXPECT]", "")
+ return output
+
+ def close(self, force: bool = False) -> None:
+ if force is True:
+ self.session.close()
+ else:
+ if self.isalive():
+ self.session.logout()
+
+ def isalive(self) -> bool:
+ return self.session.isalive()
new file mode 100644
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2010-2014 Intel Corporation
+# Copyright(c) 2022 PANTHEON.tech s.r.o.
+# Copyright(c) 2022 University of New Hampshire
+#
+
+def RED(text: str) -> str:
+ return f"\u001B[31;1m{str(text)}\u001B[0m"
+
+
+def GREEN(text: str) -> str:
+ return f"\u001B[32;1m{str(text)}\u001B[0m"