DRIVERS-828 comprehensive Atlas testing #98

p-mongo · 2021-01-05T22:55:15Z

https://jira.mongodb.com/browse/DRIVERS-828

alcaeus

Nit about debug code being left in. I'll leave the Python code review to someone that knows Python, but spec changes LGTM.

astrolabe/runner.py

prashantmital

I haven't had a chance to go through all of the changes yet. Publishing my initial comments so we can start iterating.

.evergreen/config.yml

astrolabe/cli.py

astrolabe/runner.py

prashantmital · 2021-01-20T21:26:59Z

astrolabe/runner.py

+                deadline = monotonic() + 90
+
+                while True:
+                    mc = MongoClient(cluster_config['connectionStrings']['standard'], username='atlasuser', password='mypassword123')


Why does this need a new MongoClient? Why can we not use one of the clients attached to the context for this operation?

I see two other clients in the codebase but neither of them appears to be attached to any contexts, which client did you have in mind exactly?

atlasclient/client.py

astrolabe/runner.py

atlasclient/client.py

astrolabe/runner.py

* master: PYTHON-2579 Fix Atlas planned maintenance test runner

jmikola

Reviewed the RST files as I don't have enough context for the other components of this PR.

I have two questions on the documentation for events. If no changes are required (based on your response), you can consider me a LGTM.

docs/source/spec-workload-executor.rst

prashantmital

You can disregard the Python style nits for this PR as that shouldn't block other drivers from implementing this spec. However, there seem to be several other questions that need to be addressed before this can be merged.

prashantmital · 2021-02-16T23:07:25Z

.evergreen/config.yml

@@ -78,11 +83,13 @@ functions:
          CLUSTER_NAME_SALT: ${build_id}
          ATLAS_API_USERNAME: ${atlas_key}
          ATLAS_API_PASSWORD: ${atlas_secret}
-          ATLAS_API_BASE_URL: ${atlas_url}


Is there a reason you are hardcoding this? We should prefer configuring this value in the corresponding Evergreen variable (via the UI) instead of committing it here.

I also noticied that we are currently using https://cloud-dev.mongodb.com/api/atlas - has the API base URL changed?

With the private endpoints the last path segment had to be removed, since I didn't have admin access I set the override in the PR. I can edit the project but this would break existing builds, therefore I suggest making this change in project config when the PR is ready to be merged.

Ok. In that case can you revert this change? We can update the atlas_url evergreen variable before merging this.

prashantmital · 2021-02-16T23:08:08Z

.evergreen/config.yml

@@ -30,6 +30,11 @@ functions:
        working_dir: astrolabe-src
        command: |
          ${PYTHON3_BINARY} -m pip install virtualenv
+    - command: subprocess.exec


Is this to get debugging information for the DNS issue? If so, can we do this in a separate PR?

@p-mongo what about this?

astrolabe/cli.py

prashantmital · 2021-02-17T01:57:37Z

astrolabe/runner.py

+                self.wait_for_idle()
+
+            if op_name == 'sleep':
+                _time.sleep(op_spec)


Nit: suggest casting op_spec to a float before passing it to sleep to support durations defined as string x.y.

_time.sleep(float(op_spec))

prashantmital · 2021-02-17T01:57:48Z

astrolabe/runner.py

+
+            if op_name == 'restartVms':
+                rv = self.admin_client.nds.groups[self.project.id].clusters[self.cluster_name].reboot.post(api_version='private')
+


Nit: remove newline.

prashantmital · 2021-02-17T02:01:53Z

astrolabe/runner.py

+                    self.cluster_url.processArgs.patch(**process_args)
+
+                # Step-4: wait until maintenance completes (cluster is IDLE).
+                self.wait_for_idle()


I think you need to sleep for a couple of seconds before calling waitForIdle otherwise there could be situations in which you end up seeing the IDLE server state prematurely because maintenance has not yet been kicked off by the automation agent.

I reported this for failover tests in https://jira.mongodb.org/browse/PRODTRIAGE-1232 (and the users are currently meant to add the sleeps in their scenarios to compensate), are you saying other actions are also affected?

Yes. AFAIK none of the operations on clusters are applied synchronously. Operations are put in a task queue that is consumed by the automation agent. Consequently, this could cause strange failures even for other maintenance scenarios like cluster resizing. Adding a 2-3 second wait before waiting for idle should work.

prashantmital · 2021-02-17T02:07:36Z

astrolabe/runner.py

-            self.__connection_string = connection_string
+            uri = re.sub(r'://',
+                '://%s:%s@' % (self.config.database_username, self.config.database_password),
+                cluster.srvAddress)


Since the workload executors cannot add URI options to the connection string, don't we need to add them to the connection string being generated here? This was previously being done (rather simplistically) using

connection_string += urlencode(uri_options)

but now we don't seem to be doing it at all. Can you elaborate on how workload executors are expected to respect URI options if they cannot augment the incoming connection string and if astrolabe doesn't augment the outgoing connection string?

prashantmital · 2021-02-17T02:09:10Z

docs/source/spec-workload-executor.rst

-   Note that the workload executor:
+#. MUST use the input connection string to `instantiate the
+   unified test runner <https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#id92>`_
+   of the driver being tested. Note that the workload executor:

   * MUST NOT override any of the URI options specified in the incoming connection string.
   * MUST NOT augment the incoming connection string with any additional URI options.


Is this still true? Please see my question above about how we expect to honor URI options.

jmikola

One comment about RST formatting, but content LGTM otherwise.

docs/source/spec-workload-executor.rst

prashantmital · 2021-02-17T19:18:28Z

tests/retryWrites-toggleServerSideJS.yml

+    - client:
+        id: &client0 client0
+        uriOptions:
+          retryWrites: true


How are the uriOptions actually going to be honored? Astrolabe doesn't seem to use this field and we prohibit the workload executor from using it.

This field is read by the unified test runner when it is instantiating clients.

Ok, in that case we need to update the language in docs/source/spec-workload-executor.rst. Currently, that document states:

Behavioral Description ---------------------- After accepting the inputs, the workload executor: #. MUST use the input connection string to `instantiate the unified test runner <https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#id92>`_ of the driver being tested. Note that the workload executor: * MUST NOT override any of the URI options specified in the incoming connection string. * MUST NOT augment the incoming connection string with any additional URI options.

If we want the uriOptions to be honored, we need to update this language?

I don't think the language you quoted conflicts. It's saying that the workload executor must pass the connection string it receives without modification to the unified test runner. AFAIK, the workload executor isn't connecting to MongoDB directly, so the only client objects using this connection string will be those created within the unified test runner. IMO, this has no bearing on the unified test runner spec's instructions that a client entity's uriOptions should be respected.

Perhaps a clarification of this interaction would help avoid any misunderstanding.

p-mongo changed the title ~~-828 validate failure handling~~ DRIVERS-828 comprehensive Atlas testing Jan 6, 2021

alcaeus reviewed Jan 13, 2021

View reviewed changes

astrolabe/runner.py Outdated Show resolved Hide resolved

prashantmital suggested changes Jan 20, 2021

View reviewed changes

prashantmital suggested changes Jan 21, 2021

View reviewed changes

astrolabe/runner.py Outdated Show resolved Hide resolved

atlasclient/client.py Outdated Show resolved Hide resolved

astrolabe/runner.py Outdated Show resolved Hide resolved

p added 26 commits January 21, 2021 16:42

Change schema to support multiple operations

2e8bbc1

use driver 2.13

46c1627

operations

cac3bb8

metrics collection

64a0496

test failover implementation

0fe25ee

test failover test

8560798

priority takeover

75144cf

comparison diagnostics

4a409fb

atlas descriptive details

a0091d1

vm restart

bf97152

process restart

c2ae81c

fix subset checks

257c4c2

assert region

808b853

assert region in primary takeover test

a29c304

process restart via tls args

64f4016

primary removal

20adcb3

primary takeover increase timeout

96f8caf

dev atlas

ee20807

vm restart

c24f583

reformat

b151269

fix process restart rs

f8ea7e1

process restart sharded

4154a61

sharded tests

0cdd9da

report how long polling was done for

7956354

increase polling interval for sharded clusters

f647ae9

vm move tests

c39861f

p added 12 commits February 11, 2021 09:27

fix event name

83285fe

fix dependency

7c2d0aa

load yaml safely

da1448f

update validator

32caa8a

fix error and failure count reporting

2923250

fix failure validator

96778b1

add polling diagnostics

18a4ff5

give workload executors 10 more seconds of runtime

f4f5150

fix write scenarios

01a68db

temp

5583764

Merge branch 'master' into 828

849fa47

* master: PYTHON-2579 Fix Atlas planned maintenance test runner

reinstate command monitoring

1f7529a

jmikola reviewed Feb 15, 2021

View reviewed changes

docs/source/spec-workload-executor.rst Outdated Show resolved Hide resolved

docs/source/spec-workload-executor.rst Outdated Show resolved Hide resolved

command events were reinstated

afa7f0c

prashantmital suggested changes Feb 17, 2021

View reviewed changes

jmikola approved these changes Feb 17, 2021

View reviewed changes

docs/source/spec-workload-executor.rst Show resolved Hide resolved

prashantmital reviewed Feb 17, 2021

View reviewed changes

p added 9 commits February 17, 2021 18:30

fix space

ed4bafa

comments

9504d1b

unrecognized operation

9f417ec

use rstrip

f8271f9

comment

a71d6f4

logs destination

5b03146

delete obsolete

887a336

note wait for idle

28ea6af

note wait for idle

244f00b

prashantmital approved these changes Feb 18, 2021

View reviewed changes

syntax fix

c25c664

p-mongo merged commit 6e3c4a4 into mongodb-labs:master Feb 18, 2021

prashantmital mentioned this pull request Mar 25, 2021

Use an evergreen variable to set the Atlas API base URL #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRIVERS-828 comprehensive Atlas testing #98

DRIVERS-828 comprehensive Atlas testing #98

p-mongo commented Jan 5, 2021 •

edited

Loading

alcaeus left a comment

prashantmital left a comment

prashantmital Jan 20, 2021

p-mongo Jan 22, 2021

jmikola left a comment

prashantmital left a comment

prashantmital Feb 16, 2021

prashantmital Feb 16, 2021

p-mongo Feb 17, 2021

prashantmital Feb 17, 2021

prashantmital Feb 16, 2021

prashantmital Feb 17, 2021

prashantmital Feb 17, 2021

prashantmital Feb 17, 2021

prashantmital Feb 17, 2021

p-mongo Feb 17, 2021

prashantmital Feb 17, 2021

prashantmital Feb 17, 2021

prashantmital Feb 17, 2021

jmikola left a comment •

edited

Loading

prashantmital Feb 17, 2021

p-mongo Feb 17, 2021

prashantmital Feb 17, 2021

jmikola Feb 18, 2021


		if op_name == 'restartVms':
		rv = self.admin_client.nds.groups[self.project.id].clusters[self.cluster_name].reboot.post(api_version='private')

DRIVERS-828 comprehensive Atlas testing #98

DRIVERS-828 comprehensive Atlas testing #98

Conversation

p-mongo commented Jan 5, 2021 • edited Loading

alcaeus left a comment

Choose a reason for hiding this comment

prashantmital left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

prashantmital 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

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

jmikola left a comment • 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

p-mongo commented Jan 5, 2021 •

edited

Loading

jmikola left a comment •

edited

Loading