[v4,5/9] dts: add ssh connection extension
Checks
Commit Message
The class adds logging and history records to existing pexpect methods.
Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
dts/framework/ssh_connection.py | 70 +++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 dts/framework/ssh_connection.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 5/9] dts: add ssh connection extension
>
> The class adds logging and history records to existing pexpect methods.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> dts/framework/ssh_connection.py | 70
> +++++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
> create mode 100644 dts/framework/ssh_connection.py
>
Reviewed-by: Lijuan Tu <lijuan.tu@intel.com>
Thanks,
Lijuan
On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> The class adds logging and history records to existing pexpect methods.
>
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> dts/framework/ssh_connection.py | 70 +++++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
> create mode 100644 dts/framework/ssh_connection.py
>
One comment inline below.
/Bruce
> diff --git a/dts/framework/ssh_connection.py b/dts/framework/ssh_connection.py
> new file mode 100644
> index 0000000000..bbf7c8ef01
> --- /dev/null
> +++ b/dts/framework/ssh_connection.py
> @@ -0,0 +1,70 @@
> +# 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 dataclasses
> +from typing import Any, Optional
> +
> +from .logger import DTSLOG
> +from .ssh_pexpect import SSHPexpect
> +
> +
> +@dataclasses.dataclass(slots=True, frozen=True)
> +class HistoryRecord:
> + command: str
> + name: str
> + output: str | int
> +
> +
> +class SSHConnection(object):
> + """
> + Module for create session to node.
> + """
> +
> + name: str
> + history: list[HistoryRecord]
> + logger: DTSLOG
> + session: SSHPexpect | Any
> +
> + def __init__(
> + self,
> + node: str,
> + session_name: str,
> + logger: DTSLOG,
> + username: str,
> + password: Optional[str] = "",
> + ):
> + self.session = SSHPexpect(node, username, password, logger)
> + self.name = session_name
> + self.history = []
> + self.logger = logger
> +
> + def send_expect(
> + self, cmds: str, expected: str, timeout: float = 15, verify: bool = False
> + ) -> str | int:
> + self.logger.info(cmds)
> + out = self.session.send_expect(cmds, expected, timeout, verify)
> + if isinstance(out, str):
> + self.logger.debug(out.replace(cmds, ""))
> + self.history.append(HistoryRecord(command=cmds, name=self.name, output=out))
> + return out
> +
> + def send_command(self, cmds: str, timeout: float = 1) -> str:
> + self.logger.info(cmds)
> + out = self.session.send_command(cmds, timeout)
> + self.logger.debug(out.replace(cmds, ""))
> + self.history.append(HistoryRecord(command=cmds, name=self.name, output=out))
> + return out
> +
> + def get_session_before(self, timeout: float = 15) -> str:
> + out = self.session.get_session_before(timeout)
> + self.logger.debug(out)
> + return out
> +
> + def close(self, force: bool = False) -> None:
> + if getattr(self, "logger", None):
> + self.logger.logger_exit()
Two questions on this function:
* Is the getattr() check not equivalent to "if self.logger:"?
* Why the check for a non-none logger in this function, when other
functions above always seem to call the logger directly without checking?
> +
> + self.session.close(force)
> --
> 2.30.2
>
>
> On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > The class adds logging and history records to existing pexpect methods.
> >
> > Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> > dts/framework/ssh_connection.py | 70 +++++++++++++++++++++++++++++++++
> > 1 file changed, 70 insertions(+)
> > create mode 100644 dts/framework/ssh_connection.py
> >
> One comment inline below.
> /Bruce
Two questions on this function:
* Is the getattr() check not equivalent to "if self.logger:"?
It is. I missed it when looking over this code. I know that this close
function can run in a context where it loses the ability to make system
calls (an exit hook), but that doesn't matter for this as far as I know.
> * Why the check for a non-none logger in this function, when other
functions above always seem to call the logger directly without checking?
"close" can be called before "init_log" if the program crashes early
enough, so this is avoiding calling a function on a null object. No other
function can have that issue because by the time control is returned to the
user the logger is properly initalized. This is especially important
because an early failure in the community lab will only be able to use logs
to figure out what happened.
>
> > +
> > + self.session.close(force)
> > --
> 2.30.2
> >
On Tue, Sep 13, 2022 at 01:32:33PM -0400, Owen Hilyard wrote:
> On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > The class adds logging and history records to existing pexpect
> methods.
> >
> > Signed-off-by: Owen Hilyard <[1]ohilyard@iol.unh.edu>
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> > dts/framework/ssh_connection.py | 70
> +++++++++++++++++++++++++++++++++
> > 1 file changed, 70 insertions(+)
> > create mode 100644 dts/framework/ssh_connection.py
> >
>
> One comment inline below.
>
> /Bruce
>
> Two questions on this function:
>
> * Is the getattr() check not equivalent to "if self.logger:"?
>
> It is. I missed it when looking over this code. I know that this close
> function can run in a context where it loses the ability to make system
> calls (an exit hook), but that doesn't matter for this as far as I
> know.
>
> * Why the check for a non-none logger in this function, when other
>
> functions above always seem to call the logger directly without
> checking?
>
> "close" can be called before "init_log" if the program crashes early
> enough, so this is avoiding calling a function on a null object. No
> other function can have that issue because by the time control is
> returned to the user the logger is properly initalized. This is
> especially important because an early failure in the community lab will
> only be able to use logs to figure out what happened.
>
I'm a little confused by that explanation - can you perhaps clarify? This
close function in part of an class, and the logger member is assigned its value
in the constructor for that class, so how is it possible to call
obj.close() before obj has been constructed?
>
> On Wed, Sep 14, 2022 at 3:46 AM Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> > On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > > The class adds logging and history records to existing pexpect
> > methods.
> > >
> > > Signed-off-by: Owen Hilyard <[1]ohilyard@iol.unh.edu>
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > ---
> > > dts/framework/ssh_connection.py | 70
> > +++++++++++++++++++++++++++++++++
> > > 1 file changed, 70 insertions(+)
> > > create mode 100644 dts/framework/ssh_connection.py
> > >
> >
> > One comment inline below.
> >
> > /Bruce
> >
> > Two questions on this function:
> >
> > * Is the getattr() check not equivalent to "if self.logger:"?
> >
> > It is. I missed it when looking over this code. I know that this close
> > function can run in a context where it loses the ability to make
> system
> > calls (an exit hook), but that doesn't matter for this as far as I
> > know.
> >
> > * Why the check for a non-none logger in this function, when other
> >
> > functions above always seem to call the logger directly without
> > checking?
> >
> > "close" can be called before "init_log" if the program crashes early
> > enough, so this is avoiding calling a function on a null object. No
> > other function can have that issue because by the time control is
> > returned to the user the logger is properly initalized. This is
> > especially important because an early failure in the community lab
> will
> > only be able to use logs to figure out what happened.
> >
> I'm a little confused by that explanation - can you perhaps clarify? This
> close function in part of an class, and the logger member is assigned its
> value
> in the constructor for that class, so how is it possible to call
> obj.close() before obj has been constructed?
Due to how we are forced to add the hooks to flush logger buffers to disk
before shutdown, even in the case of failures or SIGTERM, close can run
WHILE the constructor is in the middle of executing. All of the resources
except for the logger can be freed by python, but the logger requires
special handling, so we check if it is null and then flush the buffers to
disk if it is not. The actual logger object is only assigned to self.logger
after it is completely initalized, so if it's not null, then it is in a
good state and can be safely flushed. Here's a sequence of events that
would lead to that check being needed:
1. Start constructing logging handle
2. Execute first line of the constructor
3. SIGTERM
self.logger == None, since the entire world stops and moves to the cleanup
functions registered with the interpreter. If we don't do this check, then
the cleanup context crashes in this case.
On Wed, Sep 14, 2022 at 08:02:37AM -0400, Owen Hilyard wrote:
> On Wed, Sep 14, 2022 at 3:46 AM Bruce Richardson
> <[1]bruce.richardson@intel.com> wrote:
> > On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > > The class adds logging and history records to existing
> pexpect
> > methods.
> > >
> > > Signed-off-by: Owen Hilyard <[1][2]ohilyard@iol.unh.edu>
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > ---
> > > dts/framework/ssh_connection.py | 70
> > +++++++++++++++++++++++++++++++++
> > > 1 file changed, 70 insertions(+)
> > > create mode 100644 dts/framework/ssh_connection.py
> > >
> >
> > One comment inline below.
> >
> > /Bruce
> >
> > Two questions on this function:
> >
> > * Is the getattr() check not equivalent to "if self.logger:"?
> >
> > It is. I missed it when looking over this code. I know that
> this close
> > function can run in a context where it loses the ability to
> make system
> > calls (an exit hook), but that doesn't matter for this as far
> as I
> > know.
> >
> > * Why the check for a non-none logger in this function, when
> other
> >
> > functions above always seem to call the logger directly
> without
> > checking?
> >
> > "close" can be called before "init_log" if the program crashes
> early
> > enough, so this is avoiding calling a function on a null
> object. No
> > other function can have that issue because by the time control
> is
> > returned to the user the logger is properly initalized. This is
> > especially important because an early failure in the community
> lab will
> > only be able to use logs to figure out what happened.
> >
> I'm a little confused by that explanation - can you perhaps clarify?
> This
> close function in part of an class, and the logger member is
> assigned its value
> in the constructor for that class, so how is it possible to call
> obj.close() before obj has been constructed?
>
> Due to how we are forced to add the hooks to flush logger buffers to
> disk before shutdown, even in the case of failures or SIGTERM, close
> can run WHILE the constructor is in the middle of executing. All of the
> resources except for the logger can be freed by python, but the logger
> requires special handling, so we check if it is null and then flush the
> buffers to disk if it is not. The actual logger object is only assigned
> to self.logger after it is completely initalized, so if it's not null,
> then it is in a good state and can be safely flushed. Here's a sequence
> of events that would lead to that check being needed:
> 1. Start constructing logging handle
> 2. Execute first line of the constructor
> 3. SIGTERM
> self.logger == None, since the entire world stops and moves to the
> cleanup functions registered with the interpreter. If we don't do this
> check, then the cleanup context crashes in this case.
>
Ack.
Thanks for the clear explanation, having the check now makes sense.
new file mode 100644
@@ -0,0 +1,70 @@
+# 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 dataclasses
+from typing import Any, Optional
+
+from .logger import DTSLOG
+from .ssh_pexpect import SSHPexpect
+
+
+@dataclasses.dataclass(slots=True, frozen=True)
+class HistoryRecord:
+ command: str
+ name: str
+ output: str | int
+
+
+class SSHConnection(object):
+ """
+ Module for create session to node.
+ """
+
+ name: str
+ history: list[HistoryRecord]
+ logger: DTSLOG
+ session: SSHPexpect | Any
+
+ def __init__(
+ self,
+ node: str,
+ session_name: str,
+ logger: DTSLOG,
+ username: str,
+ password: Optional[str] = "",
+ ):
+ self.session = SSHPexpect(node, username, password, logger)
+ self.name = session_name
+ self.history = []
+ self.logger = logger
+
+ def send_expect(
+ self, cmds: str, expected: str, timeout: float = 15, verify: bool = False
+ ) -> str | int:
+ self.logger.info(cmds)
+ out = self.session.send_expect(cmds, expected, timeout, verify)
+ if isinstance(out, str):
+ self.logger.debug(out.replace(cmds, ""))
+ self.history.append(HistoryRecord(command=cmds, name=self.name, output=out))
+ return out
+
+ def send_command(self, cmds: str, timeout: float = 1) -> str:
+ self.logger.info(cmds)
+ out = self.session.send_command(cmds, timeout)
+ self.logger.debug(out.replace(cmds, ""))
+ self.history.append(HistoryRecord(command=cmds, name=self.name, output=out))
+ return out
+
+ def get_session_before(self, timeout: float = 15) -> str:
+ out = self.session.get_session_before(timeout)
+ self.logger.debug(out)
+ return out
+
+ def close(self, force: bool = False) -> None:
+ if getattr(self, "logger", None):
+ self.logger.logger_exit()
+
+ self.session.close(force)