cli: enhance errors reported upon conn failures #28200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
I'm taking suggestions as to how to best test this. |
petermattis
reviewed
Aug 2, 2018
There was a problem hiding this comment.
I'm taking suggestions as to how to best test this.
I would look at adapting the infrastructure in cli/cli_test.go. But rather than using cliTest.RunWithArgs, you call cli.Run directly for various commands and flag settings which exercise your new error messages.
Reviewable status:
complete! 0 of 0 LGTMs obtained
|
ok thanks will look into it |
knz
force-pushed
the
20180802-enhance-conn-failures
branch
2 times, most recently
from
fec24a7 to
6d2a8d3
Compare
|
Now with tests. RFAL. |
knz
force-pushed
the
20180802-enhance-conn-failures
branch
from
6d2a8d3 to
27154cc
Compare
bdarnell
approved these changes
Aug 6, 2018
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1.
Reviewable status:
pkg/cli/error.go, line 87 at r1 (raw file):
connFailed := func() error { const format = "cannot dial server.\n" + "Is the server started already?\n" +
s/started already/running/
Prior to this patch, the various CLI commands were making
a halfhearted attempt at decorating the underlying error with some
troubleshooting instructions.
Unfortunately, this was mixing up situations where the TCP connection
failed outright, those where the security settings were incorrect, and
connections dropped later, after the initial handshake was
successful.
In practice, we've noticed that the troubleshooting steps are rather
different for both kinds of situations. So this patch enhances the
code to clarify what is going on. It attempts to distinguish:
- TCP connection problem ("check --host vs --advertise-host").
- secure conn to insecure server ("use --insecure?")
- invalid TLS settings / TLS auth error ("check credentials").
- timeouts.
- connection lost (e.g. conn closed by server).
Release note (cli change): The various `cockroach` client comments
now better attempt to inform the user about why a connection is failing.
knz
force-pushed
the
20180802-enhance-conn-failures
branch
from
27154cc to
0894cd6
Compare
knz
commented
Aug 7, 2018
There was a problem hiding this comment.
Reviewable status:
pkg/cli/error.go, line 87 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
s/started already/running/
Done.
|
thanks! bors r+ cc @jseldess |
craig bot
pushed a commit
that referenced
this pull request
Aug 7, 2018
28200: cli: enhance errors reported upon conn failures r=knz a=knz Fixes #24788. Prior to this patch, the various CLI commands were making a halfhearted attempt at decorating the underlying error with some troubleshooting instructions. Unfortunately, this was mixing up situations where the TCP connection failed outright, those where the security settings were incorrect, and connections dropped later, after the initial handshake was successful. In practice, we've noticed that the troubleshooting steps are rather different for both kinds of situations. So this patch enhances the code to clarify what is going on. It attempts to distinguish: - TCP connection problem ("check --host vs --advertise-host"). - secure conn to insecure server ("use --insecure?") - invalid TLS settings / TLS auth error ("check credentials"). - timeouts. - connection lost (e.g. conn closed by server). Release note (cli change): The various `cockroach` client comments now better attempt to inform the user about why a connection is failing. Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24788.
Prior to this patch, the various CLI commands were making
a halfhearted attempt at decorating the underlying error with some
troubleshooting instructions.
Unfortunately, this was mixing up situations where the TCP connection
failed outright, those where the security settings were incorrect, and
connections dropped later, after the initial handshake was
successful.
In practice, we've noticed that the troubleshooting steps are rather
different for both kinds of situations. So this patch enhances the
code to clarify what is going on. It attempts to distinguish:
Release note (cli change): The various
cockroachclient commentsnow better attempt to inform the user about why a connection is failing.