-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[vtadmin] Update vtctld dialer to validate connectivity #9915
Changes from 13 commits
5f90c54
11761ee
f5ec764
f758a15
3e35df9
f078bcb
1a4fcf9
bed75c2
9fc1c2e
9215e39
d8318d6
5742703
0ae0c1a
fef9e98
5c8d160
afe15d9
92e7055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |
"context" | ||
"net" | ||
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
@@ -34,16 +35,25 @@ type fakeVtctld struct { | |
vtctlservicepb.VtctlServer | ||
} | ||
|
||
func TestDial(t *testing.T) { | ||
func initVtctlServer() (net.Listener, *grpc.Server, error) { | ||
listener, err := net.Listen("tcp", "127.0.0.1:0") | ||
require.NoError(t, err) | ||
|
||
defer listener.Close() | ||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
||
vtctld := &fakeVtctld{} | ||
server := grpc.NewServer() | ||
vtctlservicepb.RegisterVtctlServer(server, vtctld) | ||
|
||
return listener, server, err | ||
} | ||
|
||
func TestDial(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what you're seeing running just this test in isolation is super interesting, do you mind filing an issue and i can dig into it? i'm not really sure what's going, but it could be a "macs suck at local networking" issue, or something not completely correct in the code, but it's hard to say (and i don't think we should block this PR, which I still maintain is strictly an improvement over the current, uh, Situation) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: #9943 Thanks for taking a look. I'm super interested in what you find! I spent way too long looking at gRPC internals for this PR 😭 |
||
listener, server, err := initVtctlServer() | ||
require.NoError(t, err) | ||
|
||
defer listener.Close() | ||
|
||
go server.Serve(listener) | ||
defer server.Stop() | ||
|
||
|
@@ -57,7 +67,8 @@ func TestDial(t *testing.T) { | |
Id: "test", | ||
Name: "testcluster", | ||
}, | ||
Discovery: disco, | ||
Discovery: disco, | ||
ConnectivityTimeout: 2 * time.Second, | ||
}) | ||
|
||
// We don't have a vtctld host until we call Dial | ||
|
@@ -67,3 +78,94 @@ func TestDial(t *testing.T) { | |
assert.NoError(t, err) | ||
assert.Equal(t, listener.Addr().String(), proxy.host) | ||
} | ||
|
||
// TestRedial tests that vtadmin-api is able to recover from a lost connection to | ||
// a vtctld by rediscovering and redialing a new one. | ||
func TestRedial(t *testing.T) { | ||
// Initialize vtctld #1 | ||
listener1, server1, err := initVtctlServer() | ||
require.NoError(t, err) | ||
|
||
defer listener1.Close() | ||
|
||
go server1.Serve(listener1) | ||
defer server1.Stop() | ||
|
||
// Initialize vtctld #2 | ||
listener2, server2, err := initVtctlServer() | ||
require.NoError(t, err) | ||
|
||
defer listener2.Close() | ||
|
||
go server2.Serve(listener2) | ||
defer server2.Stop() | ||
|
||
// Register both vtctlds with VTAdmin | ||
disco := fakediscovery.New() | ||
disco.AddTaggedVtctlds(nil, &vtadminpb.Vtctld{ | ||
Hostname: listener1.Addr().String(), | ||
}, &vtadminpb.Vtctld{ | ||
Hostname: listener2.Addr().String(), | ||
}) | ||
|
||
proxy := New(&Config{ | ||
Cluster: &vtadminpb.Cluster{ | ||
Id: "test", | ||
Name: "testcluster", | ||
}, | ||
Discovery: disco, | ||
ConnectivityTimeout: 2 * time.Second, | ||
}) | ||
|
||
// We don't have a vtctld host until we call Dial | ||
require.Empty(t, proxy.host) | ||
|
||
// Check for a successful connection to whichever vtctld we discover first. | ||
err = proxy.Dial(context.Background()) | ||
assert.NoError(t, err) | ||
|
||
// vtadmin's fakediscovery package discovers vtctlds in random order. Rather | ||
// than force some cumbersome sequential logic, we can just do a switcheroo | ||
// here in the test to determine our "current" and (expected) "next" vtctlds. | ||
var currentVtctld *grpc.Server | ||
var nextAddr string | ||
|
||
switch proxy.host { | ||
case listener1.Addr().String(): | ||
currentVtctld = server1 | ||
nextAddr = listener2.Addr().String() | ||
|
||
case listener2.Addr().String(): | ||
currentVtctld = server2 | ||
nextAddr = listener1.Addr().String() | ||
default: | ||
t.Fatalf("invalid proxy host %s", proxy.host) | ||
} | ||
|
||
// Remove the shut down vtctld from VTAdmin's service discovery (clumsily). | ||
// Otherwise, when redialing, we may redial the vtctld that we just shut down. | ||
disco.Clear() | ||
disco.AddTaggedVtctlds(nil, &vtadminpb.Vtctld{ | ||
Hostname: nextAddr, | ||
}) | ||
|
||
// Force an ungraceful shutdown of the gRPC server to which we're connected | ||
currentVtctld.Stop() | ||
|
||
// Wait for the client connection to shut down. (If we redial too quickly, | ||
// we get into a race condition with gRPC's internal retry logic. | ||
// (Using WaitForReady here _does_ expose more function internals than is ideal for a unit test, | ||
// but it's far less flaky than using time.Sleep.) | ||
for { | ||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
defer cancel() | ||
if err = proxy.VtctldClient.WaitForReady(ctx); err != nil { | ||
break | ||
} | ||
} | ||
|
||
// Finally, check that we discover, dial + establish a new connection to the remaining vtctld. | ||
err = proxy.Dial(context.Background()) | ||
assert.NoError(t, err) | ||
assert.Equal(t, nextAddr, proxy.host) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more context about the leaked connections thing in the PR description.