Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RF: Make SSHConnection.open() errors actionable for callers() #3776

Merged
merged 3 commits into from
Oct 14, 2019

Conversation

mih
Copy link
Member

@mih mih commented Oct 12, 2019

Changes the return value semantics (see inside) and raises CommandError when no ControlMaster can be started. This enables to act on error on the caller side without causing spurious
warnings.

It turns the output given in gh-3450:

% datalad publish --to bareremote --transfer-data all
mih@datalad-test: Permission denied (publickey,password).
[WARNING] Failed to run cmd ['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']. Exit code=255
| stdout: None
| stderr: None
[ERROR  ] Cmd('/usr/lib/git-annex.linux/git') failed due to: exit code(128)
|   cmdline: /usr/lib/git-annex.linux/git fetch --progress -v bareremote
|   stderr: 'fatal: Could not read from remote repository.
|
| Please make sure you have the correct access rights
| and the repository exists.' [cmd.py:wait:412] (GitCommandError)

into a leaner, easier to comprehend

% datalad publish --to bareremote --transfer-data all
mih@datalad-test: Permission denied (publickey,password).
CommandError: command '['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']' failed with exitcode 255
Failed to open SSH connection (could not start ControlMaster process)

and thereby fixes gh-3450

See #3450 (comment) for more info on the wider implications and future work.

@adswa when this is merged, it will simplify the error captured at https://github.com/datalad-handbook/book/blame/master/docs/basics/101-135-help.rst#L166

Changes the return value semantics (see inside) and raises
`CommandError` when no ControlMaster can be started. This enables
to act on error on the caller side without causing spurious
warnings.

It turns the output given in dataladgh-3450:

```
% datalad publish --to bareremote --transfer-data all
mih@datalad-test: Permission denied (publickey,password).
[WARNING] Failed to run cmd ['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']. Exit code=255
| stdout: None
| stderr: None
[ERROR  ] Cmd('/usr/lib/git-annex.linux/git') failed due to: exit code(128)
|   cmdline: /usr/lib/git-annex.linux/git fetch --progress -v bareremote
|   stderr: 'fatal: Could not read from remote repository.
|
| Please make sure you have the correct access rights
| and the repository exists.' [cmd.py:wait:412] (GitCommandError)
```

into a leaning, easier to comprehend

```
% datalad publish --to bareremote --transfer-data all
mih@datalad-test: Permission denied (publickey,password).
CommandError: command '['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/mih/.cache/datalad/sockets/c5820c82', 'datalad-test']' failed with exitcode 255
Failed to open SSH connection (could not start ControlMaster process)
```

and thereby fixes dataladgh-3450
@mih mih requested a review from bpoldrack October 12, 2019 12:47
@mih
Copy link
Member Author

mih commented Oct 12, 2019

appveyor failure is same old stalled test_clone.test_autoenabled_remote_msg

@mih mih removed the request for review from bpoldrack October 13, 2019 10:06
@yarikoptic
Copy link
Member

Unrelated to the change: I wonder why ssh reports that it tried password whenever it seems it didn't

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic and reasoning seems good. But I think it might be worth (I cought do later) there changes history on possibly why we returned but not used it - may be we addressed some problem/use case?

datalad/support/sshconnector.py Outdated Show resolved Hide resolved
if exit_code != 0:
raise CommandError(
str(cmd),
'Failed to open SSH connection (could not start ControlMaster process)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about defining a dedicated exception subclassed from CommandError? With adding a debug level message with details and custom str for exception we could avoid dumping into user's face that long cryptic invocation line too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering how CommandError makes it too often into the user's face, we might want to change CommandError instead, anyway. We could change its __str__ to only consider self.msg and make the former __str__ a details property, that can be accessed by debug level log.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I RF'ed to use a dedicated exception. I agree that further optimization along the suggested lines should be done in a generic fashion and in a separate PR.

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see a problem with that one.

datalad/support/sshconnector.py Outdated Show resolved Hide resolved
if exit_code != 0:
raise CommandError(
str(cmd),
'Failed to open SSH connection (could not start ControlMaster process)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering how CommandError makes it too often into the user's face, we might want to change CommandError instead, anyway. We could change its __str__ to only consider self.msg and make the former __str__ a details property, that can be accessed by debug level log.

@codecov
Copy link

codecov bot commented Oct 13, 2019

Codecov Report

Merging #3776 into master will increase coverage by 34.17%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3776       +/-   ##
===========================================
+ Coverage   46.62%   80.79%   +34.17%     
===========================================
  Files         270      273        +3     
  Lines       35964    35969        +5     
===========================================
+ Hits        16769    29062    +12293     
+ Misses      19195     6907    -12288
Impacted Files Coverage Δ
datalad/support/sshconnector.py 61.46% <80%> (+16.71%) ⬆️
tools/coverage-bin/git-annex-remote-datalad 100% <0%> (ø)
tools/coverage-bin/datalad 100% <0%> (ø)
...ols/coverage-bin/git-annex-remote-datalad-archives 100% <0%> (ø)
datalad/config.py 98.81% <0%> (+0.39%) ⬆️
datalad/support/gitrepo.py 84.18% <0%> (+0.55%) ⬆️
datalad/core/local/tests/test_diff.py 99.4% <0%> (+0.59%) ⬆️
datalad/ui/progressbars.py 83.1% <0%> (+0.67%) ⬆️
datalad/support/external_versions.py 93.98% <0%> (+1.5%) ⬆️
datalad/support/tests/test_external_versions.py 91.94% <0%> (+2.01%) ⬆️
... and 139 more

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 dbb6d3a...05aebd5. Read the comment docs.

@mih mih merged commit 6be8278 into datalad:master Oct 14, 2019
@mih mih deleted the bf-3450 branch October 14, 2019 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSH error message madness
3 participants