DRIVERS-1603 Unified test format updates for serverless testing #944

patrickfreed · 2021-04-07T23:51:06Z

This PR adds a testing-only spec which requires that drivers run a certain set of tests against a live Atlas serverless instance. As part of this, the unified test format was updated to include a Serverless-specific runOnRequirement, and some tests were updated to use it as necessary.

Example patch: https://evergreen.mongodb.com/version/606e3ba332f4171c4c8b406e

source/crud/tests/README.rst

patrickfreed · 2021-04-07T23:52:52Z

source/crud/tests/unified/aggregate.yml

@@ -0,0 +1,79 @@
+description: "aggregate"


This and find.yml ensure that cursors work even with multiple batches (test required by scope).

patrickfreed · 2021-04-07T23:54:04Z

source/crud/tests/unified/estimatedDocumentCount.yml

@@ -34,6 +34,7 @@ tests:
  - description: "estimatedDocumentCount uses $collStats on 4.9.0 or greater"
    runOnRequirements:
      - minServerVersion: "4.9.0"
+        serverlessMode: forbidServerless


Ironically, serverless does not support counting via $collStats, so these need to be skipped until that has been fixed (CLOUDP-86318)

patrickfreed · 2021-04-07T23:54:25Z

source/crud/tests/v2/db-aggregate.yml

@@ -1,6 +1,7 @@
 runOn:
    -
        minServerVersion: "3.6.0"
+        serverlessMode: "forbidServerless"


$listLocalSessions is not supported in serverless.

patrickfreed · 2021-04-07T23:55:14Z

source/serverless-testing/tests/README.rst

+enabled. Additionally, Serverless requires the usage of the versioned API, so
+the tests MUST be run with a server API version specified.
+
+Required Variables


I'm wondering what would be the best way to store the values of these for drivers to refer to. The atlas connectivity ones are in a google doc, so maybe we should go that route too.

I don't think we have any great options for sharing secrets only to the DBX team. So a google doc seems OK.

@agolin95: this sounds like another thing that could ultimately benefit from some structure/process

Agreed @jmikola thank you for the tag! Taking a reminder to investigate what kind of secret sharing platform options we have available to us.

source/serverless-testing/tests/README.rst

patrickfreed · 2021-04-07T23:56:21Z

source/serverless-testing/tests/README.rst

+   ``find`` and a single ``getMore`` (e.g. specify ``batchSize=1`` for a query
+   that would match 3+ documents).
+
+Other Tests


Required by the "existing tests using cursors" portion of the spec.

alcaeus · 2021-04-08T07:36:51Z

source/crud/tests/README.rst

@@ -61,6 +61,14 @@ Each YAML file has the following keys:
    and "sharded". If this field is omitted, the default is all topologies (i.e.
    ``["single", "replicaset", "sharded"]``).

+  - ``serverlessMode``: (optional) Whether or not the test should be run on


May I humbly suggest serverless: "allow|require|forbid"? I feel it's less verbose and less confusing (serverlessMode makes it sound like it's about the configuration of the serverless proxy, not whether we're testing against it)

Sounds good, updated.

alcaeus · 2021-04-08T07:44:35Z

source/unified-test-format/unified-test-format.rst

@@ -3,13 +3,13 @@ Unified Test Format
 ===================

 :Spec Title: Unified Test Format
-:Spec Version: 1.2.3
+:Spec Version: 1.3.0


Please remember to add a changelog entry for this change.

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

source/crud/tests/unified/aggregate.yml

+createEntities:
+  - client:
+      id: &client0 client0
+      useMultipleMongoses: true # ensure cursors pin to a single server


Oh good thinking!

kevinAlbs · 2021-04-08T17:54:01Z

source/crud/tests/unified/aggregate.yml

+  - database:
+      id: &database0 database0
+      client: *client0
+      databaseName: &database0Name edc-tests


Suggested change

databaseName: &database0Name edc-tests

databaseName: &database0Name aggregate-tests

Updated. (didn't use the "commit suggestion" to group the JSON changes in the same commit).

kevinAlbs · 2021-04-08T17:58:52Z

source/crud/tests/unified/find.yml

+  - database:
+      id: &database0 database0
+      client: *client0
+      databaseName: &database0Name edc-tests


Suggested change

databaseName: &database0Name edc-tests

databaseName: &database0Name find-tests

kevinAlbs · 2021-04-08T18:02:24Z

source/crud/tests/unified/estimatedDocumentCount.yml

@@ -34,6 +34,7 @@ tests:
  - description: "estimatedDocumentCount uses $collStats on 4.9.0 or greater"
    runOnRequirements:
      - minServerVersion: "4.9.0"
+        serverless: forbid


Why is this forbidden? I think the new aggregation pipeline for estimatedDocumentCount would work against serverless.

As discussed a little bit in slack, the proxy doesn't yet support count via $collStats yet (https://jira.mongodb.org/browse/CLOUDP-86318)

Is that intended to be addressed before public release?

Either way, I think this warrants a comment (as you did for db.aggregate) and maybe a reference to the issue number so we can come back to this later.

kevinAlbs · 2021-04-08T18:03:03Z

source/crud/tests/v2/db-aggregate.yml

@@ -1,6 +1,7 @@
 runOn:
    -
        minServerVersion: "3.6.0"
+        serverless: "forbid"


Can you note in a comment saying that serverless does not support $listLocalSessions?

kevinAlbs · 2021-04-08T18:04:51Z

source/serverless-testing/tests/README.rst

+
+All tests MUST be run with wire protocol compression and authentication
+enabled. Additionally, Serverless requires the usage of the versioned API, so
+the tests MUST be run with a server API version specified.


Ah I see, I was going to ask why client entities did not have a serverApi field. I think I prefer having this noted in the readme. Otherwise, every test we run against serverless would need to be modified to have a serverApi.

kevinAlbs · 2021-04-08T18:12:06Z

source/serverless-testing/tests/README.rst

+enabled. Additionally, Serverless requires the usage of the versioned API, so
+the tests MUST be run with a server API version specified.
+
+Required Variables


I don't think we have any great options for sharing secrets only to the DBX team. So a google doc seems OK.

kevinAlbs · 2021-04-08T18:13:53Z

source/unified-test-format/tests/valid-pass/poc-crud.yml

@@ -170,6 +170,7 @@ tests:
  - description: "Aggregate with $listLocalSessions"
    runOnRequirements:
      - minServerVersion: "3.6.0"
+        serverless: forbid


Also here, can you add a comment noting $listLocalSessions is prohibited in serverless.

kevinAlbs · 2021-04-08T18:16:40Z

source/versioned-api/tests/crud-api-version-1-strict.yml

@@ -62,6 +62,8 @@ tests:
                <<: *expectedApiVersion

  - description: "aggregate on database appends declared API version"
+    runOnRequirements:


Could this test use the $currentOp stage instead of bypassing on serverless?

Edit: nevermind, $currentOp appears also forbidden.

MongoDB Enterprise mongos> db.aggregate([{$currentOp: 1}]) uncaught exception: Error: command failed: { "ok" : 0, "errmsg" : "$currentOp is not allowed in this atlas tier", "code" : 8000, "codeName" : "AtlasError" } with original command request: { "aggregate" : 1, "pipeline" : [ { "$currentOp" : 1 } ], "cursor" : { }, "lsid" : { "id" : UUID("2318af3d-5565-4a71-9a68-63d9081939eb") }, "$clusterTime" : { "clusterTime" : Timestamp(1617905922, 1), "signature" : { "hash" : BinData(0,"yWzyYH2VLZix/Wj3qp3mmupzm2Q="), "keyId" : NumberLong("6945905481428762627") } } }

Yep, I don't think we can test db-aggregate on serverless for the time being.

jmikola · 2021-04-08T20:01:42Z

source/crud/tests/v2/db-aggregate.yml

@@ -1,6 +1,7 @@
 runOn:
    -
        minServerVersion: "3.6.0"
+        serverless: "forbid"


Presumably this is the only reason you needed to modify the CRUD v2 test format. Is there no other alternative we can use to run db.aggregate() on serverless, or are all of its pipeline stages unsupported?

I'll note that this test was changed to use $listLocalSessions instead of $currentOp in #565 (comment).

This relates to @kevinAlbs's comment in https://github.com/mongodb/specifications/pull/944/files#r609975158. As a side question, I'm curious if these restrictions only have to do with "serverless" or more generally might apply to other parts of Atlas (e.g. free tier proxy). AFAIK, they are separate things, but correct me if I'm wrong.

I think the only options for db-level aggregate are $listLocalSessions and $currentOp, both of which are unsupported by the proxy.

And yeah, I believe whatever restrictions apply to the serverless proxy probably also apply to the free-tier proxy, since I think the former is a descendant of / shares a lot of code with the latter, not sure about how tightly coupled they are though.

jmikola · 2021-04-08T20:03:52Z

source/serverless-testing/tests/README.rst

+enabled. Additionally, Serverless requires the usage of the versioned API, so
+the tests MUST be run with a server API version specified.
+
+Required Variables


@agolin95: this sounds like another thing that could ultimately benefit from some structure/process

source/unified-test-format/unified-test-format.rst

jmikola · 2021-04-08T20:17:43Z

source/unified-test-format/unified-test-format.rst

@@ -3,13 +3,13 @@ Unified Test Format
 ===================

 :Spec Title: Unified Test Format
-:Spec Version: 1.2.3
+:Spec Version: 1.3.0


FYI: this and the corresponding schema version are going to conflict #935. Please reach out to @divjotarora and decide if you should use 1.4.0 here or vice versa.

I'll note that the schema versioning doesn't force drivers to implement the projects in a particular order. IIRC, I was working on Atlas testing (1.2 with loop, etc.) in PHPLIB before we had implemented versioned API (1.1).

Good point. I think whichever one is ready to merge first ought to take the 1.3 and the other can do 1.4. I'll check with him before merging anything to ensure we're on the same page.

jmikola · 2021-04-09T18:48:57Z

source/crud/tests/README.rst

+  - ``serverless``: (optional) Whether or not the test should be run on
+    serverless instances imitating sharded clusters. Valid values are "require",
+    "forbid", and "allow". If "require", the test MUST only be run on serverless
+    instances. If "forbid", the test MUST NOT be run on Serverless instances. If


"Serverless" is still capitalized here.

jmikola · 2021-04-09T18:50:07Z

source/crud/tests/v2/db-aggregate.yml

@@ -1,6 +1,7 @@
 runOn:
    -
        minServerVersion: "3.6.0"
+        serverless: "forbid" # serverless does not support $listLocalSessions or $currentOp


$currentOp isn't used in this file. If you keep this (no harm), perhaps you want to ensure poc-crud.yml has the same comment for consistency.

I included both in case any future readers might wonder why we don't just use $currentOp, updated both this and poc-crud.yml to use the same comment that should be a bit clearer.

jmikola · 2021-04-09T18:52:17Z

source/crud/tests/unified/estimatedDocumentCount.yml

@@ -34,6 +34,7 @@ tests:
  - description: "estimatedDocumentCount uses $collStats on 4.9.0 or greater"
    runOnRequirements:
      - minServerVersion: "4.9.0"
+        serverless: forbid


Is that intended to be addressed before public release?

Either way, I think this warrants a comment (as you did for db.aggregate) and maybe a reference to the issue number so we can come back to this later.

jmikola · 2021-04-09T19:18:55Z

source/unified-test-format/unified-test-format.rst

+  serverless instances imitating sharded clusters. Valid values are "require",
+  "forbid", and "allow". If "require", the test MUST only be run on serverless
+  instances. If "forbid", the test MUST NOT be run on Serverless instances. If
+  omitted or "allow", this option has no effect.


Can you confirm that serverless is entirely distinct from load balancers, in that a load balancer can front mongos or serverless instances, but drivers can also connect directly to serverless without a load balancer?

I just want to make sure I'm understanding both projects correctly before we codify any runOnRequirement changes.

For context, let me quote @patrickfreed's previous comment (now hidden) from this thread:

And yeah, I believe whatever restrictions apply to the serverless proxy probably also apply to the free-tier proxy, since I think the former is a descendant of / shares a lot of code with the latter, not sure about how tightly coupled they are though.

In #935, @divjotarora explained that drivers cannot infer what is behind a load balancer. This came up with respect with useMultipleMongoses. The solution there was to feed test runners two additional connection strings (one for an LB fronting a single mongos, and another for a LB fronting multiple mongoses), which can then be used as needed based on whether useMultipleMongoses is true or false. While this works in practice, it does invert the typical behavior for useMultipleMongoses in that drivers simply decide which connection string to use instead of inspecting the topology and possibly raising an error.

If serverless presents itself as mongos and cannot be differentiated, how is the test runner supposed to enforce the serverless option? I assume the test runner (either unified or legacy) will need to consult some explicit indicator (e.g. environment variable, configuration option) and decide based on that. If so, I think we need to clarify that. The test runners themselves need not be prescriptive beyond saying that it must be possible to inform them whether or not serverless is being used. The serverless testing doc itself can advise on a particular method for doing so.

Whatever mechanism is suggested will need to play nice with #935 in the event we ever test load balancers fronting serverless. #935 initially assumes that drivers will only test load balancers with mongos, but I think we should consider how the serverless option will interact should we ever add load balancers fronting serverless to the test matrix. Expecting serverless usage to be communicated explicitly (e.g. environment variable or otherwise configuring the test runner) seems like a good way to avoid potential conflict there.

On a separate note, if we expect that Atlas free tier has the same restrictions as serverless (at least with respect to db.aggregate, which is the main reason serverless is being introduced as a runOnRequirement), should we amend the definition to cover both environments? Or perhaps no language changes are needed in this PR and we can just consider having Atlas free tier testing make use of the same indicator (e.g. environment variable) when the time comes.

I'll confess I'm not aware of how we test on free tier today beyond the basic connectivity tests. DRIVERS-362 and DRIVERS-382 are quite old and I don't believe we have an actual spec or design for free tier testing.

By the time Serverless is GA, the only way to interact with it will be via a load balancer.

As per slack discussion, it is expected that drivers use something like an environment variable to determine if serverless is being tested, regardless of whether or not there's a load balancer in front of it or not. I've updated the specs to indicate as much.

Regarding the free tier testing, I don't think we're planning on introducing any new tests for it, so I'm not sure how much of a concern it is. Furthermore, we may want to have separate runOnRequirements for the free tier and serverless anyways even if we do add such tests.

patrickfreed · 2021-04-19T18:03:20Z

Continued in #964 (couldn't re-open this since a force-push occurred while it was closed)

patrickfreed commented Apr 7, 2021

View reviewed changes

patrickfreed marked this pull request as ready for review April 7, 2021 23:57

patrickfreed requested a review from jmikola as a code owner April 7, 2021 23:57

patrickfreed requested a review from kevinAlbs April 7, 2021 23:57

alcaeus reviewed Apr 8, 2021

View reviewed changes

kevinAlbs reviewed Apr 8, 2021

View reviewed changes

jmikola reviewed Apr 8, 2021

View reviewed changes

patrickfreed added 17 commits April 9, 2021 13:45

serverless testing

36e48f9

skip estimatedDocumentCount tests on serverless

a3e4698

remove sharded only restriction on serverlessMode

b2c9194

add warning about xtrace

900c28e

dont run sessions or versioned api tests

a39017a

fix formatting?

cf13681

fix formatting try 2

24275fd

bump the required schema version of affected tests

de4a488

clarify which variables are private

99f6351

remove cursor prose tests

18a3ac3

serverlessMode -> serverless

63c4ff2

update changelog

2a76074

update database names in new unified crud tests

6341d0c

add note to db-aggregate skip

3e0b486

add note to poc-crud unified test skip

fa0be10

lowercase serverless

fc7fc7a

update forbid wording

a0dfb5e

patrickfreed force-pushed the DRIVERS-1603/serverless-atlas-testing branch from b1703d3 to a0dfb5e Compare April 9, 2021 17:46

jmikola reviewed Apr 9, 2021

View reviewed changes

patrickfreed closed this Apr 9, 2021

jmikola mentioned this pull request Apr 14, 2021

DRIVERS-555 Unified test format changes for CSOT #959

Merged

patrickfreed changed the title ~~DRIVERS-1603 Serverless drivers testing spec~~ DRIVERS-1603 Unified test format updates for serverless testing Apr 19, 2021

	databaseName: &database0Name edc-tests
	databaseName: &database0Name aggregate-tests

	databaseName: &database0Name edc-tests
	databaseName: &database0Name find-tests

DRIVERS-1603 Unified test format updates for serverless testing #944

DRIVERS-1603 Unified test format updates for serverless testing #944

Uh oh!

Conversation

patrickfreed commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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

patrickfreed commented Apr 7, 2021 •

edited

Loading

jmikola Apr 9, 2021 •

edited

Loading