verdi calcjob gotocomputer: Add proxy command

[mbercx]

Currently the verdi calcjob gocomputer command does not work in case
the connection to the remote computer is via a proxy. Here we add the
ProxyCommand option to the parameters for the ssh command.

[mbercx]

I basically got sick enough of this not working on Piz Daint that I finally got around to fixing it. 😅

[codecov]

Codecov Report

Merging #4761 (fd7e641) into develop (f3c3359) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4761      +/-   ##
===========================================
+ Coverage    79.58%   79.62%   +0.04%     
===========================================
  Files          519      519              
  Lines        37093    37095       +2     
===========================================
+ Hits         29517    29532      +15     
+ Misses        7576     7563      -13     

Flag	Coverage Δ
django	74.36% <100.00%> (+0.04%)	⬆️
sqlalchemy	73.24% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/transports/plugins/ssh.py	79.15% <100.00%> (+0.44%)	⬆️
aiida/transports/util.py	65.63% <0.00%> (+34.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3c3359...fd7e641. Read the comment docs.

[ltalirz]

thanks @mbercx , I ran into this in #4100 (comment) as well and it's good to fix.

Just to be sure - this change will make sure to read the proxycommand from the profile as well, is that correct?

Happy to do a proper review once the tests pass

[ltalirz]

just to mention a related issue that might make sense to fix in one go as your touching this bit of the code #3311

(optional, not required of course)

[mbercx]

@ltalirz tests are still failing because of #4860, but I think I found the issue that was causing them to fail previously. It was a little subtle. In the open method of the SshTransport, the 'key_filename' and 'proxy_command' keys are pop-ed from the connection_arguments:

aiida-core/aiida/transports/plugins/ssh.py

Lines 430 to 432 in 5ab86cc

	if 'key_filename' in connection_arguments and not connection_arguments['key_filename']:
	connection_arguments.pop('key_filename')
	proxystring = connection_arguments.pop('proxy_command', None)

This makes sense, since this variable is later passed to the connect command of the SshClient, which doesn't accept e.g. proxy_command as a keyword argument. However, this also mutated the SshTransport._connect_args dictionary, removing the proxy_command. So, I decided to copy this dictionary instead.

Re #3311: Now that I've had to explore the transport code a bit, I'd be happy to work on this as well, but would fix this in a separate PR since it's not yet clear what the best solution is and although it is related to the transport code, the required changes are still quite different from those made in this PR. :)

[mbercx]

Just to be sure - this change will make sure to read the proxycommand from the profile as well, is that correct?

I'm not sure, actually. You mean the default set in the .ssh/config file? I'm still trying to figure out how and where exactly this is read. Frankly, the transport code is a little convoluted. 😅

The gotocomputer_command reads all the connection arguments from the _connect_args dictionary, which is populated based on the kwargs in the constructor of the SshTransport class. However, after doing a bit of digging, it seems that the defaults, parsed from the .ssh/config file in the parse_sshconfig method, are only obtained when the transport_option_default in the transports/cli.py module is called. Which I think is done during the creation of the options of the configure commands. So I'm assuming that when the computer is first configured, AiiDA does read these config settings from the .ssh/config file and add them to the authinfo of the computer?

[ltalirz]

thanks @mbercx , this looks good to me

[ltalirz]

Just to be sure - this change will make sure to read the proxycommand from the profile as well, is that correct?

I'm not sure, actually. You mean the default set in the .ssh/config file? I'm still trying to figure out how and where exactly this is read. Frankly, the transport code is a little convoluted. 😅

No, I meant that the gotocomputer command would now take the configuration from the database (which is indeed what you added).
The reason it also reads from the ~.ssh/config file is that the gotocomputer command directly executes ssh on the command line (it has to), which then parses the ~/.ssh/config file. This does not happen in other contexts when going through paramiko.

The gotocomputer_command reads all the connection arguments from the _connect_args dictionary, which is populated based on the kwargs in the constructor of the SshTransport class. However, after doing a bit of digging, it seems that the defaults, parsed from the .ssh/config file in the parse_sshconfig method, are only obtained when the transport_option_default in the transports/cli.py module is called. Which I think is done during the creation of the options of the configure commands. So I'm assuming that when the computer is first configured, AiiDA does read these config settings from the .ssh/config file and add them to the authinfo of the computer?

When you set up the computer, you can select whether AiiDA should parse the ssh/config or not.

[ltalirz]

I leave the merge to you.
The commit message should mention that this is a slight change in behavior: Instead of the proxycommand stored in ~.ssh/config, the gotocomputer command will now use the proxycommand stored in the database (which should anyhow work as well since it is used by AiiDA, so it's unlikely that this will cause issues).

[mbercx]

The reason it also reads from the ~.ssh/config file is that the gotocomputer command directly executes ssh on the command line (it has to), which then parses the ~/.ssh/config file. This does not happen in other contexts when going through paramiko.

But, this would only work in case the user has set the Host as the HostName, right? I usually use shorthands for the host, e.g.:

Host ela
  HostName ela.cscs.ch
  User mbercx
  IdentityFile ~/.ssh/id_rsa-daint
Host daint
  HostName daint.cscs.ch
  User mbercx
  IdentityFile ~/.ssh/id_rsa-daint
  ProxyJump ela

(Which is in line with our documentation). But AiiDA requests the "fully qualified" host name:

  -H, --hostname HOSTNAME         The fully qualified hostname of the computer
                                  (e.g. daint.cscs.ch). Use "localhost" when
                                  setting up the computer that AiiDA is
                                  running on.  [required]

So this is of course why verdi gotocomputer was failing in my case, because when you execute ssh -t daint.cscs.ch ..., my .ssh/config is not being parsed.

When you set up the computer, you can select whether AiiDA should parse the ssh/config or not.

This faces the same problem. And of course ProxyJump is ignored, since paramiko doesn't support it.

PS: Actually, after doing a bit of digging while writing this comment, my whole explanation is raised in this issue. 😅 I'll continue the discussion there!

[mbercx]

I leave the merge to you.

Thanks for the review, Leo! Will merge once #4860 is merged.

[ltalirz]

thanks, will unsubscribe from this pr
feel free to merge without further review (e.g. merging in the postgres updates)

[ramirezfranciscof]

Already approved by @ltalirz , just leaving the formal thumbs up.