Improve rpc_soak and channel_soak test to cover concurrency in Go #7926

zbilun · 2024-12-12T09:24:18Z

RELEASE NOTES:

Improved the rpc_soak and channel_soak tests to support concurrency in Go. This change increases the test coverage of the C2P E2E load test and follows a similar approach used in the Java implementation.

This PR aims to enhance the test coverage of the C2P E2E load test by improving the rpc_soak and channel_soak tests to support concurrency in Go. The updated logic closely follows the approach used in the Java implementation, which has already been merged.

rpc_soak:
The client performs many large_unary RPCs in sequence over the same channel. The test can run in either a concurrent or non-concurrent mode, depending on the number of threads specified (soak_num_threads).

channel_soak:
Similar to rpc_soak, but this time each RPC is performed on a new channel. The channel is created just before each RPC and is destroyed just after.

Note on Concurrent Execution and Channel Creation:
In a concurrent execution setting (i.e., when soak_num_threads > 1), each thread performs a portion of the total soak_iterations and creates and destroys its own channel for each RPC iteration.

Thread-specific logs will include the thread_id, helping to track performance across threads, especially when each thread is managing its own channel lifecycle.

@townba
@dfawley

…for Go.

codecov · 2024-12-12T09:29:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.04%. Comparing base (66ba4b2) to head (895af05).
Report is 11 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7926   +/-   ##
=======================================
  Coverage   82.04%   82.04%           
=======================================
  Files         377      379    +2     
  Lines       38180    38261   +81     
=======================================
+ Hits        31326    31393   +67     
- Misses       5551     5562   +11     
- Partials     1303     1306    +3

see 44 files with indirect coverage changes

interop/test_utils.go

.gitignore

interop/client/client.go

dfawley · 2024-12-16T21:16:07Z

interop/client/client.go

@@ -358,10 +359,16 @@ func main() {
 		interop.DoPickFirstUnary(ctx, tc)
 		logger.Infoln("PickFirstUnary done")
 	case "rpc_soak":
-		interop.DoSoakTest(ctxWithDeadline, tc, serverAddr, opts, false /* resetChannel */, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize, time.Duration(*soakPerIterationMaxAcceptableLatencyMs)*time.Millisecond, time.Duration(*soakMinTimeMsBetweenRPCs)*time.Millisecond)
+		interop.DoSoakTest(ctxWithDeadline, conn, serverAddr, *numThreads, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize,


Please make a struct for passing all this configuration data (long overdue & not your fault!), instead of having so many parameters, many of which are ints that one could easily get the order wrong when calling.

interop/test_utils.go

dfawley · 2024-12-16T21:19:35Z

interop/test_utils.go

+	soakResponseSize int,
+	copts []grpc.CallOption) (SoakIterationResult, error) {
+	start := time.Now()
+	var err error


Unless I'm missing something, this doesn't need to be pre-declared.

interop/test_utils.go

dfawley · 2024-12-16T21:21:23Z

interop/test_utils.go

+	mu *sync.Mutex,
+	sharedChannel *grpc.ClientConn,
+	threadID int,
+	MayCreateNewChannel ManagedChannel) {


As above, but 3x worse. :)

interop/test_utils.go

interop/xds_federation/client.go

dfawley · 2024-12-16T21:27:58Z

interop/test_utils.go

+// SoakIterationResult represents the latency and status results of a single iteration in the soak test.
+type SoakIterationResult struct {
+	LatencyMs int64
+	Status    string // The status of the iteration


Does this really want to be a string? Or a grpc codes.Code? Or *status.Status? Or just error? We can talk about what makes the most sense if you aren't sure, but I'm doubtful we want a string.

Currently, we have In the merged Java version, we have the status field as a string, where statuses like "OK" are used. To maintain consistency across the implementations, we have kept the status field as a string in this Go version as well. However, I'm open to further discussion on this.

What are we doing with this anyway? I can't see where it's read.

dfawley

Most of my comments are actually pretty superficial. Thanks for all the cleanups since the first round!

.gitignore

dfawley · 2024-12-20T17:54:49Z

interop/client/client.go

+			SoakResponseSize:                 *soakResponseSize,
+			PerIterationMaxAcceptableLatency: time.Duration(*soakPerIterationMaxAcceptableLatencyMs) * time.Millisecond,
+			MinTimeBetweenRPCs:               time.Duration(*soakMinTimeMsBetweenRPCs) * time.Millisecond,
+			OverallTimeoutSeconds:            *soakOverallTimeoutSeconds,


Can this be a Duration as well?

dfawley · 2024-12-20T17:55:24Z

interop/client/client.go

 		logger.Infoln("RpcSoak done")
 	case "channel_soak":
-		interop.DoSoakTest(ctxWithDeadline, tc, serverAddr, opts, true /* resetChannel */, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize, time.Duration(*soakPerIterationMaxAcceptableLatencyMs)*time.Millisecond, time.Duration(*soakMinTimeMsBetweenRPCs)*time.Millisecond)
+		channelSoakConfig := interop.SoakTestConfig{


This seems to be almost identical to the above. Can we share some code more for common config?

dfawley · 2024-12-20T17:58:00Z

interop/client/client.go

+			SharedChannel:                    conn,
+			MayCreateNewChannel: func(currentChannel *grpc.ClientConn) (*grpc.ClientConn, testgrpc.TestServiceClient) {


This feels awkward. How about just a ChannelForTest function that is a closure returning conn in the shared case and creates a new connection in the non-shared case. And the closure can return a cleanup function that is a nop in the shared case, or closes the connection in the non-shared case.

dfawley · 2024-12-20T17:58:59Z

interop/test_utils.go

+// SoakIterationResult represents the latency and status results of a single iteration in the soak test.
+type SoakIterationResult struct {
+	LatencyMs int64
+	Status    string // The status of the iteration


What are we doing with this anyway? I can't see where it's read.

dfawley · 2024-12-20T18:04:56Z

interop/client/client.go

@@ -79,6 +79,7 @@ var (
 	soakMinTimeMsBetweenRPCs               = flag.Int("soak_min_time_ms_between_rpcs", 0, "The minimum time in milliseconds between consecutive RPCs in a soak test (rpc_soak or channel_soak), useful for limiting QPS")
 	soakRequestSize                        = flag.Int("soak_request_size", 271828, "The request size in a soak RPC. The default value is set based on the interop large unary test case.")
 	soakResponseSize                       = flag.Int("soak_response_size", 314159, "The response size in a soak RPC. The default value is set based on the interop large unary test case.")
+	soakNumThreads                         = flag.Int("soak_num_threads", 1, "The number of threads for concurrent execution of the soak tests (rpc_soak or channel_soak). The default value is set based on the interop large unary test case.")


Note that Go doesn't really concern itself with threads, so this is wrong in the context of Go. You probably can't change this because it has cross-language implications, but this should have been thought out more and reviewed by more folks before implementation started.

dfawley · 2024-12-20T18:06:02Z

interop/test_utils.go

+
+// SoakTestConfig holds the configuration for the entire soak test.
+type SoakTestConfig struct {
+	SoakRequestSize                  int


This struct also has a lot of redundant Soaks.

dfawley · 2024-12-20T18:07:25Z

interop/test_utils.go

-	}
-	h := stats.NewHistogram(hopts)
-	for i := 0; i < soakIterations; i++ {
+func executeSoakTestInThread(ctx context.Context, config SoakTestConfig, startNs int64, threadID int, threadResults *ThreadResults, mu *sync.Mutex) {


Maybe we can at least remove "thread" from any Go code, even if we have to support a flag with that name?

dfawley · 2024-12-20T18:08:11Z

interop/test_utils.go

 		}
-		earliestNextStart := time.After(minTimeBetweenRPCs)
-		iterationsDone++
+		if time.Since(time.Unix(0, startNs)) >= timeoutDuration {


Can we pass around startNs as a time.Time instead, please?

dfawley · 2024-12-20T18:09:22Z

interop/test_utils.go

-			}
-			fmt.Fprintf(os.Stderr, "soak iteration: %d elapsed_ms: %d peer: %s server_uri: %s failed: %s\n", i, latencyMs, addrStr, serverAddr, err)
+			fmt.Fprintf(os.Stderr, "Thread %d: soak iteration: %d elapsed_ms: %d peer: %v server_uri: %s failed: %s\n", threadID, i, 0, p.Addr, config.ServerAddr, err)
+			mu.Lock()


Each goroutine should have exclusive access to its results struct. So this shouldn't need a lock.

Or just make each instance of this function return a results struct instead of mutating one that's stored elsewhere -- that would likely be a cleaner approach.

zbilun added 2 commits December 12, 2024 00:58

Improve the rpc soak test and channel soak test to cover concurrency …

fd83eee

…for Go.

Remove .idea/ directory from Git tracking

0bbe38b

zbilun added 5 commits December 12, 2024 01:33

Fix the style issue

219e17e

Remove .idea/ from .gitignore

ead030a

Add .gitignore file

cbddddc

fix channel close issue

be47357

Stop tracking .idea directory

bfd1b8e

townba reviewed Dec 13, 2024

View reviewed changes

Fix the comments style.

3929ecf

purnesh42H self-assigned this Dec 16, 2024

purnesh42H added Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. Type: Internal Cleanup Refactors, etc labels Dec 16, 2024

dfawley reviewed Dec 16, 2024

View reviewed changes

purnesh42H assigned dfawley and zbilun and unassigned purnesh42H, dfawley and zbilun Dec 17, 2024

zbilun added 7 commits December 16, 2024 23:18

Fix the go style issues.

742ed86

Fix the go style issues about the space.

9fdcd56

Debug for test failure.

b718300

Replace long parameter lists with struct for cleaner code.

2df82f8

Clean code by deleting useless comments.

2d72173

Fix the test fail

a0a3ae2

Fix the format check

957c1b4

zbilun added 9 commits December 17, 2024 15:09

Fix the p.addr

67948ce

Change the addrStr back due to the print type issue.

f03b6fe

Clean comments.

f621f31

Add print message.

2204ae8

Debug the test fail

ca86c16

Duplicate print error

22c5544

Refactor the doSoakTest func.

04282c5

Clean comments.

959c594

Clean empty line.

895af05

dfawley self-assigned this Dec 18, 2024

dfawley reviewed Dec 20, 2024

View reviewed changes

dfawley assigned zbilun and unassigned dfawley Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rpc_soak and channel_soak test to cover concurrency in Go #7926

Improve rpc_soak and channel_soak test to cover concurrency in Go #7926

zbilun commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

dfawley Dec 16, 2024

dfawley Dec 16, 2024

dfawley Dec 16, 2024

dfawley Dec 16, 2024

zbilun Dec 17, 2024

dfawley Dec 20, 2024

dfawley left a comment

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

dfawley Dec 20, 2024

		SharedChannel: conn,
		MayCreateNewChannel: func(currentChannel grpc.ClientConn) (grpc.ClientConn, testgrpc.TestServiceClient) {

Improve rpc_soak and channel_soak test to cover concurrency in Go #7926

Are you sure you want to change the base?

Improve rpc_soak and channel_soak test to cover concurrency in Go #7926

Conversation

zbilun commented Dec 12, 2024 • edited Loading

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

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

dfawley 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

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

zbilun commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading