GODRIVER-1931 Run tests against LBs in Evergreen #648

divjotarora · 2021-04-23T23:20:49Z

This PR contains the changes needed to run load balancers in Evergreen and some updates to the mtest packakge to handle load balancer URIs. I also added a load_balancer_prose_test.go file to fill in some missing test coverage. There were a couple of missing/untested operations in the unified test runner so there are some changes required for that as well.

I called out some notable changes in PR comments, but there are additional changes that span multiple files:

There is currently no upstream service (e.g. mongos) that supports running behind an LB. To test against a sharded cluster without waiting for upstream support, this PR introduces a SetServiceID flag in internal/const.go. This flag is used in description/server.go when parsing hello command responses and enables a mode in which we mock the server returning serviceId. It's set to true in mongo/integration/main_test.go and mongo/integration/unified/main_test.go if required.
I realized there are some cases where we need to skip a test in the unified test runner, but don't become aware of this until after we've started constructing entities or running operations. We could account for these tests in skippedTestDescriptions, but I thought it'd be better to add a more robust approach in which tests execution functions can return a special error type that is checked at a higher level, so I added the skipTestError type in unified_spec_runner.go.

divjotarora · 2021-04-23T23:43:51Z

mongo/integration/mtest/setup.go

+	dataLake                    bool
+	requireAPIVersion           bool
+	serverParameters            bson.Raw
+	singleMongosLoadBalancerURI string


The only changes were the addition of singleMongosLoadBalancerURI and multiMongosLoadBalancerURI. The diff looks larger due to auto-alignment by gofmt.

divjotarora · 2021-04-23T23:44:01Z

mongo/integration/mtest/setup.go

-// ConnString gets the globally configured connection string.
-func getConnString() (connstring.ConnString, error) {
+// getClusterConnString gets the globally configured connection string.
+func getClusterConnString() (connstring.ConnString, error) {


Name changed for clarity.

divjotarora · 2021-04-23T23:46:44Z

mongo/integration/mtest/setup.go

@@ -186,6 +188,20 @@ func Setup(setupOpts ...*SetupOptions) error {
 		}
 	}

+	if testContext.topoKind == LoadBalanced {


This change is due to a spec/setup requirement - when mongo-orchestration starts up a cluster, it provides a URI that includes authentication information (if needed) but does not include TLS information (e.g. CA file path). We normally work around this by manually modifying the cluster URI, which is done in getClusterConnString(), which internally calls addTLSConfig(). For LB testing, we have to make the same modification for the URIs of the LBs themselves as well, so we do all of that here and then store the processed URI in testContext.

divjotarora · 2021-04-23T23:48:23Z

mongo/integration/unified/client_entity.go

@@ -126,26 +122,19 @@ func newClientEntity(ctx context.Context, em *EntityMap, entityOptions *entityOp
 	return entity, nil
 }

-func getURIForClient(opts *entityOptions) (string, error) {
+func getURIForClient(opts *entityOptions) string {


This function originally returned an error because it would dynamically fetch the LB URI from an environment variable, which could be empty. The error is no longer needed because the env var checking is done in mtest#Setup so we can fetch the required URI via the mtest functions here.

divjotarora · 2021-04-23T23:49:14Z

mongo/integration/unified/client_entity.go

@@ -344,8 +333,14 @@ func setClientOptionsFromURIOptions(clientOpts *options.ClientOptions, uriOpts b

 	for key, value := range uriOpts {
 		switch key {
+		case "appname":


All additions here are due to missing client options that were not required by previous tests.

divjotarora · 2021-04-23T23:49:33Z

mongo/integration/unified/collection_operation_execution.go

@@ -748,6 +748,38 @@ func executeInsertOne(ctx context.Context, operation *operation) (*operationResu
 	return newDocumentResult(raw, err), nil
 }

+func executeListIndexes(ctx context.Context, operation *operation) (*operationResult, error) {


This was previously missed because no other test required it.

divjotarora · 2021-04-23T23:49:59Z

mongo/integration/unified/database_operation_execution.go

@@ -89,7 +89,7 @@ func executeListCollections(ctx context.Context, operation *operation) (*operati
 	defer cursor.Close(ctx)

 	var docs []bson.Raw
-	if err := cursor.All(ctx, &cursor); err != nil {
+	if err := cursor.All(ctx, &docs); err != nil {


This was a bug but there was no unified test that executed listCollections.

mongo/integration/unified/main_test.go

divjotarora · 2021-04-26T15:04:16Z

.evergreen/config.yml

@@ -518,6 +518,33 @@ functions:
          PKG_CONFIG_PATH=$PKG_CONFIG_PATH \
          LD_LIBRARY_PATH=$LD_LIBRARY_PATH

+  run-load-balancer-tests:


The script here is mostly a copy/paste of the run-tests function.

divjotarora · 2021-04-26T15:06:54Z

.evergreen/config.yml

+
+          export GOFLAGS=-mod=vendor
+          set +o xtrace
+          AUTH="${AUTH}" \


Notable change in env var settings between this invocation and run-tests: there are no CSFLE env vars set here (e.g. AWS_ACCESS_KEY_ID) because the CSFLE tests are not run against LBs.

divjotarora · 2021-04-26T16:44:44Z

.evergreen/config.yml

+              fi
+          fi
+
+          # Verify that the required LB URI env vars are set to ensure that the test runner can correctly connect to


We check for empty LB URIs both here and in mongo/integration/mtest/setup.go. This might be redundant, but it accounts for both running in CI and locally. If we only checked in setup.go, we could have an issue where SINGLE_MONGOS_LB_URI is unset, so MONGODB_URI would also be unset. In this case, the setup function defaults to localhost:27017 and based on topology discovery, thinks it's connecting to a mongos rather than an LB cluster, so important tests that only run against LBs would be incorrectly skipped. Having the check in both places prevents that from happening.

Note that this may not be required once an upstream service fully supports running behind an LB because the service should error if it's configured in LB mode but the driver connects without loadBalanced=true.

Nice, the extra check to avoid accidentally skipping these tests is a good precaution.

Note that this may not be required once an upstream service fully supports running behind an LB because the service should error if it's configured in LB mode but the driver connects without loadBalanced=true.

Right, since drivers send loadBalanced: true as part of the hello command, the server could reject non load balanced connections if it knows it is running behind a load balancer.

divjotarora · 2021-04-26T16:45:17Z

.evergreen/config.yml

@@ -866,6 +922,7 @@ post:
      files:
        - "src/go.mongodb.org/mongo-driver/*.suite"
  - func: upload-mo-artifacts
+  - func: stop-load-balancer


Added as a post task to ensure that the LB is shut down even if the tests fail.

divjotarora · 2021-04-26T16:46:37Z

mongo/integration/unified/client_entity.go

@@ -355,6 +350,8 @@ func setClientOptionsFromURIOptions(clientOpts *options.ClientOptions, uriOpts b
 		case "w":
 			wc.W = value
 			wcSet = true
+		case "waitQueueTimeoutMS":
+			return newSkipTestError("the waitQueueTimeoutMS client option is not supported")


This is an example of dynamically detecting that the test needs to be skipped as mentioned in the PR description.

Out of curiosity, why do we not support WaitQueueTimeoutMS in Go ?

The CMAP spec states that supporting waitQueueTimeoutMS is optional and not required if the language already has an idiomatic timeout mechanism. Go has contexts, so we don't support the option.

kevinAlbs

Looking great!

internal/const.go

.evergreen/config.yml

kevinAlbs · 2021-04-27T18:28:19Z

.evergreen/config.yml

+              fi
+          fi
+
+          # Verify that the required LB URI env vars are set to ensure that the test runner can correctly connect to


Nice, the extra check to avoid accidentally skipping these tests is a good precaution.

Note that this may not be required once an upstream service fully supports running behind an LB because the service should error if it's configured in LB mode but the driver connects without loadBalanced=true.

Right, since drivers send loadBalanced: true as part of the hello command, the server could reject non load balanced connections if it knows it is running behind a load balancer.

.evergreen/config.yml

Makefile

mongo/integration/load_balancer_prose_test.go

Co-authored-by: Kevin Albertson <[email protected]>

benjirewis

Just some questions/nits! I don't really understand SetMockServiceID so I'll ask offline.

mongo/integration/load_balancer_prose_test.go

mongo/integration/main_test.go

benjirewis · 2021-04-27T20:03:54Z

mongo/integration/unified/client_entity.go

@@ -355,6 +350,8 @@ func setClientOptionsFromURIOptions(clientOpts *options.ClientOptions, uriOpts b
 		case "w":
 			wc.W = value
 			wcSet = true
+		case "waitQueueTimeoutMS":
+			return newSkipTestError("the waitQueueTimeoutMS client option is not supported")


Out of curiosity, why do we not support WaitQueueTimeoutMS in Go ?

mongo/integration/unified/main_test.go

Co-authored-by: Benjamin Rewis <[email protected]>

iwysiu

Looks good! Just a quick question.

internal/const.go

benjirewis

LGTM!

kevinAlbs

LGTM!

Makefile

* 'master' of https://github.com/mongodb/mongo-go-driver: (39 commits) GODRIVER-2004 Add Versioned API connection examples for Docs (mongodb#665) GODRIVER-1961 Run OCSP tests against RHEL 7.0 (mongodb#664) GODRIVER-1844 finer precision for getSecondsSinceEpoch (mongodb#666) GODRIVER-1973 create internal copy of aws v4 signing code (mongodb#657) GODRIVER-1951 Update the Go version for Evergreen builds to 1.16 (mongodb#663) GODRIVER-1949 add more ignored killAllSessions errors for unified tes… (mongodb#658) GODRIVER-1963 remove dropDatabase result (mongodb#660) GODRIVER-1180 Remove legacy transform functions from mongo (mongodb#583) GODRIVER-1937 Update legacy ListCollections to support the BatchSize option for server version 2.6 (mongodb#656) GODRIVER-1933 remove xtrace from shell scripts (mongodb#661) fix README error handling of FindOne (mongodb#636) GODRIVER-1938 update mongocryptd serverSelectionTimeout to 10 seconds (mongodb#659) GODRIVER-1925 Surface cursor errors in DownloadStream fillBuffer (mongodb#653) GODRIVER-1955 create labeledError interface (mongodb#651) GODRIVER-1947 Unmarshal unicode surrogate pairs correctly in UnmarshalExtJSON. (mongodb#649) Changed order of actions in ObjectIDFromHex func (mongodb#637) GODRIVER-1750 Ensure contexts are always cancelled during server monitoring (mongodb#654) GODRIVER-1931 Sync new cursors and SDAM LB tests (mongodb#655) GODRIVER-1981 Sync new transactions tests (mongodb#652) GODRIVER-1931 Run tests against LBs in Evergreen (mongodb#648) ...

divjotarora requested review from kevinAlbs, benjirewis and iwysiu April 23, 2021 23:21

divjotarora force-pushed the godriver1931-evg-work branch from 983cc68 to aa89353 Compare April 23, 2021 23:37

divjotarora commented Apr 23, 2021

View reviewed changes

divjotarora commented Apr 26, 2021

View reviewed changes

divjotarora force-pushed the godriver1931-evg-work branch 2 times, most recently from 4395ef7 to 986dd67 Compare April 26, 2021 15:33

divjotarora commented Apr 26, 2021

View reviewed changes

GODRIVER-1931 Run tests against LBs in Evergreen

b753c20

divjotarora force-pushed the godriver1931-evg-work branch from be21ade to b753c20 Compare April 26, 2021 22:03

add prose tests

7874a68

divjotarora force-pushed the godriver1931-evg-work branch from 8ba56a8 to 7874a68 Compare April 26, 2021 23:10

kevinAlbs reviewed Apr 27, 2021

View reviewed changes

Divjot Arora and others added 2 commits April 27, 2021 15:26

kevin's comments

7d5a555

Update .evergreen/config.yml

6831ec9

Co-authored-by: Kevin Albertson <[email protected]>

divjotarora requested a review from kevinAlbs April 27, 2021 19:27

benjirewis reviewed Apr 27, 2021

View reviewed changes

Divjot Arora and others added 2 commits April 27, 2021 16:18

Update mongo/integration/main_test.go

b692b87

Co-authored-by: Benjamin Rewis <[email protected]>

Update mongo/integration/unified/main_test.go

a3db3bd

Co-authored-by: Benjamin Rewis <[email protected]>

divjotarora requested a review from benjirewis April 27, 2021 20:20

iwysiu reviewed Apr 27, 2021

View reviewed changes

internal/const.go Show resolved Hide resolved

iwysiu approved these changes Apr 27, 2021

View reviewed changes

benjirewis approved these changes Apr 27, 2021

View reviewed changes

kevinAlbs approved these changes Apr 28, 2021

View reviewed changes

Makefile Show resolved Hide resolved

divjotarora merged commit b1d1e30 into mongodb:master Apr 28, 2021

divjotarora deleted the godriver1931-evg-work branch April 28, 2021 15:43

faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022

GODRIVER-1931 Run tests against LBs in Evergreen (mongodb#648)

94b628a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-1931 Run tests against LBs in Evergreen #648

GODRIVER-1931 Run tests against LBs in Evergreen #648

divjotarora commented Apr 23, 2021 •

edited

Loading

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 23, 2021

divjotarora Apr 26, 2021

divjotarora Apr 26, 2021

divjotarora Apr 26, 2021

kevinAlbs Apr 27, 2021

divjotarora Apr 26, 2021

divjotarora Apr 26, 2021

benjirewis Apr 27, 2021

divjotarora Apr 27, 2021

kevinAlbs left a comment

kevinAlbs Apr 27, 2021

benjirewis left a comment

benjirewis Apr 27, 2021

iwysiu left a comment

benjirewis left a comment

kevinAlbs left a comment

GODRIVER-1931 Run tests against LBs in Evergreen #648

GODRIVER-1931 Run tests against LBs in Evergreen #648

Conversation

divjotarora commented Apr 23, 2021 • 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

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

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

divjotarora commented Apr 23, 2021 •

edited

Loading