Re: [PATCH v1 1/2] dts: add smoke tests

[Jeremy Spewock <jspewock@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 13221 bytes --]

On Wed, Jul 12, 2023 at 2:30 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:

> I think we're basically there, just one more point that needs to be
> addressed - the send_command_no_output method.
>
> >> >> > diff --git a/dts/framework/config/conf_yaml_schema.json
> b/dts/framework/config/conf_yaml_schema.json
> >> >> > index ca2d4a1e..3f7c301a 100644
> >> >> > --- a/dts/framework/config/conf_yaml_schema.json
> >> >> > +++ b/dts/framework/config/conf_yaml_schema.json
> >> >> > @@ -6,6 +6,76 @@
> >> >> >        "type": "string",
> >> >> >        "description": "A unique identifier for a node"
> >> >> >      },
> >> >> > +    "NIC": {
> >> >> > +      "type": "string",
> >> >> > +      "enum": [
> >> >> > +        "ALL",
> >> >> > +        "ConnectX3_MT4103",
> >> >> > +        "ConnectX4_LX_MT4117",
> >> >> > +        "ConnectX4_MT4115",
> >> >> > +        "ConnectX5_MT4119",
> >> >> > +        "ConnectX5_MT4121",
> >> >> > +        "I40E_10G-10G_BASE_T_BC",
> >> >> > +        "I40E_10G-10G_BASE_T_X722",
> >> >> > +        "I40E_10G-SFP_X722",
> >> >> > +        "I40E_10G-SFP_XL710",
> >> >> > +        "I40E_10G-X722_A0",
> >> >> > +        "I40E_1G-1G_BASE_T_X722",
> >> >> > +        "I40E_25G-25G_SFP28",
> >> >> > +        "I40E_40G-QSFP_A",
> >> >> > +        "I40E_40G-QSFP_B",
> >> >> > +        "IAVF-ADAPTIVE_VF",
> >> >> > +        "IAVF-VF",
> >> >> > +        "IAVF_10G-X722_VF",
> >> >> > +        "ICE_100G-E810C_QSFP",
> >> >> > +        "ICE_25G-E810C_SFP",
> >> >> > +        "ICE_25G-E810_XXV_SFP",
> >> >> > +        "IGB-I350_VF",
> >> >> > +        "IGB_1G-82540EM",
> >> >> > +        "IGB_1G-82545EM_COPPER",
> >> >> > +        "IGB_1G-82571EB_COPPER",
> >> >> > +        "IGB_1G-82574L",
> >> >> > +        "IGB_1G-82576",
> >> >> > +        "IGB_1G-82576_QUAD_COPPER",
> >> >> > +        "IGB_1G-82576_QUAD_COPPER_ET2",
> >> >> > +        "IGB_1G-82580_COPPER",
> >> >> > +        "IGB_1G-I210_COPPER",
> >> >> > +        "IGB_1G-I350_COPPER",
> >> >> > +        "IGB_1G-I354_SGMII",
> >> >> > +        "IGB_1G-PCH_LPTLP_I218_LM",
> >> >> > +        "IGB_1G-PCH_LPTLP_I218_V",
> >> >> > +        "IGB_1G-PCH_LPT_I217_LM",
> >> >> > +        "IGB_1G-PCH_LPT_I217_V",
> >> >> > +        "IGB_2.5G-I354_BACKPLANE_2_5GBPS",
> >> >> > +        "IGC-I225_LM",
> >> >> > +        "IGC-I226_LM",
> >> >> > +        "IXGBE_10G-82599_SFP",
> >> >> > +        "IXGBE_10G-82599_SFP_SF_QP",
> >> >> > +        "IXGBE_10G-82599_T3_LOM",
> >> >> > +        "IXGBE_10G-82599_VF",
> >> >> > +        "IXGBE_10G-X540T",
> >> >> > +        "IXGBE_10G-X540_VF",
> >> >> > +        "IXGBE_10G-X550EM_A_SFP",
> >> >> > +        "IXGBE_10G-X550EM_X_10G_T",
> >> >> > +        "IXGBE_10G-X550EM_X_SFP",
> >> >> > +        "IXGBE_10G-X550EM_X_VF",
> >> >> > +        "IXGBE_10G-X550T",
> >> >> > +        "IXGBE_10G-X550_VF",
> >> >> > +        "brcm_57414",
> >> >> > +        "brcm_P2100G",
> >> >> > +        "cavium_0011",
> >> >> > +        "cavium_a034",
> >> >> > +        "cavium_a063",
> >> >> > +        "cavium_a064",
> >> >> > +        "fastlinq_ql41000",
> >> >> > +        "fastlinq_ql41000_vf",
> >> >> > +        "fastlinq_ql45000",
> >> >> > +        "fastlinq_ql45000_vf",
> >> >> > +        "hi1822",
> >> >> > +        "virtio"
> >> >> > +      ]
> >> >> > +    },
> >> >> > +
> >> >>
> >> >> All these NICs may be overkill, do we want to trim them?
> >> >>
> >> >
> >> >
> >> > I think in general that the more we have the better to make it more
> universally usable. If a NIC isn't supported by DTS anymore we could pull
> it out but I don't see a problem with maintaining a list that has all
> supported NICs even if it does end up being long.
> >> >
> >>
> >> The broader question is what does it mean that a NIC is supported in
> >> DTS? That's a question we should address in the CI/DTS call and in the
> >> meantime, we could just leave the list as is.
> >>
> >
> > I think this would be a very good thing to bring up and agree that there
> should be more discussion on it. It probably is better to leave the list
> longer in the meantime like you were saying as well.
> >
>
> I'm keeping notes on everything we need to talk about - we'll do that
> after release.
>
>
> >> >
> >> >>
> >> >> >
> >> >> >  """
> >> >> >  The package provides modules for managing remote connections to a
> remote host (node),
> >> >> > @@ -17,7 +18,14 @@
> >> >> >
> >> >> >  from .linux_session import LinuxSession
> >> >> >  from .os_session import OSSession
> >> >> > -from .remote import CommandResult, RemoteSession, SSHSession
> >> >> > +from .remote import (
> >> >> > +    CommandResult,
> >> >> > +    InteractiveRemoteSession,
> >> >> > +    InteractiveShell,
> >> >> > +    RemoteSession,
> >> >> > +    SSHSession,
> >> >> > +    TestPmdShell,
> >> >> > +)
> >> >> >
> >> >> >
> >> >> >  def create_session(
> >> >> > diff --git a/dts/framework/remote_session/os_session.py
> b/dts/framework/remote_session/os_session.py
> >> >> > index 4c48ae25..f5f53923 100644
> >> >> > --- a/dts/framework/remote_session/os_session.py
> >> >> > +++ b/dts/framework/remote_session/os_session.py
> >> >> > @@ -12,7 +12,13 @@
> >> >> >  from framework.testbed_model import LogicalCore
> >> >> >  from framework.utils import EnvVarsDict, MesonArgs
> >> >> >
> >> >> > -from .remote import CommandResult, RemoteSession,
> create_remote_session
> >> >> > +from .remote import (
> >> >> > +    CommandResult,
> >> >> > +    InteractiveRemoteSession,
> >> >> > +    RemoteSession,
> >> >> > +    create_interactive_session,
> >> >> > +    create_remote_session,
> >> >> > +)
> >> >> >
> >> >> >
> >> >> >  class OSSession(ABC):
> >> >> > @@ -26,6 +32,7 @@ class OSSession(ABC):
> >> >> >      name: str
> >> >> >      _logger: DTSLOG
> >> >> >      remote_session: RemoteSession
> >> >> > +    interactive_session: InteractiveRemoteSession
> >> >> >
> >> >> >      def __init__(
> >> >> >          self,
> >> >> > @@ -37,6 +44,7 @@ def __init__(
> >> >> >          self.name = name
> >> >> >          self._logger = logger
> >> >> >          self.remote_session = create_remote_session(node_config,
> name, logger)
> >> >> > +        self.interactive_session =
> create_interactive_session(node_config, name, logger)
> >> >>
> >> >> We may not want to create the interactive session at this point.
> This does create a connection to the node which we don't want (it is extra
> time consumed) when creating an extra session on top of the main_session
> (with Node.create_session). I think we could move this to
> OSSession.create_interactive_shell. More below.
> >> >
> >> >
> >> > I think the idea of initializing it here was what we had discussed
> before about having an open SSH session for interactive shells open in the
> background throughout the entire run. If I understand what you're saying,
> you're suggesting that we would only initialize this connection when we
> need it the first time and then leave it in the background afterwards. I
> can see how this would be more efficient if you had a run where the
> interactive session was never used, but it would add extra logic to make
> sure that the connection is only initialized once and that it doesn't
> happen every time you create an interactive shell. This is something that
> could be done, but considering that this will always be initialized with
> smoke tests, the only way you wouldn't have an interactive remote session
> created at the start is if you disable smoke tests. I think it is easier to
> just have it running in the background rather than spawn it when it's used
> and have to worry about if a connection currently exists or not.
> >>
> >> Right, with smoke tests almost always running, there may not be that
> >> much of an advantage in initializing it only when needed. On the other
> >> hand, the check could be very simple - the same thing we do with
> >> properties such as SutNode.os_name.
> >>
> >
> > I agree that it wouldn't be hard to check if it were defined, I was just
> thinking that if we were going to spend the time more often than not
> anyway, it would make sense to do it initially so that it doesn't cause a
> slow during the test suite and instead during initialization. If you
> disagree however, we could easily change this in the future and do it as
> needed as I think, in the rare case, you are right that it would be more
> efficient, but otherwise it made more sense to me to run it during the
> initialization stages of the run.
> >
>
> Yes, it fits in init (we're initializing something after all :-)), but
> performance-wise, the property approach is better. Since the
> performance consideration is basically negligible, let's leave it as
> is.
>
>
> >> >> > +
> >> >> > +    def empty_stdout_buffer(self) -> None:
> >> >>
> >> >> Same comment on ordering as above.
> >> >>
> >> >> > +        """Removes all data from the stdout buffer.
> >> >> > +
> >> >> > +        Because of the way paramiko handles read buffers, there
> is no way to effectively
> >> >> > +        remove data from, or "flush", read buffers. This method
> essentially moves our
> >> >> > +        offset on the buffer to the end and thus "removes" the
> data from the buffer.
> >> >> > +        Timeouts are thrown on read operations of paramiko pipes
> based on whether data
> >> >> > +        had been received before timeout so we assume that if we
> reach the timeout then
> >> >> > +        we are at the end of the buffer.
> >> >> > +        """
> >> >> > +        self._ssh_channel.settimeout(1)
> >> >>
> >> >> Waiting a whole second seems to be a lot. We actually may not need
> this method from the use in the code - that is we change how the app starts.
> >> >
> >> >
> >> > We will still need this code whenever you send a command and don't
> get its output. What it is doing is essentially moving the pointer for the
> output buffer to the end of the file which is needed because if you send a
> command and you don't want any output, if we don't still move past the
> output in the buffer then it will persist and bleed into when you send a
> command and do want to collect output. Having this method allows you to
> know you are starting from an empty buffer when you are getting the output
> of the commands.
> >> >
> >>
> >> Ah, I was commenting on send_command_no_output when I mentioned "this
> >> method", so I need to restate my point. We can do basically the same
> >> thing with "send_command" and the only difference I see is that we
> >> don't care about prompt in send_command_no_output. Is there a scenario
> >> where we need that?
> >
> >
> > This method was to address the situation that I had brought up a while
> back when discussing how to handle interactive applications. The scenario
> where you want to run an application but you cannot consume a newline
> character because the line you are on requires input. In the case of
> testpmd and "bash-like" applications, we can consume a newline character
> safely but you can't with every interactive environment. The example I used
> then was if you ran a script and it asked you to enter a password or a name
> for something. Consuming a newline in this case might not give you the
> prompt again but rather would end up taking in an unintended newline.
> >
>
> Ah, so there are cases where we won't get the prompt back. For now,
> these are hypothetical scenarios which si why I'm not keen on having
> this method - we may not ever need it.
>

That's fair, I can remove it for now.


>
> >>
> >>
> >> > In the case of timing however, I could cut this down to half a
> second, it just gives the chance that a command that takes longer to
> produce its output will still contaminate the buffer. If this causes a
> problem we could always increase the time in the future.
> >> >
> >>
> >> The point is to not have any set time (since that's either too slow or
> >> unreliable), if we can avoid this.
> >
> >
> > I agree that it isn't super reliable, but I think it is good to have
> even if it isn't used as often. The reason for this is because if the case
> arose where you didn't want to collect output up until a point in the
> middle of the stdout string or maybe passed in a prompt that didn't include
> all of the output provided, this offers some way to at least clear the
> buffer somehow.
> >
>
> Yea, we need to clear the buffer, I just don't like the solution. :-)
> I'd rather remove the method (send_command_no_output) and only include
> it when we actually need it. We can then think about the best solution
> (possibly tailored to the use case).
>

I definitely agree with you, it's not a super clean way to clear the buffer
and I don't really like it either but after a lot of testing reading from
the buffer was the only way I could find to clear the buffer. Doing it with
the timeout, while not the most reliable, was the best way I found in my
testing but we can explore this more after the release.

[-- Attachment #2: Type: text/html, Size: 18080 bytes --]