CDRIVER-4062 test load balancer on evg #837

kevinAlbs · 2021-07-30T02:20:08Z

Scope

Add tasks to the Evergreen configuration based to test against a loadbalanced cluster.
- Tests with auth+SSL and with no auth+SSL on both server version 5.0 and latest.
- Runs tests against: new load balanced tests, all unified tests, retryable reads, retryable writes, change streams, and the C driver specific /loadbalanced/* tests.
Update test-libmongoc to support testing against a load balancer.
- Support the environment variable MONGOC_TEST_LOADBALANCED to enable testing against load balancers in test-libmongoc.
- Support the environment variables SINGLE_MONGOS_LB_URI, MULTI_MONGOS_LB_URI to set URIs for test clients.
- Mock the serviceId when MONGOC_TEST_LOADBALANCED is true.
- Support multiple occurrences of the -l flag in test-libmongoc and alias the flag as --match.
- Remove unnecessary constraint of retryable writes tests for replica sets.
CDRIVER-4060 add remaining support to the unified test runner for the new loadbalancer spec tests.
- Iterate the first result of a FindCursor in the createFindCursor operation.
- Support appname as a URI parameter.
- Skip listening or comparing CMAP (Connection Monitoring and Pooling) events, since the C driver does not implement CMAP (per CDRIVER-3525).

Notes for reviewers

73 of the files changed are from updating the JSON files from the specifications repo. Those were reviewed when they were added to the mongodb/specifications repo, so there is no need to review them. The "File Filter" in the review can collapse all of the JSON files.

kevinAlbs · 2021-08-04T15:49:18Z

.evergreen/config.yml

@@ -440,6 +440,9 @@ functions:
          export MONGOC_TEST_GCP_EMAIL="${client_side_encryption_gcp_email}"
          export MONGOC_TEST_GCP_PRIVATEKEY="${client_side_encryption_gcp_privatekey}"
        fi


Note the Evergreen config file is generated from Python scripts. The relevant changes are in tasks.py, functions.py, and variants.py.

kevinAlbs · 2021-08-04T15:50:21Z

.evergreen/download-mongodb.sh

@@ -51,11 +51,11 @@ get_mongodb_download_url_for ()
   _DISTRO=$1
   _VERSION=$2



This is an updated copy of the file in mongodb-labs/drivers-evergreen-tools.

kevinAlbs · 2021-08-04T16:03:10Z

build/evergreen_config_lib/variants.py

@@ -657,7 +657,7 @@ def days(n):
    ], {}, batchtime=days(7)),
    Variant ('packaging', 'Linux Distro Packaging', 'ubuntu1804-test', [
        'debian-package-build',
-        OD([('name', 'rpm-package-build'), ('distros', ['rhel80-test'])]),
+        OD([('name', 'rpm-package-build'), ('distros', ['rhel82-arm64-small'])]),


Drive-by fix. The config.yml file was manually updated in CDRIVER-4112 without the python generation files being updated.

kevinAlbs · 2021-08-04T16:05:37Z

src/libmongoc/src/mongoc/mongoc-cluster.c

@@ -2321,6 +2321,7 @@ _mongoc_cluster_stream_for_server (mongoc_cluster_t *cluster,
         mongoc_server_stream_cleanup (server_stream);
         mongoc_cluster_disconnect_node (cluster, server_id);
         bson_mutex_unlock (&topology->mutex);
+         _mongoc_bson_init_if_set (reply);


This is a bug fix. If a reply argument is passed, callers expect it to always be initialized (even on error).

I assume no ticket is needed for this since it's fixing a bug from a previous PR that was never released. Correct?

Correct, this was introduced in CDRIVER-4056: 4919d4f#diff-7723202fa2b19410a0b6dd42b6dda8d3708c52f85e7b553977c0bbe324031b41R2299

I do not think a ticket is necessary since CDRIVER-4056 is unreleased and is part of load balancer support.

kevinAlbs · 2021-08-04T17:05:38Z

src/libmongoc/tests/TestSuite.c

@@ -137,6 +137,7 @@ TestSuite_Init (TestSuite *suite, const char *name, int argc, char **argv)
   suite->flags = 0;
   suite->prgname = bson_strdup (argv[0]);
   suite->silent = false;
+   _mongoc_array_init (&suite->match_patterns, sizeof (char *));


Note, now test-libmongoc can be invoked with multiple occurrences of -l <pattern>. This was necessary to invoke with the multiple subset of tests we need to run against a load balanced deployment.

I also added an alias of --match.

kevinAlbs · 2021-08-04T17:09:09Z

src/libmongoc/tests/test-mongoc-change-stream.c

@@ -2774,6 +2774,6 @@ test_change_stream_install (TestSuite *suite)
   TestSuite_AddMockServerTest (
      suite, "/change_streams/prose_test_18", prose_test_18);

-   test_framework_resolve_path (JSON_DIR "/change_streams", resolved);
+   test_framework_resolve_path (JSON_DIR "/change_streams/legacy", resolved);


In resyncing the change_streams tests, there are now two subdirectories (unified and legacy). The unified tests are installed in the unified test runner: runner.c.

kevinAlbs · 2021-08-04T17:10:53Z

src/libmongoc/tests/TestSuite.c

@@ -857,36 +863,6 @@ TestSuite_PrintJsonFooter (FILE *stream) /* IN */
   fflush (stream);
 }

-
-static int


A distinct TestSuite_RunSerial is no longer needed. If there are no match patterns specified TestSuite_RunAll considers anything to match.

kevinAlbs · 2021-08-04T17:19:03Z

src/libmongoc/tests/unified/entity-map.c

@@ -31,6 +31,11 @@
 #define REDUCED_HEARTBEAT_FREQUENCY_MS 500
 #define REDUCED_MIN_HEARTBEAT_FREQUENCY_MS 50

+struct _entity_findcursor_t {


Cursors are lazy in libmongoc, so they do not send the initial command (in this case find) until the first iteration.

This updates the implementation of the createFindCursor test operation to adhere to the specification:

https://github.com/mongodb/specifications/blob/86197086c2290a67500321f008b7a0a0685f0da7/source/unified-test-format/unified-test-format.rst#createfindcursor

Test runners for drivers that lazily execute the find command on the first iteration of the cursor MUST iterate the resulting cursor once.

chardan

This looks pretty good! There are a few things you might want to double-check. Please note that I don't have a great understanding of the test system and it's hard for me to usefully assess what's going on in the long stretches of this, so you may definitely want a second opinion, but they /look/ ok to me. Lots of effort, nice job!

chardan · 2021-08-05T06:11:25Z

build/evergreen_config_lib/tasks.py

+    axes = OD([
+        ('asan', [True]),
+        ('build_ssl', ['openssl']), # The SSL library the C driver is built with.
+        ('test_ssl', [True, False]), # Whether tests are run with SSL connections.


Since there are two Booleans there, does this mean "either true or false" or does one flag signify something else?

@kevinAlbs can confirm, but I think the list of values is used to build the matrix. So we'll end up with LB tests running both with and without SSL.

@jmikola is right. This is to build a matrix to test with SSL and without SSL. This is to satisfy the requirement in the load balancer test readme:

drivers MUST add two Evergreen tasks: one with a sharded cluster with both authentication and TLS enabled and one with a sharded cluster with authentication and TLS disabled.

The _check_allowed function below prevents generating tasks where authentication and TLS do not match.

Ok, got it, thanks.

chardan · 2021-08-05T15:19:34Z

src/libmongoc/src/mongoc/mongoc-server-description-private.h

@@ -200,4 +200,6 @@ bool
 mongoc_server_description_has_service_id (
   const mongoc_server_description_t *description);

+extern bool global_mock_service_id;


nit: This is a good and pretty unique identifier, but /should/ it be "mongoc_" or some such?
question: Any concerns about concurrent writes to this?

Oh good point! I think otherwise a consumer of libmongoc could have conflict with this symbol since it is not static. I added the mongoc_ prefix and noted with a comment the purpose of this variable.

Any concerns about concurrent writes to this?

I do not think so. The test runner only sets this to true once if running against a load balanced cluster. It is never written to again.

chardan · 2021-08-05T15:19:53Z

src/libmongoc/src/mongoc/mongoc-server-description.c

@@ -33,6 +33,8 @@ static bson_oid_t kObjectIdZero = {{0}};

 const bson_oid_t kZeroServiceId = {{0}};

+bool global_mock_service_id = false;


+1 yay, initialization!

chardan · 2021-08-05T15:23:46Z

src/libmongoc/tests/TestSuite.c

         }
-         suite->testname = bson_strdup (argv[++i]);
+         val = bson_strdup (argv[++i]);


Is there a corresponding bson_free()?

note: bson_strdup() can return NULL if passed a NULL parameter, but I don't believe that's possible here.

Is there a corresponding bson_free()?

Yes, match_patterns is an array of char *. So L187 transfers ownership of val to match_patterns. L1018 frees the values in match_patterns.

note: bson_strdup() can return NULL if passed a NULL parameter, but I don't believe that's possible here.

I don't believe so either since argc was checked.

chardan · 2021-08-05T15:26:44Z

src/libmongoc/tests/TestSuite.c

+   /* If no match patterns were provided, then assume all match. */
+   if (suite->match_patterns.len == 0) {
+      matches = true;
+   } else {


nit: What you have here is fine, but the function is a bit simpler when the positive if-case just returns and this "else" is the rest of the function.

@kevinAlbs https://wiki.c2.com/?ElseConsideredSmelly is a good read if you have some spare time

Updated and removed the matches variable. An earlier iteration of these changes supported a --skip and --after flag. I removed them because they were unnecessary, but did not clean up this function.

@kevinAlbs wiki.c2.com/?ElseConsideredSmelly is a good read if you have some spare time

Oh thank you, I have added that to my reading list.

chardan · 2021-08-05T15:40:59Z

src/libmongoc/tests/unified/entity-map.c

-      if (!test_framework_uri_apply_multi_mongos (
-             uri, *use_multiple_mongoses, error)) {
-         goto done;
+      if (*use_multiple_mongoses && test_framework_is_loadbalanced ()) {


A good time to meditate on "use_multiple_mongoses" and the nature of plurality. ;-)

Haha, I am following the specification's lead with useMultipleMongoses. I wouldn't mind useMultipleMongeese.

chardan · 2021-08-05T15:43:36Z

src/libmongoc/tests/unified/entity-map.c

-   return _entity_map_add (em, id, "cursor", (void *) cursor, error);
+   entity_findcursor_t *findcursor;
+
+   findcursor =


Ok, this bason_malloc() abort()s the process on failure.

chardan · 2021-08-05T15:48:25Z

src/libmongoc/tests/unified/runner.c

-      test_error ("error applying multiple mongos: %s", error.message);
+   /* In load balanced mode, the internal client must use the SINGLE_LB_MONGOS_URI. */
+   if (!test_framework_is_loadbalanced ()) {
+      /* Always use multiple mongos's if speaking to a mongos.


My suspicion is that "mongoses" is probably correct (though odd). There must be a definitive answer! ;-)
https://www.merriam-webster.com/words-at-play/what-happens-to-names-when-we-make-them-plural-or-possessive

"mongoses" is also used in specs. Consider the useMultipleMongoses test syntax.

Updating to mongoses.

chardan · 2021-08-05T15:50:33Z

src/libmongoc/tests/unified/runner.c

+          * supported. */
+         ret = true;
+         goto done;
+      } else if (0 == strcmp (event_type, "command")) {


Since we've already left via goto, the extra branching isn't needed.

Updated to remove extra else.

chardan · 2021-08-05T15:51:31Z

src/libmongoc/tests/unified/util.c

+      NULL
+   };
+
+   char ** iter;


Consider declaring this in the loop, rather than at a distance.

C89 prohibits declaring within the loop unfortunately.

No C17 for us.

.evergreen/run-tests.sh

jmikola · 2021-08-06T22:06:09Z

build/evergreen_config_lib/tasks.py

+    axes = OD([
+        ('asan', [True]),
+        ('build_ssl', ['openssl']), # The SSL library the C driver is built with.
+        ('test_ssl', [True, False]), # Whether tests are run with SSL connections.


@kevinAlbs can confirm, but I think the list of values is used to build the matrix. So we'll end up with LB tests running both with and without SSL.

jmikola · 2021-08-06T22:08:03Z

build/evergreen_config_lib/functions.py

@@ -311,6 +311,9 @@
          export MONGOC_TEST_GCP_EMAIL="${client_side_encryption_gcp_email}"
          export MONGOC_TEST_GCP_PRIVATEKEY="${client_side_encryption_gcp_privatekey}"
        fi
+        export LOADBALANCED=${LOADBALANCED}
+        export SINGLE_MONGOS_LB_URI="${SINGLE_MONGOS_LB_URI}"
+        export MULTI_MONGOS_LB_URI="${MULTI_MONGOS_LB_URI}"


Are these values coming from the Evergreen project configuration?

No, these are Evergreen expansions that are created when the Evergreen task is run.

In config.yml, the "start load balancer" function runs the run-load-balancer.sh script from driver-evergreen-tools. It generates these URIs and writes them to a file on this line. The "start load balancer" function then uses the expansions.update command to load these values as Evergreen expansions.

jmikola · 2021-08-06T22:09:09Z

src/libmongoc/src/mongoc/mongoc-cluster.c

@@ -2321,6 +2321,7 @@ _mongoc_cluster_stream_for_server (mongoc_cluster_t *cluster,
         mongoc_server_stream_cleanup (server_stream);
         mongoc_cluster_disconnect_node (cluster, server_id);
         bson_mutex_unlock (&topology->mutex);
+         _mongoc_bson_init_if_set (reply);


I assume no ticket is needed for this since it's fixing a bug from a previous PR that was never released. Correct?

src/libmongoc/src/mongoc/mongoc-server-description.c

jmikola · 2021-08-09T15:20:07Z

src/libmongoc/tests/unified/entity-map.c

@@ -365,6 +392,9 @@ entity_client_new (entity_map_t *em, bson_t *bson, bson_error_t *error)
            mongoc_apm_set_command_failed_cb (callbacks, command_failed);
         } else if (0 == strcmp (event_type, "commandSucceededEvent")) {
            mongoc_apm_set_command_succeeded_cb (callbacks, command_succeeded);
+         } else if (is_unsupported_event_type (event_type)) {
+            MONGOC_DEBUG ("Skipping observing unsupported event type: %s", event_type);


Would it make sense to error here and require the test be explicitly skipped?

I'd argue it still has some value to run the test even if we cannot make assertions on CMAP events.

jmikola · 2021-08-09T15:22:32Z

src/libmongoc/tests/unified/operation.c

+    * and should be skipped for drivers that do not."
+    * TODO: (CDRIVER-3525) add this assertion when CMAP is implemented. */
+   result_from_ok (result);
+   return true;


If the tests should be skipped, should this just raise an error?

We can still run the test and make other assertions. I have a preference for keeping this as-is.

src/libmongoc/tests/unified/runner.c

jmikola · 2021-08-09T15:50:07Z

src/libmongoc/tests/unified/runner.c

+         /* TODO: (CDRIVER-3525) Explicitly ignore cmap events until CMAP is
+          * supported. */
+         ret = true;
+         goto done;


This may overlap with my previous question but would it be preferable to error here and require skipping the tests?

I have a preference for skipping the CMAP assertions and still running the test. They won't be testing as much, but it seems better to run what we can rather than skip the full test.

src/libmongoc/tests/unified/util.c

chardan

With cleanups, +1 from me! Nice work on this.

mock service ID with a global add test_framework_is_loadbalanced

…d3e2

- ignore cmap events in observeEvents - ignore cmap events in expectEvents

Add a no-op for assertNumberConnectionsCheckedOut Do not consider changestream / cursor iteration error a fatal test error.

- update handling of useMultipleMongoses in UTF - use mongoc_ prefix in mock global - clang format - clean up if/else branching

kevinAlbs force-pushed the evergreen.loadbalancer branch 2 times, most recently from f76b6ce to 95408ed Compare August 4, 2021 15:14

kevinAlbs changed the title ~~[WIP] Test load balancer on evg~~ CDRIVER-4062 Test load balancer on evg Aug 4, 2021

kevinAlbs changed the title ~~CDRIVER-4062 Test load balancer on evg~~ CDRIVER-4062 test load balancer on evg Aug 4, 2021

kevinAlbs commented Aug 4, 2021

View reviewed changes

kevinAlbs marked this pull request as ready for review August 4, 2021 17:21

kevinAlbs requested review from benjirewis and chardan August 4, 2021 18:15

chardan reviewed Aug 5, 2021

View reviewed changes

kevinAlbs requested review from jmikola and removed request for benjirewis August 6, 2021 00:45

jmikola reviewed Aug 9, 2021

View reviewed changes

kevinAlbs requested review from chardan and jmikola August 10, 2021 21:58

chardan approved these changes Aug 10, 2021

View reviewed changes

jmikola approved these changes Aug 11, 2021

View reviewed changes

kevinAlbs added 14 commits August 11, 2021 11:05

use SINGLE_MONGOS_LB_URI as test_framework_get_uri

a6820cc

mock service ID with a global add test_framework_is_loadbalanced

resync retryable reads tests

1e27b95

add load-balanced to runOn check

ab80362

resync retryable writes tests to d2de77692b727b805168eeba02c3ab29e6a1…

783710c

…d3e2

RISK: run retryable writes spec tests on all topologies

47e3f7c

support useMultipleMongos on LB in retryable writes

33aa413

remove out-dated retryableErrors test, update skip

557184f

resync change stream tests; add unified test (CDRIVER-3889)

eaaf361

apply SINGLE/MULTI_LB_MONGOS_URI to unified test runner

72d0065

sync new load balancer unified tests

9a8ce2d

add service_id to captured events; ignore cmap evts

57e52e1

- ignore cmap events in observeEvents - ignore cmap events in expectEvents

add example of unsupported batchSize on listIndexes / listCollections

7d000b0

Add a findcursor entity for lazy cursor handling.

f448895

Add a no-op for assertNumberConnectionsCheckedOut Do not consider changestream / cursor iteration error a fatal test error.

skip waitQueueTimeoutMS test; add appname URI option

9fdd109

kevinAlbs added 17 commits August 11, 2021 11:05

initialize bson reply

4171f56

allow NULL for reply

326acae

support --skip, --after, and multiple -l in test-libmongoc

9411cc4

add --match as an alias

39b093a

add WIP ASAN test tasks

1b8c27e

fix test-libmongoc path

a651033

use test framwork to construct client for /loadbalanced/ tests

c4d1f4c

add 5.0 task

b303183

generate evg config

d0ecd1f

remove --after and --skip

535befa

removed unused function

7031fb0

remove AUTHSOURCE: thisDB, remove logs

3dc5887

fix pre-opmsg command construction for auth

143b21a

update run-tests.sh

86582a7

add comment

17789c4

remove redundant test; format

2a0922c

jesse + jeremy comments

808424c

- update handling of useMultipleMongoses in UTF - use mongoc_ prefix in mock global - clang format - clean up if/else branching

kevinAlbs force-pushed the evergreen.loadbalancer branch from 9b28636 to 808424c Compare August 11, 2021 15:05

kevinAlbs merged commit 932e7e4 into mongodb:master Aug 12, 2021

kevinAlbs deleted the evergreen.loadbalancer branch August 12, 2021 00:39

kevinAlbs restored the evergreen.loadbalancer branch February 3, 2022 14:29

		@@ -51,11 +51,11 @@ get_mongodb_download_url_for ()
		_DISTRO=$1
		_VERSION=$2

		@@ -33,6 +33,8 @@ static bson_oid_t kObjectIdZero = {{0}};

		const bson_oid_t kZeroServiceId = {{0}};

		bool global_mock_service_id = false;

CDRIVER-4062 test load balancer on evg #837

CDRIVER-4062 test load balancer on evg #837

Uh oh!

Conversation

kevinAlbs commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chardan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

kevinAlbs commented Jul 30, 2021 •

edited

Loading