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