Add features tests for gRPC retry behavior #435

chronos-tachyon · 2024-03-08T22:39:06Z

What was changed

Implement a small gRPC proxy in Go that can freeze or fail requests on demand
Extend features runner to optionally launch the proxy, if the executable can be found
Add proxy-based retry tests for Go and Java
Minor style cleanups / refactoring of the features runner

Why?

We need tests that verify the client's retry behavior. A comment on features#326 says that @rross reports that sdk-java doesn't retry correctly on the first connection. Indeed, these new tests confirmed that a bug exists in sdk-java's GrpcRetryer, as the serverCapabilities.get() call can throw in a place that the code doesn't anticipate exceptions.

From here, there is some future work to be done to make these tests more generally useful:

CI needs to be modified, to build the proxy executable before running the features tests.
Test implementations for C#, TypeScript, and Python should be written.

Sushisource

This generally makes sense to me and thanks for some of the various cleanup that's in here. My only question is around the need for this proxy to be its own binary/process. We could save a little argparsing and CI code if it's not.

Sushisource · 2024-04-17T21:15:10Z

cmd/run.go

@@ -154,6 +204,22 @@ func (r *Runner) Run(ctx context.Context, patterns []string) error {
 		return err
 	}

+	var fn func(context.Context, *cmd.Run) error


Suggested change

var fn func(context.Context, *cmd.Run) error

var langRunFn func(context.Context, *cmd.Run) error

Sushisource · 2024-04-17T21:24:57Z

internal/cmd/temporal-features-test-proxy/main.go

@@ -0,0 +1,752 @@
+package main


It's not immediately obvious to me why this guy needs to be his own executable? Seemingly the runner could start this up internally rather than needing to start it as a subprocess?

Concur. @chronos-tachyon - take a peek at what I recently had to do with HTTP proxying: #448 (ignore the subprocess stuff which was because I had to test environment variables safely and unrelated to the HTTP proxy part). Some notes:

Can have a feature's config opt-in to the proxy so if someone runs a specific feature without needing a proxy it's not started

Can name it "tcp proxy" to disambiguate from http proxy (note, I called the other "http proxy" to avoid this confusion too)

Can make a TCPProxy utility as a Go library that the command just happens to use. This can help properly encapsulate the tool (even though it'll only ever be used by the cmd) instead of using globals and subprocesses and such

Features know if they need a proxy, so they can choose to, say, dial to that address instead of the default one. No need to affect existing running features by making them go through the proxy.

I couldn't do it in-process because, when I did, there were too many CPU-bound goroutines to allow the proxy server's goroutines to get serviced in a timely manner (delays on the order of 20 seconds).

That is very surprising. We test the HTTP proxy embedded in the main process (not the per-lang worker process) without any such delays. I wonder what is causing such delays. We shouldn't really have many CPU-bound goroutines here except for the case of running Go features embedded (every other feature type of run uses external processes to run the features). Also, can we consider only using the TCP proxy for features that need it (i.e. they can connect different clients to the proxy inside their tests instead of using default client)?

Sushisource · 2024-04-17T21:26:00Z

internal/cmd/temporal-features-test-proxy/main.go

+- POST /reject
+  Immediately reject incoming gRPC requests with UNAVAILABLE.
+
+- POST /accept
+  Accept incoming gRPC requests; this is the default.
+
+- POST /freeze
+  Block on incoming accepted gRPC requests.
+
+- POST /thaw
+  Process incoming accepted gRPC requests immediately; this is the default.


I assume posting to these switches to the corresponding "mode"? Might be worth mentioning this modality concept explicitly.

Sushisource · 2024-04-17T21:29:09Z

internal/cmd/temporal-features-test-proxy/main.go

+		}
+	}()
+
+	Info("HTTP control server is running on: %s", flagControl)


Nit: If user passes 127.0.0.1:0 this would show 0 rather than the bound port, yes?

Not really a problem since we're predetermining available ports and using those, but thought I'd mention it since this can be run directly in theory.

When this is made a library as part of a runner, we should let this listener pick the random port and extract the address out of it instead temporary-listener-free-port approach (which is a good approach in general, but won't be needed when this is not a separate process).

Sushisource · 2024-04-17T21:31:37Z

internal/cmd/temporal-features-test-proxy/main.go

+func HandleHelp(w http.ResponseWriter, r *http.Request) {
+	if path.Clean(r.URL.Path) != "/" {


Sushisource · 2024-04-17T21:34:48Z

internal/cmd/temporal-features-test-proxy/main.go

+var (
+	flagTrace   bool


A bit in the same vein as asking why this is its own binary, it might be nice to move all of these into some FeaturesProxyServer struct or something

Sushisource · 2024-04-17T21:40:32Z

internal/cmd/temporal-features-test-proxy/main.go

+	s.gc = nil
+	s.gs = nil
+	s.l = nil
+	s.wc = nil
+	s.ws = nil
+	s.quitCh = nil


Seems like it might be easy to miss any future fields here. I'm wondering if just having three top-level fields: The mutex, condvar, and an "everything else" container might be nice.

It could also potentially let you have some generic code for this pattern that the servers can share.

cretz

All makes sense. High-level notes on the separate proxy process.

cretz · 2024-04-18T12:34:47Z

cmd/run.go

@@ -154,6 +204,22 @@ func (r *Runner) Run(ctx context.Context, patterns []string) error {
 		return err
 	}

+	var fn func(context.Context, *cmd.Run) error


No disagreement with extracting switch case content into methods, but why move the switch up here? If it's all the same, can just leave switch where it is and make the actual invocations in each case

cretz · 2024-04-18T12:40:05Z

internal/cmd/temporal-features-test-proxy/main.go

@@ -0,0 +1,752 @@
+package main


Concur. @chronos-tachyon - take a peek at what I recently had to do with HTTP proxying: #448 (ignore the subprocess stuff which was because I had to test environment variables safely and unrelated to the HTTP proxy part). Some notes:

Can have a feature's config opt-in to the proxy so if someone runs a specific feature without needing a proxy it's not started

Can name it "tcp proxy" to disambiguate from http proxy (note, I called the other "http proxy" to avoid this confusion too)

Can make a TCPProxy utility as a Go library that the command just happens to use. This can help properly encapsulate the tool (even though it'll only ever be used by the cmd) instead of using globals and subprocesses and such

Features know if they need a proxy, so they can choose to, say, dial to that address instead of the default one. No need to affect existing running features by making them go through the proxy.

cretz · 2024-04-18T12:43:36Z

cmd/run.go

@@ -221,6 +287,49 @@ func (r *Runner) Run(ctx context.Context, patterns []string) error {
 			return err
 		}
 	}
+	r.config.DirectServer = r.config.Server


IMO the direct server should be the server and the proxy can be provided for features that know they need it (and the features can create clients to the proxy that know they need to test with the proxy)

cretz · 2024-04-18T12:44:57Z

internal/cmd/temporal-features-test-proxy/main.go

+func (s *ControlServer) serveThread() {
+	defer s.finish()
+
+	err := s.server.Serve(s.l)


Big fan of using HTTP here. I wish when the "summary server" thing was done HTTP was used there instead of homemade Temporal binary protocol, heh.

cretz · 2024-04-18T12:47:38Z

internal/cmd/temporal-features-test-proxy/main.go

+		}
+	}()
+
+	Info("HTTP control server is running on: %s", flagControl)


When this is made a library as part of a runner, we should let this listener pick the random port and extract the address out of it instead temporary-listener-free-port approach (which is a good approach in general, but won't be needed when this is not a separate process).

chronos-tachyon added 13 commits March 8, 2024 14:32

Style cleanups.

ae566cf

Implement a TCP proxy that can freeze or kill connections on demand.

c1543ab

Use HTTP for control, launch proxy if available.

8bed26e

Merge remote-tracking branch 'origin/main' into dking/SDK-1742

9fe2d08

Add --proxy-control-uri flags to all per-language harnesses.

f9bda3d

Fix Python formatting.

23b7d2a

Fix Java formatting.

95517a2

Add tests for Go.

9d85421

Switch from raw TCP to gRPC.

c72f203

Fix Python formatting.

a4bc8b4

Add --summary-uri flag to all per-lang feature test harnesses.

b67044a

Fix null pointer deref.

9819b4d

Use '--flag=value' instead of '--flag value'.

c5777c0

chronos-tachyon marked this pull request as ready for review March 28, 2024 17:23

chronos-tachyon mentioned this pull request Mar 28, 2024

Fix exception in GrpcRetryer. temporalio/sdk-java#2021

Merged

chronos-tachyon requested review from Quinn-With-Two-Ns, Sushisource and cretz March 28, 2024 20:17

Sushisource reviewed Apr 17, 2024

View reviewed changes

cretz reviewed Apr 18, 2024

View reviewed changes

chronos-tachyon closed this May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add features tests for gRPC retry behavior #435

Add features tests for gRPC retry behavior #435

chronos-tachyon commented Mar 8, 2024 •

edited

Loading

Sushisource left a comment

Sushisource Apr 17, 2024

Sushisource Apr 17, 2024

cretz Apr 18, 2024

chronos-tachyon Apr 18, 2024

cretz Apr 19, 2024 •

edited

Loading

Sushisource Apr 17, 2024

Sushisource Apr 17, 2024

cretz Apr 18, 2024

Sushisource Apr 17, 2024

Sushisource Apr 17, 2024

Sushisource Apr 17, 2024

cretz left a comment

cretz Apr 18, 2024

cretz Apr 18, 2024

cretz Apr 18, 2024

cretz Apr 18, 2024

cretz Apr 18, 2024

	var fn func(context.Context, *cmd.Run) error
	var langRunFn func(context.Context, *cmd.Run) error

		func HandleHelp(w http.ResponseWriter, r *http.Request) {
		if path.Clean(r.URL.Path) != "/" {

Add features tests for gRPC retry behavior #435

Add features tests for gRPC retry behavior #435

Conversation

chronos-tachyon commented Mar 8, 2024 • edited Loading

What was changed

Why?

Sushisource 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

cretz Apr 19, 2024 • 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

Choose a reason for hiding this comment

cretz 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

chronos-tachyon commented Mar 8, 2024 •

edited

Loading

cretz Apr 19, 2024 •

edited

Loading