chore: Handle sftp reconnections #4075
Conversation
javier-aliaga
force-pushed
the
sftp-connections
branch
19 times, most recently
from
a331e1d to
f79c833
Compare
javier-aliaga
force-pushed
the
sftp-connections
branch
2 times, most recently
from
d211516 to
d43bfab
Compare
bindings/sftp/client.go
Outdated
| c.rLock.Lock() | ||
| defer c.rLock.Unlock() |
There was a problem hiding this comment.
It is hard to follow the logic when we have 2 locks
There was a problem hiding this comment.
Added comment to explain why is each lock used, let me know what do you think
bindings/sftp/client.go
Outdated
| msg := strings.ToLower(err.Error()) | ||
| switch { | ||
| case strings.Contains(msg, "use of closed network connection"), | ||
| strings.Contains(msg, "connection reset by peer"), |
There was a problem hiding this comment.
Can we not use typed errors here? syscall.ECONNRESET
There was a problem hiding this comment.
I will add to the previous if(L230), but I will keep the message here just in case.
There was a problem hiding this comment.
I modified the method, we will not reconnect if it is one of this errors:
if errors.Is(err, sftpClient.ErrSSHFxPermissionDenied) ||
errors.Is(err, sftpClient.ErrSSHFxNoSuchFile) ||
errors.Is(err, sftpClient.ErrSSHFxOpUnsupported) {
return false
}
Any other error we will try to reconnect
javier-aliaga
force-pushed
the
sftp-connections
branch
from
55f5a6a to
90f6d6b
Compare
javier-aliaga
force-pushed
the
sftp-connections
branch
2 times, most recently
from
69d8f53 to
c3594d7
Compare
javier-aliaga
force-pushed
the
sftp-connections
branch
3 times, most recently
from
78a33bd to
cc88c05
Compare
cicoyle
left a comment
cicoyle
left a comment
There was a problem hiding this comment.
few things from me. Thx!
bindings/sftp/client.go
Outdated
| } | ||
|
|
||
| func withReconnection(c *Client, fn func() error) error { | ||
| c.lock.RLock() |
There was a problem hiding this comment.
This locking then calling to reconnect then locking again feels like maybe we should have an abstraction function with a for loop where we only lock in that one place versus in a few places. Maybe it could be cleaner if we had a client.do func that had a for loop where we check the client, then do the shouldReconnect and reconnect calls - this way we try reconnecting more than once otherwise, as is we only try once I believe.
There was a problem hiding this comment.
I did not want to put more complex mechanism with retries and backoffs, I think this component it may need to be rewritten as some customers requested different improvements. I did a small refactor to introduce a do func where we lock the action 51c4e94
what do you think?
There was a problem hiding this comment.
I agree it can be rewritten and improved further. plz open an issue so we dont forget and it can be prioritized at a later date 👍🏻
javier-aliaga
force-pushed
the
sftp-connections
branch
2 times, most recently
from
1afe19b to
72d44bf
Compare
javier-aliaga
force-pushed
the
sftp-connections
branch
from
72d44bf to
a9de6e7
Compare
Signed-off-by: Javier Aliaga <javier@diagrid.io>
javier-aliaga
force-pushed
the
sftp-connections
branch
from
a9de6e7 to
b83457d
Compare
Signed-off-by: Javier Aliaga <javier@diagrid.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
Signed-off-by: Javier Aliaga <javier@diagrid.io> Signed-off-by: Kobbi Gal <kobbi.g@akeyless.io>
4 participants
Description
This PR introduces a re-connection mechanism to the sftp binding.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #4061
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
Note: We expect contributors to open a corresponding documentation PR in the dapr/docs repository. As the implementer, you are the best person to document your work! Implementation PRs will not be merged until the documentation PR is opened and ready for review.