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.

Sign up for GitHub

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

Jump to bottom

Improve error handling for pure SSH protocol #5063

Merged
merged 3 commits into from
Feb 16, 2023
Merged

Improve error handling for pure SSH protocol #5063

merged 3 commits into from
Feb 16, 2023

Conversation

bk2204
Copy link
Member

@bk2204 bk2204 commented Jun 30, 2022

In some cases, we can end up with a panic due to a pure SSH transfer lacking any connections, but it's not clear what those cases are. Let's add some explicit checks that our connection is functional and return an error if not, and let's also add some defensiveness to make sure that if we encounter an unexpected case that we return a better error message.

@bk2204 bk2204 mentioned this pull request Jun 30, 2022
Open
@bk2204
tq: clear sshTransfer on deletion
70b3fed 
In some cases, we can end up with a panic due to a pure SSH transfer
lacking any connections, but it's not clear what those cases are.  Let's
help improve a possible failure case by explicitly setting sshTransfer
to nil, since shutting down is the only case in which we explicitly set
the number of connections to 0.

This means that we'll get a better error message in such a case, since
we'll attempt a legacy SSH connection, which won't work, and that will
be reported to the user.
@bk2204
tq: verify connection and return error on nil
91947ab 
In some cases, we can end up with a panic due to a pure SSH transfer
lacking any connections, but it's not clear what those cases are.  Let's
add some explicit checks that our connection is functional and return an
error if not.
@bk2204
ssh: improve logging for pure SSH protocol
326b1ee 
Let's improve the logging for the pure SSH protocol so that when we have
a problem, it's easier for users to use the trace output to figure out
what's wrong.
@bk2204 bk2204 marked this pull request as ready for review February 16, 2023 15:21
@bk2204 bk2204 requested a review from a team as a code owner February 16, 2023 15:21
@bk2204 bk2204 merged commit f60fcdd into git-lfs:main Feb 16, 2023
@bk2204 bk2204 deleted the pure-ssh-failure branch February 16, 2023 17:18
@chrisd8088 chrisd8088 mentioned this pull request Oct 7, 2024
Closed
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 22, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
f5391bc 
We added trace logging to several functions and methods related to
the creation and shutdown of SSH connections in commit
326b1ee of PR git-lfs#5063, which help
when debugging any problems with our implementation of the SSH-based
object transfer protocol.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a connection even
if we are returning a non-nil error value.  Therefore we revise our
trace logging there to distinguish unsuccessful and successful conditions,
based on whether the PktlineConnection structure's Start() method
returned an error or not.

As well, we update a number of our other trace log message to include
the connection ID.  Because we maintain a set of SSH connections and
do not necessarily start or shut down all of them at the same time,
this change provides further clarity as to the state of each
individual connection at different points in a trace log.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 23, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
4b7e0aa 
We added trace logging to several functions and methods related to
the creation and shutdown of SSH connections in commit
326b1ee of PR git-lfs#5063, which help
when debugging any problems with our implementation of the SSH-based
object transfer protocol.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a connection even
if we are returning a non-nil error value.  Therefore we revise our
trace logging there to distinguish unsuccessful and successful conditions,
based on whether the PktlineConnection structure's Start() method
returned an error or not.

As well, we update a number of our other trace log message to include
the connection ID.  Because we maintain a set of SSH connections and
do not necessarily start or shut down all of them at the same time,
this change provides further clarity as to the state of each
individual connection at different points in a trace log.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 29, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
ae9d173 
We added trace logging to several functions and methods related to
the creation and shutdown of SSH connections in commit
326b1ee of PR git-lfs#5063, which help
when debugging any problems with our implementation of the SSH-based
object transfer protocol.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a connection even
if we are returning a non-nil error value.  Therefore we revise our
trace logging there to distinguish unsuccessful and successful conditions,
based on whether the PktlineConnection structure's Start() method
returned an error or not.

As well, we update a number of our other trace log message to include
the connection ID.  Because we maintain a set of SSH connections and
do not necessarily start or shut down all of them at the same time,
this change provides further clarity as to the state of each
individual connection at different points in a trace log.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 1, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
bb27fff 
We added trace logging to several functions and methods related to
the creation and shutdown of SSH connections in commit
326b1ee of PR git-lfs#5063, which help
when debugging any problems with our implementation of the SSH-based
object transfer protocol.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a connection even
if we are returning a non-nil error value.  Therefore we revise our
trace logging there to distinguish unsuccessful and successful conditions,
based on whether the PktlineConnection structure's Start() method
returned an error or not.

As well, we update a number of our other trace log message to include
the connection ID.  Because we maintain a set of SSH connections and
do not necessarily start or shut down all of them at the same time,
this change provides further clarity as to the state of each
individual connection at different points in a trace log.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
bf03770 
We added trace logging to several functions and methods related to
the creation and shutdown of SSH connections in commit
326b1ee of PR git-lfs#5063, which help
when debugging any problems with our implementation of the SSH-based
object transfer protocol.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a connection even
if we are returning a non-nil error value.  Therefore we revise our
trace logging there to distinguish unsuccessful and successful conditions,
based on whether the PktlineConnection structure's Start() method
returned an error or not.

As well, we update a number of our other trace log message to include
the connection ID.  Because we maintain a set of SSH connections and
do not necessarily start or shut down all of them at the same time,
this change provides further clarity as to the state of each
individual connection at different points in a trace log.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
@chrisd8088
ssh/connection.go: fix and enhance trace logs
693c6f8 
In commit 326b1ee of PR git-lfs#5063 we added
trace logging to several functions and methods related to the creation
and termination of SSH sessions in order to help analyze the diagnostic
logs generated when transferring Git LFS objects over SSH.

However, in the startConnection() function in our ssh/connection.go
source file, we report the successful creation of a session even if
we are returning a non-nil error value.  Therefore we revise our
trace logging in that function to distinguish between unsuccessful
and successful conditions based on whether the PktlineConnection
structure's Start() method returned an error or not.

In addition, we also update a number of our other trace log messages to
include the relevant session ID.  (Note that we refer to SSH sessions
as "connections" in our code, although in practice they may share a
single SSH connection using a control socket.)

Because we maintain a set of SSH sessions and do not necessarily start
or terminate all of them at the same time, this change will provide
more clarity as to the state of each individual session at different
points in a trace log.

Finally, we rephrase several trace log messages generated by the
setConnectionCount() method of the SSHTransfer structure so they
more fully explain when the method is terminating specific sessions
because it has been asked to reduce the total number of sessions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisd8088 chrisd8088 chrisd8088 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bk2204 @chrisd8088