[Experimental] Optional gRPC connection sharing among VUs #31

chrismoran-mica · 2023-07-21T17:18:00Z

What?

Add a parameter to connectParams that would allow grpc.ClientConns to be shared across VUs. In order for connections to be shared (within a VU or across VUs) the following must be true:

A connection is eligible to be shared if a Client is connected with { connectionSharing: boolean|integer }
A connection will only be shared with another Client (within a VU or across VUs) if the addr (address) parameter to connect is the same (case-insensitive)
The decision to permit a Clients connection to be shared is made when connect is called

Logically, it would only make sense to share a connection (within a VU or across VUs) if the two Clients in question target an endpoint that either:

Serves the same gRPC service
Multiplexes the gRPC services needed by both Clients

Aside from comparing protoreflect.MethodDescriptors, I cannot think of a way to ensure the Clients should share a connection prior to making the connection and calling invoke (to generate the protoreflect.MethodDescriptor mapping). So it would be left to the test writer to decide if the Clients should share connections.

Why?

This feature has the potential to substantially reduce the resources required for multiple VUs connecting to the same service(s) or endpoint(s). This gives greater flexibility for testing scenarios.

On the other hand, this does blur the line among VUs with connectionSharing enabled on grpc.Clients. There could potentially be unforeseen consequences to sharing connections among VUs. This situation, however, is mitigated by the fact that this feature is opt-in and disabled by default.

TODO / In-process:

Limit the amount of sharing allowed by a single connection (via Latch or other such mechanism) to ensure a single gRPC connection does not exhaust the maximum permitted open channels on the server side
- This would require the client to know at connection time what that maximum is and if not set, use a sane default: (typically 100)
- This could also potentially introduce complicated connection management dealing with multiple shared, open connections across many VUs

Connection sharing limiting

The ConnectionSharing parameter can be either a boolean value OR an integer > 1
- A true value will mean ConnectionSharing is enabled for the Client and the maximum sharing is set to a sane default (100)
  - This further means that the client's underlying connection will be shared with at most (100 - 1) clients
- A false value will mean ConnectionSharing is disabled entirely for that Client
- An integer > 1 will mean ConnectionSharing is enabled for the Client and the underlying connection will be shared at most connectionSharing - 1 number of times before a new connection is made.
- It is important to note that the number of shares a client's connection has depends solely on the value of the connectionSharing parameter exactly when the actual connection was dialed.

Limiting the number of connections can be a complicated concept. Since each client.connect can have a connectionSharing parameter with any arbitrary value for connectionSharing it can quickly become confusing which connections are possibly being shared.

Checklist

Limit the connection sharing amount using a maximum connection sharing amount parameter and/or a sane default
Limit complexity of the connection sharing logic
I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make lint) and all checks pass.
I have run tests locally (make test) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

…nSharing` field of options parameter to `grpc.Client.connect` function

olegbespalov · 2023-08-12T06:02:00Z

Hi @chrismoran-mica !

Thanks for the contribution 👍

Sorry for the long reply, it was vacation season, and after that, some should be done for the upcoming k6 release.

I want to let you know that reviewing this PR is on my list, and I will do that next week.

Cheers!

olegbespalov · 2023-08-18T14:04:37Z

grpc/client.go

+
+	client, ok := v.ToObject(rt).Export().(*Client)
+	if !ok {
+		return false, errors.New("parameter must a 'Client'")


Suggested change

return false, errors.New("parameter must a 'Client'")

return false, errors.New("parameter must be a gRPC 'Client'")

olegbespalov · 2023-08-18T14:07:33Z

grpc/client.go

+func parseConnectConnectionSharingParam(params *connectParams, v interface{}) error {
+	var (
+		ok                    bool
+		connectionSharingBool bool


Out of curiosity, why do you think mixing the param type is beneficial? Why not stick with the number?

Same question 👍🏻

I would intuitively tend to prefer having two separate options: one boolean to activate/deactivate connection sharing and one number to control the maximum sharing.

But I'm happy to be convinced otherwise 🙇🏻

olegbespalov · 2023-08-18T14:18:53Z

lib/netext/grpcext/conn.go

 }

+var (
+	// Is there any other way?


it seems like yes

it seems like we need a new struct like connectionPool (a better name). This struct could be initialized and live in the RootModule
https://github.com/grafana/xk6-grpc/blob/main/grpc/grpc.go#L15-L26 This struct could also take the big part of the management of the connections probably

And pass it to the client via the NewClient
https://github.com/grafana/xk6-grpc/blob/main/grpc/grpc.go#L59-L63

👍🏻 on my side for a RootModule connection pool.

By the way, and to be we're all on the same page as to why we suggest the RootModule, that's because it will be instantiated only once throughout the execution and will be spawning new instances for VUs (hence, one can easily pass a pointer to all instances of the said pool from there).

oleiade · 2023-08-24T07:51:22Z

grpc/client.go

+func parseConnectConnectionSharingParam(params *connectParams, v interface{}) error {
+	var (
+		ok                    bool
+		connectionSharingBool bool


Same question 👍🏻

I would intuitively tend to prefer having two separate options: one boolean to activate/deactivate connection sharing and one number to control the maximum sharing.

But I'm happy to be convinced otherwise 🙇🏻

oleiade · 2023-08-24T07:56:23Z

lib/netext/grpcext/conn.go

@@ -58,9 +61,22 @@ type clientConnCloser interface {

 // Conn is a gRPC client connection.
 type Conn struct {
-	raw clientConnCloser
+	addr   string
+	shares atomic.Uint64


My understanding is that shares represents "how many concurrent clients are using this connection". Is that correct?

I would suggest at the very least, adding a godoc comment on this field to clarify what the atomic number it holds represents, and I can't think of a more explicit name, but if one of us was to come up with one, I think it could be good. When I see "shares", I think "shares of what?" which, I believe, doesn't do justice to what the attribute actually holds, if that makes sense 🤓

oleiade · 2023-08-24T07:58:24Z

lib/netext/grpcext/conn.go

 }

+var (
+	// Is there any other way?


👍🏻 on my side for a RootModule connection pool.

By the way, and to be we're all on the same page as to why we suggest the RootModule, that's because it will be instantiated only once throughout the execution and will be spawning new instances for VUs (hence, one can easily pass a pointer to all instances of the said pool from there).

oleiade · 2023-08-24T08:00:11Z

lib/netext/grpcext/conn.go

+	// Is there any other way?
+
+	//nolint:golint,gochecknoglobals
+	connections = &sync.Map{}


To raise awareness on this, the sync.Map documentation mentions:

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys.

Does the use of it we make here fit those use cases? Otherwise I would suggest a good old map with explicit locking instead 👍🏻

oleiade · 2023-08-24T08:00:27Z

lib/netext/grpcext/conn.go

+	//nolint:golint,gochecknoglobals
+	connections = &sync.Map{}
+	//nolint:golint,gochecknoglobals
+	connectionsAddrMu             = &sync.Map{}


Same comment about sync.Map here 🙇🏻

oleiade · 2023-08-24T08:09:52Z

lib/netext/grpcext/conn.go

+		err         error
+	)
+
+	// Lock for mutating connectionsAddrMu map


I'm somewhat confused as to why we lock here, based on the fact that connectionsAddrMu and connections are sync.Map at the moment, which is designed to be safe for concurrent use without locking.

Could you expand the comment to reflect that information? 🙇🏻

CLAassistant · 2023-09-20T04:19:37Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Chris Moran seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

olegbespalov · 2023-10-20T18:31:27Z

As the CLA wasn't signed, we can't merge this PR even if the comments will be addressed. So we are closing it for now. If, in the future, there will be a need to continue to work on that, we could return to it.

sanymakam · 2024-01-29T16:12:05Z

@chrismoran-mica I have a similar requirement where instead of creating new grpc connection per VU, I want to create a pool of connections and share it with VUs. Can you please help me how to use this in a K6 js file?

Chris Moran added 3 commits July 21, 2023 11:13

[Experimental] Optional grpc connection sharing enabled by `connectio…

29dcbb9

…nSharing` field of options parameter to `grpc.Client.connect` function

Linting + exception

19ad1d3

Attempting to remove timeout so negative test passes

726567f

mstoykov mentioned this pull request Jul 26, 2023

Cross-VU shared grpc.ClientConn. grafana/k6#3179

Closed

8 tasks

andrewslotin requested a review from olegbespalov August 2, 2023 10:19

olegbespalov reviewed Aug 18, 2023

View reviewed changes

olegbespalov requested review from a team and oleiade and removed request for a team August 23, 2023 12:40

oleiade reviewed Aug 24, 2023

View reviewed changes

olegbespalov closed this Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Experimental] Optional gRPC connection sharing among VUs #31

[Experimental] Optional gRPC connection sharing among VUs #31

chrismoran-mica commented Jul 21, 2023

olegbespalov commented Aug 12, 2023

olegbespalov Aug 18, 2023

olegbespalov Aug 18, 2023

oleiade Aug 24, 2023

olegbespalov Aug 18, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

oleiade Aug 24, 2023

CLAassistant commented Sep 20, 2023

olegbespalov commented Oct 20, 2023

sanymakam commented Jan 29, 2024

	return false, errors.New("parameter must a 'Client'")
	return false, errors.New("parameter must be a gRPC 'Client'")

[Experimental] Optional gRPC connection sharing among VUs #31

[Experimental] Optional gRPC connection sharing among VUs #31

Conversation

chrismoran-mica commented Jul 21, 2023

What?

Why?

Connection sharing limiting

Checklist

olegbespalov commented Aug 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

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

CLAassistant commented Sep 20, 2023

olegbespalov commented Oct 20, 2023

sanymakam commented Jan 29, 2024