grpcproxy: fix memberlist results not update when proxy node down #15835

yellowzf · 2023-05-06T03:38:07Z

If start grpc proxy with --resolver-prefix, memberlist will return all alive proxy nodes, when one grpc proxy node is down, it is expected to not return the down node, but it is still return

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

yellowzf · 2023-05-06T03:51:25Z

server/proxy/grpcproxy/cluster.go

 				case endpoints.Delete:
-					delete(cp.umap, up.Endpoint.Addr)


when op delete, up.Endpoint.Addr is not specified, can not delete the endpoint from map, use up.Key instead

looks good, I think using Key is ok because it's already used in resolver.watch https://github.com/etcd-io/etcd/blob/249c0d71d4e106e0d2f5576b9638f2e8bd75ca47/client/v3/naming/resolver/resolver.go#LL97C6-L97C12
Not sure why Endpoint.Addr was used initially

lavacat

LGTM, but needs a test.
case endpoints.Add: is covered in TestClusterProxyMemberList. But case endpoints.Delete isn't covered at all.

lavacat · 2023-05-06T05:20:58Z

server/proxy/grpcproxy/cluster.go

 				case endpoints.Delete:
-					delete(cp.umap, up.Endpoint.Addr)


looks good, I think using Key is ok because it's already used in resolver.watch https://github.com/etcd-io/etcd/blob/249c0d71d4e106e0d2f5576b9638f2e8bd75ca47/client/v3/naming/resolver/resolver.go#LL97C6-L97C12
Not sure why Endpoint.Addr was used initially

yellowzf · 2023-05-08T12:13:26Z

LGTM, but needs a test. case endpoints.Add: is covered in TestClusterProxyMemberList. But case endpoints.Delete isn't covered at all.

tests added

yellowzf · 2023-05-09T01:54:38Z

PTAL, it's ready for review @lavacat

lavacat

Please cleanup the logic and add comments to any non-obvious test setup and checks.

There is also TestRegister test that uses NewWatchChannel to listen to updates, it might be easier to update. Up to you.

lavacat · 2023-05-10T00:55:37Z

tests/integration/proxy/grpcproxy/cluster_test.go

@@ -90,6 +129,27 @@ func (cts *clusterproxyTestServer) close(t *testing.T) {
 	}
 }

+func registDelayDeleteMember(lg *zap.Logger, endpoints []string, addr string, delay time.Duration, ttl int, t *testing.T) chan struct{} {


nit: registerMemberWithTTL

removed this func in new code, use DeleteEndpoint is better

lavacat · 2023-05-10T01:02:54Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	time.Sleep(time.Second)
+	donec := make(chan struct{})
+	go func() {
+		time.Sleep(delay)


what is the point this goroutine if it's just waiting? I think this can be removed

this goroutine used to delay close the etcd client, if client is closed, lease will expire soon, then the key which lease attached will delete, it is complex, use DeleteEndpoint is more convenient

lavacat · 2023-05-10T01:04:34Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	}
+
+	//check del member succ
+	time.Sleep(wait)


wait > delay. What is the point if delay?

use delay then test case can check proxy member has successfully updated

lavacat · 2023-05-10T01:13:39Z

tests/integration/proxy/grpcproxy/cluster_test.go

+		Endpoints:   endpoints,
+		DialTimeout: 5 * time.Second,
+	}
+	client, err := integration2.NewClient(t, cfg)


Is it possible to pass client from the test without creating a new one?

yellowzf · 2023-05-10T04:08:34Z

Please cleanup the logic and add comments to any non-obvious test setup and checks.

There is also TestRegister test that uses NewWatchChannel to listen to updates, it might be easier to update. Up to you.

TestRegister only cover watch channel logic, TestClusterProxyMemberList cover member list api and watch channel.
in new code, use DeleteEndpoint to delete key of member, seems more clear.
it's ready for review, @lavacat , PTAL

lavacat · 2023-05-11T18:45:26Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	if len(mresp.Members) != 2 {
+		t.Fatalf("len(mresp.Members) expected 2, got %d (%+v)", len(mresp.Members), mresp.Members)
+	}
+	if mresp.Members[1].ClientURLs[0] != newMemberAddr {


nit: I think Members is populated from a map, so order isn't guarantied. Might cause test flakiness.

Can we add a check for Members[0] and add a comment about order?

map can not guarantee order, we should check if the new member is in mresp.Member, codes have updated, PTAL

lavacat · 2023-05-11T18:49:23Z

I wonder if anything can be done with those time.Sleep(time.Second) in the test? I understand that you are reusing prior code, but if you see some other solution would be nice to update.

yellowzf · 2023-05-12T02:46:17Z

I wonder if anything can be done with those time.Sleep(time.Second) in the test? I understand that you are reusing prior code, but if you see some other solution would be nice to update.

proxy watch specific prefix for members update, when register or delete members by put/delete key, it updates members by process watch resp, it is asynchronous, so use time.Sleep(time.Second) to wait for update, i think prior code is the same meaning

lavacat · 2023-05-12T07:16:00Z

tests/integration/proxy/grpcproxy/cluster_test.go

@@ -67,6 +71,47 @@ func TestClusterProxyMemberList(t *testing.T) {
 	if mresp.Members[0].ClientURLs[0] != cts.caddr {
 		t.Fatalf("mresp.Members[0].ClientURLs[0] expected %q, got %q", cts.caddr, mresp.Members[0].ClientURLs[0])
 	}
+
+	//test proxy member up/down


nit: test proxy member add

lavacat · 2023-05-12T07:18:49Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	}
+
+	succ := false
+	for _, member := range mresp.Members {


nit: replace with

hostname, _ := os.Hostname() ... assert.Contains(t, mresp.Members, &pb.Member{Name: hostname, ClientURLs: []string{newMemberAddr}})

lavacat · 2023-05-12T07:22:54Z

tests/integration/proxy/grpcproxy/cluster_test.go

@@ -67,6 +71,47 @@ func TestClusterProxyMemberList(t *testing.T) {
 	if mresp.Members[0].ClientURLs[0] != cts.caddr {


nit: assert.Contains(t, mresp.Members, &pb.Member{Name: hostname, ClientURLs: []string{cts.caddr}})

lavacat · 2023-05-12T07:23:33Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	if len(mresp.Members) != 1 {
+		t.Fatalf("len(mresp.Members) expected 1, got %d (%+v)", len(mresp.Members), mresp.Members)
+	}
+	if mresp.Members[0].ClientURLs[0] != cts.caddr {


nit: assert.Contains(t, mresp.Members, &pb.Member{Name: hostname, ClientURLs: []string{cts.caddr}})

lavacat · 2023-05-12T07:26:26Z

LGTM, please use assert.Contains as per comments. cc @ahrtr for final review

lavacat · 2023-05-12T07:41:31Z

tests/integration/proxy/grpcproxy/cluster_test.go

+	newMemberAddr := "127.0.0.2:6789"
+	grpcproxy.Register(lg, cts.c, prefix, newMemberAddr, 7)
+	// wait some time for proxy update members
+	time.Sleep(time.Second)


nit: I think we can switch to time.Sleep(200 * time.Millisecond) for all waits in this test. 1s seams too long.

ahrtr · 2023-05-13T01:44:21Z

If start grpc proxy with --resolver-prefix, memberlist will return all alive proxy nodes, when one grpc proxy node is down, it is expected to not return the down node, but it is still return

I do not get the real issue. Please provide the detailed steps to reproduce the issue.

If start grpc proxy with --resolver-prefix, memberlist will return all alive proxy nodes, when one grpc proxy node is down, it is expected to not return the down node, but it is still return Signed-off-by: yellowzf <zzhf3311@163.com>

yellowzf · 2023-05-15T03:15:25Z

If start grpc proxy with --resolver-prefix, memberlist will return all alive proxy nodes, when one grpc proxy node is down, it is expected to not return the down node, but it is still return

I do not get the real issue. Please provide the detailed steps to reproduce the issue.

step1: start a grpc-proxy cluster which contains 3 member, member1/member2/member3, use --resolver-prefix flag when member start.
step2: use etcd client connect to member1, call MemberList, get a response which contains member1/member2/member3.
step3: stop member3, then lease of member3 in etcd server will expire and key will be deleted.
step4: use etcd client connect to member1, call MemberList, still get a response which contains member1/member2/member3, it is expect to only contains member1/member2 because member3 is stoped. @ahrtr

yellowzf · 2023-05-15T03:18:28Z

LGTM, please use assert.Contains as per comments. cc @ahrtr for final review

all codes memtioned on comments has updated, @lavacat PTAL

ahrtr

LGTM

Thanks @yellowzf for the fix.

Could you backport this PR to 3.4 and 3.5?

yellowzf · 2023-05-16T03:57:22Z

LGTM

Thanks @yellowzf for the fix.

Could you backport this PR to 3.4 and 3.5?

3.4 use PreKv to assign addr, no need to backport, https://github.com/etcd-io/etcd/blob/release-3.4/clientv3/naming/grpc.go#L91-L100

3.5 backport at #15907

yellowzf commented May 6, 2023

View reviewed changes

lavacat suggested changes May 6, 2023

View reviewed changes

yellowzf force-pushed the grpcproxy_fix_memberlist_results_not_update_when_proxy_node_down branch from 87355e9 to eb54d18 Compare May 8, 2023 12:09

yellowzf closed this May 8, 2023

yellowzf reopened this May 8, 2023

lavacat suggested changes May 10, 2023

View reviewed changes

yellowzf force-pushed the grpcproxy_fix_memberlist_results_not_update_when_proxy_node_down branch from eb54d18 to 2fd92e4 Compare May 10, 2023 03:48

lavacat reviewed May 11, 2023

View reviewed changes

yellowzf force-pushed the grpcproxy_fix_memberlist_results_not_update_when_proxy_node_down branch from 2fd92e4 to c50e7bd Compare May 12, 2023 02:31

lavacat approved these changes May 12, 2023

View reviewed changes

lavacat reviewed May 12, 2023

View reviewed changes

yellowzf force-pushed the grpcproxy_fix_memberlist_results_not_update_when_proxy_node_down branch from c50e7bd to ca22120 Compare May 15, 2023 02:59

ahrtr added backport/v3.4 backport/v3.5 labels May 15, 2023

ahrtr approved these changes May 15, 2023

View reviewed changes

ahrtr merged commit 2df3210 into etcd-io:main May 15, 2023

yellowzf mentioned this pull request May 16, 2023

[release-3.5]grpcproxy: fix memberlist results not update when proxy node down #15907

Merged

ahrtr mentioned this pull request Apr 28, 2024

grpc-proxy has resolver-ttl broken since 3.4.28 #17887

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpcproxy: fix memberlist results not update when proxy node down #15835

grpcproxy: fix memberlist results not update when proxy node down #15835

yellowzf commented May 6, 2023

yellowzf May 6, 2023

lavacat May 6, 2023

lavacat left a comment

lavacat May 6, 2023

yellowzf commented May 8, 2023

yellowzf commented May 9, 2023

lavacat left a comment

lavacat May 10, 2023

yellowzf May 10, 2023

lavacat May 10, 2023

yellowzf May 10, 2023

lavacat May 10, 2023

yellowzf May 10, 2023 •

edited

Loading

lavacat May 10, 2023

yellowzf May 10, 2023

yellowzf commented May 10, 2023

lavacat May 11, 2023

yellowzf May 12, 2023

lavacat commented May 11, 2023

yellowzf commented May 12, 2023

lavacat May 12, 2023

lavacat May 12, 2023

lavacat May 12, 2023

lavacat May 12, 2023

lavacat commented May 12, 2023

lavacat May 12, 2023

ahrtr commented May 13, 2023

yellowzf commented May 15, 2023

yellowzf commented May 15, 2023

ahrtr left a comment

yellowzf commented May 16, 2023 •

edited

Loading

		@@ -67,6 +71,47 @@ func TestClusterProxyMemberList(t *testing.T) {
		if mresp.Members[0].ClientURLs[0] != cts.caddr {

grpcproxy: fix memberlist results not update when proxy node down #15835

grpcproxy: fix memberlist results not update when proxy node down #15835

Conversation

yellowzf commented May 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavacat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowzf commented May 8, 2023

yellowzf commented May 9, 2023

lavacat left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowzf May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yellowzf commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavacat commented May 11, 2023

yellowzf commented May 12, 2023

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

lavacat commented May 12, 2023

Choose a reason for hiding this comment

ahrtr commented May 13, 2023

yellowzf commented May 15, 2023

yellowzf commented May 15, 2023

ahrtr left a comment

Choose a reason for hiding this comment

yellowzf commented May 16, 2023 • edited Loading

yellowzf May 10, 2023 •

edited

Loading

yellowzf commented May 16, 2023 •

edited

Loading