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.

>>
>>
>> > 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).