On Wed, Jul 12, 2023 at 2:30 AM Juraj Linkeš 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.