Repository navigation
Timeout is ambiguous in SSHHook and SSHOperator #16364
Description
In SSHHook the timeout argument of the constructor is used to set a connection timeout. This is fine.
But in SSHOperator the timeout argument of the constructor is used for both the timeout of the SSHHook and the timeout of the command itself (see paramiko's ssh client exec_command use of the timeout parameter). This ambiguous use of the same parameter is very dirty.
I see two ways to clean the behaviour:
- Let the SSHHook constructor be the only way to handle the connection timeout (thus, if one wants a specific timeout they should explicitely build a hook to be passed to the operator using the operator's constructor).
- Split the timeout argument in SSHOperator into two arguments conn_timeout and cmd_timeout for example.
The choice between 1 and 2 depends on how frequently people are supposed to want to change the connection timeout. If it is something very frequent. then go for 2. if not go for 1.
BR and thanks for the code!
Activity
uranusjr commentedon Jun 11, 2021
I feel Option 2 is clearer regardless, since it is still quite confusing if
SSHHook(timeout=...)andSSHOperator(timeout=...)mean two different things. It’s probably better to be explicit about what the timeout is for:conn_timeoutand pass it toSSHCHook(timeout).cmd_timeoutfor the command line timeout.SSHOperator(timeout); if it’s passed, setconn_timeoutandcmd_timeouttotimeoutif they areNone.TypeErroriftimeoutis all three are passed.SSHOperator(timeout)in favour ofconn_timeoutandcmd_timeout.potiuk commentedon Jun 11, 2021
Agree and I think point 4. should not be optional. We should deprecate it.
One more thing - I noticed that "timeout" (among others) options for sshHook which also should be updated:
I though also that we could convert the SSHHook to be configurable via UI changing the
extra__ssh__names similarly to #15159 (and follow up fix #16388). However, unfortunately this is a bit more complex, there is a small caveat with this - we have SFTPHook that derives from SSHHook and this means that we could not immediately reuse the logic of the fields defined there (extras__ssh__cmd_timeoutvs.extras__sftp__cmd_timeout) .I think it's not worth for this change, but we should probably reimplement the way the extra UI fields are defined to support inheritance better.
potiuk commentedon Jun 11, 2021
@ashb See ^^ I think you mentioned you had some idea that we should get rid of the
__extra__CONN_TYPE__ prefix? correct? It indeed makes things complex for inheritance case and we have about half-half connections now - the UI ones are implementing extra__CONN_TYPE prefix but there are a number of them that have "bare" extras.ashb commentedon Jun 11, 2021
@potiuk Yeah, I always just found it annoying to have the prefix on it, mostly when setting the connection via a URI. Are you saying this issue means we should keep the prefix, or you would like to remove it too?
potiuk commentedon Jun 11, 2021
SSH/SFTP does not have the prefix (it does not support UI fields for extras). Since long term remove the prefix (I agree it's annoying)- we should not touch it here then and keep it without the prefix and implement removal of the prefix later (and then we can add UI support for SSH/SFTP).
So summarizing @NBardelot : just don't forget to add the "cmd_timeout" and "conn_timeout" to extras (without prefix).
ashb commentedon Jun 11, 2021
Do we need
cmd_timeouton the Hook? Isn't that more a property that only makes sense for the operator because it changes from task-to-task, and isn't anything to do with the Connection.potiuk commentedon Jun 11, 2021
I think both should be in Hook/connection - acting as default value for all SSH commands. It could be done via dag defaults as well, but both "cmd_timeout" and "timeout" are a bit too generic IMHO for Dag defaults, so it would have to be "ssh_command_timeout" or something like tha. In this case, it rather belongs to connection.
fatmumuhomer commentedon Jul 18, 2021
@potiuk or @ashb - Can someone please assign to me if no one else is already working on it? Thanks!
potiuk commentedon Jul 18, 2021
Please!
fatmumuhomer commentedon Jul 19, 2021
Just to be clear on the preferred usage going forward:
It doesn't seem to make sense to accept a cmd_timeout in the hook, so I am planning to do the following:
uranusjr commentedon Jul 19, 2021
Sounds reasonable to me. You’ll also need to keep the
timeoutarguments on both the operator and hook for backward compatibility (also with a deprecation warning).fatmumuhomer commentedon Jul 19, 2021
Thanks, @uranusjr. For the deprecation warning, is it fair to warn that it will be removed in a future version without specifying the exact one?
uranusjr commentedon Jul 19, 2021
Yes; you can search for
DeprecationWarningto get some inspiratrions how to format the message.fatmumuhomer commentedon Jul 26, 2021
@uranusjr, @potiuk I created a pull request for this change - #17236
Can someone take a look when you have time? Also, it says I need a maintainer to approve running workflows since I am a first time contributor.
Thank you.
potiuk commentedon Dec 10, 2022
@jayam26 Not sure what was your goal here. If you see an issue with using the timeouts, commenting here with such statement. makes very little sense if you want help - I suggsest you start a discussion with providing some more things you tried and failed and ask what you want help with. But you need to start with explaining the details (and don't do it in the closed issue - you might refer to it if you want to refer to somethign discussed here.