From: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
To: Stanislaw Kardach <kda@semihalf.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"ronan.randles@intel.com" <ronan.randles@intel.com>,
"Honnappa.Nagarahalli@arm.com" <Honnappa.Nagarahalli@arm.com>,
"ohilyard@iol.unh.edu" <ohilyard@iol.unh.edu>,
"lijuan.tu@intel.com" <lijuan.tu@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v4 4/9] dts: add ssh pexpect library
Date: Thu, 22 Sep 2022 09:41:40 +0000 [thread overview]
Message-ID: <7f360965ca2c47b29e2413e3207234db@pantheon.tech> (raw)
In-Reply-To: <20220914094222.vprahs3kfhnsklyh@toster>
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
next prev parent reply other threads:[~2022-09-22 9:41 UTC|newest]
Thread overview: 105+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 12:14 [PATCH v1 0/8] dts: ssh connection to a node Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 1/8] dts: add ssh pexpect library Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 2/8] dts: add locks for parallel node connections Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 3/8] dts: add ssh connection extension Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 4/8] dts: add basic logging facility Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 5/8] dts: add Node base class Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 6/8] dts: add config parser module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 7/8] dts: add dts runtime workflow module Juraj Linkeš
2022-06-22 12:14 ` [PATCH v1 8/8] dts: add main script for running dts Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 0/8] ssh connection to a node Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 1/8] dts: add basic logging facility Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 2/8] dts: add ssh pexpect library Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 3/8] dts: add locks for parallel node connections Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 4/8] dts: add ssh connection extension Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 5/8] dts: add config parser module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 6/8] dts: add Node base class Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 7/8] dts: add dts workflow module Juraj Linkeš
2022-07-11 14:51 ` [PATCH v2 8/8] dts: add dts executable script Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 1/9] dts: add project tools config Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 2/9] dts: add developer tools Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 3/9] dts: add basic logging facility Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 5/9] dts: add ssh connection extension Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 6/9] dts: add config parser module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 7/9] dts: add Node base class Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 8/9] dts: add dts workflow module Juraj Linkeš
2022-07-28 10:00 ` [PATCH v3 9/9] dts: add dts executable script Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 0/9] dts: ssh connection to a node Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 1/9] dts: add project tools config Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:16 ` Bruce Richardson
2022-09-09 13:38 ` Juraj Linkeš
2022-09-09 13:52 ` Bruce Richardson
2022-09-09 14:13 ` Juraj Linkeš
2022-09-12 14:06 ` Owen Hilyard
2022-09-12 15:15 ` Bruce Richardson
2022-09-13 12:08 ` Juraj Linkeš
2022-09-13 14:18 ` Bruce Richardson
2022-09-13 19:03 ` Honnappa Nagarahalli
2022-09-13 19:19 ` Honnappa Nagarahalli
2022-09-14 9:37 ` Thomas Monjalon
2022-09-14 12:55 ` Juraj Linkeš
2022-09-14 13:11 ` Bruce Richardson
2022-09-14 14:28 ` Thomas Monjalon
2022-09-21 10:49 ` Juraj Linkeš
2022-09-13 19:11 ` Honnappa Nagarahalli
2022-07-29 10:55 ` [PATCH v4 2/9] dts: add developer tools Juraj Linkeš
2022-08-10 6:30 ` Tu, Lijuan
2022-09-07 16:37 ` Bruce Richardson
2022-09-13 12:38 ` Juraj Linkeš
2022-09-13 20:38 ` Honnappa Nagarahalli
2022-09-14 7:37 ` Bruce Richardson
2022-09-14 12:45 ` Juraj Linkeš
2022-09-14 13:13 ` Bruce Richardson
2022-09-14 14:26 ` Thomas Monjalon
2022-09-14 19:08 ` Honnappa Nagarahalli
2022-09-20 12:14 ` Juraj Linkeš
2022-09-20 12:22 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 3/9] dts: add basic logging facility Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 8:31 ` Bruce Richardson
2022-09-13 12:52 ` Juraj Linkeš
2022-09-13 23:31 ` Honnappa Nagarahalli
2022-09-14 12:51 ` Juraj Linkeš
2022-07-29 10:55 ` [PATCH v4 4/9] dts: add ssh pexpect library Juraj Linkeš
2022-08-10 6:31 ` Tu, Lijuan
2022-09-08 9:53 ` Bruce Richardson
2022-09-13 13:36 ` Juraj Linkeš
2022-09-13 14:23 ` Bruce Richardson
2022-09-13 14:59 ` Stanislaw Kardach
2022-09-13 17:23 ` Owen Hilyard
2022-09-14 0:03 ` Honnappa Nagarahalli
2022-09-14 7:42 ` Bruce Richardson
2022-09-14 7:58 ` Stanislaw Kardach
2022-09-14 19:57 ` Honnappa Nagarahalli
2022-09-19 14:21 ` Owen Hilyard
2022-09-20 17:54 ` Honnappa Nagarahalli
2022-09-21 1:01 ` Tu, Lijuan
2022-09-21 5:37 ` Jerin Jacob
2022-09-22 9:03 ` Juraj Linkeš
2022-09-14 9:42 ` Stanislaw Kardach
2022-09-22 9:41 ` Juraj Linkeš [this message]
2022-09-22 14:32 ` Stanislaw Kardach
2022-09-23 7:22 ` Juraj Linkeš
2022-09-23 8:15 ` Bruce Richardson
2022-09-23 10:18 ` Stanislaw Kardach
2022-07-29 10:55 ` [PATCH v4 5/9] dts: add ssh connection extension Juraj Linkeš
2022-08-10 6:32 ` Tu, Lijuan
2022-09-13 17:04 ` Bruce Richardson
2022-09-13 17:32 ` Owen Hilyard
2022-09-14 7:46 ` Bruce Richardson
2022-09-14 12:02 ` Owen Hilyard
2022-09-14 13:15 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 6/9] dts: add config parser module Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-09-13 17:19 ` Bruce Richardson
2022-09-13 17:47 ` Owen Hilyard
2022-09-14 7:48 ` Bruce Richardson
2022-07-29 10:55 ` [PATCH v4 7/9] dts: add Node base class Juraj Linkeš
2022-08-10 6:33 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 8/9] dts: add dts workflow module Juraj Linkeš
2022-08-10 6:34 ` Tu, Lijuan
2022-07-29 10:55 ` [PATCH v4 9/9] dts: add dts executable script Juraj Linkeš
2022-08-10 6:35 ` Tu, Lijuan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f360965ca2c47b29e2413e3207234db@pantheon.tech \
--to=juraj.linkes@pantheon.tech \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=kda@semihalf.com \
--cc=lijuan.tu@intel.com \
--cc=ohilyard@iol.unh.edu \
--cc=ronan.randles@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).