Closed mebel-christ closed 1 year ago
Ah, yeah, after "taking" a channel, you are responsible for its lifecycle. And SSHConnector
doesn't properly take care of its channel here. Your suggestion is close, but what we should really do is this:
with h.ch.take() as ch:
yield ch
this has the benefit that it also closes the channel in case of an exception. Your code would still leak the channel in such situations.
That said, looking at the code, I wonder why it isn't using the h.open_channel()
method. It looks almost identical to the code in ssh.py
but comes with a few more features/safeguards. I think this change should work:
with h.open_channel(
*cmd,
*hk_disable,
*multiplexing,
*["-p", str(self.port)],
*[arg for opt in self.ssh_config for arg in ["-o", opt]],
f"{self.username}@{self.hostname}",
) as ch:
yield ch
Ah, great solution, I totally missed that. I just tried the open_channel
method and it seems to integrate very well.
Additionally a quick question about this: https://github.com/Rahix/tbot/blob/6fbc41003b159286b983107ea60477918fda6470/tbot/machine/connector/ssh.py#L166-L170
The "modern" solution would be this right?
with tbot.ctx() as cx:
if self.host is None:
self.host = cx.request(tbot.role.LabHost)
h = cx.enter_context(self.host.clone())
Is it possible to modernize this? I would supply both changes in one PR since they change the same function or should this remain unchanged?
Should be tbot.role.LocalHost
, but I think you are right. I'm always a bit scared about anything involving the old tbot.selectable
interface as I don't know how many people still rely on it in some way and it is super fragile... But we can't support legacy stuff forever so if this does cause a breakage, people either need to speak up or update to the new interface ^^ So I'd say go ahead and do this change, too :)
Just curious, why would you go with LocalHost instead of LabHost? By default it should be the same anyway and if one uses a remote Lab it would automagically work as well right? Or is this to fragile?
Just curious, why would you go with LocalHost instead of LabHost? By default it should be the same anyway and if one uses a remote Lab it would automagically work as well right? Or is this to fragile?
Okay, this entire situation is a big mess. Actually, the from_context()
constructor is supposed to pass along the lab-host in 99% of cases so your suggested automagically correct host is exactly what I had in mind there. The local-host here is just the fall back for sitations where a machine is instantiated outside of the context and as such a lab-host may not exist or is not wanted. However, I believe this "choose lab-host" codepath is currently broken - still need to verify.
Moving a bit further back, this plays into the bigger issue of seamless integration between machines and the context. This is one of the warts that still need to be solved, the decision of what host an ssh connection should originate from. I think, in the end, the solution is to give the SSHConnector
a new configuration attribute which holds a role to use, defaulting to LabHost + magic (see f9f2fe8a2512). Still don't feel entirely comfortable with it yet, though, as I first want to more clearly define the interaction of machines with the context...
Also see #92.
Even after #88 there are still some file-descriptors that appear to remain open (even though they should be closed) when I connect to a DUT with the SSHConnector. I use this setup:
config/eval.py
:tc/test_fd.py
:I execute the setup without any additional configuration like this:
newbot -c config.eval tc.test_fd.test_fds
This is the output:This time it seems that some processes remain active, after the connection is closed. htop shows this:![tbot](https://user-images.githubusercontent.com/71810564/231217686-731371b3-d0bb-45a3-b322-277c80eb3388.png)
@Rahix can you confirm this happens as well on your end?
Proposed solution
Change the yield step in the current _connect() method of SSHConnector like this:
This properly closes all remaining channels / processes and results in the following test run: