Tell git-annex to go through datalad-sshrun #5389
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5389 +/- ##
==========================================
- Coverage 89.59% 86.16% -3.43%
==========================================
Files 296 296
Lines 41711 45308 +3597
==========================================
+ Hits 37372 39041 +1669
- Misses 4339 6267 +1928
Continue to review full report at Codecov.
|
|
Thank you @kyleam !! I am yet to better grasp the situation, but I wonder if we force annex to disregard annex-ssh-options (by always setting GIT_SSH_COMMAND) what side effects we cause? Eg, in reproman then there would be no way forward to make data getable from ec2 with pure datalad get (instead of orchestrator which points to the identity file ATM). |
There was a problem hiding this comment.
This looks good to me and the consolidating all this logic outside annexrepo.py is very attractive.
Thank you @kyleam !! I am yet to better grasp the situation, but I wonder if we force annex to disregard annex-ssh-options (by always setting GIT_SSH_COMMAND) what side effects we cause? Eg, in reproman then there would be no way forward to make data getable from ec2 with pure datalad get (instead of orchestrator which points to the identity file ATM).
AFAICS datalad sshrun supports the config switch datalad.ssh.identityfile (by using SSHManager.get_connection() which queries it). I am not sure what other configurability is needed for reproman, but at least your given example should not be an obstacle.
Thx @kyleam !
Edit: I now see that precisely the support for that switch was the original motivation for this PR #5382
right - somehow I have missed this obvious solution! Thanks There would remain the issue of rendering git config's |
|
Thanks @yarikoptic @mih @bpoldrack for the reviews/feedback. @yarikoptic says:
Yes, true, that's a breaking change here, and I didn't give it much consideration, being a bit too focused on "fix things to make git-annex use sshrun as intended". Even without the annex-ssh-options behavior change, this is aggressive enough that I think it should be against master, and there is a semantic conflict, so I'll change the base (though with 0.14 quickly approaching, it of course doesn't really matter). As for how to deal with the change in behavior, I guess the options are:
Thoughts? Other options to consider? |
setup_package() overrides $HOME during the tests. This invalidates the default values for a good number of configuration options. One particularly problematic case is datalad.locations.sockets because tests inspect SSHManager's socket_dir, which still points to the non-test location (e.g., $HOME/.cache/datalad/sockets/), _but_ when 'datalad sshrun' is executed via git or git-annex, the test location is in effect. test_annex_ssh() is the test that should hit into the above scenario. The reason it doesn't fail is because git-annex isn't actually going through datalad-sshrun despite GIT_SSH_COMMAND being set, so everything is operating with the non-test location. That will change with the next commit. To fix this, store the defaults that are subject to change as functions and add a helper that uses these to set "default" in common_cfg.definitions. This gives setup_package() and teardown_package() an easy hook to use.
git-annex started honoring GIT_SSH and GIT_SSH_COMMAND in 6.20170321. However, since 6.20170510, it won't do so unless the caller sets GIT_ANNEX_USE_GIT_SSH to confirm that the custom SSH command supports `-n`. So, although get_git_environ_adjusted() sets GIT_SSH_COMMAND to 'datalad-ssrun' (which supports `-n`), git-annex doesn't actually use it. Update get_git_environ_adjusted() to set GIT_ANNEX_USE_GIT_SSH as well. Fixes datalad#5382.
With the last commit wiring up git-annex to go through datalad-sshrun, _set_shared_connection() is no longer relevant. git-annex ignores .annex-ssh-options when GIT_SSH_COMMAND is set. And nothing is gained by priming things with ssh_manager.get_connection(url); it's whether the connection is open that matters for it staying alive after a given process ends.
There are already some spots that call ssh_manager.get_connection(url).open() to enable SSH connection caching (e.g., Git.Repo._fetch_push_helper() and GitRepo.pull()). If they didn't, each datalad-sshrun subprocess (invoked via git) would close the connections because _it_ was the one that opened them. Add a helper that handles the common case, and use it in a few more obvious spots, including AnnexRepo.get() and AnnexRepo.copy_to(). (As of a few commits back, git-annex now goes through datalad-sshrun.) These are probably the lowest hanging fruit, but there are almost certainly other spots that would benefit from opening up a connection, and perhaps there's a better level to address this than individual methods.
kyleam
force-pushed
the
git-annex-use-git-ssh
branch
from
9d8aebe
to
bf6a773
Compare
|
sad that we have not settled down on this PR before releasing. As for approaches: it is not feasible/difficult to not set |
Would disable However, if we decide users should be able to separate datalad and annex wrt to SSH, we arguably would also need to check whether or not GIT_ANNEX_USE_GIT_SSH and GIT_SSH_COMMAND are already set and leave them untouched. |
That's pretty much where I'm landing. On top of that, I think from a developer perspective the datalad code around annex ssh handling is fairly hard to troubleshoot and reason about; adding a branch in the logic would make matters worse (whereas the current state of this PR drops much of the custom annexrepo logic). I do think it is unfortunate that |
|
just another observation: if I prescribe ControlPath etc in |
Isn't that already the current state of things with DataLad [*]? Say I have this entry in my ~/.ssh/config: datalad-get scriptset -eu
cd "$(mktemp -d /tmp/dl-ssh-XXXXXXX)"
tdir="$(pwd)"
find ~/.cache/datalad/sockets -type s
ls /tmp/dl-test-socket-* || :
datalad create a
(
cd a
echo one >one
datalad save
)
datalad clone datalad-test:"$tdir"/a b
(
cd b
datalad -l 8 get .
)
find ~/.cache/datalad/sockets -type s
ls /tmp/dl-test-socket-* || :If I run that on the base of this PR (1b986aa), the socket specified in ssh_config isn't used. [*] as well as with git-annex directly, unless |
yes... it was just an observation, and indeed it attests to the fact that datalad (and possibly git-annex, I do not remember if I have tried) already is not playing 100% nice with system/use-wide ssh configuration... so most likely this PR would not widen the already existing rift. |
My vote is on this option and therefore just merge. I don't think it's possible to not break any possible setup around SSH+git+git-annex that is designed w/o datalad. Neither the |
|
I am also thinking about having a config item which would instruct datalad to not bother doing anything about SSH, but ideally that one should be set'able per remote. This way users would be able to configure via |
I see the appeal of such an approach, but more complicated than it sounds, I think. What would that mean? Not touching any of the env vars when calling git/git-annex? Not even sufficient. How do you know which remote |
|
Looking at the discussion again after a month, and weighing the pros and cons I still think the benefits dominate the penalties. So I stand by my original approval. Looking into a better way to make datalad and SSH play with each other is much desirable, but this particular PR does not make things worse at the behavior level...code-wise it is a substantial improvement. I restarted the error-test on Travis FTR, but I would merge once things are green. |
This series addresses gh-5382. The first commit fixes a test bug that was revealed by the
datalad-sshrunswitch, the second does the actual switch, and the rest are non-essential tweaks on top.