Added client-auto-sync-interval argument to the grpc-proxy #14354

biosvs · 2022-08-16T13:49:23Z

etcdmain: added client-auto-sync-interval argument to the grpc-proxy

Currently it's possible to enable auto sync in client, which connects to grpc-proxies (if --namespace is used). But grpc-proxy itself don't have an option to update endpoints list from etcd clusters.

This PR introduces an option client-auto-sync-interval for grpc-proxy, which allows to specify auto sync interval for grpc-proxy itself.

ahrtr · 2022-08-18T01:12:42Z

server/etcdmain/grpc_proxy.go

@@ -130,6 +131,7 @@ func newGRPCProxyStartCommand() *cobra.Command {
 	cmd.Flags().StringVar(&grpcProxyMetricsListenAddr, "metrics-addr", "", "listen for endpoint /metrics requests on an additional interface")
 	cmd.Flags().BoolVar(&grpcProxyInsecureDiscovery, "insecure-discovery", false, "accept insecure SRV records")
 	cmd.Flags().StringSliceVar(&grpcProxyEndpoints, "endpoints", []string{"127.0.0.1:2379"}, "comma separated etcd cluster endpoints")
+	cmd.Flags().DurationVar(&grpcProxyClientAutoSyncInterval, "client-auto-sync-interval", 0, "etcd endpoints auto sync interval (disabled by default)")


rename to endpoints_auto_sync_interval?

Yep, looks better. Changed.

ahrtr · 2022-08-18T01:14:34Z

Thanks @biosvs for the PR, two comments:

Please add a changelog item into CHANGELOG-3.6;
Please try to add an integration/e2e test case.

biosvs · 2022-08-18T17:07:47Z

@ahrtr thank you for the quick review!

Appended changelog.
Wrote e2e test - I used plain commands instead of wrappers, because looks like that's the only way to run independent grpc-proxy and to change cluster members list (without discovery, which I didn't want to use to avoid over complication). But I open for any suggestion, of course.

ahrtr · 2022-08-18T22:15:36Z

tests/e2e/etcd_grpcproxy_test.go

+	"go.etcd.io/etcd/tests/v3/framework/e2e"
+)
+
+func TestGrpcProxyAutoSync(t *testing.T) {


I would suggest to add a test into gateway_test.go and reuse some existing code, and also reuse ctlV3MemberAdd or ctlV3MemberRemove

I believe it can greatly simplify the implementation.

Actually a better approach is to add a common test, which can be executed on both integration and e2e test environment. FYI. #13637

I'm not sure why grpc-proxy tests should be in the same file with the gateway tests? They are different things, and therefore it's impossible to reuse startGateway function, for example.

I tried to use ctl* function, but they accept ctlCtx which I don't have and can't create properly. It useless without setting up epc field of type e2e.EtcdProcessCluster, which in turn I can't use because I need to add/remove member, which is impossible with e2e.EtcdProcessCluster.

I really love your idea to use common framework to write tests once and then run them for both integration and e2e env. But for this particular case I have two concerns:
a. Neither e2e framework nor common framework have API for adding/removing members properly.
b. GRPC-proxy is a standalone application, I'm not sure if we at can talk about integration tests at all.

I also thought how to write integration test for grpc-proxy, but auto sync is part of client. What I want to check is the code inside etcdmain, which cannot be tested in integration tests way (without actually ports binding, running additional processes, etc.).

So in fact my changes can be tested only in e2e tests, I guess.

I'm not sure why grpc-proxy tests should be in the same file with the gateway tests? They are different things, and therefore it's impossible to reuse startGateway function, for example.

Yes, you are right. Sorry I misread the filename and content. They are indeed different things.

So in fact my changes can be tested only in e2e tests, I guess.

Agreed.

I tried to use ctl* function, but they accept ctlCtx which I don't have and can't create properly. It useless without setting up epc field of type e2e.EtcdProcessCluster, which in turn I can't use because I need to add/remove member, which is impossible with e2e.EtcdProcessCluster.

There are lots of examples in the existing e2e test cases. Please try to reuse the existing utilities/methods/functions, such as testCtl, getMemberList, ctlV3MemberAdd, ctlV3MemberRemove, etc.

Used e2e.EtcdctlV3, now it looks better (without custom json parser).

As for ctlV3 - I'm afraid I can't use it. Let me explain myself:

gprc-proxy is not part of ctl utils, it's part of main etcd.

All ctl* test function requires ctlCtx, which could be created only with e2e.EtcdProcessCluster.

I can't use e2e.EtcdProcessCluster (neither with ctl*, nor directly), because it spawns all etcd nodes by its own. It's possible to change member list, but impossible to actually start new node or stop another.

I also can't use e2e.EtcdProcess, because it could be created only from e2e.EtcdServerProcessConfig, which has private field lg, which is set only by function that creates e2e.EtcdProcessCluster, which I can't use (previous paragraph).

At last, I found number of examples in e2e tests, where nodes are spawned explicitly. So I hope for now my test fits into other e2e tests (:

It's possible to change member list, but impossible to actually start new node or stop another

It seems like a real gap. Please consider to improve the existing e2e test framework if you have bandwidth and interested, of course can be in a separate PR.

BTW I found another place where adding new member would be useful: https://github.com/etcd-io/etcd/blob/main/tests/e2e/ctl_v3_snapshot_test.go#L258

Sure, I'll try to extend API.

ahrtr · 2022-08-23T22:08:43Z

tests/e2e/etcd_grpcproxy_test.go

+	time.Sleep(autoSyncInterval + 5*time.Second)
+
+	memberList, err := memberCtl.MemberList()
+	require.NoError(t, err)


Hard coded the 5 seconds sleep seems not good. Please consider to wait until the 2 members are ready, of course with a timeout.

There is no any API event regarding endpoints update. But fortunately grpc-proxy dumps endpoints into log, so I used Expect to wait for the update.

ahrtr · 2022-08-23T22:09:10Z

tests/e2e/etcd_grpcproxy_test.go

+	memberList, err := memberCtl.MemberList()
+	require.NoError(t, err)
+
+	node1MemberID, err := findMemberIDByEndpoint(memberList.Members, node1ClientURL)


Check node2ClientURL as well?

It's not necessary here, because later we're ensuring that gprc-proxy reads value from second node.

tests/e2e/etcd_grpcproxy_test.go

ahrtr · 2022-08-23T22:16:47Z

tests/e2e/etcd_grpcproxy_test.go

+	require.NoError(t, proc1.Stop())
+
+	// Wait for the full stop
+	time.Sleep(time.Second)


It seems unnecessary.

Changed all hardcoded timeouts to retries.

biosvs · 2022-08-25T09:57:16Z

Used new Expect function with the context. And all checks finally have passed.

ahrtr

Great work! Thanks @biosvs

I believe majority of the test code should be integrated into the existing e2e test framework, and accordingly simplify this test case and benefit other similar e2e cases. Of course, it can be addressed in separate PR.

cc @serathius @spzala @ptabor to double check.

serathius · 2022-08-25T11:24:30Z

tests/e2e/etcd_grpcproxy_test.go

+	require.NoError(t, proxyProc.Stop())
+}
+
+func runEtcdNode(name, dataDir, clientURL, peerURL, clusterState, initialCluster string) (*expect.ExpectProcess, error) {


Could you use `tests/framework/e2e.NewEtcdServerProcess instead?

Unfortunately I can't, described reasons above: #14354 (comment)

Heh, ctlV3 is terrible :(. Sorry for not noticing above comment.

serathius · 2022-08-25T11:27:12Z

tests/e2e/etcd_grpcproxy_test.go

+	// ExpectFunc reads existing lines, but we're expecting for the events from the future,
+	// that's why sometimes we have to repeat ExpectFunc calls.
+	for {
+		select {


No need for checking ctx.Done here. There is no other IO operations in this loop than ExpectFunc which already does this for you.

serathius · 2022-08-25T11:31:13Z

tests/e2e/etcd_grpcproxy_test.go

+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	// ExpectFunc reads existing lines, but we're expecting for the events from the future,


Maybe I'm missing something, but I think this statement is not true. ExpectFunc has an infinite for loop that only ends if we match the line or there is an error reported (expected when program exit). So it should be able to read from future events, if it doesn't this is a bug.

Yes, you're right. My mind somehow kept old interpretation of what this function does. Yesterday I changed my mind, but today forget it once again, lol.

Got rid of redundant code.

Signed-off-by: Vitalii Levitskii <[email protected]>

biosvs · 2022-08-25T14:39:21Z

Is it possible to rerun check (https://github.com/etcd-io/etcd/actions/runs/2926560942)? Seems like it flaky.

biosvs · 2022-08-26T08:36:07Z

@ahrtr how to you think, is it possible to port it to 3.5.5?

ahrtr · 2022-08-26T09:13:31Z

@ahrtr how to you think, is it possible to port it to 3.5.5?

Although it's an enhancement, but it's also a bug from another perspective, because the gRPC proxy may lose connection to the backend etcd cluster if there are member changes.

I am OK to backport this, but I have no strong opinion on this. What do you think? @serathius @spzala @ptabor

biosvs · 2022-08-29T07:27:18Z

So @ahrtr seems like there are no objections. How it's better to do? Just cherry-pick the commit into release-3.5 branch? Or apply patch with another commit?

ahrtr · 2022-08-29T08:01:10Z

So @ahrtr seems like there are no objections. How it's better to do? Just cherry-pick the commit into release-3.5 branch? Or apply patch with another commit?

Yes, please feel free to deliver a PR for this. I think you need to manually take care of the cherry-pick instead of automatically applying the patch, because there is big difference between release-3.5 and main.

biosvs force-pushed the add-client-auto-sync-to-proxy branch from 493290a to 885cd9e Compare August 16, 2022 14:01

ahrtr reviewed Aug 18, 2022

View reviewed changes

biosvs force-pushed the add-client-auto-sync-to-proxy branch from 885cd9e to 555da87 Compare August 18, 2022 17:00

ahrtr reviewed Aug 18, 2022

View reviewed changes

biosvs force-pushed the add-client-auto-sync-to-proxy branch 2 times, most recently from 380ffb5 to b17814d Compare August 22, 2022 08:56

ahrtr reviewed Aug 23, 2022

View reviewed changes

biosvs force-pushed the add-client-auto-sync-to-proxy branch 2 times, most recently from 3e90b96 to b6303e4 Compare August 25, 2022 09:38

ahrtr approved these changes Aug 25, 2022

View reviewed changes

serathius reviewed Aug 25, 2022

View reviewed changes

Added client-auto-sync-interval argument to the grpc-proxy

be58a25

Signed-off-by: Vitalii Levitskii <[email protected]>

biosvs force-pushed the add-client-auto-sync-to-proxy branch from b6303e4 to be58a25 Compare August 25, 2022 12:33

biosvs requested a review from serathius August 25, 2022 19:50

serathius approved these changes Aug 25, 2022

View reviewed changes

ahrtr merged commit e408285 into etcd-io:main Aug 25, 2022

biosvs deleted the add-client-auto-sync-to-proxy branch August 26, 2022 08:35

This was referenced Aug 29, 2022

Backport of pull/14354 to release-3.5 #14397

Merged

Added member interaction into EtcdProcessCluster #14404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added client-auto-sync-interval argument to the grpc-proxy #14354

Added client-auto-sync-interval argument to the grpc-proxy #14354

biosvs commented Aug 16, 2022

ahrtr Aug 18, 2022

biosvs Aug 18, 2022

ahrtr commented Aug 18, 2022

biosvs commented Aug 18, 2022 •

edited

Loading

ahrtr Aug 18, 2022

ahrtr Aug 18, 2022

biosvs Aug 19, 2022

biosvs Aug 19, 2022

ahrtr Aug 19, 2022

biosvs Aug 22, 2022

ahrtr Aug 23, 2022 •

edited

Loading

biosvs Aug 24, 2022

ahrtr Aug 23, 2022 •

edited

Loading

biosvs Aug 24, 2022

ahrtr Aug 23, 2022

biosvs Aug 24, 2022

ahrtr Aug 23, 2022

biosvs Aug 24, 2022

biosvs commented Aug 25, 2022

ahrtr left a comment

serathius Aug 25, 2022

biosvs Aug 25, 2022

serathius Aug 25, 2022

serathius Aug 25, 2022

serathius Aug 25, 2022

biosvs Aug 25, 2022 •

edited

Loading

biosvs commented Aug 25, 2022

biosvs commented Aug 26, 2022

ahrtr commented Aug 26, 2022

biosvs commented Aug 29, 2022

ahrtr commented Aug 29, 2022

Added client-auto-sync-interval argument to the grpc-proxy #14354

Added client-auto-sync-interval argument to the grpc-proxy #14354

Conversation

biosvs commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Aug 18, 2022

biosvs commented Aug 18, 2022 • edited Loading

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

ahrtr Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Aug 23, 2022 • edited Loading

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

biosvs commented Aug 25, 2022

ahrtr 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

biosvs Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

biosvs commented Aug 25, 2022

biosvs commented Aug 26, 2022

ahrtr commented Aug 26, 2022

biosvs commented Aug 29, 2022

ahrtr commented Aug 29, 2022

biosvs commented Aug 18, 2022 •

edited

Loading

ahrtr Aug 23, 2022 •

edited

Loading

ahrtr Aug 23, 2022 •

edited

Loading

biosvs Aug 25, 2022 •

edited

Loading