clientv3: add SetEndpoints method #6330

gyuho · 2016-09-01T14:42:00Z

Fix #5491.
Related to #5920.

heyitsanthony · 2016-09-01T16:17:20Z

clientv3/client.go

+		eps = append(eps, ep)
+	}
+
+	c.balancer.mu.Lock()


only balancer methods should lock the balancer so there's a clear division of responsibility

xiang90 · 2016-09-01T16:23:13Z

@heyitsanthony @gyuho Shouldnt the first step be making the users be able to change the endpoints? then the auto-mebership-sync would be a wrapper around it?

heyitsanthony · 2016-09-01T16:26:09Z

clientv3/integration/cluster_test.go

@@ -62,6 +63,39 @@ func TestMemberAdd(t *testing.T) {
 	}
 }

+func TestMemberRemoveWithAutoSync(t *testing.T) {


This is only testing that the client is getting the member list when the important part is really client connectivity. A good test of functionality would be something like:

boot a cluster with one member

attach an autosync client

add two more members

wait for sync to grab the new members

stop the original member

use the client to access the cluster (the rpc will only go through if the sync worked)

Ok will rework on this. Thanks

gyuho · 2016-09-04T12:07:47Z

clientv3/integration/cluster_test.go

+	clus.Members[0].Stop(t)
+	clus.WaitLeader(t)
+
+	// rpc will only go through if the sync worked


This will fail because adding more endpoints doesn't update the unavailable pinned endpoint.

Do we expect users to detect the unavailable endpoint and change the pinned one manually? How should we test this?

The balancer should automatically choose another endpoint to pin when it detects the old pinned endpoint is down. For a non-mutable RPC like Get or MemberList, the user shouldn't notice the failover.

I think our clientv3 is not yet implementing Start(target string) method of grpc.Balancer interface?

func (b *simpleBalancer) Start(target string) error { return nil }

So there's no routine that watches unavailable endpoints and changes the pinned one by calling Up(addr).

The balancer shouldn't call Up, only grpc does that since that's how it signals to the balancer it established a connection. I don't think implementing Start will help-- it looks like it's only called on Dial and the endpoint updates from sync shouldn't trigger the dial path.

You are right. grpc internally calls Up via transportMonitor and resetAddrConn, but it is not being called in our test case, for some reason. I will look into it more.

gyuho · 2016-09-07T05:10:53Z

@heyitsanthony This needs more work than I thought.

Problems with this PR is

MemberList API returns client URLs, but in our integration test, we use nodeName + random number as a grpc endpoint, so sync operation based member list doesn't work in integration tests
If a client with only one endpoint cannot connect to that endpoint, the periodic MemberList call would also fail. I am not sure how we should handle this, while separating sync logic to balancer
I notice when etcd client tries to send RPC to unavailable server, the grpc.Balancer.Up(addr) never gets called because transport.NewClientTransport here is always failing as below

2016-09-07 14:04:43.654328 W | etcdserver: failed to reach the peerURL(unix://127.0.0.1:21001.31686.sock) of member 90eca40574dc4fec (Get http://127.0.0.1:21001.31686.sock/version: dial unix 127.0.0.1:21001.31686.sock: connect: no such file or directory)
2016-09-07 14:04:43.654337 W | etcdserver: cannot get the version of member 90eca40574dc4fec (Get http://127.0.0.1:21001.31686.sock/version: dial unix 127.0.0.1:21001.31686.sock: connect: no such file or directory)
2016-09-07 14:04:43.657548 W | rafthttp: lost the TCP streaming connection with peer 90eca40574dc4fec (stream Message writer)
2016-09-07 14:04:44.280331 W | rafthttp: lost the TCP streaming connection with peer 90eca40574dc4fec (stream MsgApp v2 writer)
2016-09-07 14:04:44.411989 I | v3rpc/grpc: grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial unix localhost:node4420673713181363807.sock.bridge: connect: no such file or directory"; Reconnecting to {"localhost:node4420673713181363807.sock.bridge" <nil>}
2016-09-07 14:04:46.254541 I | v3rpc/grpc: grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial unix localhost:node4420673713181363807.sock.bridge: connect: no such file or directory"; Reconnecting to {"localhost:node4420673713181363807.sock.bridge" <nil>}
2016-09-07 14:04:46.713559 W | rafthttp: lost the TCP streaming connection with peer 90eca40574dc4fec (stream Message writer)
2016-09-07 14:04:47.654631 W | etcdserver: failed to reach the peerURL(unix://127.0.0.1:21001.31686.sock) of member 90eca40574dc4fec (Get http://127.0.0.1:21001.31686.sock/version: dial unix 127.0.0.1:21001.31686.sock: connect: no such file or directory)
2016-09-07 14:04:47.654649 W | etcdserver: cannot get the version of member 90eca40574dc4fec (Get http://127.0.0.1:21001.31686.sock/version: dial unix 127.0.0.1:21001.31686.sock: connect: no such file or directory)

Any suggestion? Thanks!

heyitsanthony · 2016-09-07T05:46:47Z

Override the client.Cluster with a mock implementation that returns the right values
I think it's OK to just ignore that failure
Why would Up be called if the server is unavailable? Up is only called if grpc can establish a connection.

gyuho · 2016-09-07T18:11:27Z

clientv3/balancer.go

+		addrs = append(addrs, grpc.Address{Addr: getHost(ep)})
+	}
+	b.addrs = addrs
+	b.notifyCh <- addrs


@heyitsanthony So I am seeing gRPC is being notified of these new addrs via Notify and lbWatchers, but I am still seeing the same error in tests. Do you see anything else that I am doing wrong?

I was talking about https://github.com/grpc/grpc-go/blob/master/clientconn.go#L384-L385.

I'm not really familiar with the semantics of the notify channel. I don't think this function should be resetting readyc/upEps/upc/pinAddr though; anything that's waiting on upc or readyc will break for sure.

Yeah, I need to read more gRPC part. Will rework on this. Thanks.

gyuho · 2016-09-16T16:07:50Z

Believe test failures are not related to this.

gyuho · 2016-09-17T02:25:40Z

@heyitsanthony PTAL.

Test failures were related to the changes. Now fixed.

Have processCreds return if schema (://) does not exist.

Thanks.

heyitsanthony · 2016-09-19T19:29:01Z

clientv3/client.go

@@ -171,7 +183,8 @@ func (c *Client) dialSetupOpts(endpoint string, dopts ...grpc.DialOption) (opts
 	}
 	opts = append(opts, grpc.WithDialer(f))

-	_, _, creds := c.dialTarget(endpoint)
+	proto, _, scheme := parseEndpoint(endpoint)
+	creds := c.processCreds(proto, scheme)


I think processCreds can do without the bool argument:

creds := c.creds if proto, _, scheme := parseEndpoint(endpoint); scheme { creds = c.processCreds(proto) }

heyitsanthony · 2016-09-19T19:29:56Z

lgtm following processCreds args fixup. Thanks!

gyuho force-pushed the balancer-sync branch from f2f6bbb to ae51ed8 Compare September 1, 2016 14:46

gyuho added the kind/release-note label Sep 1, 2016

heyitsanthony reviewed Sep 1, 2016
View reviewed changes

gyuho force-pushed the balancer-sync branch 2 times, most recently from 1e6fa9f to f010129 Compare September 4, 2016 12:05

gyuho reviewed Sep 4, 2016
View reviewed changes

gyuho force-pushed the balancer-sync branch 3 times, most recently from 67736d0 to fff2ce4 Compare September 5, 2016 02:20

gyuho added the WIP label Sep 6, 2016

gyuho force-pushed the balancer-sync branch from fff2ce4 to a5a2fcc Compare September 7, 2016 05:00

gyuho force-pushed the balancer-sync branch 4 times, most recently from 5fd6a0c to 096ff3b Compare September 7, 2016 18:09

gyuho reviewed Sep 7, 2016
View reviewed changes

gyuho changed the title *: autoSync in balancer *: AutoSync in balancer Sep 9, 2016

gyuho changed the title *: AutoSync in balancer *: AutoSync client endpoints Sep 9, 2016

gyuho force-pushed the balancer-sync branch from 096ff3b to 5ad9f16 Compare September 11, 2016 02:06

gyuho changed the title *: AutoSync client endpoints clientv3: add UpdateEndpoints method Sep 11, 2016

gyuho force-pushed the balancer-sync branch from 5ad9f16 to c3c078a Compare September 11, 2016 02:11

gyuho force-pushed the balancer-sync branch 2 times, most recently from 274c1b1 to e4d6312 Compare September 16, 2016 15:52

gyuho force-pushed the balancer-sync branch 11 times, most recently from 95d5a26 to 4a51719 Compare September 17, 2016 02:07

gyuho force-pushed the balancer-sync branch 5 times, most recently from 95d2a15 to dfc8c73 Compare September 18, 2016 16:34

heyitsanthony reviewed Sep 19, 2016

View reviewed changes

heyitsanthony approved these changes Sep 19, 2016

View reviewed changes

gyuho added 2 commits September 20, 2016 04:36

clientv3: add 'SetEndpoints' method

b9d18d4

clientv3/integration: test 'SetEndpoints'

f11b35e

gyuho force-pushed the balancer-sync branch from dfc8c73 to f11b35e Compare September 19, 2016 19:36

gyuho merged commit c9e06fa into etcd-io:master Sep 19, 2016

gyuho deleted the balancer-sync branch September 19, 2016 19:52

gyuho mentioned this pull request Sep 19, 2016

clientv3: add 'Sync' method #6474

Merged

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientv3: add SetEndpoints method #6330

clientv3: add SetEndpoints method #6330

gyuho commented Sep 1, 2016 •

edited

heyitsanthony Sep 1, 2016

xiang90 commented Sep 1, 2016

heyitsanthony Sep 1, 2016

gyuho Sep 1, 2016

gyuho Sep 4, 2016 •

edited

heyitsanthony Sep 5, 2016

gyuho Sep 5, 2016

heyitsanthony Sep 6, 2016

gyuho Sep 6, 2016

gyuho commented Sep 7, 2016

heyitsanthony commented Sep 7, 2016

gyuho Sep 7, 2016

gyuho Sep 7, 2016

heyitsanthony Sep 7, 2016

gyuho Sep 7, 2016

gyuho commented Sep 16, 2016

gyuho commented Sep 17, 2016

heyitsanthony Sep 19, 2016 •

edited

heyitsanthony commented Sep 19, 2016

clientv3: add SetEndpoints method #6330

clientv3: add SetEndpoints method #6330

Conversation

gyuho commented Sep 1, 2016 • edited

Choose a reason for hiding this comment

xiang90 commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho Sep 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Sep 7, 2016

heyitsanthony commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Sep 16, 2016

gyuho commented Sep 17, 2016

heyitsanthony Sep 19, 2016 • edited

Choose a reason for hiding this comment

heyitsanthony commented Sep 19, 2016

gyuho commented Sep 1, 2016 •

edited

gyuho Sep 4, 2016 •

edited

heyitsanthony Sep 19, 2016 •

edited