connection: add tests#1
Conversation
|
There's one more test that might be useful: ensuring that gRPC does not attempt to reconnect after loosing the connection. I'll have a look at that now. |
| assert.InEpsilon(t, time.Second, endTime.Sub(startTime), 1, "connection loss should be detected quickly") | ||
| } | ||
|
|
||
| // No reconnection either. |
There was a problem hiding this comment.
I am not sure what this test tries to do. If server is gone, gRPC should return codes.Unavailable quickly. That's correct. But it will try to reconnect if you re-start the server. So No reconnection either comment is quite confusing.
|
Jan Šafránek <notifications@github.com> writes:
jsafrane commented on this pull request.
+ // No reconnection either.
I am not sure what this test tries to do. If server is gone, gRPC
should return `codes.Unavailable` quickly. That's correct. But it will
try to reconnect if you re-start the server. So `No reconnection
either` comment is quite confusing.
Is was trying to rule out that a second method call somehow causes gRPC
to do something - not sure what exactly, though.
You are right, a much better test is to restart the server. We don't
want to reconnect to the server under any condition, do we? If gRPC does
it under the hood, then we won't notice that cached information became
stale.
The test above does not cover that because the client gets a
"Unavailable" once. A better test is to have no call pending, restart
the server and then do a method call. This should still result in an
"Unavailable" error.
|
|
We need to be very careful here. Is Digging into code, there are few places when csi-lib-utils/vendor/google.golang.org/grpc/internal/transport/transport.go Lines 664 to 673 in 179d6f9 |
3a96d52 to
978a77b
Compare
Some but not all CSI sidecar apps may want to cache information retrieved from a CSI driver. Those (and only) those apps needs a way to detect that the cached information became invalid due to a CSI driver restart and then react to that. The Connect function supports notifying the app via an optional callback which then can: - kill the app via os.Exit - invalidate the cached information and continue by reconnecting - prevent reconnecting and exit the application when gRPC method calls fail with status.Unavailable The default behavior is as before, i.e. the connection is getting restablished.
This verifies that both unix:/// and absolute paths work and that the timing behavior is as expected (wait for server, react promptly once it appears).
978a77b to
fad9258
Compare
|
@jsafrane I've implemented the approach with an explicit "connection lost" callback in this PR. Shall we merge that into your branch and from there into the upstream repo or shall I create a new PR? |
This verifies that both unix:/// and absolute paths work and that the
timing behavior is as expected (wait for server, react promptly once
it appears).