SSHHook get_conn() does not re-use client #10874
Comments
|
@kaxil Please assign this to me. |
|
I read the issue and - unless I misunderstood it - I think we should close both the issue and PR. @baolsen Why would you like to run The right way of reusing a connection (even if you want to compose or subclass something) is to reuse HOOK not operator. You can easily create single Hook and pass it around and use it many times while keeping the connection open, but you should not try to do it with the operator - pretty much always when you want to write some behaviour where you want to reuse connection for multiple API calls etc, the hook reuse is right not the operator. You should always in this case write a custom operator. You can even subclass existing operator, if this operator has some reusable methods, but then you should never call super execute method multiple times. Execute is not designed to be called more than once. I am tempted to close it straight away, but this a very old issue @kaxil and @eladkal reacted to it - without pointing that out - maybe I do understand it wrongly ? Can someone shed some light on it :) |
The problem is hook = SSHHook(...)
client = hook.get_conn()
# Does not reuse! A brand new connection is established.
# Plus the previous connection is now dangling.
client = hook.get_conn()But I don’t think there’s a good quick solution to the problem; |
|
Thank you all for the feedback. My use case is to split and zip some files on a remote server if those files are above a specific size. To do this, I do the following over SSH:
I put them in a single task using CustomSSHOperator which inherits SSHOperator. I want my task to be:
In my environment connections are expensive (for local and remote), complex and error prone. But SSHOperator does not allow multiple commands to be run over 1 connection even when subclassed. I could have subclassed SSHOperator and copied all of the "boiler plate" client code. So I ended up for each command, changing the self.command and calling super.execute() as it was simplest. One option could be to refactor SSHOperator to isolate the "run an SSH command" part which can be reused by subclasses. But we seem not decided yet whether the hook is a better place to do this. If so I think the changes could be more extensive and impactful. |
This makes total sense to me, but I believe we still need to rewrite the hook a bit to make this happen. The operator is backend by the hook, so it can’t do anything the hook does not do; in order to be able to run multiple commands with the same connection, the hook needs to be able to return that connection for reuse in the first place, and currently the hook does not do that. And that change is not trivial, is what I was trying to say. |
|
| One option could be to refactor SSHOperator to isolate the "run an SSH command" part which can be reused by subclasses. Oh yeah Makes total sense. And (see below) it can be easily fixed to provide client caching in Hook Yeah. Because SSHHook pretty much does not have its own methods so it was never a problem, I guess. It delegates everything to SSHClient. It is a Hook that on it's own is not even a thin wrapper around the SSHClient but delegates all the work to it. I think quick fixing it and caching the client should be two lines code (it happens in other operators): |
|
That would also mean if self.client:
return clientThis would no longer work: hook = SSHHook(...)
with hook.get_conn() as client:
...
with hook.get_conn() as client:
# Will fail! The client was already closed, and calling __enter__ again does not reopen it.So it’s breaking one usage to accomodate another, which feels wrong to me. |
|
Proposals? |
|
I can put together a PR, to refactor SSHOperator to isolate the "run an SSH command" part |
|
PR is up :) Observations and suggestions welcome. |
Apache Airflow version: 1.10.8
Environment:
uname -a):Linux 3.10.0-957.el7.x86_64 #1 SMP Thu Oct 4 20:48:51 UTC 2018 x86_64 x86_64 x86_64 GNU/LinuxWhat happened:
Sub-classing the SSHOperator and calling its execute repeatedly will create a new SSH connection each time, to run the command.
Not sure if this is a bug or an enhancement / feature. I can re-log as a feature request if needed.
What you expected to happen:
SSH client / connection should be re-used if it was already established.
How to reproduce it:
Sub-class the SSHOperator.
In your sub class execute method, call super().execute() a few times.
Observe in the logs how an SSH Connection is created each time.
Anything else we need to know:
The SSHHook.get_conn() method creates a new Paramiko SSH client each time. Despite storing the client on self.client before returning, the hook get_conn() method does not actually use the self.client next time. A new connection is therefore created.
I think this is because the SSH Operator uses a context manager to operate on the Paramiko client, so the Hook needs to create a new client if a previous context manager had closed the last one.
Fixing this would mean changing the SSH Operator execute() to not use the ssh_hook.get_conn() as a context manager since this will open and close the session each time. Perhaps the conn can be closed with the operator's post_execute method rather than in the execute.
Example logs
The text was updated successfully, but these errors were encountered: