From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 6310DA0032;
Wed, 14 Sep 2022 11:42:34 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 1307F4021D;
Wed, 14 Sep 2022 11:42:34 +0200 (CEST)
Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com
[209.85.208.180])
by mails.dpdk.org (Postfix) with ESMTP id 36AC240156
for <dev@dpdk.org>; Wed, 14 Sep 2022 11:42:32 +0200 (CEST)
Received: by mail-lj1-f180.google.com with SMTP id j13so2801521ljh.4
for <dev@dpdk.org>; Wed, 14 Sep 2022 02:42:32 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google;
h=in-reply-to:content-transfer-encoding:content-disposition
:mime-version:references:message-id:subject:cc:to:from:date:from:to
:cc:subject:date;
bh=uObhPNDOU57P2yEYt8A2RKUPd20hFPHJO4iu/NMxhFc=;
b=lYLoOxVJ5XSeC+rNYwm7YmdVD7U8qbcoILw5wtsjHiQzjbBqwIqQPs+iIG70/neLOG
z2QOjEajVhqLmSm+HrOOU2NSN0esdtlzqVOkurLL7pBi3h45G+CSJqmeZBwmut3x5RCU
xzEKz8EoumrS/Jnu/2Nk085EBPIMz4U/+t4AuZHQmOnFTUCdyy2+8/WqboLYOo/G39Q9
aLiBiqni1VLLKKbhp8o6g+SVF/Fy4AO0ZQpXQ7o3wTDAAGfN81qb7b0j3yr7IfLyI/BM
I7mSjnFweeiSSuzZpSx2cDaaae44B7YDdeI0lpzTUxKprH1cNoBsDP+N9OYfK6SdZy71
vGiA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20210112;
h=in-reply-to:content-transfer-encoding:content-disposition
:mime-version:references:message-id:subject:cc:to:from:date
:x-gm-message-state:from:to:cc:subject:date;
bh=uObhPNDOU57P2yEYt8A2RKUPd20hFPHJO4iu/NMxhFc=;
b=gwN3BIH3JZDOQEezNW2jGHOLo3RXBOhDsaZRjBO7ajO/J4+vk236ALy1TedhRT8Z/1
vgpft8/0254RmLjGCncjuMnY88fkQZMTMLUo9qkKdK57JhHXGULMqLBHYPWyFPBuJB8i
vCKUEhDyQsL+zX/+rpb0OlM1G7Bcx3cTZSBF4j9/jfBA9fVQ3Lu1K1QKOPdDnZUF1cLE
5warPxB8Sz+HFNkc31p0gWb31ChYEiRbCVqMH0fqCIRd5q/3JyMjBYepc/59ro8ANJK/
9tj0zCC7tgh0P9etZVK1NOGGlB5mfeqUHCQeclGK15M1nMlgWip3TThfnaOI7YoSOj6t
yoPA==
X-Gm-Message-State: ACgBeo0MdbLhwnrpqD208Sl5+fnS7RUbr+uok4CFzrkixfMljmAD3pTY
PNFruZEG5hmnu4OZfV5679teXg==
X-Google-Smtp-Source: AA6agR7CRja4YKQETPjYqzRtMH65O6Qe2QZDg5drjys0vWTy+tTXLvbYMapQMA2KprnPmopR9GC9fw==
X-Received: by 2002:a05:651c:1544:b0:25f:5036:ece2 with SMTP id
y4-20020a05651c154400b0025f5036ece2mr11348679ljp.73.1663148551568;
Wed, 14 Sep 2022 02:42:31 -0700 (PDT)
Received: from toster (87-206-67-180.dynamic.chello.pl. [87.206.67.180])
by smtp.gmail.com with ESMTPSA id
s10-20020a056512314a00b004946a1e045fsm2292780lfi.197.2022.09.14.02.42.30
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Wed, 14 Sep 2022 02:42:31 -0700 (PDT)
Date: Wed, 14 Sep 2022 11:42:22 +0200
From: Stanislaw Kardach <kda@semihalf.com>
To: Juraj =?utf-8?Q?Linke=C5=A1?= <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
Message-ID: <20220914094222.vprahs3kfhnsklyh@toster>
References: <20220728100044.1318484-1-juraj.linkes@pantheon.tech>
<20220729105550.1382664-1-juraj.linkes@pantheon.tech>
<20220729105550.1382664-5-juraj.linkes@pantheon.tech>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220729105550.1382664-5-juraj.linkes@pantheon.tech>
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
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
>
--
Best Regards,
Stanislaw Kardach